Re: [PATCH 8/9] builtin: check pager.<cmd> configuration if RUN_SETUP_GENTLY is used

[Jonathan Nieder <jrnieder@gmail.com>]

Nguyen Thai Ngoc Duy wrote:
> 2010/4/13 Jonathan Nieder <jrnieder@gmail.com>:

>> @@ -251,7 +251,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
[...]
>> -		if (use_pager == -1 && p->option & RUN_SETUP)
>> +		if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY))
>>			use_pager = check_pager_config(p->cmd);
>>		if (use_pager == -1 && p->option & USE_PAGER)
>>			use_pager = 1;
>
> This still leaves a chance of going wrong: when user explicitly gives
> "--paginate", use_pager will be 1, but commands like "git init" does
> not have RUN_SETUP*. So when setup_pager is called later on, it will
> mess things up.

I decided not to check_pager_config() unconditionally to avoid
breaking ‘git diff’ (especially ‘git diff --exit-code’).  Maybe that
is too conservative; not sure.  The commit message should be more
explicit, something like:

   Eventually, all git commands should check their configuration at
   start-up.  For now, reading configuration before repository discovery
   is dangerous because it could cause a pager.<cmd> setting from the
   current repository to be neglected.

   But for commands with RUN_SETUP or RUN_SETUP_GENTLY set, it is safe.

   This will not affect commands like "git init" that cannot have
   RUN_SETUP* set; they will have to be helped separately later.  In
   particular, commands such as "git diff" that use command-specific
   options to decide whether to search for a repository and whether to
   paginate output are outside the scope of this change.

Guiding principle: better to leave some bugs for later than risk
regressions.

> This could be solved completely (indeed I have a patch
> under testing), but it would require unset_git_directory(), making
> this series a bit longer :(

I agree; that’s the right way to do it.  run_builtin() ought to be
relatively sure about whether a repo is going to be searched for.  For
commands like diff and grep, I think it should be okay to search for a
repo unless --no-index is the first argument.

Thanks for your thoughtfulness.
Jonathan