Re: [PATCH 5/7] t0060: test obscured .gitattributes and .gitignore matching

From: Jeff King <peff@peff.net>
Date: Oct 5, 2020, 08:40 UTC
Message-ID: <20201005084051.GD2862927@coredump.intra.peff.net>
In-Reply-To: <20201005080353.GH1166820@google.com>

On Mon, Oct 05, 2020 at 01:03:53AM -0700, Jonathan Nieder wrote:

Show 6 quoted lines

> > Note that the ntfs magic prefix names in the tests come from the
> > algorithm described in e7cb0b4455 (and are different for each file).
> 
> Doesn't block this patch, but I'm curious: how hard would it be to make
> a test with an NTFS prerequisite that makes sure we got the magic prefix
> right?

I suspect hard since Dscho punted on it in the original series. :) If I understand correctly, it would require having an NTFS filesystem, and generating 10,000+ files with a clashing prefix.

Show 11 quoted lines

> > +	for (; *argv; argv++) {
> > +		if (!strcmp("--not", *argv))
> > +			expect = !expect;
> > +		else if (expect != (is_hfs(*argv) || is_ntfs(*argv)))
> > +			 res = error("'%s' is %s.%s", *argv,
> > +				     expect ? "not " : "", x);
> > +		else
> > +			fprintf(stderr, "ok: '%s' is %s.%s\n",
> > +				*argv, expect ? "" : "not ", x);
> 
> micronit: extra space on the "res" line.

Thanks, fixed.

> This "if" cascade is a little hard to read, even though it does the
> right thing.  Can we make it more explicit?  E.g.

This is directly moved from the existing code. I'd prefer to keep the overall structure intact to make that clear.

> I think it's a little easier to read with either (a) the dot included
> in the 'x' parameter or (b) the entire ".git" missing from the 'x'
> parameter.

Yeah, I agree that's worth doing. I took (b), as "dotgitx" implies that "x" is "modules", etc. I had originally planned to automatically turn "gitmodules" into "is_ntfs_dotgitmodules", too, but it required macros and string-pasting. So I decided it was a bit too ugly. :)

-Peff