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

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

[Per Cederqvist]

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.

The following script reproduces the problem.  I've tried both git
2.4.3 and git 2.5.0.

----- cut here -----
#!/bin/sh
set -e -x

# All created files are created inside the "badcolor" directory.
mkdir badcolor
cd badcolor

# Create a bare repo.
mkdir upstream.git
(cd upstream.git && git init --bare)

# Make an initial commit.
git clone upstream.git repo-a
(cd repo-a && echo one > a && git add a && git commit -m"First A commit")
(cd repo-a && git push origin master)

# Make a second clone.
git clone upstream.git repo-b

# Make one more commit, that the second clone won't have for a while.
(cd repo-a && echo two > a && git add a && git commit -m"Second A commit")
(cd repo-a && git push origin master)

# Create a third commit; this make the history non-linear, but since
# the commit only touched a new file it should be trivial to linearize
# it.
(cd repo-b && echo one > b && git add b && git commit -m"First B commit")

# Set pager.pull true so that we trigger the bug.
(cd repo-b && git config pager.pull true)

# Attempt to make the history linear.  This command will fail if TERM
# specifies a color-capable terminal.

(cd repo-b && git pull --rebase)

exit 0
----- cut here -----

Thanks,

    /ceder
-- 
Per Cederqvist <cederp@opera.com>

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

[Jeff King]

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

> 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);
 }
 
 /*

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

[Jeff King]

On Sun, Aug 09, 2015 at 07:42:38PM -0400, Jeff King wrote:

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

Here's the fix I came up with. The first patch is just a tiny
refactoring; second one is the interesting bit.

  [1/2]: pager_in_use: use git_env_bool
  [2/2]: pager_in_use: make sure output is still going to pager

-Peff

[PATCH 1/2] pager_in_use: use git_env_bool

[Jeff King]

This function basically reimplements git_env_bool (because
it predates it). Let's reuse that helper, which is shorter
and avoids repeating a string literal.

Signed-off-by: Jeff King <peff@peff.net>
---
 pager.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/pager.c b/pager.c
index 070dc11..e3ad465 100644
--- a/pager.c
+++ b/pager.c
@@ -95,9 +95,7 @@ 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;
+	return git_env_bool("GIT_PAGER_IN_USE", 0);
 }
 
 /*
-- 
2.5.0.414.g670f2a4

[PATCH 2/2] pager_in_use: make sure output is still going to pager

[Jeff King]

When we start a pager, we set GIT_PAGER_IN_USE=1 in the
environment. This lets sub-processes know that even though
isatty(1) is not true, it is because it is connected to a
pager (and we should still turn on human-readable niceties
like auto-color).

Unfortunately, this is too inclusive for scripts which
invoke git sub-programs with their stdout going somewhere
else. For example, if you run "git -p pull rebase", git-pull
will invoke "git rebase", which invokes:

  git format-patch ... >rebased-patches

This format-patch process knows that its stdout is not a
tty, but because of GIT_PAGER_IN_USE it assumes this is
because stdout is going to a pager. As a result, it writes
colorized output, and the matching "git am" invocation
chokes on it, causing the rebase to fail.

We could work around this by passing "--no-color" to
format-patch, or by removing GIT_PAGER_IN_USE from the
environment. But we should not have to do so; format-patch
should be able to realize that even though GIT_PAGER_IN_USE
is set, its stdout is not actually going to that pager.

For this simple case, format-patch could see that its output
is not even a pipe. But that would not catch a case like:

  git format-patch | some-program >rebased-patches

where it cannot distinguish between the pipe to the pager
and the pipe to some-program.

This patch solves it by actually noting the inode of the
pipe to the pager in the environment, which readers of
GIT_PAGER_IN_USE can check against their stdout. This
technically makes GIT_PAGER_IN_USE redundant (we can just
check the new GIT_PAGER_PIPE_ID), but we keep using both
variables for compatibility with external scripts:

  - scripts which check GIT_PAGER_IN_USE can continue to do
    so, and will just ignore the new pipe-id variable.
    Meaning they may accidentally turn on colors if their
    output is redirected to a file, but that is the same as
    today and we cannot fix that. We do not actively break
    them from showing colors when their stdout _does_ go to
    the pager.

  - scripts which set GIT_PAGER_IN_USE but not
    GIT_PAGER_PIPE_ID will continue to turn on colorization
    for git sub-commands (again, they do not benefit from
    the new code, but we are not making anything worse).

The inode-retrieval code itself is abstracted into compat/,
as different platforms may represent the pipe id
differently. These ids do not need to be portable across
systems, only within processes on the same system.

Note that there is an existing test in t7006 which tests for
the exact _opposite_ of what we are trying to achieve
(namely, that GIT_PAGER_IN_USE does _not_ cause us to write
colors to a random file). This test comes from a battery of
tests added by 60b6e22 (tests: Add tests for automatic use
of pager, 2010-02-19), and I think is simply misguided, as
evidenced by the real "git pull" bug above. If you want to
ensure colors in a file, you do it with "--color", not by
pretending you have a pager.

Rather than delete the test, though, we simply re-title it
here. It actually makes a good check of the "scripts which
set PAGER_IN_USE but not PAGER_PIPE_ID" historical
compatibility mentioned above.

Signed-off-by: Jeff King <peff@peff.net>
---
+cc Jonathan, whose test I am calling misguided above. :)

I dug in the list and could not find anything interesting said about
this particular one. The motivation for adding the tests was a
regression in git-svn, and we added a bunch of tests in response. I
doubt that you remember it well 5 years later, but it does not hurt to
check.

 Makefile         |  1 +
 compat/pipe-id.c | 25 +++++++++++++++++++++++++
 compat/pipe-id.h | 27 +++++++++++++++++++++++++++
 pager.c          | 16 +++++++++++++++-
 t/t7006-pager.sh | 20 +++++++++++++++++++-
 5 files changed, 87 insertions(+), 2 deletions(-)
 create mode 100644 compat/pipe-id.c
 create mode 100644 compat/pipe-id.h

diff --git a/Makefile b/Makefile
index 7efedbe..72f7b56 100644
--- a/Makefile
+++ b/Makefile
@@ -679,6 +679,7 @@ LIB_OBJS += column.o
 LIB_OBJS += combine-diff.o
 LIB_OBJS += commit.o
 LIB_OBJS += compat/obstack.o
+LIB_OBJS += compat/pipe-id.o
 LIB_OBJS += compat/terminal.o
 LIB_OBJS += config.o
 LIB_OBJS += connect.o
diff --git a/compat/pipe-id.c b/compat/pipe-id.c
new file mode 100644
index 0000000..4764c5f
--- /dev/null
+++ b/compat/pipe-id.c
@@ -0,0 +1,25 @@
+#include "git-compat-util.h"
+#include "compat/pipe-id.h"
+#include "strbuf.h"
+
+const char *pipe_id_get(int fd)
+{
+	static struct strbuf id = STRBUF_INIT;
+	struct stat st;
+
+	if (fstat(fd, &st) < 0 || !S_ISFIFO(st.st_mode))
+		return NULL;
+
+	strbuf_reset(&id);
+	strbuf_addf(&id, "%lu", (unsigned long)st.st_ino);
+	return id.buf;
+}
+
+int pipe_id_match(int fd, const char *id)
+{
+	struct stat st;
+
+	if (fstat(fd, &st) < 0 || !S_ISFIFO(st.st_mode))
+		return 0;
+	return st.st_ino == strtoul(id, NULL, 10);
+}
diff --git a/compat/pipe-id.h b/compat/pipe-id.h
new file mode 100644
index 0000000..5ddff2c
--- /dev/null
+++ b/compat/pipe-id.h
@@ -0,0 +1,27 @@
+#ifndef PIPE_ID_H
+#define PIPE_ID_H
+
+/**
+ * This module allows callers to save a string pipe identifier, and later find
+ * out whether a file descriptor refers to the same pipe.
+ *
+ * The ids should be opaque to the callers, as their implementation may be
+ * system dependent. The generated ids can be used between processes on the
+ * same system, but are not portable between systems, or even between different
+ * versions of git.
+ */
+
+/**
+ * Returns a string representing the pipe-id of the file descriptor `fd`, or
+ * NULL if an error occurs. Note that the return value may be invalidated by
+ * subsequent calls to pipe_id_get.
+ */
+const char *pipe_id_get(int fd);
+
+/**
+ * Returns 1 if the pipe at `fd` matches the id `id`, or 0 otherwise (or if an
+ * error occurs).
+ */
+int pipe_id_match(int fd, const char *id);
+
+#endif /* PIPE_ID_H */
diff --git a/pager.c b/pager.c
index e3ad465..de00def 100644
--- a/pager.c
+++ b/pager.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "run-command.h"
 #include "sigchain.h"
