| Message ID | 20201204061623.1170745-8-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: > > There's no need to display the annoying warning on every pull... only > the ones that are not fast-forward. I like this. :-) > This requires the tests to pick another base, so the merge is not > fast-forward. > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > Documentation/git-pull.txt | 3 +++ > builtin/pull.c | 2 +- > t/t7601-merge-pull-config.sh | 28 +++++++++++++++++----------- > 3 files changed, 21 insertions(+), 12 deletions(-) > > diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt > index dc812139f4..ad33d2472c 100644 > --- a/Documentation/git-pull.txt > +++ b/Documentation/git-pull.txt > @@ -60,6 +60,9 @@ However, a non-fast-foward case looks very different. > origin/master in your repository > ------------ > > +By default `git pull` will warn about these situations, however, most likely > +you would want to force a merge, which you can do with `git pull --no-rebase`. > + Everything in this sentence after the first comma seems dangerous to me for a number of workflows. > Then "`git pull`" will fetch and replay the changes from the remote > `master` branch since it diverged from the local `master` (i.e., `E`) > until its current commit (`C`) on top of `master` and record the > diff --git a/builtin/pull.c b/builtin/pull.c > index f82e214fc8..f0b1c6bfea 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -1013,7 +1013,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > > can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]); > > - if (default_mode && opt_verbosity >= 0 && !opt_ff) { > + if (default_mode && !can_ff && opt_verbosity >= 0 && !opt_ff) { > advise(_("Pulling without specifying how to reconcile divergent branches is\n" > "discouraged; you need to specify if you want a merge, or a rebase.\n" > "You can squelch this message by running one of the following commands:\n" > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh > index 6774e9d86f..6b4adab8b1 100755 > --- a/t/t7601-merge-pull-config.sh > +++ b/t/t7601-merge-pull-config.sh > @@ -28,7 +28,7 @@ test_expect_success 'setup' ' > ' > > test_expect_success 'pull.rebase not set' ' > - git reset --hard c0 && > + git reset --hard c2 && > git -c color.advice=always pull . c1 2>err && > test_decode_color <err >decoded && > test_i18ngrep "<YELLOW>hint: " decoded && > @@ -36,54 +36,60 @@ test_expect_success 'pull.rebase not set' ' > > ' > > -test_expect_success 'pull.rebase not set and pull.ff=true' ' > +test_expect_success 'pull.rebase not set (fast-forward)' ' > git reset --hard c0 && > + git pull . c1 2>err && > + test_i18ngrep ! "Pulling without specifying how to reconcile" err > +' > + > +test_expect_success 'pull.rebase not set and pull.ff=true' ' > + git reset --hard c2 && > test_config pull.ff true && > git pull . c1 2>err && > test_i18ngrep ! "Pulling without specifying how to reconcile" err > ' > > test_expect_success 'pull.rebase not set and pull.ff=false' ' > - git reset --hard c0 && > + git reset --hard c2 && > test_config pull.ff false && > git pull . c1 2>err && > test_i18ngrep ! "Pulling without specifying how to reconcile" err > ' > > test_expect_success 'pull.rebase not set and pull.ff=only' ' > - git reset --hard c0 && > + git reset --hard c2 && > test_config pull.ff only && > - git pull . c1 2>err && > + test_must_fail git pull . c1 2>err && It makes sense to me that you'd need test_must_fail here...but why wasn't it needed before?? How did this test pass without it before this series? > test_i18ngrep ! "Pulling without specifying how to reconcile" err > ' > > test_expect_success 'pull.rebase not set and --rebase given' ' > - git reset --hard c0 && > + git reset --hard c2 && > git pull --rebase . c1 2>err && > test_i18ngrep ! "Pulling without specifying how to reconcile" err > ' > > test_expect_success 'pull.rebase not set and --no-rebase given' ' > - git reset --hard c0 && > + git reset --hard c2 && > git pull --no-rebase . c1 2>err && > test_i18ngrep ! "Pulling without specifying how to reconcile" err > ' > > test_expect_success 'pull.rebase not set and --ff given' ' > - git reset --hard c0 && > + git reset --hard c2 && > git pull --ff . c1 2>err && > test_i18ngrep ! "Pulling without specifying how to reconcile" err > ' > > test_expect_success 'pull.rebase not set and --no-ff given' ' > - git reset --hard c0 && > + git reset --hard c2 && > git pull --no-ff . c1 2>err && > test_i18ngrep ! "Pulling without specifying how to reconcile" err > ' > > test_expect_success 'pull.rebase not set and --ff-only given' ' > - git reset --hard c0 && > - git pull --ff-only . c1 2>err && > + git reset --hard c2 && > + test_must_fail git pull --ff-only . c1 2>err && Same question; how did this test pass before without the test_must_fail?? > test_i18ngrep ! "Pulling without specifying how to reconcile" err > ' > > -- > 2.29.2
On Fri, Dec 4, 2020 at 5:24 PM Elijah Newren <newren@gmail.com> wrote: > > On Thu, Dec 3, 2020 at 10:16 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > > > There's no need to display the annoying warning on every pull... only > > the ones that are not fast-forward. > > I like this. :-) > > > This requires the tests to pick another base, so the merge is not > > fast-forward. > > > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > > --- > > Documentation/git-pull.txt | 3 +++ > > builtin/pull.c | 2 +- > > t/t7601-merge-pull-config.sh | 28 +++++++++++++++++----------- > > 3 files changed, 21 insertions(+), 12 deletions(-) > > > > diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt > > index dc812139f4..ad33d2472c 100644 > > --- a/Documentation/git-pull.txt > > +++ b/Documentation/git-pull.txt > > @@ -60,6 +60,9 @@ However, a non-fast-foward case looks very different. > > origin/master in your repository > > ------------ > > > > +By default `git pull` will warn about these situations, however, most likely > > +you would want to force a merge, which you can do with `git pull --no-rebase`. > > + > > Everything in this sentence after the first comma seems dangerous to > me for a number of workflows. And this is precisely the reason why the default of "git pull" must be changed to --ff-only (or similar). But today "git pull" by default is the equivalent of "git pull --merge". We need to explain to users why the warning is there, and that "git pull" is doing an implicit "git pull --merge", and if they do it explicitly they can get rid of the warning. Later on when the default is changed to --ff-only this text can be updated to something less dangerous. But not explaining what "git pull" is doing today by default will not make it any less dangerous. > > Then "`git pull`" will fetch and replay the changes from the remote > > `master` branch since it diverged from the local `master` (i.e., `E`) > > until its current commit (`C`) on top of `master` and record the > > diff --git a/builtin/pull.c b/builtin/pull.c > > index f82e214fc8..f0b1c6bfea 100644 > > --- a/builtin/pull.c > > +++ b/builtin/pull.c > > @@ -1013,7 +1013,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > > > > can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]); > > > > - if (default_mode && opt_verbosity >= 0 && !opt_ff) { > > + if (default_mode && !can_ff && opt_verbosity >= 0 && !opt_ff) { > > advise(_("Pulling without specifying how to reconcile divergent branches is\n" > > "discouraged; you need to specify if you want a merge, or a rebase.\n" > > "You can squelch this message by running one of the following commands:\n" > > diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh > > index 6774e9d86f..6b4adab8b1 100755 > > --- a/t/t7601-merge-pull-config.sh > > +++ b/t/t7601-merge-pull-config.sh > > @@ -28,7 +28,7 @@ test_expect_success 'setup' ' > > ' > > > > test_expect_success 'pull.rebase not set' ' > > - git reset --hard c0 && > > + git reset --hard c2 && > > git -c color.advice=always pull . c1 2>err && > > test_decode_color <err >decoded && > > test_i18ngrep "<YELLOW>hint: " decoded && > > @@ -36,54 +36,60 @@ test_expect_success 'pull.rebase not set' ' > > > > ' > > > > -test_expect_success 'pull.rebase not set and pull.ff=true' ' > > +test_expect_success 'pull.rebase not set (fast-forward)' ' > > git reset --hard c0 && > > + git pull . c1 2>err && > > + test_i18ngrep ! "Pulling without specifying how to reconcile" err > > +' > > + > > +test_expect_success 'pull.rebase not set and pull.ff=true' ' > > + git reset --hard c2 && > > test_config pull.ff true && > > git pull . c1 2>err && > > test_i18ngrep ! "Pulling without specifying how to reconcile" err > > ' > > > > test_expect_success 'pull.rebase not set and pull.ff=false' ' > > - git reset --hard c0 && > > + git reset --hard c2 && > > test_config pull.ff false && > > git pull . c1 2>err && > > test_i18ngrep ! "Pulling without specifying how to reconcile" err > > ' > > > > test_expect_success 'pull.rebase not set and pull.ff=only' ' > > - git reset --hard c0 && > > + git reset --hard c2 && > > test_config pull.ff only && > > - git pull . c1 2>err && > > + test_must_fail git pull . c1 2>err && > > It makes sense to me that you'd need test_must_fail here...but why > wasn't it needed before?? How did this test pass without it before > this series? Because before it was a fast-forward (from c0), not so anymore (from c2); "pull.ff=only" fails if it's not a fast-forward. Cheers.
diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt index dc812139f4..ad33d2472c 100644 --- a/Documentation/git-pull.txt +++ b/Documentation/git-pull.txt @@ -60,6 +60,9 @@ However, a non-fast-foward case looks very different. origin/master in your repository ------------ +By default `git pull` will warn about these situations, however, most likely +you would want to force a merge, which you can do with `git pull --no-rebase`. + Then "`git pull`" will fetch and replay the changes from the remote `master` branch since it diverged from the local `master` (i.e., `E`) until its current commit (`C`) on top of `master` and record the diff --git a/builtin/pull.c b/builtin/pull.c index f82e214fc8..f0b1c6bfea 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -1013,7 +1013,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) can_ff = get_can_ff(&orig_head, &merge_heads.oid[0]); - if (default_mode && opt_verbosity >= 0 && !opt_ff) { + if (default_mode && !can_ff && opt_verbosity >= 0 && !opt_ff) { advise(_("Pulling without specifying how to reconcile divergent branches is\n" "discouraged; you need to specify if you want a merge, or a rebase.\n" "You can squelch this message by running one of the following commands:\n" diff --git a/t/t7601-merge-pull-config.sh b/t/t7601-merge-pull-config.sh index 6774e9d86f..6b4adab8b1 100755 --- a/t/t7601-merge-pull-config.sh +++ b/t/t7601-merge-pull-config.sh @@ -28,7 +28,7 @@ test_expect_success 'setup' ' ' test_expect_success 'pull.rebase not set' ' - git reset --hard c0 && + git reset --hard c2 && git -c color.advice=always pull . c1 2>err && test_decode_color <err >decoded && test_i18ngrep "<YELLOW>hint: " decoded && @@ -36,54 +36,60 @@ test_expect_success 'pull.rebase not set' ' ' -test_expect_success 'pull.rebase not set and pull.ff=true' ' +test_expect_success 'pull.rebase not set (fast-forward)' ' git reset --hard c0 && + git pull . c1 2>err && + test_i18ngrep ! "Pulling without specifying how to reconcile" err +' + +test_expect_success 'pull.rebase not set and pull.ff=true' ' + git reset --hard c2 && test_config pull.ff true && git pull . c1 2>err && test_i18ngrep ! "Pulling without specifying how to reconcile" err ' test_expect_success 'pull.rebase not set and pull.ff=false' ' - git reset --hard c0 && + git reset --hard c2 && test_config pull.ff false && git pull . c1 2>err && test_i18ngrep ! "Pulling without specifying how to reconcile" err ' test_expect_success 'pull.rebase not set and pull.ff=only' ' - git reset --hard c0 && + git reset --hard c2 && test_config pull.ff only && - git pull . c1 2>err && + test_must_fail git pull . c1 2>err && test_i18ngrep ! "Pulling without specifying how to reconcile" err ' test_expect_success 'pull.rebase not set and --rebase given' ' - git reset --hard c0 && + git reset --hard c2 && git pull --rebase . c1 2>err && test_i18ngrep ! "Pulling without specifying how to reconcile" err ' test_expect_success 'pull.rebase not set and --no-rebase given' ' - git reset --hard c0 && + git reset --hard c2 && git pull --no-rebase . c1 2>err && test_i18ngrep ! "Pulling without specifying how to reconcile" err ' test_expect_success 'pull.rebase not set and --ff given' ' - git reset --hard c0 && + git reset --hard c2 && git pull --ff . c1 2>err && test_i18ngrep ! "Pulling without specifying how to reconcile" err ' test_expect_success 'pull.rebase not set and --no-ff given' ' - git reset --hard c0 && + git reset --hard c2 && git pull --no-ff . c1 2>err && test_i18ngrep ! "Pulling without specifying how to reconcile" err ' test_expect_success 'pull.rebase not set and --ff-only given' ' - git reset --hard c0 && - git pull --ff-only . c1 2>err && + git reset --hard c2 && + test_must_fail git pull --ff-only . c1 2>err && test_i18ngrep ! "Pulling without specifying how to reconcile" err '
There's no need to display the annoying warning on every pull... only the ones that are not fast-forward. This requires the tests to pick another base, so the merge is not fast-forward. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- Documentation/git-pull.txt | 3 +++ builtin/pull.c | 2 +- t/t7601-merge-pull-config.sh | 28 +++++++++++++++++----------- 3 files changed, 21 insertions(+), 12 deletions(-)