Subject: Re: [PATCH] pull: add pull.autoStash config option

From: Lidong Yan <[email-masked]>
Date: Sat, 19 Jul 2025 11:14:48 +0800

Junio C Hamano <[email-masked]> writes:
> 
> Lidong Yan <[email-masked]> 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