format-patch signoff argument no longer works

format-patch signoff argument no longer works

[Geoff Russell]

Hi,

The --signoff argument no longer works in git-format-patch.  Was this
intentional?
It still appears in the documentation for the command.

It appears to have got lost when the shell script got converted to C.

Cheers,
Geoff Russell

Re: format-patch signoff argument no longer works

[Matthias Kestenholz]

* Geoff Russell (geoffrey.russell@gmail.com) wrote:
> The --signoff argument no longer works in git-format-patch.  Was this
> intentional?
> It still appears in the documentation for the command.
> 
> It appears to have got lost when the shell script got converted to C.
> 

Yes, this was intentional. You should sign off your changes while
committing (git commit -s|--signoff)

Thanks,
Matthias

Re: format-patch signoff argument no longer works

[Seth Falcon]

Matthias Kestenholz <lists@spinlock.ch> writes:

> * Geoff Russell (geoffrey.russell@gmail.com) wrote:
>> It appears to have got lost when the shell script got converted to C.
>> 
> Yes, this was intentional. You should sign off your changes while
> committing (git commit -s|--signoff)

When should one commit _without_ signoff?  

The obvious answer is: when one doesn't approve of the changes in the
commit... But in my usual workflow, commit means
works-for-me-I-think-it-is-good. :-)


Also, here's a trivial patch to the git-format-patch doc.  I recently
had the same confusion trying to get git-format-patch to add signoff
for me...

Remove reference to signoff option (-s) in git-format-patch

Signed-off-by: Seth Falcon <sethfalcon@gmail.com>
---
 Documentation/git-format-patch.txt |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 7cc7faf..d13f463 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -9,7 +9,7 @@ git-format-patch - Prepare patches for e
 SYNOPSIS
 --------
 [verse]
-'git-format-patch' [-n | -k] [-o <dir> | --stdout] [--attach] [-s] [-c]
+'git-format-patch' [-n | -k] [-o <dir> | --stdout] [--attach] [-c]
                 [--diff-options] <his> [<mine>]
 
 DESCRIPTION
@@ -44,10 +44,6 @@ OPTIONS
        Do not strip/add '[PATCH]' from the first line of the
        commit log message.
 
--s|--signoff::
-       Add `Signed-off-by:` line to the commit message, using
-       the committer identity of yourself.
-
 -c|--check::
         Display suspicious lines in the patch.  The definition
         of 'suspicious lines' is currently the lines that has
-- 
1.3.3.gb931

Re: format-patch signoff argument no longer works

[Johannes Schindelin]

Hi,

On Wed, 31 May 2006, Seth Falcon wrote:

> Matthias Kestenholz <lists@spinlock.ch> writes:
> 
> > * Geoff Russell (geoffrey.russell@gmail.com) wrote:
> >> It appears to have got lost when the shell script got converted to C.
> >> 
> > Yes, this was intentional. You should sign off your changes while
> > committing (git commit -s|--signoff)
> 
> When should one commit _without_ signoff?  
> 
> The obvious answer is: when one doesn't approve of the changes in the
> commit... But in my usual workflow, commit means
> works-for-me-I-think-it-is-good. :-)

Well, there are often times when I commit something to a throw-away 
branch, where I do not want to sign it off.

That was my original incentive to add this option BTW: I usually clean up 
a patch series in a cycle of tests and fixes, and only want to sign off on 
the final version.

If people need this option, I'll implement it, so speak up!

Ciao,
Dscho

[PATCH] Update documentation for git-format-patch

[Dennis Stosberg]

Signed-off-by: Dennis Stosberg <dennis@stosberg.net>
---
This updates the documentation for git-format-patch to reflect the
changes that the built-in version brought.

In addition to the functional changes, I reworded a few expressions
which sounded suspicious or unclear to me.  However, I'm not a
native English speaker, so corrections are welcome.

