From: Jeff King <peff@peff.net>
To: larsxschneider@gmail.com
Cc: git@vger.kernel.org
Subject: Re: [PATCH v1] config: add '--sources' option to print the source of a config value
Date: Fri, 5 Feb 2016 06:20:01 -0500 [thread overview]
Message-ID: <20160205112001.GA13397@sigill.intra.peff.net> (raw)
In-Reply-To: <1454661750-85703-1-git-send-email-larsxschneider@gmail.com>
On Fri, Feb 05, 2016 at 09:42:30AM +0100, larsxschneider@gmail.com wrote:
> @@ -538,6 +569,17 @@ int cmd_config(int argc, const char **argv, const char *prefix)
> error("--name-only is only applicable to --list or --get-regexp");
> usage_with_options(builtin_config_usage, builtin_config_options);
> }
> +
> + const int is_query_action = actions & (
> + ACTION_GET|ACTION_GET_ALL|ACTION_GET_REGEXP|ACTION_LIST|
> + ACTION_GET_COLOR|ACTION_GET_COLORBOOL|ACTION_GET_URLMATCH
> + );
> +
> + if (show_sources && !is_query_action) {
> + error("--sources is only applicable to --list or --get-* actions");
> + usage_with_options(builtin_config_usage, builtin_config_options);
> + }
Hmm. I had originally envisioned this only being used with "--list", but
I guess it makes sense to say "--sources --get" to show where the value
for a particular option is coming from.
The plural "sources" is a little funny there, though, as we list only
the "last one wins" final value, not all of them (for that, you can use
"--sources --get-all", which seems handy). I think it would be
sufficient for the documentation to make this clear (speaking of which,
this patch needs documentation...).
Also, I don't think the feature works with --get-color, --get-colorbool,
or --get-urlmatch (which don't do their output in quite the same way).
I think it would be fine to simply disallow --sources with those options
rather than worry about making it work.
> +/* output to either fp or buf; only one should be non-NULL */
> +static void show_config_source(struct strbuf *buf, FILE *fp)
> +{
> + const char *fn = current_config_filename();
> + if (!fn)
> + return;
I'm not sure returning here is the best idea. We won't have a config
filename if we are reading from "-c", but if we return early from this
function, it parses differently than every other line. E.g., with your
patch, if I do:
git config -c foo.bar=true config --sources --list
I'll get:
/home/peff/.gitconfig <tab> user.name=Jeff King
/home/peff/.gitconfig <tab> user.email=peff@peff.net
...etc...
foo.bar=true
If somebody is parsing this as a tab-delimited list, then instead of the
source field for that line being empty, it is missing (and it looks like
"foo.bar=true" is the source file). I think it would be more friendly to
consumers of the output to have a blank (i.e., set "fn" to the empty
string and continue in the function).
> +
> + char term = '\t';
This declaration comes after the "if" above, but git style doesn't allow
declaration-after-statement (to support ancient compilers).
> +test_expect_success '--sources' '
> + >.git/config &&
> + >"$HOME"/.gitconfig &&
> + INCLUDE_DIR="$HOME/include" &&
> + mkdir -p "$INCLUDE_DIR" &&
> + cat >"$INCLUDE_DIR"/include.conf <<-EOF &&
> + [user]
> + include = true
> + EOF
> + cat >"$HOME"/file.conf <<-EOF &&
> + [user]
> + custom = true
> + EOF
> + test_config_global user.global "true" &&
> + test_config_global user.override "global" &&
> + test_config_global include.path "$INCLUDE_DIR"/include.conf &&
Here you include the file by its absolute path. I wondered what would
happen if we used a relative path. E.g.:
git config include.path=foo
git config -f .git/foo included.config=true
git config --sources --list
which shows it as ".git/foo", because we resolved it by manipulating the
relative path ".git/config". Whereas including it from ~/.gitconfig will
show the absolute path, because we use the absolute path to get to
~/.gitconfig in the first place.
I think that's all sane. I don't know if it's worth noting it in the
documentation or not.
> + cat >expect <<-EOF &&
> + $HOME/.gitconfig user.global=true
> + $HOME/.gitconfig user.override=global
> + $HOME/.gitconfig include.path=$INCLUDE_DIR/include.conf
> + $INCLUDE_DIR/include.conf user.include=true
> + .git/config user.local=true
> + .git/config user.override=local
> + user.cmdline=true
> + EOF
If the filename has funny characters (e.g., a literal tab), it will be
quoted here (but not in the --null output below). Worth including in the
test?
> + cat >expect <<-EOF &&
> + .git/config local
> + EOF
> + git config --sources user.override >output &&
> + test_cmp expect output &&
Good thoroughness in checking the override case.
> + cat >expect <<-EOF &&
> + a9d9f9e555b5c6f07cbe09d3f06fe3df11e09c08 user.custom=true
> + EOF
> + blob=$(git hash-object -w "$HOME"/file.conf) &&
> + git config --blob=$blob --sources --list >output &&
> + test_cmp expect output
This one was unexpected to me, but I think it makes sense. The option is
"--sources" and not "--source-filenames", after all. It's probably worth
mentioning in the documentation.
I think we also use the original name given, so if there was ref
resolution, you would see the ref name. Might be worth testing that.
-Peff
next prev parent reply other threads:[~2016-02-05 11:20 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-05 8:42 [PATCH v1] config: add '--sources' option to print the source of a config value larsxschneider
2016-02-05 11:13 ` Sebastian Schuberth
2016-02-05 11:22 ` Jeff King
2016-02-07 19:28 ` Lars Schneider
2016-02-08 11:22 ` Sebastian Schuberth
2016-02-08 12:11 ` Jeff King
2016-02-05 11:20 ` Jeff King [this message]
2016-02-05 11:31 ` Sebastian Schuberth
2016-02-05 13:58 ` Jeff King
2016-02-07 19:44 ` Lars Schneider
2016-02-08 12:12 ` Jeff King
2016-02-08 11:25 ` Sebastian Schuberth
2016-02-08 12:08 ` Jeff King
2016-02-07 18:26 ` Lars Schneider
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=20160205112001.GA13397@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=larsxschneider@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.
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).