Re: [PATCH 2/5] ref-filter: add `short` option for 'tree' and 'parent'

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 27, 2020, 23:21 UTC
Message-ID: <xmqq1rkwfss2.fsf@gitster.c.googlers.com>
In-Reply-To: <49344f1b5559e7b4c63bad323a4dab5956331dde.1595882588.git.gitgitgadget@gmail.com>

"Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 6 quoted lines

> -		if (!strcmp(name, "tree")) {
> +		if (!strcmp(name, "tree"))
>  			v->s = xstrdup(oid_to_hex(get_commit_tree_oid(commit)));
> -		}
> +		else if (!strcmp(name, "tree:short"))
> +			v->s = xstrdup(find_unique_abbrev(get_commit_tree_oid(commit), DEFAULT_ABBREV));

Again, isn't this going in totally unacceptable direction?

By the time grab_foo() helper functions are reached, the requested format should have been parsed to atom->u.foo.option and the only thing grab_foo() helper functions should look at are the option.

Perhaps studying how "objectname" and its ":"-modified forms are handled before writing this series would be beneficial.

 - objectname_atom_parser() is called when the parser for --format
   notices "objectname:modifier"; it is responsible for setting up
   atom->u.objectname.option.  Note that this is done only once at
   the very begining of processing

 - grab_objectname() is called for each and every object ref-filter
   iterates over and "objectname" and/or its modified form
   (e.g. "objectname:short") is requested.  Since the modifier is
   already parsed, it can do a simple switch on the value in
   .option.

I do not know if patches [3-5/5] follow the pattern used in [1-2/5], but if they do, then they all need to be fixed, I think.

Thanks.