Is there any "patch" program at all that understands git's rename
patches?

Regards,
Dennis


 Documentation/git-format-patch.txt |   82 +++++++++++++++---------------------
 1 files changed, 35 insertions(+), 47 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 7cc7faf..64d3f1e 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -9,70 +9,58 @@ git-format-patch - Prepare patches for e
 SYNOPSIS
 --------
 [verse]
-'git-format-patch' [-n | -k] [-o <dir> | --stdout] [--attach] [-s] [-c]
-		 [--diff-options] <his> [<mine>]
+'git-format-patch' [-n | -k] [-o <dir> | --stdout] [--attach]
+	           [--diff-options] [--start-number <n>]
+		   <since>[..<until>]
 
 DESCRIPTION
 -----------
-Prepare each commit with its patch since <mine> head forked from
-<his> head, one file per patch formatted to resemble UNIX mailbox
-format, for e-mail submission or use with gitlink:git-am[1].
+
+Prepare each commit between <since> and <until> with its patch in
+one file per commit, formatted to resemble UNIX mailbox format.
+If ..<until> is not specified, the head of the current working
+tree is implied.
+
+The output of this command is convenient for e-mail submission or
+for use with gitlink:git-am[1].
 
 Each output file is numbered sequentially from 1, and uses the
-first line of the commit message (massaged for pathname safety)
-as the filename.
+first line of the commit message (checked for pathname safety) as
+the filename. The names of the output files are printed to standard
+output, unless the --stdout option is specified.
 
-When -o is specified, output files are created in <dir>; otherwise
-they are created in the current working directory.  This option
-is ignored if --stdout is specified.
+If -o is specified, output files are created in <dir>.  Otherwise
+they are created in the current working directory.
 
-When -n is specified, instead of "[PATCH] Subject", the first
-line is formatted as "[PATCH N/M] Subject", unless you have only
-one patch.
+If -n is specified, instead of "[PATCH] Subject", the first line
+is formatted as "[PATCH n/m] Subject".
 
 
 OPTIONS
 -------
 -o|--output-directory <dir>::
 	Use <dir> to store the resulting files, instead of the
-	current working directory.
+	current working directory. This option is ignored if
+	--stdout is specified.
 
 -n|--numbered::
 	Name output in '[PATCH n/m]' format.
 
+--start-number <n>::
+	Start numbering the patches with <n> instead of 1.
+
 -k|--keep-subject::
 	Do not strip/add '[PATCH]' from the first line of the
 	commit log message.
 
--s|--signoff::
-	Add `Signed-off-by:` line to the commit message, using
-	the committer identity of yourself.
-
--c|--check::
-        Display suspicious lines in the patch.  The definition
-        of 'suspicious lines' is currently the lines that has
-        trailing whitespaces, and the lines whose indentation
-        has a SP character immediately followed by a TAB
-        character.
-
 --stdout::
-	This flag generates the mbox formatted output to the
-	standard output, instead of saving them into a file per
-	patch and implies --mbox.
+	Print all commits to the standard output in mbox format,
+	instead of creating a file for each one.
 
 --attach::
 	Create attachments instead of inlining patches.
 
 
-CONFIGURATION
--------------
-You can specify extra mail header lines to be added to each
-message in the repository configuration as follows:
-
-[format]
-        headers = "Organization: git-foo\n"
-
-
 EXAMPLES
 --------
 
@@ -82,18 +70,18 @@ git-format-patch -k --stdout R1..R2 | gi
 	cherry-pick them.
 
 git-format-patch origin::
-	Extract commits the current branch accumulated since it
-	pulled from origin the last time in a patch form for
-	e-mail submission.
+	Extract all commits which are in the current branch but
+	not in the origin branch.  For each commit a separate file
+	is created in the current directory.
 
 git-format-patch -M -B origin::
