Junio C Hamano <gitster@xxxxxxxxx> writes:
> Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx> writes:
>
>> Actually, we can do simpler: we still have the original line available,
>> ...
>>
>> I took this (modulo s/line.len[0]/line.buf[0]/, and s/rtrim/trim/ to be
>> robust to leading whitespace (not really important, but doesn't harm).
>
> I'd prefer us to be more strict when we know we are reading our own
> output; rtrim is sensible, as the log line has end-user subject
> the end and the subject might have a trailing whitespace we want to
> trim, but there is no valid reason to expect leading whitespace.
I would agree with "more strict" is it was about rejecting the input (to
catch errors), but here we're still accepting it without complaining if
it has leading whitespaces. So, being strict does not seem to have any
benefit to me. Being a bit more robust is just one character _less_ in
the code.
Actually, there's a hidden benefit in accepting not-well-formatted
input: it mimicks the shell equivalent closer, which means that we're
close to replacing the shell's collapse_todo_ids and expand_todo_ids in
C which would avoid C/shell duplication.
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1046,12 +1046,8 @@ static void abbrev_sha1_in_line(struct strbuf *line)
> 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)) {
> + if (!get_sha1_hex(split[1]->buf, sha1) &&
> + !strcmp(split[1]->buf + 40, " ")) {
> abbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);
> strbuf_reset(split[1]);
> strbuf_addf(split[1], "%s ", abbrev);
I prefer my version. It's simpler: when we can't expand, keep the
original "line", don't try to deconstruct/reconstruct it, it's
unmodified by construction. And my version works if the line is just
"pick <sha1>\n", while yours doesn't.
About get_sha1_hex Vs get_sha1, the same argument as above applies:
accepting short sha1s here means that implementing expand_todo_ids based
on the function would be relatively easy.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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