| Message ID | 20201210100538.696787-3-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5,1/3] pull: refactor fast-forward check | expand |
Felipe Contreras <felipe.contreras@gmail.com> writes: > Eventually we want to be able to display the warning only when > fast-forward merges are not possible. > > In order to do so we need to move the default warning up to the point > where we can check if we can fast-forward or not. Makes sense. > Additionally, config_get_rebase() was probably never its true home. I agree with this point. I've always found it suboptimal. > This requires a temporary variable to check if we are in the > "default mode" (no --rebase or --no-rebase specified). Two points: - "mode" is so overused a word; a more focused word is preferrable. - by introducing a local variable in cmd_pull() and passing a pointer to it to config_get_rebase(), we can easily avoid having to rely on an extra global variable. I'd suggest addressing the above along the following lines. -static enum rebase_type config_get_rebase(void) +static enum rebase_type config_get_rebase(int *rebase_unspecified) { + *rebase_unspecified = 0; ... various "return" of configured values ... + *rebase_unspecified = 1; return REBASE_FALSE; } Then the caller would declare int rebase_unspecified = 0; and call if (opt_rebase < 0) opt_rebase = config_get_rebase(&rebase_unspecified); to possibly cause it to set to true, and use that instead of the global variable to decide if we want to give the help text. When the helper is not called due to opt_rebase already being set, it is not using configured value but using the choice from the command line, so rebase_unspecified is still false after this point. > @@ -1040,6 +1029,21 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > if (opt_rebase && merge_heads.nr > 1) > die(_("Cannot rebase onto multiple branches.")); And this is the point where we finish various error checks and starts to run either rebase or merge. It is as late as we could delay the "non-ff and you are not configured" message. In other words, the place chosen in cmd_pull() to move this code to is optimal. > + if (default_mode && opt_verbosity >= 0 && !opt_ff) { > + advise(_("Pulling without specifying how to reconcile divergent branches is\n" > + "discouraged. You can squelch this message by running one of the following\n" > + "commands sometime before your next pull:\n" > + "\n" > + " git config pull.rebase false # merge (the default strategy)\n" > + " git config pull.rebase true # rebase\n" > + " git config pull.ff only # fast-forward only\n" > + "\n" > + "You can replace \"git config\" with \"git config --global\" to set a default\n" > + "preference for all repositories. You can also pass --rebase, --no-rebase,\n" > + "or --ff-only on the command line to override the configured default per\n" > + "invocation.\n")); > + } Either as a part of this step, as a part of the next step, or a separate follow-up patch, we should - create a single-purpose helper function that only calls advise() with the message and returns; name it show_advice_pull_non_ff(). - correct the if() statement above, so that regardless of verbosity level, we can do _something_ common when the history does not fast-forward. I.e. if (rebase_unspecified && !opt_ff) { if (opt_verbosity >= 0) show_advice_pull_non_ff(); } These would allow us to further turn the logic to if (rebase_unspecified && !opt_ff) { if (opt_verbosity >= 0 && advice_pull_non_ff) show_advice_pull_non_ff(); die("not a fast-forward; must merge or rebase"); } later in the far future, and we do not want that die() to be affected by verbosity settings. I'll queue such a fix-up patch on top of the series before pushing the integration results out on 'seen'. Thanks.
On Fri, Dec 11, 2020 at 12:54 AM Junio C Hamano <gitster@pobox.com> wrote: > > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > Eventually we want to be able to display the warning only when > > fast-forward merges are not possible. > > > > In order to do so we need to move the default warning up to the point > > where we can check if we can fast-forward or not. > > Makes sense. > > > Additionally, config_get_rebase() was probably never its true home. > > I agree with this point. I've always found it suboptimal. > > > This requires a temporary variable to check if we are in the > > "default mode" (no --rebase or --no-rebase specified). > > Two points: > > - "mode" is so overused a word; a more focused word is preferrable. There's literally only one instance in the file, and it's a call to an external function. Plus, it refers to "git pull" without any --merge or --rebase (that's the default mode in my book). > - by introducing a local variable in cmd_pull() and passing a > pointer to it to config_get_rebase(), we can easily avoid having > to rely on an extra global variable. > > I'd suggest addressing the above along the following lines. ... > to possibly cause it to set to true, and use that instead of the > global variable to decide if we want to give the help text. Yeah, there's only 38 global variables. We wouldn't want another one which makes the code pretty straightforward. > > @@ -1040,6 +1029,21 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > > if (opt_rebase && merge_heads.nr > 1) > > die(_("Cannot rebase onto multiple branches.")); > > And this is the point where we finish various error checks and > starts to run either rebase or merge. It is as late as we could > delay the "non-ff and you are not configured" message. In other > words, the place chosen in cmd_pull() to move this code to is > optimal. Which is right before the fork between rebase and merge. > > + if (default_mode && opt_verbosity >= 0 && !opt_ff) { > > + advise(_("Pulling without specifying how to reconcile divergent branches is\n" > > + "discouraged. You can squelch this message by running one of the following\n" > > + "commands sometime before your next pull:\n" > > + "\n" > > + " git config pull.rebase false # merge (the default strategy)\n" > > + " git config pull.rebase true # rebase\n" > > + " git config pull.ff only # fast-forward only\n" > > + "\n" > > + "You can replace \"git config\" with \"git config --global\" to set a default\n" > > + "preference for all repositories. You can also pass --rebase, --no-rebase,\n" > > + "or --ff-only on the command line to override the configured default per\n" > > + "invocation.\n")); > > + } > > Either as a part of this step, as a part of the next step, or a > separate follow-up patch, we should > > - create a single-purpose helper function that only calls advise() > with the message and returns; name it show_advice_pull_non_ff(). If we are going for low-hanging fruit there were many in v4 of this series, which actually improve behavior, not just code organization, but OK. > - correct the if() statement above, so that regardless of verbosity > level, we can do _something_ common when the history does not > fast-forward. I.e. > > if (rebase_unspecified && !opt_ff) { > if (opt_verbosity >= 0) > show_advice_pull_non_ff(); > } > > These would allow us to further turn the logic to > > if (rebase_unspecified && !opt_ff) { > if (opt_verbosity >= 0 && advice_pull_non_ff) > show_advice_pull_non_ff(); > die("not a fast-forward; must merge or rebase"); > } Should actually be something like: if (rebase_unspecified && !can_ff) die("Not a fast-forward; must either merge or rebase"); But yeah.
Felipe Contreras <felipe.contreras@gmail.com> writes: >> - correct the if() statement above, so that regardless of verbosity >> level, we can do _something_ common when the history does not >> fast-forward. I.e. >> >> if (rebase_unspecified && !opt_ff) { >> if (opt_verbosity >= 0) >> show_advice_pull_non_ff(); >> } >> >> These would allow us to further turn the logic to >> >> if (rebase_unspecified && !opt_ff) { >> if (opt_verbosity >= 0 && advice_pull_non_ff) >> show_advice_pull_non_ff(); >> die("not a fast-forward; must merge or rebase"); >> } > > Should actually be something like: > > if (rebase_unspecified && !can_ff) > die("Not a fast-forward; must either merge or rebase"); The illustration I gave in the message you are responding to was made in the context of patch 2/3; with patch 3/3 where can_ff exists, it would not become like what you gave above. It should instead become if (rebase_unspecified && !opt_ff && !can_ff) { if (opt_verbosity >= 0 && advice_pull_non_ff) show_advice_pull_non_ff(); die("not a fast-forward; must merge or rebase"); } i.e. when we can fast-forward, we do not trigger the "you must specify rebase/merge" message, and we do not trigger the "not a fast-forward" error.
On Fri, Dec 11, 2020 at 6:00 PM Junio C Hamano <gitster@pobox.com> wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > Should actually be something like: > > > > if (rebase_unspecified && !can_ff) > > die("Not a fast-forward; must either merge or rebase"); > > The illustration I gave in the message you are responding to was > made in the context of patch 2/3; with patch 3/3 where can_ff > exists, it would not become like what you gave above. It should > instead become > > if (rebase_unspecified && !opt_ff && !can_ff) { > if (opt_verbosity >= 0 && advice_pull_non_ff) > show_advice_pull_non_ff(); > die("not a fast-forward; must merge or rebase"); > } > > i.e. when we can fast-forward, we do not trigger the "you must > specify rebase/merge" message, and we do not trigger the "not a > fast-forward" error. It's not the !can_ff part I'm trying to highlight, it's the lack of advice *after* we have decided to flip the switch. As I said in another thread: I don't think we have any long condescending error in any other command. Cheers.
Felipe Contreras wrote: > On Fri, Dec 11, 2020 at 12:54 AM Junio C Hamano <gitster@pobox.com> wrote: > > - by introducing a local variable in cmd_pull() and passing a > > pointer to it to config_get_rebase(), we can easily avoid having > > to rely on an extra global variable. > > > > I'd suggest addressing the above along the following lines. > > ... > > > to possibly cause it to set to true, and use that instead of the > > global variable to decide if we want to give the help text. > > Yeah, there's only 38 global variables. We wouldn't want another one > which makes the code pretty straightforward. Another thing that I mentioned in another version of the patch series but not on this one (since the relevant patch was removed) is that this variable is only needed temporarily; it can be removed by further code reorganization. So, I don't think it makes sense to change more code than necessary for the sake of this variable (that might very well disappear).
Felipe Contreras <felipe.contreras@gmail.com> writes: > On Fri, Dec 11, 2020 at 6:00 PM Junio C Hamano <gitster@pobox.com> wrote: >> Felipe Contreras <felipe.contreras@gmail.com> writes: > >> > Should actually be something like: >> > >> > if (rebase_unspecified && !can_ff) >> > die("Not a fast-forward; must either merge or rebase"); >> >> The illustration I gave in the message you are responding to was >> made in the context of patch 2/3; with patch 3/3 where can_ff >> exists, it would not become like what you gave above. It should >> instead become >> >> if (rebase_unspecified && !opt_ff && !can_ff) { >> if (opt_verbosity >= 0 && advice_pull_non_ff) >> show_advice_pull_non_ff(); >> die("not a fast-forward; must merge or rebase"); >> } >> >> i.e. when we can fast-forward, we do not trigger the "you must >> specify rebase/merge" message, and we do not trigger the "not a >> fast-forward" error. > > It's not the !can_ff part I'm trying to highlight, it's the lack of > advice *after* we have decided to flip the switch. > > As I said in another thread: I don't think we have any long > condescending error in any other command. Only the "not a fast-forward and you must choose between merge and rebase" part (i.e. what I listed as the first part of three-part message) is the error. The rest, the message that teaches how to choose between merge and rebase from command line and configuration (or how to choose permanently not to make the choice between the two), is not an error---it's called advice. Even if we were to introduce the third choice (i.e. permanently not to choose between rebase or merge and instead just error out without getting advice), the advice must stay for those who didn't make any choice among the three (i.e. merge, rebase or ff-only). We have many of them. An easy example to spot is a similar sized onefor "git checkout HEAD^0". Neither that one or the one under discussion is particularly condescending.
Junio C Hamano wrote: > Felipe Contreras <felipe.contreras@gmail.com> writes: > > > On Fri, Dec 11, 2020 at 6:00 PM Junio C Hamano <gitster@pobox.com> wrote: > >> Felipe Contreras <felipe.contreras@gmail.com> writes: > > > >> > Should actually be something like: > >> > > >> > if (rebase_unspecified && !can_ff) > >> > die("Not a fast-forward; must either merge or rebase"); > >> > >> The illustration I gave in the message you are responding to was > >> made in the context of patch 2/3; with patch 3/3 where can_ff > >> exists, it would not become like what you gave above. It should > >> instead become > >> > >> if (rebase_unspecified && !opt_ff && !can_ff) { > >> if (opt_verbosity >= 0 && advice_pull_non_ff) > >> show_advice_pull_non_ff(); > >> die("not a fast-forward; must merge or rebase"); > >> } > >> > >> i.e. when we can fast-forward, we do not trigger the "you must > >> specify rebase/merge" message, and we do not trigger the "not a > >> fast-forward" error. > > > > It's not the !can_ff part I'm trying to highlight, it's the lack of > > advice *after* we have decided to flip the switch. > > > > As I said in another thread: I don't think we have any long > > condescending error in any other command. > > Only the "not a fast-forward and you must choose between merge and > rebase" part (i.e. what I listed as the first part of three-part > message) is the error. > > The rest, the message that teaches how to choose between merge and > rebase from command line and configuration (or how to choose > permanently not to make the choice between the two), is not an > error---it's called advice. Right. After re-reading the 2013 discussion, and the GitHub trainers' input that Jeff King shared with us [1], it might make sense to eventually keep this advice (permanently), since many users might not know what a rebase is, and would not want to know at this point of their learning curve. That being said, the advice still needs to be improved, and I have already yet another version taking this into consideration ready to be sent. Just waiting for feedback on the previous can_ff series. > Even if we were to introduce the third choice (i.e. permanently not > to choose between rebase or merge and instead just error out without > getting advice), the advice must stay for those who didn't make any > choice among the three (i.e. merge, rebase or ff-only). With the introduction of such mode, and the permanent advice, we would have: if (!can_ff) { if (!mode && opt_verbosity >= 0 && advice_pull_non_ff) show_advice_pull_non_ff(); if (mode == PULL_MODE_FF_ONLY) die(_("Not a fast-forward, either merge or rebase.\n")); } If in the future we decide to make it the default, we would have: if (!can_ff) { if (!mode && opt_verbosity >= 0 && advice_pull_non_ff) show_advice_pull_non_ff(); if (!mode || mode == PULL_MODE_FF_ONLY) die(_("Not a fast-forward, either merge or rebase.\n")); } The transition is very straightforward. > We have many of them. An easy example to spot is a similar sized > onefor "git checkout HEAD^0". Neither that one or the one under > discussion is particularly condescending. I am not familiar with these "advices", since I basically ignore them, and they are not presented to me in any different color, or special presentation. I generally ignore textwalls in my command line. But fine, if these walls of text already exist, perhaps it makes sense to have yet another permanent one (just with better text, and better options). With an improved advice--which includes an option that is good enough we *can* make the default (pull.mode=ff-only)--perhaps that would be enough, and in a couple of years we can make the assessment if it's enough or not. But clearly *today* we don't have such mode, which people have found missing for more than a decadte. Cheers. [1] https://lore.kernel.org/git/20130909201751.GA14437@sigill.intra.peff.net/
diff --git a/builtin/pull.c b/builtin/pull.c index 03e6d53243..ff8e3ce137 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -27,6 +27,8 @@ #include "commit-reach.h" #include "sequencer.h" +static int default_mode; + /** * Parses the value of --rebase. If value is a false value, returns * REBASE_FALSE. If value is a true value, returns REBASE_TRUE. If value is @@ -344,20 +346,7 @@ static enum rebase_type config_get_rebase(void) if (!git_config_get_value("pull.rebase", &value)) return parse_config_rebase("pull.rebase", value, 1); - if (opt_verbosity >= 0 && !opt_ff) { - advise(_("Pulling without specifying how to reconcile divergent branches is\n" - "discouraged. You can squelch this message by running one of the following\n" - "commands sometime before your next pull:\n" - "\n" - " git config pull.rebase false # merge (the default strategy)\n" - " git config pull.rebase true # rebase\n" - " git config pull.ff only # fast-forward only\n" - "\n" - "You can replace \"git config\" with \"git config --global\" to set a default\n" - "preference for all repositories. You can also pass --rebase, --no-rebase,\n" - "or --ff-only on the command line to override the configured default per\n" - "invocation.\n")); - } + default_mode = 1; return REBASE_FALSE; } @@ -1040,6 +1029,21 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (opt_rebase && merge_heads.nr > 1) die(_("Cannot rebase onto multiple branches.")); + if (default_mode && opt_verbosity >= 0 && !opt_ff) { + advise(_("Pulling without specifying how to reconcile divergent branches is\n" + "discouraged. You can squelch this message by running one of the following\n" + "commands sometime before your next pull:\n" + "\n" + " git config pull.rebase false # merge (the default strategy)\n" + " git config pull.rebase true # rebase\n" + " git config pull.ff only # fast-forward only\n" + "\n" + "You can replace \"git config\" with \"git config --global\" to set a default\n" + "preference for all repositories. You can also pass --rebase, --no-rebase,\n" + "or --ff-only on the command line to override the configured default per\n" + "invocation.\n")); + } + if (opt_rebase) { int ret = 0; int ran_ff = 0;
Eventually we want to be able to display the warning only when fast-forward merges are not possible. In order to do so we need to move the default warning up to the point where we can check if we can fast-forward or not. Additionally, config_get_rebase() was probably never its true home. This requires a temporary variable to check if we are in the "default mode" (no --rebase or --no-rebase specified). Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- builtin/pull.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)