Re: [PATCH 05/10] evolve: add the change-table structure

[Phillip Wood <phillip.wood123@gmail.com>]

On 27/09/2022 14:27, Phillip Wood wrote:
>> +    /**
>> +     * Determine the object id for the latest content commit for each 
>> change.
>> +     * Fetch the commit at the head of each change ref. If it's a 
>> normal commit,
>> +     * that's the commit we want. If it's a metacommit, locate its 
>> content parent
>> +     * and use that.
>> +     */
>> +
>> +    for (i = 0; i < matching_refs.nr; i++) {
>> +        struct ref_array_item *item = matching_refs.items[i];
>> +        struct commit *commit = item->commit;
>> +
>> +        commit = lookup_commit_reference_gently(repo, 
>> &item->objectname, 1);
> 
> We're assigning commit twice - why do we need to look it up if 
> filter_refs returns it?

I think we want to look it up to check that item->objectname is a 
commit. item->commit is not filled unless we specify the verbose flag 
and I'm not sure what the implications of setting that are. If we get an 
objectname that does not name a commit we should call BUG() as suggested 
below.

> There are a number of places where we call 
> lookup_commit_reference_gently(..., 1) to silence the warning if the 
> objectname does not dereference to a commit. It is not clear to me that 
> we want to hide those errors. Indeed I think we should be doing
> 
>          commit = lookup_commit_reference(repo, oid)
>          if (!commit)
>              BUG("commit missing ...")
> 
> unless there is a good reason that the lookup can fail.