From: Junio C Hamano <gitster@pobox.com>
To: Kousik Sanagavarapu <five231003@gmail.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: Fri, 14 Jul 2023 13:57:40 -0700 [thread overview]
Message-ID: <xmqqilamnrcr.fsf@gitster.g> (raw)
In-Reply-To: <20230714194249.66862-3-five231003@gmail.com> (Kousik Sanagavarapu's message of "Sat, 15 Jul 2023 00:50:27 +0530")
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?
> +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?
> + break;
> + case D_MATCH:
> + case D_EXCLUDE:
> + if (match_atom_arg_value(arg, describe_opts[opt],
> + &arg, &argval, &arglen)) {
> + if (!arglen)
> + return strbuf_addf_ret(err, -1,
> + _("value expected describe:%s="), describe_opts[opt]);
> + strvec_pushf(&args, "--%s=%.*s",
> + describe_opts[opt],
> + (int)arglen, argval);
> + found = 1;
> + }
> + break;
> + }
> + }
> + if (!found)
> + break;
> + }
> + atom->u.describe.args = strvec_detach(&args);
> + return 0;
> +}
> +
> static int raw_atom_parser(struct ref_format *format UNUSED,
> struct used_atom *atom,
> const char *arg, struct strbuf *err)
> @@ -723,6 +819,7 @@ static struct {
> [ATOM_TAGGERDATE] = { "taggerdate", SOURCE_OBJ, FIELD_TIME },
> [ATOM_CREATOR] = { "creator", SOURCE_OBJ },
> [ATOM_CREATORDATE] = { "creatordate", SOURCE_OBJ, FIELD_TIME },
> + [ATOM_DESCRIBE] = { "describe", SOURCE_OBJ, FIELD_STR, describe_atom_parser },
> [ATOM_SUBJECT] = { "subject", SOURCE_OBJ, FIELD_STR, subject_atom_parser },
> [ATOM_BODY] = { "body", SOURCE_OBJ, FIELD_STR, body_atom_parser },
> [ATOM_TRAILERS] = { "trailers", SOURCE_OBJ, FIELD_STR, trailers_atom_parser },
> @@ -1542,6 +1639,54 @@ static void append_lines(struct strbuf *out, const char *buf, unsigned long size
> }
> }
>
> +static void grab_describe_values(struct atom_value *val, int deref,
> + struct object *obj)
> +{
> + struct commit *commit = (struct commit *)obj;
> + int i;
> +
> + for (i = 0; i < used_atom_cnt; i++) {
> + struct used_atom *atom = &used_atom[i];
> + enum atom_type type = atom->atom_type;
> + const char *name = atom->name;
> + struct atom_value *v = &val[i];
> +
> + struct child_process cmd = CHILD_PROCESS_INIT;
> + struct strbuf out = STRBUF_INIT;
> + struct strbuf err = STRBUF_INIT;
> +
> + if (type != ATOM_DESCRIBE)
> + continue;
We already have parsed the %(describe:...) and the result is stored
in the used_atom[] array. We iterate over the array, and we just
found that its atom_type member is ATOM_DESCRIBE here (otherwise we
would have moved on to the next array element).
> + if (!!deref != (*name == '*'))
> + continue;
This is trying to avoid %(*describe) answering when the given object
is the tag itself, or %(describe) answering when the given object is
what the tag dereferences to, so having it here makes sense (by the
way, do you add any test for "%(*describe)?").
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.
next prev parent reply other threads:[~2023-07-14 20:57 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 [this message]
2023-07-15 18:24 ` Kousik Sanagavarapu
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=xmqqilamnrcr.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=christian.couder@gmail.com \
--cc=five231003@gmail.com \
--cc=git@vger.kernel.org \
--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).