Re: [PATCH v6 2/2] builtin/config.c: support `--type=<type>` as preferred alias for `--type`

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 7, 2018, 00:39 UTC
Message-ID: <20180407003912.GA78042@syl.local>
In-Reply-To: <CAPig+cQPB+74T3YenoZ_oVFGh2c38AbcFdc+yjw6f32Ojestwg@mail.gmail.com>

On Fri, Apr 06, 2018 at 03:04:53AM -0400, Eric Sunshine wrote:

Show 22 quoted lines

> On Fri, Apr 6, 2018 at 2:39 AM, Taylor Blau <me@ttaylorr.com> wrote:
> > [...]
> > In this patch, we support `--type=<int|bool|bool-or-int|...>` in
> > addition to `--int`, `--bool`, and etc. This allows the aforementioned
> > upcoming patch to support querying a color value with a default via
> > `--type=color --default=...`, without squandering `--color`.
> >
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> > @@ -38,12 +38,12 @@ existing values that match the regexp are updated or unset.  If
> > +The `--type` option requests 'git config' to ensure that the configured values
> > +associated with the given variable(s) are of the given type. When given
> > +`--type`, 'git config' will ensure that the variable(s) are of the given type
> > +and convert the value to the canonical form. If no type specifier is passed, no
> > +checks or transformations are performed on the value. Callers may unset any
> > +pre-existing type specifiers with `--no-type`.
>
> Sorry for being such a stickler, but this is still too mushy. The
> first two sentences are saying effectively the same thing. One or the
> other should be dropped or they should somehow be combined in a way
> that says everything that needs to be said without repetition.

I agree, and please do not apologize for being a stickler. I think that it's important we reach consensus before landing this, since other humans will read this.

How do you feel about?

  The `--type=<type>` option instructs 'git config' to ensure that
  incoming and outgoing values are canonicalize-able under the given
  <type>.  If no `--type=<type>` is given, no canonicalization will be
  performed. Callers may unset the existing `--type` specifier with
  `--no-type`.

I think documenting that `--no-type` unsets any pre-existing `--type` specifier is worthwhile. That said, I also think that there's a way to combine the last two sentences, but it might be clearer as two smaller pieces rather than one big one.

If this is still off base, could you provide some pointers on how you would word it?

> If necessary, iterate over updates of this paragraph here in the email
> thread until a polished version is reached rather than re-rolling the
> entire series every few minutes.

Thanks, and will do :-). I am quite new to this and was unaware of the situations when re-rolling is and isn't desirable. I am going to wait to re-roll this series until it has gathered more feedback, or at least consensus, after which I think it will be ready for queueing as-is.

Thanks for your patience and guidance.

Thanks, Taylor