On 05 Feb 2016, at 14:58, Jeff King <p...@peff.net> wrote: > On Fri, Feb 05, 2016 at 12:31:15PM +0100, Sebastian Schuberth wrote: > >>> 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=p...@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). >> >> Or to come up with a special string to denote config values specified on the >> command line. Maybe somehting like >> >> <command line> <tab> foo.bar=true >> >> I acknowledge that "<command line>" would be a valid filename on some >> filesystems, but I think the risk is rather low that someone would actually >> be using that name for a Git config file. > > Yeah, I agree it's unlikely. And the output is already ambiguous, as the > first field could be a blob (though I guess the caller knows if they > passed "--blob" or not). If we really wanted an unambiguous output, we > could have something like "file:...", "blob:...", etc. But that's a bit > less readable for humans, and I don't think solves any real-world > problems. > > So I think it would be OK to use "<command line>" here, as long as the > token is documented. Sounds good to me. I'll add it!
> Are there any other reasons why current_config_filename() would return > NULL, besides command-line config? I don't think so. It looks like we > can read config from stdin, but we use the token "<stdin>" there for the > name field (another ambiguity!). During my testing I haven't found any other case either. To be honest I didn't know the "stdin" way to set the config! I added a test case for that, too! Thanks, Lars-- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
