Hi Jonathan,
I fixed everything; just a few comments/ doubts:
Jonathan Nieder writes:
>> } else
>> - return NULL;
>> + return -1;
>
> Unrelated to this patch: maybe
>
> return error("unrecognized action in sequencer file: %s", start);
>
> to be easier to debug.
I'd initially refrained from doing this so that errors don't become
overtly verbose, but I suppose it's alright considering the fact that
we're going to make the instruction sheet editable sometime in the
future. I tweaked the error strings a little so that we get something
like:
error: Unrecognized action: part
error: Could not parse line 3.
fatal: Unusable instruction sheet: .git/sequencer
In essence, I didn't want to be redundant and mention the sequencer in
every line. I like the above.
>> q = strchr(p, ' ');
>> if (!q)
>> - return NULL;
>> + return -1;
>
> So we reject "pick a87c8989"? That's a shame.
Good point. Fixing this will have to be in another patch, where I'll
advertise the fact that I'm changing the instruction sheet format.
>> q++;
>>
>> strlcpy(sha1_abbrev, p, q - p);
>
> memcpy would be clearer. Can't this overflow the sha1_abbrev buffer?
Good point. I'm tempted to check (q - p < 40); is there a better way
to do this by not hardcoding "40" perhaps?
> Acked-by: Jonathan Nieder <jrnieder@xxxxxxxxx>
Thanks.
-- Ram
--
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