[PATCH 0/2] Allow difftool to be run outside of Git worktrees

[PATCH 0/2] Allow difftool to be run outside of Git worktrees

[Johannes Schindelin via GitGitGadget]

It was reported in https://github.com/git-for-windows/git/issues/2123 that 
git difftool --no-index fails to work outside worktrees, even if it should
work.

I fear this is a regression I introduced over two years ago (!) when I
converted the Perl script to C.

At least now that I know about the bug, I can fix it.

Johannes Schindelin (2):
  difftool: remove obsolete (and misleading) comment
  difftool: allow running outside Git worktrees with --no-index

 builtin/difftool.c  | 21 +++++++++++++++++----
 git.c               |  2 +-
 t/t7800-difftool.sh | 10 ++++++++++
 3 files changed, 28 insertions(+), 5 deletions(-)


base-commit: e902e9bcae2010bc42648c80ab6adc6c5a16a4a5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-163%2Fdscho%2Fdifftool-no-index-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-163/dscho/difftool-no-index-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/163
-- 
gitgitgadget

[PATCH 1/2] difftool: remove obsolete (and misleading) comment

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

We will always spawn something from `git difftool`, so we will always
have to set `GIT_DIR` and `GIT_WORK_TREE`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/difftool.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index a3ea60ea71..31eece0c8d 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -727,7 +727,6 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	if (tool_help)
 		return print_tool_help();
 
-	/* NEEDSWORK: once we no longer spawn anything, remove this */
 	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
 	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
 
-- 
gitgitgadget

[PATCH 2/2] difftool: allow running outside Git worktrees with --no-index

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

As far as this developer can tell, the conversion from a Perl script to
a built-in caused the regression in the difftool that it no longer runs
outside of a Git worktree (with `--no-index`, of course).

It is a bit embarrassing that it took over two years after retiring the
Perl version to discover this regression, but at least we now know, and
can do something, about it.

