Re: [PATCH 5/5] commit: support the --pathspec-from-file option

[Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>]

On 11.12.2019 15:27, Phillip Wood wrote:

> Thanks, one other thing I forgot to mention yesterday is to ask what the 
> expected behavior is if the user passes an empty file to 
> --pathspec-from-file. With no pathspecs on the command line commit, 
> checkout, reset and restore-files all default to operating on all paths 
> but passing --pathspec-from-file implies the user wants to specify 
> specific paths so I think it would perhaps be better to error out if no 
> paths are given. There is a precedent for this in checkout-index which 
> does nothing if no paths are given (though I can't remember if it errors 
> out or not).

Back when I composed patch for `git commit`, I spent some time thinking 
about this question. I agree that empty `--pathspec-from-file` is weird. 
Eventually I decided that `--pathspec-from-file` shall be as close as 
possible to passing pathspec args. So empty file should behave the same 
way as passing zero pathspec args. It's more or less the question "if 
user does something unexpected, what is expected?". I decided that 
treating as zero args doesn't sound dangerous to me, and it's a very 
weird case anyway, so let's just treat as zero args.

>> What I find obscure in your suggestion/patch is that innocently 
>> looking `prepare_index()` suddenly clears pathspec as well. It's even 
>> harder to see when called through `dry_run_commit()`.
> 
> It would be easy enough to clear pathspec in cmd_commit() I just didn't 
> bother to do it.

Yes, this thought also came to me after I replied. I agree that this is 
viable and seems to solve most problems.

Now I'm in the situation where before me, code already wasn't perfect, 
and I can continue to extend it in the same direction, or try to 
refactor to make it better. I actually choose the second path: [1][2]. 
These were two "don't" lessons for me, as both topics now gather dust in 
mail graveyard. No good deed remains unpunished, I guess?

> I think that's a bit different to 
> the pathspec case as it's about the lifetime of the return value of a 
> function rather than a function freeing an argument passed to it.

Both cases are about lifetime that ends where it was not expected. You 
can probably agree that it never makes code easier to understand, and at 
best it will be properly accounted for and does not explode. Again, both 
choices were somewhat evil.

[1] 
https://public-inbox.org/git/pull.479.git.1574969538.gitgitgadget@gmail.com/
[2] 
https://public-inbox.org/git/pull.477.git.1574848137.gitgitgadget@gmail.com/T/#u