Re: [PATCH] t7800-difftool: don't accidentally match tmp dirs

From: Junio C Hamano <gitster@pobox.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] t7800-difftool: don't accidentally match tmp dirs
Date: Fri, 08 Jan 2021 12:10:00 -0800	[thread overview]
Message-ID: <xmqq1revi413.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20210108092036.GR8396@szeder.dev> ("SZEDER =?utf-8?Q?G=C3=A1?= =?utf-8?Q?bor=22's?= message of "Fri, 8 Jan 2021 10:20:36 +0100")

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Wed, Jan 06, 2021 at 10:24:27PM -0800, Junio C Hamano wrote:
>> SZEDER Gábor <szeder.dev@gmail.com> writes:
>> 
>> > diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
>> > index a578b35761..fe02fe1688 100755
>> > --- a/t/t7800-difftool.sh
>> > +++ b/t/t7800-difftool.sh
>> > @@ -439,73 +439,104 @@ run_dir_diff_test () {
>> >  }
>> >  
>> >  run_dir_diff_test 'difftool -d' '
>> > +	cat >expect <<-\EOF &&
>> > +	file
>> > +	file2
>> > +
>> > +	file
>> > +	file2
>> > +	sub
>> > +	EOF
>> >  	git difftool -d $symlinks --extcmd ls branch >output &&
>> > -	grep sub output &&
>> > -	grep file output
>> > +	grep -v ^/ output >actual &&
>> 
>> This unfortunately would not catch full paths on certain platforms.
>> 
>> See https://github.com/git/git/runs/1660588243?check_suite_focus=true#step:7:4186
>> for an example X-<.
>
> Hrm, one has to log in to view those CI logs?  Really?! :(

Yup, it sucks.  I am curious (but not strongly interested enough to
demand) to learn the reason why from GitHub folks.

> Anyway, I (apparently falsely) assumed that the output these tests
> look at come from Git itself, and therefore we can rely on difftool's
> temporary directories being normalized UNIX-style absolute paths...
> But it seems they don't actually come from Git but from 'ls', because
> that's what those '--extcmd ls' options do, and I now going to assume
> that 'ls' prints those absolute paths with drive letter prefixes and
> whatnot on Windows.
>
> The initial version of this patch just tightened all potentially
> problematic 'grep' patterns, e.g. 'grep ^sub$ output && grep ^file$
> output'.  That should work on Windows as well, shouldn't it.  Will see
> whether I can dig it out from the reflogs.

I only see 'file', 'file2', 'b', 'c', etc. used in the tests; do we
ever use any path that ends with a colon?

I wonder if would it be more robust to use something like

	sed -e 's|^.*/\([a-z]*\)/:$|/directory-path-\1/:|'

in place of "grep -v /" to redact the temporary directory names
difftool creates.  It would give you /directory-path-left/: or
/directory-path-right/: instead of removing these lines and it would
clarify what these two series of file names are, which may be an
added bonus.