This fixes https://github.com/git-for-windows/git/issues/2123

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/difftool.c  | 20 +++++++++++++++++---
 git.c               |  2 +-
 t/t7800-difftool.sh | 10 ++++++++++
 3 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 31eece0c8d..d08ef31f04 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -690,7 +690,7 @@ static int run_file_diff(int prompt, const char *prefix,
 int cmd_difftool(int argc, const char **argv, const char *prefix)
 {
 	int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
-	    tool_help = 0;
+	    tool_help = 0, i, no_index = 0;
 	static char *difftool_cmd = NULL, *extcmd = NULL;
 	struct option builtin_difftool_options[] = {
 		OPT_BOOL('g', "gui", &use_gui_tool,
@@ -727,8 +727,22 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	if (tool_help)
 		return print_tool_help();
 
-	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
-	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
+	for (i = 0; i < argc; i++)
+		if (!strcmp(argv[i], "--"))
+			break;
+		else if (!strcmp(argv[i], "--no-index")) {
+			no_index = 1;
+			break;
+		}
+
+	if (!no_index && !startup_info->have_repository)
+		die(_("difftool requires worktree or --no-index"));
+
+	if (!no_index){
+		setup_work_tree();
+		setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
+		setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
+	}
 
 	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
 		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
diff --git a/git.c b/git.c
index 2014aab6b8..46365ed86a 100644
--- a/git.c
+++ b/git.c
@@ -491,7 +491,7 @@ static struct cmd_struct commands[] = {
 	{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },
 	{ "diff-tree", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT },
-	{ "difftool", cmd_difftool, RUN_SETUP | NEED_WORK_TREE },
+	{ "difftool", cmd_difftool, RUN_SETUP_GENTLY },
 	{ "fast-export", cmd_fast_export, RUN_SETUP },
 	{ "fetch", cmd_fetch, RUN_SETUP },
 	{ "fetch-pack", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT },
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index bb9a7f4ff9..4907627656 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -705,4 +705,14 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 	test_cmp expect actual
 '
 
+test_expect_success 'outside worktree' '
+	mkdir outside &&
+	echo 1 >outside/1 &&
+	echo 2 >outside/2 &&
+	test_expect_code 1 env GIT_CEILING_DIRECTORIES="$(pwd)" git \
+		-c diff.tool=echo -c difftool.echo.cmd="echo \$LOCAL \$REMOTE" \
+		-C outside difftool --no-prompt --no-index 1 2 >out &&
+	test "1 2" = "$(cat out)"
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH 2/2] difftool: allow running outside Git worktrees with --no-index

[Jeff King]

On Wed, Mar 13, 2019 at 12:20:14PM -0700, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> As far as this developer can tell, the conversion from a Perl script to
> a built-in caused the regression in the difftool that it no longer runs
> outside of a Git worktree (with `--no-index`, of course).
> 
> It is a bit embarrassing that it took over two years after retiring the
> Perl version to discover this regression, but at least we now know, and
> can do something, about it.

If a bug falls in the forest, does it make a sound?

I get the impression that `--no-index` is not used all that much, given
how long bugs seem to hang around in it (and doubly so when intersected
with the difftool).

> -	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
> -	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
> +	for (i = 0; i < argc; i++)
> +		if (!strcmp(argv[i], "--"))
> +			break;
> +		else if (!strcmp(argv[i], "--no-index")) {
> +			no_index = 1;
> +			break;
> +		}

Instead of this ad-hoc parsing, can we just add an OPT_BOOL("no-index")
to the parse-options array? We'll have already run parse_options() at
this point.

We'd just have to remember to add it back to the argv of diff
sub-commands we run.

> +	if (!no_index && !startup_info->have_repository)
> +		die(_("difftool requires worktree or --no-index"));
> +
> +	if (!no_index){
> +		setup_work_tree();
> +		setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
> +		setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
> +	}

This part makes sense.

There may be other subtle dependencies on having a repo from
sub-functions we run, but I didn't see any from a quick scan. And
anyway, if there is such a code path, it is no worse off than before
your patch (and in fact much better, because it would hopefully yield a
BUG() that would tell us what we need to fix).

> index 2014aab6b8..46365ed86a 100644
> --- a/git.c
> +++ b/git.c
> @@ -491,7 +491,7 @@ static struct cmd_struct commands[] = {
>  	{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
>  	{ "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },
>  	{ "diff-tree", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT },
> -	{ "difftool", cmd_difftool, RUN_SETUP | NEED_WORK_TREE },
> +	{ "difftool", cmd_difftool, RUN_SETUP_GENTLY },

We necessarily lost NEED_WORK_TREE, but that's OK because you added in a
setup_work_tree() in the function. Good.

> +test_expect_success 'outside worktree' '
> +	mkdir outside &&
> +	echo 1 >outside/1 &&
> +	echo 2 >outside/2 &&
> +	test_expect_code 1 env GIT_CEILING_DIRECTORIES="$(pwd)" git \
> +		-c diff.tool=echo -c difftool.echo.cmd="echo \$LOCAL \$REMOTE" \
> +		-C outside difftool --no-prompt --no-index 1 2 >out &&

We have a helper for running outside a repo. Because you have to set up
the "outside" space, it unfortunately doesn't shorten things as much as
it does in some other spots:

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 4907627656..255a787614 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -706,12 +706,12 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 '
 
 test_expect_success 'outside worktree' '
-	mkdir outside &&
-	echo 1 >outside/1 &&
-	echo 2 >outside/2 &&
-	test_expect_code 1 env GIT_CEILING_DIRECTORIES="$(pwd)" git \
+	mkdir non-repo &&
+	echo 1 >non-repo/1 &&
+	echo 2 >non-repo/2 &&
+	test_expect_code 1 nongit git \
 		-c diff.tool=echo -c difftool.echo.cmd="echo \$LOCAL \$REMOTE" \
-		-C outside difftool --no-prompt --no-index 1 2 >out &&
+		difftool --no-prompt --no-index 1 2 >out &&
 	test "1 2" = "$(cat out)"
 '
 

but it might be worth using anyway, just for consistency.

> +	test "1 2" = "$(cat out)"

A minor nit, but I think:

  echo "1 2" >expect &&
  test_cmp expect actual

produces nicer output on failure, and costs the same number of
processes (it is an extra file write, but I think the main driver of
performance in the test suite is just processes).

-Peff

Re: [PATCH 2/2] difftool: allow running outside Git worktrees with --no-index

[Johannes Schindelin]

Hi Peff,

On Wed, 13 Mar 2019, Jeff King wrote:

> On Wed, Mar 13, 2019 at 12:20:14PM -0700, Johannes Schindelin via
> GitGitGadget wrote:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > 
> > As far as this developer can tell, the conversion from a Perl script to
> > a built-in caused the regression in the difftool that it no longer runs
> > outside of a Git worktree (with `--no-index`, of course).
> > 
> > It is a bit embarrassing that it took over two years after retiring the
> > Perl version to discover this regression, but at least we now know, and
> > can do something, about it.
> 
> If a bug falls in the forest, does it make a sound?

I am glad you asked! Last time I checked, yes, it made a sound. It was a
really curious rattling sound. But I did not want to bother the bug again,
so I left it alone.

> I get the impression that `--no-index` is not used all that much, given
> how long bugs seem to hang around in it (and doubly so when intersected
> with the difftool).

Or maybe `--no-index` is used in pretty canonical ways. I, for one, used
to be a really heavy user of `--no-index` before `range-diff`, and it was
almost always with two files, sometimes with `--color-words`, sometimes
with `--patience`, sometimes both, but never anything crazy like using
Bash's `<(<command>)` syntax.

In other words, my take is that the ways in which `--no-index` is used are
probably not very different from one another, and the bugs lurk in
really rarely exercised code paths.

> > -	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
> > -	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
> > +	for (i = 0; i < argc; i++)
> > +		if (!strcmp(argv[i], "--"))
> > +			break;
> > +		else if (!strcmp(argv[i], "--no-index")) {
> > +			no_index = 1;
> > +			break;
> > +		}
> 
> Instead of this ad-hoc parsing, can we just add an OPT_BOOL("no-index")
> to the parse-options array? We'll have already run parse_options() at
> this point.
> 
> We'd just have to remember to add it back to the argv of diff
> sub-commands we run.

It was that "add it back" that I was not keen to implement.

But you gave me an idea: why not teach parse_options() to optionally keep
individual parsed arguments in `argv`?

And I was half-way finished with implementing that idea when I discovered
`OPT_ARGUMENT()`. This seemed to be *almost* what I needed: it puts the
argument immediately back into `argv`. However, it did not record that
fact, so I would not know whether it was part of the command-line or not.

So I was already done with implementing `OPT_ARGUMENT_SEEN()`, based on
`OPT_ARGUMENT()`, and testing it with my difftool patch, when it occurred
to me to look what existing users of `OPT_ARGUMENT()` do. Guess what:
there are none, apart from that test helper used in t0040 to verify that
`parse_options()` works as intended. And there were none other. In the
entire commit history.

In the end, I changed `OPT_ARGUMENT()`, and I find the end result rather
pleasing.

> > +	if (!no_index && !startup_info->have_repository)
> > +		die(_("difftool requires worktree or --no-index"));
> > +
> > +	if (!no_index){
> > +		setup_work_tree();
> > +		setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
> > +		setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
> > +	}
> 
> This part makes sense.
> 
> There may be other subtle dependencies on having a repo from
> sub-functions we run, but I didn't see any from a quick scan. And
> anyway, if there is such a code path, it is no worse off than before
> your patch (and in fact much better, because it would hopefully yield a
> BUG() that would tell us what we need to fix).

Indeed.

> > +test_expect_success 'outside worktree' '
> > +	mkdir outside &&
> > +	echo 1 >outside/1 &&
> > +	echo 2 >outside/2 &&
> > +	test_expect_code 1 env GIT_CEILING_DIRECTORIES="$(pwd)" git \
> > +		-c diff.tool=echo -c difftool.echo.cmd="echo \$LOCAL \$REMOTE" \
> > +		-C outside difftool --no-prompt --no-index 1 2 >out &&
> 
> We have a helper for running outside a repo. Because you have to set up
> the "outside" space, it unfortunately doesn't shorten things as much as
> it does in some other spots:
> 
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index 4907627656..255a787614 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -706,12 +706,12 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
>  '
>  
>  test_expect_success 'outside worktree' '
> -	mkdir outside &&
> -	echo 1 >outside/1 &&
> -	echo 2 >outside/2 &&
> -	test_expect_code 1 env GIT_CEILING_DIRECTORIES="$(pwd)" git \
> +	mkdir non-repo &&
> +	echo 1 >non-repo/1 &&
> +	echo 2 >non-repo/2 &&
> +	test_expect_code 1 nongit git \
>  		-c diff.tool=echo -c difftool.echo.cmd="echo \$LOCAL \$REMOTE" \
> -		-C outside difftool --no-prompt --no-index 1 2 >out &&
> +		difftool --no-prompt --no-index 1 2 >out &&
>  	test "1 2" = "$(cat out)"
>  '
>  
> 
> but it might be worth using anyway, just for consistency.

I totally agree. Thanks for pointing me to `nongit`; I was unaware of it.

And I was able to shorten it a bit, because the files `1` and `2` do not
need to live in that `non-repo` directory.

Again, a rather pleasing change.

> > +	test "1 2" = "$(cat out)"
> 
> A minor nit, but I think:
> 
>   echo "1 2" >expect &&
>   test_cmp expect actual
> 
> produces nicer output on failure, and costs the same number of
> processes (it is an extra file write, but I think the main driver of
> performance in the test suite is just processes).

You are totally right! After all, a regression test does not need to make
anything easy when it passes. It needs to make it easy to act when it
fails.

Thank you so much for your helpful suggestions!
Dscho

Re: [PATCH 2/2] difftool: allow running outside Git worktrees with --no-index

[Jeff King]

On Thu, Mar 14, 2019 at 01:16:45PM +0100, Johannes Schindelin wrote:

> In other words, my take is that the ways in which `--no-index` is used are
> probably not very different from one another, and the bugs lurk in
> really rarely exercised code paths.

Yeah, that's more likely (and consistent with the bugs I remember fixing
in the last few years).

> > We'd just have to remember to add it back to the argv of diff
> > sub-commands we run.
> 
> It was that "add it back" that I was not keen to implement.

Yes, I saw that we pass on the argv we get back from parse_options()
literally to the sub-functions. I was thinking you'd do some trickery
with an argv_array. But I like your approach of using OPT_ARGUMENT()
much better.

> So I was already done with implementing `OPT_ARGUMENT_SEEN()`, based on
> `OPT_ARGUMENT()`, and testing it with my difftool patch, when it occurred
> to me to look what existing users of `OPT_ARGUMENT()` do. Guess what:
> there are none, apart from that test helper used in t0040 to verify that
> `parse_options()` works as intended. And there were none other. In the
> entire commit history.

Heh. That is not the first time I have hit that with the parse-options
code.

I see you posted the new patches, so I'll try to give a careful read in
that part of the thread.

-Peff

[PATCH v2 0/3] Allow difftool to be run outside of Git worktrees

[Johannes Schindelin via GitGitGadget]

It was reported in https://github.com/git-for-windows/git/issues/2123 that 
git difftool --no-index fails to work outside worktrees, even if it should
work.

I fear this is a regression I introduced over two years ago (!) when I
converted the Perl script to C.

At least now that I know about the bug, I can fix it.

Changes since v1:

 * Instead of ad-hoc parsing to look for --no-index, the OPT_ARGUMENT() was
   enhanced and not has its first real user! After all those lonely, long
   years (11 years, 1 month and 11 days), what a wonderful thing to
   celebrate on π day.
 * The test now uses the nongit helper.
 * The test uses test_cmp to make diagnosing regressions easier.

Johannes Schindelin (3):
  difftool: remove obsolete (and misleading) comment
  parse-options: make OPT_ARGUMENT()  more useful
  difftool: allow running outside Git worktrees with --no-index

 Documentation/technical/api-parse-options.txt |  4 +++-
 builtin/difftool.c                            | 14 ++++++++++----
 git.c                                         |  2 +-
 parse-options.c                               |  2 ++
 parse-options.h                               |  4 ++--
 t/helper/test-parse-options.c                 |  2 +-
 t/t7800-difftool.sh                           | 10 ++++++++++
 7 files changed, 29 insertions(+), 9 deletions(-)


base-commit: e902e9bcae2010bc42648c80ab6adc6c5a16a4a5
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-163%2Fdscho%2Fdifftool-no-index-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-163/dscho/difftool-no-index-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/163

Range-diff vs v1:

 1:  2ad91f19c5 = 1:  2ad91f19c5 difftool: remove obsolete (and misleading) comment
 -:  ---------- > 2:  10775638ad parse-options: make OPT_ARGUMENT()  more useful
 2:  9f6eb60eee ! 3:  8cca9f800c difftool: allow running outside Git worktrees with --no-index
     @@ -22,24 +22,24 @@
       {
       	int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
      -	    tool_help = 0;
     -+	    tool_help = 0, i, no_index = 0;
     ++	    tool_help = 0, no_index = 0;
       	static char *difftool_cmd = NULL, *extcmd = NULL;
       	struct option builtin_difftool_options[] = {
       		OPT_BOOL('g', "gui", &use_gui_tool,
     +@@
     + 			    "tool returns a non - zero exit code")),
     + 		OPT_STRING('x', "extcmd", &extcmd, N_("command"),
     + 			   N_("specify a custom command for viewing diffs")),
     ++		OPT_ARGUMENT("no-index", &no_index, N_("passed to `diff`")),
     + 		OPT_END()
     + 	};
     + 
      @@
       	if (tool_help)
       		return print_tool_help();
       
      -	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
      -	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
     -+	for (i = 0; i < argc; i++)
     -+		if (!strcmp(argv[i], "--"))
     -+			break;
     -+		else if (!strcmp(argv[i], "--no-index")) {
     -+			no_index = 1;
     -+			break;
     -+		}
     -+
      +	if (!no_index && !startup_info->have_repository)
      +		die(_("difftool requires worktree or --no-index"));
      +
     @@ -73,13 +73,13 @@
       '
       
      +test_expect_success 'outside worktree' '
     -+	mkdir outside &&
     -+	echo 1 >outside/1 &&
     -+	echo 2 >outside/2 &&
     -+	test_expect_code 1 env GIT_CEILING_DIRECTORIES="$(pwd)" git \
     ++	echo 1 >1 &&
     ++	echo 2 >2 &&
     ++	test_expect_code 1 nongit git \
      +		-c diff.tool=echo -c difftool.echo.cmd="echo \$LOCAL \$REMOTE" \
     -+		-C outside difftool --no-prompt --no-index 1 2 >out &&
     -+	test "1 2" = "$(cat out)"
     ++		difftool --no-prompt --no-index ../1 ../2 >actual &&
     ++	echo "../1 ../2" >expect &&
     ++	test_cmp expect actual
      +'
      +
       test_done

-- 
gitgitgadget

[PATCH v2 1/3] difftool: remove obsolete (and misleading) comment

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

We will always spawn something from `git difftool`, so we will always
have to set `GIT_DIR` and `GIT_WORK_TREE`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/difftool.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index a3ea60ea71..31eece0c8d 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -727,7 +727,6 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	if (tool_help)
 		return print_tool_help();
 
-	/* NEEDSWORK: once we no longer spawn anything, remove this */
 	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
 	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
 
-- 
gitgitgadget

[PATCH v2 3/3] difftool: allow running outside Git worktrees with --no-index

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

As far as this developer can tell, the conversion from a Perl script to
a built-in caused the regression in the difftool that it no longer runs
outside of a Git worktree (with `--no-index`, of course).

It is a bit embarrassing that it took over two years after retiring the
Perl version to discover this regression, but at least we now know, and
can do something, about it.

This fixes https://github.com/git-for-windows/git/issues/2123

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/difftool.c  | 13 ++++++++++---
 git.c               |  2 +-
 t/t7800-difftool.sh | 10 ++++++++++
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 31eece0c8d..4fff1e83f9 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -690,7 +690,7 @@ static int run_file_diff(int prompt, const char *prefix,
 int cmd_difftool(int argc, const char **argv, const char *prefix)
 {
 	int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
-	    tool_help = 0;
+	    tool_help = 0, no_index = 0;
 	static char *difftool_cmd = NULL, *extcmd = NULL;
 	struct option builtin_difftool_options[] = {
 		OPT_BOOL('g', "gui", &use_gui_tool,
@@ -714,6 +714,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 			    "tool returns a non - zero exit code")),
 		OPT_STRING('x', "extcmd", &extcmd, N_("command"),
 			   N_("specify a custom command for viewing diffs")),
+		OPT_ARGUMENT("no-index", &no_index, N_("passed to `diff`")),
 		OPT_END()
 	};
 
@@ -727,8 +728,14 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 	if (tool_help)
 		return print_tool_help();
 
-	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
-	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
+	if (!no_index && !startup_info->have_repository)
+		die(_("difftool requires worktree or --no-index"));
+
+	if (!no_index){
+		setup_work_tree();
+		setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
+		setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
+	}
 
 	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
 		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
diff --git a/git.c b/git.c
index 2014aab6b8..46365ed86a 100644
--- a/git.c
+++ b/git.c
@@ -491,7 +491,7 @@ static struct cmd_struct commands[] = {
 	{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },
 	{ "diff-tree", cmd_diff_tree, RUN_SETUP | NO_PARSEOPT },
-	{ "difftool", cmd_difftool, RUN_SETUP | NEED_WORK_TREE },
+	{ "difftool", cmd_difftool, RUN_SETUP_GENTLY },
 	{ "fast-export", cmd_fast_export, RUN_SETUP },
 	{ "fetch", cmd_fetch, RUN_SETUP },
 	{ "fetch-pack", cmd_fetch_pack, RUN_SETUP | NO_PARSEOPT },
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index bb9a7f4ff9..480dd0633f 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -705,4 +705,14 @@ test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' '
 	test_cmp expect actual
 '
 
+test_expect_success 'outside worktree' '
+	echo 1 >1 &&
+	echo 2 >2 &&
+	test_expect_code 1 nongit git \
+		-c diff.tool=echo -c difftool.echo.cmd="echo \$LOCAL \$REMOTE" \
+		difftool --no-prompt --no-index ../1 ../2 >actual &&
+	echo "../1 ../2" >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
gitgitgadget

Re: [PATCH v2 3/3] difftool: allow running outside Git worktrees with --no-index

[Jeff King]

On Thu, Mar 14, 2019 at 04:25:04AM -0700, Johannes Schindelin via GitGitGadget wrote:

> @@ -714,6 +714,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
>  			    "tool returns a non - zero exit code")),
>  		OPT_STRING('x', "extcmd", &extcmd, N_("command"),
>  			   N_("specify a custom command for viewing diffs")),
> +		OPT_ARGUMENT("no-index", &no_index, N_("passed to `diff`")),
>  		OPT_END()
>  	};

Much nicer.

> +test_expect_success 'outside worktree' '
> +	echo 1 >1 &&
> +	echo 2 >2 &&
> +	test_expect_code 1 nongit git \
> +		-c diff.tool=echo -c difftool.echo.cmd="echo \$LOCAL \$REMOTE" \
> +		difftool --no-prompt --no-index ../1 ../2 >actual &&
> +	echo "../1 ../2" >expect &&
> +	test_cmp expect actual
> +'

And this fixed all of my nits from the previous version. The whole
series looks good to me.

-Peff

Re: [PATCH v2 3/3] difftool: allow running outside Git worktrees with --no-index

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 966 bytes --]

Hi Peff,

On Thu, 14 Mar 2019, Jeff King wrote:

> On Thu, Mar 14, 2019 at 04:25:04AM -0700, Johannes Schindelin via GitGitGadget wrote:
> 
> > @@ -714,6 +714,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
> >  			    "tool returns a non - zero exit code")),
> >  		OPT_STRING('x', "extcmd", &extcmd, N_("command"),
> >  			   N_("specify a custom command for viewing diffs")),
> > +		OPT_ARGUMENT("no-index", &no_index, N_("passed to `diff`")),
> >  		OPT_END()
> >  	};
> 
> Much nicer.
> 
> > +test_expect_success 'outside worktree' '
> > +	echo 1 >1 &&
> > +	echo 2 >2 &&
> > +	test_expect_code 1 nongit git \
> > +		-c diff.tool=echo -c difftool.echo.cmd="echo \$LOCAL \$REMOTE" \
> > +		difftool --no-prompt --no-index ../1 ../2 >actual &&
> > +	echo "../1 ../2" >expect &&
> > +	test_cmp expect actual
> > +'
> 
> And this fixed all of my nits from the previous version. The whole
> series looks good to me.

Thanks! (စ ͜ စ)
Dscho

[PATCH v2 2/3] parse-options: make OPT_ARGUMENT() more useful

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

`OPT_ARGUMENT()` is intended to keep the specified long option in `argv`
and not to do anything else.

However, it would make a lot of sense for the caller to know whether
this option was seen at all or not. For example, we want to teach `git
difftool` to work outside of any Git worktree, but only when
`--no-index` was specified.

Note: nothing in Git uses OPT_ARGUMENT(). Even worse, looking through
the commit history, one can easily see that nothing even
ever used it, apart from the regression test.

So not only do we make `OPT_ARGUMENT()` more useful, we are also about
to introduce its first real user!

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/technical/api-parse-options.txt | 4 +++-
 parse-options.c                               | 2 ++
 parse-options.h                               | 4 ++--
 t/helper/test-parse-options.c                 | 2 +-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 2b036d7838..2e2e7c10c6 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -198,8 +198,10 @@ There are some macros to easily define options:
 	The filename will be prefixed by passing the filename along with
 	the prefix argument of `parse_options()` to `prefix_filename()`.
 
-`OPT_ARGUMENT(long, description)`::
+`OPT_ARGUMENT(long, &int_var, description)`::
 	Introduce a long-option argument that will be kept in `argv[]`.
+	If this option was seen, `int_var` will be set to one (except
+	if a `NULL` pointer was passed).
 
 `OPT_NUMBER_CALLBACK(&var, description, func_ptr)`::
 	Recognize numerical options like -123 and feed the integer as
diff --git a/parse-options.c b/parse-options.c
index cec74522e5..1d57802da0 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -286,6 +286,8 @@ static enum parse_opt_result parse_long_opt(
 					     optname(options, flags));
 			if (*rest)
 				continue;
+			if (options->value)
+				*(int *)options->value = options->defval;
 			p->out[p->cpidx++] = arg - 2;
 			return PARSE_OPT_DONE;
 		}
