[PATCH v4 05/12] 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 aut...@example.com
(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
Signed-off-by: C O Mmitter commit...@example.com
instead of this:
Signed-off-by: A U Thor aut...@example.com
(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
Signed-off-by: C O Mmitter commit...@example.com
[with improvements from Jonathan Nieder]
Signed-off-by: Brandon Casey bca...@nvidia.com
Acked-by: Jonathan Nieder jrnie...@gmail.com
---
sequencer.c | 51
t/t3511-cherry-pick-x.sh | 55
2 files changed, 92 insertions(+), 14 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index aa2cb8e..93495b0 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,16 +1022,44 @@ 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)
{
- char ch, prev;
- int i, j, k;
+ int i;
+
+ for (i = 0; i len; i++) {
+ int ch = buf[i];
+ if (ch == ':')
+ return 1;
+ if (!isalnum(ch) ch != '-')
+ break;
+ }
+
+ return 0;
+}
+
+static int is_cherry_picked_from_line(const char *buf, int len)
+{
+ /*
+* We only care that it looks roughly like (cherry picked from ...)
+*/
+ return len strlen(cherry_picked_prefix) + 1
+ !prefixcmp(buf, cherry_picked_prefix) buf[len - 1] == ')';
+}
+
+static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
+{
+ char prev;
+ int i, k;
int len = sb-len - ignore_footer;
const char *buf = sb-buf;
+ /* footer must end with newline */
+ if (!len || buf[len - 1] != '\n')
+ return 0;
+
prev = '\0';
for (i = len - 1; i 0; i--) {
- ch = buf[i];
+ char ch = buf[i];
if (prev == '\n' ch == '\n') /* paragraph break */
break;
prev = ch;
@@ -1045,15 +1074,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 - 1)
+ !is_cherry_picked_from_line(buf + i, k - i - 1))
return 0;
- }
}
return 1;
}
@@ -1070,7 +1093,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 2a040b7..73da182 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:
Re: [PATCH v4 05/12] sequencer.c: recognize (cherry picked from ... as part of s-o-b footer
Brandon Casey draf...@gmail.com writes:
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:
...
+static int is_cherry_picked_from_line(const char *buf, int len)
+{
+ /*
+ * We only care that it looks roughly like (cherry picked from ...)
+ */
+ return len strlen(cherry_picked_prefix) + 1
+ !prefixcmp(buf, cherry_picked_prefix) buf[len - 1] == ')';
+}
Does the first is it longer than the prefix? check matter? If it
is not, prefixcmp() would not match anyway, no?
--
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 v4 05/12] sequencer.c: recognize (cherry picked from ... as part of s-o-b footer
On 2/12/2013 11:13 AM, Junio C Hamano wrote:
Brandon Casey draf...@gmail.com writes:
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:
...
+static int is_cherry_picked_from_line(const char *buf, int len)
+{
+/*
+ * We only care that it looks roughly like (cherry picked from ...)
+ */
+return len strlen(cherry_picked_prefix) + 1
+!prefixcmp(buf, cherry_picked_prefix) buf[len - 1] == ')';
+}
Does the first is it longer than the prefix? check matter? If it
is not, prefixcmp() would not match anyway, no?
Probably not in practice, but technically we should only be accessing
len characters in buf even though buf may be longer than len. So the
check is just making sure the function doesn't access chars it's not
supposed to.
-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.
---
--
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 v4 05/12] sequencer.c: recognize (cherry picked from ... as part of s-o-b footer
Brandon Casey bca...@nvidia.com writes: + return len strlen(cherry_picked_prefix) + 1 + !prefixcmp(buf, cherry_picked_prefix) buf[len - 1] == ')'; +} Does the first is it longer than the prefix? check matter? If it is not, prefixcmp() would not match anyway, no? Probably not in practice, but technically we should only be accessing len characters in buf even though buf may be longer than len. So the check is just making sure the function doesn't access chars it's not supposed to. Sorry, I do not follow. Isn't caller's buf terminated with LF at buf[len], which would never match cherry_picked_prefix even if len is shorter than the prefix? -- 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 v4 05/12] sequencer.c: recognize (cherry picked from ... as part of s-o-b footer
Brandon Casey wrote:
On 2/12/2013 11:13 AM, Junio C Hamano wrote:
Brandon Casey draf...@gmail.com writes:
+static int is_cherry_picked_from_line(const char *buf, int len)
+{
+ /*
+* We only care that it looks roughly like (cherry picked from ...)
+*/
+ return len strlen(cherry_picked_prefix) + 1
+ !prefixcmp(buf, cherry_picked_prefix) buf[len - 1] == ')';
+}
Does the first is it longer than the prefix? check matter? If it
is not, prefixcmp() would not match anyway, no?
Probably not in practice, but technically we should only be accessing
len characters in buf even though buf may be longer than len.
Yep. Technically the buf[len - 1] == ')' check is enough to avoid
false positives, but if it and the 'len' check were dropped then this
would be checking that buf is a (cherry-picked from line instead of
checking that its first 'len' bytes are one.
So it's just paranoid futureproofing. In the long term, it would be
nice to drop the number of bytes to ignore at the end argument to
append_signoff to avoid having to think about this kind of thing.
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 v4 05/12] sequencer.c: recognize (cherry picked from ... as part of s-o-b footer
On 2/12/2013 11:36 AM, Junio C Hamano wrote: Brandon Casey bca...@nvidia.com writes: + return len strlen(cherry_picked_prefix) + 1 + !prefixcmp(buf, cherry_picked_prefix) buf[len - 1] == ')'; +} Does the first is it longer than the prefix? check matter? If it is not, prefixcmp() would not match anyway, no? Probably not in practice, but technically we should only be accessing len characters in buf even though buf may be longer than len. So the check is just making sure the function doesn't access chars it's not supposed to. Sorry, I do not follow. Isn't caller's buf terminated with LF at buf[len], which would never match cherry_picked_prefix even if len is shorter than the prefix? Heh, I almost pointed that out in my reply. Yes, buf will be terminated with LF at buf[len]. And yes, that means that we will never get a false positive from prefixcmp even if the comparison overruns buf+len while doing its comparison. That's why the check doesn't matter in practice, i.e. based on the way that is_cherry_picked_from_line is being called right now and the content of cherry_picked_prefix. But, hasn't is_cherry_picked_from_line entered into a contract with the caller and said I will not access more than len characters? It's ok with me if you think it reads better without the check. -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. --- -- 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 v4 05/12] sequencer.c: recognize (cherry picked from ... as part of s-o-b footer
Brandon Casey bca...@nvidia.com writes: On 2/12/2013 11:36 AM, Junio C Hamano wrote: Brandon Casey bca...@nvidia.com writes: + return len strlen(cherry_picked_prefix) + 1 + !prefixcmp(buf, cherry_picked_prefix) buf[len - 1] == ')'; +} Does the first is it longer than the prefix? check matter? If it is not, prefixcmp() would not match anyway, no? Probably not in practice, but technically we should only be accessing len characters in buf even though buf may be longer than len. So the check is just making sure the function doesn't access chars it's not supposed to. Sorry, I do not follow. Isn't caller's buf terminated with LF at buf[len], which would never match cherry_picked_prefix even if len is shorter than the prefix? Heh, I almost pointed that out in my reply. Yes, buf will be terminated with LF at buf[len]. And yes, that means that we will never get a false positive from prefixcmp even if the comparison overruns buf+len while doing its comparison. That's why the check doesn't matter in practice, i.e. based on the way that is_cherry_picked_from_line is being called right now and the content of cherry_picked_prefix. But, hasn't is_cherry_picked_from_line entered into a contract with the caller and said I will not access more than len characters? It's ok with me if you think it reads better without the check. As Jonathan says, if you rewrite it to return buf[len - 1] == ')' !prefixcmp(buf, cherry_picked_prefix); then the code can keep its promise without the length check, because it knows there is no ')' in cherry-picked-prefix, and it also knows prefixcmp() stops at the first difference. It is not a huge deal; I was primarily reacting to the ugly multi-line boolean expresion that is not inside a pair of parentheses (and because this is a return statement, there is no good reason to have parentheses except that this is a multi-line expression), which looked odd. -- 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
