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@xxxxxxxxx/
[2]
https://public-inbox.org/git/pull.477.git.1574848137.gitgitgadget@xxxxxxxxx/T/#u
