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

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Date: Dec 11, 2019, 15:06 UTC
Message-ID: <9d77a425-da8d-d6e7-f51f-c4ce6ee988d5@syntevo.com>
In-Reply-To: <b9454df6-7d31-e255-84bd-8a1c548cffd7@gmail.com>

On 11.12.2019 15:27, Phillip Wood wrote:

Show 9 quoted lines

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

Show 6 quoted lines

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