Re: [PATCH] ref-filter: share bases and is_base_tips between formatting and sorting

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> For now there is no program that uses more than one ref-filter format.
> But it seems like an obvious interface that would want to be lib-ified
> eventually. We are not there yet because of the static global used_atoms
> array. But the obvious path forward is to have a context struct
> representing one ref-filter iteration.
>
> I think the intent was that ref_format would be that context struct,
> though arguably it is a little funny since it forces the sorting and
> formatting to be joined (OTOH, that is very much how the code works,
> since it wants to share results between the two for efficiency).
>
> 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.
>
> I dunno. Your patch is deleting more code, which is nice. But I think in
> the long run we'd end up replacing it. But maybe making a clean slate
> now would make that easier? I could go either way.

I agree with you that libification effort would want to move more
static variables to members in a context structure.  I initially
suspected that would be more or less orthogonal, because it is not
like we are adding a static or two to a code path that already holds
everything else in a context structure, and would be a lot more
work, but now you have a patch that makes the first step in that
direction and it does not look too bad at all, so ...

Thanks.

> ---
>  builtin/branch.c       |  2 +-
>  builtin/for-each-ref.c |  2 +-
>  builtin/ls-remote.c    |  4 +++-
>  builtin/tag.c          |  2 +-
>  ref-filter.c           | 19 ++++++++-----------
>  ref-filter.h           |  2 +-
>  6 files changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 6e7b0cfddb..0c3f35cd0a 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -875,7 +875,7 @@ int cmd_branch(int argc,
>  		 * local branches 'refs/heads/...' and finally remote-tracking
>  		 * branches 'refs/remotes/...'.
>  		 */
> -		sorting = ref_sorting_options(&sorting_options);
> +		sorting = ref_sorting_options(&sorting_options, &format);
>  		ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);
>  		ref_sorting_set_sort_flags_all(
>  			sorting, REF_SORTING_DETACHED_HEAD_FIRST, 1);
> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
> index 715745a262..4f247efe57 100644
> --- a/builtin/for-each-ref.c
> +++ b/builtin/for-each-ref.c
> @@ -80,7 +80,7 @@ int cmd_for_each_ref(int argc,
>  	if (verify_ref_format(&format))
>  		usage_with_options(for_each_ref_usage, opts);
>  
> -	sorting = ref_sorting_options(&sorting_options);
> +	sorting = ref_sorting_options(&sorting_options, &format);
>  	ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);
>  	filter.ignore_case = icase;
>  
> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> index 42f34e1236..ed38b82346 100644
> --- a/builtin/ls-remote.c
> +++ b/builtin/ls-remote.c
> @@ -61,6 +61,7 @@ int cmd_ls_remote(int argc,
>  	const struct ref *ref;
>  	struct ref_array ref_array;
>  	struct ref_sorting *sorting;
> +	struct ref_format format = REF_FORMAT_INIT;
>  	struct string_list sorting_options = STRING_LIST_INIT_DUP;
>  
>  	struct option options[] = {
> @@ -155,7 +156,7 @@ int cmd_ls_remote(int argc,
>  		item->symref = xstrdup_or_null(ref->symref);
>  	}
>  
> -	sorting = ref_sorting_options(&sorting_options);
> +	sorting = ref_sorting_options(&sorting_options, &format);
>  	ref_array_sort(sorting, &ref_array);
>  
>  	for (i = 0; i < ref_array.nr; i++) {
> @@ -173,6 +174,7 @@ int cmd_ls_remote(int argc,
>  		status = 1;
>  	transport_ls_refs_options_release(&transport_options);
>  
> +	ref_format_clear(&format);
>  	strvec_clear(&pattern);
>  	string_list_clear(&server_options, 0);
>  	return status;
> diff --git a/builtin/tag.c b/builtin/tag.c
> index c4bd145831..a5240f66e2 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -574,7 +574,7 @@ int cmd_tag(int argc,
>  			die(_("options '%s' and '%s' cannot be used together"), "--column", "-n");
>  		colopts = 0;
>  	}
> -	sorting = ref_sorting_options(&sorting_options);
> +	sorting = ref_sorting_options(&sorting_options, &format);
>  	ref_sorting_set_sort_flags_all(sorting, REF_SORTING_ICASE, icase);
>  	filter.ignore_case = icase;
>  	if (cmdmode == 'l') {
> diff --git a/ref-filter.c b/ref-filter.c
> index 23054694c2..f5d0c448ed 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -3536,23 +3536,19 @@ void pretty_print_ref(const char *name, const struct object_id *oid,
>  	free_array_item(ref_item);
>  }
>  
> -static int parse_sorting_atom(const char *atom)
> +static int parse_sorting_atom(struct ref_format *format, const char *atom)
>  {
> -	/*
> -	 * This parses an atom using a dummy ref_format, since we don't
> -	 * actually care about the formatting details.
> -	 */
> -	struct ref_format dummy = REF_FORMAT_INIT;
>  	const char *end = atom + strlen(atom);
>  	struct strbuf err = STRBUF_INIT;
> -	int res = parse_ref_filter_atom(&dummy, atom, end, &err);
> +	int res = parse_ref_filter_atom(format, atom, end, &err);
>  	if (res < 0)
>  		die("%s", err.buf);
>  	strbuf_release(&err);
>  	return res;
>  }
>  
> -static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg)
> +static void parse_ref_sorting(struct ref_format *format,
> +			      struct ref_sorting **sorting_tail, const char *arg)
>  {
>  	struct ref_sorting *s;
>  
> @@ -3567,17 +3563,18 @@ static void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg
>  	if (skip_prefix(arg, "version:", &arg) ||
>  	    skip_prefix(arg, "v:", &arg))
>  		s->sort_flags |= REF_SORTING_VERSION;
> -	s->atom = parse_sorting_atom(arg);
> +	s->atom = parse_sorting_atom(format, arg);
>  }
>  
> -struct ref_sorting *ref_sorting_options(struct string_list *options)
> +struct ref_sorting *ref_sorting_options(struct string_list *options,
> +					struct ref_format *format)
>  {
>  	struct string_list_item *item;
>  	struct ref_sorting *sorting = NULL, **tail = &sorting;
>  
>  	if (options->nr) {
>  		for_each_string_list_item(item, options)
> -			parse_ref_sorting(tail, item->string);
> +			parse_ref_sorting(format, tail, item->string);
>  	}
>  
>  	/*
> diff --git a/ref-filter.h b/ref-filter.h
> index 754038ab07..1531bf1762 100644
> --- a/ref-filter.h
> +++ b/ref-filter.h
> @@ -168,7 +168,7 @@ int format_ref_array_item(struct ref_array_item *info,
>  /* Release a "struct ref_sorting" */
>  void ref_sorting_release(struct ref_sorting *);
>  /*  Convert list of sort options into ref_sorting */
> -struct ref_sorting *ref_sorting_options(struct string_list *);
> +struct ref_sorting *ref_sorting_options(struct string_list *, struct ref_format *);
>  /*  Function to parse --merged and --no-merged options */
>  int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
>  /*  Get the current HEAD's description */