Re: [PATCH] git-send-email.perl: fix In-Reply-To for second and subsequent patches

From: Antonio Ospite <ospite@studenti.unina.it>
Date: Oct 15, 2010, 07:56 UTC
Message-ID: <20101015095651.b75c4b54.ospite@studenti.unina.it>
In-Reply-To: <20101014182250.GA18341@burratino>

On Thu, 14 Oct 2010 13:22:50 -0500 Jonathan Nieder <jrnieder@gmail.com> wrote:

> (+cc: some send-email people)
>

For the new recipients, the original mail is here btw: http://permalink.gmane.org/gmane.comp.version-control.git/159039

More comments below.

Show 11 quoted lines

> Hi,
> 
> Antonio Ospite wrote:
> 
> > Make second and subsequent patches appear as replies to the first patch,
> > even when an initial In-Reply-To is supplied
> [...]
> > Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
> 
> Thanks.
>

Thanks for commenting Jonathan.

Show 10 quoted lines

> >   - When $initial_reply_to is asked to the user, it is asked as the
> >     "Message-ID to be used as In-Reply-To for the _first_ email", this
> >     makes me think that the second and subsequent patches are not using
> >     it
> 
> This kind of justification belongs in the commit message, no?
> That way, we can save future readers the trouble of figuring out
> the rationale all over again when considering future changes to this
> code.
>

Ok, I can add this in the commit message, I am waiting some days for v2, in case someone else has more to say.

Show 12 quoted lines

> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -1313,7 +1313,7 @@ foreach my $t (@files) {
> >  
> >  	# set up for the next message
> >  	if ($thread && $message_was_sent &&
> > -		(chain_reply_to() || !defined $reply_to || length($reply_to) == 0)) {
> > +		($message_num == 1 || chain_reply_to() || !defined $reply_to || length($reply_to) == 0)) {
> >  		$reply_to = $message_id;
> 
> Would it be possible to break this long line?
>

I like the OR chain on the same line, but I can split it anyways if that's the preference.

> If you're feeling particularly adventurous, it would be nice to add a
> test for the changed functionality to t/t9001-send-email.sh, so we
> don't break it with other changes in the future.
>

No promises, but I might give that a try.

Show 6 quoted lines

> I haven't looked too deeply or even tried running applying the patch,
> but generally it looks good to me.
> 
> Ciao,
> Jonathan
>

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

PGP public key ID: 0x4553B001

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?