All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Thomas Gummerer <t.gummerer@gmail.com>
Cc: git@vger.kernel.org, "Jeff King" <peff@peff.net>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Subject: Re: [PATCH 3/3] builtin/grep: allow implicit --no-index
Date: Mon, 11 Jan 2016 11:35:38 -0800	[thread overview]
Message-ID: <xmqqmvsbx62d.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20160111192844.GD10612@hank> (Thomas Gummerer's message of "Mon, 11 Jan 2016 20:28:44 +0100")

Thomas Gummerer <t.gummerer@gmail.com> writes:

>> > +#define GREP_NO_INDEX_EXPLICIT 1
>> > +#define GREP_NO_INDEX_IMPLICIT 2
>>
>> I am not sure this is the best way to do this.  For things like
>> this, the usual pattern is to initialize "no_index" to an "unknown"
>> value, allow "--no-index" to toggle it to true (by the way, I think
>> we should reject "--no-no-index", but that is a separate topic), and
>> then after command line parsing finishes, tweak the no_index if it
>> is still "unknown".
>
> The reason for this (and the change in 02/03) is so we can distinguish
> whether there is an explicit no-index or not for the error messages.
> I think it would be okay to have more generic error messages
> ("--cached or --untracked cannot be used without index" and
> "--untracked or no index mode cannot be used with revs").  What do you
> think?

I can understand that you need three states (--no-index given
explicitly from the command line, we fall back to --no-index when we
found we are in a diretory not under control by Git, and we do want
to use the index) to be able to give different messages between the
first two cases.

The usual way we do that is by making the variable tristate (which I
outlined in the previous message, initialize use_index to -1
"unspecified" and then fix it up when it is left unspecified after
you check the config and the command line option).

I however fail to see why that necessitates to change use_index to
no_index, making the code harder to follow by introducing double
negation.

  reply	other threads:[~2016-01-11 19:35 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-01-10 14:19 [PATCH 0/3] Implicitly use --no-index if git grep is used outside of repo Thomas Gummerer
2016-01-10 14:19 ` [PATCH 1/3] t7810: correct --no-index test Thomas Gummerer
2016-01-10 14:19 ` [PATCH 2/3] builtin/grep: rename use_index to no_index Thomas Gummerer
2016-01-11 17:33   ` Junio C Hamano
2016-01-10 14:19 ` [PATCH 3/3] builtin/grep: allow implicit --no-index Thomas Gummerer
2016-01-11  0:50   ` Duy Nguyen
2016-01-11 11:10     ` Thomas Gummerer
2016-01-11 17:26       ` Junio C Hamano
2016-01-11 17:48         ` Thomas Gummerer
2016-01-11 17:59           ` Jeff King
2016-01-11 18:37             ` Junio C Hamano
2016-01-11  1:54   ` Eric Sunshine
2016-01-11 11:29     ` Thomas Gummerer
2016-01-11 17:30   ` Junio C Hamano
2016-01-11 19:28     ` Thomas Gummerer
2016-01-11 19:35       ` Junio C Hamano [this message]
2016-01-11 19:44         ` Junio C Hamano
2016-01-11 21:01           ` Thomas Gummerer
2016-01-11 17:00 ` [PATCH v2 0/3] Introduce a --use-index command line argument in git grep Thomas Gummerer
2016-01-11 17:00   ` [PATCH v2 1/3] t7810: correct --no-index test Thomas Gummerer
2016-01-11 17:00   ` [PATCH v2 2/3] builtin/grep: rename use_index to no_index Thomas Gummerer
2016-01-11 17:00   ` [PATCH v2 3/3] builtin/grep: introduce --use-index argument Thomas Gummerer
2016-01-11 21:26 ` [PATCH v3 0/2] grep: add fallbackToNoIndex config option Thomas Gummerer
2016-01-11 21:26   ` [PATCH v3 1/2] t7810: correct --no-index test Thomas Gummerer
2016-01-11 21:26   ` [PATCH v3 2/2] builtin/grep: add grep.fallbackToNoIndex config Thomas Gummerer
2016-01-11 21:48     ` Jeff King
2016-01-11 22:35       ` Thomas Gummerer
2016-01-12 10:40   ` [PATCH v4 0/2] grep: add fallbackToNoIndex config option Thomas Gummerer
2016-01-12 10:40     ` [PATCH v4 1/2] t7810: correct --no-index test Thomas Gummerer
2016-01-12 10:40     ` [PATCH v4 2/2] builtin/grep: add grep.fallbackToNoIndex config Thomas Gummerer
2016-01-12 12:11       ` Jeff King
2016-01-12 15:50         ` Thomas Gummerer

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=xmqqmvsbx62d.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    --cc=t.gummerer@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.