From: Taylor Blau <me@ttaylorr.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Taylor Blau <me@ttaylorr.com>,
git@vger.kernel.org, ericsunshine@sunshineco.com
Subject: Re: [PATCH v7 1/2] builtin/config.c: treat type specifiers singularly
Date: Mon, 9 Apr 2018 19:12:53 -0700 [thread overview]
Message-ID: <20180410021253.GA937@syl.local> (raw)
In-Reply-To: <xmqqy3hv4zzi.fsf@gitster-ct.c.googlers.com>
On Tue, Apr 10, 2018 at 10:22:25AM +0900, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > Internally, we represent `git config`'s type specifiers as a bitset
> > using OPT_BIT. 'bool' is 1<<0, 'int' is 1<<1, and so on. This technique
> > allows for the representation of multiple type specifiers in the `int
> > types` field, but this multi-representation is left unused.
> >
> > In fact, `git config` will not accept multiple type specifiers at a
> > time, as indicated by:
> >
> > $ git config --int --bool some.section
> > error: only one type at a time.
> >
> > This patch uses `OPT_SET_INT` to prefer the _last_ mentioned type
> > specifier, so that the above command would instead be valid, and a
> > synonym of:
> >
> > $ git config --bool some.section
> >
> > This change is motivated by two urges: (1) it does not make sense to
> > represent a singular type specifier internally as a bitset, only to
> > complain when there are multiple bits in the set. `OPT_SET_INT` is more
> > well-suited to this task than `OPT_BIT` is. (2) a future patch will
> > introduce `--type=<type>`, and we would like not to complain in the
> > following situation:
> >
> > $ git config --int --type=int
>
> The above does not exactly argue for adopting the last-one-wins
> semantics, and still leaves it unclear if we want to complain
> against
>
> $ git config --bool --type=int
>
> Is it intentionally left vague if we want to (or not want to)
> complain when such a conflicting specification is given?
>
> We could keep the traditional behaviour of "only one type at a time"
> error and still move away from the bitset representation that does
> not make sense, if we wanted to. Initialize the "type" variable to
> an unset value, and use a callback to ensure either the variable is
> set to the unset value, or the value being set is already in the
> variable. I think if you use OPT_CMDMODE(), it would do all of that
> for you automatically.
>
> I suspect that it may be OK to switch to last-one-wins, but then we
> should give a justification that is a bit stronger than "we want to
> avoid complaining against --int --type=int" (i.e. "we want to switch
> to last-one-wins for such and such reasons").
I think that the major justification is to treat --type=int as a _true_
synonym of --int, such that neither `--type=<t1> --type=<t2>` nor
`--<t1> --<t2>` will complain. This, as well as the fact that
OPT_SET_BIT brings us closer to the semantics of `--verbose=1
--verbose=2`, which is something that Eric had mentioned above.
I think that OPT_CMDMODE would not work quite in the way we desire,
since the error messages would not quite line up with the command typed.
For instance, after applying the following diff:
diff --git a/builtin/config.c b/builtin/config.c
index 5c8952a17c..d9b73b949a 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -111,11 +111,11 @@ static struct option builtin_config_options[] = {
OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
OPT_GROUP(N_("Type")),
OPT_CALLBACK('t', "type", &type, N_("type"), N_("value is given this type"), option_parse_type),
- OPT_SET_INT(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),
- OPT_SET_INT(0, "int", &type, N_("value is decimal number"), TYPE_INT),
- OPT_SET_INT(0, "bool-or-int", &type, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
- OPT_SET_INT(0, "path", &type, N_("value is a path (file or directory name)"), TYPE_PATH),
- OPT_SET_INT(0, "expiry-date", &type, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
+ OPT_CMDMODE(0, "bool", &type, N_("value is \"true\" or \"false\""), TYPE_BOOL),
+ OPT_CMDMODE(0, "int", &type, N_("value is decimal number"), TYPE_INT),
+ OPT_CMDMODE(0, "bool-or-int", &type, N_("value is --bool or --int"), TYPE_BOOL_OR_INT),
+ OPT_CMDMODE(0, "path", &type, N_("value is a path (file or directory name)"), TYPE_PATH),
+ OPT_CMDMODE(0, "expiry-date", &type, N_("value is an expiry date"), TYPE_EXPIRY_DATE),
OPT_GROUP(N_("Other")),
OPT_BOOL('z', "null", &end_null, N_("terminate values with NUL byte")),
OPT_BOOL(0, "name-only", &omit_values, N_("show variable names only")),
The following occurs:
~/g/git (tb/config-type-specifier-option!) $ ./git-config --type=int --bool foo.bar
error: option `bool' : incompatible with --int
Whereas I would expect that to say:
error: option `bool' is incompatible with `--type=int'.
I am not sure whether amending the implementation of OPT_CMDMODE is something
that you're interested in here.
I can amend my patch to include this extra reasoning, if you think that
would be helpful.
> > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> > index 4f8e6f5fde..24de37d544 100755
> > --- a/t/t1300-repo-config.sh
> > +++ b/t/t1300-repo-config.sh
> > @@ -1611,4 +1611,15 @@ test_expect_success '--local requires a repo' '
> > test_expect_code 128 nongit git config --local foo.bar
> > '
> >
> > +cat >.git/config <<-\EOF &&
> > +[core]
> > +number = 10
> > +EOF
> > +
> > +test_expect_success 'later legacy specifiers are given precedence' '
> > + git config --bool --int core.number >actual &&
> > + echo 10 >expect &&
> > + test_cmp expect actual
> > +'
>
> And this expects more than we gave justifications for in the
> proposed log message. I do not think it is necessarily a bad idea
> to switch to last-one-wins, but if that is where we really want to
> go, the proposed log message is being misleading. It is true that
> OPT_SET_INT is more suited to complain when two conflicting things
> are given than OPT_BIT, but this example actually tells us that you
> no longer want to catch an error to give conflicting requests.
Yes, since I'd like to be able to insert `--type=` and have it behave
the same way. Since `--type=bool --type=int` will not complain (and
instead adopt `--type=int` given that it comes later in the command
string), neither should `--bool --int`.
Thanks,
Taylor
next prev parent reply other threads:[~2018-04-10 2:12 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-28 23:47 [PATCH] builtin/config.c: prefer `--type=bool` over `--bool`, etc Taylor Blau
2018-03-29 20:18 ` Junio C Hamano
2018-03-29 22:11 ` Jeff King
2018-03-30 5:27 ` Taylor Blau
2018-03-30 13:53 ` Jeff King
2018-03-30 16:00 ` Junio C Hamano
2018-03-30 18:27 ` Jeff King
2018-03-30 5:28 ` [PATCH v2 1/2] builtin/config.c: treat type specifiers singularly Taylor Blau
2018-03-30 5:28 ` [PATCH v2 2/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc Taylor Blau
2018-03-30 6:17 ` René Scharfe
2018-03-30 13:48 ` Jeff King
2018-03-30 13:41 ` [PATCH v2 1/2] builtin/config.c: treat type specifiers singularly Jeff King
2018-04-04 6:07 ` [PATCH v3 0/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc Taylor Blau
2018-04-04 6:07 ` [PATCH v3 1/2] builtin/config.c: treat type specifiers singularly Taylor Blau
2018-04-04 7:57 ` Eric Sunshine
2018-04-05 1:53 ` Taylor Blau
2018-04-05 21:51 ` Jeff King
2018-04-04 6:07 ` [PATCH v3 2/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc Taylor Blau
2018-04-04 7:27 ` Eric Sunshine
2018-04-05 1:47 ` Taylor Blau
2018-04-05 2:00 ` [PATCH v4 0/2] " Taylor Blau
2018-04-05 21:58 ` Jeff King
2018-04-05 22:15 ` Taylor Blau
[not found] ` <cover.1522893363.git.me@ttaylorr.com>
2018-04-05 2:00 ` [PATCH v4 1/2] builtin/config.c: treat type specifiers singularly Taylor Blau
2018-04-05 2:00 ` [PATCH v4 2/2] builtin/config.c: prefer `--type=bool` over `--bool`, etc Taylor Blau
2018-04-05 22:29 ` Eric Sunshine
2018-04-05 22:40 ` Jeff King
2018-04-06 5:19 ` Taylor Blau
2018-04-06 5:17 ` Taylor Blau
2018-04-05 2:02 ` Taylor Blau
2018-04-05 22:12 ` Jeff King
2018-04-05 22:15 ` Taylor Blau
2018-04-06 5:08 ` Taylor Blau
2018-04-06 6:38 ` [PATCH v6 0/2] builtin/config.c: support `--type=<type>` as preferred alias for `--type` Taylor Blau
[not found] ` <cover.1522996619.git.me@ttaylorr.com>
2018-04-06 6:39 ` [PATCH v6 1/2] builtin/config.c: treat type specifiers singularly Taylor Blau
2018-04-06 6:39 ` [PATCH v6 2/2] builtin/config.c: support `--type=<type>` as preferred alias for `--type` Taylor Blau
2018-04-06 7:04 ` Eric Sunshine
2018-04-07 0:39 ` Taylor Blau
2018-04-07 8:25 ` Eric Sunshine
2018-04-09 22:46 ` [PATCH v7 0/2] " Taylor Blau
2018-04-09 23:11 ` Eric Sunshine
[not found] ` <cover.1523313730.git.me@ttaylorr.com>
2018-04-09 22:46 ` [PATCH v7 1/2] builtin/config.c: treat type specifiers singularly Taylor Blau
2018-04-10 1:22 ` Junio C Hamano
2018-04-10 2:12 ` Taylor Blau [this message]
2018-04-10 4:13 ` Eric Sunshine
2018-04-09 22:46 ` [PATCH v7 2/2] builtin/config.c: support `--type=<type>` as preferred alias for `--type` Taylor Blau
2018-04-10 1:44 ` Junio C Hamano
2018-04-10 2:17 ` Taylor Blau
2018-04-11 1:06 ` [PATCH v8 0/2] " Taylor Blau
2018-04-11 1:24 ` Junio C Hamano
2018-04-11 1:33 ` Taylor Blau
2018-04-11 3:11 ` Junio C Hamano
2018-04-11 3:49 ` Taylor Blau
[not found] ` <cover.1523408336.git.me@ttaylorr.com>
2018-04-11 1:06 ` [PATCH v8 1/2] builtin/config.c: treat type specifiers singularly Taylor Blau
2018-04-11 1:07 ` [PATCH v8 2/2] builtin/config.c: support `--type=<type>` as preferred alias for `--type` Taylor Blau
2018-04-18 21:43 ` [PATCH v9 0/2] " Taylor Blau
[not found] ` <cover.1524087557.git.me@ttaylorr.com>
2018-04-18 21:43 ` [PATCH v9 1/2] builtin/config.c: treat type specifiers singularly Taylor Blau
2018-04-18 21:43 ` [PATCH v9 2/2] builtin/config.c: support `--type=<type>` as preferred alias for `--type` Taylor Blau
2018-04-19 2:47 ` Junio C Hamano
2018-04-19 3:01 ` Taylor Blau
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=20180410021253.GA937@syl.local \
--to=me@ttaylorr.com \
--cc=ericsunshine@sunshineco.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.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.
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).