Re: [PATCH] builtin/stash: configs keepIndex, includeUntracked

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Feb 20, 2024, 11:01 UTC
Message-ID: <78a8733b-c74a-4017-8905-d29b2e05adb1@gmail.com>
In-Reply-To: <df020dcc-afe8-4dd8-b476-4ca032206214@gmail.com>

Hi Ricardo

On 18/02/2024 17:54, Ricardo C wrote:

Show 15 quoted lines

> Hi Phillip,
> 
> On 2/18/24 05:32, Phillip Wood wrote:
>> How does "stash.keepIndex" interact with "git rebase --autostash" and 
>> "git merge --autostash"? I think both those commands expect a clean 
>> index after running "git stash". They could just override the config 
>> setting but it might get a bit confusing if some commands respect the 
>> config and others don't.
> 
> Both `git rebase --autostash` and `git merge --autostash` seem to be 
> hardcoded to clean the index, regardless of the configuration or CLI 
> flags. They do not use regular `git stash` to do so, but rather `git 
> stash create`. This is unaffected by my changes, since it follows a 
> different code path and does not accept `--keep-index` nor 
> `--include-untracked`.

Ah, I'd forgotten that we hand "git stash create" as well as "git stash push". It's good that these changes do not affect "git stash create" but I think Junio is right to be concerned about the effect on tools that are using "git stash push" where they arguably ought to be using "git stash create".

> I'll add some tests for `git rebase --autostash` 
> and `git merge --autostash`, just in case this behavior is changed in 
> the future and causes breakage.

That's a good idea but I think it would be better to test that "git stash create" is not affected by the config as we don't want to change the behavior of its behavior.

Show 16 quoted lines

>> I've only given the patch a very quick scan, but it looked sensible. 
>> The only thing that jumped out at me was that quite a few tests seem 
>> to do
>>
>>      git init repo &&
>>      (
>>          cd repo &&
>>          # test things
>>      ) &&
>>
>> Our normal practice is to run all the tests in the same file in the 
>> same repository rather than setting up a new one each time.
> 
> I was doing this because it makes comparing different commands easier, 
> but looking through other tests again, it seems like I should be 
> comparing the outputs to hardcoded files anyway.

Yes, that is our usual practice.

Best Wishes

Phillip