Commit graph

617 commits

Author SHA1 Message Date
Junio C Hamano b4f0038525 Merge branch 'en/rebase-backend'
Band-aid fixes for two fallouts from switching the default "rebase"
backend.

* en/rebase-backend:
  git-rebase.txt: highlight backend differences with commit rewording
  sequencer: clear state upon dropping a become-empty commit
  i18n: unmark a message in rebase.c
2020-03-12 14:28:01 -07:00
Elijah Newren 9a1b7474d6 sequencer: clear state upon dropping a become-empty commit
In commit e98c4269c8 ("rebase (interactive-backend): fix handling of
commits that become empty", 2020-02-15), the merge backend was changed
to drop commits that did not start empty but became so after being
applied (because their changes were a subset of what was already
upstream).  This new code path did not need to go through the process of
creating a commit, since we were dropping the commit instead.
Unfortunately, this also means we bypassed the clearing of the
CHERRY_PICK_HEAD and MERGE_MSG files, which if there were no further
commits to cherry-pick would mean that the rebase would end but assume
there was still an operation in progress.  Ensure that we clear such
state files when we decide to drop the commit.

Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-03-11 12:11:05 -07:00
Junio C Hamano 8c22bd9ff9 Merge branch 'en/rebase-backend'
"git rebase" has learned to use the merge backend (i.e. the
machinery that drives "rebase -i") by default, while allowing
"--apply" option to use the "apply" backend (e.g. the moral
equivalent of "format-patch piped to am").  The rebase.backend
configuration variable can be set to customize.

* en/rebase-backend:
  rebase: rename the two primary rebase backends
  rebase: change the default backend from "am" to "merge"
  rebase: make the backend configurable via config setting
  rebase tests: repeat some tests using the merge backend instead of am
  rebase tests: mark tests specific to the am-backend with --am
  rebase: drop '-i' from the reflog for interactive-based rebases
  git-prompt: change the prompt for interactive-based rebases
  rebase: add an --am option
  rebase: move incompatibility checks between backend options a bit earlier
  git-rebase.txt: add more details about behavioral differences of backends
  rebase: allow more types of rebases to fast-forward
  t3432: make these tests work with either am or merge backends
  rebase: fix handling of restrict_revision
  rebase: make sure to pass along the quiet flag to the sequencer
  rebase, sequencer: remove the broken GIT_QUIET handling
  t3406: simplify an already simple test
  rebase (interactive-backend): fix handling of commits that become empty
  rebase (interactive-backend): make --keep-empty the default
  t3404: directly test the behavior of interest
  git-rebase.txt: update description of --allow-empty-message
2020-03-02 15:07:19 -08:00
Elijah Newren c2417d3af7 rebase: drop '-i' from the reflog for interactive-based rebases
A large variety of rebase types are supported by the interactive
machinery, not just the explicitly interactive ones.  These all share
the same code and write the same reflog messages, but the "-i" moniker
in those messages doesn't really have much meaning.  It also becomes
somewhat distracting once we switch the default from the am-backend to
the interactive one.  Just remove the "-i" from these messages.

Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-16 15:40:42 -08:00
Elijah Newren 8a997ed132 rebase, sequencer: remove the broken GIT_QUIET handling
The GIT_QUIET environment variable was used to signal the non-am
backends that the rebase should perform quietly.  The preserve-merges
backend does not make use of the quiet flag anywhere (other than to
write out its state whenever it writes state), and this mechanism was
broken in the conversion from shell to C.  Since this environment
variable was specifically designed for scripts and the only backend that
would still use it is no longer a script, just gut this code.

A subsequent commit will fix --quiet for the interactive/merge backend
in a different way.

Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-16 15:40:42 -08:00
Elijah Newren e98c4269c8 rebase (interactive-backend): fix handling of commits that become empty
As established in the previous commit and commit b00bf1c9a8
(git-rebase: make --allow-empty-message the default, 2018-06-27), the
behavior for rebase with different backends in various edge or corner
cases is often more happenstance than design.  This commit addresses
another such corner case: commits which "become empty".

A careful reader may note that there are two types of commits which would
become empty due to a rebase:

  * [clean cherry-pick] Commits which are clean cherry-picks of upstream
    commits, as determined by `git log --cherry-mark ...`.  Re-applying
    these commits would result in an empty set of changes and a
    duplicative commit message; i.e. these are commits that have
    "already been applied" upstream.

  * [become empty] Commits which are not empty to start, are not clean
    cherry-picks of upstream commits, but which still become empty after
    being rebased.  This happens e.g. when a commit has changes which
    are a strict subset of the changes in an upstream commit, or when
    the changes of a commit can be found spread across or among several
    upstream commits.

Clearly, in both cases the changes in the commit in question are found
upstream already, but the commit message may not be in the latter case.

When cherry-mark can determine a commit is already upstream, then
because of how cherry-mark works this means the upstream commit message
was about the *exact* same set of changes.  Thus, the commit messages
can be assumed to be fully interchangeable (and are in fact likely to be
completely identical).  As such, the clean cherry-pick case represents a
case when there is no information to be gained by keeping the extra
commit around.  All rebase types have always dropped these commits, and
no one to my knowledge has ever requested that we do otherwise.

For many of the become empty cases (and likely even most), we will also
be able to drop the commit without loss of information -- but this isn't
quite always the case.  Since these commits represent cases that were
not clean cherry-picks, there is no upstream commit message explaining
the same set of changes.  Projects with good commit message hygiene will
likely have the explanation from our commit message contained within or
spread among the relevant upstream commits, but not all projects run
that way.  As such, the commit message of the commit being rebased may
have reasoning that suggests additional changes that should be made to
adapt to the new base, or it may have information that someone wants to
add as a note to another commit, or perhaps someone even wants to create
an empty commit with the commit message as-is.

Junio commented on the "become-empty" types of commits as follows[1]:

    WRT a change that ends up being empty (as opposed to a change that
    is empty from the beginning), I'd think that the current behaviour
    is desireable one.  "am" based rebase is solely to transplant an
    existing history and want to stop much less than "interactive" one
    whose purpose is to polish a series before making it publishable,
    and asking for confirmation ("this has become empty--do you want to
    drop it?") is more appropriate from the workflow point of view.

[1] https://lore.kernel.org/git/xmqqfu1fswdh.fsf@gitster-ct.c.googlers.com/

I would simply add that his arguments for "am"-based rebases actually
apply to all non-explicitly-interactive rebases.  Also, since we are
stating that different cases should have different defaults, it may be
worth providing a flag to allow users to select which behavior they want
for these commits.

Introduce a new command line flag for selecting the desired behavior:
    --empty={drop,keep,ask}
with the definitions:
    drop: drop commits which become empty
    keep: keep commits which become empty
    ask:  provide the user a chance to interact and pick what to do with
          commits which become empty on a case-by-case basis

In line with Junio's suggestion, if the --empty flag is not specified,
pick defaults as follows:
    explicitly interactive: ask
    otherwise: drop

Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-16 15:40:42 -08:00
Elijah Newren d48e5e21da rebase (interactive-backend): make --keep-empty the default
Different rebase backends have different treatment for commits which
start empty (i.e. have no changes relative to their parent), and the
--keep-empty option was added at some point to allow adjusting behavior.
The handling of commits which start empty is actually quite similar to
commit b00bf1c9a8 (git-rebase: make --allow-empty-message the default,
2018-06-27), which pointed out that the behavior for various backends is
often more happenstance than design.  The specific change made in that
commit is actually quite relevant as well and much of the logic there
directly applies here.

It makes a lot of sense in 'git commit' to error out on the creation of
empty commits, unless an override flag is provided.  However, once
someone determines that there is a rare case that merits using the
manual override to create such a commit, it is somewhere between
annoying and harmful to have to take extra steps to keep such
intentional commits around.  Granted, empty commits are quite rare,
which is why handling of them doesn't get considered much and folks tend
to defer to existing (accidental) behavior and assume there was a reason
for it, leading them to just add flags (--keep-empty in this case) that
allow them to override the bad defaults.  Fix the interactive backend so
that --keep-empty is the default, much like we did with
--allow-empty-message.  The am backend should also be fixed to have
--keep-empty semantics for commits that start empty, but that is not
included in this patch other than a testcase documenting the failure.

Note that there was one test in t3421 which appears to have been written
expecting --keep-empty to not be the default as correct behavior.  This
test was introduced in commit 00b8be5a4d ("add tests for rebasing of
empty commits", 2013-06-06), which was part of a series focusing on
rebase topology and which had an interesting original cover letter at
https://lore.kernel.org/git/1347949878-12578-1-git-send-email-martinvonz@gmail.com/
which noted
    Your input especially appreciated on whether you agree with the
    intent of the test cases.
and then went into a long example about how one of the many tests added
had several questions about whether it was correct.  As such, I believe
most the tests in that series were about testing rebase topology with as
many different flags as possible and were not trying to state in general
how those flags should behave otherwise.

Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-16 15:40:42 -08:00
Junio C Hamano 4dbeecba27 Merge branch 'ag/edit-todo-drop-check'
Allow the rebase.missingCommitsCheck configuration to kick in when
"rebase --edit-todo" and "rebase --continue" restarts the procedure.

* ag/edit-todo-drop-check:
  rebase-interactive: warn if commit is dropped with `rebase --edit-todo'
  sequencer: move check_todo_list_from_file() to rebase-interactive.c
2020-02-14 12:54:21 -08:00
Junio C Hamano d8b8d59054 Merge branch 'ag/rebase-avoid-unneeded-checkout'
"git rebase -i" (and friends) used to unnecessarily check out the
tip of the branch to be rebased, which has been corrected.

* ag/rebase-avoid-unneeded-checkout:
  rebase -i: stop checking out the tip of the branch to rebase
2020-02-14 12:54:21 -08:00
Junio C Hamano 251187084d Merge branch 'js/rebase-i-with-colliding-hash'
"git rebase -i" identifies existing commits in its todo file with
their abbreviated object name, which could become ambigous as it
goes to create new commits, and has a mechanism to avoid ambiguity
in the main part of its execution.  A few other cases however were
not covered by the protection against ambiguity, which has been
corrected.

* js/rebase-i-with-colliding-hash:
  rebase -i: also avoid SHA-1 collisions with missingCommitsCheck
  rebase -i: re-fix short SHA-1 collision
  parse_insn_line(): improve error message when parsing failed
2020-02-14 12:54:20 -08:00
Jeff King d20bc01a51 avoid computing zero offsets from NULL pointer
The Undefined Behavior Sanitizer in clang-11 seems to have learned a new
trick: it complains about computing offsets from a NULL pointer, even if
that offset is 0. This causes numerous test failures. For example, from
t1090:

  unpack-trees.c:1355:41: runtime error: applying zero offset to null pointer
  ...
  not ok 6 - in partial clone, sparse checkout only fetches needed blobs

The code in question looks like this:

  struct cache_entry **cache_end = cache + nr;
  ...
  while (cache != cache_end)

and we sometimes pass in a NULL and 0 for "cache" and "nr". This is
conceptually fine, as "cache_end" would be equal to "cache" in this
case, and we wouldn't enter the loop at all. But computing even a zero
offset violates the C standard. And given the fact that UBSan is
noticing this behavior, this might be a potential problem spot if the
compiler starts making unexpected assumptions based on undefined
behavior.

So let's just avoid it, which is pretty easy. In some cases we can just
switch to iterating with a numeric index (as we do in sequencer.c here).
In other cases (like the cache_end one) the use of an end pointer is
more natural; we can keep that by just explicitly checking for the
NULL/0 case when assigning the end pointer.

Note that there are two ways you can write this latter case, checking
for the pointer:

  cache_end = cache ? cache + nr : cache;

or the size:

  cache_end = nr ? cache + nr : cache;

For the case of a NULL/0 ptr/len combo, they are equivalent. But writing
it the second way (as this patch does) has the property that if somebody
were to incorrectly pass a NULL pointer with a non-zero length, we'd
continue to notice and segfault, rather than silently pretending the
length was zero.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-28 23:12:48 -08:00
Alban Gruin 5a5445d878 rebase-interactive: warn if commit is dropped with `rebase --edit-todo'
When set to "warn" or "error", `rebase.missingCommitsCheck' would make
`rebase -i' warn if the user removed commits from the todo list to
prevent mistakes.  Unfortunately, `rebase --edit-todo' and `rebase
--continue' don't take it into account.

This adds the ability for `rebase --edit-todo' and `rebase --continue'
to check if commits were dropped by the user.  As both edit_todo_list()
and complete_action() parse the todo list and check for dropped commits,
the code doing so in the latter is removed to reduce duplication.
`edit_todo_list_advice' is removed from sequencer.c as it is no longer
used there.

This changes when a backup of the todo list is made.  Until now, it was
saved only once, before the initial edit.  Now, it is also made if the
original todo list has no errors or no dropped commits.  Thus, the
backup should be error-free.  Without this, sequencer_continue()
(`rebase --continue') could only compare the current todo list against
the original, unedited list.  Before this change, this file was only
used by edit_todo_list() and `rebase -p' to create the backup before
the initial edit, and check_todo_list_from_file(), only used by
`rebase -p' to check for dropped commits after its own initial edit.

If the edited list has an error, a file, `dropped', is created to
report the issue.  Otherwise, it is deleted.  Usually, the edited list
is compared against the list before editing, but if this file exists,
it will be compared to the backup.  Also, if the file exists,
sequencer_continue() checks the list for dropped commits.  If the
check was performed every time, it would fail when resuming a rebase
after resolving a conflict, as the backup will contain commits that
were picked, but they will not be in the new list.  It's safe to
ignore this check if `dropped' does not exist, because that means that
no errors were found at the last edition, so any missing commits here
have already been picked.

Five tests are added to t3404.  The tests for
`rebase.missingCommitsCheck = warn' and `rebase.missingCommitsCheck =
error' have a similar structure.  First, we start a rebase with an
incorrect command on the first line.  Then, we edit the todo list,
removing the first and the last lines.  This demonstrates that
`--edit-todo' notices dropped commits, but not when the command is
incorrect.  Then, we restore the original todo list, and edit it to
remove the last line.  This demonstrates that if we add a commit after
the initial edit, then remove it, `--edit-todo' will notice that it
has been dropped.  Then, the actual rebase takes place.  In the third
test, it is also checked that `--continue' will refuse to resume the
rebase if commits were dropped.  The fourth test checks that no errors
are raised when resuming a rebase after resolving a conflict, the fifth
checks that no errors are raised when editing the todo list after
pausing the rebase.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-28 14:14:57 -08:00
Alban Gruin 1da5874c1b sequencer: move check_todo_list_from_file() to rebase-interactive.c
The message contained in `edit_todo_list_advice' (sequencer.c) is
printed after the initial edit of the todo list if it can't be parsed or
if commits were dropped.  This is done either in complete_action() for
`rebase -i', or in check_todo_list_from_file() for `rebase -p'.

Since we want to add this check when editing the list, we also want to
use this message from edit_todo_list() (rebase-interactive.c).  To this
end, check_todo_list_from_file() is moved to rebase-interactive.c, and
`edit_todo_list_advice' is copied there.  In the next commit,
complete_action() will stop using it, and `edit_todo_list_advice' will
be removed from sequencer.c.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-28 14:13:41 -08:00
Alban Gruin 767a9c417e rebase -i: stop checking out the tip of the branch to rebase
One of the first things done when using a sequencer-based
rebase (ie. `rebase -i', `rebase -r', or `rebase -m') is to make a todo
list.  This requires knowledge of the commit range to rebase.  To get
the oid of the last commit of the range, the tip of the branch to rebase
is checked out with prepare_branch_to_be_rebased(), then the oid of the
head is read.  After this, the tip of the branch is not even modified.
The `am' backend, on the other hand, does not check out the branch.

On big repositories, it's a performance penalty: with `rebase -i', the
user may have to wait before editing the todo list while git is
extracting the branch silently, and "quiet" rebases will be slower than
`am'.

Since we already have the oid of the tip of the branch in
`opts->orig_head', it's useless to switch to this commit.

This removes the call to prepare_branch_to_be_rebased() in
do_interactive_rebase(), and adds a `orig_head' parameter to
get_revision_ranges().  prepare_branch_to_be_rebased() is removed as it
is no longer used.

This introduces a visible change: as we do not switch on the tip of the
branch to rebase, no reflog entry is created at the beginning of the
rebase for it.

Unscientific performance measurements, performed on linux.git, are as
follow:

  Before this patch:

    $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50

    real    0m8,940s
    user    0m6,830s
    sys     0m2,121s

  After this patch:

    $ time git rebase -m --onto v4.18 463fa44eec2fef50~ 463fa44eec2fef50

    real    0m1,834s
    user    0m0,916s
    sys     0m0,206s

Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-24 10:29:42 -08:00
Johannes Schindelin b6992261de rebase -i: re-fix short SHA-1 collision
In 66ae9a57b8 (t3404: rebase -i: demonstrate short SHA-1 collision,
2013-08-23), we added a test case that demonstrated how it is possible
that a previously unambiguous short commit ID could become ambiguous
*during* a rebase.

In 75c6976655 (rebase -i: fix short SHA-1 collision, 2013-08-23), we
fixed that problem simply by writing out the todo list with expanded
commit IDs (except *right* before letting the user edit the todo list,
in which case we shorten them, but we expand them right after the file
was edited).

However, the bug resurfaced as a side effect of 393adf7a6f (sequencer:
directly call pick_commits() from complete_action(), 2019-11-24): as of
this commit, the sequencer no longer re-reads the todo list after
writing it out with expanded commit IDs.

The only redeeming factor is that the todo list is already parsed at
that stage, including all the commits corresponding to the commands,
therefore the sequencer can continue even if the internal todo list has
short commit IDs.

That does not prevent problems, though: the sequencer writes out the
`done` and `git-rebase-todo` files incrementally (i.e. overwriting the
todo list with a version that has _short_ commit IDs), and if a merge
conflict happens, or if an `edit` or a `break` command is encountered, a
subsequent `git rebase --continue` _will_ re-read the todo list, opening
an opportunity for the "short SHA-1 collision" bug again.

To avoid that, let's make sure that we do expand the commit IDs in the
todo list as soon as we have parsed it after letting the user edit it.

Additionally, we improve the 'short SHA-1 collide' test case in t3404 to
test specifically for the case where the rebase is resumed. We also
hard-code the expected colliding short SHA-1s, to document the
expectation (and to make it easier on future readers).

Note that we specifically test that the short commit ID is used in the
`git-rebase-todo.tmp` file: this file is created by the fake editor in
the test script and reflects the state that would have been presented to
the user to edit.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-23 12:48:11 -08:00
Johannes Schindelin d859dcad94 parse_insn_line(): improve error message when parsing failed
In the case that a `get_oid()` call failed, we showed some rather bogus
part of the line instead of the precise string we sent to said function.
That makes it rather hard for users to understand what is going wrong,
so let's fix that.

While at it, return a negative value from `parse_insn_line()` in case of
an error, as per our convention. This function's only caller,
`todo_list_parse_insn_buffer()`, cares only whether that return value is
non-zero or not, i.e. does not need to be changed.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-01-23 12:48:05 -08:00
Junio C Hamano 4d924528d8 Revert "Merge branch 'ra/rebase-i-more-options'"
This reverts commit 5d9324e0f4, reversing
changes made to c58ae96fc4.

The topic turns out to be too buggy for real use.

cf. <f2fe7437-8a48-3315-4d3f-8d51fe4bb8f1@gmail.com>
2020-01-12 13:25:18 -08:00
Junio C Hamano 37c2619d91 Merge branch 'ag/sequencer-todo-updates'
Reduce unnecessary reading of state variables back from the disk
during sequencer operation.

* ag/sequencer-todo-updates:
  sequencer: directly call pick_commits() from complete_action()
  rebase: fill `squash_onto' in get_replay_opts()
  sequencer: move the code writing total_nr on the disk to a new function
  sequencer: update `done_nr' when skipping commands in a todo list
  sequencer: update `total_nr' when adding an item to a todo list
2019-12-16 13:08:31 -08:00
Junio C Hamano 7cc5f89088 Merge branch 'ag/sequencer-continue-leakfix'
Leakfix.

* ag/sequencer-continue-leakfix:
  sequencer: fix a memory leak in sequencer_continue()
2019-12-10 13:11:46 -08:00
Junio C Hamano 5d9324e0f4 Merge branch 'ra/rebase-i-more-options'
"git rebase -i" learned a few options that are known by "git
rebase" proper.

* ra/rebase-i-more-options:
  rebase -i: finishing touches to --reset-author-date
  rebase: add --reset-author-date
  rebase -i: support --ignore-date
  sequencer: rename amend_author to author_to_rename
  rebase -i: support --committer-date-is-author-date
  sequencer: allow callers of read_author_script() to ignore fields
  rebase -i: add --ignore-whitespace flag
2019-12-10 13:11:41 -08:00
Junio C Hamano f233c9f455 Merge branch 'sg/assume-no-todo-update-in-cherry-pick'
While running "revert" or "cherry-pick --edit" for multiple
commits, a recent regression incorrectly detected "nothing to
commit, working tree clean", instead of replaying the commits,
which has been corrected.

* sg/assume-no-todo-update-in-cherry-pick:
  sequencer: don't re-read todo for revert and cherry-pick
2019-12-06 15:09:22 -08:00
Junio C Hamano c9208597a9 Merge branch 'pw/sequencer-compare-with-right-parent-to-check-empty-commits'
The sequencer machinery compared the HEAD and the state it is
attempting to commit to decide if the result would be a no-op
commit, even when amending a commit, which was incorrect, and
has been corrected.

* pw/sequencer-compare-with-right-parent-to-check-empty-commits:
  sequencer: fix empty commit check when amending
2019-12-05 12:52:46 -08:00
Junio C Hamano 995b1b1411 Merge branch 'dd/rebase-merge-reserves-onto-label'
The logic to avoid duplicate label names generated by "git rebase
--rebase-merges" forgot that the machinery itself uses "onto" as a
label name, which must be avoided by auto-generated labels, which
has been corrected.

* dd/rebase-merge-reserves-onto-label:
  sequencer: handle rebase-merges for "onto" message
2019-12-05 12:52:43 -08:00
Junio C Hamano 917d0d6234 Merge branch 'js/rebase-r-safer-label'
A label used in the todo list that are generated by "git rebase
--rebase-merges" is used as a part of a refname; the logic to come
up with the label has been tightened to avoid names that cannot be
used as such.

* js/rebase-r-safer-label:
  rebase -r: let `label` generate safer labels
  rebase-merges: move labels' whitespace mangling into `label_oid()`
2019-12-05 12:52:43 -08:00
Junio C Hamano 6511cb33c9 Merge branch 'dd/sequencer-utf8'
Handling of commit objects that use non UTF-8 encoding during
"rebase -i" has been improved.

* dd/sequencer-utf8:
  sequencer: reencode commit message for am/rebase --show-current-patch
  sequencer: reencode old merge-commit message
  sequencer: reencode squashing commit's message
  sequencer: reencode revert/cherry-pick's todo list
  sequencer: reencode to utf-8 before arrange rebase's todo list
  t3900: demonstrate git-rebase problem with multi encoding
  configure.ac: define ICONV_OMITS_BOM if necessary
  t0028: eliminate non-standard usage of printf
2019-12-01 09:04:36 -08:00
Junio C Hamano d3096d2ba6 Merge branch 'en/doc-typofix'
Docfix.

* en/doc-typofix:
  Fix spelling errors in no-longer-updated-from-upstream modules
  multimail: fix a few simple spelling errors
  sha1dc: fix trivial comment spelling error
  Fix spelling errors in test commands
  Fix spelling errors in messages shown to users
  Fix spelling errors in names of tests
  Fix spelling errors in comments of testcases
  Fix spelling errors in code comments
  Fix spelling errors in documentation outside of Documentation/
  Documentation: fix a bunch of typos, both old and new
2019-12-01 09:04:35 -08:00
Alban Gruin f6b9413baf sequencer: fix a memory leak in sequencer_continue()
When continuing an interactive rebase after a merge conflict was solved,
if the resolution could not be committed, sequencer_continue() would
return early without releasing its todo list, resulting in a memory
leak.  This plugs this leak by jumping to the end of the function, where
the todo list is deallocated.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-30 09:40:35 -08:00
Alban Gruin 393adf7a6f sequencer: directly call pick_commits() from complete_action()
Currently, complete_action(), used by builtin/rebase.c to start a new
rebase, calls sequencer_continue() to do it.  Before the former calls
pick_commits(), it

 - calls read_and_refresh_cache() -- this is unnecessary here as we've
   just called require_clean_work_tree() in complete_action()
 - calls read_populate_opts() -- this is unnecessary as we're starting a
   new rebase, so `opts' is fully populated
 - loads the todo list -- this is unnecessary as we've just populated
   the todo list in complete_action()
 - commits any staged changes -- this is unnecessary as we're starting a
   new rebase, so there are no staged changes
 - calls record_in_rewritten() -- this is unnecessary as we're starting
   a new rebase.

This changes complete_action() to directly call pick_commits() to avoid
these unnecessary steps.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-25 12:24:49 +09:00
Alban Gruin 3f34f2d8a4 sequencer: move the code writing total_nr on the disk to a new function
The total number of commands can be used to show the progression of the
rebasing in a shell.  It is written to the disk by read_populate_todo()
when the todo list is loaded from sequencer_continue() or
pick_commits(), but not by complete_action().

This moves the part writing total_nr to a new function so it can be
called from complete_action().

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-25 12:24:49 +09:00
Alban Gruin 34065541e3 sequencer: update `done_nr' when skipping commands in a todo list
In a todo list, `done_nr' is the number of commands that were executed
or skipped, but skip_unnecessary_picks() did not update it.

This variable is mostly used by command prompts (ie. git-prompt.sh and
the like).  As in the previous commit, this inconsistent behaviour is
not a problem yet, but it would start to matter at the end of this
series the same reason.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-25 12:24:48 +09:00
Alban Gruin 8638114e06 sequencer: update `total_nr' when adding an item to a todo list
`total_nr' is the total number of items, counting both done and todo,
that are in a todo list.  But unlike `nr', it was not updated when an
item was appended to the list.

This variable is mostly used by command prompts (ie. git-prompt.sh and
the like).  By forgetting to update it, the original code made it not
reflect the reality, but this flaw was masked by the code calling
unnecessarily read_populate_todo() again to update the variable to its
correct value.  At the end of this series, the unnecessary call will be
removed, and the inconsistency addressed by this patch would start to
matter.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-25 12:24:48 +09:00
SZEDER Gábor befd4f6a81 sequencer: don't re-read todo for revert and cherry-pick
When 'git revert' or 'git cherry-pick --edit' is invoked with multiple
commits, then after editing the first commit message is finished both
these commands should continue with processing the second commit and
launch another editor for its commit message, assuming there are
no conflicts, of course.

Alas, this inadvertently changed with commit a47ba3c777 (rebase -i:
check for updated todo after squash and reword, 2019-08-19): after
editing the first commit message is finished, both 'git revert' and
'git cherry-pick --edit' exit with error, claiming that "nothing to
commit, working tree clean".

The reason for the changed behaviour is twofold:

  - Prior to a47ba3c777 the up-to-dateness of the todo list file was
    only checked after 'exec' instructions, and that commit moved
    those checks to the common code path.  The intention was that this
    check should be performed after instructions spawning an editor
    ('squash' and 'reword') as well, so the ongoing 'rebase -i'
    notices when the user runs a 'git rebase --edit-todo' while
    squashing/rewording a commit message.

    However, as it happened that check is now performed even after
    'revert' and 'pick' instructions when they involved editing the
    commit message.  And 'revert' by default while 'pick' optionally
    (with 'git cherry-pick --edit') involves editing the commit
    message.

  - When invoking 'git revert' or 'git cherry-pick --edit' with
    multiple commits they don't read a todo list file but assemble the
    todo list in memory, thus the associated stat data used to check
    whether the file has been updated is all zeroed out initially.

    Then the sequencer writes all instructions (including the very
    first) to the todo file, executes the first 'revert/pick'
    instruction, and after the user finished editing the commit
    message the changes of a47ba3c777 kick in, and it checks whether
    the todo file has been modified.  The initial all-zero stat data
    obviously differs from the todo file's current stat data, so the
    sequencer concludes that the file has been modified.  Technically
    it is not wrong, of course, because the file just has been written
    indeed by the sequencer itself, though the file's contents still
    match what the sequencer was invoked with in the beginning.
    Consequently, after re-reading the todo file the sequencer
    executes the same first instruction _again_, thus ending up in
    that "nothing to commit" situation.

The todo list was never meant to be edited during multi-commit 'git
revert' or 'cherry-pick' operations, so perform that "has the todo
file been modified" check only when the sequencer was invoked as part
of an interactive rebase.

Reported-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-24 13:50:40 +09:00
Phillip Wood 2d05ef2778 sequencer: fix empty commit check when amending
This fixes a regression introduced in 356ee4659b ("sequencer: try to
commit without forking 'git commit'", 2017-11-24). When amending a
commit try_to_commit() was using the wrong parent when checking if the
commit would be empty. When amending we need to check against HEAD^ not
HEAD.

t3403 may not seem like the natural home for the new tests but a further
patch series will improve the advice printed by `git commit`. That
series will mutate these tests to check that the advice includes
suggesting `rebase --skip` to skip the fixup that would empty the
commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-23 10:52:32 +09:00
Doan Tran Cong Danh e02058a729 sequencer: handle rebase-merges for "onto" message
In order to work correctly, git-rebase --rebase-merges needs to make
initial todo list with unique labels.

Those unique labels is being handled by employing a hashmap and
appending an unique number if any duplicate is found.

But, we forget that beside those labels for side branches,
we also have a special label `onto' for our so-called new-base.

In a special case that any of those labels for side branches named
`onto', git will run into trouble.

Correct it.

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-20 11:53:57 +09:00
Matthew Rogers cd5522271f rebase -r: let label generate safer labels
The `label` todo command in interactive rebases creates temporary refs
in the `refs/rewritten/` namespace. These refs are stored as loose refs,
i.e. as files in `.git/refs/rewritten/`, therefore they have to conform
with file name limitations on the current filesystem in addition to the
accepted ref format.

This poses a problem in particular on NTFS/FAT, where e.g. the colon,
double-quote and pipe characters are disallowed as part of a file name.

Let's safeguard against this by replacing not only white-space
characters by dashes, but all non-alpha-numeric ones.

However, we exempt non-ASCII UTF-8 characters from that, as it should be
quite possible to reflect branch names such as `↯↯↯` in refs/file names.

Signed-off-by: Matthew Rogers <mattr94@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-18 12:49:17 +09:00
Johannes Schindelin 867bc1d236 rebase-merges: move labels' whitespace mangling into label_oid()
One of the trickier aspects of the design of `git rebase
--rebase-merges` is the way labels are generated for the initial todo
list: those labels are supposed to be intuitive and first and foremost
unique.

To that end, `label_oid()` appends a unique suffix when necessary.

Those labels not only need to be unique, but they also need to be valid
refs. To make sure of that, `make_script_with_merges()` replaces
whitespace by dashes.

That would appear to be the wrong layer for that sanitizing step,
though: all callers of `label_oid()` should get that same benefit.

Even if it does not make a difference currently (the only called of
`label_oid()` that passes a label that might need to be sanitized _is_
`make_script_with_merges()`), let's move the responsibility for
sanitizing labels into the `label_oid()` function.

This commit is best viewed with `-w` because it unfortunately needs to
change the indentation of a large block of code in `label_oid()`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-18 12:49:16 +09:00
Doan Tran Cong Danh 52f52e5ae4 sequencer: reencode commit message for am/rebase --show-current-patch
The message file will be used as commit message for the
git-{am,rebase} --continue.

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-11 18:03:35 +09:00
Doan Tran Cong Danh 5772b0c745 sequencer: reencode old merge-commit message
During rebasing, old merge's message (encoded in old encoding)
will be used as message for new merge commit (created by rebase).

In case of the value of i18n.commitencoding has been changed after the
old merge time. We will receive an unusable message for this new merge.

Correct it.

This change also notice a breakage with git-rebase label system.

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-11 18:03:35 +09:00
Doan Tran Cong Danh b375744274 sequencer: reencode squashing commit's message
On fixup/squash-ing rebase, git will create new commit in
i18n.commitencoding, reencode the commit message to that said encode.

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-11 09:43:48 +09:00
Doan Tran Cong Danh 019a9d8362 sequencer: reencode revert/cherry-pick's todo list
Keep revert/cherry-pick's todo list in line with rebase todo list.

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-11 09:43:48 +09:00
Doan Tran Cong Danh 0798d16fe3 sequencer: reencode to utf-8 before arrange rebase's todo list
On musl libc, ISO-2022-JP encoder is too eager to switch back to
1 byte encoding, musl's iconv always switch back after every combining
character. Comparing glibc and musl's output for this command
$ sed q t/t3900/ISO-2022-JP.txt| iconv -f ISO-2022-JP -t utf-8 |
	iconv -f utf-8 -t ISO-2022-JP | xxd

glibc:
00000000: 1b24 4224 4f24 6c24 5224 5b24 551b 2842  .$B$O$l$R$[$U.(B
00000010: 0a                                       .

musl:
00000000: 1b24 4224 4f1b 2842 1b24 4224 6c1b 2842  .$B$O.(B.$B$l.(B
00000010: 1b24 4224 521b 2842 1b24 4224 5b1b 2842  .$B$R.(B.$B$[.(B
00000020: 1b24 4224 551b 2842 0a                   .$B$U.(B.

Although musl iconv's output isn't optimal, it's still correct.

From commit 7d509878b8, ("pretty.c: format string with truncate respects
logOutputEncoding", 2014-05-21), we're encoding the message to utf-8
first, then format it and convert the message to the actual output
encoding on git commit --squash.

Thus, t3900::test_commit_autosquash_flags is failing on musl libc.

Reencode to utf-8 before arranging rebase's todo list.

By doing this, we also remove a breakage noticed by a test added in the
previous commit.

Signed-off-by: Doan Tran Cong Danh <congdanhqx@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-11 09:43:48 +09:00
Junio C Hamano 5c8c0a0d78 Merge branch 'pw/post-commit-from-sequencer'
"rebase -i" ceased to run post-commit hook by mistake in an earlier
update, which has been corrected.

* pw/post-commit-from-sequencer:
  sequencer: run post-commit hook
  move run_commit_hook() to libgit and use it there
  sequencer.h fix placement of #endif
  t3404: remove uneeded calls to set_fake_editor
  t3404: set $EDITOR in subshell
  t3404: remove unnecessary subshell
2019-11-10 18:02:12 +09:00
Elijah Newren 15beaaa3d1 Fix spelling errors in code comments
Reported-by: Jens Schleusener <Jens.Schleusener@fossies.org>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-10 16:00:54 +09:00
Rohit Ashiwal 08187b4cba rebase -i: support --ignore-date
rebase am already has this flag to "lie" about the author date
by changing it to the committer (current) date. Let's add the same
for interactive machinery.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-02 15:37:12 +09:00
Rohit Ashiwal 0185c683c9 sequencer: rename amend_author to author_to_rename
The purpose of amend_author was to free() the malloc()'d string
obtained from get_author() while amending a commit. But we can
also use the variable to free() the author at our convenience.
Rename it to convey this meaning.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-02 15:36:23 +09:00
Rohit Ashiwal cbd8db17ac rebase -i: support --committer-date-is-author-date
rebase am already has this flag to "lie" about the committer date
by changing it to the author date. Let's add the same for
interactive machinery.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-02 15:36:23 +09:00
Rohit Ashiwal c068bcc59b sequencer: allow callers of read_author_script() to ignore fields
The current callers of the read_author_script() function read name,
email and date from the author script.  Allow callers to signal that
they are not interested in some among these three fields by passing
NULL.

Note that fields that are ignored still must exist and be formatted
correctly in the author script.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-11-02 15:34:50 +09:00
Phillip Wood 4627bc777e sequencer: run post-commit hook
Prior to commit 356ee4659b ("sequencer: try to commit without forking
'git commit'", 2017-11-24) the sequencer would always run the
post-commit hook after each pick or revert as it forked `git commit` to
create the commit. The conversion to committing without forking `git
commit` omitted to call the post-commit hook after creating the commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-10-16 10:30:51 +09:00
Phillip Wood 49697cb721 move run_commit_hook() to libgit and use it there
This function was declared in commit.h but was implemented in
builtin/commit.c so was not part of libgit. Move it to libgit so we can
use it in the sequencer. This simplifies the implementation of
run_prepare_commit_msg_hook() and will be used in the next commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2019-10-16 10:30:51 +09:00
Junio C Hamano 5efabc7ed9 Merge branch 'ew/hashmap'
Code clean-up of the hashmap API, both users and implementation.

* ew/hashmap:
  hashmap_entry: remove first member requirement from docs
  hashmap: remove type arg from hashmap_{get,put,remove}_entry
  OFFSETOF_VAR macro to simplify hashmap iterators
  hashmap: introduce hashmap_free_entries
  hashmap: hashmap_{put,remove} return hashmap_entry *
  hashmap: use *_entry APIs for iteration
  hashmap_cmp_fn takes hashmap_entry params
  hashmap_get{,_from_hash} return "struct hashmap_entry *"
  hashmap: use *_entry APIs to wrap container_of
  hashmap_get_next returns "struct hashmap_entry *"
  introduce container_of macro
  hashmap_put takes "struct hashmap_entry *"
  hashmap_remove takes "const struct hashmap_entry *"
  hashmap_get takes "const struct hashmap_entry *"
  hashmap_add takes "struct hashmap_entry *"
  hashmap_get_next takes "const struct hashmap_entry *"
  hashmap_entry_init takes "struct hashmap_entry *"
  packfile: use hashmap_entry in delta_base_cache_entry
  coccicheck: detect hashmap_entry.hash assignment
  diff: use hashmap_entry_init on moved_entry.ent
2019-10-15 13:48:02 +09:00