diff --git a/parse-options.h b/parse-options.h
index 7d83e2971d..c3d45ba1ac 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -138,8 +138,8 @@ struct option {
 	{ OPTION_CALLBACK, (s), (l), (v), (a), (h), (f), (cb) }
 
 #define OPT_END()                   { OPTION_END }
-#define OPT_ARGUMENT(l, h)          { OPTION_ARGUMENT, 0, (l), NULL, NULL, \
-				      (h), PARSE_OPT_NOARG}
+#define OPT_ARGUMENT(l, v, h)       { OPTION_ARGUMENT, 0, (l), (v), NULL, \
+				      (h), PARSE_OPT_NOARG, NULL, 1 }
 #define OPT_GROUP(h)                { OPTION_GROUP, 0, NULL, NULL, NULL, (h) }
 #define OPT_BIT(s, l, v, h, b)      OPT_BIT_F(s, l, v, h, b, 0)
 #define OPT_BITOP(s, l, v, h, set, clear) { OPTION_BITOP, (s), (l), (v), NULL, (h), \
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index cc88fba057..2232b2f79e 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -132,7 +132,7 @@ int cmd__parse_options(int argc, const char **argv)
 		OPT_NOOP_NOARG(0, "obsolete"),
 		OPT_STRING_LIST(0, "list", &list, "str", "add str to list"),
 		OPT_GROUP("Magic arguments"),
