Re : 2 questions on git-send-email usage

Re : 2 questions on git-send-email usage

[moreau francis]

moreau francis wrote:
> (please let me CCed when replying)
>  
>  2006/7/10, Jakub Narebski <jnareb@gmail.com>:
>  > moreau francis wrote:
>  > 
>  > > I'm wondering what am I supposed to answer when git-send-email
>  > > is asking me :
>  > >
>  > > Message-ID to be used as In-Reply-To for the first email?
>  > >
>  > > I'm running this command:
>  > >
>  > > $ git-send-email --no-signed-off-by-cc --no-chain-reply-to --to \
>  > >   foo@bar.com --compose /tmp/patch/
>  > >
>  > > to write an introductory message, and all patches are sent as replies to
>  > > this introductory email sent.
>  > 
>  > Empty string (i.e. RET) should do if you don't want to attach your series of
>  > patches somewhere in existing thread.
>  
>  ok I'll try
>  
>  --in-reply-to ""

ok it works. But wouldn't it make more sense to have by default --in-reply-to ""
when --compose is set ? That would mean "by default all patches are sent
as replies to the email I'm composing" which is usely what happens, no ?

>  
>  > 
>  > > I also noticed that git-send-email removes the commit message of each
>  > > patches I sent, I don't think this is the normal behaviour though. What
>  > > am I missing ?
>  > 
>  > Are patches formatted using git-format-patch?
>  > 
>  
>  yes
>  

I think I have found out a clue. The commit message and Signed-off-by are
missing because the header patches are formatted like this:

>From 90df31ca209f85108976d18916f33f352a6ef340 Mon Sep 17 00:00:00 2001
From: Francis <francis.moreau2000@yahoo.fr>
Date: Thu, 8 Jun 2006 09:51:12 +0200
Subject: [PATCH 3/4] step #3: interrupt implementation
(cherry picked from 427778e2e622cdefa2c834edcc19bf102a35bc2d commit)
(cherry picked from fe4692336801fcbb42bb734bb6b6f9c041d63087 commit)
Signed-off-by: Francis <francis_moreau2000@yahoo.fr>
---

2 RETs is missing. One after the Subject line and the other before the 
Signed-off-by line. If I add the first missing RET, all works fine.  I guess
it's missing because of git-cherry-pick command. But I don't understand
why the last RET is missing

Can anybody tell me why ?

Thanks

Francis

Re: Re : 2 questions on git-send-email usage

[Franck Bui-Huu]

moreau francis wrote:
> 2 RETs is missing. One after the Subject line and the other before the 
> Signed-off-by line. If I add the first missing RET, all works fine.  I guess
> it's missing because of git-cherry-pick command. But I don't understand
> why the last RET is missing
> 
> Can anybody tell me why ?
> 

Maybe that patch does what you want.

-- >8 --

Subject: [PATCH] Add a newline before appending "Signed-off-by:"

