| Message ID | a03075167c1f4410a1b4f415313f11a7e1c3a594.1655621424.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix merge restore state | expand |
Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> 于2022年6月19日周日 14:50写道： > > From: Elijah Newren <newren@gmail.com> > > Merge strategies can fail -- not just have conflicts, but give up and > say that they are unable to handle the current type of merge. However, > they can also make changes to the index and working tree before giving > up; merge-octopus does this, for example. Currently, we do not expect > the individual strategies to clean up after themselves, but instead > expect builtin/merge.c to do so. For it to be able to, it needs to save > the state before trying the merge strategy so it can have something to > restore to. Therefore, remove the shortcut bypassing the save_state() > call. > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > builtin/merge.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/builtin/merge.c b/builtin/merge.c > index 2dc56fab70b..aaee8f6a553 100644 > --- a/builtin/merge.c > +++ b/builtin/merge.c > @@ -1663,12 +1663,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > * tree in the index -- this means that the index must be in > * sync with the head commit. The strategies are responsible > * to ensure this. > + * > + * Stash away the local changes so that we can try more than one. > */ > - if (use_strategies_nr == 1 || > - /* > - * Stash away the local changes so that we can try more than one. > - */ > - save_state(&stash)) > + if (save_state(&stash)) > oidclr(&stash); > > for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) { > -- > gitgitgadget > So now we will not make "stash" empty even if we are using only one merge strategy (e.g. octopus), so we can reset to the original state correctly. Good. ZheNing Hu
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Elijah Newren <newren@gmail.com> > > Merge strategies can fail -- not just have conflicts, but give up and > say that they are unable to handle the current type of merge. However, > they can also make changes to the index and working tree before giving > up; merge-octopus does this, for example. Currently, we do not expect > the individual strategies to clean up after themselves, but instead > expect builtin/merge.c to do so. For it to be able to, it needs to save > the state before trying the merge strategy so it can have something to > restore to. Therefore, remove the shortcut bypassing the save_state() > call. > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > builtin/merge.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/builtin/merge.c b/builtin/merge.c > index 2dc56fab70b..aaee8f6a553 100644 > --- a/builtin/merge.c > +++ b/builtin/merge.c > @@ -1663,12 +1663,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > * tree in the index -- this means that the index must be in > * sync with the head commit. The strategies are responsible > * to ensure this. > + * > + * Stash away the local changes so that we can try more than one. > */ The comment explains why we limited the save_state() to avoid wasted cycles and SSD wear and tear by looking at the number of strategies. But because we are removing the restriction (which I am not 100% sure is a good idea), "so that we can try more than one" no longer applies as the reason why we run save_state() here. > - if (use_strategies_nr == 1 || > - /* > - * Stash away the local changes so that we can try more than one. > - */ > - save_state(&stash)) > + if (save_state(&stash)) > oidclr(&stash); > > for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {
On Tue, Jul 19, 2022 at 3:57 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Elijah Newren <newren@gmail.com> > > > > Merge strategies can fail -- not just have conflicts, but give up and > > say that they are unable to handle the current type of merge. However, > > they can also make changes to the index and working tree before giving > > up; merge-octopus does this, for example. Currently, we do not expect > > the individual strategies to clean up after themselves, but instead > > expect builtin/merge.c to do so. For it to be able to, it needs to save > > the state before trying the merge strategy so it can have something to > > restore to. Therefore, remove the shortcut bypassing the save_state() > > call. > > > > Signed-off-by: Elijah Newren <newren@gmail.com> > > --- > > builtin/merge.c | 8 +++----- > > 1 file changed, 3 insertions(+), 5 deletions(-) > > > > diff --git a/builtin/merge.c b/builtin/merge.c > > index 2dc56fab70b..aaee8f6a553 100644 > > --- a/builtin/merge.c > > +++ b/builtin/merge.c > > @@ -1663,12 +1663,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > > * tree in the index -- this means that the index must be in > > * sync with the head commit. The strategies are responsible > > * to ensure this. > > + * > > + * Stash away the local changes so that we can try more than one. > > */ > > The comment explains why we limited the save_state() to avoid wasted > cycles and SSD wear and tear by looking at the number of strategies. > But because we are removing the restriction (which I am not 100% > sure is a good idea), "so that we can try more than one" no longer > applies as the reason why we run save_state() here. I should probably change it to "Stash away the local changes so that we can try more than one and/or recover from merge strategies bailing". In regards to the "good idea" side, I don't really like it either, but: 1. Merge strategies are allowed to make whatever changes they want to the index and working tree. They have no requirement to clean up after themselves. 2. Merge strategies are allowed to bail and say, "Nevermind, not only can I not successfully merge this, I can't even leave conflicts for the users to resolve; I just can't handle it at all. Try some other strategy." (See the "exit 2" codepath of commit 98efc8f3d8 ("octopus: allow manual resolve on the last round.", 2006-01-13), for example) 3. Merge strategies can bail with the "try some other strategy" response _after_ mucking with the index and working tree. octopus does (again, see the commit mentioned above) 4. builtin/merge.c previously would _only_ cleanup for merge strategies if there was more than 1. This combination is clearly broken. We need to fix either item 1 or item 4 (or maybe item 3?). Since 1 might run into issues with user-custom merge strategies which may have been written to mimic octopus and thus rely on the assurances in items 1-3 above, I figured fixing item 4 was the easiest.
diff --git a/builtin/merge.c b/builtin/merge.c index 2dc56fab70b..aaee8f6a553 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1663,12 +1663,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix) * tree in the index -- this means that the index must be in * sync with the head commit. The strategies are responsible * to ensure this. + * + * Stash away the local changes so that we can try more than one. */ - if (use_strategies_nr == 1 || - /* - * Stash away the local changes so that we can try more than one. - */ - save_state(&stash)) + if (save_state(&stash)) oidclr(&stash); for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {