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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 26, 2009, 23:29 UTC
Message-ID: <7vws1c27ch.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7v7htc3mqo.fsf@alter.siamese.dyndns.org>

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

Show 19 quoted lines

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

Show 11 quoted lines

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