-	The same as the previous one, except detect and handle
-	renames and complete rewrites intelligently to produce
-	renaming patch.  A renaming patch reduces the amount of
-	text output, and generally makes it easier to review
-	it.  Note that the "patch" program does not understand
-	renaming patch well, so use it only when you know the
-	recipient uses git to apply your patch.
+	The same as the previous one.  Additionally, it detects
+	and handles renames and complete rewrites intelligently to
+	produce a renaming patch.  A renaming patch reduces the
+	amount of text output, and generally makes it easier to
+	review it.  Note that the "patch" program does not
+	understand renaming patches, so use it only when you know
+	the recipient uses git to apply your patch.
 
 
 See Also
-- 
1.3.3+git20060531-dest1

Re: [PATCH] Update documentation for git-format-patch

[Johannes Schindelin]

Hi,

On Wed, 31 May 2006, Dennis Stosberg wrote:

> Is there any "patch" program at all that understands git's rename
> patches?

Why, yes: git-apply!

Ciao,
Dscho

Re: [PATCH] Update documentation for git-format-patch

[Jakub Narebski]

Dennis Stosberg wrote:

> --c|--check::
> -        Display suspicious lines in the patch.  The definition
> -        of 'suspicious lines' is currently the lines that has
> -        trailing whitespaces, and the lines whose indentation
> -        has a SP character immediately followed by a TAB
> -        character.

So is this option also lost in built-in git-format-patch?

> -CONFIGURATION
> --------------
> -You can specify extra mail header lines to be added to each
> -message in the repository configuration as follows:
> -
> -[format]
> -        headers = "Organization: git-foo\n"

So is this configuration option also lost in built-in git-format-patch?


-- 
Jakub Narebski
Warsaw, Poland

Re: [PATCH] Update documentation for git-format-patch

[Dennis Stosberg]

Jakub Narebski wrote:

> Dennis Stosberg wrote:
> 
> > -        Display suspicious lines in the patch.  The definition
> > -        of 'suspicious lines' is currently the lines that has
> > -        trailing whitespaces, and the lines whose indentation
> > -        has a SP character immediately followed by a TAB
> > -        character.
> 
> So is this option also lost in built-in git-format-patch?

Johannes Schindelin made that one a diff option, so it can be used in
other ways, too.  I think it should be documented in diff-options.txt.

> > -CONFIGURATION
> > -You can specify extra mail header lines to be added to each
> > -message in the repository configuration as follows:
> > -
> > -[format]
> > -        headers = "Organization: git-foo\n"
> 
> So is this configuration option also lost in built-in git-format-patch?

Grep'ing the sources, I couldn't find any trace of it.

Regards,
Dennis

Re: format-patch signoff argument no longer works

[Junio C Hamano]

Seth Falcon <sethfalcon@gmail.com> writes:

> Matthias Kestenholz <lists@spinlock.ch> writes:
>
>> * Geoff Russell (geoffrey.russell@gmail.com) wrote:
>>> It appears to have got lost when the shell script got converted to C.
>>> 
>> Yes, this was intentional. You should sign off your changes while
>> committing (git commit -s|--signoff)

A bit on this later, but first to clear one thing up...

> When should one commit _without_ signoff?  
>
> The obvious answer is: when one doesn't approve of the changes in the
> commit... But in my usual workflow, commit means
> works-for-me-I-think-it-is-good. :-)

Please, calm down and read Documentation/SubmittingPatches,
item (6), to understand what sign-off means.  It does not have
anything to do with the result of the commit "working".  I do
not use -s when making commits during my day-job, for example.

We do not want to make sign-off the default.  It has to be a
concious act on the signer's part to add one.  Otherwise it
would not carry much weight.

