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@xxxxxx>
> >
> > 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
