Thread (1 message) 1 message, 1 author, 2016-06-15

Re: [PATCH 4/6] revert: Make commit descriptions in insn sheet optional

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:52:13

Ramkumar Ramachandra [off-list ref] writes:
quoted hunk
Change the instruction sheet format subtly so that a description of
the commit after the object name is optional.  As a result, an
instruction sheet like this is now perfectly valid:

  pick 35b0426
  pick fbd5bbcbc2e
  pick 7362160f

Suggested-by: Jonathan Nieder <redacted>
Acked-by: Jonathan Nieder <redacted>
Signed-off-by: Ramkumar Ramachandra <redacted>
---
 builtin/revert.c                |   19 ++++++++-----------
 t/t3510-cherry-pick-sequence.sh |   14 ++++++++++++++
 2 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 6451089..aa6c34e 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -692,26 +692,23 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
 	unsigned char commit_sha1[20];
 	char sha1_abbrev[40];
 	enum replay_action action;
-	int insn_len = 0;
-	char *p, *q;
+	const char *p, *q;
 
+	p = start;
 	if (!prefixcmp(start, "pick ")) {
 		action = CHERRY_PICK;
-		insn_len = strlen("pick");
-		p = start + insn_len + 1;
+		p += strlen("pick ");
 	} else if (!prefixcmp(start, "revert ")) {
 		action = REVERT;
-		insn_len = strlen("revert");
-		p = start + insn_len + 1;
+		p += strlen("revert ");
 	} else
 		return NULL;
 
-	q = strchr(p, ' ');
-	if (!q || q - p + 1 > sizeof(sha1_abbrev))
+	q = p + strcspn(p, " \n");
+	if (q - p + 1 > sizeof(sha1_abbrev))
 		return NULL;
-	q++;
-
-	strlcpy(sha1_abbrev, p, q - p);
+	memcpy(sha1_abbrev, p, q - p);
+	sha1_abbrev[q - p] = '\0';
Since this is a part of clean-up series...

Do you even need to have a sha1_abbrev[] local array that is limited to 40
bytes here? The incoming _line_ is not "const char *start", so you should
at least be able to temporarily terminate the commit object name with NUL
(while remembering what byte there was before), give it to get_sha1(), and
then restore the byte at the end before returning from this function.

A bonus point would be to introduce a variant of get_sha1() that can take
(a pointer + len) not (a pointer to NUL terminated string). While I think
that would be a right thing to do in the longer term, it is outside of the
scope of this series.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help