[PATCH 0/2] format-patch: fix dashdash

[PATCH 0/2] format-patch: fix dashdash

[Felipe Contreras]

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

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

[Felipe Contreras]

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

diff --git a/builtin-log.c b/builtin-log.c
index 33fa6ea..1766349 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -976,7 +976,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	 */
 	argc = parse_options(argc, argv, prefix, builtin_format_patch_options,
 			     builtin_format_patch_usage,
-			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN);
+			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
+			     PARSE_OPT_KEEP_DASHDASH);
 
 	if (do_signoff) {
 		const char *committer;
-- 
1.6.6.rc0.61.geeb75

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

[Junio C Hamano]

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

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

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

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

[Felipe Contreras]

On Thu, Nov 26, 2009 at 9:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Otherwise 'git format-patch <committish> -- <non-existent-path>' doesn't
>> work.
>
> 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().

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

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

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

-- 
Felipe Contreras

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

[Junio C Hamano]

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

> On Thu, Nov 26, 2009 at 9:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> Otherwise 'git format-patch <committish> -- <non-existent-path>' doesn't
>>> work.
>>
>> 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().
>
> Complaining is one thing... failing to do anything is another.

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?

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

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

diff --git a/builtin-log.c b/builtin-log.c
index 33fa6ea..3a9bc69 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -1036,6 +1036,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	argc = setup_revisions(argc, argv, &rev, "HEAD");
 	if (argc > 1)
 		die ("unrecognized argument: %s", argv[1]);
+	if (rev.prune_data)
+		die("unexpected pathspec");
 
 	if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
 		die("--name-only does not make sense");

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

[Felipe Contreras]

On Fri, Nov 27, 2009 at 1:11 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Is it clearer what I meant?  More importantly, did I get the details
> right?

Yeah, I guess so.

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

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

That's why revision filtering options make sense.

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

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

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

[Junio C Hamano]

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

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

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.

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

[Felipe Contreras]

On Fri, Nov 27, 2009 at 2:03 AM, Junio C Hamano <gitster@pobox.com> wrote:
>  (0) take your patch with an updated message (eh, that is not "next step"
>     but the "first step");

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

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

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

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

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

Cheers.

-- 
Felipe Contreras

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

[Junio C Hamano]

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.

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

[Björn Steinbrink]

On 2009.11.26 15:11:27 -0800, Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> > On Thu, Nov 26, 2009 at 9:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >> 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.
> 
> 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).

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

[PATCH 2/2] format-patch: add test for dashdash

[Felipe Contreras]

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

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 7f267f9..d5b002d 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -536,6 +536,10 @@ test_expect_success 'format-patch --signoff' '
 	grep "^Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
 '
 
+test_expect_success 'format-patch -- <path>' '
+	git format-patch master..side -- file
+'
+
 echo "fatal: --name-only does not make sense" > expect.name-only
 echo "fatal: --name-status does not make sense" > expect.name-status
 echo "fatal: --check does not make sense" > expect.check
-- 
1.6.6.rc0.61.geeb75