Geoff RussellHi,
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
threads / discuss / 4360
format-patch signoff argument no longer works
Subject: format-patch signoff argument no longer works
Geoff RussellRe: format-patch signoff argument no longer works
* Geoff Russell (geoffrey.russell@gmail.com) wrote:
Show 6 quoted lines
> 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
Seth FalconRe: format-patch signoff argument no longer works
Matthias Kestenholz <lists@spinlock.ch> writes:
Show 5 quoted lines
> * 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
Johannes SchindelinRe: format-patch signoff argument no longer works
Hi,
On Wed, 31 May 2006, Seth Falcon wrote:
Show 13 quoted lines
> 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
Junio C HamanoRe: format-patch signoff argument no longer works
Seth Falcon <sethfalcon@gmail.com> writes:
Show 7 quoted lines
> 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...
Show 5 quoted lines
> 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
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.
Seth FalconRe: format-patch signoff argument no longer works
Junio C Hamano <junkio@cox.net> writes:
Show 7 quoted lines
> 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
Junio C Hamano[PATCH] format-patch --signoff
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
Johannes SchindelinRe: [PATCH] format-patch --signoff
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.
Show 8 quoted lines
> + 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?
Show 11 quoted lines
> + /* 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
Junio C HamanoRe: [PATCH] format-patch --signoff
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.
Show 15 quoted lines
> 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...
Johannes SchindelinRe: [PATCH] format-patch --signoff
Hi,
On Wed, 31 May 2006, Junio C Hamano wrote:
Show 33 quoted lines
> 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
[PATCH] Update documentation for git-format-patch
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
Johannes SchindelinRe: [PATCH] Update documentation for git-format-patch
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
Jakub NarebskiRe: [PATCH] Update documentation for git-format-patch
Dennis Stosberg wrote:
Show 6 quoted lines
> --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?
Show 7 quoted lines
> -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
Jakub Narebski wrote:
Show 9 quoted lines
> 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.
Show 8 quoted lines
> > -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