| Message ID | 20201204061623.1170745-5-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pull: default warning improvements | expand |
On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > This essentially reverts commit f15e7cf5cc. > > Once commit d9f15d37f1 introduced the autostash option for the merge > mode, it's not necessary to skip the fast-forward run_merge() when > autostash is set. It helps reviewers and future code readers if you provide a little context when referring to commits, making use of git log's --pretty=reference option to get the output. So, for example, here your commit would read: """ This essentially reverts commit f15e7cf5cc (pull: ff --rebase --autostash works in dirty repo, 2017-06-01). Once commit d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07) introduced the autostash option for the merge mode, it's not necessary to skip the fast-forward run_merge() when autostash is set. """ I still found it slightly hard to follow the explanation even with the added summaries, though. An extra sentence at the end of the second paragraph to make it clear what is being changed ("So, change the code to fast-forward even when autostash is set.") seems to help. > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > builtin/pull.c | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/builtin/pull.c b/builtin/pull.c > index 6279e9ee37..c38548dab8 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -927,7 +927,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > struct oid_array merge_heads = OID_ARRAY_INIT; > struct object_id orig_head, curr_head; > struct object_id rebase_fork_point; > - int autostash; > > if (!getenv("GIT_REFLOG_ACTION")) > set_reflog_message(argc, argv); > @@ -960,8 +959,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > if (get_oid("HEAD", &orig_head)) > oidclr(&orig_head); > > - autostash = config_autostash; > if (opt_rebase) { > + int autostash = config_autostash; > if (opt_autostash != -1) > autostash = opt_autostash; > > @@ -1030,13 +1029,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) && > submodule_touches_in_range(the_repository, &rebase_fork_point, &curr_head)) > die(_("cannot rebase with locally recorded submodule modifications")); > - if (!autostash) { > - if (get_can_ff(&orig_head, &merge_heads.oid[0])) { > - /* we can fast-forward this without invoking rebase */ > - opt_ff = "--ff-only"; > - ran_ff = 1; > - ret = run_merge(); > - } > + if (get_can_ff(&orig_head, &merge_heads.oid[0])) { > + /* we can fast-forward this without invoking rebase */ > + opt_ff = "--ff-only"; > + ran_ff = 1; > + ret = run_merge(); > } > if (!ran_ff) > ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point); > -- > 2.29.2 >
On Fri, Dec 4, 2020 at 5:07 PM Elijah Newren <newren@gmail.com> wrote: > > On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > This essentially reverts commit f15e7cf5cc. > > > > Once commit d9f15d37f1 introduced the autostash option for the merge > > mode, it's not necessary to skip the fast-forward run_merge() when > > autostash is set. > > It helps reviewers and future code readers if you provide a little > context when referring to commits, making use of git log's > --pretty=reference option to get the output. Yes, I actually have this alias: short = show --quiet --format='%C(auto)%h (%s)%C(reset)' Which shows almost the same thing. I've updated it to --format=reference. Thanks for the suggestion. And I usually add those descriptions. > So, for example, here > your commit would read: > > """ > This essentially reverts commit f15e7cf5cc (pull: ff --rebase > --autostash works in dirty repo, 2017-06-01). > > Once commit d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07) > introduced the autostash option for the merge > mode, it's not necessary to skip the fast-forward run_merge() when > autostash is set. > """ > > I still found it slightly hard to follow the explanation even with the > added summaries, though. And that's the reason I didn't add them. You need to look at both the commits to understand why one cancels the other. In my opinion in this particular case the description only makes the text harder to read. Probably some better $subjects like f15e7cf5cc (pull: skip ff merge shortcut on --rebase --autostash) would have helped. > An extra sentence at the end of the second > paragraph to make it clear what is being changed ("So, change the code > to fast-forward even when autostash is set.") seems to help. OK. That's implied by "it's not necessary to skip the fast-forward" but it's better to be explicit. How about this: Currently "git pull --rebase" takes a shortcut in the case a fast-forward merge is possible; run_merge() is called with --ff-only. However, "git merge" didn't have an --autostash option, so, when "git pull --rebase --autostash" was called *and* the fast-forward merge shortcut was taken, then the pull failed. This was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash works in dirty repo, 2017-06-01) by simply skipping the fast-forward merge shortcut. Later on "git merge" learned the --autostash option [a03b55530a (merge: teach --autostash option, 2020-04-07)], and so did "git pull" [d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)]. Therefore it's not necessary to skip the fast-forward merge shortcut anymore when called with --rebase --autostash. Let's always take the fast-forward merge shortcut by essentially reverting f15e7cf5cc. Cheers.
On Fri, Dec 4, 2020 at 4:47 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > > On Fri, Dec 4, 2020 at 5:07 PM Elijah Newren <newren@gmail.com> wrote: > > > > On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras > > <felipe.contreras@gmail.com> wrote: > > > > > > This essentially reverts commit f15e7cf5cc. > > > > > > Once commit d9f15d37f1 introduced the autostash option for the merge > > > mode, it's not necessary to skip the fast-forward run_merge() when > > > autostash is set. > > > > It helps reviewers and future code readers if you provide a little > > context when referring to commits, making use of git log's > > --pretty=reference option to get the output. > > Yes, I actually have this alias: > > short = show --quiet --format='%C(auto)%h (%s)%C(reset)' > > Which shows almost the same thing. I've updated it to > --format=reference. Thanks for the suggestion. > > And I usually add those descriptions. > > > So, for example, here > > your commit would read: > > > > """ > > This essentially reverts commit f15e7cf5cc (pull: ff --rebase > > --autostash works in dirty repo, 2017-06-01). > > > > Once commit d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07) > > introduced the autostash option for the merge > > mode, it's not necessary to skip the fast-forward run_merge() when > > autostash is set. > > """ > > > > I still found it slightly hard to follow the explanation even with the > > added summaries, though. > > And that's the reason I didn't add them. You need to look at both the > commits to understand why one cancels the other. In my opinion in this > particular case the description only makes the text harder to read. > > Probably some better $subjects like f15e7cf5cc (pull: skip ff merge > shortcut on --rebase --autostash) would have helped. > > > An extra sentence at the end of the second > > paragraph to make it clear what is being changed ("So, change the code > > to fast-forward even when autostash is set.") seems to help. > > OK. That's implied by "it's not necessary to skip the fast-forward" > but it's better to be explicit. > > How about this: > > Currently "git pull --rebase" takes a shortcut in the case a > fast-forward merge is possible; run_merge() is called with --ff-only. > > However, "git merge" didn't have an --autostash option, so, when "git > pull --rebase --autostash" was called *and* the fast-forward merge > shortcut was taken, then the pull failed. > > This was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash > works in dirty repo, 2017-06-01) by simply skipping the fast-forward > merge shortcut. > > Later on "git merge" learned the --autostash option [a03b55530a > (merge: teach --autostash option, 2020-04-07)], and so did "git pull" > [d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)]. > > Therefore it's not necessary to skip the fast-forward merge shortcut > anymore when called with --rebase --autostash. > > Let's always take the fast-forward merge shortcut by essentially > reverting f15e7cf5cc. Yes, very nice! Thanks. Elijah
diff --git a/builtin/pull.c b/builtin/pull.c index 6279e9ee37..c38548dab8 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -927,7 +927,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix) struct oid_array merge_heads = OID_ARRAY_INIT; struct object_id orig_head, curr_head; struct object_id rebase_fork_point; - int autostash; if (!getenv("GIT_REFLOG_ACTION")) set_reflog_message(argc, argv); @@ -960,8 +959,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (get_oid("HEAD", &orig_head)) oidclr(&orig_head); - autostash = config_autostash; if (opt_rebase) { + int autostash = config_autostash; if (opt_autostash != -1) autostash = opt_autostash; @@ -1030,13 +1029,11 @@ int cmd_pull(int argc, const char **argv, const char *prefix) recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) && submodule_touches_in_range(the_repository, &rebase_fork_point, &curr_head)) die(_("cannot rebase with locally recorded submodule modifications")); - if (!autostash) { - if (get_can_ff(&orig_head, &merge_heads.oid[0])) { - /* we can fast-forward this without invoking rebase */ - opt_ff = "--ff-only"; - ran_ff = 1; - ret = run_merge(); - } + if (get_can_ff(&orig_head, &merge_heads.oid[0])) { + /* we can fast-forward this without invoking rebase */ + opt_ff = "--ff-only"; + ran_ff = 1; + ret = run_merge(); } if (!ran_ff) ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);
This essentially reverts commit f15e7cf5cc. Once commit d9f15d37f1 introduced the autostash option for the merge mode, it's not necessary to skip the fast-forward run_merge() when autostash is set. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- builtin/pull.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)