Re: [PATCH v5 3/4] status: give more information during rebase -i

Eric Sunshine [off-list ref] writes:

I was about to mention the same shortcoming, but you beat me to it.

quoted

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].

Yeah, that is why get_sha1_hex() is checked before we try to make
sure buf[40] has " " in the code snippet you quoted.

Isn't that how && short-cut works?

On Wed, Jul 1, 2015 at 12:36 PM, Junio C Hamano [off-list ref] wrote:

Eric Sunshine [off-list ref] writes:

quoted

I was about to mention the same shortcoming, but you beat me to it.

quoted

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].

Yeah, that is why get_sha1_hex() is checked before we try to make
sure buf[40] has " " in the code snippet you quoted.

Isn't that how && short-cut works?

Argh, I misread the code. Sorry for the noise.