Re: [PATCH v2 1/1] config: learn the --stdin option for instructions

From: Jeff King <peff@peff.net>
Date: Jan 28, 2020, 09:19 UTC
Message-ID: <20200128091914.GA574544@coredump.intra.peff.net>
In-Reply-To: <20200127100933.10765-2-git@zjvandeweg.nl>

On Mon, Jan 27, 2020 at 11:09:33AM +0100, Zeger-Jan van de Weg wrote:

Show 9 quoted lines

> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 899e92a1c9..9f7462284d 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -23,6 +23,7 @@ SYNOPSIS
>  'git config' [<file-option>] [--show-origin] [-z|--null] [--name-only] -l | --list
>  'git config' [<file-option>] --get-color name [default]
>  'git config' [<file-option>] --get-colorbool name [stdout-is-tty]
> +'git config' [<file-option>] [-z|--null] --stdin

Instead of a stdin mode just for setting, you have a general --stdin mode. That would later allow "get" commands, too. I'm not sure if that's a good idea, though. Here are two complications which came to mind immediately:

  - the permissions models for getting and setting might be different.
    Might we ever feed untrusted input to something like "git config
    --get --stdin", but not want it to be able to set any values?

  - what happens when get/set commands are intermixed? When are set
    commands flushed? E.g., if I send:

     set foo.bar baz
     get foo.bar

    then do I see "baz", or whatever was there before?

I think those are solvable, but unless we have a compelling use case for a process that will both get and set values, it seems like it might be simpler to keep the major modes separate.

Show 10 quoted lines

> +STDIN
> +-----
> +
> +With `--stdin`, config reads instructions from standard input and performs
> +all modifications in sequence.
> +
> +Specify commands of the form:
> +
> +	set SP <key> SP <newvalue>
> +	unset SP <key>

This protocol doesn't leave much room for giving more details about each set operation. There are several things you can do on the command line now that you might want to specify on a per-key basis:

  - adding keys versus replacing

  - likewise, replace-all versus a single replace

  - likewise, unset versus unset-all

  - type interpretation / normalization

  - value regex matching

Even if we don't implement all of that immediately, it would be nice to have a plan for how they'd be added to stdin mode eventually.

> +Alternatively, use `-z` or `--null` to specify in NUL-terminated format, without
> +quoting:

This mentions quoting, but the earlier form doesn't discuss it at all. I can see from the quote that it does unquote_c_style(). It might be worth saying so explicitly; since we're dealing with config, people may assume that the syntax is like the one in config files (which is similar, but not quite the same).

Show 21 quoted lines

> +static const char *parse_cmd_set(const char *next, const char *max_char)
> +{
> +	char *key, *value;
> +	int ret;
> +
> +	key = parse_key_or_value(&next, max_char);
> +	if (!key)
> +		die(_("set: missing key"));
> +
> +	value = parse_key_or_value(&next, max_char);
> +	if (!value)
> +		die(_("set: missing value"));
> +
> +	ret = git_config_set_in_file_gently(given_config_source.file, key, value);
> +	if (ret)
> +		die(_("cannot set key value pair: %d"), ret);
> +
> +	free(key);
> +	free(value);
> +	return next;
> +}

I mentioned flushing earlier. It looks like this will flush each individual "set" command by rewriting the whole file. But another reason one might want to use --stdin is to set multiple keys in one go, either for efficiency or because you'd like readers to see the result atomically.

I don't think we actually have a function to set multiple keys in one pass yet. And I'd be OK starting with a less-efficient implementation. But we probably should document that the current flushing behavior is not something people should count on, to leave us room to change it in the future.

The code itself looked cleanly done, though I admit I didn't look too closely. If I've convinced you on any of the design considerations above, I think it would need rewritten a bit.

-Peff