-		OPT_ARGUMENT("quux", "means --quux"),
+		OPT_ARGUMENT("quux", NULL, "means --quux"),
 		OPT_NUMBER_CALLBACK(&integer, "set integer to NUM",
 			number_callback),
 		{ OPTION_COUNTUP, '+', NULL, &boolean, NULL, "same as -b",
-- 
gitgitgadget

Re: [PATCH v2 2/3] parse-options: make OPT_ARGUMENT() more useful

[Jeff King]

On Thu, Mar 14, 2019 at 04:25:04AM -0700, Johannes Schindelin via GitGitGadget wrote:

> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
> index 2b036d7838..2e2e7c10c6 100644
> --- a/Documentation/technical/api-parse-options.txt
> +++ b/Documentation/technical/api-parse-options.txt
> @@ -198,8 +198,10 @@ There are some macros to easily define options:
>  	The filename will be prefixed by passing the filename along with
>  	the prefix argument of `parse_options()` to `prefix_filename()`.
>  
> -`OPT_ARGUMENT(long, description)`::
> +`OPT_ARGUMENT(long, &int_var, description)`::
>  	Introduce a long-option argument that will be kept in `argv[]`.
> +	If this option was seen, `int_var` will be set to one (except
> +	if a `NULL` pointer was passed).

So this effectively makes it into a "bool" that we keep. I think that's
fine. It always uses NOARG, so it is not like we would ever need to see
"we got --foo, and this is the argument it had".

I did wonder if it was possible for "--no-foo" to trigger this (leaving
the caller who looks at the int unsure if they saw "--foo" or
"--no-foo"), but it seems that the parse-options code checks for
OPTION_ARGUMENT before it ever looks at negation.

Curiously, it also checks it before doing the usual prefix-matching
magic. So you could otherwise say "--no-inde", but OPT_ARGUMENT() will
not allow it. I think that's probably sane and not worth thinking
further about, but it is an interesting quirk that a user could possibly
run into.

> diff --git a/parse-options.c b/parse-options.c
> index cec74522e5..1d57802da0 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -286,6 +286,8 @@ static enum parse_opt_result parse_long_opt(
>  					     optname(options, flags));
>  			if (*rest)
>  				continue;
> +			if (options->value)
> +				*(int *)options->value = options->defval;

Cute. You could actually assign any defval you like, though of course
the convenient OPT_ARGUMENT() macro just always uses 1.

I wondered if you might need another cast for defval itself, but it's an
intptr_t (so it's the types that use it as a string that need to cast to
"const char *").

This looks very clean overall, and I agree it's much nicer than the
alternatives for your use case.

-Peff

Re: [PATCH v2 2/3] parse-options: make OPT_ARGUMENT() more useful

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2659 bytes --]