About the droppage of "format-patch -s", I have come to think of
it as a mistake (yes, I can change my mind).  Consider:

 * You are the leader of a group of people who hack on a part of
   the kernel, internally in your company.  You and other
   developers make improvements and make commits, with "git
   commit -s".

 * As the in-company integrator, you maintain the canonical
   "company tree" by pulling from others in your group.

 * It's time to send good pieces to Linus and/or Andrew and as
   the group lead you are responsible for sending them out.  The
   commits would have Sign-off's by the original committers, but
   as the contact person (representative) of your group, your
   name is better recognizable in the outside community, and as
   the leader of your group, it is a good practice for _you_ to
   vouch for what your group did.

In that scenario, in addition to what "commit -s" gives us, it
is handy for the person who is sending the patches out via
e-mail to add his own sign-off.

Now, we could do that by re-adding "format-patch -s" option, or
alternatively we could add that to "send-email".  We might want
to do both ;-)

Re: format-patch signoff argument no longer works

[J. Bruce Fields]

On Wed, May 31, 2006 at 12:02:33PM -0700, Junio C Hamano wrote:
> Now, we could do that by re-adding "format-patch -s" option, or
> alternatively we could add that to "send-email".  We might want
> to do both ;-)

Personally, just to give myself the best change of catching problems
with the outgoing email, I'd prefer to have as much as possible be done
*before* send-email, so whatever I review is as close to what's sent out
as possible.

--b.

Re: format-patch signoff argument no longer works

[Seth Falcon]

Junio C Hamano <junkio@cox.net> writes:

> Seth Falcon <sethfalcon@gmail.com> writes:
>> When should one commit _without_ signoff?  
>
> Please, calm down and read Documentation/SubmittingPatches,
> item (6), to understand what sign-off means.  It does not have
> anything to do with the result of the commit "working".  I do
> not use -s when making commits during my day-job, for example.

Doh!  In glancing through the Documentation dir I missed the
SubmittingPatches file. [weak excuse: there are many doc files in that
dir and most are for the commands themselves.  I expected this to be
in howto/.]

Anyhow, much calmer now (apologies if I sounded un-calm, that wasn't
my intention).  The SubmittingPatches doc was _exactly_ what I was
looking for when I posted my original question [*].  My bad for not
finding it when it was staring me in the face.  Thanks for bearing
with me and pointing me to the fine manual :-)



[*] http://marc.theaimsgroup.com/?l=git&m=114884854119660&w=2

[PATCH] format-patch --signoff

[Junio C Hamano]

