Re: [PATCH 05/10] evolve: add the change-table structure
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Sep 27, 2022, 14:18 UTC
- Message-ID
- <7c310e1a-1011-c426-fe64-7be21b69052c@dunelm.org.uk>
- In-Reply-To
- <3c61e0b3-5526-f42e-48a7-c4465d06ccb3@dunelm.org.uk>
On 27/09/2022 14:27, Phillip Wood wrote:
Show 19 quoted lines
>> + /**
>> + * 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.
Show 10 quoted lines
> 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.