Re: [PATCH v2] config: add string mapping for enum config_scope

From: Jeff King <peff@peff.net>
Date: Dec 12, 2019, 03:55 UTC
Message-ID: <20191212035515.GB1200720@coredump.intra.peff.net>
In-Reply-To: <20191212034547.GA226275@google.com>

On Wed, Dec 11, 2019 at 07:45:47PM -0800, Emily Shaffer wrote:

Show 14 quoted lines

> > There are no callers added here, so I'm not sure exactly how the new
> > function is meant to be used. But I'd caution that it might be worth
> > showing the scope _and_ the path, instead of one or the other.
> 
> Yeah, I hear you - I had added this originally to the config-based hooks
> topic to get an output like this:
> 
> $ git hook --list pre-commit
> 001	global	~/foo.sh
> 002	local	~/bar.sh
> 
> That's a scenario where it might be handy to add the path, especially if
> it's coming in via an import, sure. (For brevity I think I'd want to
> turn it on via an argument.)

Yeah, there I think showing the file path would be helpful.

Show 8 quoted lines

> As I was working through the comments on v3 of git-bugreport, though, I
> saw a request to add the origin of the configs - and that's a case where
> I don't necessarily want someone to see, say:
> 
> [Selected Configs]
> user.name (/home/emily/robot-revolution/stairclimber/.git/config) : Emily Shaffer
> 
> when I mail that bugreport to the Git list.

Yeah, agreed. You'd want less information there, because we should be sensitive to sharing user filesystem paths.

Show 12 quoted lines

> So, I think I hear what you're saying - use wisely - but I think it's OK
> for a user to say:
> 
>   printf("%s (%s): %s = %s\n",
>          current_config_name(),
> 	 config_scope_to_string(current_config_scope()),
> 	 var,
> 	 value);
> 
> That is, I don't think the right solution is to make
> current_config_name() provide a stringification of
> current_config_scope() as well.

Yes, I think the way you're thinking about it all makes sense. We definitely can't just change current_config_name(); its output ends up in --show-origin, which is plumbing.

> Or, I guess we can decide that the bugreport scenario is different
> enough that this helper should exist only there, and everybody else
> should use current_config_name().

No, I think this helper makes sense in config.c. And then callers can choose how much (and in what format) to show the various bits as appropriate.

So this seems like the right direction.

-Peff