Re: [PATCH 2/3] pager: factor out a helper to prepare a child process to run the pager

On Tue, Feb 16, 2016 at 03:49:55PM -0800, Junio C Hamano wrote:

quoted

And if you put the git_pager() call inside prepare_pager_args (which I
agree would be cleaner), we just have:

  void prepare_pager_args(struct child_process *pager_process);

which is pretty self-explanatory (though it might need a new name; I'd
be tempted to call it init_pager_process() or something, and actually
have it do the child_process_init() to make sure it is working with a
sane clean slate).

Conceptually I am on the same page, but I am not sure how well that
interacts with what "git am -i" codepath wants to do, though.

One big difference between the "we'll feed our output to pager"
codepath and "we'll spawn a pager to let a file on the filesystem be
read" codepath is that the former needs to call git_pager() and
check the NULL-ness of the return value to decide that it does not
want to spawn a pager and let the standard output just go straight
to the outside world.  The latter, on the other hand, does want to
spawn something to cause the file to be presented to the end user
even git_pager() returns NULL.

And that is why I didn't make this helper call git_pager() itself.

That makes sense. I didn't dig into it carefully. I saw the "pager=cat"
thing in the context of your diff to git-am, and assumed it was weird
fallback that should be done by the regular pager infrastructure. But
it's the exact thing you're talking about here.

So of all of the things I suggested, I think the non-varargs one that
takes "pager" as a string makes the most sense.

-Peff