git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: "Martin Ågren" <martin.agren@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH] merge: use skip_prefix to parse config key
Date: Fri, 10 Apr 2020 09:44:56 -0700	[thread overview]
Message-ID: <xmqqo8rzjnnb.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200410155827.GA71011@coredump.intra.peff.net> (Jeff King's message of "Fri, 10 Apr 2020 11:58:27 -0400")

Jeff King <peff@peff.net> writes:

> In general, parsing subsections accurately involves looking from both
> the start and the end of the string, pulling out the section and key and
> leaving the rest in the middle. But I think we can get away with this
> left-to-right parsing because we're only interested in matching a
> _specific_ subsection name, and a specific key. So there are no cases it
> will handle incorrectly.

In other words, if k were "branch.A.B.mergeoptions", it can only be
the 'branch.*.mergeoptions' variable attached to branch "A.B", but
when checking for branch=="A", the first two skip_prefix() would
pass and the only thing that protects us from misparsing is that
"B.mergeoptions" is not what we are looking for.

> The more general form would be:
>
>   const char *subsection, *key;
>   int subsection_len;
>
>   if (!parse_config_key("branch", &subsection, &subsection_len, &key) &&
>       subsection_len == strlen(branch) && !strncmp(subsection, branch) &&
>       !strcmp(key, "mergeoptions"))
>          ...
>
> but that's a bit more awkward (it would be less so if we had a helper
> function for comparing a NUL-terminated string against a ptr/len pair).

Yes, but even with such a helper, i.e.

	if (branch &&
	    !parse_config_key("branch", &sub, &sublen, &key) &&
	    !spanstrcmp(sub, sublen, branch) &&
	    !strcmp(key, "mergeoptions"))

what Martin wrote, especially if it is reflowed to match the above, i.e.

	if (branch &&
	    skip_prefix(key, "branch.", &sub) &&
	    skip_prefix(sub, branch, &sub) &&
	    !strcmp(sub, ".mergeoptions")

I find it just as, if not more, easy to read.

Where the parse_config_key() helper shines, I think, is when we do
not have the middle level to compare against, and in that case, we
must work only from the given key, scanning from both ends for dot.

Thanks.



  reply	other threads:[~2020-04-10 16:45 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-10 15:10 Martin Ågren
2020-04-10 15:58 ` Jeff King
2020-04-10 16:44   ` Junio C Hamano [this message]
2020-04-10 16:56     ` Jeff King
2020-04-10 17:12       ` Junio C Hamano
2020-04-11  7:11       ` [PATCH v2] " Martin Ågren

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=xmqqo8rzjnnb.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=martin.agren@gmail.com \
    --cc=peff@peff.net \
    --subject='Re: [PATCH] merge: use skip_prefix to parse config key' \
    /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

Code repositories for project(s) associated with this 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).