Junio C Hamano <gitster@xxxxxxxxx> writes: > > Lidong Yan <yldhome2d2@xxxxxxxxx> writes: > >> Git uses `rebase.autostash` or `merge.autostash` to determine whether a >> dirty worktree is allowed during pull. However, this behavior is not >> clearly documented, making it difficult for users to discover how to >> enable autostash, or causing them to unknowingly enable it. Add new >> config option `pull.autostash` along with its documentation and test >> cases. >> >> `pull.autostash` provides the same functionality as `rebase.autostash` >> and `merge.autostash`, but overrides them when set. If `pull.autostash` >> is not set, it falls back to `rebase.autostash` or `merge.autostash`, >> depending on the value of `pull.rebase`. > > Very well reasoned and described. > >> diff --git a/Documentation/config/pull.adoc b/Documentation/config/pull.adoc >> index 9349e09261..3aa1e67923 100644 >> --- a/Documentation/config/pull.adoc >> +++ b/Documentation/config/pull.adoc >> @@ -13,6 +13,17 @@ pull.rebase:: >> of merging the default branch from the default remote when "git >> pull" is run. See "branch.<name>.rebase" for setting this on a >> per-branch basis. >> + >> +pull.autoStash:: >> + When true, Git will automatically perform a `git stash` before the >> + operation and then restore the local changes with `git stash pop` >> + after the pull is complete. This means that you can run pull on a >> + dirty worktree. If `pull.autostash` is set, it takes precedence over >> + `rebase.autostash` and `merge.autostash`. If `pull.autostash` is not >> + set, it falls back to `rebase.autostash` or `merge.autostash`, >> + depending on the value of `pull.rebase`. This option can be >> + overridden by the `--no-autostash` and `--autostash` options of >> + linkgit:git-pull[1]. Defaults to false. >> + >> When `merges` (or just 'm'), pass the `--rebase-merges` option to 'git rebase' >> so that the local merge commits are included in the rebase (see > > The new text is inserted at a wrong spot. This "+\nWhen `merges`" > is a continuation of the text that describes `pull.rebase`. If that > is set to `true`, one thing happens. If that is set to `merges`, > something else happens. > > Insert the text for `pull.autoStash` immediately before the > description of the `pull.octopus` configuration variable. > > As to the text itself, "you can run pull on a dirty worktree" may > not be what you want to say here, for a few reasons. > > * (pedantic) Even without the configuration variable set, you can > run "git pull" in a dirty working tree; it just will refuse to do > any damage until you stash the local changes away yourself. > > * If your "git pull" merges, it would work even in a dirty working > tree as long as your local change doesn't overlap with what the > merge would bring in. This is quite useful for a maintainer with > "upcoming" change to GIT-VERSION-GEN always updated locally in > the working tree and not having to worry about pulling from > contributors and submaintainers who won't usually be touching > that file, for example. > > * Not limited to this instance, when you have to say "(This|It) > means <<B>>" immediately after making a statement <<A>, I would > like us to think if we can just say <<B>> without saying <<A> at > all. In this case, it is not so, which makes me suspect that > perhaps we do not even want to say <<B>>, as it may not mean > <<B>> after all. > > Here is my attempt. > > When set to true, automatically create a temporary stash entry > to record the local changes before the operation begins, and > restore them after the operation completes. When your "git > pull" rebases (instead of merges), this may be convenient, since > unlike merging pull that tolerates local changes that do not > interfere with the merge, rebasing pull refuses to work with any > local changes. > + > If `pull.autostash` is set (either to true or false), > `merge.autostash` and `rebase.autostash` are ignored. If > `pull.autostash` is not set at all, depending on the value of > `pull.rebase`, `merge.autostash` or `rebase.autostash` is used > instead. Can be overridden by the `--[no-]autostash` command line > option. > >> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh >> index 63c9a8f04b..134da2185c 100755 >> --- a/t/t5520-pull.sh >> +++ b/t/t5520-pull.sh >> @@ -472,6 +472,96 @@ test_expect_success 'pull --no-autostash & merge.autostash unset' ' >> test_pull_autostash_fail --no-autostash --no-rebase >> ' >> >> +test_expect_success 'pull succeeds with dirty working directory and pull.autostash set' ' >> + test_config pull.autostash true && >> + test_pull_autostash 1 --rebase && >> + test_pull_autostash 2 --no-rebase >> +' > > Most trivial case. No command line override. > >> +test_expect_success 'pull --autostash & pull.autostash=true' ' >> + test_config pull.autostash true && >> + test_pull_autostash 1 --autostash --rebase && >> + test_pull_autostash 2 --autostash --no-rebase >> +' > > Command line override specifies the same behaviour as the > configuration, so we cannot learn much from this test. It still > should keep working, so the test is worth having [*], but I wonder > if makes sense to combine the above two into one test, i.e. set the > configuration variable to true once, and then try --rebase and > --no-rebase with and without --autostash (four combinations). > > [*] In this review, unless I explicitly say "this test is wrong > and expects an incorrect result", they are not wrong, even > though what they test may not be as interesting as others, and I > am not suggesting its removal. This is one of these tests. > >> +test_expect_success 'pull --autostash & pull.autostash=false' ' >> + test_config pull.autostash false && >> + test_pull_autostash 1 --autostash --rebase && >> + test_pull_autostash 2 --autostash --no-rebase >> +' > > Configuration should be overridden by the command line option, which > is a good thing to test. > >> +test_expect_success 'pull --autostash & pull.autostash unset' ' >> + test_unconfig pull.autostash && >> + test_pull_autostash 1 --autostash --rebase && >> + test_pull_autostash 2 --autostash --no-rebase >> +' > > Another most trivial case. Shouldn't we already have an existing > test for this, back from the days before pull.autostash got > introduced, since the command line option has been there all along? > >> +test_expect_success 'pull --no-autostash & pull.autostash=true' ' >> + test_config pull.autostash true && >> + test_pull_autostash_fail --no-autostash --rebase && >> + test_pull_autostash_fail --no-autostash --no-rebase >> +' > > Configuration overridden by the option, opposite of what we saw > earlier, which is another good thing to test. > >> +test_expect_success 'pull --no-autostash & pull.autostash=false' ' >> + test_config pull.autostash false && >> + test_pull_autostash_fail --no-autostash --rebase && >> + test_pull_autostash_fail --no-autostash --no-rebase >> +' > > Uninteresting test that does not tell us much; we cannot tell which > between the configuration and the command line option caused us not > to auto stash with this test. > > Two cases that may be worth adding to this test immediately after > setting pull.autostash to false are: > > test_pull_autostash_fail --rebase && > test_pull_autostash_fail --no-rebase && > >> +test_expect_success 'pull --no-autostash & pull.autostash unset' ' >> + test_unconfig pull.autostash && >> + test_pull_autostash_fail --no-autostash --rebase && >> + test_pull_autostash_fail --no-autostash --no-rebase >> +' > > Another uninteresting case that probably should be already covered > by existing test, since this tests "what happens when autostash is > explicitly declined from the command line when there is no > configuration variable to intervene?". > >> +test_expect_success 'pull.autostash=true & rebase.autostash=true' ' >> + test_config pull.autostash true && >> + test_config rebase.autostash true && >> + test_pull_autostash 1 --rebase >> +' > > OK. Perhaps make sure "--no-autostash --rebase" would fail while at > it in the same test? > >> +test_expect_success 'pull.autostash=true & rebase.autostash=false' ' >> + test_config pull.autostash true && >> + test_config rebase.autostash false && >> + test_pull_autostash 1 --rebase >> +' > > This is more interesting than the previous one, as we make sure that > pull.* trumps rebase.* with this test. Perhaps throw --no-autostash > specified on the command line into the mix? > >> +test_expect_success 'pull.autostash=false & rebase.autostash=true' ' >> + test_config pull.autostash false && >> + test_config rebase.autostash true && >> + test_pull_autostash_fail --rebase >> +' > > Another good one. It might be intereseting to test --no-rebase and > make sure it also fails? I dunno. > >> +test_expect_success 'pull.autostash=false & rebase.autostash=false' ' >> + test_config pull.autostash false && >> + test_config rebase.autostash false && >> + test_pull_autostash_fail --rebase >> +' > > Not as interesting as others. > >> +test_expect_success 'pull.autostash=true & merge.autostash=true' ' >> + test_config pull.autostash true && >> + test_config merge.autostash true && >> + test_pull_autostash 2 --no-rebase >> +' > > Not as interesting as others. Throw --no-autostash given on the > command line into the mix as well? > >> +test_expect_success 'pull.autostash=true & merge.autostash=false' ' >> + test_config pull.autostash true && >> + test_config merge.autostash false && >> + test_pull_autostash 2 --no-rebase >> +' > > OK. pull.*=true trumps merge.*=false. We test the other way around > next. Good. > >> +test_expect_success 'pull.autostash=false & merge.autostash=true' ' >> + test_config pull.autostash false && >> + test_config merge.autostash true && >> + test_pull_autostash_fail --no-rebase >> +' >> + >> +test_expect_success 'pull.autostash=false & merge.autostash=false' ' >> + test_config pull.autostash false && >> + test_config merge.autostash false && >> + test_pull_autostash_fail --no-rebase >> +' > > Not very interesting. Throw anothre that gives --autostash from the > command line in the mix, perhaps? > >> test_expect_success 'pull.rebase' ' >> git reset --hard before-rebase && >> test_config pull.rebase true && Thank you very much for your thorough review. I will reorganize the documentation and test cases in v2. - Lidong
