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

[Junio C Hamano]

Michael Haggerty  writes:

> Junio, I'm surprised that you have merged the `mh/notes-cleanup`
> branch into `next` already. Was that intentional?

Yup, I was clearing the deck as much as possible before I go
offline, as there didn't seem to be any glaring problem that we do
not even want to see in the history of our codebase in the series,
and I thought it would be better to give wider exposure early, as
long as small improvements are done incrementally.

Thanks.

ps. I'll still be offline for a bit more, so please do not get
disappointed if your updates do not show up in my tree until I come
back.

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

[Michael Haggerty]

Junio, I'm surprised that you have merged the `mh/notes-cleanup`
branch into `next` already. Was that intentional? Aside from the fact
that the topic has had very little cooking time, there's the issue of
the assertion that you asked for. I have implemented the assertion in
a new version of the branch that I pushed to GitHub but haven't yet
submitted to the list.

How would you like to proceed? Do you want me to submit a patch
(adding the assertion) that applies on top of this branch?

Michael

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

[Michael Haggerty]

On Sat, Aug 26, 2017 at 6:38 PM, Junio C Hamano  wrote:
> Michael Haggerty  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.
> [...]

That's a valid complaint. I've adjusted the patch series to add the
assertion and explain the reasoning better in the commit message. I've
pushed the revised series to GitHub, but I'll wait a couple of days to
see if there's more feedback before resubmitting.

Thanks,
Michael

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

[Junio C Hamano]

Michael Haggerty  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 
> ---
>  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(>key_oid, _oid);
> - oidcpy(>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(>key_oid), t->ref);
> + type = PTR_TYPE_NOTE;
> + l = (struct leaf_node *)
> + xcalloc(1, sizeof(struct leaf_node));
> + oidcpy(>key_oid, _oid);
> + oidcpy(>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(>key_oid), t->ref);
> +
>   continue;
>  
>  handle_non_note:

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

[Michael Haggerty]

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

Signed-off-by: Michael Haggerty 
---
 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(>key_oid, _oid);
-   oidcpy(>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(>key_oid), t->ref);
+   type = PTR_TYPE_NOTE;
+   l = (struct leaf_node *)
+   xcalloc(1, sizeof(struct leaf_node));
+   oidcpy(>key_oid, _oid);
+   oidcpy(>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(>key_oid), t->ref);
+
continue;
 
 handle_non_note:
-- 
2.11.0