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

From: Duy Nguyen <pclouds@gmail.com>
Date: Apr 17, 2017, 10:07 UTC
Message-ID: <CACsJy8D-=3qPbEO-dq2+Mr-WZ+5RrivkSw9NX7_FhEJFy+59zA@mail.gmail.com>
In-Reply-To: <20170416155131.ppp5iakohiiphzmk@sigill.intra.peff.net>

(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@peff.net> wrote:

Show 14 quoted lines

>> +     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.

Show 15 quoted lines

> 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