Re: [PATCH RFC] show-branch: use pager

From: Jeff King <peff@peff.net>
Date: Jun 13, 2013, 07:01 UTC
Message-ID: <20130613070118.GA23235@sigill.intra.peff.net>
In-Reply-To: <1371105811-3112-1-git-send-email-oystwa@gmail.com>

On Thu, Jun 13, 2013 at 08:43:31AM +0200, Øystein Walle wrote:

> This is for consistency with other porcelain commands such as 'log'.

I do not use show-branch myself, but being consistent with the other porcelain commands makes sense to me.

> I marked this as an RFC because of Jeff King's comments in
> daa0c3d97 where I got the impression this this might not be a good idea.
> However I haven't found any bugs and all the tests pass. It is more a huble
> suggestion than anything but I thought I might as well send it as a patch.

I don't think the problems described in daa0c3d97 should be an issue for us, as the purpose of that commit was to delay the color decision until the last minute. That helps commands which load color config before having decided on whether to use a pager. In other words, it covers the exact situation you introduce here:

Show 9 quoted lines

> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index 90fc6b1..bd3e10c 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -683,6 +683,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  	};
>  
>  	git_config(git_show_branch_config, NULL);
> +	setup_pager();

So I think your patch is fine with respect to those problems.

However, I do not see any need for show_branch to delay its pager setup at all. Commands like "git diff" and "git log" must do so, because they do not know whether they want a pager or not until after parsing command-line arguments. But in this case we are always starting the pager.

Would it make more sense to just set the USE_PAGER flag in the "show-branch" entry in git.c (see the "shortlog" entry for an example)?

-Peff