On Tue, Jan 14, 2025 at 07:55:03PM +0100, René Scharfe wrote:
> A totally different thing that bugs me: Calling ahead-behind an atom is
> weird; it's more of a molecule. It should be possible to add separate
> ahead and behind atoms, with scalar values, that we then could sort
> separately, preferably numerically instead of lexically.
Yeah, I agree that would be the most flexible way to do it. I'm not sure
if people would find it useful or not (I've never even used the
ahead-behind atom myself).
> > So one solution would be to make the use of that context struct more
> > explicit, and require ref_sorting callers to provide a format struct.
> > Like the patch below, which also passes your tests.
>
> Did that in the first version of the patch. It works, but keeps the
> cause of the issue unaddressed: The separation of used_atom and the
> string_lists, which together represent the parsed items.
Right. I think the end goal is: all pieces of data together in a
non-global struct. Anything short of that has to choose the least-bad
option. ;)
In a sense, having _anything_ global means that there is little point in
having a struct at all (since it is still not safe to be called twice).
But it feels like that's a necessary step on the way, and getting rid of
the struct is a step backwards.
I dunno. I still have dreams about rewriting ref-filter completely for
better clarity and efficiency.
> I'm not convinced that ref_format is the right place for them, but
> haven't thought this through, admittedly. struct ref_filter and a
> new dedicated struct would be alternatives. Moving used_atom will be
> painful in any case.
Yet another option in the near term might be storing these ahead-behind
bits in the individual atoms. Since the point is to do a single
traversal, we'd have to marshal them into a unified data structure at
some point. But we already do that! In filter_ahead_behind() we convert
the string list into an array (and ironically do not even look at the
strings, only their "util" fields).
So something like this (only lightly tested) seems to work:
diff --git a/ref-filter.c b/ref-filter.c
index 23054694c2..4c10b6fe75 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -235,6 +235,9 @@ static struct used_atom {
enum { S_BARE, S_GRADE, S_SIGNER, S_KEY,
S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;
} signature;
+ struct {
+ struct commit *commit;
+ } base;
struct strvec describe_args;
struct refname_atom refname;
char *head;
@@ -892,17 +895,14 @@ static int rest_atom_parser(struct ref_format *format UNUSED,
}
static int ahead_behind_atom_parser(struct ref_format *format,
- struct used_atom *atom UNUSED,
+ struct used_atom *atom,
const char *arg, struct strbuf *err)
{
- struct string_list_item *item;
-
if (!arg)
return strbuf_addf_ret(err, -1, _("expected format: %%(ahead-behind:<committish>)"));
- item = string_list_append(&format->bases, arg);
- item->util = lookup_commit_reference_by_name(arg);
- if (!item->util)
+ atom->u.base.commit = lookup_commit_reference_by_name(arg);
+ if (!atom->u.base.commit)
die("failed to find '%s'", arg);
return 0;
@@ -3088,18 +3088,31 @@ void filter_ahead_behind(struct repository *r,
struct ref_array *array)
{
struct commit **commits;
- size_t commits_nr = format->bases.nr + array->nr;
+ size_t bases_nr, commits_nr;
- if (!format->bases.nr || !array->nr)
+ if (!array->nr)
return;
+ bases_nr = 0;
+ for (size_t i = 0; i < used_atom_cnt; i++) {
+ if (used_atom[i].atom_type == ATOM_AHEADBEHIND)
+ bases_nr++;
+ }
+
+ if (!bases_nr)
+ return;
+
+ commits_nr = bases_nr + array->nr;
+
ALLOC_ARRAY(commits, commits_nr);
- for (size_t i = 0; i < format->bases.nr; i++)
- commits[i] = format->bases.items[i].util;
+ for (size_t i = 0, j = 0; i < used_atom_cnt; i++) {
+ if (used_atom[i].atom_type == ATOM_AHEADBEHIND)
+ commits[j++] = used_atom[i].u.base.commit;
+ }
- ALLOC_ARRAY(array->counts, st_mult(format->bases.nr, array->nr));
+ ALLOC_ARRAY(array->counts, st_mult(bases_nr, array->nr));
- commits_nr = format->bases.nr;
+ commits_nr = bases_nr;
array->counts_nr = 0;
for (size_t i = 0; i < array->nr; i++) {
const char *name = array->items[i]->refname;
@@ -3108,8 +3121,8 @@ void filter_ahead_behind(struct repository *r,
if (!commits[commits_nr])
continue;
- CALLOC_ARRAY(array->items[i]->counts, format->bases.nr);
- for (size_t j = 0; j < format->bases.nr; j++) {
+ CALLOC_ARRAY(array->items[i]->counts, bases_nr);
+ for (size_t j = 0; j < bases_nr; j++) {
struct ahead_behind_count *count;
count = &array->counts[array->counts_nr++];
count->tip_index = commits_nr;
@@ -3277,9 +3290,12 @@ static inline int can_do_iterative_format(struct ref_filter *filter,
* - filtering on reachability
* - including ahead-behind information in the formatted output
*/
+ for (size_t i = 0; i < used_atom_cnt; i++) {
+ if (used_atom[i].atom_type == ATOM_AHEADBEHIND)
+ return 0;
+ }
return !(filter->reachable_from ||
filter->unreachable_from ||
- format->bases.nr ||
format->is_base_tips.nr);
}
@@ -3647,7 +3663,6 @@ void ref_format_init(struct ref_format *format)
void ref_format_clear(struct ref_format *format)
{
- string_list_clear(&format->bases, 0);
string_list_clear(&format->is_base_tips, 0);
ref_format_init(format);
}
diff --git a/ref-filter.h b/ref-filter.h
index 754038ab07..d048317802 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -99,9 +99,6 @@ struct ref_format {
/* Internal state to ref-filter */
int need_color_reset_at_eol;
- /* List of bases for ahead-behind counts. */
- struct string_list bases;
-
/* List of bases for is-base indicators. */
struct string_list is_base_tips;
@@ -117,7 +114,6 @@ struct ref_format {
}
#define REF_FORMAT_INIT { \
.use_color = -1, \
- .bases = STRING_LIST_INIT_DUP, \
.is_base_tips = STRING_LIST_INIT_DUP, \
}
We should be able to do the same thing with is_base_tips, but I've left
it as an exercise for the reader. ;)
-Peff
