On 05 Feb 2016, at 12:20, Jeff King <peff@xxxxxxxx> wrote:
> On Fri, Feb 05, 2016 at 09:42:30AM +0100, larsxschneider@xxxxxxxxx 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...).
Oops. I will add 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.
OK, I'll remove them. I don't have experience with these options as I
have never really used them, yet.
>> +/* 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@xxxxxxxx
> ...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).
I actually wondered about that exact point in your original patch but
"parsing" did not come to my mind. Now I understand your reasoning and I
agree.
>
>> +
>> + char term = '\t';
>
> This declaration comes after the "if" above, but git style doesn't allow
> declaration-after-statement (to support ancient compilers).
Interesting, I noticed the style and wondered about it! Should we add
"-Werror=declaration-after-statement" to the TravisCI [1] build to catch these
kind of cases automatically?
After enabling this flag the compiler showed me that I did the same
error a few lines above in "const int is_query_action ...".
[1] https://travis-ci.org/larsxschneider/git/jobs/107610347
>
>> +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.
I agree, this is the behavior I would expect and therefore I don't think
any additional documentation is necessary. The relative include is a good
idea! I added it to the test case.
>
>> + 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?
Yes! Added!
>
>> + cat >expect <<-EOF &&
>> + .git/config local
>> + EOF
>> + git config --sources user.override >output &&
>> + test_cmp expect output &&
>
> Good thoroughness in checking the override case.
Thanks :)
>
>> + 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.
OK
>
> 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.
Good idea! Added!
Thanks for the review,
Lars
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html