`git log` or `git show` does not work when setting pager.<cmd> in version 2.11.0(2) or later #1077

hattya · 2017-03-01T12:35:15Z

I was not able to find an open or closed issue matching what I'm seeing

Setup

Which version of Git for Windows are you using? Is it 32-bit or 64-bit?

$ git --version --build-options
git version 2.11.0.windows.1
sizeof-long: 4
machine: x86_64

Which version of Windows are you running? Vista, 7, 8, 10? Is it 32-bit or 64-bit?

$ cmd.exe /c ver

Microsoft Windows [Version 10.0.14393]

Details

What commands did you run to trigger this issue? If you can provide a
Minimal, Complete, and Verifiable example
this will help us understand the issue.

$ git config --global pager.diff 'diff-highlight | less'
$ git config --global pager.log 'diff-highlight | less'
$ git config --global pager.show 'diff-highlight | less'
$ git diff
$ git log
diff-highlight | less: No such file or directory
$ git show
diff-highlight | less: No such file or directory

The issue is introduced in commit 382b41b, and it seems that it is appeared by commit a9b8a09.

Following patch fixes the issue:

--- a/git.c
+++ b/git.c
@@ -633,10 +633,10 @@ static int run_argv(int *argcp, const char ***argv)
                                argv_array_push(&args, (*argv)[i]);

                        if (get_super_prefix())
-                               die("%s doesn't support --super-prefix", args.argv[0]);
+                               die("%s doesn't support --super-prefix", args.argv[1]);

                        if (use_pager == -1)
-                               use_pager = check_pager_config(args.argv[0]);
+                               use_pager = check_pager_config(args.argv[1]);
                        commit_pager_choice();

                        trace_argv_printf(args.argv, "trace: exec:");

The text was updated successfully, but these errors were encountered:

dscho · 2017-03-02T11:37:16Z

Following patch fixes the issue:

Does it really? Have you verified that args.argv[1] is the command (not the first parameter)?

diff-highlight | less: No such file or directory

This indicates to me that the pager.<cmd> value is interpreted as path, not as command-line. From the documentation, it is not clear that it was ever meant to refer to a command-line instead of a path...

However, from the source code it appears as if the value is actually passed to the shell, so it is a bit puzzling that it is not interpreted correctly.

dscho · 2017-03-02T14:05:57Z

it is a bit puzzling that it is not interpreted correctly.

The reason is that a special "we have arguments" handling kicks in because in the Windows case apparently not only argv[0] is set to the pager value, but also argv[1]. This is a bug. I am continuing to investigate.

dscho · 2017-03-02T14:18:24Z

Hah. The problem is that isatty(1) returns non-zero even after the pager was started and stdout redirected.

We newly handle isatty() by special-casing the stdin/stdout/stderr file descriptors, caching the return value. However, we missed the case where dup2() overrides the respective file descriptor. That poses a problem e.g. where the `show` builtin asks for a pager very early, the `setup_pager()` function sets the pager depending on the return value of `isatty()` and then redirects stdout. Subsequently, `cmd_log_init_finish()` calls `setup_pager()` *again*. What should happen now is that `isatty()` reports that stdout is *not* a TTY and consequently stdout should be left alone. Let's override dup2() to handle this appropriately. This fixes #1077 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

hattya · 2017-03-03T09:20:33Z

Thank you for the fix!

I think this patch should also be applied, it is a fix for commit 382b41b.

It seems that commit 382b41b is copied from execv_dashed_external() without updating the index. In this scope, args.argv[0] is git and args.argv[1] (equals argv[0]) is a builtin command. Because get_builtin() returns non-NULL.

dscho · 2017-03-03T16:06:29Z

@hattya Uh oh. I must have been real tired when I looked at your patch. You are absolutely correct! Thanks!

dscho · 2017-03-03T16:18:27Z

BTW the code in question should only kick in after an alias was expanded. So I think you would need something like

git -c pager.log 'diff-highlight | less' -c  alias.lg=log lg -1 -p

to trigger that bug.

When copy-pasting from execv_dashed_external(), the array index was left unchanged even if we now have an array with `git` shifted in. Instead of adjusting the array index, simply use the original array. While at it, also reorder the loop until after the potential early exit. Pointed out by Akinori Hattori in #1077 (comment) Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho · 2017-03-03T16:21:20Z

I chose to apply a slightly different patch, which also made the calls conciser and reordered to allow an early die() without having to initialize the args first.

hattya · 2017-03-05T04:48:15Z

Sorry, I thought I tested with git log and git show, but their aliases...

We newly handle isatty() by special-casing the stdin/stdout/stderr file descriptors, caching the return value. However, we missed the case where dup2() overrides the respective file descriptor. That poses a problem e.g. where the `show` builtin asks for a pager very early, the `setup_pager()` function sets the pager depending on the return value of `isatty()` and then redirects stdout. Subsequently, `cmd_log_init_finish()` calls `setup_pager()` *again*. What should happen now is that `isatty()` reports that stdout is *not* a TTY and consequently stdout should be left alone. Let's override dup2() to handle this appropriately. This fixes #1077 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

We newly handle isatty() by special-casing the stdin/stdout/stderr file descriptors, caching the return value. However, we missed the case where dup2() overrides the respective file descriptor. That poses a problem e.g. where the `show` builtin asks for a pager very early, the `setup_pager()` function sets the pager depending on the return value of `isatty()` and then redirects stdout. Subsequently, `cmd_log_init_finish()` calls `setup_pager()` *again*. What should happen now is that `isatty()` reports that stdout is *not* a TTY and consequently stdout should be left alone. Let's override dup2() to handle this appropriately. This fixes git-for-windows#1077 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>