Junio C Hamano <gitster@xxxxxxxxx> writes:
> Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
>
>> - refname = commit->util;
>> + refname = *revision_sources_peek(&revision_sources, commit);
>
> At first glance, I thought that the reason why this uses _peek() is
> because the "sources" is expected to only sparsely be populated,
> perhaps because get_tags_and_duplicates() annotates only the tips
> mentioned on the command line via rev_cmdline mechanism and this
> code does not want to auto-vivify the slot, only to read NULL from
> it.
>
> But the code that follows this point liberally uses refname without
> checking if it is NULL, so I am not quite sure what is going on.
Ah, of course, this is about the code that propagates the "source"
(i.e. from which tip given on the command line did we start the
traversal to reach this commit?), so that is what ensures there is
something in commit->util and revision_sources not just has an entry
for the commit but the entry should have a non-NULL string.
So shouldn't *slab_peek() here be *slab_at() instead?
> In
> any case, wouldn't *slab_peek() an anti-pattern? You use _peek()
> because you expect that a slot is not yet allocated for a commit,
> you desire not to vivify all the slots for all the commits, and
> instead you are prepared to see a NULL returned from the call. But
> I do not think that is what is happening here, so shouldn't it be
> using _at() instead of _peek()?
>
>> if (anonymize) {
>> refname = anonymize_refname(refname);
>
>> anonymize_ident_line(&committer, &committer_end);
>> @@ -862,10 +864,11 @@ static void get_tags_and_duplicates(struct rev_cmdline_info *info)
>> * This ref will not be updated through a commit, lets make
>> * sure it gets properly updated eventually.
>> */
>> - if (commit->util || commit->object.flags & SHOWN)
>> + if (*revision_sources_at(&revision_sources, commit) ||
>> + commit->object.flags & SHOWN)
>
> Here it uses *slab_at() which makes more sense.
