Re: [PATCH v3 3/5] sha1_name: Unroll len loop in find_unique_abbrev_r

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 4, 2017, 06:07 UTC
Message-ID: <xmqqtvzfcuoy.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171002145651.204984-4-dstolee@microsoft.com>

Derrick Stolee <dstolee@microsoft.com> writes:

Show 22 quoted lines

> -int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)
> +struct min_abbrev_data {
> +	unsigned int init_len;
> +	unsigned int cur_len;
> +	char *hex;
> +};
> +
> +static int extend_abbrev_len(const struct object_id *oid, void *cb_data)
>  {
> -	int status, exists;
> +	struct min_abbrev_data *mad = cb_data;
> +
> +	char *hex = oid_to_hex(oid);
> +	unsigned int i = mad->init_len;
> +	while (mad->hex[i] && mad->hex[i] == hex[i])
> +		i++;
> +
> +	if (i < GIT_MAX_RAWSZ && i >= mad->cur_len)
> +		mad->cur_len = i + 1;
> +
> +	return 0;
> +}

The idea is to keep the target to be abbreviated in mad->hex[], and as the two functions find_short_{object_filename,packed_object} discover objects whose first 'len' letters of hexname are the same as the target, they'd call into the "always_call_fn" callback, which is this function, and it keeps track of the maximum of the common prefix it has seen. Which makes sense. Well, sort of.

Show 13 quoted lines

> -	exists = has_sha1_file(sha1);
> -	while (len < GIT_SHA1_HEXSZ) {
> -		struct object_id oid_ret;
> -		status = get_short_oid(hex, len, &oid_ret, GET_OID_QUIETLY);
> -		if (exists
> -		    ? !status
> -		    : status == SHORT_NAME_NOT_FOUND) {
> -			hex[len] = 0;
> -			return len;
> -		}
> -		len++;
> -	}
> -	return len;

The "always_call_fn" thing is a big sledgehammer that overrides everything else in update_candidates(). It bypasses the careful machinery set up to avoid having to open ambiguous object to learn their types as much as possible. One narrow exception when it is OK to use is if we never limit our candidates with type.

And it might appear that the conversion is safe (if only because we do not see any type limitation in the get_short_oid() call above), but I think there is one case where this patch changes the behaviour: what happens if core.disambiguate was set to anything other than "none"? The new code does not know anything about type based filtering, so it can end up reporting longer abbreviation than it was asked to produce. It may not be a problem in practice, though.

I am not sure if setting core.disambiguate is generally a good idea in the first place, and if it is OK to break find_unique_abbrev() with respect to the configuration variable like this patch does.

I'd feel safe if we get extra input from Peff, who introduced the feature in 5b33cb1f ("get_short_sha1: make default disambiguation configurable", 2016-09-27).

Show 21 quoted lines

> +
> +	if (init_object_disambiguation(hex, len, &ds) < 0)
> +		return -1;
> +
> +	mad.init_len = len;
> +	mad.cur_len = len;
> +	mad.hex = hex;
> +
> +	ds.fn = extend_abbrev_len;
> +	ds.always_call_fn = 1;
> +	ds.cb_data = (void*)&mad;
> +
> +	find_short_object_filename(&ds);
> +	find_short_packed_object(&ds);
> +	(void)finish_object_disambiguation(&ds, &oid_ret);
> +
> +	hex[mad.cur_len] = 0;
> +	return mad.cur_len;
>  }
>  
>  const char *find_unique_abbrev(const unsigned char *sha1, int len)