All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>,
	Tanay Abhra <tanayabh@gmail.com>,
	git@vger.kernel.org, Ramkumar Ramachandra <artagnon@gmail.com>
Subject: Re: [PATCH] config: teach "git -c" to recognize an empty string
Date: Mon, 04 Aug 2014 15:25:23 -0700	[thread overview]
Message-ID: <xmqq61i7riy4.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <20140804215644.GA21510@peff.net> (Jeff King's message of "Mon, 4 Aug 2014 17:56:44 -0400")

Jeff King <peff@peff.net> writes:

> This is technically a backwards incompatibility, but I'd consider it a
> simple bugfix. The existing behavior was unintentional, made no sense,
> and was never documented.

Yeah, I tend to agree.  I actually would not shed any tears if the
breakage were that it was impossible to pass "NULL is true" boolean
via "git -c" interface, but it is the other way around.  It is much
more grave a problem that we cannot pass an empty string as a value,
and we should fix it.

> Looking over strbuf_split's interface, I think it's rather
> counter-intuitive, and I was tempted to change it. But there are several
> other callers that rely on it, and the chance for introducing a subtle
> bug is high. This is the least invasive fix (and it really is not any
> less readable than what was already there :) ).

;-)

> +# We just need a type-specifier here that cares about the
> +# distinction internally between a NULL boolean and a real
> +# string (because most of git's internal parsers do care).
> +# Using "--path" works, but we do not otherwise care about
> +# its semantics.
> +test_expect_success 'git -c can represent empty string' '
> +	echo >expect &&
> +	git -c foo.empty= config --path foo.empty >actual &&
> +	test_cmp expect actual
> +'

Another way may be "git config -l" and see if we see a = on the
entry for foo.empty, but I think the way you did this is nicer.

>  test_expect_success 'key sanity-checking' '
>  	test_must_fail git config foo=bar &&
>  	test_must_fail git config foo=.bar &&

      reply	other threads:[~2014-08-04 22:25 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-08-04 14:41 [PATCH] pretty.c: make git_pretty_formats_config return -1 on git_config_string failure Tanay Abhra
2014-08-04 15:45 ` Matthieu Moy
2014-08-04 18:56   ` Eric Sunshine
2014-08-04 19:49     ` Matthieu Moy
2014-08-04 20:33   ` Jeff King
2014-08-04 21:06     ` Matthieu Moy
2014-08-04 21:56       ` [PATCH] config: teach "git -c" to recognize an empty string Jeff King
2014-08-04 22:25         ` Junio C Hamano [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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq61i7riy4.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox.com \
    --cc=Matthieu.Moy@grenoble-inp.fr \
    --cc=artagnon@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --cc=tanayabh@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.