Felipe Contreras <felipe.contreras@xxxxxxxxx> writes:
> On Thu, Nov 26, 2009 at 9:57 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Felipe Contreras <felipe.contreras@xxxxxxxxx> 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");
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html