summaryrefslogtreecommitdiff
path: root/sequencer.c
AgeCommit message (Collapse)AuthorFilesLines
2020-05-14Merge branch 'js/rebase-autosquash-double-fixup-fix'Libravatar Junio C Hamano1-2/+5
"rebase -i" segfaulted when rearranging a sequence that has a fix-up that applies another fix-up (which may or may not be a fix-up of yet another step). * js/rebase-autosquash-double-fixup-fix: rebase --autosquash: fix a potential segfault
2020-05-09rebase --autosquash: fix a potential segfaultLibravatar Johannes Schindelin1-2/+5
When rearranging the todo list so that the fixups/squashes are reordered just after the commits they intend to fix up, we use two arrays to maintain that list: `next` and `tail`. The idea is that `next[i]`, if set to a non-negative value, contains the index of the item that should be rearranged just after the `i`th item. To avoid having to walk the entire `next` chain when appending another fixup/squash, we also store the end of the `next` chain in `tail[i]`. The logic we currently use to update these array items is based on the assumption that given a fixup/squash item at index `i`, we just found the index `i2` indicating the first item in that fixup chain. However, as reported by Paul Ganssle, that need not be true: the special form `fixup! <commit-hash>` is allowed to point to _another_ fixup commit in the middle of the fixup chain. Example: * 0192a To fixup * 02f12 fixup! To fixup * 03763 fixup! To fixup * 04ecb fixup! 02f12 Note how the fourth commit targets the second commit, which is already a fixup that targets the first commit. Previously, we would update `next` and `tail` under our assumption that every `fixup!` commit would find the start of the `fixup!`/`squash!` chain. This would lead to a segmentation fault because we would actually end up with a `next[i]` pointing to a `fixup!` but the corresponding `tail[i]` pointing nowhere, which would the lead to a segmentation fault. Let's fix this by _inserting_, rather than _appending_, the item. In other words, if we make a given line successor of another line, we do not simply forget any previously set successor of the latter, but make it a successor of the former. In the above example, at the point when we insert 04ecb just after 02f12, 03763 would already be recorded as a successor of 04ecb, and we now "squeeze in" 04ecb. To complete the idea, we now no longer assume that `next[i]` pointing to a line means that `last[i]` points to a line, too. Instead, we extend the concept of `last` to cover also partial `fixup!`/`squash!` chains, i.e. chains starting in the middle of a larger such chain. In the above example, after processing all lines, `last[0]` (corresponding to 0192a) would point to 03763, which indeed is the end of the overall `fixup!` chain, and `last[1]` (corresponding to 02f12) would point to 04ecb (which is the last `fixup!` targeting 02f12, but it has 03763 as successor, i.e. it is not the end of overall `fixup!` chain). Reported-by: Paul Ganssle <paul@ganssle.io> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-29Merge branch 'dl/merge-autostash'Libravatar Junio C Hamano1-47/+129
"git merge" learns the "--autostash" option. * dl/merge-autostash: (22 commits) pull: pass --autostash to merge t5520: make test_pull_autostash() accept expect_parent_num merge: teach --autostash option sequencer: implement apply_autostash_oid() sequencer: implement save_autostash() sequencer: unlink autostash in apply_autostash() sequencer: extract perform_autostash() from rebase rebase: generify create_autostash() rebase: extract create_autostash() reset: extract reset_head() from rebase rebase: generify reset_head() rebase: use apply_autostash() from sequencer.c sequencer: rename stash_sha1 to stash_oid sequencer: make apply_autostash() accept a path rebase: use read_oneliner() sequencer: make read_oneliner() extern sequencer: configurably warn on non-existent files sequencer: make read_oneliner() accept flags sequencer: make file exists check more efficient sequencer: stop leaking buf ...
2020-04-22Merge branch 'jt/rebase-allow-duplicate'Libravatar Junio C Hamano1-1/+2
Allow "git rebase" to reapply all local commits, even if the may be already in the upstream, without checking first. * jt/rebase-allow-duplicate: rebase --merge: optionally skip upstreamed commits
2020-04-22Merge branch 'en/rebase-no-keep-empty'Libravatar Junio C Hamano1-0/+11
"git rebase" (again) learns to honor "--no-keep-empty", which lets the user to discard commits that are empty from the beginning (as opposed to the ones that become empty because of rebasing). The interactive rebase also marks commits that are empty in the todo. * en/rebase-no-keep-empty: rebase: fix an incompatible-options error message rebase: reinstate --no-keep-empty rebase -i: mark commits that begin empty in todo editor
2020-04-22Merge branch 'dd/no-gpg-sign'Libravatar Junio C Hamano1-0/+2
"git rebase" learned the "--no-gpg-sign" option to countermand commit.gpgSign the user may have. * dd/no-gpg-sign: Documentation: document merge option --no-gpg-sign Documentation: merge commit-tree --[no-]gpg-sign Documentation: reword commit --no-gpg-sign Documentation: document am --no-gpg-sign cherry-pick/revert: honour --no-gpg-sign in all case rebase.c: honour --no-gpg-sign
2020-04-22Merge branch 'en/sequencer-reflog-action'Libravatar Junio C Hamano1-2/+8
"git rebase -i" did not leave the reflog entries correctly. * en/sequencer-reflog-action: sequencer: honor GIT_REFLOG_ACTION
2020-04-22Merge branch 'ag/rebase-merge-allow-ff-under-abbrev-command'Libravatar Junio C Hamano1-3/+6
"git rebase" with the merge backend did not work well when the rebase.abbreviateCommands configuration was set. * ag/rebase-merge-allow-ff-under-abbrev-command: t3432: test `--merge' with `rebase.abbreviateCommands = true', too sequencer: don't abbreviate a command if it doesn't have a short form
2020-04-11rebase --merge: optionally skip upstreamed commitsLibravatar Jonathan Tan1-1/+2
When rebasing against an upstream that has had many commits since the original branch was created: O -- O -- ... -- O -- O (upstream) \ -- O (my-dev-branch) it must read the contents of every novel upstream commit, in addition to the tip of the upstream and the merge base, because "git rebase" attempts to exclude commits that are duplicates of upstream ones. This can be a significant performance hit, especially in a partial clone, wherein a read of an object may end up being a fetch. Add a flag to "git rebase" to allow suppression of this feature. This flag only works when using the "merge" backend. This flag changes the behavior of sequencer_make_script(), called from do_interactive_rebase() <- run_rebase_interactive() <- run_specific_rebase() <- cmd_rebase(). With this flag, limit_list() (indirectly called from sequencer_make_script() through prepare_revision_walk()) will no longer call cherry_pick_list(), and thus PATCHSAME is no longer set. Refraining from setting PATCHSAME both means that the intermediate commits in upstream are no longer read (as shown by the test) and means that no PATCHSAME-caused skipping of commits is done by sequencer_make_script(), either directly or through make_script_with_merges(). Signed-off-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-11rebase: reinstate --no-keep-emptyLibravatar Elijah Newren1-0/+6
Commit d48e5e21da ("rebase (interactive-backend): make --keep-empty the default", 2020-02-15) turned --keep-empty (for keeping commits which start empty) into the default. The logic underpinning that commit was: 1) 'git commit' errors out on the creation of empty commits without an override flag 2) Once someone determines that the override is worthwhile, it's annoying and/or harmful to required them to take extra steps in order to keep such commits around (and to repeat such steps with every rebase). While the logic on which the decision was made is sound, the result was a bit of an overcorrection. Instead of jumping to having --keep-empty being the default, it jumped to making --keep-empty the only available behavior. There was a simple workaround, though, which was thought to be good enough at the time. People could still drop commits which started empty the same way the could drop any commits: by firing up an interactive rebase and picking out the commits they didn't want from the list. However, there are cases where external tools might create enough empty commits that picking all of them out is painful. As such, having a flag to automatically remove start-empty commits may be beneficial. Provide users a way to drop commits which start empty using a flag that existed for years: --no-keep-empty. Interpret --keep-empty as countermanding any previous --no-keep-empty, but otherwise leaving --keep-empty as the default. This might lead to some slight weirdness since commands like git rebase --empty=drop --keep-empty git rebase --empty=keep --no-keep-empty look really weird despite making perfect sense (the first will drop commits which become empty, but keep commits that started empty; the second will keep commits which become empty, but drop commits which started empty). However, --no-keep-empty was named years ago and we are predominantly keeping it for backward compatibility; also we suspect it will only be used rarely since folks already have a simple way to drop commits they don't want with an interactive rebase. Reported-by: Bryan Turner <bturner@atlassian.com> Reported-by: Sami Boukortt <sami@boukortt.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-11rebase -i: mark commits that begin empty in todo editorLibravatar Elijah Newren1-0/+5
While many users who intentionally create empty commits do not want them thrown away by a rebase, there are third-party tools that generate empty commits that a user might not want. In the past, users have used rebase to get rid of such commits (a side-effect of the fact that the --apply backend is not currently capable of keeping them). While such users could fire up an interactive rebase and just remove the lines corresponding to empty commits, that might be difficult if the third-party tool generates many of them. Simplify this task for users by marking such lines with a suffix of " # empty" in the todo list. Suggested-by: Sami Boukortt <sami@boukortt.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-10sequencer: implement apply_autostash_oid()Libravatar Denton Liu1-12/+26
Split apply_save_autostash() into apply_autostash_oid() and apply_save_autostash() where the former operates on an OID string and the latter reads the OID from a file before passing it into apply_save_autostash_oid(). This function is required for a future commmit which will rely on being able to apply an autostash whose OID is stored as a string. Signed-off-by: Denton Liu <liu.denton@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-10sequencer: implement save_autostash()Libravatar Denton Liu1-10/+27
Extract common functionality of apply_autostash() into apply_save_autostash() and use it to implement save_autostash(). This function will be used in a future commit. The difference between save_autostash() and apply_autostash() is that the former does not try to apply the stash. It skips that step and just stores the created entry in the stash reflog. This is useful in the case where we abort an operation when an autostash is present but we don't want to dirty the worktree with the application of the stash. For example, in a future commit, we will implement `git merge --autostash`. Since merges can be aborted using `git reset --hard`, we'd make use of save_autostash() to save the autostash entry instead of applying it to the worktree thus keeping the worktree undirtied. Signed-off-by: Denton Liu <liu.denton@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-10sequencer: unlink autostash in apply_autostash()Libravatar Denton Liu1-0/+1
Explicitly remove autostash file in apply_autostash() once it has been applied successfully. This is currently a no-op because the only users of this function will unlink the state (including the autostash file) after this function runs. However, in the future, we will introduce a user of the function that does not explicitly remove the state so we do it here. Signed-off-by: Denton Liu <liu.denton@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-10sequencer: extract perform_autostash() from rebaseLibravatar Denton Liu1-0/+50
Lib-ify the autostash code by extracting perform_autostash() from rebase into sequencer. In a future commit, this will be used to implement `--autostash` in other builtins. This patch is best viewed with `--color-moved`. Signed-off-by: Denton Liu <liu.denton@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-10rebase: use apply_autostash() from sequencer.cLibravatar Denton Liu1-1/+1
The apply_autostash() function in builtin/rebase.c is similar enough to the apply_autostash() function in sequencer.c that they are almost interchangeable, except for the type of arg they accept. Make the sequencer.c version extern and use it in rebase. The rebase version was introduced in 6defce2b02 (builtin rebase: support `--autostash` option, 2018-09-04) as part of the shell to C conversion. It opted to duplicate the function because, at the time, there was another in-progress project converting interactive rebase from shell to C as well and they did not want to clash with them by refactoring sequencer.c version of apply_autostash(). Since both efforts are long done, we can freely combine them together now. Signed-off-by: Denton Liu <liu.denton@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-10sequencer: rename stash_sha1 to stash_oidLibravatar Denton Liu1-8/+8
The preferred terminology is to refer to object identifiers as "OIDs". Rename the `stash_sha1` variable to `stash_oid` in order to conform to this. Signed-off-by: Denton Liu <liu.denton@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-10sequencer: make apply_autostash() accept a pathLibravatar Denton Liu1-7/+7
In order to make apply_autostash() more generic for future extraction, make it accept a `path` argument so that the location from where to read the reference to the autostash commit can be customized. Remove the `opts` argument since it was unused before anyway. Signed-off-by: Denton Liu <liu.denton@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-07sequencer: make read_oneliner() externLibravatar Denton Liu1-13/+1
The function read_oneliner() is a generally useful util function. Instead of hiding it as a static function within sequencer.c, extern it so that it can be reused by others. This patch is best viewed with --color-moved. Signed-off-by: Denton Liu <liu.denton@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-07sequencer: configurably warn on non-existent filesLibravatar Denton Liu1-1/+3
In the future, we plan on externing read_oneliner(). Future users of read_oneliner() will want the ability to output warnings in the event that the `path` doesn't exist. Introduce the `READ_ONELINER_WARN_MISSING` flag which, if active, would issue a warning when a file doesn't exist by always executing warning_errno() in the case where strbuf_read_file() fails. Signed-off-by: Denton Liu <liu.denton@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-07sequencer: make read_oneliner() accept flagsLibravatar Denton Liu1-7/+14
In a future commit, we will need read_oneliner() to accept flags other than just `skip_if_empty`. Instead of having an argument for each flag, teach read_oneliner() to accept the bitfield `flags` instead. For now, only recognize the `READ_ONELINER_SKIP_IF_EMPTY` flag. More flags will be added in a future commit. The result of this is that parallel topics which introduce invocations of read_oneliner() will still be compatible with this new function signature since, instead of passing 1 or 0 for `skip_if_empty`, they'll be passing 1 or 0 to `flags`, which gives equivalent behavior. Mechanically fix up invocations of read_oneliner() with the following spatch @@ expression a, b; @@ read_oneliner(a, b, - 1 + READ_ONELINER_SKIP_IF_EMPTY ) and manually break up long lines in the result. Signed-off-by: Denton Liu <liu.denton@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-07sequencer: make file exists check more efficientLibravatar Denton Liu1-4/+2
We currently check whether a file exists and return early before reading the file. Instead of accessing the file twice, always read the file and check `errno` to see if the file doesn't exist. Signed-off-by: Denton Liu <liu.denton@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-07sequencer: honor GIT_REFLOG_ACTIONLibravatar Elijah Newren1-2/+8
There is a lot of code to honor GIT_REFLOG_ACTION throughout git, including some in sequencer.c; unfortunately, reflog_message() and its callers ignored it. Instruct reflog_message() to check the existing environment variable, and use it when present as an override to action_name(). Also restructure pick_commits() to only temporarily modify GIT_REFLOG_ACTION for a short duration and then restore the old value, so that when we do this setting within a loop we do not keep adding " (pick)" substrings and end up with a reflog message of the form rebase (pick) (pick) (pick) (finish): returning to refs/heads/master Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-05sequencer: stop leaking bufLibravatar Denton Liu1-4/+9
In read_populate_opts(), we call read_oneliner() _after_ calling strbuf_release(). This means that `buf` is leaked at the end of the function. Always clean up the strbuf by going to `done_rebase_i` whether or not we return an error. Signed-off-by: Denton Liu <liu.denton@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-04-03cherry-pick/revert: honour --no-gpg-sign in all caseLibravatar Đoàn Trần Công Danh1-0/+2
{cherry-pick,revert} --edit hasn't honoured --no-gpg-sign yet. Pass this option down to git-commit to honour it. Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-03-30sequencer: don't abbreviate a command if it doesn't have a short formLibravatar Alban Gruin1-3/+6
When the sequencer is requested to abbreviate commands, it will replace those that do not have a short form (eg. `noop') by a comment mark. `noop' serves no purpose, except when fast-forwarding (ie. by running `git rebase'). Removing it will break this command when `rebase.abbreviateCommands' is set to true. Teach todo_list_to_strbuf() to check if a command has an actual short form, and to ignore it if not. Signed-off-by: Alban Gruin <alban.gruin@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-03-28sequencer: mark messages for translationLibravatar Alban Gruin1-3/+3
Signed-off-by: Alban Gruin <alban.gruin@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-03-26Merge branch 'bc/filter-process'Libravatar Junio C Hamano1-0/+1
Provide more information (e.g. the object of the tree-ish in which the blob being converted appears, in addition to its path, which has already been given) to smudge/clean conversion filters. * bc/filter-process: t0021: test filter metadata for additional cases builtin/reset: compute checkout metadata for reset builtin/rebase: compute checkout metadata for rebases builtin/clone: compute checkout metadata for clones builtin/checkout: compute checkout metadata for checkouts convert: provide additional metadata to filters convert: permit passing additional metadata to filter processes builtin/checkout: pass branch info down to checkout_worktree
2020-03-26Merge branch 'bc/sha-256-part-1-of-4'Libravatar Junio C Hamano1-1/+1
SHA-256 transition continues. * bc/sha-256-part-1-of-4: (22 commits) fast-import: add options for rewriting submodules fast-import: add a generic function to iterate over marks fast-import: make find_marks work on any mark set fast-import: add helper function for inserting mark object entries fast-import: permit reading multiple marks files commit: use expected signature header for SHA-256 worktree: allow repository version 1 init-db: move writing repo version into a function builtin/init-db: add environment variable for new repo hash builtin/init-db: allow specifying hash algorithm on command line setup: allow check_repository_format to read repository format t/helper: make repository tests hash independent t/helper: initialize repository if necessary t/helper/test-dump-split-index: initialize git repository t6300: make hash algorithm independent t6300: abstract away SHA-1-specific constants t: use hash-specific lookup tables to define test constants repository: require a build flag to use SHA-256 hex: add functions to parse hex object IDs in any algorithm hex: introduce parsing variants taking hash algorithms ...
2020-03-25Merge branch 'pw/advise-rebase-skip'Libravatar Junio C Hamano1-8/+44
The mechanism to prevent "git commit" from making an empty commit or amending during an interrupted cherry-pick was broken during the rewrite of "git rebase" in C, which has been corrected. * pw/advise-rebase-skip: commit: give correct advice for empty commit during a rebase commit: encapsulate determine_whence() for sequencer commit: use enum value for multiple cherry-picks sequencer: write CHERRY_PICK_HEAD for reword and edit cherry-pick: check commit error messages cherry-pick: add test for `--skip` advice in `git commit` t3404: use test_cmp_rev
2020-03-16builtin/rebase: compute checkout metadata for rebasesLibravatar brian m. carlson1-0/+1
Signed-off-by: brian m. carlson <bk2204@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-03-12Merge branch 'en/rebase-backend'Libravatar Junio C Hamano1-0/+2
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-11sequencer: clear state upon dropping a become-empty commitLibravatar Elijah Newren1-0/+2
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-02Merge branch 'en/rebase-backend'Libravatar Junio C Hamano1-31/+51
"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-02-24commit: use expected signature header for SHA-256Libravatar brian m. carlson1-1/+1
The transition plan anticipates that we will allow signatures using multiple algorithms in a single commit. In order to do so, we need to use a different header per algorithm so that it will be obvious over which data to compute the signature. The transition plan specifies that we should use "gpgsig-sha256", so wire up the commit code such that it can write and parse the current algorithm, and it can remove the headers for any algorithm when creating a new commit. Add tests to ensure that we write using the right header and that git fsck doesn't reject these commits. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
2020-02-16rebase: drop '-i' from the reflog for interactive-based rebasesLibravatar Elijah Newren1-4/+4
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-16rebase, sequencer: remove the broken GIT_QUIET handlingLibravatar Elijah Newren1-4/+2
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-16rebase (interactive-backend): fix handling of commits that become emptyLibravatar Elijah Newren1-11/+39
As established in the previous commit and commit b00bf1c9a8dd (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-16rebase (interactive-backend): make --keep-empty the defaultLibravatar Elijah Newren1-13/+7
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 b00bf1c9a8dd (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 00b8be5a4d38 ("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-14Merge branch 'ag/edit-todo-drop-check'Libravatar Junio C Hamano1-40/+11
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-14Merge branch 'ag/rebase-avoid-unneeded-checkout'Libravatar Junio C Hamano1-14/+0
"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-14Merge branch 'js/rebase-i-with-colliding-hash'Libravatar Junio C Hamano1-4/+14
"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-01-28avoid computing zero offsets from NULL pointerLibravatar Jeff King1-3/+3
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-28rebase-interactive: warn if commit is dropped with `rebase --edit-todo'Libravatar Alban Gruin1-11/+11
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-28sequencer: move check_todo_list_from_file() to rebase-interactive.cLibravatar Alban Gruin1-29/+0
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-24rebase -i: stop checking out the tip of the branch to rebaseLibravatar Alban Gruin1-14/+0
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-23rebase -i: re-fix short SHA-1 collisionLibravatar Johannes Schindelin1-1/+10
In 66ae9a57b88 (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 75c69766554 (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 393adf7a6f6 (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-23parse_insn_line(): improve error message when parsing failedLibravatar Johannes Schindelin1-3/+4
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-12Revert "Merge branch 'ra/rebase-i-more-options'"Libravatar Junio C Hamano1-133/+8
This reverts commit 5d9324e0f4210bb7d52bcb79efe3935703083f72, reversing changes made to c58ae96fc4bb11916b62a96940bb70bb85ea5992. The topic turns out to be too buggy for real use. cf. <f2fe7437-8a48-3315-4d3f-8d51fe4bb8f1@gmail.com>
2019-12-16Merge branch 'ag/sequencer-todo-updates'Libravatar Junio C Hamano1-9/+23
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