Re: [PATCH 1/2] format-patch: fix dashdash usage

[Junio C Hamano <gitster@pobox.com>]

Junio C Hamano <gitster@pobox.com> writes:

> Felipe Contreras <felipe.contreras@gmail.com> writes:
> ...
>>> I actually have a bigger question, though.  Does it even make sense to
>>> allow pathspecs to format-patch?  We sure are currently loose and take
>>> them, but I doubt it is by design.
>>
>> Not everyone has clean branches only with pertinent patches.
>>
>> I stumbled upon this trying to re-create (cleanly) a "branch" that was
>> constantly merged into another "master" branch that had a lot more
>> stuff. Maybe there was a smarter way to do that with 'git rebase', but
>> that doesn't mean format-patch -- <path> shouldn't work.
>>
>>> The patch itself looks good and is a candidate 'maint' material, if the
>>> answer to the above question is a convincing "yes, because ...".
>>
>> Yeah, I also think this should go into 'maint'.
>
> Hmm, I have not seen a clear "yes, because..." yet.

Sorry, I guess I did it again of assuming the reader would read my mind.
Let's try to be a bit more explicit.

Your description defends why you think showing only part of a single
change in a patch form is jusitified.  What I found unconvincing is that
it does not even try to justify why it is a good idea to give the full
description that explains the _whole change_, even the part to the set of
files that were omitted by the pathspecs, as an applicable format-patch
output.  And that is why I suspect that it may be a long rope that harms
users.

> For one thing, Documentation/git-format-patch.txt does not even hint that
> you can give pathspecs.  builtin_format_patch_usage[] doesn't, either.  As
> I wrote the initial version of format-patch I can say with some authority
> that use with pathspecs were never meant to be supported---if it works, it
> works by accident, giving long enough rope to users to potentially cause
> themselves harm.
>
> I am inclined to think that we shouldn't encourage use of pathspecs (just
> like we never encouraged use of options like --name-only that never makes
> sense in the context of the command) but I am undecided if we also should
> forbid the use of pathspecs (just like we did for --name-only recently).

Compared to --name-only and friends that makes the output unapplicable by
"git am", a patch generated with pathspecs is worse.  The output will
apply cleanly and the user can choose not to bother or forget cleaning up
the log message from the resulting commits.

On the other hand, if the pathspec affected only the choice of commits but
the command is changed in such a way that patches were always generated
with --full-diff option, I can understand its usefulness in the use case
you described.  Instead of having to do "format-patch master..branch" and
then pick only the ones that are necessary by visual inspection, you would
run it to generate the ones that _might_ need to be applied by giving
pathspec to cover the files all relevant changes _must_ touch, only to cut
down the search space of your visual inspection of picking and choosing.

Then at least the ones that you choose to apply are expressed in full and
the patch text and the description should match, and I wouldn't find it
problematic nor a long rope that harms users.

But without such an explanation, I can only _guess_ what the intention
is.  That is why I asked for justifications from you, as this was your
itch.