* [PATCH 1/5] t/test-lib-functions.sh: allow to specify the tag name to test_commit @ 2012-11-15 1:37 Brandon Casey 2012-11-15 1:37 ` [PATCH 2/5] t/t3511: demonstrate breakage in cherry-pick -s Brandon Casey ` (4 more replies) 0 siblings, 5 replies; 14+ messages in thread From: Brandon Casey @ 2012-11-15 1:37 UTC (permalink / raw) To: git; +Cc: 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..4d6057e 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>. +# Both <file> <contents> and <tag> 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/5] t/t3511: demonstrate breakage in cherry-pick -s 2012-11-15 1:37 [PATCH 1/5] t/test-lib-functions.sh: allow to specify the tag name to test_commit Brandon Casey @ 2012-11-15 1:37 ` Brandon Casey 2012-11-16 1:58 ` Junio C Hamano 2012-11-15 1:37 ` [PATCH 3/5] sequencer.c: handle rfc2822 continuation lines correctly Brandon Casey ` (3 subsequent siblings) 4 siblings, 1 reply; 14+ messages in thread From: Brandon Casey @ 2012-11-15 1:37 UTC (permalink / raw) To: git; +Cc: Brandon Casey, Brandon Casey The cherry-pick -s functionality is currently broken in two ways. 1. handling of rfc2822 continuation lines has a bug, and the continuation lines are not handled correctly. 2. the "(cherry picked from ...)" lines are commonly appended to the end of the s-o-b footer and should be detected as part of the footer. They should not cause a new line to be inserted before the new s-o-b is added. i.e. we should produce this: Signed-off-by: A.U. Thor <author@example.com> (cherry picked from ) Signed-off-by: C O Mmitter <committer@example.com> not Signed-off-by: A.U. Thor <author@example.com> (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709) Signed-off-by: C O Mmitter <committer@example.com> Signed-off-by: Brandon Casey <bcasey@nvidia.com> --- t/t3511-cherry-pick-x.sh | 77 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 77 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..b4e5c65 --- /dev/null +++ b/t/t3511-cherry-pick-x.sh @@ -0,0 +1,77 @@ +#!/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 +} + +non_rfc2822_mesg='base with footer + +Commit message body is here.' + +rfc2822_mesg="$non_rfc2822_mesg + +Signed-off-by: A.U. Thor + <author@example.com> +Signed-off-by: B.U. Thor <buthor@example.com>" + +rfc2822_cherry_mesg="$rfc2822_mesg +(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709) +Tested-by: C.U. Thor <cuthor@example.com>" + + +test_expect_success setup ' + git config advice.detachedhead false && + echo unrelated >unrelated && + git add unrelated && + test_commit initial foo a && + test_commit "$non_rfc2822_mesg" foo b non-rfc2822-base && + git reset --hard initial && + test_commit "$rfc2822_mesg" foo b rfc2822-base && + git reset --hard initial && + test_commit "$rfc2822_cherry_mesg" foo b rfc2822-cherry-base && + pristine_detach initial && + test_commit conflicting unrelated +' + +test_expect_success 'cherry-pick -s inserts blank line after non-rfc2822 footer' ' + pristine_detach initial && + git cherry-pick -s non-rfc2822-base && + cat <<-EOF >expect && + $non_rfc2822_mesg + + 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 not confused by rfc2822 continuation line' ' + pristine_detach initial && + git cherry-pick -s rfc2822-base && + cat <<-EOF >expect && + $rfc2822_mesg + 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 treats -s "(cherry picked from..." line as part of footer' ' + pristine_detach initial && + git cherry-pick -s rfc2822-cherry-base && + cat <<-EOF >expect && + $rfc2822_cherry_mesg + 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/5] t/t3511: demonstrate breakage in cherry-pick -s 2012-11-15 1:37 ` [PATCH 2/5] t/t3511: demonstrate breakage in cherry-pick -s Brandon Casey @ 2012-11-16 1:58 ` Junio C Hamano 2012-11-16 2:40 ` Brandon Casey 0 siblings, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2012-11-16 1:58 UTC (permalink / raw) To: Brandon Casey; +Cc: git, Brandon Casey Brandon Casey <drafnel@gmail.com> writes: > The cherry-pick -s functionality is currently broken in two ways. > > 1. handling of rfc2822 continuation lines has a bug, and the > continuation lines are not handled correctly. This is not limited to you, but people should think twice when writing "has a bug" and "are not handled correctly" in their log message. Did you write what the expected and actual behaviours? > +rfc2822_mesg="$non_rfc2822_mesg > + > +Signed-off-by: A.U. Thor > + <author@example.com> > +Signed-off-by: B.U. Thor <buthor@example.com>" The S-o-b: lines are meant to record people's contact info in human readable forms, and folding the lines like the above makes it a lot harder to read. They typically do not have to be folded. Besides, the footer lines are *not* RFC2822 headers (and are not used as such when send-email comes up with Cc: list) in the first place; have we ever said anything about supporting the RFC2822 line folding in the commit footer? If not (and I am reasonably sure we never have), I personally think we should actively *discourage* line folding there. > i.e. we should produce this: > > Signed-off-by: A.U. Thor <author@example.com> > (cherry picked from ) > Signed-off-by: C O Mmitter <committer@example.com> > > not > > Signed-off-by: A.U. Thor <author@example.com> > (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709) > > Signed-off-by: C O Mmitter <committer@example.com> I can buy that, but then this makes it very clear that these footer lines are not shaped like RFC2822 headers, no? Thanks. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/5] t/t3511: demonstrate breakage in cherry-pick -s 2012-11-16 1:58 ` Junio C Hamano @ 2012-11-16 2:40 ` Brandon Casey 0 siblings, 0 replies; 14+ messages in thread From: Brandon Casey @ 2012-11-16 2:40 UTC (permalink / raw) To: Junio C Hamano; +Cc: git@vger.kernel.org, Brandon Casey On Thu, Nov 15, 2012 at 5:58 PM, Junio C Hamano <gitster@pobox.com> wrote: > Brandon Casey <drafnel@gmail.com> writes: > >> The cherry-pick -s functionality is currently broken in two ways. >> >> 1. handling of rfc2822 continuation lines has a bug, and the >> continuation lines are not handled correctly. > > This is not limited to you, but people should think twice when > writing "has a bug" and "are not handled correctly" in their log > message. Did you write what the expected and actual behaviours? Yeah, I wasn't clear here. The "bug" was that the incorrect index variable was being used, which caused the wrong line to be examined to see if it was an rfc2822 continuation line. I could have mentioned that. >> +rfc2822_mesg="$non_rfc2822_mesg >> + >> +Signed-off-by: A.U. Thor >> + <author@example.com> >> +Signed-off-by: B.U. Thor <buthor@example.com>" > > The S-o-b: lines are meant to record people's contact info in human > readable forms, and folding the lines like the above makes it a lot > harder to read. They typically do not have to be folded. Well, I wasn't adding functionality here, I was only fixing what I noticed was broken when I started touching this code. > Besides, the footer lines are *not* RFC2822 headers (and are not > used as such when send-email comes up with Cc: list) in the first > place; have we ever said anything about supporting the RFC2822 line > folding in the commit footer? If not (and I am reasonably sure we > never have), I personally think we should actively *discourage* line > folding there. That's fine with me. I can't think of a reason that would make it necessary to support line continuation. >> i.e. we should produce this: >> >> Signed-off-by: A.U. Thor <author@example.com> >> (cherry picked from ) >> Signed-off-by: C O Mmitter <committer@example.com> >> >> not >> >> Signed-off-by: A.U. Thor <author@example.com> >> (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709) >> >> Signed-off-by: C O Mmitter <committer@example.com> > > I can buy that, but then this makes it very clear that these footer > lines are not shaped like RFC2822 headers, no? The lines that are _not_ "(cherry picked from ...)" lines do follow the format defined by rfc2822 for header lines (mostly). That's probably why the author of the function in sequencer.c that checks for a s-o-b footer named it "ends_rfc2822_footer". I'll remove the *broken* existing code that was intended to support continuation lines and submit a new patch. -Brandon ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/5] sequencer.c: handle rfc2822 continuation lines correctly 2012-11-15 1:37 [PATCH 1/5] t/test-lib-functions.sh: allow to specify the tag name to test_commit Brandon Casey 2012-11-15 1:37 ` [PATCH 2/5] t/t3511: demonstrate breakage in cherry-pick -s Brandon Casey @ 2012-11-15 1:37 ` Brandon Casey 2012-11-15 1:37 ` [PATCH 4/5] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer Brandon Casey ` (2 subsequent siblings) 4 siblings, 0 replies; 14+ messages in thread From: Brandon Casey @ 2012-11-15 1:37 UTC (permalink / raw) To: git; +Cc: Brandon Casey, Brandon Casey ends_rfc2822_footer() was incorrectly checking whether the current line was a continuation of the previous line. It was actually checking the next line instead of the current line. Let's fix this and mark the test as expect_success. Signed-off-by: Brandon Casey <bcasey@nvidia.com> --- sequencer.c | 2 +- t/t3511-cherry-pick-x.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/sequencer.c b/sequencer.c index be0cb8b..01edec2 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1040,7 +1040,7 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer) ; /* do nothing */ k++; - if ((buf[k] == ' ' || buf[k] == '\t') && !first) + if ((buf[i] == ' ' || buf[i] == '\t') && !first) continue; first = 0; diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh index b4e5c65..b2098e0 100755 --- a/t/t3511-cherry-pick-x.sh +++ b/t/t3511-cherry-pick-x.sh @@ -52,7 +52,7 @@ test_expect_success 'cherry-pick -s inserts blank line after non-rfc2822 footer' test_cmp expect actual ' -test_expect_failure 'cherry-pick -s not confused by rfc2822 continuation line' ' +test_expect_success 'cherry-pick -s not confused by rfc2822 continuation line' ' pristine_detach initial && git cherry-pick -s rfc2822-base && cat <<-EOF >expect && -- 1.8.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/5] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer 2012-11-15 1:37 [PATCH 1/5] t/test-lib-functions.sh: allow to specify the tag name to test_commit Brandon Casey 2012-11-15 1:37 ` [PATCH 2/5] t/t3511: demonstrate breakage in cherry-pick -s Brandon Casey 2012-11-15 1:37 ` [PATCH 3/5] sequencer.c: handle rfc2822 continuation lines correctly Brandon Casey @ 2012-11-15 1:37 ` Brandon Casey 2012-11-15 17:55 ` [PATCH v2 " Brandon Casey 2012-11-15 1:37 ` [PATCH/RFC 5/5] sequencer.c: always separate "(cherry picked from" from commit body Brandon Casey 2012-11-15 3:20 ` [PATCH 1/5] t/test-lib-functions.sh: allow to specify the tag name to test_commit Matt Kraai 4 siblings, 1 reply; 14+ messages in thread From: Brandon Casey @ 2012-11-15 1:37 UTC (permalink / raw) To: git; +Cc: Brandon Casey, Brandon Casey Currently, if the s-o-b footer of a commit message contains a "(cherry picked from ..." line that was added by a previous cherry-pick -x, it is not recognized as a s-o-b footer and will cause a newline to be inserted before an additional s-o-b is added. So, rework ends_rfc2822_footer to recognize the "(cherry picked from ..." string as part of the footer. Plus mark the test in t3511 as fixed. Signed-off-by: Brandon Casey <bcasey@nvidia.com> --- sequencer.c | 44 +++++++++++++++++++++++++++++--------------- t/t3511-cherry-pick-x.sh | 2 +- 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/sequencer.c b/sequencer.c index 01edec2..27e684c 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: "; +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,13 +1018,34 @@ 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 ends_rfc2822_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; - int first = 1; + int last_was_rfc2822 = 0; const char *buf = sb->buf; for (i = len - 1; i > 0; i--) { @@ -1040,20 +1062,12 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer) ; /* do nothing */ k++; - if ((buf[i] == ' ' || buf[i] == '\t') && !first) + if (last_was_rfc2822 && (buf[i] == ' ' || buf[i] == '\t')) continue; - first = 0; - - for (j = 0; i + j < len; j++) { - ch = buf[i + j]; - if (ch == ':') - break; - if (isalnum(ch) || - (ch == '-')) - continue; + if (!((last_was_rfc2822 = is_rfc2822_line(buf+i, k-i)) || + is_cherry_pick_from_line(buf+i, k-i))) return 0; - } } return 1; } diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh index b2098e0..785486e 100755 --- a/t/t3511-cherry-pick-x.sh +++ b/t/t3511-cherry-pick-x.sh @@ -63,7 +63,7 @@ test_expect_success 'cherry-pick -s not confused by rfc2822 continuation line' ' test_cmp expect actual ' -test_expect_failure 'cherry-pick treats -s "(cherry picked from..." line as part of footer' ' +test_expect_success 'cherry-pick treats -s "(cherry picked from..." line as part of footer' ' pristine_detach initial && git cherry-pick -s rfc2822-cherry-base && cat <<-EOF >expect && -- 1.8.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 4/5] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer 2012-11-15 1:37 ` [PATCH 4/5] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer Brandon Casey @ 2012-11-15 17:55 ` Brandon Casey 0 siblings, 0 replies; 14+ messages in thread From: Brandon Casey @ 2012-11-15 17:55 UTC (permalink / raw) To: git; +Cc: Brandon Casey, Brandon Casey Currently, if the s-o-b footer of a commit message contains a "(cherry picked from ..." line that was added by a previous cherry-pick -x, it is not recognized as a s-o-b footer and will cause a newline to be inserted before an additional s-o-b is added. So, rework ends_rfc2822_footer to recognize the "(cherry picked from ..." string as part of the footer. Plus mark the test in t3511 as fixed. Signed-off-by: Brandon Casey <bcasey@nvidia.com> --- Declare cherry_picked_prefix variable as static. This is the only change with respect to v1. -Brandon sequencer.c | 44 +++++++++++++++++++++++++++++--------------- t/t3511-cherry-pick-x.sh | 2 +- 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/sequencer.c b/sequencer.c index 01edec2..213fa4f 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,13 +1018,34 @@ 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 ends_rfc2822_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; - int first = 1; + int last_was_rfc2822 = 0; const char *buf = sb->buf; for (i = len - 1; i > 0; i--) { @@ -1040,20 +1062,12 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer) ; /* do nothing */ k++; - if ((buf[i] == ' ' || buf[i] == '\t') && !first) + if (last_was_rfc2822 && (buf[i] == ' ' || buf[i] == '\t')) continue; - first = 0; - - for (j = 0; i + j < len; j++) { - ch = buf[i + j]; - if (ch == ':') - break; - if (isalnum(ch) || - (ch == '-')) - continue; + if (!((last_was_rfc2822 = is_rfc2822_line(buf+i, k-i)) || + is_cherry_pick_from_line(buf+i, k-i))) return 0; - } } return 1; } diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh index b2098e0..785486e 100755 --- a/t/t3511-cherry-pick-x.sh +++ b/t/t3511-cherry-pick-x.sh @@ -63,7 +63,7 @@ test_expect_success 'cherry-pick -s not confused by rfc2822 continuation line' ' test_cmp expect actual ' -test_expect_failure 'cherry-pick treats -s "(cherry picked from..." line as part of footer' ' +test_expect_success 'cherry-pick treats -s "(cherry picked from..." line as part of footer' ' pristine_detach initial && git cherry-pick -s rfc2822-cherry-base && cat <<-EOF >expect && -- 1.8.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH/RFC 5/5] sequencer.c: always separate "(cherry picked from" from commit body 2012-11-15 1:37 [PATCH 1/5] t/test-lib-functions.sh: allow to specify the tag name to test_commit Brandon Casey ` (2 preceding siblings ...) 2012-11-15 1:37 ` [PATCH 4/5] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer Brandon Casey @ 2012-11-15 1:37 ` Brandon Casey 2012-11-15 23:24 ` [PATCH 6/5] sequencer.c: refrain from adding duplicate s-o-b lines Brandon Casey 2012-11-15 3:20 ` [PATCH 1/5] t/test-lib-functions.sh: allow to specify the tag name to test_commit Matt Kraai 4 siblings, 1 reply; 14+ messages in thread From: Brandon Casey @ 2012-11-15 1:37 UTC (permalink / raw) To: git; +Cc: 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. Also, introduce tests to test this functionality. Signed-off-by: Brandon Casey <bcasey@nvidia.com> --- This seems like the right thing to do, but it's more of a change in policy than the others, so I marked it as RFC. Any disagreement here? -Brandon sequencer.c | 110 ++++++++++++++++++++++++----------------------- t/t3511-cherry-pick-x.sh | 77 +++++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 54 deletions(-) diff --git a/sequencer.c b/sequencer.c index 27e684c..0da0538 100644 --- a/sequencer.c +++ b/sequencer.c @@ -20,6 +20,60 @@ const char sign_off_header[] = "Signed-off-by: "; 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 ends_rfc2822_footer(struct strbuf *sb, int ignore_footer) +{ + int hit = 0; + int i, k; + int len = sb->len - ignore_footer; + int last_was_rfc2822 = 0; + 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 (last_was_rfc2822 && (buf[i] == ' ' || buf[i] == '\t')) + continue; + + if (!((last_was_rfc2822 = 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 (!ends_rfc2822_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,60 +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 ends_rfc2822_footer(struct strbuf *sb, int ignore_footer) -{ - int hit = 0; - int i, k; - int len = sb->len - ignore_footer; - int last_was_rfc2822 = 0; - 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 (last_was_rfc2822 && (buf[i] == ' ' || buf[i] == '\t')) - continue; - - if (!((last_was_rfc2822 = 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 785486e..af7a87c 100755 --- a/t/t3511-cherry-pick-x.sh +++ b/t/t3511-cherry-pick-x.sh @@ -40,6 +40,19 @@ test_expect_success setup ' test_commit conflicting unrelated ' +test_expect_success 'cherry-pick -x inserts blank line after non-rfc2822 footer' ' + pristine_detach initial && + sha1=`git rev-parse non-rfc2822-base^0` && + git cherry-pick -x non-rfc2822-base && + cat <<-EOF >expect && + $non_rfc2822_mesg + + (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 non-rfc2822 footer' ' pristine_detach initial && git cherry-pick -s non-rfc2822-base && @@ -52,6 +65,32 @@ test_expect_success 'cherry-pick -s inserts blank line after non-rfc2822 footer' test_cmp expect actual ' +test_expect_success 'cherry-pick -x -s inserts blank line after non-rfc2822 footer' ' + pristine_detach initial && + sha1=`git rev-parse non-rfc2822-base^0` && + git cherry-pick -x -s non-rfc2822-base && + cat <<-EOF >expect && + $non_rfc2822_mesg + + (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 not confused by rfc2822 continuation line' ' + pristine_detach initial && + sha1=`git rev-parse rfc2822-base^0` && + git cherry-pick -x rfc2822-base && + cat <<-EOF >expect && + $rfc2822_mesg + (cherry picked from commit $sha1) + EOF + git log -1 --pretty=format:%B >actual && + test_cmp expect actual +' + test_expect_success 'cherry-pick -s not confused by rfc2822 continuation line' ' pristine_detach initial && git cherry-pick -s rfc2822-base && @@ -63,6 +102,31 @@ test_expect_success 'cherry-pick -s not confused by rfc2822 continuation line' ' test_cmp expect actual ' +test_expect_success 'cherry-pick -x -s not confused by rfc2822 continuation line' ' + pristine_detach initial && + sha1=`git rev-parse rfc2822-base^0` && + git cherry-pick -x -s rfc2822-base && + cat <<-EOF >expect && + $rfc2822_mesg + (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 treats -x "(cherry picked from..." line as part of footer' ' + pristine_detach initial && + sha1=`git rev-parse rfc2822-cherry-base^0` && + git cherry-pick -x rfc2822-cherry-base && + cat <<-EOF >expect && + $rfc2822_cherry_mesg + (cherry picked from commit $sha1) + EOF + git log -1 --pretty=format:%B >actual && + test_cmp expect actual +' + test_expect_success 'cherry-pick treats -s "(cherry picked from..." line as part of footer' ' pristine_detach initial && git cherry-pick -s rfc2822-cherry-base && @@ -74,4 +138,17 @@ test_expect_success 'cherry-pick treats -s "(cherry picked from..." line as part test_cmp expect actual ' +test_expect_success 'cherry-pick treats -x -s "(cherry picked from..." line as part of footer' ' + pristine_detach initial && + sha1=`git rev-parse rfc2822-cherry-base^0` && + git cherry-pick -x -s rfc2822-cherry-base && + cat <<-EOF >expect && + $rfc2822_cherry_mesg + (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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 6/5] sequencer.c: refrain from adding duplicate s-o-b lines 2012-11-15 1:37 ` [PATCH/RFC 5/5] sequencer.c: always separate "(cherry picked from" from commit body Brandon Casey @ 2012-11-15 23:24 ` Brandon Casey 2012-11-16 2:03 ` Junio C Hamano 0 siblings, 1 reply; 14+ messages in thread From: Brandon Casey @ 2012-11-15 23:24 UTC (permalink / raw) To: pclouds; +Cc: git, Brandon Casey, Brandon Casey Detect whether the s-o-b already exists in the commit footer and refrain from adding a duplicate. Update t3511 to test new behavior. Signed-off-by: Brandon Casey <bcasey@nvidia.com> --- Hi Duy, How about this patch on top of my series as a base for your patch to unify the code paths that append signoff in format-patch, commit, and sequencer? -Brandon sequencer.c | 28 ++++++++++++++++++---------- t/t3511-cherry-pick-x.sh | 20 ++++++++++++++++++-- 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/sequencer.c b/sequencer.c index 7ad1163..546dacb 100644 --- a/sequencer.c +++ b/sequencer.c @@ -42,13 +42,15 @@ static int is_cherry_pick_from_line(const char *buf, int len) !prefixcmp(buf, cherry_picked_prefix); } -static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer) +static int ends_rfc2822_footer(struct strbuf *sb, struct strbuf *sob, + int ignore_footer) { int hit = 0; int i, k; int len = sb->len - ignore_footer; int last_was_rfc2822 = 0; const char *buf = sb->buf; + int found_sob = 0; for (i = len - 1; i > 0; i--) { if (hit && buf[i] == '\n') @@ -66,12 +68,15 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer) if (last_was_rfc2822 && (buf[i] == ' ' || buf[i] == '\t')) continue; + if ((last_was_rfc2822 = is_rfc2822_line(buf+i, k-i)) && + sob && !found_sob && + !strncasecmp(buf+i, sob->buf, sob->len)) + found_sob = 1; - if (!((last_was_rfc2822 = is_rfc2822_line(buf+i, k-i)) || - is_cherry_pick_from_line(buf+i, k-i))) + if (!(last_was_rfc2822 || is_cherry_pick_from_line(buf+i, k-i))) return 0; } - return 1; + return 1 + found_sob; } static void remove_sequencer_state(void) @@ -547,7 +552,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) } if (opts->record_origin) { - if (!ends_rfc2822_footer(&msgbuf, 0)) + if (!ends_rfc2822_footer(&msgbuf, NULL, 0)) strbuf_addch(&msgbuf, '\n'); strbuf_addstr(&msgbuf, cherry_picked_prefix); strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1)); @@ -1077,6 +1082,7 @@ int sequencer_pick_revisions(struct replay_opts *opts) void append_signoff(struct strbuf *msgbuf, int ignore_footer) { struct strbuf sob = STRBUF_INIT; + int has_footer = 0; int i; strbuf_addstr(&sob, sign_off_header); @@ -1085,10 +1091,12 @@ 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 || !ends_rfc2822_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 = + ends_rfc2822_footer(msgbuf, &sob, ignore_footer))) + strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, + "\n", 1); + if (has_footer != 2) + strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, + sob.len); strbuf_release(&sob); } diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh index af7a87c..a15b199 100755 --- a/t/t3511-cherry-pick-x.sh +++ b/t/t3511-cherry-pick-x.sh @@ -11,9 +11,10 @@ pristine_detach () { git clean -d -f -f -q -x } -non_rfc2822_mesg='base with footer +non_rfc2822_mesg="base with footer -Commit message body is here.' +Commit message body is here. +Not an s-o-b Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" rfc2822_mesg="$non_rfc2822_mesg @@ -25,6 +26,9 @@ rfc2822_cherry_mesg="$rfc2822_mesg (cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709) Tested-by: C.U. Thor <cuthor@example.com>" +rfc2822_cherry_sob_mesg="$rfc2822_cherry_mesg +Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> +Signed-off-by: C.U. Thor <cuthor@example.com>" test_expect_success setup ' git config advice.detachedhead false && @@ -36,6 +40,8 @@ test_expect_success setup ' test_commit "$rfc2822_mesg" foo b rfc2822-base && git reset --hard initial && test_commit "$rfc2822_cherry_mesg" foo b rfc2822-cherry-base && + git reset --hard initial && + test_commit "$rfc2822_cherry_sob_mesg" foo b rfc2822-cherry-sob-base && pristine_detach initial && test_commit conflicting unrelated ' @@ -151,4 +157,14 @@ test_expect_success 'cherry-pick treats -x -s "(cherry picked from..." line as p test_cmp expect actual ' +test_expect_success 'cherry-pick -s detects committer s-o-b already exists' ' + pristine_detach initial && + git cherry-pick -s rfc2822-cherry-sob-base && + cat <<-EOF >expect && + $rfc2822_cherry_sob_mesg + EOF + git log -1 --pretty=format:%B >actual && + test_cmp expect actual +' + test_done -- 1.8.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 6/5] sequencer.c: refrain from adding duplicate s-o-b lines 2012-11-15 23:24 ` [PATCH 6/5] sequencer.c: refrain from adding duplicate s-o-b lines Brandon Casey @ 2012-11-16 2:03 ` Junio C Hamano 2012-11-16 23:55 ` Brandon Casey 0 siblings, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2012-11-16 2:03 UTC (permalink / raw) To: Brandon Casey; +Cc: pclouds, git, Brandon Casey Brandon Casey <drafnel@gmail.com> writes: > Detect whether the s-o-b already exists in the commit footer and refrain > from adding a duplicate. If you are trying to forbid git cherry-pick -s $other from adding s-o-b: A when $other ends with these two existing s-o-b: s-o-b: A s-o-b: B then I think that is a wrong thing to do. In such a case, the resulting commit should gain another s-o-b from A to record the provenance as a chain of events. A originally wrote the patch, B forwarded it (possibly with his own tweaks), and then A picked it up and recorded the result to the history, possibly with a final tweak or two. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 6/5] sequencer.c: refrain from adding duplicate s-o-b lines 2012-11-16 2:03 ` Junio C Hamano @ 2012-11-16 23:55 ` Brandon Casey 0 siblings, 0 replies; 14+ messages in thread From: Brandon Casey @ 2012-11-16 23:55 UTC (permalink / raw) To: Junio C Hamano Cc: Nguyễn Thái Ngọc Duy, git@vger.kernel.org, Brandon Casey On Thu, Nov 15, 2012 at 6:03 PM, Junio C Hamano <gitster@pobox.com> wrote: > Brandon Casey <drafnel@gmail.com> writes: > >> Detect whether the s-o-b already exists in the commit footer and refrain >> from adding a duplicate. > > If you are trying to forbid > > git cherry-pick -s $other > > from adding s-o-b: A when $other ends with these two existing s-o-b: > > s-o-b: A > s-o-b: B > > then I think that is a wrong thing to do. > > In such a case, the resulting commit should gain another s-o-b from > A to record the provenance as a chain of events. A originally wrote > the patch, B forwarded it (possibly with his own tweaks), and then A > picked it up and recorded the result to the history, possibly with a > final tweak or two. Hmm. I've never thought that it was necessary to add an additional sob for the patches that I've cherry picked that I had previously signed-off-on. I considered one sign-off to be enough. In your example, A is the committer and the patch set already contains A's sign-off. For me that indicates that A still considers the commit to comply with whatever s-o-b implies. We also seem to have a few tools to help people avoid adding duplicate sob's, like the current behavior of format-patch and the sample commit-msg hook. I did some quick searching through the kernel commits to try to find some examples that could set a precedence. I didn't find anything that supported either argument. I didn't see any commits that were cherry-picked _and_ had an existing sob that was not the last sob. I didn't see any that had duplicate sob lines either. I'm not mentioning this to say that lack of a prior use should mean we should actively disallow the practice of adding duplicate sob's, I'm just providing it as a data point. I've always thought that the reason that 'commit -s' and 'cherry-pick -s' checked only the last line of the commit message was simply that a full scan of the footer had not been implemented. Whichever behavior is determined to be the right way for git to do it, format-patch should be brought in-line with the others and be built on top of the code in sequencer.c. So, if git _should_ create duplicate sob's then this patch should just be dropped. Duy's unification patch can just be built on top of sequencer.c:append_signoff() without bringing over any of the duplicate sob detection from log-tree.c. -Brandon ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] t/test-lib-functions.sh: allow to specify the tag name to test_commit 2012-11-15 1:37 [PATCH 1/5] t/test-lib-functions.sh: allow to specify the tag name to test_commit Brandon Casey ` (3 preceding siblings ...) 2012-11-15 1:37 ` [PATCH/RFC 5/5] sequencer.c: always separate "(cherry picked from" from commit body Brandon Casey @ 2012-11-15 3:20 ` Matt Kraai 2012-11-15 5:43 ` Brandon Casey 2012-11-15 5:49 ` [PATCH 1/5 v2] " Brandon Casey 4 siblings, 2 replies; 14+ messages in thread From: Matt Kraai @ 2012-11-15 3:20 UTC (permalink / raw) To: Brandon Casey; +Cc: git, Brandon Casey On Wed, Nov 14, 2012 at 05:37:50PM -0800, Brandon Casey wrote: > -# Both <file> and <contents> default to <message>. > +# Both <file> <contents> and <tag> default to <message>. I think this line would be better as # <file>, <contents>, and <tag> all default to <message>. since there's now more than two arguments that default to message. -- Matt ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] t/test-lib-functions.sh: allow to specify the tag name to test_commit 2012-11-15 3:20 ` [PATCH 1/5] t/test-lib-functions.sh: allow to specify the tag name to test_commit Matt Kraai @ 2012-11-15 5:43 ` Brandon Casey 2012-11-15 5:49 ` [PATCH 1/5 v2] " Brandon Casey 1 sibling, 0 replies; 14+ messages in thread From: Brandon Casey @ 2012-11-15 5:43 UTC (permalink / raw) To: Matt Kraai; +Cc: git@vger.kernel.org, Brandon Casey Good eye. Thanks. On Wed, Nov 14, 2012 at 7:20 PM, Matt Kraai <kraai@ftbfs.org> wrote: > On Wed, Nov 14, 2012 at 05:37:50PM -0800, Brandon Casey wrote: >> -# Both <file> and <contents> default to <message>. >> +# Both <file> <contents> and <tag> default to <message>. > > I think this line would be better as > > # <file>, <contents>, and <tag> all default to <message>. > > since there's now more than two arguments that default to message. > > -- > Matt ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/5 v2] t/test-lib-functions.sh: allow to specify the tag name to test_commit 2012-11-15 3:20 ` [PATCH 1/5] t/test-lib-functions.sh: allow to specify the tag name to test_commit Matt Kraai 2012-11-15 5:43 ` Brandon Casey @ 2012-11-15 5:49 ` Brandon Casey 1 sibling, 0 replies; 14+ messages in thread From: Brandon Casey @ 2012-11-15 5:49 UTC (permalink / raw) To: kraai; +Cc: git, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2012-11-16 23:55 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-11-15 1:37 [PATCH 1/5] t/test-lib-functions.sh: allow to specify the tag name to test_commit Brandon Casey 2012-11-15 1:37 ` [PATCH 2/5] t/t3511: demonstrate breakage in cherry-pick -s Brandon Casey 2012-11-16 1:58 ` Junio C Hamano 2012-11-16 2:40 ` Brandon Casey 2012-11-15 1:37 ` [PATCH 3/5] sequencer.c: handle rfc2822 continuation lines correctly Brandon Casey 2012-11-15 1:37 ` [PATCH 4/5] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer Brandon Casey 2012-11-15 17:55 ` [PATCH v2 " Brandon Casey 2012-11-15 1:37 ` [PATCH/RFC 5/5] sequencer.c: always separate "(cherry picked from" from commit body Brandon Casey 2012-11-15 23:24 ` [PATCH 6/5] sequencer.c: refrain from adding duplicate s-o-b lines Brandon Casey 2012-11-16 2:03 ` Junio C Hamano 2012-11-16 23:55 ` Brandon Casey 2012-11-15 3:20 ` [PATCH 1/5] t/test-lib-functions.sh: allow to specify the tag name to test_commit Matt Kraai 2012-11-15 5:43 ` Brandon Casey 2012-11-15 5:49 ` [PATCH 1/5 v2] " Brandon Casey
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).