Re: [PATCH 2/5] git send-email: interpret unknown files as revision lists

On Tue, Nov 04, 2008 at 11:54:26PM +0000, Junio C Hamano wrote:

Pierre Habouzit [off-list ref] writes:

quoted

@@ -363,10 +366,22 @@ if (@alias_files and $aliasfiletype and defined $parse_alias{$aliasfiletype}) {
 
 ($sender) = expand_aliases($sender) if defined $sender;
 
+sub check_file_rev_conflict($) {
+	my $f = shift;
+	if ($repo->command('rev-parse', '--verify', '--quiet', $f)) {
+		die("revision/filename conflict on `$f'");

Perhaps wording this a bit more to the point?  This is triggered when 
'$f' can be both a filename or a revision, so...

	File '$f' exists but it could also be the range of commits
        to produce patches for.  Please disambiguate by...

	* Saying "./$f" if you mean a file; or
        * Giving -F option if you mean a range.

Earlier I suggested that "origin^0" is a way for the user to disambiguate
favouring a rev, but such a filename can exist, so we cannot blindly
suggest to say "$f^0" here.  I think adding -F (or --format-patch) option
to send-email to explicitly disable file/directory interpretation would be
a cleaner solution for this (and it would allow you to drive this from a
script without worrying about what garbage files you happen to have in the
working tree).

Okay, still having --[no-]format-patch is probably a good idea indeed
for scripts. Will do.

On Tue, Nov 04, 2008 at 11:54:39PM +0000, Junio C Hamano wrote:

Pierre Habouzit [off-list ref] writes:

quoted

+	print C <<EOT;
+From $tpl_sender # This line is ignored.
+GIT: Lines beginning in "GIT: " will be removed.
+GIT: Consider including an overall diffstat or table of contents
+GIT: for the patch you are writing.
+GIT:
+GIT: Clear the body content if you don't wish to send a summary.
+From: $tpl_sender
+Subject: $tpl_subject
+In-Reply-To: $tpl_reply_to
+

Somebody already suggested this but I really think GIT: lines should be at
the end and use '# ' prefix instead.

This will break previous editor syntax hilighting stuff even more, and
has the drawback that you can't put shell sniplets in here. I think it's
why GIT: was chosen. But maybe we just don't care.

With the ability to give --cover-letter option to underlying format-patch
do you still need this?

quoted

+	my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+
+	if ($annotate) {
+		do_edit($compose_filename, @files);
+	} else {
+		do_edit($compose_filename);
+	}

Don't we want to abort the whole process when the user kills the editor
instead of normal exit (iow, do_edit() which is system() reports that the
editor was killed)?

Probably, I kept what was done as is, but we probably want do_edit() to
die() if the user killed it.

On mer, nov 05, 2008 at 07:03:42 +0000, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

On Tue, Nov 04, 2008 at 03:54:54PM -0800, Junio C Hamano wrote:

Yeah, if send-email did not have --compose to begin with, we could just
say "don't use --compose; use --cover-letter when you use send-email to
front-end format-patch instead", but some people perhaps are used to run
format-patch separately without --cover-letter and then create the cover
letter from scratch with --compose (which seems a bit more work to me,
though).

So I am not opposed to a sendemail.foo configuration option.

Will do


-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org