git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Jeff King <peff@peff.net>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 2/2] difftool: allow running outside Git worktrees with --no-index
Date: Thu, 14 Mar 2019 13:16:45 +0100 (STD)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1903141305550.41@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20190313204644.GA5397@sigill.intra.peff.net>

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

  reply	other threads:[~2019-03-14 12:17 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-13 19:20 [PATCH 0/2] Allow difftool to be run outside of Git worktrees Johannes Schindelin via GitGitGadget
2019-03-13 19:20 ` [PATCH 1/2] difftool: remove obsolete (and misleading) comment Johannes Schindelin via GitGitGadget
2019-03-13 19:20 ` [PATCH 2/2] difftool: allow running outside Git worktrees with --no-index Johannes Schindelin via GitGitGadget
2019-03-13 20:46   ` Jeff King
2019-03-14 12:16     ` Johannes Schindelin [this message]
2019-03-15  3:08       ` Jeff King
2019-03-14 11:25 ` [PATCH v2 0/3] Allow difftool to be run outside of Git worktrees Johannes Schindelin via GitGitGadget
2019-03-14 11:25   ` [PATCH v2 1/3] difftool: remove obsolete (and misleading) comment Johannes Schindelin via GitGitGadget
2019-03-14 11:25   ` [PATCH v2 3/3] difftool: allow running outside Git worktrees with --no-index Johannes Schindelin via GitGitGadget
2019-03-15  3:17     ` Jeff King
2019-03-15 13:20       ` Johannes Schindelin
2019-03-14 11:25   ` [PATCH v2 2/3] parse-options: make OPT_ARGUMENT() more useful Johannes Schindelin via GitGitGadget
2019-03-15  3:15     ` Jeff King
2019-03-15 13:24       ` Johannes Schindelin
2019-03-18  2:47       ` Junio C Hamano
2019-03-18 21:04         ` Jeff King
2019-03-19  0:25           ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.QRO.7.76.6.1903141305550.41@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).