Re: [PATCH] t3706: Add test for wildcard vs literal pathspec

From: JAYATHEERTH K <jayatheerthkulkarni2005@gmail.com>
Date: Apr 14, 2025, 17:08 UTC
Message-ID: <CA+rGoLc=CYehMms0bxaLAu3xP-p62cMJ++RnBxSMOF2Ppt38QQ@mail.gmail.com>
In-Reply-To: <FFC5C6BE-9A4B-4EFD-AEC4-E69CE6610054@gmail.com>

On Mon, Apr 14, 2025 at 10:21 PM Lucas Seiki Oshiro <lucasseikioshiro@gmail.com> wrote:

>
> Hi!
>

Hi Lucas,

Show 38 quoted lines

> > +test_expect_success 'setup: create files and initial commit' '
> > +    mkdir testdir &&
> > +    >testdir/f\* &&
> > +    >testdir/f\*\* &&
> > +    >testdir/foo &&
> > +    git add testdir &&
> > +    git commit -m "Initial setup with literal wildcard files"
> > +'
> > +
> > +test_expect_success 'clean slate before testing wildcard behavior' '
> > +    git rm -rf testdir &&
> > +    git commit -m "Clean state"
> > +'
> >
> > +test_expect_success 'recreate files to test add behavior' '
> > +    mkdir testdir &&
> > +    >testdir/f\* &&
> > +    >testdir/f\*\* &&
> > +    >testdir/foo
> > +'
>
>
> Two questions:
>
> 1. Does this need to be inside a test_expect_success? It seems to me
>    that those two tests cases are actually setup code for the next
>    two.
>
> 2. If so, does it need to have all that setup? I could reproduce the
>    bug by only running:
>
>    ```
>    git reset
>    touch foo 'f*' 'f**'
>    git add 'f*'
>    git ls-files
>    ```
>

Umm, I think the setup should just be a single block. I will send a patch on this I'm parallely working on a second patch.

>    btw, this works with your code, congrats!
>

Thank you, credit goes to Peff(Jeff King) I almost lost track.

> Other idea: `?` is another wildcard for matching only one character.
> Have you tested if the same bug happens with it?
>

Yup I think that's also a great suggestion, I think consolidating setup and adding at least a few different wildcards will be good.

Show 6 quoted lines

> PS: while I was writing this review I pushed this to my GitHub just
> to make the CI run the entire test suite since pathspecs are a
> sensible part of Git.
>
> Take look at this, it seems that your tests aren't passing on Windows:
> https://github.com/lucasoshiro/git/actions/runs/14450183624/job/40521015897.

Oh damn!! That's a silly mistake. I almost forgot windows exist!! Thanks for letting me know

> Perhaps you'll need to change something there. It seems to be
> related to how Windows handle paths (specially the \ character, which
> means the same as / in Unix). Personally, I'm not a Windows guy and

Same not a windows guy, but I will have to read some things out. Will figure it out.

Show 6 quoted lines

> can't help you further with this. A quick reference on how paths on
> Windows work is this (and yeah, they are far more complex than in
> Unix):
> https://www.fileside.app/blog/2023-03-17_windows-file-paths/
>
>

Thanks again Lucas, these help.

-Jayatheerth