Hi Peff,

On Thu, 14 Mar 2019, Jeff King wrote:

> On Thu, Mar 14, 2019 at 04:25:04AM -0700, Johannes Schindelin via GitGitGadget wrote:
> 
> > diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
> > index 2b036d7838..2e2e7c10c6 100644
> > --- a/Documentation/technical/api-parse-options.txt
> > +++ b/Documentation/technical/api-parse-options.txt
> > @@ -198,8 +198,10 @@ There are some macros to easily define options:
> >  	The filename will be prefixed by passing the filename along with
> >  	the prefix argument of `parse_options()` to `prefix_filename()`.
> >  
> > -`OPT_ARGUMENT(long, description)`::
> > +`OPT_ARGUMENT(long, &int_var, description)`::
> >  	Introduce a long-option argument that will be kept in `argv[]`.
> > +	If this option was seen, `int_var` will be set to one (except
> > +	if a `NULL` pointer was passed).
> 
> So this effectively makes it into a "bool" that we keep.

Almost. It is only a "seen" in the sense that you cannot re-set it to
`false`.

But yeah, the difference is subtle.

> I think that's fine. It always uses NOARG, so it is not like we would
> ever need to see "we got --foo, and this is the argument it had".

Yep, I checked that.

> I did wonder if it was possible for "--no-foo" to trigger this (leaving
> the caller who looks at the int unsure if they saw "--foo" or
> "--no-foo"), but it seems that the parse-options code checks for
> OPTION_ARGUMENT before it ever looks at negation.

