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

From: Alexandr Miloslavskiy <alexandr.miloslavskiy@syntevo.com>
Date: Dec 11, 2019, 11:43 UTC
Message-ID: <2b573436-0ed2-9d24-f375-dfea0825a39e@syntevo.com>
In-Reply-To: <4401823b-8039-99b4-2436-ed2f1a571d78@gmail.com>

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.

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.

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

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.