This resurrects --signoff option to format-patch.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 builtin-log.c |   18 ++++++++++++++++--
 log-tree.c    |   35 +++++++++++++++++++++++++++++++++++
 revision.h    |    1 +
 3 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index db1912a..ac4822d 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -150,6 +150,7 @@ int cmd_format_patch(int argc, const cha
 	int numbered = 0;
 	int start_number = -1;
 	int keep_subject = 0;
+	char *add_signoff = NULL;
 
 	init_revisions(&rev);
 	rev.commit_format = CMIT_FMT_EMAIL;
@@ -179,11 +180,13 @@ int cmd_format_patch(int argc, const cha
 			if (i == argc)
 				die("Need a number for --start-number");
 			start_number = strtol(argv[i], NULL, 10);
-		} else if (!strcmp(argv[i], "-k") ||
+		}
+		else if (!strcmp(argv[i], "-k") ||
 				!strcmp(argv[i], "--keep-subject")) {
 			keep_subject = 1;
 			rev.total = -1;
-		} else if (!strcmp(argv[i], "-o")) {
+		}
+		else if (!strcmp(argv[i], "-o")) {
 			if (argc < 3)
 				die ("Which directory?");
 			if (mkdir(argv[i + 1], 0777) < 0 && errno != EEXIST)
@@ -192,6 +195,16 @@ int cmd_format_patch(int argc, const cha
 			output_directory = strdup(argv[i + 1]);
 			i++;
 		}
+		else if (!strcmp(argv[i], "--signoff") ||
+			 !strcmp(argv[i], "-s")) {
+			const char *committer = git_committer_info(1);
+			const char *endpos = strchr(committer, '>');
+			if (!endpos)
+				die("bogos committer info %s\n", committer);
+			add_signoff = xmalloc(endpos - committer + 2);
+			memcpy(add_signoff, committer, endpos - committer + 1);
+			add_signoff[endpos - committer + 1] = 0;
+		}
 		else if (!strcmp(argv[i], "--attach"))
 			rev.mime_boundary = git_version_string;
 		else if (!strncmp(argv[i], "--attach=", 9))
@@ -230,6 +243,7 @@ int cmd_format_patch(int argc, const cha
 	total = nr;
 	if (numbered)
 		rev.total = total + start_number - 1;
+	rev.add_signoff = add_signoff;
 	while (0 <= --nr) {
 		int shown;
 		commit = list[nr];
diff --git a/log-tree.c b/log-tree.c
index 58b0163..e86e16b 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -12,6 +12,37 @@ static void show_parents(struct commit *
 	}
 }
 
+static int append_signoff(char *buf, int buf_sz, int at, const char *signoff)
+{
+	int signoff_len = strlen(signoff);
+	static const char signed_off_by[] = "Signed-off-by: ";
+	char *cp = buf;
+
+	/* Do we have enough space to add it? */
+	if (buf_sz - at <= strlen(signed_off_by) + signoff_len + 2)
+		return at;
+
+	/* First see if we already have the sign-off by the signer */
+	while (1) {
+		cp = strstr(cp, signed_off_by);
+		if (!cp)
+			break;
+		cp += strlen(signed_off_by);
+		if ((cp + signoff_len < buf + at) &&
+		    !strncmp(cp, signoff, signoff_len) &&
+		    isspace(cp[signoff_len]))
+			return at; /* we already have him */
+	}
+
+	strcpy(buf + at, signed_off_by);
+	at += strlen(signed_off_by);
+	strcpy(buf + at, signoff);
+	at += signoff_len;
+	buf[at++] = '\n';
+	buf[at] = 0;
+	return at;
+}
+
 void show_log(struct rev_info *opt, struct log_info *log, const char *sep)
 {
 	static char this_header[16384];
@@ -111,6 +142,10 @@ void show_log(struct rev_info *opt, stru
 	 * And then the pretty-printed message itself
 	 */
 	len = pretty_print_commit(opt->commit_format, commit, ~0u, this_header, sizeof(this_header), abbrev, subject, after_subject);
+
+	if (opt->add_signoff)
+		len = append_signoff(this_header, sizeof(this_header), len,
+				     opt->add_signoff);
 	printf("%s%s%s", this_header, extra, sep);
 }
 
diff --git a/revision.h b/revision.h
index bdbdd23..75796bc 100644
--- a/revision.h
+++ b/revision.h
@@ -60,6 +60,7 @@ struct rev_info {
 	struct log_info *loginfo;
 	int		nr, total;
 	const char	*mime_boundary;
+	const char	*add_signoff;
 
 	/* special limits */
 	int max_count;
-- 
1.3.3.g1361-dirty

Re: [PATCH] format-patch --signoff

[Johannes Schindelin]

Hi,

On Wed, 31 May 2006, Junio C Hamano wrote:

> This resurrects --signoff option to format-patch.

Sorry; I was in cinema, so I missed all the action.


> +			const char *committer = git_committer_info(1);
> +			const char *endpos = strchr(committer, '>');
> +			if (!endpos)
> +				die("bogos committer info %s\n", committer);
> +			add_signoff = xmalloc(endpos - committer + 2);
> +			memcpy(add_signoff, committer, endpos - committer + 1);
> +			add_signoff[endpos - committer + 1] = 0;
> +		}

I don't know, but it may be a good idea to make this more general: Why not 
build the sign-off line here, so that you could also add more than one 
sign-off lines ('--signoff="The great committer <ter@mit.com>"'), and 
maybe even Acked-by's?

> +	/* First see if we already have the sign-off by the signer */
> +	while (1) {
> +		cp = strstr(cp, signed_off_by);
> +		if (!cp)
> +			break;
> +		cp += strlen(signed_off_by);
> +		if ((cp + signoff_len < buf + at) &&
> +		    !strncmp(cp, signoff, signoff_len) &&
> +		    isspace(cp[signoff_len]))
> +			return at; /* we already have him */
> +	}

Okay, this would be a little harder with multiple sign-offs. But the check 
could be easier, i.e. if we say

	rev.add_signoff = xmalloc(enough_room);
	strcpy(rev.add_signoff, "\nSigned-off-by: ");
	strcat(rev.add_signoff, committer_ident);
	strcat(rev.add_signoff, "\n");

then a simple

	p = strstr(commit_buffer, rev.add_signoff);
	if (p)
		return (int)(p - commit_buffer);

would do the trick.

And shouldn't we error out if there is not enough room for a sign-off?

Sorry for all the nit-picking.

Ciao,
Dscho

Re: [PATCH] format-patch --signoff

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> I don't know, but it may be a good idea to make this more general: Why not 
> build the sign-off line here, so that you could also add more than one 
> sign-off lines ('--signoff="The great committer <ter@mit.com>"'), and 
> maybe even Acked-by's?

Perhaps.

> Okay, this would be a little harder with multiple sign-offs. But the check 
> could be easier, i.e. if we say
>
> 	rev.add_signoff = xmalloc(enough_room);
> 	strcpy(rev.add_signoff, "\nSigned-off-by: ");
> 	strcat(rev.add_signoff, committer_ident);
> 	strcat(rev.add_signoff, "\n");
>
> then a simple
>
> 	p = strstr(commit_buffer, rev.add_signoff);
> 	if (p)
> 		return (int)(p - commit_buffer);
>
> would do the trick.

Do you mean, by "multiple sign-offs", something like this?

	for (so_list = rev.add_signoff; so_list; so_list = so_list->next) {
		if (strstr(commit_buffer, so_list->item))
                	continue;
                append_to_commit_buffer(so_list->item);
	}
	return tail - commit_buffer;

> And shouldn't we error out if there is not enough room for a sign-off?

I do not think we error out if the commit message is too long
either, so...

Re: [PATCH] format-patch --signoff

[Johannes Schindelin]

Hi,

On Wed, 31 May 2006, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > I don't know, but it may be a good idea to make this more general: Why not 
> > build the sign-off line here, so that you could also add more than one 
> > sign-off lines ('--signoff="The great committer <ter@mit.com>"'), and 
> > maybe even Acked-by's?
> 
> Perhaps.
> 
> > Okay, this would be a little harder with multiple sign-offs. But the check 
> > could be easier, i.e. if we say
> >
> > 	rev.add_signoff = xmalloc(enough_room);
> > 	strcpy(rev.add_signoff, "\nSigned-off-by: ");
> > 	strcat(rev.add_signoff, committer_ident);
> > 	strcat(rev.add_signoff, "\n");
> >
> > then a simple
> >
> > 	p = strstr(commit_buffer, rev.add_signoff);
> > 	if (p)
> > 		return (int)(p - commit_buffer);
> >
> > would do the trick.
> 
> Do you mean, by "multiple sign-offs", something like this?
> 
> 	for (so_list = rev.add_signoff; so_list; so_list = so_list->next) {
> 		if (strstr(commit_buffer, so_list->item))
>                 	continue;
>                 append_to_commit_buffer(so_list->item);
> 	}
> 	return tail - commit_buffer;

Actually, I did not think of a linked list, but one buffer, but I like 
your solution better.

> > And shouldn't we error out if there is not enough room for a sign-off?
> 
> I do not think we error out if the commit message is too long
> either, so...

... so you could say that should be an error, too.

Ciao,
Dscho