Web lists-archives.com

Re: [PATCH 2/2] config: handle conditional include when $GIT_DIR is not set up




(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