(To Junio, this series conflicts slightly with
nd/conditional-config-include, let me know if you want me to rebase
this on top of that)
On Sun, Apr 16, 2017 at 10:51 PM, Jeff King <peff@xxxxxxxx> wrote:
>> + if (opts.git_dir) {
>> struct git_config_source repo_config;
>>
>> memset(&repo_config, 0, sizeof(repo_config));
>> - strbuf_addstr(&buf, "/config");
>> + strbuf_addf(&buf, "%s/config", opts.git_dir);
>> repo_config.file = buf.buf;
>> git_config_with_options(cb, data, &repo_config, &opts);
>> }
>
> ...we have to re-add the git_dir.
>
> Might it be simpler to just xstrdup() to opts.git_dir, and then leave
> this later code alone?
Sure thing. But we need to restore the "if" expression too. Otherwise,
if have_git_dir() is true, we may come here with an empty "buf" before
that strbuf_addstr(&buf, "/config") is called. It does not matter much
anyway because this block will be removed.
> You can see the second problem with:
>
> # random external
> cat >git-foo <<-\EOF
> #!/bin/sh
> echo foo
> EOF
> chmod +x git-foo
> PATH=$PWD:$PATH
>
> git init
> git config pager.foo 'sed s/^/repo:/'
> git -c pager.foo='sed s/^/cmdline:/' foo
>
> That command should prefer the cmdline config, but it doesn't.
I actually had problems seeing the problem, for some reason it didn't
work for me. I guess I made a mistake somewhere.
> The fix is something like what's below, which is easy on top of your new
> options struct. I can wrap it up with a config message and test on top
> of your series.
... anyway I read this last sentence too late and spent too much time
wrapping your changes in a patch (well most of the time was spent on
writing a new test actually), so I'm sending it out too. My test uses
test-config though (I have given up on dealing with pager and tty).
Off topic. Back to the pager.foo thing (because I had to fight it a
bit before giving up). I find it a bit unintuitive that "--paginate"
forces the pager back to less (or $PAGER) when I say "pager.foo =
my-favorite-pager". Back when pager.foo is a boolean thing, it makes
sense for --paginate to override the "to page or not to page"
decision. But then you added a new meaning too pager.foo (what command
to run). "--paginate" should respect the command pager.foo specifies
when its value is a command, I think.
--
Duy