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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Ricardo

On 18/02/2024 17:54, Ricardo C wrote:
> 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.

>> 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