* [PATCH 01/11] sequencer.c: remove broken support for rfc2822 continuation in footer
2012-11-26 1:45 [PATCH 00/11] alternative unify appending of sob Brandon Casey
@ 2012-11-26 1:45 ` Brandon Casey
2012-11-26 1:45 ` [PATCH 02/11] t/test-lib-functions.sh: allow to specify the tag name to test_commit Brandon Casey
` (11 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Brandon Casey @ 2012-11-26 1:45 UTC (permalink / raw)
To: git; +Cc: pclouds, gitster, Brandon Casey, Brandon Casey
Commit c1e01b0c generalized the detection of the last paragraph
signed-off-by footer and used rfc2822 as a guideline. Support for rfc2822
style continuation lines was also implemented, but not correctly, so it has
never detected a line beginning with space or tab as a continuation of the
previous line.
Since a commit message is not governed by the line length limits imposed
by rfc2822 for email messages, and it does not seem like this functionality
would produce "better" commit messages anyway, let's remove this broken
functionality.
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
sequencer.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 2260490..656df6b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1023,7 +1023,6 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
int hit = 0;
int i, j, k;
int len = sb->len - ignore_footer;
- int first = 1;
const char *buf = sb->buf;
for (i = len - 1; i > 0; i--) {
@@ -1040,11 +1039,6 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
; /* do nothing */
k++;
- if ((buf[k] == ' ' || buf[k] == '\t') && !first)
- continue;
-
- first = 0;
-
for (j = 0; i + j < len; j++) {
ch = buf[i + j];
if (ch == ':')
--
1.8.0.284.g959048a
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 02/11] t/test-lib-functions.sh: allow to specify the tag name to test_commit
2012-11-26 1:45 [PATCH 00/11] alternative unify appending of sob Brandon Casey
2012-11-26 1:45 ` [PATCH 01/11] sequencer.c: remove broken support for rfc2822 continuation in footer Brandon Casey
@ 2012-11-26 1:45 ` Brandon Casey
2012-11-26 1:45 ` [PATCH 03/11] t/t3511: add some tests of 'cherry-pick -s' functionality Brandon Casey
` (10 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Brandon Casey @ 2012-11-26 1:45 UTC (permalink / raw)
To: git; +Cc: pclouds, gitster, Brandon Casey, Brandon Casey
The <message> part of test_commit() may not be appropriate for a tag name.
So let's allow test_commit to accept a fourth argument to specify the tag
name.
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
t/test-lib-functions.sh | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8889ba5..9e2b8b8 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -135,12 +135,13 @@ test_pause () {
fi
}
-# Call test_commit with the arguments "<message> [<file> [<contents>]]"
+# Call test_commit with the arguments "<message> [<file> [<contents> [<tag>]]]"
#
# This will commit a file with the given contents and the given commit
-# message. It will also add a tag with <message> as name.
+# message. It will also add a tag with <message> as name unless <tag> is
+# given.
#
-# Both <file> and <contents> default to <message>.
+# <file>, <contents>, and <tag> all default to <message>.
test_commit () {
notick= &&
@@ -168,7 +169,7 @@ test_commit () {
test_tick
fi &&
git commit $signoff -m "$1" &&
- git tag "$1"
+ git tag "${4:-$1}"
}
# Call test_merge with the arguments "<message> <commit>", where <commit>
--
1.8.0.284.g959048a
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 03/11] t/t3511: add some tests of 'cherry-pick -s' functionality
2012-11-26 1:45 [PATCH 00/11] alternative unify appending of sob Brandon Casey
2012-11-26 1:45 ` [PATCH 01/11] sequencer.c: remove broken support for rfc2822 continuation in footer Brandon Casey
2012-11-26 1:45 ` [PATCH 02/11] t/test-lib-functions.sh: allow to specify the tag name to test_commit Brandon Casey
@ 2012-11-26 1:45 ` Brandon Casey
2012-11-26 1:45 ` [PATCH 04/11] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer Brandon Casey
` (9 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Brandon Casey @ 2012-11-26 1:45 UTC (permalink / raw)
To: git; +Cc: pclouds, gitster, Brandon Casey, Brandon Casey
Add some tests to ensure that 'cherry-pick -s' operates in the following
manner:
* Inserts a blank line before appending a s-o-b to a commit message that
does not contain a s-o-b footer
* Does not mistake first line "subject: description" as a s-o-b footer
* Does not mistake single word message body as conforming to rfc2822
* Appends a s-o-b when last s-o-b in footer does not match committer
s-o-b, even when committer's s-o-b exists elsewhere in footer.
* Does not append a s-o-b when last s-o-b matches committer s-o-b
* Correctly detects a non-conforming footer containing a mix of s-o-b
like elements and s-o-b elements. (marked "expect failure")
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
t/t3511-cherry-pick-x.sh | 111 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 111 insertions(+)
create mode 100755 t/t3511-cherry-pick-x.sh
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
new file mode 100755
index 0000000..a6c4168
--- /dev/null
+++ b/t/t3511-cherry-pick-x.sh
@@ -0,0 +1,111 @@
+#!/bin/sh
+
+test_description='Test cherry-pick -x and -s'
+
+. ./test-lib.sh
+
+pristine_detach () {
+ git cherry-pick --quit &&
+ git checkout -f "$1^0" &&
+ git read-tree -u --reset HEAD &&
+ git clean -d -f -f -q -x
+}
+
+mesg_one_line='base: commit message'
+
+mesg_no_footer="$mesg_one_line
+
+OneWordBodyThatsNotA-S-o-B"
+
+mesg_with_footer="$mesg_no_footer
+
+Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+Signed-off-by: A.U. Thor <author@example.com>
+Signed-off-by: B.U. Thor <buthor@example.com>"
+
+mesg_broken_footer="$mesg_no_footer
+
+Signed-off-by: B.U. Thor <buthor@example.com>
+Not a valid footer element
+Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+
+mesg_with_footer_sob="$mesg_with_footer
+Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+
+
+test_expect_success setup '
+ git config advice.detachedhead false &&
+ echo unrelated >unrelated &&
+ git add unrelated &&
+ test_commit initial foo a &&
+ test_commit "$mesg_one_line" foo b mesg-one-line &&
+ git reset --hard initial &&
+ test_commit "$mesg_no_footer" foo b mesg-no-footer &&
+ git reset --hard initial &&
+ test_commit "$mesg_broken_footer" foo b mesg-broken-footer &&
+ git reset --hard initial &&
+ test_commit "$mesg_with_footer" foo b mesg-with-footer &&
+ git reset --hard initial &&
+ test_commit "$mesg_with_footer_sob" foo b mesg-with-footer-sob &&
+ pristine_detach initial &&
+ test_commit conflicting unrelated
+'
+
+test_expect_success 'cherry-pick -s inserts blank line after one line subject' '
+ pristine_detach initial &&
+ git cherry-pick -s mesg-one-line &&
+ cat <<-EOF >expect &&
+ $mesg_one_line
+
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
+test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '
+ pristine_detach initial &&
+ git cherry-pick -s mesg-broken-footer &&
+ cat <<-EOF >expect &&
+ $mesg_broken_footer
+
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick -s inserts blank line when conforming footer not found' '
+ pristine_detach initial &&
+ git cherry-pick -s mesg-no-footer &&
+ cat <<-EOF >expect &&
+ $mesg_no_footer
+
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committer' '
+ pristine_detach initial &&
+ git cherry-pick -s mesg-with-footer &&
+ cat <<-EOF >expect &&
+ $mesg_with_footer
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob' '
+ pristine_detach initial &&
+ git cherry-pick -s mesg-with-footer-sob &&
+ cat <<-EOF >expect &&
+ $mesg_with_footer_sob
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
1.8.0.284.g959048a
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 04/11] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
2012-11-26 1:45 [PATCH 00/11] alternative unify appending of sob Brandon Casey
` (2 preceding siblings ...)
2012-11-26 1:45 ` [PATCH 03/11] t/t3511: add some tests of 'cherry-pick -s' functionality Brandon Casey
@ 2012-11-26 1:45 ` Brandon Casey
2012-11-29 0:02 ` Junio C Hamano
2012-11-26 1:45 ` [PATCH 05/11] sequencer.c: always separate "(cherry picked from" from commit body Brandon Casey
` (8 subsequent siblings)
12 siblings, 1 reply; 18+ messages in thread
From: Brandon Casey @ 2012-11-26 1:45 UTC (permalink / raw)
To: git; +Cc: pclouds, gitster, Brandon Casey, Brandon Casey
When 'cherry-pick -s' is used to append a signed-off-by line to a cherry
picked commit, it does not currently detect the "(cherry picked from..."
that may have been appended by a previous 'cherry-pick -x' as part of the
s-o-b footer and it will insert a blank line before appending a new s-o-b.
Let's detect "(cherry picked from...)" as part of the footer so that we
will produce this:
Signed-off-by: A U Thor <author@example.com>
(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
Signed-off-by: C O Mitter <committer@example.com>
instead of this:
Signed-off-by: A U Thor <author@example.com>
(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
Signed-off-by: C O Mitter <committer@example.com>
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
sequencer.c | 42 ++++++++++++++++++++++++------------
t/t3511-cherry-pick-x.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 84 insertions(+), 13 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 656df6b..19eaf11 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -18,6 +18,7 @@
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
const char sign_off_header[] = "Signed-off-by: ";
+static const char cherry_picked_prefix[] = "(cherry picked from commit ";
static void remove_sequencer_state(void)
{
@@ -492,7 +493,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
}
if (opts->record_origin) {
- strbuf_addstr(&msgbuf, "(cherry picked from commit ");
+ strbuf_addstr(&msgbuf, cherry_picked_prefix);
strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
strbuf_addstr(&msgbuf, ")\n");
}
@@ -1017,11 +1018,32 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return pick_commits(todo_list, opts);
}
-static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
+static int is_rfc2822_line(const char *buf, int len)
+{
+ int i;
+
+ for (i = 0; i < len; i++) {
+ int ch = buf[i];
+ if (ch == ':')
+ break;
+ if (isalnum(ch) || (ch == '-'))
+ continue;
+ return 0;
+ }
+
+ return 1;
+}
+
+static int is_cherry_pick_from_line(const char *buf, int len)
+{
+ return (strlen(cherry_picked_prefix) + 41) <= len &&
+ !prefixcmp(buf, cherry_picked_prefix);
+}
+
+static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
{
- int ch;
int hit = 0;
- int i, j, k;
+ int i, k;
int len = sb->len - ignore_footer;
const char *buf = sb->buf;
@@ -1039,15 +1061,9 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
; /* do nothing */
k++;
- for (j = 0; i + j < len; j++) {
- ch = buf[i + j];
- if (ch == ':')
- break;
- if (isalnum(ch) ||
- (ch == '-'))
- continue;
+ if (!(is_rfc2822_line(buf+i, k-i) ||
+ is_cherry_pick_from_line(buf+i, k-i)))
return 0;
- }
}
return 1;
}
@@ -1064,7 +1080,7 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer)
for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
; /* do nothing */
if (prefixcmp(msgbuf->buf + i, sob.buf)) {
- if (!i || !ends_rfc2822_footer(msgbuf, ignore_footer))
+ if (!i || !has_conforming_footer(msgbuf, ignore_footer))
strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);
}
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index a6c4168..32c0bb1 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -32,6 +32,10 @@ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
mesg_with_footer_sob="$mesg_with_footer
Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+mesg_with_cherry_footer="$mesg_with_footer_sob
+(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)
+Tested-by: C.U. Thor <cuthor@example.com>"
+
test_expect_success setup '
git config advice.detachedhead false &&
@@ -47,6 +51,8 @@ test_expect_success setup '
test_commit "$mesg_with_footer" foo b mesg-with-footer &&
git reset --hard initial &&
test_commit "$mesg_with_footer_sob" foo b mesg-with-footer-sob &&
+ git reset --hard initial &&
+ test_commit "$mesg_with_cherry_footer" foo b mesg-with-cherry-footer &&
pristine_detach initial &&
test_commit conflicting unrelated
'
@@ -98,6 +104,19 @@ test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committe
test_cmp expect actual
'
+test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match committer' '
+ pristine_detach initial &&
+ sha1=`git rev-parse mesg-with-footer^0` &&
+ git cherry-pick -x -s mesg-with-footer &&
+ cat <<-EOF >expect &&
+ $mesg_with_footer
+ (cherry picked from commit $sha1)
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob' '
pristine_detach initial &&
git cherry-pick -s mesg-with-footer-sob &&
@@ -108,4 +127,40 @@ test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob'
test_cmp expect actual
'
+test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists for committer' '
+ pristine_detach initial &&
+ sha1=`git rev-parse mesg-with-footer-sob^0` &&
+ git cherry-pick -x -s mesg-with-footer-sob &&
+ cat <<-EOF >expect &&
+ $mesg_with_footer_sob
+ (cherry picked from commit $sha1)
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick -x treats "(cherry picked from..." line as part of footer' '
+ pristine_detach initial &&
+ sha1=`git rev-parse mesg-with-cherry-footer^0` &&
+ git cherry-pick -x mesg-with-cherry-footer &&
+ cat <<-EOF >expect &&
+ $mesg_with_cherry_footer
+ (cherry picked from commit $sha1)
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick -s treats "(cherry picked from..." line as part of footer' '
+ pristine_detach initial &&
+ git cherry-pick -s mesg-with-cherry-footer &&
+ cat <<-EOF >expect &&
+ $mesg_with_cherry_footer
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.8.0.284.g959048a
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 04/11] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
2012-11-26 1:45 ` [PATCH 04/11] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer Brandon Casey
@ 2012-11-29 0:02 ` Junio C Hamano
2012-11-29 1:28 ` Brandon Casey
0 siblings, 1 reply; 18+ messages in thread
From: Junio C Hamano @ 2012-11-29 0:02 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, Brandon Casey
Brandon Casey <drafnel@gmail.com> writes:
> -static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
> +static int is_rfc2822_line(const char *buf, int len)
> +{
> + int i;
> +
> + for (i = 0; i < len; i++) {
> + int ch = buf[i];
> + if (ch == ':')
> + break;
> + if (isalnum(ch) || (ch == '-'))
> + continue;
> + return 0;
> + }
> +
> + return 1;
> +}
> +
> +static int is_cherry_pick_from_line(const char *buf, int len)
> +{
> + return (strlen(cherry_picked_prefix) + 41) <= len &&
> + !prefixcmp(buf, cherry_picked_prefix);
> +}
> +
> +static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
> {
> - int ch;
> int hit = 0;
> - int i, j, k;
> + int i, k;
> int len = sb->len - ignore_footer;
> const char *buf = sb->buf;
>
> @@ -1039,15 +1061,9 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
> ; /* do nothing */
> k++;
>
> - for (j = 0; i + j < len; j++) {
> - ch = buf[i + j];
> - if (ch == ':')
> - break;
> - if (isalnum(ch) ||
> - (ch == '-'))
> - continue;
> + if (!(is_rfc2822_line(buf+i, k-i) ||
> + is_cherry_pick_from_line(buf+i, k-i)))
> return 0;
> - }
> }
> return 1;
> }
Refactored code looks vastly more readable, but I think the
is_cherry_pick_from_line() function (by the way, shouldn't it be
named is_cherry_picked_from_line()?) shows its ambivalence. It
insists that the line has to be long enough to hold 40-hex object
name plus a closing parenthesis, but it only makes sure that the
prefix matches, without checking if the line has 40-hex object name,
or the object name is immediately followed by a closing parenthesis.
It also does not care if there are other garbage after it.
If the code is trying to be strict to avoid misidentification, then
the check should be tightened (i.e. require the known fixed length,
make sure get_sha1_hex() is happy, 41st byte is a close parenthesis
that is followed by the end of line). If the code is trying to be
more lenient to allow people hand-editing the cherry-picked-from
line that was generated, the check could be loosened (people may
truncate the 40-hex down to 12-hex or something).
I cannot read from this code which one was intended; the code must
make up its mind, whichever way (I do not have a strong preference).
> +test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match committer' '
Is the title of this test appropriate? It looks like it is making
sure we do not add an extra blank line after the existing footer to
me.
> + pristine_detach initial &&
> + sha1=`git rev-parse mesg-with-footer^0` &&
> + git cherry-pick -x -s mesg-with-footer &&
> + cat <<-EOF >expect &&
> + $mesg_with_footer
> + (cherry picked from commit $sha1)
> + Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
> + EOF
> + git log -1 --pretty=format:%B >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists for committer' '
> + pristine_detach initial &&
> + sha1=`git rev-parse mesg-with-footer-sob^0` &&
> + git cherry-pick -x -s mesg-with-footer-sob &&
> + cat <<-EOF >expect &&
> + $mesg_with_footer_sob
> + (cherry picked from commit $sha1)
> + Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
> + EOF
> + git log -1 --pretty=format:%B >actual &&
> + test_cmp expect actual
> +'
For people on the sideline, $mesg_with_footer_sob ends with s-o-b by
the same "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" we are adding
here. This is probably a sensible thing to do.
One thing I am not so happy about this whole "(cherry picked from"
thing (and I am again agreeing with Linus who made me change the
default long time ago not to add this line by default) is this. If
you use "cherry-pick -s -x $commit" to transplant a commit from an
unrelated part of the history, you will get the object name in the
resulting commit. But you can do the same in at least two different
ways. The easiest is:
git format-patch -1 --stdout $commit | git am -s3
and a bit closer to what "cherry-pick" does is:
git checkout $commit^0 &&
git rebase --onto @{-1} HEAD^ &&
git checkout -B @{-1}
i.e. rebase the single commit on top of the branch you were on.
In either way, you don't get "cherry picked from", even though you
just did the moral equivalent of it. Also we need to realize that
the first one is essentially what happens all the time here; the "|"
is implemented by the mailing list. And nobody misses "cherry
picked from" to point at the original commit object, which is
useless for the recipient of the resulting commit.
But that is an orthogonal issue.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 04/11] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
2012-11-29 0:02 ` Junio C Hamano
@ 2012-11-29 1:28 ` Brandon Casey
0 siblings, 0 replies; 18+ messages in thread
From: Brandon Casey @ 2012-11-29 1:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Casey, git@vger.kernel.org, pclouds@gmail.com
On 11/28/2012 4:02 PM, Junio C Hamano wrote:
> Brandon Casey <drafnel@gmail.com> writes:
>
>> -static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
>> +static int is_rfc2822_line(const char *buf, int len)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < len; i++) {
>> + int ch = buf[i];
>> + if (ch == ':')
>> + break;
>> + if (isalnum(ch) || (ch == '-'))
>> + continue;
>> + return 0;
>> + }
>> +
>> + return 1;
>> +}
>> +
>> +static int is_cherry_pick_from_line(const char *buf, int len)
>> +{
>> + return (strlen(cherry_picked_prefix) + 41) <= len &&
>> + !prefixcmp(buf, cherry_picked_prefix);
>> +}
>> +
>> +static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
>> {
>> - int ch;
>> int hit = 0;
>> - int i, j, k;
>> + int i, k;
>> int len = sb->len - ignore_footer;
>> const char *buf = sb->buf;
>>
>> @@ -1039,15 +1061,9 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
>> ; /* do nothing */
>> k++;
>>
>> - for (j = 0; i + j < len; j++) {
>> - ch = buf[i + j];
>> - if (ch == ':')
>> - break;
>> - if (isalnum(ch) ||
>> - (ch == '-'))
>> - continue;
>> + if (!(is_rfc2822_line(buf+i, k-i) ||
>> + is_cherry_pick_from_line(buf+i, k-i)))
>> return 0;
>> - }
>> }
>> return 1;
>> }
>
> Refactored code looks vastly more readable, but I think the
> is_cherry_pick_from_line() function (by the way, shouldn't it be
> named is_cherry_picked_from_line()?
Yes.
> ) shows its ambivalence. It
> insists that the line has to be long enough to hold 40-hex object
> name plus a closing parenthesis, but it only makes sure that the
> prefix matches, without checking if the line has 40-hex object name,
> or the object name is immediately followed by a closing parenthesis.
> It also does not care if there are other garbage after it.
>
> If the code is trying to be strict to avoid misidentification, then
> the check should be tightened (i.e. require the known fixed length,
> make sure get_sha1_hex() is happy, 41st byte is a close parenthesis
> that is followed by the end of line). If the code is trying to be
> more lenient to allow people hand-editing the cherry-picked-from
> line that was generated, the check could be loosened (people may
> truncate the 40-hex down to 12-hex or something).
>
> I cannot read from this code which one was intended; the code must
> make up its mind, whichever way (I do not have a strong preference).
The intention was a stricter-type match, but implemented somewhat
lazily. Part of me doesn't think that anyone should need to modify
the string that 'cherry-pick -x' adds and that a strict match is
appropriate. The other part of me doesn't want to reject a line that
looks like "(cherry picked from ...)" line that has a trimmed sha1
inside.
I think I'll submit a somewhat loosened check.
>> +test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match committer' '
>
> Is the title of this test appropriate? It looks like it is making
> sure we do not add an extra blank line after the existing footer to
> me.
It's really just part of the series of tests that checks for correct
operation when "the last sob doesn't match committer". This test
has a few elements in it:
* existing sob footer contains the committer's sob, but not last.
ensure that we don't refrain from appending a sob just because
one already exists in the footer.
* ensure extra blank isn't inserted after existing footer and
"(cherry picked from..."
* ensure a blank isn't inserted between "(cherry picked from..."
and new sob
The title of the test is me trying to fit "correctly adds \"(cherry
picked from...\" and sob when footer contains committer's sob but
not last" within a single line.
>> + pristine_detach initial &&
>> + sha1=`git rev-parse mesg-with-footer^0` &&
>> + git cherry-pick -x -s mesg-with-footer &&
>> + cat <<-EOF >expect &&
>> + $mesg_with_footer
>> + (cherry picked from commit $sha1)
>> + Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
>> + EOF
>> + git log -1 --pretty=format:%B >actual &&
>> + test_cmp expect actual
>> +'
>> +
>
>> +test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists for committer' '
>> + pristine_detach initial &&
>> + sha1=`git rev-parse mesg-with-footer-sob^0` &&
>> + git cherry-pick -x -s mesg-with-footer-sob &&
>> + cat <<-EOF >expect &&
>> + $mesg_with_footer_sob
>> + (cherry picked from commit $sha1)
>> + Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
>> + EOF
>> + git log -1 --pretty=format:%B >actual &&
>> + test_cmp expect actual
>> +'
>
> For people on the sideline, $mesg_with_footer_sob ends with s-o-b by
> the same "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" we are adding
> here. This is probably a sensible thing to do.
>
> One thing I am not so happy about this whole "(cherry picked from"
> thing (and I am again agreeing with Linus who made me change the
> default long time ago not to add this line by default) is this. If
> you use "cherry-pick -s -x $commit" to transplant a commit from an
> unrelated part of the history, you will get the object name in the
> resulting commit. But you can do the same in at least two different
> ways. The easiest is:
>
> git format-patch -1 --stdout $commit | git am -s3
>
> and a bit closer to what "cherry-pick" does is:
>
> git checkout $commit^0 &&
> git rebase --onto @{-1} HEAD^ &&
> git checkout -B @{-1}
>
> i.e. rebase the single commit on top of the branch you were on.
>
> In either way, you don't get "cherry picked from", even though you
> just did the moral equivalent of it. Also we need to realize that
> the first one is essentially what happens all the time here; the "|"
> is implemented by the mailing list. And nobody misses "cherry
> picked from" to point at the original commit object, which is
> useless for the recipient of the resulting commit.
>
> But that is an orthogonal issue.
Are you suggesting that people shouldn't use 'cherry-pick -x' at all?
I agree that it is useless to use -x when the commit that will be
referenced will not be available as part of the permanent history
of the project.
I do however think it is useful to add a reference to the original
commit when back- or forward- porting a commit. I think it adds
a little visibility to the commit to say that it was not originally
implemented on top of the base it is built on. I find it helpful
to examine the original implementation (the referenced commit) when
verifying the correctness or attempting to understand the ported
commit.
-Brandon
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 05/11] sequencer.c: always separate "(cherry picked from" from commit body
2012-11-26 1:45 [PATCH 00/11] alternative unify appending of sob Brandon Casey
` (3 preceding siblings ...)
2012-11-26 1:45 ` [PATCH 04/11] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer Brandon Casey
@ 2012-11-26 1:45 ` Brandon Casey
2012-11-26 1:45 ` [PATCH 06/11] sequencer.c: teach append_signoff how to detect duplicate s-o-b Brandon Casey
` (7 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Brandon Casey @ 2012-11-26 1:45 UTC (permalink / raw)
To: git; +Cc: pclouds, gitster, Brandon Casey, Brandon Casey
Start treating the "(cherry picked from" line added by cherry-pick -x
the same way that the s-o-b lines are treated. Namely, separate them
from the main commit message body with an empty line.
Introduce tests to test this functionality.
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
sequencer.c | 106 +++++++++++++++++++++++++----------------------
t/t3511-cherry-pick-x.sh | 53 ++++++++++++++++++++++++
2 files changed, 109 insertions(+), 50 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 19eaf11..7c0852a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -20,6 +20,60 @@
const char sign_off_header[] = "Signed-off-by: ";
static const char cherry_picked_prefix[] = "(cherry picked from commit ";
+static int is_rfc2822_line(const char *buf, int len)
+{
+ int i;
+
+ for (i = 0; i < len; i++) {
+ int ch = buf[i];
+ if (ch == ':')
+ break;
+ if (isalnum(ch) || (ch == '-'))
+ continue;
+ return 0;
+ }
+
+ return 1;
+}
+
+static int is_cherry_pick_from_line(const char *buf, int len)
+{
+ return (strlen(cherry_picked_prefix) + 41) <= len &&
+ !prefixcmp(buf, cherry_picked_prefix);
+}
+
+static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
+{
+ int hit = 0;
+ int i, k;
+ int len = sb->len - ignore_footer;
+ const char *buf = sb->buf;
+
+ for (i = len - 1; i > 0; i--) {
+ if (hit && buf[i] == '\n')
+ break;
+ hit = (buf[i] == '\n');
+ }
+
+ /* require at least one blank line */
+ if (!hit || buf[i] != '\n')
+ return 0;
+
+ while (i < len - 1 && buf[i] == '\n')
+ i++;
+
+ for (; i < len; i = k) {
+ for (k = i; k < len && buf[k] != '\n'; k++)
+ ; /* do nothing */
+ k++;
+
+ if (!(is_rfc2822_line(buf+i, k-i) ||
+ is_cherry_pick_from_line(buf+i, k-i)))
+ return 0;
+ }
+ return 1;
+}
+
static void remove_sequencer_state(void)
{
struct strbuf seq_dir = STRBUF_INIT;
@@ -493,6 +547,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
}
if (opts->record_origin) {
+ if (!has_conforming_footer(&msgbuf, 0))
+ strbuf_addch(&msgbuf, '\n');
strbuf_addstr(&msgbuf, cherry_picked_prefix);
strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
strbuf_addstr(&msgbuf, ")\n");
@@ -1018,56 +1074,6 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return pick_commits(todo_list, opts);
}
-static int is_rfc2822_line(const char *buf, int len)
-{
- int i;
-
- for (i = 0; i < len; i++) {
- int ch = buf[i];
- if (ch == ':')
- break;
- if (isalnum(ch) || (ch == '-'))
- continue;
- return 0;
- }
-
- return 1;
-}
-
-static int is_cherry_pick_from_line(const char *buf, int len)
-{
- return (strlen(cherry_picked_prefix) + 41) <= len &&
- !prefixcmp(buf, cherry_picked_prefix);
-}
-
-static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
-{
- int hit = 0;
- int i, k;
- int len = sb->len - ignore_footer;
- const char *buf = sb->buf;
-
- for (i = len - 1; i > 0; i--) {
- if (hit && buf[i] == '\n')
- break;
- hit = (buf[i] == '\n');
- }
-
- while (i < len - 1 && buf[i] == '\n')
- i++;
-
- for (; i < len; i = k) {
- for (k = i; k < len && buf[k] != '\n'; k++)
- ; /* do nothing */
- k++;
-
- if (!(is_rfc2822_line(buf+i, k-i) ||
- is_cherry_pick_from_line(buf+i, k-i)))
- return 0;
- }
- return 1;
-}
-
void append_signoff(struct strbuf *msgbuf, int ignore_footer)
{
struct strbuf sob = STRBUF_INIT;
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 32c0bb1..9dd6d5d 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -57,6 +57,19 @@ test_expect_success setup '
test_commit conflicting unrelated
'
+test_expect_success 'cherry-pick -x inserts blank line after one line subject' '
+ pristine_detach initial &&
+ sha1=`git rev-parse mesg-one-line^0` &&
+ git cherry-pick -x mesg-one-line &&
+ cat <<-EOF >expect &&
+ $mesg_one_line
+
+ (cherry picked from commit $sha1)
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'cherry-pick -s inserts blank line after one line subject' '
pristine_detach initial &&
git cherry-pick -s mesg-one-line &&
@@ -81,6 +94,19 @@ test_expect_failure 'cherry-pick -s inserts blank line after non-conforming foot
test_cmp expect actual
'
+test_expect_success 'cherry-pick -x inserts blank line when conforming footer not found' '
+ pristine_detach initial &&
+ sha1=`git rev-parse mesg-no-footer^0` &&
+ git cherry-pick -x mesg-no-footer &&
+ cat <<-EOF >expect &&
+ $mesg_no_footer
+
+ (cherry picked from commit $sha1)
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'cherry-pick -s inserts blank line when conforming footer not found' '
pristine_detach initial &&
git cherry-pick -s mesg-no-footer &&
@@ -93,6 +119,20 @@ test_expect_success 'cherry-pick -s inserts blank line when conforming footer no
test_cmp expect actual
'
+test_expect_success 'cherry-pick -x -s inserts blank line when conforming footer not found' '
+ pristine_detach initial &&
+ sha1=`git rev-parse mesg-no-footer^0` &&
+ git cherry-pick -x -s mesg-no-footer &&
+ cat <<-EOF >expect &&
+ $mesg_no_footer
+
+ (cherry picked from commit $sha1)
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committer' '
pristine_detach initial &&
git cherry-pick -s mesg-with-footer &&
@@ -163,4 +203,17 @@ test_expect_success 'cherry-pick -s treats "(cherry picked from..." line as part
test_cmp expect actual
'
+test_expect_success 'cherry-pick -x -s treats "(cherry picked from..." line as part of footer' '
+ pristine_detach initial &&
+ sha1=`git rev-parse mesg-with-cherry-footer^0` &&
+ git cherry-pick -x -s mesg-with-cherry-footer &&
+ cat <<-EOF >expect &&
+ $mesg_with_cherry_footer
+ (cherry picked from commit $sha1)
+ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
test_done
--
1.8.0.284.g959048a
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 06/11] sequencer.c: teach append_signoff how to detect duplicate s-o-b
2012-11-26 1:45 [PATCH 00/11] alternative unify appending of sob Brandon Casey
` (4 preceding siblings ...)
2012-11-26 1:45 ` [PATCH 05/11] sequencer.c: always separate "(cherry picked from" from commit body Brandon Casey
@ 2012-11-26 1:45 ` Brandon Casey
2012-11-29 0:35 ` Junio C Hamano
2012-11-26 1:45 ` [PATCH 07/11] sequencer.c: teach append_signoff to avoid adding a duplicate newline Brandon Casey
` (6 subsequent siblings)
12 siblings, 1 reply; 18+ messages in thread
From: Brandon Casey @ 2012-11-26 1:45 UTC (permalink / raw)
To: git; +Cc: pclouds, gitster, Brandon Casey, Brandon Casey
Teach append_signoff how to detect a duplicate s-o-b in the commit footer.
This is in preparation to unify the append_signoff implementations in
log-tree.c and sequencer.c.
Fixes test in t3511.
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
builtin/commit.c | 2 +-
sequencer.c | 43 +++++++++++++++++++++++++++++++------------
sequencer.h | 2 +-
t/t3511-cherry-pick-x.sh | 2 +-
4 files changed, 34 insertions(+), 15 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 1dd2ec5..7b9e2ac 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -700,7 +700,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
previous = eol;
}
- append_signoff(&sb, ignore_footer);
+ append_signoff(&sb, ignore_footer, 0);
}
if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
diff --git a/sequencer.c b/sequencer.c
index 7c0852a..3062ad4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -42,12 +42,19 @@ static int is_cherry_pick_from_line(const char *buf, int len)
!prefixcmp(buf, cherry_picked_prefix);
}
-static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
+/* Returns 0 for non-conforming footer
+ * Returns 1 for conforming footer
+ * Returns 2 when sob exists within conforming footer
+ * Returns 3 when sob exists within conforming footer as last entry
+ */
+static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
+ int ignore_footer)
{
int hit = 0;
- int i, k;
+ int i, k = 0;
int len = sb->len - ignore_footer;
const char *buf = sb->buf;
+ int found_sob = 0;
for (i = len - 1; i > 0; i--) {
if (hit && buf[i] == '\n')
@@ -63,14 +70,24 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
i++;
for (; i < len; i = k) {
+ int found_rfc2822;
+
for (k = i; k < len && buf[k] != '\n'; k++)
; /* do nothing */
k++;
- if (!(is_rfc2822_line(buf+i, k-i) ||
- is_cherry_pick_from_line(buf+i, k-i)))
+ found_rfc2822 = is_rfc2822_line(buf+i, k-i);
+ if (found_rfc2822 && sob &&
+ !strncasecmp(buf+i, sob->buf, sob->len))
+ found_sob = k;
+
+ if (!(found_rfc2822 || is_cherry_pick_from_line(buf+i, k-i)))
return 0;
}
+ if (found_sob == i)
+ return 3;
+ if (found_sob)
+ return 2;
return 1;
}
@@ -291,7 +308,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
rollback_lock_file(&index_lock);
if (opts->signoff)
- append_signoff(msgbuf, 0);
+ append_signoff(msgbuf, 0, 0);
if (!clean) {
int i;
@@ -547,7 +564,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
}
if (opts->record_origin) {
- if (!has_conforming_footer(&msgbuf, 0))
+ if (!has_conforming_footer(&msgbuf, NULL, 0))
strbuf_addch(&msgbuf, '\n');
strbuf_addstr(&msgbuf, cherry_picked_prefix);
strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
@@ -1074,9 +1091,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return pick_commits(todo_list, opts);
}
-void append_signoff(struct strbuf *msgbuf, int ignore_footer)
+void append_signoff(struct strbuf *msgbuf, int ignore_footer, int no_dup_sob)
{
struct strbuf sob = STRBUF_INIT;
+ int has_footer = 0;
int i;
strbuf_addstr(&sob, sign_off_header);
@@ -1085,10 +1103,11 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer)
strbuf_addch(&sob, '\n');
for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
; /* do nothing */
- if (prefixcmp(msgbuf->buf + i, sob.buf)) {
- if (!i || !has_conforming_footer(msgbuf, ignore_footer))
- strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
- strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);
- }
+ if (!i || !(has_footer =
+ has_conforming_footer(msgbuf, &sob, ignore_footer)))
+ strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
+ if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
+ strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
+ sob.buf, sob.len);
strbuf_release(&sob);
}
diff --git a/sequencer.h b/sequencer.h
index 9d57d57..c4c7132 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -48,6 +48,6 @@ int sequencer_pick_revisions(struct replay_opts *opts);
extern const char sign_off_header[];
-void append_signoff(struct strbuf *msgbuf, int ignore_footer);
+void append_signoff(struct strbuf *msgbuf, int ignore_footer, int no_dup_sob);
#endif
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 9dd6d5d..4b67343 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -82,7 +82,7 @@ test_expect_success 'cherry-pick -s inserts blank line after one line subject' '
test_cmp expect actual
'
-test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '
+test_expect_success 'cherry-pick -s inserts blank line after non-conforming footer' '
pristine_detach initial &&
git cherry-pick -s mesg-broken-footer &&
cat <<-EOF >expect &&
--
1.8.0.284.g959048a
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 06/11] sequencer.c: teach append_signoff how to detect duplicate s-o-b
2012-11-26 1:45 ` [PATCH 06/11] sequencer.c: teach append_signoff how to detect duplicate s-o-b Brandon Casey
@ 2012-11-29 0:35 ` Junio C Hamano
0 siblings, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2012-11-29 0:35 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, Brandon Casey
Brandon Casey <drafnel@gmail.com> writes:
> +/* Returns 0 for non-conforming footer
Please format it like this:
/*
* Returns 0 for ...
> + * Returns 1 for conforming footer
> + * Returns 2 when sob exists within conforming footer
> + * Returns 3 when sob exists within conforming footer as last entry
> + */
> +static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
> + int ignore_footer)
> {
> int hit = 0;
> - int i, k;
> + int i, k = 0;
This is not wrong per-se, but I do not think it is necessary ('k' is
always initialized at least to 'i' before it gets used). If we need
to do this, I'd prefer to see it done in 1/11 that cleaned up the
containing function as a preparation for this series.
> int len = sb->len - ignore_footer;
> const char *buf = sb->buf;
> + int found_sob = 0;
>
> for (i = len - 1; i > 0; i--) {
> if (hit && buf[i] == '\n')
> @@ -63,14 +70,24 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
> i++;
>
> for (; i < len; i = k) {
> + int found_rfc2822;
> +
> for (k = i; k < len && buf[k] != '\n'; k++)
> ; /* do nothing */
> k++;
>
> - if (!(is_rfc2822_line(buf+i, k-i) ||
> - is_cherry_pick_from_line(buf+i, k-i)))
> + found_rfc2822 = is_rfc2822_line(buf+i, k-i);
Not limited to this place but please have SP around binary operators, i.e.
found_rfc2822 = is_rfc2822_line(buf + i, k - i);
> + if (found_rfc2822 && sob &&
> + !strncasecmp(buf+i, sob->buf, sob->len))
> + found_sob = k;
Are we sure we want strncasecmp() here? I *think* you are trying to
catch "signed-off-By:" and other misspellings, and even though I
know in practice we know "Brandon Casey" and "brandon casey" are the
same person, it still feels somewhat sloppy. Perhaps it is just me.
> +
> + if (!(found_rfc2822 || is_cherry_pick_from_line(buf+i, k-i)))
> return 0;
> }
> + if (found_sob == i)
> + return 3;
> + if (found_sob)
> + return 2;
> return 1;
> }
>
> @@ -291,7 +308,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
> rollback_lock_file(&index_lock);
>
> if (opts->signoff)
> - append_signoff(msgbuf, 0);
> + append_signoff(msgbuf, 0, 0);
>
> if (!clean) {
> int i;
> @@ -547,7 +564,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
> }
>
> if (opts->record_origin) {
> - if (!has_conforming_footer(&msgbuf, 0))
> + if (!has_conforming_footer(&msgbuf, NULL, 0))
> strbuf_addch(&msgbuf, '\n');
> strbuf_addstr(&msgbuf, cherry_picked_prefix);
> strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
> @@ -1074,9 +1091,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)
> return pick_commits(todo_list, opts);
> }
>
> -void append_signoff(struct strbuf *msgbuf, int ignore_footer)
> +void append_signoff(struct strbuf *msgbuf, int ignore_footer, int no_dup_sob)
> {
> struct strbuf sob = STRBUF_INIT;
> + int has_footer = 0;
> int i;
>
> strbuf_addstr(&sob, sign_off_header);
> @@ -1085,10 +1103,11 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer)
> strbuf_addch(&sob, '\n');
> for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
> ; /* do nothing */
> - if (prefixcmp(msgbuf->buf + i, sob.buf)) {
> - if (!i || !has_conforming_footer(msgbuf, ignore_footer))
> - strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
> - strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);
> - }
> + if (!i || !(has_footer =
> + has_conforming_footer(msgbuf, &sob, ignore_footer)))
> + strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
> + if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
> + strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
> + sob.buf, sob.len);
Avoid assignment inside if () conditional. It is not immediately
obvious what value is compared against 3 in the second one, as the
above makes it appear as if has_footer is uninitialized when i is
zero.
Thanks.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 07/11] sequencer.c: teach append_signoff to avoid adding a duplicate newline
2012-11-26 1:45 [PATCH 00/11] alternative unify appending of sob Brandon Casey
` (5 preceding siblings ...)
2012-11-26 1:45 ` [PATCH 06/11] sequencer.c: teach append_signoff how to detect duplicate s-o-b Brandon Casey
@ 2012-11-26 1:45 ` Brandon Casey
2012-11-26 1:45 ` [PATCH 08/11] t4014: more tests about appending s-o-b lines Brandon Casey
` (5 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Brandon Casey @ 2012-11-26 1:45 UTC (permalink / raw)
To: git; +Cc: pclouds, gitster, Brandon Casey, Brandon Casey
Teach append_signoff to detect whether a blank line exists at the position
that the signed-off-by line will be added, and avoid adding an additional
one if one already exists. This is necessary to allow format-patch to add a
s-o-b to a patch with no commit message without adding an extra newline. A
following patch will make format-patch use this function rather than the
append_signoff implementation inside log-tree.c.
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
sequencer.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 3062ad4..eb93dd6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1103,8 +1103,8 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, int no_dup_sob)
strbuf_addch(&sob, '\n');
for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
; /* do nothing */
- if (!i || !(has_footer =
- has_conforming_footer(msgbuf, &sob, ignore_footer)))
+ if (msgbuf->buf[i] != '\n' && (!i || !(has_footer =
+ has_conforming_footer(msgbuf, &sob, ignore_footer))))
strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
--
1.8.0.284.g959048a
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 08/11] t4014: more tests about appending s-o-b lines
2012-11-26 1:45 [PATCH 00/11] alternative unify appending of sob Brandon Casey
` (6 preceding siblings ...)
2012-11-26 1:45 ` [PATCH 07/11] sequencer.c: teach append_signoff to avoid adding a duplicate newline Brandon Casey
@ 2012-11-26 1:45 ` Brandon Casey
2012-11-26 1:45 ` [PATCH 09/11] format-patch: stricter S-o-b detection Brandon Casey
` (4 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Brandon Casey @ 2012-11-26 1:45 UTC (permalink / raw)
To: git; +Cc: pclouds, gitster, Brandon Casey
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
[bc: fix test 90 "signoff: some random signoff-alike" and mark as failing.
Correct behavior should insert a blank line after message body and
signed-off-by ]
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
t/t4014-format-patch.sh | 146 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 146 insertions(+)
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 16a4ca1..dfe9209 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -963,4 +963,150 @@ test_expect_success 'format patch ignores color.ui' '
test_cmp expect actual
'
+append_signoff()
+{
+ C=`git commit-tree HEAD^^{tree} -p HEAD` &&
+ git format-patch --stdout --signoff ${C}^..${C} |
+ tee append_signoff.patch |
+ sed -n "1,/^---$/p" |
+ grep -n -E "^Subject|Sign|^$"
+}
+
+test_expect_success 'signoff: commit with no body' '
+ append_signoff </dev/null >actual &&
+ cat <<\EOF | sed "s/EOL$//" >expected &&
+4:Subject: [PATCH] EOL
+8:
+9:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success 'signoff: commit with only subject' '
+ echo subject | append_signoff >actual &&
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+9:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success 'signoff: commit with only subject that does not end with NL' '
+ echo -n subject | append_signoff >actual &&
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+9:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success 'signoff: no existing signoffs' '
+ append_signoff <<\EOF >actual &&
+subject
+
+body
+EOF
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success 'signoff: no existing signoffs and no trailing NL' '
+ printf "subject\n\nbody" | append_signoff >actual &&
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success 'signoff: some random signoff' '
+ append_signoff <<\EOF >actual &&
+subject
+
+body
+
+Signed-off-by: my@house
+EOF
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: my@house
+12:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
+test_expect_failure 'signoff: some random signoff-alike' '
+ append_signoff <<\EOF >actual &&
+subject
+
+body
+Fooled-by-me: my@house
+EOF
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+11:
+12:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success 'signoff: the same signoff at the end' '
+ append_signoff <<\EOF >actual &&
+subject
+
+body
+
+Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success 'signoff: the same signoff at the end, no trailing NL' '
+ printf "subject\n\nSigned-off-by: C O Mitter <committer@example.com>" |
+ append_signoff >actual &&
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+9:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success 'signoff: the same signoff NOT at the end' '
+ append_signoff <<\EOF >actual &&
+subject
+
+body
+
+Signed-off-by: C O Mitter <committer@example.com>
+Signed-off-by: my@house
+EOF
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: C O Mitter <committer@example.com>
+12:Signed-off-by: my@house
+EOF
+ test_cmp expected actual
+'
+
test_done
--
1.8.0.284.g959048a
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 09/11] format-patch: stricter S-o-b detection
2012-11-26 1:45 [PATCH 00/11] alternative unify appending of sob Brandon Casey
` (7 preceding siblings ...)
2012-11-26 1:45 ` [PATCH 08/11] t4014: more tests about appending s-o-b lines Brandon Casey
@ 2012-11-26 1:45 ` Brandon Casey
2012-11-26 1:45 ` [PATCH 10/11] format-patch: update append_signoff prototype Brandon Casey
` (3 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Brandon Casey @ 2012-11-26 1:45 UTC (permalink / raw)
To: git; +Cc: pclouds, gitster, Brandon Casey
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
S-o-b in the middle of a sentence, at the beginning of the sentence
but it is just because of text wrapping, or a full paragraph of valid
S-o-b in the middle of the message. All these are not S-o-b, but
detected as is. Fix them.
[bc: add two additional tests to demonstrate shortcomings of the current
code:
1. failure to detect non-conforming elements in the footer when last
line matches committer's s-o-b.
2. failure to handle various s-o-b -like elements in the footer as
conforming. e.g. "Change-id: IXXXX or Bug: 1234"
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
log-tree.c | 37 +++++++++++++++++--
t/t4014-format-patch.sh | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 130 insertions(+), 2 deletions(-)
diff --git a/log-tree.c b/log-tree.c
index 4f86def..14ebf69 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -260,14 +260,47 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
int has_signoff = 0;
char *cp;
- cp = sb->buf;
+ /*
+ * Only search in the last paragrah, don't be fooled by a
+ * paragraph full of valid S-o-b in the middle.
+ */
+ cp = sb->buf + sb->len - 1;
+ while (cp > sb->buf) {
+ if (cp[0] == '\n' && cp[1] == '\n') {
+ cp += 2;
+ break;
+ }
+ cp--;
+ }
/* First see if we already have the sign-off by the signer */
while ((cp = strstr(cp, signed_off_by))) {
+ const char *s = cp;
+ cp += strlen(signed_off_by);
+
+ if (!has_signoff && s > sb->buf) {
+ /*
+ * S-o-b in the middle of a sentence is not
+ * really S-o-b
+ */
+ if (s[-1] != '\n')
+ continue;
+
+ if (s - 1 > sb->buf && s[-2] == '\n')
+ ; /* the first S-o-b */
+ else if (!detect_any_signoff(sb->buf, s - sb->buf))
+ /*
+ * The previous line looks like an
+ * S-o-b. There's still no guarantee
+ * that it's an actual S-o-b. For that
+ * we need to look back until we find
+ * a blank line, which is too costly.
+ */
+ continue;
+ }
has_signoff = 1;
- cp += strlen(signed_off_by);
if (cp + signoff_len >= sb->buf + sb->len)
break;
if (strncmp(cp, signoff, signoff_len))
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index dfe9209..30e37a7 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1062,6 +1062,60 @@ EOF
test_cmp expected actual
'
+test_expect_success 'signoff: not really a signoff' '
+ append_signoff <<\EOF >actual &&
+subject
+
+I want to mention about Signed-off-by: here.
+EOF
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+9:I want to mention about Signed-off-by: here.
+10:
+11:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success 'signoff: not really a signoff (2)' '
+ append_signoff <<\EOF >actual &&
+subject
+
+My unfortunate
+Signed-off-by: example happens to be wrapped here.
+EOF
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:Signed-off-by: example happens to be wrapped here.
+11:
+12:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success 'signoff: valid S-o-b paragraph in the middle' '
+ append_signoff <<\EOF >actual &&
+subject
+
+Signed-off-by: my@house
+Signed-off-by: your@house
+
+A lot of houses.
+EOF
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+9:Signed-off-by: my@house
+10:Signed-off-by: your@house
+11:
+13:
+14:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
test_expect_success 'signoff: the same signoff at the end' '
append_signoff <<\EOF >actual &&
subject
@@ -1109,4 +1163,45 @@ EOF
test_cmp expected actual
'
+test_expect_failure 'signoff: detect garbage in non-conforming footer' '
+ append_signoff <<\EOF >actual &&
+subject
+
+body
+
+Tested-by: my@house
+Some Trash
+Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+13:Signed-off-by: C O Mitter <committer@example.com>
+14:
+15:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
+test_expect_failure 'signoff: footer begins with non-signoff without @ sign' '
+ append_signoff <<\EOF >actual &&
+subject
+
+body
+
+Tested-by: my@house
+Change-id: Ideadbeef
+Signed-off-by: C O Mitter <committer@example.com>
+Bug: 1234
+EOF
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+13:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
test_done
--
1.8.0.284.g959048a
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 10/11] format-patch: update append_signoff prototype
2012-11-26 1:45 [PATCH 00/11] alternative unify appending of sob Brandon Casey
` (8 preceding siblings ...)
2012-11-26 1:45 ` [PATCH 09/11] format-patch: stricter S-o-b detection Brandon Casey
@ 2012-11-26 1:45 ` Brandon Casey
2012-11-26 1:45 ` [PATCH 11/11] Unify appending signoff in format-patch, commit and sequencer Brandon Casey
` (2 subsequent siblings)
12 siblings, 0 replies; 18+ messages in thread
From: Brandon Casey @ 2012-11-26 1:45 UTC (permalink / raw)
To: git; +Cc: pclouds, gitster, Brandon Casey
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
This is a preparation step for merging with append_signoff from
sequencer.c
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
builtin/log.c | 13 +------------
log-tree.c | 21 +++++++++++++--------
revision.h | 2 +-
3 files changed, 15 insertions(+), 21 deletions(-)
diff --git a/builtin/log.c b/builtin/log.c
index e7b7db1..bb48344 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1058,7 +1058,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
struct commit *origin = NULL, *head = NULL;
const char *in_reply_to = NULL;
struct patch_ids ids;
- char *add_signoff = NULL;
struct strbuf buf = STRBUF_INIT;
int use_patch_format = 0;
int quiet = 0;
@@ -1154,16 +1153,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
PARSE_OPT_KEEP_DASHDASH);
- if (do_signoff) {
- const char *committer;
- const char *endpos;
- committer = git_committer_info(IDENT_STRICT);
- endpos = strchr(committer, '>');
- if (!endpos)
- die(_("bogus committer info %s"), committer);
- add_signoff = xmemdupz(committer, endpos - committer + 1);
- }
-
for (i = 0; i < extra_hdr.nr; i++) {
strbuf_addstr(&buf, extra_hdr.items[i].string);
strbuf_addch(&buf, '\n');
@@ -1354,7 +1343,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
total++;
start_number--;
}
- rev.add_signoff = add_signoff;
+ rev.add_signoff = do_signoff;
while (0 <= --nr) {
int shown;
commit = list[nr];
diff --git a/log-tree.c b/log-tree.c
index 14ebf69..be8e7ab 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -253,9 +253,11 @@ static int detect_any_signoff(char *letter, int size)
return seen_head && seen_name;
}
-static void append_signoff(struct strbuf *sb, const char *signoff)
+static void append_signoff(struct strbuf *sb, int flags)
{
- static const char signed_off_by[] = "Signed-off-by: ";
+ char* signoff = xstrdup(fmt_name(getenv("GIT_COMMITTER_NAME"),
+ getenv("GIT_COMMITTER_EMAIL")));
+ static const char sign_off_header[] = "Signed-off-by: ";
size_t signoff_len = strlen(signoff);
int has_signoff = 0;
char *cp;
@@ -274,9 +276,9 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
}
/* First see if we already have the sign-off by the signer */
- while ((cp = strstr(cp, signed_off_by))) {
+ while ((cp = strstr(cp, sign_off_header))) {
const char *s = cp;
- cp += strlen(signed_off_by);
+ cp += strlen(sign_off_header);
if (!has_signoff && s > sb->buf) {
/*
@@ -317,9 +319,10 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
if (!has_signoff)
strbuf_addch(sb, '\n');
- strbuf_addstr(sb, signed_off_by);
+ strbuf_addstr(sb, sign_off_header);
strbuf_add(sb, signoff, signoff_len);
strbuf_addch(sb, '\n');
+ free(signoff);
}
static unsigned int digits_in_number(unsigned int number)
@@ -695,8 +698,10 @@ void show_log(struct rev_info *opt)
/*
* And then the pretty-printed message itself
*/
- if (ctx.need_8bit_cte >= 0)
- ctx.need_8bit_cte = has_non_ascii(opt->add_signoff);
+ if (ctx.need_8bit_cte >= 0 && opt->add_signoff)
+ ctx.need_8bit_cte =
+ has_non_ascii(fmt_name(getenv("GIT_COMMITTER_NAME"),
+ getenv("GIT_COMMITTER_EMAIL")));
ctx.date_mode = opt->date_mode;
ctx.date_mode_explicit = opt->date_mode_explicit;
ctx.abbrev = opt->diffopt.abbrev;
@@ -707,7 +712,7 @@ void show_log(struct rev_info *opt)
pretty_print_commit(&ctx, commit, &msgbuf);
if (opt->add_signoff)
- append_signoff(&msgbuf, opt->add_signoff);
+ append_signoff(&msgbuf, 0);
if ((ctx.fmt != CMIT_FMT_USERFORMAT) &&
ctx.notes_message && *ctx.notes_message) {
diff --git a/revision.h b/revision.h
index 059bfff..435a60b 100644
--- a/revision.h
+++ b/revision.h
@@ -137,7 +137,7 @@ struct rev_info {
int numbered_files;
char *message_id;
struct string_list *ref_message_ids;
- const char *add_signoff;
+ int add_signoff;
const char *extra_headers;
const char *log_reencode;
const char *subject_prefix;
--
1.8.0.284.g959048a
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 11/11] Unify appending signoff in format-patch, commit and sequencer
2012-11-26 1:45 [PATCH 00/11] alternative unify appending of sob Brandon Casey
` (9 preceding siblings ...)
2012-11-26 1:45 ` [PATCH 10/11] format-patch: update append_signoff prototype Brandon Casey
@ 2012-11-26 1:45 ` Brandon Casey
2012-11-26 7:56 ` [PATCH 00/11] alternative unify appending of sob Nguyen Thai Ngoc Duy
2012-11-26 22:00 ` Junio C Hamano
12 siblings, 0 replies; 18+ messages in thread
From: Brandon Casey @ 2012-11-26 1:45 UTC (permalink / raw)
To: git; +Cc: pclouds, gitster, Brandon Casey, Brandon Casey
There are two implementations of append_signoff in log-tree.c and
sequencer.c, which do more or less the same thing. Unify on top of the
sequencer.c implementation.
Add a test in t4014 to demonstrate support for non-s-o-b elements in the
commit footer provided by sequence.c:append_sob. Mark tests fixed as
appropriate.
[Commit message mostly stolen from Nguyễn Thái Ngọc Duy's original
unification patch]
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
log-tree.c | 122 +-----------------------------------------------
t/t4014-format-patch.sh | 27 +++++++++--
2 files changed, 26 insertions(+), 123 deletions(-)
diff --git a/log-tree.c b/log-tree.c
index be8e7ab..1fb0a16 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -9,6 +9,7 @@
#include "string-list.h"
#include "color.h"
#include "gpg-interface.h"
+#include "sequencer.h"
struct decoration name_decoration = { "object names" };
@@ -206,125 +207,6 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
putchar(')');
}
-/*
- * Search for "^[-A-Za-z]+: [^@]+@" pattern. It usually matches
- * Signed-off-by: and Acked-by: lines.
- */
-static int detect_any_signoff(char *letter, int size)
-{
- char *cp;
- int seen_colon = 0;
- int seen_at = 0;
- int seen_name = 0;
- int seen_head = 0;
-
- cp = letter + size;
- while (letter <= --cp && *cp == '\n')
- continue;
-
- while (letter <= cp) {
- char ch = *cp--;
- if (ch == '\n')
- break;
-
- if (!seen_at) {
- if (ch == '@')
- seen_at = 1;
- continue;
- }
- if (!seen_colon) {
- if (ch == '@')
- return 0;
- else if (ch == ':')
- seen_colon = 1;
- else
- seen_name = 1;
- continue;
- }
- if (('A' <= ch && ch <= 'Z') ||
- ('a' <= ch && ch <= 'z') ||
- ch == '-') {
- seen_head = 1;
- continue;
- }
- /* no empty last line doesn't match */
- return 0;
- }
- return seen_head && seen_name;
-}
-
-static void append_signoff(struct strbuf *sb, int flags)
-{
- char* signoff = xstrdup(fmt_name(getenv("GIT_COMMITTER_NAME"),
- getenv("GIT_COMMITTER_EMAIL")));
- static const char sign_off_header[] = "Signed-off-by: ";
- size_t signoff_len = strlen(signoff);
- int has_signoff = 0;
- char *cp;
-
- /*
- * Only search in the last paragrah, don't be fooled by a
- * paragraph full of valid S-o-b in the middle.
- */
- cp = sb->buf + sb->len - 1;
- while (cp > sb->buf) {
- if (cp[0] == '\n' && cp[1] == '\n') {
- cp += 2;
- break;
- }
- cp--;
- }
-
- /* First see if we already have the sign-off by the signer */
- while ((cp = strstr(cp, sign_off_header))) {
- const char *s = cp;
- cp += strlen(sign_off_header);
-
- if (!has_signoff && s > sb->buf) {
- /*
- * S-o-b in the middle of a sentence is not
- * really S-o-b
- */
- if (s[-1] != '\n')
- continue;
-
- if (s - 1 > sb->buf && s[-2] == '\n')
- ; /* the first S-o-b */
- else if (!detect_any_signoff(sb->buf, s - sb->buf))
- /*
- * The previous line looks like an
- * S-o-b. There's still no guarantee
- * that it's an actual S-o-b. For that
- * we need to look back until we find
- * a blank line, which is too costly.
- */
- continue;
- }
-
- has_signoff = 1;
-
- if (cp + signoff_len >= sb->buf + sb->len)
- break;
- if (strncmp(cp, signoff, signoff_len))
- continue;
- if (!isspace(cp[signoff_len]))
- continue;
- /* we already have him */
- return;
- }
-
- if (!has_signoff)
- has_signoff = detect_any_signoff(sb->buf, sb->len);
-
- if (!has_signoff)
- strbuf_addch(sb, '\n');
-
- strbuf_addstr(sb, sign_off_header);
- strbuf_add(sb, signoff, signoff_len);
- strbuf_addch(sb, '\n');
- free(signoff);
-}
-
static unsigned int digits_in_number(unsigned int number)
{
unsigned int i = 10, result = 1;
@@ -712,7 +594,7 @@ void show_log(struct rev_info *opt)
pretty_print_commit(&ctx, commit, &msgbuf);
if (opt->add_signoff)
- append_signoff(&msgbuf, 0);
+ append_signoff(&msgbuf, 0, 1);
if ((ctx.fmt != CMIT_FMT_USERFORMAT) &&
ctx.notes_message && *ctx.notes_message) {
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 30e37a7..1dea58b 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1046,7 +1046,28 @@ EOF
test_cmp expected actual
'
-test_expect_failure 'signoff: some random signoff-alike' '
+test_expect_success 'signoff: misc conforming footer elements' '
+ append_signoff <<\EOF >actual &&
+subject
+
+body
+
+Signed-off-by: my@house
+(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)
+Tested-by: Some One <someone@example.com>
+Bug: 1234
+EOF
+ cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: my@house
+15:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+ test_cmp expected actual
+'
+
+test_expect_success 'signoff: some random signoff-alike' '
append_signoff <<\EOF >actual &&
subject
@@ -1163,7 +1184,7 @@ EOF
test_cmp expected actual
'
-test_expect_failure 'signoff: detect garbage in non-conforming footer' '
+test_expect_success 'signoff: detect garbage in non-conforming footer' '
append_signoff <<\EOF >actual &&
subject
@@ -1184,7 +1205,7 @@ EOF
test_cmp expected actual
'
-test_expect_failure 'signoff: footer begins with non-signoff without @ sign' '
+test_expect_success 'signoff: footer begins with non-signoff without @ sign' '
append_signoff <<\EOF >actual &&
subject
--
1.8.0.284.g959048a
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 00/11] alternative unify appending of sob
2012-11-26 1:45 [PATCH 00/11] alternative unify appending of sob Brandon Casey
` (10 preceding siblings ...)
2012-11-26 1:45 ` [PATCH 11/11] Unify appending signoff in format-patch, commit and sequencer Brandon Casey
@ 2012-11-26 7:56 ` Nguyen Thai Ngoc Duy
2012-11-29 5:56 ` Junio C Hamano
2012-11-26 22:00 ` Junio C Hamano
12 siblings, 1 reply; 18+ messages in thread
From: Nguyen Thai Ngoc Duy @ 2012-11-26 7:56 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, gitster, Brandon Casey
On Mon, Nov 26, 2012 at 8:45 AM, Brandon Casey <drafnel@gmail.com> wrote:
> From: Brandon Casey <bcasey@nvidia.com>
>
> I hate to have the battle of the patches, but I kinda prefer the
> append_signoff code in sequencer.c over the code in log-tree.c as a
> foundation to build on.
>
> So, this series is similar to Duy's "unification" series, but it goes in the
> opposite direction and builds on top of sequencer.c and additionally adds the
> elements from my original series to treat the "(cherry picked from..." line
> added by 'cherry-pick -x' in the same way that other footer elements are
> treated (after addressing Junio's comments about rfc2822 continuation lines
> and duplicate s-o-b's).
>
> I've integrated Duy's series with a few minor tweaks. I added a couple
> of additional tests to t4014 and corrected one of the tests which had
> incorrect behavior. I think his sign-off's should still be valid, so I
> kept them in. Sorry that I've been slow, and now the two of us are stepping
> on each other's toes, but Duy please take a look and let me know if there's
> anything you dislike.
I'm still not sure whether format-patch should follow cherry-pick's
rule in appending sob. If it does, it probably makes more sense to fix
the sequencer.c code then delete log-tree.c (not fixes on log-tree.c
then delete it). I see that your changes pass all the new tests I
added in format-patch so sequencer.c is probably good enough,
log-tree.c changes are probably not needed. Feel free take over the
series :-)
--
Duy
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 00/11] alternative unify appending of sob
2012-11-26 7:56 ` [PATCH 00/11] alternative unify appending of sob Nguyen Thai Ngoc Duy
@ 2012-11-29 5:56 ` Junio C Hamano
0 siblings, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2012-11-29 5:56 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Brandon Casey, git, Brandon Casey
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> On Mon, Nov 26, 2012 at 8:45 AM, Brandon Casey <drafnel@gmail.com> wrote:
>
>> I've integrated Duy's series with a few minor tweaks. I added a couple
>> of additional tests to t4014 and corrected one of the tests which had
>> incorrect behavior. I think his sign-off's should still be valid, so I
>> kept them in. Sorry that I've been slow, and now the two of us are stepping
>> on each other's toes, but Duy please take a look and let me know if there's
>> anything you dislike.
>
> I'm still not sure whether format-patch should follow cherry-pick's
> rule in appending sob. If it does, it probably makes more sense to fix
> the sequencer.c code then delete log-tree.c (not fixes on log-tree.c
> then delete it). I see that your changes pass all the new tests I
> added in format-patch so sequencer.c is probably good enough,
> log-tree.c changes are probably not needed. Feel free take over the
> series :-)
After reading the series over, I agree with the above.
Patch #9 that fixes the copy in log-tree.c only to discard it in
patch #11 does not seem to be the best organization of the series.
Instead, perhaps we can salvage the tests in patch #9 (but mark
failing ones as expecting failure) without updating the one in
log-tree.c, adjust prototype in patch #10 (still broken in
log-tree.c) to avoid having to make changes to the callers in patch
#11, and then conclude the series with #11?
Other than the code in patches #06 and #07 that I already commented
on, i.e. assignments in if () condition that make it harder to
follow the logic, I did not find anything majorly objectionable in
the series.
Thanks for a pleasant read.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 00/11] alternative unify appending of sob
2012-11-26 1:45 [PATCH 00/11] alternative unify appending of sob Brandon Casey
` (11 preceding siblings ...)
2012-11-26 7:56 ` [PATCH 00/11] alternative unify appending of sob Nguyen Thai Ngoc Duy
@ 2012-11-26 22:00 ` Junio C Hamano
12 siblings, 0 replies; 18+ messages in thread
From: Junio C Hamano @ 2012-11-26 22:00 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, pclouds, Brandon Casey
Brandon Casey <drafnel@gmail.com> writes:
> So, this series should result in s-o-b and "(cherry picked from...)" being
> appended to commit messages in a more consistent and deterministic way. For
> example, the decision about whether to insert a blank line before appending
> a s-o-b. As discussed, cherry-pick and commit will only refrain from
> appending a s-o-b if the committer's s-o-b appears as the last one in a
> conforming footer, while format-patch will refrain from appending it if it
> appears anywhere within the footer.
Sounds sensible; we may want to fix format-patch later, but with
something like this series, it is just the matter of flipping one
bit.
Will queue. Thanks.
^ permalink raw reply [flat|nested] 18+ messages in thread