{"thread":{"id":"40000","subject":"\"git pull --rebase\" fails if pager.pull is true, after producing a colorized diff it cannot apply","startedAt":"2015-08-03T15:21:43Z","lastAt":"2015-08-11T07:48:50Z","messageCount":8,"participants":["Per Cederqvist","Jeff King","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"267315","messageId":"CAP=KgsTp=D1cSPmudDVEe32Q8gHhfSfuL7+V9YGZ65F1ZDUFiA@mail.gmail.com","threadId":"40000","inReplyTo":null,"subject":"\"git pull --rebase\" fails if pager.pull is true, after producing a colorized diff it cannot apply","fromName":"Per Cederqvist","fromEmail":"cederp@opera.com","sentAt":"2015-08-03T15:21:43Z","receivedAt":"2015-08-03T15:21:43Z","isPatch":false,"sender":{"key":"cederp@opera.com","avatar":"https://gravatar.com/avatar/008fe28caedea0ba34042cd5157705267934f4ba7a9b29fb248ae18ec5b3055e?d=mp&s=160"},"body":"If you run:\n\n    git config pager.pull true\n\nin the hope of getting the output of \"git pull\" via a pager, you are\nin for a surpise the next time you run \"git pull --rebase\" and it has\nto rebase your work.  It will fail with a nonsensical error message:\n\n> Applying: First B commit\n> fatal: unrecognized input\n> Repository lacks necessary blobs to fall back on 3-way merge.\n> Cannot fall back to three-way merge.\n> Patch failed at 0001 First B commit\n> The copy of the patch that failed is found in:\n>    /home/cederp/badcolor/repo-b/.git/rebase-apply/patch\n>\n> When you have resolved this problem, run \"git rebase --continue\".\n> If you prefer to skip this patch, run \"git rebase --skip\" instead.\n> To check out the original branch and stop rebasing, run \"git rebase --abort\".\n\nUsing \"cat -vet\" to look at the problematic patch, you can see that\nthere are embedded escape codes that tries to colorize the patch.\n\nThis bug is dependent on the TERM setting.  On my system (Ubuntu\n14.04) it reproduces if TERM=vt220 or TERM=rxvt-unicode, but not if\nTERM=dumb.  It might depend on the color.diff setting as well, but\nit does reproduce with the default setting.\n\nThe following script reproduces the problem.  I've tried both git\n2.4.3 and git 2.5.0.\n\n----- cut here -----\n#!/bin/sh\nset -e -x\n\n# All created files are created inside the \"badcolor\" directory.\nmkdir badcolor\ncd badcolor\n\n# Create a bare repo.\nmkdir upstream.git\n(cd upstream.git && git init --bare)\n\n# Make an initial commit.\ngit clone upstream.git repo-a\n(cd repo-a && echo one > a && git add a && git commit -m\"First A commit\")\n(cd repo-a && git push origin master)\n\n# Make a second clone.\ngit clone upstream.git repo-b\n\n# Make one more commit, that the second clone won't have for a while.\n(cd repo-a && echo two > a && git add a && git commit -m\"Second A commit\")\n(cd repo-a && git push origin master)\n\n# Create a third commit; this make the history non-linear, but since\n# the commit only touched a new file it should be trivial to linearize\n# it.\n(cd repo-b && echo one > b && git add b && git commit -m\"First B commit\")\n\n# Set pager.pull true so that we trigger the bug.\n(cd repo-b && git config pager.pull true)\n\n# Attempt to make the history linear.  This command will fail if TERM\n# specifies a color-capable terminal.\n\n(cd repo-b && git pull --rebase)\n\nexit 0\n----- cut here -----\n\nThanks,\n\n    /ceder\n-- \nPer Cederqvist <cederp@opera.com>\n"},{"id":"267699","messageId":"20150809234238.GB25769@sigill.intra.peff.net","threadId":"40000","inReplyTo":"CAP=KgsTp=D1cSPmudDVEe32Q8gHhfSfuL7+V9YGZ65F1ZDUFiA@mail.gmail.com","subject":"Re: \"git pull --rebase\" fails if pager.pull is true, after producing a colorized diff it cannot apply","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-09T23:42:39Z","receivedAt":"2015-08-09T23:42:39Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 03, 2015 at 05:21:43PM +0200, Per Cederqvist wrote:\n\n> If you run:\n> \n>     git config pager.pull true\n> \n> in the hope of getting the output of \"git pull\" via a pager, you are\n> in for a surpise the next time you run \"git pull --rebase\" and it has\n> to rebase your work.  It will fail with a nonsensical error message:\n> \n> > Applying: First B commit\n> > fatal: unrecognized input\n> > Repository lacks necessary blobs to fall back on 3-way merge.\n> > Cannot fall back to three-way merge.\n> > Patch failed at 0001 First B commit\n> > The copy of the patch that failed is found in:\n> >    /home/cederp/badcolor/repo-b/.git/rebase-apply/patch\n> >\n> > When you have resolved this problem, run \"git rebase --continue\".\n> > If you prefer to skip this patch, run \"git rebase --skip\" instead.\n> > To check out the original branch and stop rebasing, run \"git rebase --abort\".\n> \n> Using \"cat -vet\" to look at the problematic patch, you can see that\n> there are embedded escape codes that tries to colorize the patch.\n> \n> This bug is dependent on the TERM setting.  On my system (Ubuntu\n> 14.04) it reproduces if TERM=vt220 or TERM=rxvt-unicode, but not if\n> TERM=dumb.  It might depend on the color.diff setting as well, but\n> it does reproduce with the default setting.\n\nIt looks like the use of a pager is fooling our \"should we colorize the\ndiff\" check when generating the patches. Usually we check isatty(1) to\nsee if we should use color, so \"git format-patch >patches\" does the\nright thing. But if a pager is in use, we have to override that check\n(since stdout goes to the pager, but the pager is going to a tty). That\npropagates to children via the GIT_PAGER_IN_USE environment variable.\n\nWe could work around this by having pull explicitly tell rebase that it\nis not using a pager (by unsetting GIT_PAGER_IN_USE). Or by having\nrebase tell it to format-patch. But I think the best thing is probably\nto teach the low-level \"are we going to a pager\" check to only say \"yes\"\nif stdout is still a pipe, like the patch below. That lets:\n\n  git format-patch --stdout >patches\n\ndo the right thing; it knows that even if a pager is in use, its output\nis not going to it, because stdout isn't a pipe.\n\nUnfortunately this does not help:\n\n  git format-patch --stdout | some_program\n\nbecause it cannot tell the difference between the pipe to the original\npager. I wonder if we could do something even more clever, like putting\nthe inode number in the environment. Then we could check if we have the\n_same_ pipe going to the pager.\n\ndiff --git a/pager.c b/pager.c\nindex 070dc11..5b3b3fd 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -95,9 +95,11 @@ void setup_pager(void)\n \n int pager_in_use(void)\n {\n-\tconst char *env;\n-\tenv = getenv(\"GIT_PAGER_IN_USE\");\n-\treturn env ? git_config_bool(\"GIT_PAGER_IN_USE\", env) : 0;\n+\tstruct stat st;\n+\n+\treturn git_env_bool(\"GIT_PAGER_IN_USE\", 0) &&\n+\t       !fstat(1, &st) &&\n+\t       S_ISFIFO(st.st_mode);\n }\n \n /*\n"},{"id":"267702","messageId":"20150810051901.GA9262@sigill.intra.peff.net","threadId":"40000","inReplyTo":"20150809234238.GB25769@sigill.intra.peff.net","subject":"Re: \"git pull --rebase\" fails if pager.pull is true, after producing a colorized diff it cannot apply","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T05:19:01Z","receivedAt":"2015-08-10T05:19:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 09, 2015 at 07:42:38PM -0400, Jeff King wrote:\n\n> It looks like the use of a pager is fooling our \"should we colorize the\n> diff\" check when generating the patches. Usually we check isatty(1) to\n> see if we should use color, so \"git format-patch >patches\" does the\n> right thing. But if a pager is in use, we have to override that check\n> (since stdout goes to the pager, but the pager is going to a tty). That\n> propagates to children via the GIT_PAGER_IN_USE environment variable.\n\nHere's the fix I came up with. The first patch is just a tiny\nrefactoring; second one is the interesting bit.\n\n  [1/2]: pager_in_use: use git_env_bool\n  [2/2]: pager_in_use: make sure output is still going to pager\n\n-Peff\n"},{"id":"267703","messageId":"20150810051933.GA15441@sigill.intra.peff.net","threadId":"40000","inReplyTo":"20150810051901.GA9262@sigill.intra.peff.net","subject":"[PATCH 1/2] pager_in_use: use git_env_bool","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T05:19:34Z","receivedAt":"2015-08-10T05:19:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function basically reimplements git_env_bool (because\nit predates it). Let's reuse that helper, which is shorter\nand avoids repeating a string literal.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pager.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 070dc11..e3ad465 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -95,9 +95,7 @@ void setup_pager(void)\n \n int pager_in_use(void)\n {\n-\tconst char *env;\n-\tenv = getenv(\"GIT_PAGER_IN_USE\");\n-\treturn env ? git_config_bool(\"GIT_PAGER_IN_USE\", env) : 0;\n+\treturn git_env_bool(\"GIT_PAGER_IN_USE\", 0);\n }\n \n /*\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267704","messageId":"20150810052353.GB15441@sigill.intra.peff.net","threadId":"40000","inReplyTo":"20150810051901.GA9262@sigill.intra.peff.net","subject":"[PATCH 2/2] pager_in_use: make sure output is still going to pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T05:23:54Z","receivedAt":"2015-08-10T05:23:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we start a pager, we set GIT_PAGER_IN_USE=1 in the\nenvironment. This lets sub-processes know that even though\nisatty(1) is not true, it is because it is connected to a\npager (and we should still turn on human-readable niceties\nlike auto-color).\n\nUnfortunately, this is too inclusive for scripts which\ninvoke git sub-programs with their stdout going somewhere\nelse. For example, if you run \"git -p pull rebase\", git-pull\nwill invoke \"git rebase\", which invokes:\n\n  git format-patch ... >rebased-patches\n\nThis format-patch process knows that its stdout is not a\ntty, but because of GIT_PAGER_IN_USE it assumes this is\nbecause stdout is going to a pager. As a result, it writes\ncolorized output, and the matching \"git am\" invocation\nchokes on it, causing the rebase to fail.\n\nWe could work around this by passing \"--no-color\" to\nformat-patch, or by removing GIT_PAGER_IN_USE from the\nenvironment. But we should not have to do so; format-patch\nshould be able to realize that even though GIT_PAGER_IN_USE\nis set, its stdout is not actually going to that pager.\n\nFor this simple case, format-patch could see that its output\nis not even a pipe. But that would not catch a case like:\n\n  git format-patch | some-program >rebased-patches\n\nwhere it cannot distinguish between the pipe to the pager\nand the pipe to some-program.\n\nThis patch solves it by actually noting the inode of the\npipe to the pager in the environment, which readers of\nGIT_PAGER_IN_USE can check against their stdout. This\ntechnically makes GIT_PAGER_IN_USE redundant (we can just\ncheck the new GIT_PAGER_PIPE_ID), but we keep using both\nvariables for compatibility with external scripts:\n\n  - scripts which check GIT_PAGER_IN_USE can continue to do\n    so, and will just ignore the new pipe-id variable.\n    Meaning they may accidentally turn on colors if their\n    output is redirected to a file, but that is the same as\n    today and we cannot fix that. We do not actively break\n    them from showing colors when their stdout _does_ go to\n    the pager.\n\n  - scripts which set GIT_PAGER_IN_USE but not\n    GIT_PAGER_PIPE_ID will continue to turn on colorization\n    for git sub-commands (again, they do not benefit from\n    the new code, but we are not making anything worse).\n\nThe inode-retrieval code itself is abstracted into compat/,\nas different platforms may represent the pipe id\ndifferently. These ids do not need to be portable across\nsystems, only within processes on the same system.\n\nNote that there is an existing test in t7006 which tests for\nthe exact _opposite_ of what we are trying to achieve\n(namely, that GIT_PAGER_IN_USE does _not_ cause us to write\ncolors to a random file). This test comes from a battery of\ntests added by 60b6e22 (tests: Add tests for automatic use\nof pager, 2010-02-19), and I think is simply misguided, as\nevidenced by the real \"git pull\" bug above. If you want to\nensure colors in a file, you do it with \"--color\", not by\npretending you have a pager.\n\nRather than delete the test, though, we simply re-title it\nhere. It actually makes a good check of the \"scripts which\nset PAGER_IN_USE but not PAGER_PIPE_ID\" historical\ncompatibility mentioned above.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n+cc Jonathan, whose test I am calling misguided above. :)\n\nI dug in the list and could not find anything interesting said about\nthis particular one. The motivation for adding the tests was a\nregression in git-svn, and we added a bunch of tests in response. I\ndoubt that you remember it well 5 years later, but it does not hurt to\ncheck.\n\n Makefile         |  1 +\n compat/pipe-id.c | 25 +++++++++++++++++++++++++\n compat/pipe-id.h | 27 +++++++++++++++++++++++++++\n pager.c          | 16 +++++++++++++++-\n t/t7006-pager.sh | 20 +++++++++++++++++++-\n 5 files changed, 87 insertions(+), 2 deletions(-)\n create mode 100644 compat/pipe-id.c\n create mode 100644 compat/pipe-id.h\n\ndiff --git a/Makefile b/Makefile\nindex 7efedbe..72f7b56 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -679,6 +679,7 @@ LIB_OBJS += column.o\n LIB_OBJS += combine-diff.o\n LIB_OBJS += commit.o\n LIB_OBJS += compat/obstack.o\n+LIB_OBJS += compat/pipe-id.o\n LIB_OBJS += compat/terminal.o\n LIB_OBJS += config.o\n LIB_OBJS += connect.o\ndiff --git a/compat/pipe-id.c b/compat/pipe-id.c\nnew file mode 100644\nindex 0000000..4764c5f\n--- /dev/null\n+++ b/compat/pipe-id.c\n@@ -0,0 +1,25 @@\n+#include \"git-compat-util.h\"\n+#include \"compat/pipe-id.h\"\n+#include \"strbuf.h\"\n+\n+const char *pipe_id_get(int fd)\n+{\n+\tstatic struct strbuf id = STRBUF_INIT;\n+\tstruct stat st;\n+\n+\tif (fstat(fd, &st) < 0 || !S_ISFIFO(st.st_mode))\n+\t\treturn NULL;\n+\n+\tstrbuf_reset(&id);\n+\tstrbuf_addf(&id, \"%lu\", (unsigned long)st.st_ino);\n+\treturn id.buf;\n+}\n+\n+int pipe_id_match(int fd, const char *id)\n+{\n+\tstruct stat st;\n+\n+\tif (fstat(fd, &st) < 0 || !S_ISFIFO(st.st_mode))\n+\t\treturn 0;\n+\treturn st.st_ino == strtoul(id, NULL, 10);\n+}\ndiff --git a/compat/pipe-id.h b/compat/pipe-id.h\nnew file mode 100644\nindex 0000000..5ddff2c\n--- /dev/null\n+++ b/compat/pipe-id.h\n@@ -0,0 +1,27 @@\n+#ifndef PIPE_ID_H\n+#define PIPE_ID_H\n+\n+/**\n+ * This module allows callers to save a string pipe identifier, and later find\n+ * out whether a file descriptor refers to the same pipe.\n+ *\n+ * The ids should be opaque to the callers, as their implementation may be\n+ * system dependent. The generated ids can be used between processes on the\n+ * same system, but are not portable between systems, or even between different\n+ * versions of git.\n+ */\n+\n+/**\n+ * Returns a string representing the pipe-id of the file descriptor `fd`, or\n+ * NULL if an error occurs. Note that the return value may be invalidated by\n+ * subsequent calls to pipe_id_get.\n+ */\n+const char *pipe_id_get(int fd);\n+\n+/**\n+ * Returns 1 if the pipe at `fd` matches the id `id`, or 0 otherwise (or if an\n+ * error occurs).\n+ */\n+int pipe_id_match(int fd, const char *id);\n+\n+#endif /* PIPE_ID_H */\ndiff --git a/pager.c b/pager.c\nindex e3ad465..de00def 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n #include \"run-command.h\"\n #include \"sigchain.h\"\n+#include \"compat/pipe-id.h\"\n \n #ifndef DEFAULT_PAGER\n #define DEFAULT_PAGER \"less\"\n@@ -57,6 +58,7 @@ const char *git_pager(int stdout_is_tty)\n void setup_pager(void)\n {\n \tconst char *pager = git_pager(isatty(1));\n+\tconst char *pipe_id;\n \n \tif (!pager)\n \t\treturn;\n@@ -88,6 +90,10 @@ void setup_pager(void)\n \t\tdup2(pager_process.in, 2);\n \tclose(pager_process.in);\n \n+\tpipe_id = pipe_id_get(1);\n+\tif (pipe_id)\n+\t\tsetenv(\"GIT_PAGER_PIPE_ID\", pipe_id, 1);\n+\n \t/* this makes sure that the parent terminates after the pager */\n \tsigchain_push_common(wait_for_pager_signal);\n \tatexit(wait_for_pager);\n@@ -95,7 +101,15 @@ void setup_pager(void)\n \n int pager_in_use(void)\n {\n-\treturn git_env_bool(\"GIT_PAGER_IN_USE\", 0);\n+\tconst char *pipe_id;\n+\n+\tif (!git_env_bool(\"GIT_PAGER_IN_USE\", 0))\n+\t\treturn 0;\n+\n+\tpipe_id = getenv(\"GIT_PAGER_PIPE_ID\");\n+\tif (!pipe_id) /* historical compatibility */\n+\t\treturn 1;\n+\treturn pipe_id_match(1, pipe_id);\n }\n \n /*\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 947b690..2f77299 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -158,7 +158,7 @@ test_expect_success TTY 'colors are suppressed by color.pager' '\n \t! colorful paginated.out\n '\n \n-test_expect_success 'color when writing to a file intended for a pager' '\n+test_expect_success 'color goes to files when GIT_PAGER_PIPE_ID is not used' '\n \trm -f colorful.log &&\n \ttest_config color.ui auto &&\n \t(\n@@ -177,6 +177,24 @@ test_expect_success TTY 'colors are sent to pager for external commands' '\n \tcolorful paginated.out\n '\n \n+test_expect_success TTY 'no color when paged program writes to file' '\n+\ttest_config alias.externallog \"!git log >log.out\" &&\n+\ttest_config color.ui auto &&\n+\ttest_terminal env TERM=vt100 git -p externallog &&\n+\ttest_line_count = 0 paginated.out &&\n+\ttest -s log.out &&\n+\t! colorful log.out\n+'\n+\n+test_expect_success TTY 'no color when paged program writes to pipe' '\n+\ttest_config alias.externallog \"!git log | cat >log.out\" &&\n+\ttest_config color.ui auto &&\n+\ttest_terminal env TERM=vt100 git -p externallog &&\n+\ttest_line_count = 0 paginated.out &&\n+\ttest -s log.out &&\n+\t! colorful log.out\n+'\n+\n # Use this helper to make it easy for the caller of your\n # terminal-using function to specify whether it should fail.\n # If you write\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267773","messageId":"98d092607588cb5c98e7a2deb2163f94@www.dscho.org","threadId":"40000","inReplyTo":"20150810052353.GB15441@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] pager_in_use: make sure output is still going to pager","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-08-10T16:38:10Z","receivedAt":"2015-08-10T16:38:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn 2015-08-10 07:23, Jeff King wrote:\n\n> diff --git a/compat/pipe-id.c b/compat/pipe-id.c\n> new file mode 100644\n> index 0000000..4764c5f\n> --- /dev/null\n> +++ b/compat/pipe-id.c\n> @@ -0,0 +1,25 @@\n> +#include \"git-compat-util.h\"\n> +#include \"compat/pipe-id.h\"\n> +#include \"strbuf.h\"\n> +\n> +const char *pipe_id_get(int fd)\n> +{\n> +\tstatic struct strbuf id = STRBUF_INIT;\n> +\tstruct stat st;\n> +\n> +\tif (fstat(fd, &st) < 0 || !S_ISFIFO(st.st_mode))\n> +\t\treturn NULL;\n\nJust 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):\n\n> +test_expect_success TTY 'no color when paged program writes to pipe' '\n> +\ttest_config alias.externallog \"!git log | cat >log.out\" &&\n> +\ttest_config color.ui auto &&\n> +\ttest_terminal env TERM=vt100 git -p externallog &&\n> +\ttest_line_count = 0 paginated.out &&\n> +\ttest -s log.out &&\n> +\t! colorful log.out\n> +'\n\nThe output is \"colorful\" ;-)\n\nI hope to find some time tomorrow to figure out some workaround that makes this work on Windows.\n\nCiao,\nDscho\n"},{"id":"267777","messageId":"20150810172448.GA20168@sigill.intra.peff.net","threadId":"40000","inReplyTo":"98d092607588cb5c98e7a2deb2163f94@www.dscho.org","subject":"Re: [PATCH 2/2] pager_in_use: make sure output is still going to pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T17:24:49Z","receivedAt":"2015-08-10T17:24:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 10, 2015 at 06:38:10PM +0200, Johannes Schindelin wrote:\n\n> > +const char *pipe_id_get(int fd)\n> > +{\n> > +\tstatic struct strbuf id = STRBUF_INIT;\n> > +\tstruct stat st;\n> > +\n> > +\tif (fstat(fd, &st) < 0 || !S_ISFIFO(st.st_mode))\n> > +\t\treturn NULL;\n> \n> Just a quick note: it seems that this check is not really working on\n> Windows. I tested this by running this test case manually (because TTY\n> is not set on Windows):\n\nYeah, I'm not too surprised. I'm guessing your st_ino for pipes are all\njust the same or something. Or maybe S_ISFIFO doesn't pass (we don't\ntechnically need it, I don't think, and could just drop that check).\n\nAnyway, I had planned that we would need to stick a big \"#ifdef WINDOWS\"\naround these two functions.\n\n> I hope to find some time tomorrow to figure out some workaround that\n> makes this work on Windows.\n\nCool. I can't comment on Windows-specific stuff, but I'm happy to review\nthe rest of it. :)\n\n-Peff\n"},{"id":"267815","messageId":"CAP=KgsSnHg_007oMXdHYQZQWwL4McUTS8ty4-MYe-=3QB8+d8Q@mail.gmail.com","threadId":"40000","inReplyTo":"20150810172448.GA20168@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] pager_in_use: make sure output is still going to pager","fromName":"Per Cederqvist","fromEmail":"cederp@opera.com","sentAt":"2015-08-11T07:48:50Z","receivedAt":"2015-08-11T07:48:50Z","isPatch":true,"sender":{"key":"cederp@opera.com","avatar":"https://gravatar.com/avatar/008fe28caedea0ba34042cd5157705267934f4ba7a9b29fb248ae18ec5b3055e?d=mp&s=160"},"body":"On Mon, Aug 10, 2015 at 7:24 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, Aug 10, 2015 at 06:38:10PM +0200, Johannes Schindelin wrote:\n>\n>> > +const char *pipe_id_get(int fd)\n>> > +{\n>> > +   static struct strbuf id = STRBUF_INIT;\n>> > +   struct stat st;\n>> > +\n>> > +   if (fstat(fd, &st) < 0 || !S_ISFIFO(st.st_mode))\n>> > +           return NULL;\n>>\n>> Just a quick note: it seems that this check is not really working on\n>> Windows. I tested this by running this test case manually (because TTY\n>> is not set on Windows):\n>\n> Yeah, I'm not too surprised. I'm guessing your st_ino for pipes are all\n> just the same or something. Or maybe S_ISFIFO doesn't pass (we don't\n> technically need it, I don't think, and could just drop that check).\n\nIf you remove the S_ISFIFO check, you probably need to include\nthe st_dev field in the pipe id.  Otherwise, you could be unlucky and\nredirect the output to a file that just happens to have the same inode\nnumber as the pager pipe. Remember, inode numbers are only unique\nwithin a certain st_dev.\n\n    /ceder\n"}]}