Re: [PATCH 1/1] diff: munmap() file contents before running external diff

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Mon, 8 Jul 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > When running an external diff from, say, a diff tool, it is safe to
> > assume that we want to write the files in question. On Windows, that
> > means that there cannot be any other process holding an open handle to
> > said files.
>
> Please add "It is not enough to close the file descriptor; having a
> region that is still mmapped keeps the file busy" or something like
> that at the end.

Good call.

> > So let's make sure that `git diff` itself is not holding any open handle
> > to the files in question.
> >
> > This fixes https://github.com/git-for-windows/git/issues/1315
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  diff.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/diff.c b/diff.c
> > index 4d3cf83a27..0afb76bbca 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -4206,6 +4206,10 @@ static void run_external_diff(const char *pgm,
> >  	argv_array_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter);
> >  	argv_array_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr);
> >
> > +	if (one && one->should_munmap)
> > +		diff_free_filespec_data(one);
> > +	if (two && two->should_munmap)
> > +		diff_free_filespec_data(two);
>
> I wondered if a single diff_filespec instance can be used in two
> diff_filepair instances (e.g. file A is in-place modified and also
> used to create file C), and if so after showing the diff for file A,
> we have problems with showing file C.  But I do not think it should
> pose a problem, as "free data after comparing a pair" is what we do
> for the in-core codepath in builtin_diff().

Precisely.

> We can lose the NULL-ness test for one and two if these "free the
> resource once we no longer need it" is done inside "if (one && two)".
> After all, once add_external_diff_name()[*1*] does its thing, we do
> not need the data for these diff_filespec instances, right?

Yes, but we still need the `should_munmap` test, I believe. So we have
that `if` anyway.

> Also, just like builtin_diff() unconditionally frees the resources
> held by diff_filespec instances, shouldn't this function do so, even
> the ones that are not marked with should_munmap?

I have not inspected the code path vigorously, nor am I confident that
this wouldn't be broken easily. At least when regions are mapped, I am
fairly certain that my patch does not break anything.

But if you are confident that this won't break anything, I'll certainly
be happy with that assessment.

I'll make that change and trust the CI build to fail if your assumption
was incorrect.

Ciao,
Dscho

>
>
> >  	if (run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv))
> >  		die(_("external diff died, stopping at %s"), name);
>