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

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

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

Show 16 quoted lines

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

Show 15 quoted lines

>> 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");