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

From: Junio C Hamano <gitster@pobox.com>
To: "Hariom Verma via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Hariom Verma <hariom18599@gmail.com>
Subject: Re: [PATCH 2/5] ref-filter: add `short` option for 'tree' and 'parent'
Date: Mon, 27 Jul 2020 16:21:49 -0700	[thread overview]
Message-ID: <xmqq1rkwfss2.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <49344f1b5559e7b4c63bad323a4dab5956331dde.1595882588.git.gitgitgadget@gmail.com> (Hariom Verma via GitGitGadget's message of "Mon, 27 Jul 2020 20:43:05 +0000")

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

> -		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.