Re: [PATCH] object_as_type: initialize commit-graph-related fields of 'struct commit'

[Jeff King <peff@peff.net>]

On Sun, Jan 27, 2019 at 02:08:32PM +0100, SZEDER Gábor wrote:

> When the commit graph and generation numbers were introduced in
> commits 177722b344 (commit: integrate commit graph with commit
> parsing, 2018-04-10) and 83073cc994 (commit: add generation number to
> struct commit, 2018-04-25), they tried to make sure that the
> corresponding 'graph_pos' and 'generation' fields of 'struct commit'
> are initialized conservatively, as if the commit were not included in
> the commit-graph file.
> 
> Alas, initializing those fields only in alloc_commit_node() missed the
> case when an object that happens to be a commit is first looked up via
> lookup_unknown_object(), and is then later converted to a 'struct
> commit' via the object_as_type() helper function (either calling it
> directly, or as part of a subsequent lookup_commit() call).
> Consequently, both of those fields incorrectly remain set to zero,
> which means e.g. that the commit is present in and is the first entry
> of the commit-graph file.  This will result in wrong timestamp, parent
> and root tree hashes, if such a 'struct commit' instance is later
> filled from the commit-graph.
> 
> Extract the initialization of 'struct commit's fields from
> alloc_commit_node() into a helper function, and call it from
> object_as_type() as well, to make sure that it properly initializes
> the two commit-graph-related fields, too.  With this helper function
> it is hopefully less likely that any new fields added to 'struct
> commit' in the future would remain uninitialized.
> 
> With this change alloc_commit_index() won't have any remaining callers
> outside of 'alloc.c', so mark it as static.

Good find, and nicely explained.

> ---
> 
> So, it turns out that ec0c5798ee (revision: use commit graph in
> get_reference(), 2018-12-04) is not the culprit after all, it merely
> highlighted a bug that is as old as the commit-graph feature itself.
> This patch fixes this and all other related issues I reported
> upthread.
> 
> I couldn't find any other place where an object of unknown type is
> turned into a 'struct commit', so this might have been the only place
> that needed fixing.

This should be the only place. We already ran into this with the
commit-index field, which was what caused us to create object_as_type()
in the first place, to give a central place for coercing OBJ_NONE into
other types.

> Other object types seem to be fine, because they contain only fields
> that should be zero initialized.  At least for now, because a similar
> issue might arise in the future, if one of them gains a new field that
> should not be initialized to zero...  but will they ever get such a
> field?  So I'm not too keen on introducing similar init_tree_node(),
> etc. helper funcions at the moment.

Agreed. We can deal with those if it ever becomes necessary. In theory
adding empty placeholder functions might help somebody realize they'd
need to handle this case, but I have the feeling that they'd be as
likely to miss init_tree_node() as they would object_as_type().

I dunno. I guess if init_tree_node() were actually called from
alloc_tree_node(), it might be harder to miss.

>  alloc.c  | 11 ++++++++---
>  alloc.h  |  2 +-
>  object.c |  5 +++--
>  3 files changed, 12 insertions(+), 6 deletions(-)

The patch itself looks good to me.

-Peff