On Fri, Feb 05, 2016 at 09:42:30AM +0100, larsxschneider@gmail.com wrote:

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