Re: "git pull --rebase" fails if pager.pull is true, after producing a colorized diff it cannot apply

From: Jeff King <peff@peff.net>
Date: Aug 9, 2015, 23:42 UTC
Message-ID: <20150809234238.GB25769@sigill.intra.peff.net>
In-Reply-To: <CAP=KgsTp=D1cSPmudDVEe32Q8gHhfSfuL7+V9YGZ65F1ZDUFiA@mail.gmail.com>

On Mon, Aug 03, 2015 at 05:21:43PM +0200, Per Cederqvist wrote:

Show 27 quoted lines

> If you run:
> 
>     git config pager.pull true
> 
> in the hope of getting the output of "git pull" via a pager, you are
> in for a surpise the next time you run "git pull --rebase" and it has
> to rebase your work.  It will fail with a nonsensical error message:
> 
> > Applying: First B commit
> > fatal: unrecognized input
> > Repository lacks necessary blobs to fall back on 3-way merge.
> > Cannot fall back to three-way merge.
> > Patch failed at 0001 First B commit
> > The copy of the patch that failed is found in:
> >    /home/cederp/badcolor/repo-b/.git/rebase-apply/patch
> >
> > When you have resolved this problem, run "git rebase --continue".
> > If you prefer to skip this patch, run "git rebase --skip" instead.
> > To check out the original branch and stop rebasing, run "git rebase --abort".
> 
> Using "cat -vet" to look at the problematic patch, you can see that
> there are embedded escape codes that tries to colorize the patch.
> 
> This bug is dependent on the TERM setting.  On my system (Ubuntu
> 14.04) it reproduces if TERM=vt220 or TERM=rxvt-unicode, but not if
> TERM=dumb.  It might depend on the color.diff setting as well, but
> it does reproduce with the default setting.

It looks like the use of a pager is fooling our "should we colorize the diff" check when generating the patches. Usually we check isatty(1) to see if we should use color, so "git format-patch >patches" does the right thing. But if a pager is in use, we have to override that check (since stdout goes to the pager, but the pager is going to a tty). That propagates to children via the GIT_PAGER_IN_USE environment variable.

We could work around this by having pull explicitly tell rebase that it is not using a pager (by unsetting GIT_PAGER_IN_USE). Or by having rebase tell it to format-patch. But I think the best thing is probably to teach the low-level "are we going to a pager" check to only say "yes" if stdout is still a pipe, like the patch below. That lets:

  git format-patch --stdout >patches

do the right thing; it knows that even if a pager is in use, its output is not going to it, because stdout isn't a pipe.

Unfortunately this does not help:

  git format-patch --stdout | some_program

because it cannot tell the difference between the pipe to the original pager. I wonder if we could do something even more clever, like putting the inode number in the environment. Then we could check if we have the _same_ pipe going to the pager.

diff --git a/pager.c b/pager.c
index 070dc11..5b3b3fd 100644
--- a/pager.c
+++ b/pager.c
@@ -95,9 +95,11 @@ void setup_pager(void)
 
 int pager_in_use(void)
 {
-	const char *env;
-	env = getenv("GIT_PAGER_IN_USE");
-	return env ? git_config_bool("GIT_PAGER_IN_USE", env) : 0;
+	struct stat st;
+
+	return git_env_bool("GIT_PAGER_IN_USE", 0) &&
+	       !fstat(1, &st) &&
+	       S_ISFIFO(st.st_mode);
 }
 
 /*