Re: [PATCH] t7800-difftool: don't accidentally match tmp dirs
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 8, 2021, 20:10 UTC
- Message-ID
- <xmqq1revi413.fsf@gitster.c.googlers.com>
- In-Reply-To
- <20210108092036.GR8396@szeder.dev>
SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 30 quoted lines
> 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.
Show 12 quoted lines
> 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.