I checked that, too.

> Curiously, it also checks it before doing the usual prefix-matching
> magic. So you could otherwise say "--no-inde", but OPT_ARGUMENT() will
> not allow it. I think that's probably sane and not worth thinking
> further about, but it is an interesting quirk that a user could possibly
> run into.

I missed that... ¯\_(ツ)_/¯

> > diff --git a/parse-options.c b/parse-options.c
> > index cec74522e5..1d57802da0 100644
> > --- a/parse-options.c
> > +++ b/parse-options.c
> > @@ -286,6 +286,8 @@ static enum parse_opt_result parse_long_opt(
> >  					     optname(options, flags));
> >  			if (*rest)
> >  				continue;
> > +			if (options->value)
> > +				*(int *)options->value = options->defval;
> 
> Cute. You could actually assign any defval you like, though of course
> the convenient OPT_ARGUMENT() macro just always uses 1.
> 
> I wondered if you might need another cast for defval itself, but it's an
> intptr_t (so it's the types that use it as a string that need to cast to
> "const char *").

Exactly.

> This looks very clean overall, and I agree it's much nicer than the
> alternatives for your use case.

Thank you! 😊
Dscho

Re: [PATCH v2 2/3] parse-options: make OPT_ARGUMENT() more useful

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +`OPT_ARGUMENT(long, &int_var, description)`::
>>  	Introduce a long-option argument that will be kept in `argv[]`.
>> +	If this option was seen, `int_var` will be set to one (except
>> +	if a `NULL` pointer was passed).
>
> So this effectively makes it into a "bool" that we keep. I think that's
> fine. It always uses NOARG, so it is not like we would ever need to see
> "we got --foo, and this is the argument it had".
>
> I did wonder if it was possible for "--no-foo" to trigger this (leaving
> the caller who looks at the int unsure if they saw "--foo" or
> "--no-foo"), but it seems that the parse-options code checks for
> OPTION_ARGUMENT before it ever looks at negation.

