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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Dec 11, 2019, 14:27 UTC
Message-ID: <b9454df6-7d31-e255-84bd-8a1c548cffd7@gmail.com>
In-Reply-To: <2b573436-0ed2-9d24-f375-dfea0825a39e@syntevo.com>

Hi Alexandr

On 11/12/2019 11:43, Alexandr Miloslavskiy wrote:

Show 11 quoted lines

> On 10.12.2019 11:42, Phillip Wood wrote:
>> I don't think it's so bad if the pathspec is cleaned up just after it 
>> is used,
>> the diff below applies on top of your patch - see what you think. The 
>> diff
>> also adds dies if --all is given with --pathspec-from-file which 
>> (together
>> with a test) would be worth adding to you series I think.
> 
> Unfortunately, your reply came too late, topic was cooking in pu/next 
> for a while and merged into master yesterday: c58ae96f.

Ah I thought it might be a bit late to fix up the patch, there could always be a follow patch though.

Show 9 quoted lines

> 
> I understand that your patch consists of two parts:
> 
> 1) Adding test for --all
> ------------------------
> I must admit that I overlooked that there was a similar test for 
> args-based pathspec. I will add this part in my next topic for 
> --pathspec-from-file. I expect it to appear in the next day or two. I 
> will try to remember to CC you to it.

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

Show 10 quoted lines

> 
> 2) Moving parsing/validation into `parse_and_validate_options()`
> ------------------------
> Again, I agree that having parsing/validation outside is suboptimal.
> However, with current code, I find it to be a choice between two evils, 
> and my choice was "outside but clear" to "inside but obscure".
> 
> 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.

Show 11 quoted lines

> 
> Now, let me illustrate. There's a similar case in my TODO list, this 
> time involving a real bug in git. In `init_db()`, the bug occurs in 
> `set_git_dir(real_path(git_dir))`, also due to unexpected clearing.
> 
> Now that I pointed my finger at it, please try to find what's wrong. I 
> imagine that it won't be easy at all. And it's way harder when there's 
> no reason to dig deep into a very specific line of code.
> 
> I really try to avoid such type of pitfalls. That's why my choice 
> between two evils is what I did.

If I had to hazard a guess I'd say that either set_git_dir() calls real_path() which messes up the path passed to it or it does not copy the path passed to it and it is messed up by some other code calling real_path() after set_git_dir() has returned. If my guess is correct (I wouldn't be surprised if it's wrong) 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.

Best Wishes

Phillip