| Message ID | f401bd5ad0dd7564412e72d19a4193ad3f64e638.1658466942.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix merge restore state | expand |
On Fri, Jul 22 2022, Elijah Newren via GitGitGadget wrote: > From: Elijah Newren <newren@gmail.com> > > There are multiple issues at play here: > > 1) If `git merge` is invoked with staged changes, it should abort > without doing any merging, and the user's working tree and index > should be the same as before merge was invoked. > 2) Merge strategies are responsible for enforcing the index == HEAD > requirement. (See 9822175d2b ("Ensure index matches head before > invoking merge machinery, round N", 2019-08-17) for some history > around this.) > 3) Merge strategies can bail saying they are not an appropriate > handler for the merge in question (possibly allowing other > strategies to be used instead). > 4) Merge strategies can make changes to the index and working tree, > and have no expectation to clean up after themselves, *even* if > they bail out and say they are not an appropriate handler for > the merge in question. (The `octopus` merge strategy does this, > for example.) > 5) Because of (3) and (4), builtin/merge.c stashes state before > trying merge strategies and restores it afterward. > > Unfortunately, if users had staged changes before calling `git merge`, > builtin/merge.c could do the following: > > * stash the changes, in order to clean up after the strategies > * try all the merge strategies in turn, each of which report they > cannot function due to the index not matching HEAD > * restore the changes via "git stash apply" > > But that last step would have the net effect of unstaging the user's > changes. Fix this by adding the "--index" option to "git stash apply". > While at it, also squelch the stash apply output; we already report > "Rewinding the tree to pristine..." and don't need a detailed `git > status` report afterwards. > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > builtin/merge.c | 5 +++-- > t/t6424-merge-unrelated-index-changes.sh | 7 ++++++- > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/builtin/merge.c b/builtin/merge.c > index 4170c30317e..f807bf335bd 100644 > --- a/builtin/merge.c > +++ b/builtin/merge.c > @@ -383,14 +383,15 @@ static void reset_hard(const struct object_id *oid, int verbose) > static void restore_state(const struct object_id *head, > const struct object_id *stash) > { > - const char *args[] = { "stash", "apply", NULL, NULL }; > + const char *args[] = { "stash", "apply", "--index", "--quiet", > + NULL, NULL }; > > if (is_null_oid(stash)) > return; > > reset_hard(head, 1); > > - args[2] = oid_to_hex(stash); > + args[4] = oid_to_hex(stash); > > /* > * It is OK to ignore error here, for example when there was Just a nit/side comment: This is one of these older-style arg constructions that we've replaced with strvec in most other places. Let's leave this alone for now (especially in a v4), but FWIW I wouldn't mind if these sort of changes were strvec converted while at it: diff --git a/builtin/merge.c b/builtin/merge.c index 64def49734a..c3a3a1fde50 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -383,21 +383,23 @@ static void reset_hard(const struct object_id *oid, int verbose) static void restore_state(const struct object_id *head, const struct object_id *stash) { - const char *args[] = { "stash", "apply", "--index", "--quiet", - NULL, NULL }; + struct strvec args = STRVEC_INIT; + + strvec_pushl(&args, "stash", "apply", "--index", "--quiet", NULL); reset_hard(head, 1); if (is_null_oid(stash)) goto refresh_cache; - args[4] = oid_to_hex(stash); + strvec_push(&args, oid_to_hex(stash)); /* * It is OK to ignore error here, for example when there was * nothing to restore. */ - run_command_v_opt(args, RUN_GIT_CMD); + run_command_v_opt(args.v, RUN_GIT_CMD); + strvec_clear(&args); refresh_cache: if (discard_cache() < 0 || read_cache() < 0) I.e. it takes about as much mental energy to review that as counting the args elements and seeing that 2 to 4 is correct :)
On Fri, Jul 22, 2022 at 3:55 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > On Fri, Jul 22 2022, Elijah Newren via GitGitGadget wrote: > > > From: Elijah Newren <newren@gmail.com> > > > > There are multiple issues at play here: > > > > 1) If `git merge` is invoked with staged changes, it should abort > > without doing any merging, and the user's working tree and index > > should be the same as before merge was invoked. > > 2) Merge strategies are responsible for enforcing the index == HEAD > > requirement. (See 9822175d2b ("Ensure index matches head before > > invoking merge machinery, round N", 2019-08-17) for some history > > around this.) > > 3) Merge strategies can bail saying they are not an appropriate > > handler for the merge in question (possibly allowing other > > strategies to be used instead). > > 4) Merge strategies can make changes to the index and working tree, > > and have no expectation to clean up after themselves, *even* if > > they bail out and say they are not an appropriate handler for > > the merge in question. (The `octopus` merge strategy does this, > > for example.) > > 5) Because of (3) and (4), builtin/merge.c stashes state before > > trying merge strategies and restores it afterward. > > > > Unfortunately, if users had staged changes before calling `git merge`, > > builtin/merge.c could do the following: > > > > * stash the changes, in order to clean up after the strategies > > * try all the merge strategies in turn, each of which report they > > cannot function due to the index not matching HEAD > > * restore the changes via "git stash apply" > > > > But that last step would have the net effect of unstaging the user's > > changes. Fix this by adding the "--index" option to "git stash apply". > > While at it, also squelch the stash apply output; we already report > > "Rewinding the tree to pristine..." and don't need a detailed `git > > status` report afterwards. > > > > Signed-off-by: Elijah Newren <newren@gmail.com> > > --- > > builtin/merge.c | 5 +++-- > > t/t6424-merge-unrelated-index-changes.sh | 7 ++++++- > > 2 files changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/builtin/merge.c b/builtin/merge.c > > index 4170c30317e..f807bf335bd 100644 > > --- a/builtin/merge.c > > +++ b/builtin/merge.c > > @@ -383,14 +383,15 @@ static void reset_hard(const struct object_id *oid, int verbose) > > static void restore_state(const struct object_id *head, > > const struct object_id *stash) > > { > > - const char *args[] = { "stash", "apply", NULL, NULL }; > > + const char *args[] = { "stash", "apply", "--index", "--quiet", > > + NULL, NULL }; > > > > if (is_null_oid(stash)) > > return; > > > > reset_hard(head, 1); > > > > - args[2] = oid_to_hex(stash); > > + args[4] = oid_to_hex(stash); > > > > /* > > * It is OK to ignore error here, for example when there was > > Just a nit/side comment: This is one of these older-style arg > constructions that we've replaced with strvec in most other places. > > Let's leave this alone for now (especially in a v4), but FWIW I wouldn't > mind if these sort of changes were strvec converted while at it: > > diff --git a/builtin/merge.c b/builtin/merge.c > index 64def49734a..c3a3a1fde50 100644 > --- a/builtin/merge.c > +++ b/builtin/merge.c > @@ -383,21 +383,23 @@ static void reset_hard(const struct object_id *oid, int verbose) > static void restore_state(const struct object_id *head, > const struct object_id *stash) > { > - const char *args[] = { "stash", "apply", "--index", "--quiet", > - NULL, NULL }; > + struct strvec args = STRVEC_INIT; > + > + strvec_pushl(&args, "stash", "apply", "--index", "--quiet", NULL); > > reset_hard(head, 1); > > if (is_null_oid(stash)) > goto refresh_cache; > > - args[4] = oid_to_hex(stash); > + strvec_push(&args, oid_to_hex(stash)); > > /* > * It is OK to ignore error here, for example when there was > * nothing to restore. > */ > - run_command_v_opt(args, RUN_GIT_CMD); > + run_command_v_opt(args.v, RUN_GIT_CMD); > + strvec_clear(&args); > > refresh_cache: > if (discard_cache() < 0 || read_cache() < 0) > > I.e. it takes about as much mental energy to review that as counting the > args elements and seeing that 2 to 4 is correct :) I like this change and included it in my reroll. Thanks!
diff --git a/builtin/merge.c b/builtin/merge.c index 4170c30317e..f807bf335bd 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -383,14 +383,15 @@ static void reset_hard(const struct object_id *oid, int verbose) static void restore_state(const struct object_id *head, const struct object_id *stash) { - const char *args[] = { "stash", "apply", NULL, NULL }; + const char *args[] = { "stash", "apply", "--index", "--quiet", + NULL, NULL }; if (is_null_oid(stash)) return; reset_hard(head, 1); - args[2] = oid_to_hex(stash); + args[4] = oid_to_hex(stash); /* * It is OK to ignore error here, for example when there was diff --git a/t/t6424-merge-unrelated-index-changes.sh b/t/t6424-merge-unrelated-index-changes.sh index 3019d030e07..c96649448fa 100755 --- a/t/t6424-merge-unrelated-index-changes.sh +++ b/t/t6424-merge-unrelated-index-changes.sh @@ -285,6 +285,7 @@ test_expect_success 'resolve && recursive && ort' ' test_seq 0 10 >a && git add a && + git rev-parse :a >expect && sane_unset GIT_TEST_MERGE_ALGORITHM && test_must_fail git merge -s resolve -s recursive -s ort C^0 >output 2>&1 && @@ -292,7 +293,11 @@ test_expect_success 'resolve && recursive && ort' ' grep "Trying merge strategy resolve..." output && grep "Trying merge strategy recursive..." output && grep "Trying merge strategy ort..." output && - grep "No merge strategy handled the merge." output + grep "No merge strategy handled the merge." output && + + # Changes to "a" should remain staged + git rev-parse :a >actual && + test_cmp expect actual ' test_done