Re: [PATCH 1/2] sha1_name: stop hard-coding 40-character hex checks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Jun 15, 2013 at 1:38 PM, Ramkumar Ramachandra
<artagnon@xxxxxxxxx> wrote:
> In two places, get_sha1_basic() assumes that strings are possibly sha1
> hexes if they are 40 characters long, and calls get_sha1_hex() in these
> two cases.  This 40-character check is ugly and wrong: there is nothing
> preventing a revision or branch name from being exactly 40 characters.
> Replace it with a call to the more robust get_short_sha1().

I share your disdain for the bare '40's in the code.  But I think this
code is less clear than the previous version with the magic number.

>
> Signed-off-by: Ramkumar Ramachandra <artagnon@xxxxxxxxx>
> ---
>  sha1_name.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 90419ef..d862af3 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -451,7 +451,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>         int refs_found = 0;
>         int at, reflog_len, nth_prior = 0;
>
> -       if (len == 40 && !get_sha1_hex(str, sha1)) {
> +       if (!get_short_sha1(str, strlen(str), sha1, GET_SHA1_QUIETLY)) {

Use len instead of strlen(str) here.  It's faster and more correct.

But also get_short_sha1 is much heavier than get_sha1_hex and does not
seem appropriate here.

>                 refs_found = dwim_ref(str, len, tmp_sha1, &real_ref);
>                 if (refs_found > 0 && warn_ambiguous_refs) {
>                         warning(warn_msg, len, str);
> @@ -492,9 +492,9 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>                 int detached;
>
>                 if (interpret_nth_prior_checkout(str, &buf) > 0) {
> -                       detached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));
> +                       detached = get_short_sha1(buf.buf, buf.len, sha1, GET_SHA1_QUIETLY);
>                         strbuf_release(&buf);
> -                       if (detached)
> +                       if (detached != SHORT_NAME_NOT_FOUND)

The semantic meaning of 'detached' seems less clear now if you have to
compare against an enumerated constant to determine the result.  But
also, I do not see why you have to test '!= SHORT_NAME_NOT_FOUND' here
but you did not have to in the other instance.

I think it would be improved if you did this comparison in the
assignment of detached so 'detached' could keep its original boolean
meaning.

But anyway, having looked inside get_short_sha1, it really does seem
to do much more than you want here.

>                                 return 0;
>                 }
>         }
> --
> 1.8.3.1.438.g96d34e8
>
> --
> 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
--
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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]