From: Eric Sunshine <sunshine@sunshineco.com>
To: Karthik Nayak <karthik.188@gmail.com>
Cc: Git List <git@vger.kernel.org>, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2 11/11] ref-filter: introduce objectname_atom_parser()
Date: Fri, 18 Dec 2015 01:24:50 -0500 [thread overview]
Message-ID: <CAPig+cSFG86U=+onvJ1aaosmX3k+fzD-431q-hFTnWHpg=pZVw@mail.gmail.com> (raw)
In-Reply-To: <1450279802-29414-12-git-send-email-Karthik.188@gmail.com>
On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> Introduce objectname_atom_parser() which will parse the
> '%(objectname)' atom and store information into the 'used_atom'
> structure based on the modifiers used along with the atom.
>
> Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Signed-off-by: Karthik Nayak <Karthik.188@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -43,6 +43,7 @@ static struct used_atom {
> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
> unsigned int no_lines;
> } contents;
> + enum { O_FULL, O_SHORT } objectname;
> } u;
> } *used_atom;
> @@ -124,6 +125,21 @@ static void contents_atom_parser(struct used_atom *atom)
> +static void objectname_atom_parser(struct used_atom *atom)
> +{
> + const char * buf;
> +
> + if (match_atom_name(atom->str, "objectname", &buf))
> + atom->u.objectname = O_FULL;
> + if (!buf)
> + return;
Let me make sure that I understand this correctly.
make_atom_name("objectname") will return true only for "objectname" or
"objectname:", and will return false for anything else, such as
"objectnamely" or "schmorf". Furthermore, the only way
objectname_atom_parser() can be called is when %(objectname) or
%(objectname:...) is seen, thus match_atom_name() *must* return true
here, which means the above conditional is misleading, suggesting that
it could somehow return false.
And, if match_atom_name() did return false here, then that indicates a
programming error: objectname_atom_parser() somehow got called for
something other than %(objectname) or %(objectname:...). This implies
that the code should instead be structured like this:
if (!match_atom_name(..., "objectname", &buf)
die("BUG: parsing non-'objectname'")
if (!buf)
atom->u.objectname = O_FULL;
else if (!strcmp(buf, "short"))
atom->u.objectname = O_SHORT;
else
die(_("unrecognized %%(objectname) argument: %s"), buf);
However, this can be simplified further by recognizing that, following
this patch series, match_atom_name() is *only* called by these new
parse functions[1], which means that, as a convenience,
match_atom_name() itself could become a void rather than boolean
function and die() if the expected atom name is not found. Thus, the
code would become:
match_atom_name(...);
if (!buf)
...
else if (!strcmp(...))
...
...
By the way, the above commentary isn't specific to this patch and
%(objectname), but is in fact also relevant for all of the preceding
patches which introduce parse functions calling match_atom_name().
More below...
[1]: ...assuming you replace the unnecessary match_atom_name() in
populate_value() with starts_with() as suggested in my patch 7/11
review addendum[2].
[2]: http://article.gmane.org/gmane.comp.version-control.git/282700
> +
> + if (!strcmp(buf, "short"))
> + atom->u.objectname = O_SHORT;
> + else
> + die(_("unrecognized %%(objectname) argument: %s"), buf);
> +}
> +
> @@ -461,15 +477,16 @@ static void *get_obj(const unsigned char *sha1, struct object **obj, unsigned lo
> static int grab_objectname(const char *name, const unsigned char *sha1,
> - struct atom_value *v)
> + struct atom_value *v, struct used_atom *atom)
> {
> - if (!strcmp(name, "objectname")) {
> - v->s = xstrdup(sha1_to_hex(sha1));
> - return 1;
> - }
> - if (!strcmp(name, "objectname:short")) {
> - v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
> - return 1;
> + if (starts_with(name, "objectname")) {
> + if (atom->u.objectname == O_SHORT) {
> + v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
> + return 1;
> + } else if (atom->u.objectname == O_FULL) {
> + v->s = xstrdup(sha1_to_hex(sha1));
> + return 1;
> + }
Since 'objectname' can only ever be O_SHORT or O_FULL wouldn't it be a
programming error if it ever falls through to this point after the
closing brace? Perhaps a die("BUG:...") would be appropriate?
> }
> return 0;
> }
> @@ -493,7 +510,7 @@ static void grab_common_values(struct atom_value *val, int deref, struct object
> v->s = xstrfmt("%lu", sz);
> }
> else if (deref)
> - grab_objectname(name, obj->sha1, v);
> + grab_objectname(name, obj->sha1, v, &used_atom[i]);
This patch hunk is somehow corrupt and doesn't apply. Was there some
hand-editing involved, or were some earlier patches regenerated after
this patch was made or something?
> }
> }
>
> @@ -999,7 +1016,7 @@ static void populate_value(struct ref_array_item *ref)
> v->s = xstrdup(buf + 1);
> }
> continue;
> - } else if (!deref && grab_objectname(name, ref->objectname, v)) {
> + } else if (!deref && grab_objectname(name, ref->objectname, v, atom)) {
> continue;
> } else if (!strcmp(name, "HEAD")) {
> const char *head;
> --
> 2.6.4
next prev parent reply other threads:[~2015-12-18 6:24 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-16 15:29 [PATCH v2 00/11] ref-filter: use parsing functions Karthik Nayak
2015-12-16 15:29 ` [PATCH v2 01/11] strbuf: introduce strbuf_split_str_without_term() Karthik Nayak
2015-12-16 20:35 ` Eric Sunshine
2015-12-17 8:24 ` Karthik Nayak
2015-12-16 15:29 ` [PATCH v2 02/11] ref-filter: use strbuf_split_str_omit_term() Karthik Nayak
2015-12-16 15:29 ` [PATCH v2 03/11] ref-filter: introduce struct used_atom Karthik Nayak
2015-12-16 20:57 ` Eric Sunshine
2015-12-19 4:42 ` Karthik Nayak
2015-12-16 15:29 ` [PATCH v2 04/11] ref-fitler: bump match_atom() name to the top Karthik Nayak
2015-12-16 15:29 ` [PATCH v2 05/11] ref-filter: skip deref specifier in match_atom_name() Karthik Nayak
2015-12-16 21:11 ` Eric Sunshine
2015-12-19 5:07 ` Karthik Nayak
2015-12-16 15:29 ` [PATCH v2 06/11] ref-filter: introduce color_atom_parser() Karthik Nayak
2015-12-16 21:21 ` Eric Sunshine
2015-12-19 6:00 ` Karthik Nayak
2015-12-16 15:29 ` [PATCH v2 07/11] ref-filter: introduce align_atom_parser() Karthik Nayak
2015-12-16 21:54 ` Eric Sunshine
2015-12-19 11:12 ` Karthik Nayak
2015-12-19 11:35 ` Karthik Nayak
2015-12-18 5:50 ` Eric Sunshine
2015-12-16 15:29 ` [PATCH v2 08/11] ref-filter: introduce prefixes for the align atom Karthik Nayak
2015-12-17 8:59 ` Eric Sunshine
2015-12-31 13:19 ` Karthik Nayak
2015-12-16 15:30 ` [PATCH v2 09/11] ref-filter: introduce remote_ref_atom_parser() Karthik Nayak
2015-12-17 9:22 ` Eric Sunshine
2015-12-24 7:42 ` Karthik Nayak
2015-12-16 15:30 ` [PATCH v2 10/11] ref-filter: introduce contents_atom_parser() Karthik Nayak
2015-12-17 9:39 ` Eric Sunshine
2015-12-24 8:27 ` Karthik Nayak
2015-12-16 15:30 ` [PATCH v2 11/11] ref-filter: introduce objectname_atom_parser() Karthik Nayak
2015-12-18 6:24 ` Eric Sunshine [this message]
2015-12-25 13:44 ` Karthik Nayak
2015-12-25 18:09 ` Eric Sunshine
2015-12-25 18:24 ` Karthik Nayak
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAPig+cSFG86U=+onvJ1aaosmX3k+fzD-431q-hFTnWHpg=pZVw@mail.gmail.com' \
--to=sunshine@sunshineco.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=karthik.188@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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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).