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

From: Pierre Habouzit <madcoder@debian.org>
Date: Nov 5, 2008, 10:40 UTC
Message-ID: <20081105104001.GA22272@artemis.corp>
In-Reply-To: <7viqr2mz75.fsf@gitster.siamese.dyndns.org>

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

> Pierre Habouzit <madcoder@debian.org> writes:

Show 25 quoted lines

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

Show 16 quoted lines

> Pierre Habouzit <madcoder@debian.org> writes:
> 
> > +	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.

Show 14 quoted lines

> With the ability to give --cover-letter option to underlying format-patch
> do you still need this?
> 
> > +	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:

Show 10 quoted lines

> Jeff King <peff@peff.net> writes:
> > 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