git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "René Scharfe" <rene.scharfe@lsrfire.ath.cx>
To: Junio C Hamano <gitster@pobox.com>
Cc: Thomas Rast <trast@inf.ethz.ch>,
	git@vger.kernel.org, Bert Wesarg <bert.wesarg@googlemail.com>,
	Geoffrey Irving <irving@naml.us>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Pierre Habouzit <madcoder@debian.org>
Subject: Re: [PATCH 2/3] parse-options: allow positivation of options starting, with no-
Date: Mon, 27 Feb 2012 18:56:03 +0100	[thread overview]
Message-ID: <4F4BC3B3.7080000@lsrfire.ath.cx> (raw)
In-Reply-To: <7v8vjob3ff.fsf@alter.siamese.dyndns.org>

Am 27.02.2012 18:18, schrieb Junio C Hamano:
> Thomas Rast<trast@inf.ethz.ch>  writes:
>
>> Junio C Hamano<gitster@pobox.com>  writes:
>>
>>> I would naïvely expect that it would be sufficient to update an existing
>>> definition for "--no-frotz" that uses PARSE_OPT_NONEG to instead define
>>> "--frotz" that by itself is a no-op, and "--no-frotz" would cause whatever
>>> the option currently means, with an update to the help text that says
>>> something to the effect that "--frotz by itself is meaningless and is
>>> always used as --no-frotz".
>>
>> Doesn't that last quote already answer your question?
>
> Yes, but only partly.  I would agree with the awkwardness in
>
>> It would be rather awkward to see, in 'git apply -h',
>>
>>    --add                 Also apply additions in the patch.  This is the
>>                          default; use --no-add to disable it.
>
> but it feeels somewhat questionable that the solution to get this:
>
>>
>> Compare to the current concise wording
>>
>>    --no-add              ignore additions made by the patch
>
> is to define OPT_BOOL("no-add") that does not have any hint (other than
> the fact that the option name begins with 3 character "no-") that this is
> an already negated boolean and the "no-" negation can be removed.

The parser already knows that the prefix "no-" negates an option.  It 
currenmtly only applies this knowledge if that prefix is added, but not 
if it is removed, which is inconsistent.

> This means an option "no-$foo" can never mean anything but "not foo".  Not
> that we would have to or necessarily want to support an option to give the
> number of foo as --no-foo=47, as --num-foo=47 is a perfectly good spelling
> for such an option.

With the patch, you can define a --no-foo option that doesn't accept 
--foo as its negation by specifying PARSE_OPT_NONEG.  That would also 
forbid --no-no-foo, though, but that's probably a good thing.

> If it were OPT_BOOL("no-foo", OPT_ISNEG | ...) that signals the parser
> that:
>
>   - the option name is already negative;
>   - the leading "no-" is to be removed to negate it; and
>   - no extra leading "no-", i.e. "--no-no-foo", is accepted.
>
> I probably wouldn't have felt this uneasy iffiness.

Teaching the parser to understand that removal of the prefix "no-" 
negates an option on top of its existing knowledge that adding it does 
the same just adds the other side of the same coin, which was curiously 
missing.

The patch does not forbid adding "no-" to an option that already starts 
with "no-".  This stricter rule would be easy to add, but since that is 
currently the only way to negate such options, it would break backwards 
compatibility and thus should be added in a separate patch, if at all.

With the patch, the following guidelines are followed:

	- "no-" means no, for both developers and users.
	- The user doesn't have to to say "no-no-".

The results feels simpler to me.

René

  reply	other threads:[~2012-02-27 17:56 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-02-25 19:07 [PATCH 0/3] parse-options: no- symmetry René Scharfe
2012-02-25 19:11 ` [PATCH 1/3] test-parse-options: convert to OPT_BOOL() René Scharfe
2012-02-25 19:14 ` [PATCH 2/3] parse-options: allow positivation of options starting, with no- René Scharfe
2012-02-26 23:32   ` Junio C Hamano
2012-02-27  8:30     ` Thomas Rast
2012-02-27 17:18       ` Junio C Hamano
2012-02-27 17:56         ` René Scharfe [this message]
2012-02-27 20:48           ` Junio C Hamano
2012-02-28 20:12             ` [PATCH 4/3] parse-options: disallow --no-no-sth René Scharfe
2012-02-28 21:15               ` Junio C Hamano
2012-02-29 18:06                 ` René Scharfe
2012-02-29 19:02                   ` Junio C Hamano
2012-02-25 19:15 ` [PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP René Scharfe
2012-02-27 18:25   ` Jeff King
2012-02-27 18:58     ` Junio C Hamano
2012-02-27 22:26     ` René Scharfe
2012-02-28  0:34       ` Jeff King
2012-02-28 19:06   ` [PATCH 3/3 v2] " René Scharfe
2012-02-28 19:09     ` Jeff King

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=4F4BC3B3.7080000@lsrfire.ath.cx \
    --to=rene.scharfe@lsrfire.ath.cx \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=bert.wesarg@googlemail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=irving@naml.us \
    --cc=madcoder@debian.org \
    --cc=trast@inf.ethz.ch \
    /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).