Re: [PATCH v2 04/10] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
Hi,
Brandon Casey wrote:
> Let's detect "(cherry picked from...)" as part of the footer so that we
> will produce this:
>
>Signed-off-by: A U Thor
>(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
>Signed-off-by: C O Mmitter
>
> instead of this:
>
>Signed-off-by: A U Thor
>(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
>
>Signed-off-by: C O Mmitter
My last review was based on a misunderstanding of
ends_rfc2822_footer(). This time I can unreservedly say I like this.
[...]
> --- 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 ";
The static/nonstatic mismatch looks strange. Not about this patch,
but probably rest_is_empty() from builtin-commit.c should move here
some day.
[...]
> @@ -1021,11 +1022,36 @@ int sequencer_pick_revisions(struct replay_opts *opts)
[...]
> +static int is_cherry_picked_from_line(const char *buf, int len)
> +{
> + /*
> + * We only care that it looks roughly like (cherry picked from ...)
> + */
> + return !prefixcmp(buf, cherry_picked_prefix) &&
> + (buf[len - 1] == ')' ||
> + (buf[len - 1] == '\n' && buf[len - 2] == ')'));
These two cases are based on whether the commit message ended with
a '(cherry picked from' with no trailing newline, I guess?
I wonder if switching the convention for 'k' would make this simpler:
const char *line, *eol;
line = buf + i;
eol = memchr(buf, '\n', len - i);
if (!eol)
eol = buf + len;
if (!is_rfc2822_field(line, eol - line) &&
!is_cherry_picked_from_line(line, eol - line))
return 0;
I notice you just sent a new version of the series. I'll try this out
on top of that.
Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/10] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
Brandon Casey wrote: > Let's detect "(cherry picked from...)" as part of the footer so that we > will produce this: > >Signed-off-by: A U Thor >(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709) >Signed-off-by: C O Mmitter > > instead of this: > >Signed-off-by: A U Thor >(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709) > >Signed-off-by: C O Mmitter Yes, looks sane. A downside is that this produces an arguably worse result when using "-x -s" for a commit that does not already have a sign-off. Before, we had: test: do something great Do something fantastic in a clean and elegant way that only takes two lines of explanation. (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709) Signed-off-by: C H Errypicker Afterwards, we will have: test: do something great Do something fantastic in a clean and elegant way that only takes two lines of explanation. (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709) Signed-off-by: C H Errypicker An ideal result would be completely different: test: do something great commit da39a3ee5e6b4b0d3255bfef95601890afd80709 upstream. Do something fantastic in a clean and elegant way that only takes two lines of explanation. Signed-off-by: C H Errypicker In other words, the -x output format that puts the commit id at the end with odd spacing seems to be of questionable taste anyway. But given the constraint of leaving that alone, cramming together the sign-off like this seems like the best we can do, so for what it's worth, Acked-by: Jonathan Nieder -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 04/10] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer
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
(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
Signed-off-by: C O Mmitter
instead of this:
Signed-off-by: A U Thor
(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
Signed-off-by: C O Mmitter
Signed-off-by: Brandon Casey
---
sequencer.c | 46
t/t3511-cherry-pick-x.sh | 55
2 files changed, 88 insertions(+), 13 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index fe25ef4..fe76a1d 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)
{
@@ -496,7 +497,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");
}
@@ -1021,11 +1022,36 @@ 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_picked_from_line(const char *buf, int len)
+{
+ /*
+* We only care that it looks roughly like (cherry picked from ...)
+*/
+ return !prefixcmp(buf, cherry_picked_prefix) &&
+ (buf[len - 1] == ')' ||
+(buf[len - 1] == '\n' && buf[len - 2] == ')'));
+}
+
+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;
@@ -1043,15 +1069,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_picked_from_line(buf + i, k - i)))
return 0;
- }
}
return 1;
}
@@ -1068,7 +1088,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 "
+
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 i