When a caller that needs to tell --no-foo and lack of any foo
related option arises, we should be able to update the function
further so that the caller can initialize the variable to -1
(unspecified) and make sure that 0 is left upon seeing --no-foo
so it's not a show stopper, I guess.

Re: [PATCH v2 2/3] parse-options: make OPT_ARGUMENT() more useful

[Jeff King]

On Mon, Mar 18, 2019 at 11:47:20AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> +`OPT_ARGUMENT(long, &int_var, description)`::
> >>  	Introduce a long-option argument that will be kept in `argv[]`.
> >> +	If this option was seen, `int_var` will be set to one (except
> >> +	if a `NULL` pointer was passed).
> >
> > So this effectively makes it into a "bool" that we keep. I think that's
> > fine. It always uses NOARG, so it is not like we would ever need to see
> > "we got --foo, and this is the argument it had".
> >
> > I did wonder if it was possible for "--no-foo" to trigger this (leaving
> > the caller who looks at the int unsure if they saw "--foo" or
> > "--no-foo"), but it seems that the parse-options code checks for
> > OPTION_ARGUMENT before it ever looks at negation.
> 
> When a caller that needs to tell --no-foo and lack of any foo
> related option arises, we should be able to update the function
> further so that the caller can initialize the variable to -1
> (unspecified) and make sure that 0 is left upon seeing --no-foo
> so it's not a show stopper, I guess.

The way it is written, I think the intent is that you would do:

  OPT_ARGUMENT("foo", &saw_foo, ...),
  OPT_ARGUMENT("no-foo", &saw_no_foo, ...),

I'm happy to punt on it until it ever comes up (which I suspect may be
never).

-Peff

Re: [PATCH v2 2/3] parse-options: make OPT_ARGUMENT() more useful

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The way it is written, I think the intent is that you would do:
>
>   OPT_ARGUMENT("foo", &saw_foo, ...),
>   OPT_ARGUMENT("no-foo", &saw_no_foo, ...),
>
> I'm happy to punt on it until it ever comes up (which I suspect may be
> never).

Yeah, having to have one extra element like the above may be ugly
and less optimal API design, but something that is survivable, and
when we actually need to use it, find it too ugly to live, and are
motivated to fix it, it will be fixed, so I won't be too worried
about it, either.