Re: [PATCH 2/2] difftool: allow running outside Git worktrees with --no-index
Hi Peff,
On Wed, 13 Mar 2019, Jeff King wrote:
Show 14 quoted lines
> 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.
Show 16 quoted lines
> > - 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.
Show 16 quoted lines
> > + 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).Show 36 quoted lines
> > +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.
Show 10 quoted lines
> > + 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
Johannes Schindelin via GitGitGadget
