From: Kousik Sanagavarapu <five231003@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org,
Christian Couder <christian.couder@gmail.com>,
Hariom Verma <hariom18599@gmail.com>
Subject: Re: [PATCH v2 2/3] ref-filter: add new "describe" atom
Date: Sat, 15 Jul 2023 23:54:39 +0530 [thread overview]
Message-ID: <ZLLkZ4Vx2quwWwRz@five231003> (raw)
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?
I kept the enum because I thought it could act as an index for the
describe_opts array. Now that I think about it,
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
next prev parent reply other threads:[~2023-07-15 18:24 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-05 17:57 [PATCH 0/2] Add new "describe" atom Kousik Sanagavarapu
2023-07-05 17:57 ` [PATCH 1/2] ref-filter: add " Kousik Sanagavarapu
2023-07-06 16:58 ` Junio C Hamano
2023-07-09 6:16 ` Kousik Sanagavarapu
2023-07-05 17:57 ` [PATCH 2/2] t6300: run describe atom tests on a different repo Kousik Sanagavarapu
2023-07-14 19:20 ` [PATCH v2 0/3] Add new "describe" atom Kousik Sanagavarapu
2023-07-14 19:20 ` [RFC PATCH v2 1/3] ref filter: add multiple-option parsing functions Kousik Sanagavarapu
2023-07-14 19:20 ` [PATCH v2 2/3] ref-filter: add new "describe" atom Kousik Sanagavarapu
2023-07-14 20:57 ` Junio C Hamano
2023-07-15 18:24 ` Kousik Sanagavarapu [this message]
2023-07-15 18:56 ` Junio C Hamano
2023-07-14 19:20 ` [PATCH v2 3/3] t6300: run describe atom tests on a different repo Kousik Sanagavarapu
2023-07-19 16:15 ` [PATCH v3 0/2] Add new "describe" atom Kousik Sanagavarapu
2023-07-19 16:15 ` [PATCH v3 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
2023-07-19 23:23 ` Junio C Hamano
2023-07-20 5:21 ` Junio C Hamano
2023-07-20 16:52 ` Kousik Sanagavarapu
2023-07-20 17:59 ` Junio C Hamano
2023-07-20 17:42 ` Glen Choo
2023-07-20 20:30 ` Junio C Hamano
2023-07-21 18:26 ` Glen Choo
2023-07-19 16:15 ` [PATCH v3 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
2023-07-19 22:56 ` Junio C Hamano
2023-07-20 22:52 ` [PATCH v3 0/2] Add " Junio C Hamano
2023-07-20 23:10 ` Junio C Hamano
2023-07-21 4:17 ` Kousik Sanagavarapu
2023-07-23 16:19 ` [PATCH v4 " Kousik Sanagavarapu
2023-07-23 16:19 ` [PATCH v4 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
2023-07-24 17:29 ` Junio C Hamano
2023-07-24 18:12 ` Kousik Sanagavarapu
2023-07-24 20:39 ` Junio C Hamano
2023-07-25 19:27 ` Junio C Hamano
2023-07-23 16:19 ` [PATCH v4 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
2023-07-24 17:21 ` Junio C Hamano
2023-07-25 20:51 ` [PATCH v5 0/2] Add " Kousik Sanagavarapu
2023-07-25 20:51 ` [PATCH v5 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
2023-07-25 20:51 ` [PATCH v5 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
2023-07-25 21:46 ` [PATCH v5 0/2] Add " Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZLLkZ4Vx2quwWwRz@five231003 \
--to=five231003@gmail.com \
--cc=christian.couder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=hariom18599@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).