Re: [PATCH v2 2/3] ref-filter: add new "describe" atom

From

Kousik Sanagavarapu <five231003@gmail.com>

Date

Jul 15, 2023, 18:24 UTC

Message-ID

<ZLLkZ4Vx2quwWwRz@five231003>

In-Reply-To

<xmqqilamnrcr.fsf@gitster.g>

On Fri, Jul 14, 2023 at 01:57:40PM -0700, Junio C Hamano wrote:

> Kousik Sanagavarapu <five231003@gmail.com> writes:
> 
> > +		struct {
> > +			enum { D_BARE, D_TAGS, D_ABBREV,
> > +			       D_EXCLUDE, D_MATCH } option;
> > +			const char **args;
> > +		} describe;
> 
> As you parse this into a strvec that has command line options for
> the "git describe" invocation, I do not see the point of having the
> "enum option" in this struct.  The describe->option member seems to
> be unused throughout this patch.
> 
> In fact, a single "const char **describe_args" should be able to
> replace the structure, no?

diff --git a/ref-filter.c b/ref-filter.c
index fe4830dbea..df7cb39be2 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -219,9 +219,7 @@ static struct used_atom {
 			enum { EO_RAW, EO_TRIM, EO_LOCALPART } option;
 		} email_option;
 		struct {
-			enum { D_BARE, D_TAGS, D_ABBREV,
-			       D_EXCLUDE, D_MATCH } option;
-			const char **args;
+			const char **decsribe_args;
 		} describe;
 		struct refname_atom refname;
 		char *head;
@@ -533,13 +531,16 @@ static int describe_atom_parser(struct ref_format *format UNUSED,
 				struct used_atom *atom,
 				const char *arg, struct strbuf *err)
 {
-	const char *describe_opts[] = {
-		"",
-		"tags",
-		"abbrev",
-		"match",
-		"exclude",
-		NULL
+	struct {
+		char *optname;
+		enum { D_BARE, D_TAGS, D_ABBREV,
+		       D_MATCH, D_EXCLUDE } option;
+	} describe_opts[] = {
+		{ "", D_BARE },
+		{ "tags", D_TAGS },
+		{ "abbrev", D_ABBREV },
+		{ "match", D_MATCH },
+		{ "exclude", D_EXCLUDE }
 	};
 
 	struct strvec args = STRVEC_INIT;

conveys it better or is it too much unnecessary stuff to and should we
just do

	struct {
		const char **describe_args;
	} describe;

leaving the describe_opts array as is and changing the how the switch is
written.

> > +static int describe_atom_parser(struct ref_format *format UNUSED,
> > +				struct used_atom *atom,
> > +				const char *arg, struct strbuf *err)
> > +{
> > +	const char *describe_opts[] = {
> > +		"",
> > +		"tags",
> > +		"abbrev",
> > +		"match",
> > +		"exclude",
> > +		NULL
> > +	};
> > +
> > +	struct strvec args = STRVEC_INIT;
> > +	for (;;) {
> > +		int found = 0;
> > +		const char *argval;
> > +		size_t arglen = 0;
> > +		int optval = 0;
> > +		int opt;
> > +
> > +		if (!arg)
> > +			break;
> > +
> > +		for (opt = D_BARE; !found && describe_opts[opt]; opt++) {
> > +			switch(opt) {
> > +			case D_BARE:
> > +				/*
> > +				 * Do nothing. This is the bare describe
> > +				 * atom and we already handle this above.
> > +				 */
> > +				break;
> > +			case D_TAGS:
> > +				if (match_atom_bool_arg(arg, describe_opts[opt],
> > +							&arg, &optval)) {
> > +					if (!optval)
> > +						strvec_pushf(&args, "--no-%s",
> > +							     describe_opts[opt]);
> > +					else
> > +						strvec_pushf(&args, "--%s",
> > +							     describe_opts[opt]);
> > +					found = 1;
> > +				}
> 
> As match_atom_bool_arg() and ...
> 
> > +				break;
> > +			case D_ABBREV:
> > +				if (match_atom_arg_value(arg, describe_opts[opt],
> > +							 &arg, &argval, &arglen)) {
> > +					char *endptr;
> > +					int ret = 0;
> > +
> > +					if (!arglen)
> > +						ret = -1;
> > +					if (strtol(argval, &endptr, 10) < 0)
> > +						ret = -1;
> > +					if (endptr - argval != arglen)
> > +						ret = -1;
> > +
> > +					if (ret)
> > +						return strbuf_addf_ret(err, ret,
> > +								_("positive value expected describe:abbrev=%s"), argval);
> > +					strvec_pushf(&args, "--%s=%.*s",
> > +						     describe_opts[opt],
> > +						     (int)arglen, argval);
> > +					found = 1;
> > +				}
> 
> ... match_atom_arg_value() are both silent when they return false,
> we do not see any diagnosis when these two case arms set the "found"
> flag.  Shouldn't we have a corresponding "else" clause to these "if
> (match_atom_blah())" blocks to issue an error message or something?

Yeah, I'll add this.

> [...] 
> Now, is the code from here ...
> 
> > +		if (deref)
> > +			name++;
> > +
> > +		if (!skip_prefix(name, "describe", &name) ||
> > +		    (*name && *name != ':'))
> > +			    continue;
> > +		if (!*name)
> > +			name = NULL;
> > +		else
> > +			name++;
> 
> ... down to here doing anything useful?  After all, you already have
> all you need to describe the commit in atom->u.describe_args to run
> "git describe" with, no?  In fact, after computing "name" with the
> above code with some complexity, nobody even looks at it.
> 
> Perhaps the above was copied from some other grab_* functions; the
> reason why they were relevant there needs to be understood, and it
> also has to be considered if the same reason to have the code here
> applies to this codepath.

Sorry you had to read through this. I'll remove these if constructs,
because as you said, they do nothing since we already parse everything
we need and also check for the type and the deref.

There is not test for "%(*describe)", but I'll add one in v3 if you
think it is necessary (if we are doing this, should we also do one for
multiple options?).

Thanks