From: Junio C Hamano <gitster@pobox•com>
To: Namhyung Kim <namhyung@gmail•com>
Cc: Junio C Hamano <gitster@pobox•com>, git@vger•kernel.org
Subject: Re: [PATCH 1/2] sha1_name: try to use same abbrev length when core.abbrevguard is specified
Date: Wed, 09 Mar 2011 11:12:00 -0800 [thread overview]
Message-ID: <7vipvsaz27.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <1299581951-4670-1-git-send-email-namhyung@gmail.com> (Namhyung Kim's message of "Tue, 8 Mar 2011 19:59:10 +0900")
Namhyung Kim <namhyung@gmail•com> writes:
> If find_unique_abbrev() finds a ambiguous SHA1 name, it tries
> to find again with increased length. In this case, result hex
> strings could have different lengths even though the
> core.abbrevguard config option is specified. But if the option
> is specified and increased length (delta) is less than its
> value, the result could be adjusted to the same length.
I am not sure if I can understand what problem you are trying to solve
from the above description.
The function is given "len" from the caller to specify the minimum length
of the output the caller expects (i.e. even if 4 hexdigits is enough to
identify the given commit in a small project, the caller can say it wants
to see at least 7 hexdigits). The loop without your patch finds the
shortest prefix whose length is at least that given length that uniquely
identifies the given object (or the shortest prefix that doesn't identify
any existing object if the given sha1 does not exist in the repository).
And then ensures the returned value is longer by the guard as an extra
safety measure, so that later when the project grows, the disambiguation
we find today has a better chance to survive.
With this patch, the loop decreases the length of the guard when "len"
given by the caller is insufficient to ensure uniqueness, which does not
sound right.
Suppose the given object has ambiguous other objects and you need 8
hexdigits at least to make it unique in today's history. The caller gives
you len of 7, and the guard is set to 3.
With the original code, the loop starts with 7, finds that it is not
long enough to disambiguate, increments and retries, finds that 8 is the
shortest prefix, and then adds the guard and returns 11 hexdigits.
With your patch, the loop starts with 7 with extra set to 3, finds that 7
is not long enough and decrements extra to 2, finds that 8 is the shortest
prefix, and then returns only 10 hexdigits.
Which feels like totally going against the reason why we added the guard.
What am I missing?
> sha1_name.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 709ff2e..6bb8942 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -197,6 +197,7 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
> {
> int status, exists;
> static char hex[41];
> + int extra_len = unique_abbrev_extra_length;
>
> exists = has_sha1_file(sha1);
> memcpy(hex, sha1_to_hex(sha1), 40);
> @@ -208,12 +209,14 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
> if (exists
> ? !status
> : status == SHORT_NAME_NOT_FOUND) {
> - int cut_at = len + unique_abbrev_extra_length;
> + int cut_at = len + extra_len;
> cut_at = (cut_at < 40) ? cut_at : 40;
> hex[cut_at] = 0;
> return hex;
> }
> len++;
> + if (extra_len > 0)
> + extra_len--;
> }
> return hex;
> }
next prev parent reply other threads:[~2011-03-09 19:12 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-03-08 10:59 [PATCH 1/2] sha1_name: try to use same abbrev length when core.abbrevguard is specified Namhyung Kim
2011-03-08 10:59 ` [PATCH 2/2] blame: introduce -u/--unique option Namhyung Kim
2011-03-09 19:45 ` Junio C Hamano
2011-03-10 6:13 ` Namhyung Kim
2011-03-10 9:43 ` Junio C Hamano
2011-03-10 13:58 ` Namhyung Kim
2011-03-09 19:12 ` Junio C Hamano [this message]
2011-03-09 19:20 ` [PATCH 1/2] sha1_name: try to use same abbrev length when core.abbrevguard is specified Junio C Hamano
2011-03-10 9:19 ` Re* " Junio C Hamano
2011-03-10 11:52 ` Namhyung Kim
2011-03-10 11:54 ` Namhyung Kim
2011-03-10 19:11 ` Junio C Hamano
2011-03-10 5:27 ` Namhyung Kim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7vipvsaz27.fsf@alter.siamese.dyndns.org \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=namhyung@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox