Hi Alexandr
On 11/12/2019 11:43, Alexandr Miloslavskiy wrote:
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.
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).
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.
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
