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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 12, 2024, 21:05 UTC
Message-ID: <xmqqsev9zc2j.fsf@gitster.g>
In-Reply-To: <13341e7e51241e077a85ea83eb76d4e48d04be7b.1723397687.git.gitgitgadget@gmail.com>

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

Show 7 quoted lines

> +is-base:<committish>::
> +	In at most one row, `(<committish>)` will appear to indicate the ref
> +	that is most likely the ref used as a starting point for the branch
> +	that produced `<committish>`. This choice is made using a heuristic:
> +	choose the ref that minimizes the number of commits in the
> +	first-parent history of `<committish>` and not in the first-parent
> +	history of the ref.

Very nicely described.

Giving the end-user oriented "purpose/meaning" first makes it easier to understand for readers when they want to use it, and giving the heuristics to compute the result (and the example) next allows them to verify that the feature matches what they are looking for.

Show 10 quoted lines

> @@ -2475,6 +2495,16 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err)
>  				v->s = xstrdup("");
>  			}
>  			continue;
> +		} 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?

Show 41 quoted lines

> +				v->s = xstrdup("");
> +			}
> +			is_base_atoms++;
> +			continue;
>  		} else
>  			continue;
>  
> @@ -2876,6 +2906,7 @@ static void free_array_item(struct ref_array_item *item)
>  		free(item->value);
>  	}
>  	free(item->counts);
> +	free(item->is_base);
>  	free(item);
>  }
>  
> @@ -3040,6 +3071,49 @@ void filter_ahead_behind(struct repository *r,
>  	free(commits);
>  }
>  
> +void filter_is_base(struct repository *r,
> +		    struct ref_format *format,
> +		    struct ref_array *array)
> +{
> +	struct commit **bases;
> +	size_t bases_nr = 0;
> +	struct ref_array_item **back_index;
> +
> +	if (!format->is_base_tips.nr || !array->nr)
> +		return;
> +
> +	CALLOC_ARRAY(back_index, array->nr);
> +	CALLOC_ARRAY(bases, array->nr);
> +
> +	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)?

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

Thanks.