git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "Martin Ågren" <martin.agren@gmail.com>
To: git@vger.kernel.org
Cc: Jeff King <peff@peff.net>, Junio C Hamano <gitster@pobox.com>
Subject: [PATCH v2] merge: use skip_prefix to parse config key
Date: Sat, 11 Apr 2020 09:11:45 +0200	[thread overview]
Message-ID: <20200411071145.6839-1-martin.agren@gmail.com> (raw)
In-Reply-To: <20200410165644.GA79836@coredump.intra.peff.net>

On Fri, 10 Apr 2020 at 18:56, Jeff King <peff@peff.net> wrote:
>
> On Fri, Apr 10, 2020 at 09:44:56AM -0700, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > 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.
>
> Yes, exactly.
>
> > > The more general form would be:
> > [...]
> > Yes, but even with such a helper, i.e.
> > [...]
> > what Martin wrote, especially if it is reflowed to match the above, i.e.
> > [...]
> > I find it just as, if not more, easy to read.
>
> Yeah, sorry if I was unclear; I think the left-to-right is perfectly
> fine for this case.

Thanks both for an interesting discussion. Junio left a remark about
"[easy to read] especially if it is reflowed" which I recognized from my
own thinking yesterday. Then I went for fewer lines in what arguably
looks a bit crammed. Since I was not the only one to find readability in
the larger number of lines, here it is as v2.

Junio, if you go "meh, what I've already picked up will do just as
fine", feel free to act on that, i.e., to not act on this v2.

> Another option for known-value cases like this is to do it outside of
> the callback handler, like:
>
>   char *key = xstrfmt("branch.%s.mergeoptions");
>   const char *value;
>   if (!git_config_get_string_const(key, &value))
>      ...
>   free(key);
>
> The allocation is a bit awkward [...]

I very briefly thought about such an approach here, but still in the
callback handler, where the allocation would be quite a bit more
awkward, since we'd do it over and over.

Something I considered a bit more seriously was a helper that takes
three strings ("branch", branch and "mergeoptions") and does the check.
But as you mention in this thread, it's a bit special that we're looking
for a specific subsection (that is not a compile-time constant). So it
felt like the kind of "nice-to-have" helper that you'd end up using just
once or twice. (That said, I didn't look to see if I could find other
instances.)

Martin

-- >8 --
Instead of using `starts_with()`, the magic number 7, `strlen()` and a
fair number of additions to verify the three parts of the config key
"branch.<branch>.mergeoptions", use `skip_prefix()` to jump through them
more explicitly.

We need to introduce a new variable for this (we certainly can't modify
`k` just because we see "branch."!). With `skip_prefix()` we often use
quite bland names like `p` or `str`. Let's do the same. If and when this
function needs to do more prefix-skipping, we'll have a generic variable
ready for this.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin/merge.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index d127d2225f..df83ba2a80 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -597,10 +597,12 @@ static void parse_branch_merge_options(char *bmo)
 static int git_merge_config(const char *k, const char *v, void *cb)
 {
 	int status;
+	const char *str;
 
-	if (branch && starts_with(k, "branch.") &&
-		starts_with(k + 7, branch) &&
-		!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
+	if (branch &&
+	    skip_prefix(k, "branch.", &str) &&
+	    skip_prefix(str, branch, &str) &&
+	    !strcmp(str, ".mergeoptions")) {
 		free(branch_mergeoptions);
 		branch_mergeoptions = xstrdup(v);
 		return 0;
-- 
2.26.0


      parent reply	other threads:[~2020-04-11  7:13 UTC|newest]

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

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=20200411071145.6839-1-martin.agren@gmail.com \
    --to=martin.agren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --subject='Re: [PATCH v2] 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).