Re: [PATCH 02/12] load_subtree(): remove unnecessary conditional

[Junio C Hamano <gitster@pobox.com>]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> At this point in the code, len is *always* <= 20.

This is the kind of log message that makes me unconfortable, as it
lacks "because", and the readers would need to find out themselves
by following the same codepath the patch author already followed.

There is an assert earlier before the control gets in this loop

	prefix_len = subtree->key_oid.hash[KEY_INDEX];
	assert(prefix_len * 2 >= n);
	memcpy(object_oid.hash, subtree->key_oid.hash, prefix_len);

that tries to ensure there is sufficient number of prefix defined in
that key, and the codeflow may ensure that prefix_len is both an
even number and shorter than 20 (the correctness of the code depends
on these, it seems, and if for some reason prefix_len is much
larger, calls to get_oid_hex_segment() will overflow the oid.hash[]
array without checking).  I'd at least feel safer to have an assert
next to the existing one that catches a bug to throw a randomly
large value into subtree->key_oid.hash[KEY_INDEX].  Then we can
safely say "at this point in the code, len is always <= 20", as that
assert will makes it obvious without looking at anything other than
this code and get_oid_hex_segment() implementaiton (combined with
the fact that this function is the only one that coerces len and
puts it into ->key_oid.hash[KEY_INDEX], but that is a weak assurance
as we cannot tell where "subtree" came from---it may have full
20-byte oid in its key_oid field---without following the callchain a
lot more widely).

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  notes.c | 35 +++++++++++++++++------------------
>  1 file changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/notes.c b/notes.c
> index 00630a9396..f7ce64ff48 100644
> --- a/notes.c
> +++ b/notes.c
> @@ -446,25 +446,24 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,
>  		 * If object SHA1 is incomplete (len < 20), and current
>  		 * component consists of 2 hex chars, assume note subtree
>  		 */
> -		if (len <= GIT_SHA1_RAWSZ) {
> -			type = PTR_TYPE_NOTE;
> -			l = (struct leaf_node *)
> -				xcalloc(1, sizeof(struct leaf_node));
> -			oidcpy(&l->key_oid, &object_oid);
> -			oidcpy(&l->val_oid, entry.oid);
> -			if (len < GIT_SHA1_RAWSZ) {
> -				if (!S_ISDIR(entry.mode) || path_len != 2)
> -					goto handle_non_note; /* not subtree */
> -				l->key_oid.hash[KEY_INDEX] = (unsigned char) len;
> -				type = PTR_TYPE_SUBTREE;
> -			}
> -			if (note_tree_insert(t, node, n, l, type,
> -					     combine_notes_concatenate))
> -				die("Failed to load %s %s into notes tree "
> -				    "from %s",
> -				    type == PTR_TYPE_NOTE ? "note" : "subtree",
> -				    oid_to_hex(&l->key_oid), t->ref);
> +		type = PTR_TYPE_NOTE;
> +		l = (struct leaf_node *)
> +			xcalloc(1, sizeof(struct leaf_node));
> +		oidcpy(&l->key_oid, &object_oid);
> +		oidcpy(&l->val_oid, entry.oid);
> +		if (len < GIT_SHA1_RAWSZ) {
> +			if (!S_ISDIR(entry.mode) || path_len != 2)
> +				goto handle_non_note; /* not subtree */
> +			l->key_oid.hash[KEY_INDEX] = (unsigned char) len;
> +			type = PTR_TYPE_SUBTREE;
>  		}
> +		if (note_tree_insert(t, node, n, l, type,
> +				     combine_notes_concatenate))
> +			die("Failed to load %s %s into notes tree "
> +			    "from %s",
> +			    type == PTR_TYPE_NOTE ? "note" : "subtree",
> +			    oid_to_hex(&l->key_oid), t->ref);
> +
>  		continue;
>  
>  handle_non_note: