On Wed, Jul 1, 2015 at 12:18 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Matthieu Moy <Matthieu.Moy@xxxxxxx> writes:
>> +/*
>> + * Turn
>> + * pick d6a2f0303e897ec257dd0e0a39a5ccb709bc2047 some message
>> + * into
>> + * pick d6a2f03 some message
>> + */
>> +static void abbrev_sha1_in_line(struct strbuf *line)
>> +{
>> + struct strbuf **split;
>> + int i;
>> +
>> + if (starts_with(line->buf, "exec ") ||
>> + starts_with(line->buf, "x "))
>> + return;
>> +
>> + split = strbuf_split_max(line, ' ', 3);
>> + if (split[0] && split[1]) {
>> + unsigned char sha1[20];
>> + const char *abbrev;
>> +
>> + /*
>> + * strbuf_split_max left a space. Trim it and re-add
>> + * it after abbreviation.
>> + */
>> + strbuf_trim(split[1]);
>> + if (!get_sha1(split[1]->buf, sha1)) {
>> + abbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);
>> + strbuf_reset(split[1]);
>> + strbuf_addf(split[1], "%s ", abbrev);
>> + }
>
> ... else?
>
> That is, "we thought there would be a full SHA-1, but it turns out
> that there wasn't, so we keep split[1] as-is" would need to add the
> space back, no?
I was about to mention the same shortcoming, but you beat me to it.
> Perhaps be more strict and do this instead (without
> leading strbuf_trim):
>
> if (!get_sha1_hex(split[1]->buf, sha1) &&
> !strcmp(split[1]->buf + 40, " ") {
> replace split[1] with "%s " abbrev
> }
Isn't it dangerous to assume that you can index 40 characters into
split[1]? If (for some reason), the user botched the todo line such
that the SHA1 is no longer a valid hex string, then split[1] will be
that botched string, which might be shorter than 40 characters. For
instance, if the user-edited todo line is:
pick oops nothing
then git-rebase--interactive.sh:transform_todo_ids() will leave "oops"
as-is, since it can't unabbreviate it, and then this code will place
"oops" into split[1].
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html