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.