patch, 2 partsformat-patch: fix dashdash

This doesn't work git format-patch <committish> -- <path not in working dir>

So here are some fixes and a test.

Felipe Contreras (2):
  format-patch: fix dashdash usage
  format-patch: add test for dashdash

 builtin-log.c           |    3 ++-
 t/t4014-format-patch.sh |    4 ++++
 2 files changed, 6 insertions(+), 1 deletions(-)

Otherwise 'git format-patch <committish> -- <non-existent-path>' doesn't work.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin-log.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Felipe Contreras <felipe.contreras@gmail.com> writes:

Instead of "doesn't work", I really wished you wrote something like:

    $ git format-patch <commit> -- <path>

    complains that <path> does not exist in the current work tree and the
    user needs to explicitly specify "--", even though the user _did_ give
    a "--".  This is because it incorrectly removes "--" from the command
    line arguments that is later passed to setup_revisions().

Remember that you are trying to help somebody who has to write Release Notes out of "git log" output.

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.

The patch itself looks good and is a candidate 'maint' material, if the answer to the above question is a convincing "yes, because ...".

On Thu, Nov 26, 2009 at 9:57 PM, Junio C Hamano <gitster@pobox.com> wrote:

Complaining is one thing... failing to do anything is another.

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.

Yeah, I also think this should go into 'maint'.

Felipe Contreras <felipe.contreras@gmail.com> writes:

Oh, I didn't mean to say "tone down from doesn't-work to complains".

    Instead of "doesn't work" and saying nothing else useful to describe
    the nature of the problem you are addressing, I really wished you
    wrote something that has enough details like the sample explaination I
    gave you has.

Is it clearer what I meant? More importantly, did I get the details right?

Hmm, I have not seen a clear "yes, because..." yet.

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

An appropriate patch that should go to 'maint' _might_ even be something like this in addition to your patch, if we decide to be consistent.

---
 builtin-log.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

On Fri, Nov 27, 2009 at 1:11 AM, Junio C Hamano <gitster@pobox.com> wrote:

Yeah, I guess so.

I'll repeat: Not everyone has clean branches only with pertinent patches.

That's why revision filtering options make sense.

How about 'git format-patch --full-diff'? Isn't that a valid way to filter patches just like --author, --grep, and so on?

Felipe Contreras <felipe.contreras@gmail.com> writes:

Our messages obviously crossed and I think we are in agreement that pathspec that only is used to pick which commit to show and not limits which parts of the chosen commits are shown might have some uses.

In any case, your patch we are discussing, with a proper commit log message (without discussing if it is a good idea to give pathspecs) would be a good first step, regardless of which direction we end up going as the next step.

As to the "next step", my current thinking, unless there are convincing arguments why there should be a way to also limit the parts of the commits are shown, is to

 (0) take your patch with an updated message (eh, that is not "next step"
     but the "first step");

 (1) make --full-diff implicit and default of format-patch (--no-full-diff
     could be supported _if_ somebody can argue successfully why limiting
     the diff is also a useful thing to do); and

 (2) document clearly that format-patch takes optional pathspecs, and in
     what situation they are useful.

I think (0) is 'maint' material, and with a good documentation update (1) and (2) could also be.

On Fri, Nov 27, 2009 at 2:03 AM, Junio C Hamano <gitster@pobox.com> wrote:

I guess it depends of what is the "fist step". To me the "first step" is realizing there's a problem. First patch submission is already many steps afterwards.

But sure, I'll do this next step :)

I started writing this patch but it turns out there's no --no-full-diff.

I guess this would have to be done in multiple places so it might take me some time.

Cheers.

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

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.

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.

On 2009.11.26 15:11:27 -0800, Junio C Hamano wrote:

A year ago, there was someone who had done a subtree merge and had commits that changed the subtree in the "supertree" branch. He wanted to generate patches to send them to upstream, and ended up using format-patch with --relative and pathspecs.

http://thread.gmane.org/gmane.comp.version-control.git/101742

I guess this could be done by some "git rebase -s subtree ..." invocation though, to first get commits that sit directly on the subtree branch, and then you could turn them into patches as usual... Hmm..

Björn

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t4014-format-patch.sh |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)