Skip to content

Commit

Permalink
pull: warn if the user didn't say whether to rebase or to merge
Browse files Browse the repository at this point in the history 
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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
@alexhenrie @gitster
alexhenrie authored and gitster committed Mar 10, 2020
1 parent 2d2118b commit d18c950
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 11 deletions.
16 changes: 16 additions & 0 deletions builtin/pull.c
View file
Expand Up @@ -327,6 +327,22 @@ 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 || 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;
}

Expand Down
22 changes: 11 additions & 11 deletions t/t5521-pull-options.sh
View file
Expand Up @@ -11,10 +11,10 @@ test_expect_success 'setup' '
git commit -m one)
'

test_expect_success 'git pull -q' '
test_expect_success 'git pull -q --no-rebase' '
mkdir clonedq &&
(cd clonedq && git init &&
git pull -q "../parent" >out 2>err &&
git pull -q --no-rebase "../parent" >out 2>err &&
test_must_be_empty err &&
test_must_be_empty out)
'
Expand All @@ -30,10 +30,10 @@ test_expect_success 'git pull -q --rebase' '
test_must_be_empty out)
'

test_expect_success 'git pull' '
test_expect_success 'git pull --no-rebase' '
mkdir cloned &&
(cd cloned && git init &&
git pull "../parent" >out 2>err &&
git pull --no-rebase "../parent" >out 2>err &&
test -s err &&
test_must_be_empty out)
'
Expand All @@ -46,10 +46,10 @@ test_expect_success 'git pull --rebase' '
test_must_be_empty out)
'

test_expect_success 'git pull -v' '
test_expect_success 'git pull -v --no-rebase' '
mkdir clonedv &&
(cd clonedv && git init &&
git pull -v "../parent" >out 2>err &&
git pull -v --no-rebase "../parent" >out 2>err &&
test -s err &&
test_must_be_empty out)
'
Expand All @@ -62,25 +62,25 @@ test_expect_success 'git pull -v --rebase' '
test_must_be_empty out)
'

test_expect_success 'git pull -v -q' '
test_expect_success 'git pull -v -q --no-rebase' '
mkdir clonedvq &&
(cd clonedvq && git init &&
git pull -v -q "../parent" >out 2>err &&
git pull -v -q --no-rebase "../parent" >out 2>err &&
test_must_be_empty out &&
test_must_be_empty err)
'

test_expect_success 'git pull -q -v' '
test_expect_success 'git pull -q -v --no-rebase' '
mkdir clonedqv &&
(cd clonedqv && git init &&
git pull -q -v "../parent" >out 2>err &&
git pull -q -v --no-rebase "../parent" >out 2>err &&
test_must_be_empty out &&
test -s err)
'
test_expect_success 'git pull --cleanup errors early on invalid argument' '
mkdir clonedcleanup &&
(cd clonedcleanup && git init &&
test_must_fail git pull --cleanup invalid "../parent" >out 2>err &&
test_must_fail git pull --no-rebase --cleanup invalid "../parent" >out 2>err &&
test_must_be_empty out &&
test -s err)
'
Expand Down
38 changes: 38 additions & 0 deletions t/t7601-merge-pull-config.sh
View file
Expand Up @@ -27,6 +27,44 @@ test_expect_success 'setup' '
git tag c3
'

test_expect_success 'pull.rebase not set' '
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=false' '
git reset --hard c0 &&
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 &&
test_config pull.ff only &&
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 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 pull --no-rebase . 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 &&
test_i18ngrep ! "Pulling without specifying how to reconcile" err
'

test_expect_success 'merge c1 with c2' '
git reset --hard c1 &&
test -f c0.c &&
Expand Down

4 comments on commit d18c950

@RoestVrijStaal
Copy link

@RoestVrijStaal RoestVrijStaal commented on d18c950 Aug 27, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for confusing everyone instead of just doing the default strategy when no parameter is given.

Imagine when every (CLI) application warns the user about not having each default value within the configuration file or given as parameter, despite the user is OK with (or does not care about) the default behaviour.

@dscho
Copy link
Member

@dscho dscho commented on d18c950 Aug 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RoestVrijStaal if you really want to complain, do note that this is the wrong forum for it, and that the Git mailing list is the correct forum for it. And if you truly care about this issue, I would highly recommend trying to phrase it in a way that is more likely to elicit a constructive discussion. The way you wrote it, your comment puts the very people in the defensive that you hope to convince to fix this.

@RoestVrijStaal
Copy link

@RoestVrijStaal RoestVrijStaal commented on d18c950 Oct 5, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dscho

  1. The harm is already done.
  2. Also if this twisted "solution" is the decision what came from that bunch of people, I estimate they are too hardboiled to convince.
  3. I've bad memories about using mailing lists, e.g. nobody replies and email address is public visible.

@dscho
Copy link
Member

@dscho dscho commented on d18c950 Oct 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what the point is. It's the wrong location to accomplish anything, it's not constructive, the choice of words seems designed to cause emotional distress more than anything else. If you want to convince your readers that you're right, there are probably better ways to do so.

Please sign in to comment.