Allow difftool to be run outside of Git worktrees
PATCH 2/2 difftool: allow running outside Git worktrees with --no-index
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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
--- builtin/difftool.c | 20 +++++++++++++++++--- git.c | 2 +- t/t7800-difftool.sh | 10 ++++++++++ 3 files changed, 28 insertions(+), 4 deletions(-)
builtin/difftool.c +17 -3
@@ -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 );
git.c +1 -1
@@ -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 },
t/t7800-difftool.sh +10 -0
@@ -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
Signature · 1 line
gitgitgadget
cover Allow difftool to be run outside of Git worktrees
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
Signature · 1 line
gitgitgadget
PATCH 1/2 difftool: remove obsolete (and misleading) comment
From: Johannes Schindelin <[email protected]>
We will always spawn something from git difftool, so we will always have to set GIT_DIR and GIT_WORK_TREE.
--- builtin/difftool.c | 1 - 1 file changed, 1 deletion(-)
builtin/difftool.c +0 -1
@@ -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 );
Signature · 1 line
gitgitgadget
Re: [PATCH 2/2] difftool: allow running outside Git worktrees with --no-index
In reply to Johannes Schindelin via GitGitGadgetQuoted 9 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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).
Review
Quoted 9 lines
(quoted diff) +7
-2- 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.
Review
Quoted 8 lines
(quoted diff) +8
-0+ 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).
git.c static struct cmd_struct commands[] = { · line 494
Quoted 9 lines
index
2014aab6b8..46365ed86a100644git.c +1
-1@@ -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.
git.c static struct cmd_struct commands[] = {
Quoted 7 lines
git.c +7
-0+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:
t/t7800-difftool.sh +5 -5
@@ -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.
git.c
Quoted 1 line
git.c +1
-0+ 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
v2 cover Allow difftool to be run outside of Git worktrees
In reply to Johannes Schindelin via GitGitGadgetIt 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_doneSignature · 1 line
gitgitgadget
PATCH v2 1/3 difftool: remove obsolete (and misleading) comment
From: Johannes Schindelin <[email protected]>
We will always spawn something from git difftool, so we will always have to set GIT_DIR and GIT_WORK_TREE.
--- builtin/difftool.c | 1 - 1 file changed, 1 deletion(-)
builtin/difftool.c +0 -1
@@ -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 );
Signature · 1 line
gitgitgadget
PATCH v2 2/3 parse-options: make OPT_ARGUMENT() more useful
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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!
--- 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(-)
Documentation/technical/api-parse-options.txt +3 -1
@@ -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
parse-options.c +2 -0
@@ -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 ; }
parse-options.h +2 -2
@@ -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), \
t/helper/test-parse-options.c +1 -1
@@ -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",
Signature · 1 line
gitgitgadget
PATCH v2 3/3 difftool: allow running outside Git worktrees with --no-index
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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
--- builtin/difftool.c | 13 ++++++++++--- git.c | 2 +- t/t7800-difftool.sh | 10 ++++++++++ 3 files changed, 21 insertions(+), 4 deletions(-)
builtin/difftool.c +10 -3
@@ -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 );
git.c +1 -1
@@ -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 },
t/t7800-difftool.sh +10 -0
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH 2/2] difftool: allow running outside Git worktrees with --no-index
In reply to Jeff KingHi Peff,
Quoted 9 lines · Jeff King
Quoted 6 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
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).
Quoted 5 lines
Quoted 3 lines
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.
Quoted 3 lines
I get the impression that
--no-indexis 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.
Quoted 14 lines
Review
Quoted 9 lines
(quoted diff) +7
-2- 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 runparse_options()at this point.
Quoted 2 lines
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.
Quoted 11 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+ 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.
Quoted 5 lines
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.
Quoted 35 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+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:
t/t7800-difftool.sh +5
-5@@ -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)" '
Quoted 1 line
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.
Quoted 10 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ test "1 2" = "$(cat out)"A minor nit, but I think:
echo "1 2" >expect &&
test_cmpexpect actualproduces 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
Quoted 3 lines · Johannes Schindelin
In other words, my take is that the ways in which
--no-indexis 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).
Quoted 4 lines
Quoted 2 lines
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.
Quoted 6 lines
So I was already done with implementing
OPT_ARGUMENT_SEEN(), based onOPT_ARGUMENT(), and testing it with my difftool patch, when it occurred to me to look what existing users ofOPT_ARGUMENT()do. Guess what:
there are none, apart from that test helper used in t0040 to verify thatparse_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
Re: [PATCH v2 2/3] parse-options: make OPT_ARGUMENT() more useful
In reply to Johannes Schindelin via GitGitGadgetDocumentation/technical/api-parse-options.txt There are some macros to easily define options: · line 204
Quoted 13 lines · Johannes Schindelin via GitGitGadget
Documentation/technical/api-parse-options.txt +3
-1@@ -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.
parse-options.c static enum parse_opt_result parse_long_opt( · line 290
Quoted 10 lines
parse-options.c +2
-0@@ -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 3/3] difftool: allow running outside Git worktrees with --no-index
In reply to Johannes Schindelin via GitGitGadgetbuiltin/difftool.c int cmd_difftool(int argc, const char **argv, const char *prefix) · line 719
Quoted 7 lines · Johannes Schindelin via GitGitGadget
builtin/difftool.c +1
-0@@ -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.
t/t7800-difftool.sh test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' ' · line 716
Quoted 9 lines
t/t7800-difftool.sh +9
-0+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
Hi Peff,
Quoted 12 lines · Jeff King
builtin/difftool.c int cmd_difftool(int argc, const char **argv, const char *prefix) · line 719
Quoted 7 lines · Johannes Schindelin via GitGitGadget
builtin/difftool.c +1
-0@@ -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.
Quoted 12 lines
t/t7800-difftool.sh test_expect_success SYMLINKS 'difftool --dir-diff handles modified symlinks' ' · line 716
Quoted 9 lines
t/t7800-difftool.sh +9
-0+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
Re: [PATCH v2 2/3] parse-options: make OPT_ARGUMENT() more useful
In reply to Jeff KingHi Peff,
Quoted 10 lines · Jeff King
Quoted 8 lines · Johannes Schindelin via GitGitGadget
Documentation/technical/api-parse-options.txt +0
-0@@ -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()`.
Quoted 7 lines
Documentation/technical/api-parse-options.txt There are some macros to easily define options: · line 204
Quoted 5 lines
Documentation/technical/api-parse-options.txt +3
-1-`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.
Quoted 2 lines
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.
Quoted 4 lines
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_ARGUMENTbefore it ever looks at negation.
I checked that, too.
Quoted 5 lines
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... ¯\_(ツ)_/¯
Quoted 14 lines
parse-options.c static enum parse_opt_result parse_long_opt( · line 290
Quoted 10 lines
parse-options.c +2
-0@@ -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.
Quoted 3 lines
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.
Quoted 2 lines
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
In reply to Jeff KingQuoted 9 lines · Jeff King
Documentation/technical/api-parse-options.txt There are some macros to easily define options: · line 204
Quoted 4 lines
+
OPT_ARGUMENT(long, &int_var, description)::Documentation/technical/api-parse-options.txt +2
-0Introduce 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".
Quoted 4 lines
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_ARGUMENTbefore 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
Quoted 16 lines · Junio C Hamano
Quoted 9 lines · Jeff King
Documentation/technical/api-parse-options.txt There are some macros to easily define options: · line 204
Quoted 4 lines
+
OPT_ARGUMENT(long, &int_var, description)::Documentation/technical/api-parse-options.txt +2
-0Introduce 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".
Quoted 4 lines
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_ARGUMENTbefore it ever looks at negation.
Quoted 5 lines
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
Quoted 7 lines · Jeff King
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.