| Message ID | 20200304022931.2469455-1-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] pull: warn if the user didn't say whether to rebase or to merge | expand |
Alex Henrie <alexhenrie24@gmail.com> writes: > - Revise warning message based on Junio's feedback > - Consistently wrap warning lines to 75 characters for easy viewing in > PO files Nice to see attention to such a detail ;-) > - Fix test failures Ah, OK, hmmm. For --quiet test, that wants to ensure that "pull --quiet" does not say anything, it certainly stops the test from failing if we set the configuration before executing such a test, but I wonder if that is in line with the spirit of the feature the test tries to protect in the first place. I would imagine those who write "pull --quiet" in automation would not want to see any non-error message, and because this is not an error, they do not want to see any output. Shouldn't such a use of "pull --quiet" bypass this warning, too? > --- > builtin/pull.c | 15 +++++++++++++++ > t/t5521-pull-options.sh | 3 ++- > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/builtin/pull.c b/builtin/pull.c > index 3e624d1e00..351b933c4d 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -327,6 +327,21 @@ 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_ff || strcmp(opt_ff, "--ff-only")) { If we want to squelch the warning under "--quiet", I think we can do this. if (0 < opt_verbosity && (!opt_ff || strcmp(opt_ff, "--ff-only"))) { and ... > + warning(_("Pulling without specifying how to reconcile divergent branches is\n" > ... > diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh > index ccde8ba491..6e890ec936 100755 > --- a/t/t5521-pull-options.sh > +++ b/t/t5521-pull-options.sh > @@ -8,7 +8,8 @@ test_expect_success 'setup' ' > mkdir parent && > (cd parent && git init && > echo one >file && git add file && > - git commit -m one) > + git commit -m one) && > + git config pull.rebase false ... this change can safely go. If we agree that "git pull --quiet" should stay quiet in an environment where the user has been happy with the default choice, that is. I am raising this issue to invite others to think about it, and I am on the fence, but I am leaning towards saying "yes". > ' Regardless of what the resolution for "pull --quiet" would be, shouldn't we have a test for this change to ensure that we do warn under the condition we should, and that we do not do so when we should not? Thanks.
Junio C Hamano <gitster@pobox.com> writes: > > Alex Henrie <alexhenrie24@gmail.com> writes: > >> - Revise warning message based on Junio's feedback >> - Consistently wrap warning lines to 75 characters for easy viewing in >> PO files > > Nice to see attention to such a detail ;-) > >> - Fix test failures > > Ah, OK, hmmm. > > For --quiet test, that wants to ensure that "pull --quiet" does not > say anything, it certainly stops the test from failing if we set the > configuration before executing such a test, but I wonder if that is > in line with the spirit of the feature the test tries to protect in > the first place. I would imagine those who write "pull --quiet" in > automation would not want to see any non-error message, and because > this is not an error, they do not want to see any output. Shouldn't > such a use of "pull --quiet" bypass this warning, too? Actually, the change to t/t5521 does not even sweep the problem under the rug X-< >> diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh >> index ccde8ba491..6e890ec936 100755 >> --- a/t/t5521-pull-options.sh >> +++ b/t/t5521-pull-options.sh >> @@ -8,7 +8,8 @@ test_expect_success 'setup' ' >> mkdir parent && >> (cd parent && git init && >> echo one >file && git add file && >> - git commit -m one) >> + git commit -m one) && >> + git config pull.rebase false This affects the repository at the top of the trash directory, but the test immediately after this one looks like this: test_expect_success 'git pull -q' ' mkdir clonedq && (cd clonedq && git init && git pull -q "../parent" >out 2>err && test_must_be_empty err && test_must_be_empty out) ' The patch added pull.rebase to "t/trash directory.5521/.git/config", but it would not affect this "git pull -q", that is run in the repository that uses "t/trash directory.5521/clonedq/.git/config" as its configuration. For now, I added the following patch on top of the topic before queuing it to 'pu'. Thanks. -- >8 -- Subject: [PATCH] SQUASH??? Let's declare that for the purpose of "pull --quiet", the new "you might have created a merge when you may wanted to rebase" warning is an unwanted noise. Revert the hack made to t5521; the second test that creates a new repository and runs a pull inside it will not get affected by the local configuration setting it makes to the original trash repository and the breakage won't be worked around with it anyways. This is not a full remedy for the previous step, yet. The topic needs a test to ensure the warning is emitted when it should be. Existing t5521 is a test that ensures the warning is not given in one of the conditions it should not be (i.e. when --quiet is in effect). Negative tests for other cases (e.g. when --ff-only is given from the command line, without pull.rebase configured) should also be added. --- builtin/pull.c | 3 ++- t/t5521-pull-options.sh | 3 +-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index 351b933c4d..0ef192fd64 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -327,7 +327,8 @@ 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_ff || strcmp(opt_ff, "--ff-only")) { + if (0 <= opt_verbosity && + (!opt_ff || strcmp(opt_ff, "--ff-only"))) { warning(_("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" diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh index 6e890ec936..ccde8ba491 100755 --- a/t/t5521-pull-options.sh +++ b/t/t5521-pull-options.sh @@ -8,8 +8,7 @@ test_expect_success 'setup' ' mkdir parent && (cd parent && git init && echo one >file && git add file && - git commit -m one) && - git config pull.rebase false + git commit -m one) ' test_expect_success 'git pull -q' '
diff --git a/builtin/pull.c b/builtin/pull.c index 3e624d1e00..351b933c4d 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -327,6 +327,21 @@ 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_ff || strcmp(opt_ff, "--ff-only")) { + warning(_("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")); + } + return REBASE_FALSE; } diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh index ccde8ba491..6e890ec936 100755 --- a/t/t5521-pull-options.sh +++ b/t/t5521-pull-options.sh @@ -8,7 +8,8 @@ test_expect_success 'setup' ' mkdir parent && (cd parent && git init && echo one >file && git add file && - git commit -m one) + git commit -m one) && + git config pull.rebase false ' test_expect_success 'git pull -q' '
Often novice Git users forget to say "pull --rebase" and end up with an unnecessary merge from upstream. What they usually want is either "pull --rebase" in the simpler cases, or "pull --ff-only" to update the copy of main integration branches, and rebase their work separately. The pull.rebase configuration variable exists to help them in the simpler cases, but there is no mechanism to make these users aware of it. Issue a warning message when no --[no-]rebase option from the command line and no pull.rebase configuration variable is given. This will inconvenience those who never want to "pull --rebase", who haven't had to do anything special, but the cost of the inconvenience is paid only once per user, which should be a reasonable cost to help a number of new users. Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- v4: - Revise warning message based on Junio's feedback - Consistently wrap warning lines to 75 characters for easy viewing in PO files - Fix test failures --- builtin/pull.c | 15 +++++++++++++++ t/t5521-pull-options.sh | 3 ++- 2 files changed, 17 insertions(+), 1 deletion(-)