threads / discuss / 4848

Re : 2 questions on git-send-email usage

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

## tl;dr

10 messages between Jul 11, 2006 and Jul 13, 2006.

replies: 9people: 4as markdown or json

moreau francis· Jul 11, 2006, 08:46 UTC · lore
moreau francis wrote:
Show 24 quoted lines
> (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 ?

Show 11 quoted lines
>  
>  > 
>  > > 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
Franck Bui-Huu· Jul 11, 2006, 10:08 UTC · re: moreau francis · lore

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

moreau francis wrote:
Show 7 quoted lines
> 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
Junio C Hamano· Jul 11, 2006, 19:22 UTC · re: Franck Bui-Huu · lore

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

Franck Bui-Huu <vagabon.xyz@gmail.com> writes:
Show 9 quoted lines
> 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.

Franck Bui-Huu· Jul 12, 2006, 07:37 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 6 quoted lines
> 
> 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
Linus Torvalds· Jul 12, 2006, 15:43 UTC · re: Franck Bui-Huu · lore

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

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
Junio C Hamano· Jul 12, 2006, 16:19 UTC · re: Linus Torvalds · lore

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

Linus Torvalds <torvalds@osdl.org> writes:
Show 9 quoted lines
> 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?

Junio C Hamano· Jul 12, 2006, 16:24 UTC · re: Linus Torvalds · lore

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

Linus Torvalds <torvalds@osdl.org> writes:
Show 17 quoted lines
> 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.

Linus Torvalds· Jul 12, 2006, 16:37 UTC · re: Junio C Hamano · lore

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

On Wed, 12 Jul 2006, Junio C Hamano wrote:
Show 8 quoted lines
> 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
Junio C Hamano· Jul 13, 2006, 04:34 UTC · re: Linus Torvalds · lore

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

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
Linus Torvalds· Jul 13, 2006, 04:40 UTC · re: Junio C Hamano · lore

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

On Wed, 12 Jul 2006, Junio C Hamano wrote:
Show 5 quoted lines
> 
> 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

← back to recent threads