+#include "compat/pipe-id.h"
 
 #ifndef DEFAULT_PAGER
 #define DEFAULT_PAGER "less"
@@ -57,6 +58,7 @@ const char *git_pager(int stdout_is_tty)
 void setup_pager(void)
 {
 	const char *pager = git_pager(isatty(1));
+	const char *pipe_id;
 
 	if (!pager)
 		return;
@@ -88,6 +90,10 @@ void setup_pager(void)
 		dup2(pager_process.in, 2);
 	close(pager_process.in);
 
+	pipe_id = pipe_id_get(1);
+	if (pipe_id)
+		setenv("GIT_PAGER_PIPE_ID", pipe_id, 1);
+
 	/* this makes sure that the parent terminates after the pager */
 	sigchain_push_common(wait_for_pager_signal);
 	atexit(wait_for_pager);
@@ -95,7 +101,15 @@ void setup_pager(void)
 
 int pager_in_use(void)
 {
-	return git_env_bool("GIT_PAGER_IN_USE", 0);
+	const char *pipe_id;
+
+	if (!git_env_bool("GIT_PAGER_IN_USE", 0))
+		return 0;
+
+	pipe_id = getenv("GIT_PAGER_PIPE_ID");
+	if (!pipe_id) /* historical compatibility */
+		return 1;
+	return pipe_id_match(1, pipe_id);
 }
 
 /*
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 947b690..2f77299 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -158,7 +158,7 @@ test_expect_success TTY 'colors are suppressed by color.pager' '
 	! colorful paginated.out
 '
 
-test_expect_success 'color when writing to a file intended for a pager' '
+test_expect_success 'color goes to files when GIT_PAGER_PIPE_ID is not used' '
 	rm -f colorful.log &&
 	test_config color.ui auto &&
 	(
@@ -177,6 +177,24 @@ test_expect_success TTY 'colors are sent to pager for external commands' '
 	colorful paginated.out
 '
 
+test_expect_success TTY 'no color when paged program writes to file' '
+	test_config alias.externallog "!git log >log.out" &&
+	test_config color.ui auto &&
+	test_terminal env TERM=vt100 git -p externallog &&
+	test_line_count = 0 paginated.out &&
+	test -s log.out &&
+	! colorful log.out
+'
+
+test_expect_success TTY 'no color when paged program writes to pipe' '
+	test_config alias.externallog "!git log | cat >log.out" &&
+	test_config color.ui auto &&
+	test_terminal env TERM=vt100 git -p externallog &&
+	test_line_count = 0 paginated.out &&
+	test -s log.out &&
+	! colorful log.out
+'
+
 # Use this helper to make it easy for the caller of your
 # terminal-using function to specify whether it should fail.
 # If you write
-- 
2.5.0.414.g670f2a4

Re: [PATCH 2/2] pager_in_use: make sure output is still going to pager

[Johannes Schindelin]

Hi Peff,

On 2015-08-10 07:23, Jeff King wrote:

> diff --git a/compat/pipe-id.c b/compat/pipe-id.c
> new file mode 100644
> index 0000000..4764c5f
> --- /dev/null
> +++ b/compat/pipe-id.c
> @@ -0,0 +1,25 @@
> +#include "git-compat-util.h"
> +#include "compat/pipe-id.h"
> +#include "strbuf.h"
> +
> +const char *pipe_id_get(int fd)
> +{
> +	static struct strbuf id = STRBUF_INIT;
> +	struct stat st;
> +
> +	if (fstat(fd, &st) < 0 || !S_ISFIFO(st.st_mode))
> +		return NULL;

Just a quick note: it seems that this check is not really working on Windows. I tested this by running this test case manually (because TTY is not set on Windows):

> +test_expect_success TTY 'no color when paged program writes to pipe' '
> +	test_config alias.externallog "!git log | cat >log.out" &&
> +	test_config color.ui auto &&
> +	test_terminal env TERM=vt100 git -p externallog &&
> +	test_line_count = 0 paginated.out &&
> +	test -s log.out &&
> +	! colorful log.out
> +'

The output is "colorful" ;-)

I hope to find some time tomorrow to figure out some workaround that makes this work on Windows.

Ciao,
Dscho

Re: [PATCH 2/2] pager_in_use: make sure output is still going to pager

[Jeff King]

On Mon, Aug 10, 2015 at 06:38:10PM +0200, Johannes Schindelin wrote:

> > +const char *pipe_id_get(int fd)
> > +{
> > +	static struct strbuf id = STRBUF_INIT;
> > +	struct stat st;
> > +
> > +	if (fstat(fd, &st) < 0 || !S_ISFIFO(st.st_mode))
> > +		return NULL;
> 
> Just a quick note: it seems that this check is not really working on
> Windows. I tested this by running this test case manually (because TTY
> is not set on Windows):

Yeah, I'm not too surprised. I'm guessing your st_ino for pipes are all
just the same or something. Or maybe S_ISFIFO doesn't pass (we don't
technically need it, I don't think, and could just drop that check).

Anyway, I had planned that we would need to stick a big "#ifdef WINDOWS"
around these two functions.

> I hope to find some time tomorrow to figure out some workaround that
> makes this work on Windows.

Cool. I can't comment on Windows-specific stuff, but I'm happy to review
the rest of it. :)

-Peff

Re: [PATCH 2/2] pager_in_use: make sure output is still going to pager

[Per Cederqvist]

On Mon, Aug 10, 2015 at 7:24 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Aug 10, 2015 at 06:38:10PM +0200, Johannes Schindelin wrote:
>
>> > +const char *pipe_id_get(int fd)
>> > +{
>> > +   static struct strbuf id = STRBUF_INIT;
>> > +   struct stat st;
>> > +
>> > +   if (fstat(fd, &st) < 0 || !S_ISFIFO(st.st_mode))
>> > +           return NULL;
>>
>> Just a quick note: it seems that this check is not really working on
>> Windows. I tested this by running this test case manually (because TTY
>> is not set on Windows):
>
> Yeah, I'm not too surprised. I'm guessing your st_ino for pipes are all
> just the same or something. Or maybe S_ISFIFO doesn't pass (we don't
> technically need it, I don't think, and could just drop that check).

If you remove the S_ISFIFO check, you probably need to include
the st_dev field in the pipe id.  Otherwise, you could be unlucky and
redirect the output to a file that just happens to have the same inode
number as the pager pipe. Remember, inode numbers are only unique
within a certain st_dev.

    /ceder