Web lists-archives.com

Re: [PATCH v2 08/14] revision.c: use commit-slab for show_source




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. 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.