It looks nicer.

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 log-tree.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index ebb49f2..2551a3f 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -19,7 +19,7 @@ static int append_signoff(char *buf, int
 	char *cp = buf;
 
 	/* Do we have enough space to add it? */
-	if (buf_sz - at <= strlen(signed_off_by) + signoff_len + 2)
+	if (buf_sz - at <= strlen(signed_off_by) + signoff_len + 3)
 		return at;
 
 	/* First see if we already have the sign-off by the signer */
@@ -34,6 +34,7 @@ static int append_signoff(char *buf, int
 			return at; /* we already have him */
 	}
 
+	buf[at++] = '\n';
 	strcpy(buf + at, signed_off_by);
 	at += strlen(signed_off_by);
 	strcpy(buf + at, signoff);
-- 
1.4.1.g35c6-dirty

Re: Re : 2 questions on git-send-email usage

[Junio C Hamano]

Franck Bui-Huu <vagabon.xyz@gmail.com> writes:

> Maybe that patch does what you want.
>
> -- >8 --
>
> Subject: [PATCH] Add a newline before appending "Signed-off-by:"
>
> It looks nicer.
>
> Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>

Haven't checked the code around the patch yet, but does it work
when the original commit log message ends with a blank line and
existing signed-off-by lines by other people?  You do not want
an extra blank lines there.

Re: Re : 2 questions on git-send-email usage

[Franck Bui-Huu]

Junio C Hamano wrote:
> 
> Haven't checked the code around the patch yet, but does it work
> when the original commit log message ends with a blank line and
> existing signed-off-by lines by other people?  You do not want
> an extra blank lines there.
> 

argh, no I just tested the previous case. Here is an update
which fix all cases.

-- >8 --

[PATCH] Add a newline before appending "Signed-off-by:"

It looks nicer.

Signed-off-by: Franck Bui-Huu <vagabon.xyz@gmail.com>
---
 log-tree.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 9d8d46f..69d5c8a 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -17,9 +17,10 @@ static int append_signoff(char *buf, int
 	int signoff_len = strlen(signoff);
 	static const char signed_off_by[] = "Signed-off-by: ";
 	char *cp = buf;
+	int has_signoff = 0;
 
 	/* Do we have enough space to add it? */
-	if (buf_sz - at <= strlen(signed_off_by) + signoff_len + 2)
+	if (buf_sz - at <= strlen(signed_off_by) + signoff_len + 3)
 		return at;
 
 	/* First see if we already have the sign-off by the signer */
@@ -32,8 +33,11 @@ static int append_signoff(char *buf, int
 		    !strncmp(cp, signoff, signoff_len) &&
 		    isspace(cp[signoff_len]))
 			return at; /* we already have him */
+		has_signoff = 1;
 	}
 
+	if (!has_signoff)
+		buf[at++] = '\n';
 	strcpy(buf + at, signed_off_by);
 	at += strlen(signed_off_by);
 	strcpy(buf + at, signoff);
-- 
1.4.1.g55b7

Re: Re : 2 questions on git-send-email usage

[Linus Torvalds]

On Wed, 12 Jul 2006, Franck Bui-Huu wrote:
> 
> [PATCH] Add a newline before appending "Signed-off-by:"
> 
> It looks nicer.

Yes. However, I think the sign-off detection is a bit broken (quite 
independently of your patch).

A number of people end up capitalizing the sign-off differently, so you 
have lines like "Signed-Off-By: Xy Zzy <xyzzy@example.org>".

Also, at least for the kernel, we often have alternative formats, like

	Acked-by: Elliot Xavier Ample <example@dummy.org>

and for that case, adding the extra newline is actually bad.

So I would suggest a totally different approach: instead of using 
"strstr(comments, signed_off_by)", it would probably be much better to 
just look for the last non-empty line, and see if it matches the format

	"^[nonspace]*: .*@.*$"

(yeah, that's not a valid regexp, but you get the idea).

On a slightly related note, I absolutely _hate_ how cherry-picking adds 
"(cherry-picked from commit <sha1>)" at the end. It's wrong for so many 
reasons, one of them being that it then breaks things like this, but the 
main one being that <sha1> will quite often actually end up not even 
_existing_ in the resulting archive (you cherry-picked from your private 
branch, and even if you keep your branch, you don't necessarily push it 
out).

Junio, can we make the default _not_ to do it, please?

			Linus

Re: Re : 2 questions on git-send-email usage

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On a slightly related note, I absolutely _hate_ how cherry-picking adds 
> "(cherry-picked from commit <sha1>)" at the end. It's wrong for so many 
> reasons, one of them being that it then breaks things like this, but the 
> main one being that <sha1> will quite often actually end up not even 
> _existing_ in the resulting archive (you cherry-picked from your private 
> branch, and even if you keep your branch, you don't necessarily push it 
> out).
>
> Junio, can we make the default _not_ to do it, please?

I understand that it can be annoying.

I was hoping that however when you do something like this:

	git log --filter-backported-commits v2.6.16.9..v2.6.17

an improved log-inspection tool could notice and filter out the
ones that was backported from the mainline to the maintenance
branch.

But I realize that is probably a faulty logic.  First of all,
not all backports are cherry-picked (the same patch can be
applied from an e-mail, for example), so if we really want to do
the above filtering, we would need to be able to detect the same
patch anyway without "cherry-picked from" information.

To a certain degree, git-patch-id would help detecting such a
duplicated patch, but it would not help you detect "moral
equivalent" changes that are textually different.  A cherry-pick
after a conflict resolution that ends up applying a textually
different patch would still leave the "cherry-picked from"
message in the commit log (or a "note " header if we implement
it to reduce cluttering the log message), which would be the
only advantage of recording the information somehow.  But that
is probably not worth it -- it means "moral equivalent" changes
need to be recorded somehow by hand, which is unnecessarly
developer burden if it is rare enough to want to filter such
duplicates when inspecting the log.

Do people find "cherry-picked from" information useful?  Does
anybody mind if we change the default not to record it?

Re: Re : 2 questions on git-send-email usage

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> Yes. However, I think the sign-off detection is a bit broken (quite 
> independently of your patch).
>
> A number of people end up capitalizing the sign-off differently, so you 
> have lines like "Signed-Off-By: Xy Zzy <xyzzy@example.org>".
>
> Also, at least for the kernel, we often have alternative formats, like
>
> 	Acked-by: Elliot Xavier Ample <example@dummy.org>
>
> and for that case, adding the extra newline is actually bad.
>
> So I would suggest a totally different approach: instead of using 
> "strstr(comments, signed_off_by)", it would probably be much better to 
> just look for the last non-empty line, and see if it matches the format
>
> 	"^[nonspace]*: .*@.*$"

Documentation/SubmittingPatches (the kernel one) does not show
the ugly Camel-Case-With-Hyphen spelling, and I've been wondring
why people do that.  A hidden agenda by me was to migrate people
away from that practice, but that is an independent issue ;-).

I like your "detect lines that looks like a RFC2822 header that
has some e-mail address" approach quite a lot.

Re: Re : 2 questions on git-send-email usage

[Linus Torvalds]

On Wed, 12 Jul 2006, Junio C Hamano wrote:

> Linus Torvalds <torvalds@osdl.org> writes:
> >
> > A number of people end up capitalizing the sign-off differently, so you 
> > have lines like "Signed-Off-By: Xy Zzy <xyzzy@example.org>".
> 
> Documentation/SubmittingPatches (the kernel one) does not show
> the ugly Camel-Case-With-Hyphen spelling, and I've been wondring
> why people do that.

Yeah, I actually try to edit it to the "proper" format when I notice it 
(which is not most of the time, but it's pretty rare to begin with).

More commonly, people mistype their own email addresses, and 
_occasionally_ just mis-type the whole Signed-off-by: line (we've got a 
few semi-colons instead of colons in the kernel, for example, and some 
lines that are missing the final '>' in the email etc.

So being somewhat forgiving might help, but I think another thing that 
migth help is a flag to "git-am" to _not_ apply a patch that lacks a 
previous sign-off.

I, for example, don't use the --signoff flag, partly because I want to 
make sure that I sign of only on patches that already have a sign-off from 
the previous person when it comes as email (or I add the sign-off only 
after looking at the patch closely). But if there was a 
"--error-on-no-signoff" flag, I could use it.

			Linus

Re: Re : 2 questions on git-send-email usage

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> So being somewhat forgiving might help, but I think another thing that 
> migth help is a flag to "git-am" to _not_ apply a patch that lacks a 
> previous sign-off.

How about having this in $GIT_DIR/hooks/applypatch-msg?

	#!/bin/sh
	grep '^Signed-off-by: ' "$1" >/dev/null

Re: Re : 2 questions on git-send-email usage

[Linus Torvalds]

On Wed, 12 Jul 2006, Junio C Hamano wrote:
> 
> How about having this in $GIT_DIR/hooks/applypatch-msg?
> 
> 	#!/bin/sh
> 	grep '^Signed-off-by: ' "$1" >/dev/null

Not good, because then I have no way to select a different behaviour with 
a flag. If I decide it was ok to apply (say, it's just a silly typo fix), 
I would want to say so.

		Linus