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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 14, 2010, 04:38 UTC
Message-ID: <20100414043844.GA28864@progeny.tock>
In-Reply-To: <u2ufcaeb9bf1004122229l72de0e55i8dbd993d6cb16d2b@mail.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)

[...]

Show 10 quoted lines

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