Re: [PATCH v2 05/10] git.c: provide setup_auto_pager()

From: Jeff King <peff@peff.net>
Date: Jul 31, 2017, 03:34 UTC
Message-ID: <20170731033429.dgftfeum6n3ddxua@sigill.intra.peff.net>
In-Reply-To: <b6ab825d3bda4b40201b0aa8e97c9fd721ed8171.1500321658.git.martin.agren@gmail.com>

On Mon, Jul 17, 2017 at 10:10:47PM +0200, Martin Ågren wrote:

Show 15 quoted lines

> The previous patch introduced a way for builtins to declare that they
> will take responsibility for handling the `pager.foo`-config item. (See
> the commit message of that patch for why that could be useful.)
> 
> Provide setup_auto_pager(), which builtins can call in order to handle
> `pager.<cmd>`, including possibly starting the pager. Make this function
> don't do anything if a pager has already been started, as indicated by
> use_pager or pager_in_use().
> 
> Whenever this function is called from a builtin, git.c will already have
> called commit_pager_choice(). Since commit_pager_choice() treats the
> special value -1 as "punt" or "not yet decided", it is not a problem
> that we might end up calling commit_pager_choice() once in git.c and
> once (or more) in the builtin. Make the new function use -1 in the same
> way and document it as "punt".

At first I wasn't sure if it would ever make sense to use "-1" here. The "punt" that happens in earlier calls to commit_pager_choice() is there because we might adjust our decision later. And this would generally be the final decision, I would think. So I'd be surprised if we had anything besides "0" or "1" in the "def" argument.

But thinking on it, the most plausible case is something like:

  setup_auto_pager("foo", -1);
  ...
  /* fallback to some historical compatibility name */
  setup_auto_pager("bar", 0);

And it's important for the "-1" there to be a true punt, and not do anything in commit_pager_choice(). So it's probably worth documenting the "-1" behavior as you did here as a possible value for "def".

-Peff