Re: [PATCH v2 2/3] for-each-ref: add 'is-base' token

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 13, 2024, 13:44 UTC
Message-ID: <8b1b05fa-829b-4f3d-9c25-cbb478176531@gmail.com>
In-Reply-To: <xmqqsev9zc2j.fsf@gitster.g>

On 8/12/24 5:05 PM, Junio C Hamano wrote:

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 10 quoted lines

>> +		} else if (atom_type == ATOM_ISBASE) {
>> +			if (ref->is_base && ref->is_base[is_base_atoms]) {
>> +				v->s = xstrfmt("(%s)", ref->is_base[is_base_atoms]);
>> +				free(ref->is_base[is_base_atoms]);
>> +			} else {
>> +				/* Not a commit. */
> 
> This is unexpected.  I thought that most of the branches except at
> most one that gets annotated with "Yeah, this is forked from branch
> B" would take the "else" side.  They are still commits, no?

You are correct. This is leftover from copy-pasting the ahead-behind section. Will remove.

Show 11 quoted lines

>> +	for (size_t i = 0; i < array->nr; i++) {
>> +		const char *name = array->items[i]->refname;
>> +		struct commit *c = lookup_commit_reference_by_name(name);
>> +
>> +		CALLOC_ARRAY(array->items[i]->is_base, format->is_base_tips.nr);
>> +
>> +		if (!c)
>> +			continue;
> 
> Hmph, wouldn't we want to leave array->items[i]->is_base NULL if
> "name" looked up to "c" happens to be non-commit (i.e. NULL)?

Your comment initially made me second-guess the logic here, but...

>> +		back_index[bases_nr] = array->items[i];
>> +		bases[bases_nr] = c;
>> +		bases_nr++;

This array of "back_index" is intended to allow the array being passed to get_branch_base_for_tip() to have no gaps with NULL commits. The indices are then translated back to the original array items when scanning the results.

This matches the behavior of the ahead-behind code, giving an existing behavior. The alternative would be to allow get_branch_base_for_tip() to be sensitive to NULL commits in the 'bases' array. But since we need to create an array of commit pointers (different from the array of ref items that we start with) this is likely the simplest approach.

You did inspire me to double-check that this code works in the presence of non-commit refs, so I'll update some things and send a v3 with a new test. It will also include some things to make error messages quieter for that case.

Thanks, -Stolee