Re: [PATCH] apply: reload .gitattributes after patching it

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Aug 9, 2019, 12:43 UTC
Message-ID: <20190809124318.GJ20404@szeder.dev>
In-Reply-To: <20190809113613.GA3957@sigill.intra.peff.net>

On Fri, Aug 09, 2019 at 07:36:14AM -0400, Jeff King wrote:

Show 25 quoted lines

> On Fri, Aug 09, 2019 at 11:25:52AM +0000, brian m. carlson wrote:
> 
> > > > +			if (!flush_attributes && patch->new_name) {
> > > > +				char *dummy = strip_path_suffix(patch->new_name, GITATTRIBUTES_FILE);
> > > 
> > > It's a shame that 'strip_path_suffix' doesn't take a 'char *out', and
> > > accept NULL for that (which would save us the assignment and subsequent
> > > 'free'). In either case, this is certainly the appropriate function.
> > 
> > Yeah, I felt the same way. We could refactor this out into two separate
> > functions, one which returns an ssize_t and one which actually
> > allocates, but I'm not sure it makes a huge amount of sense with just
> > one caller. The allocation is relatively small, and I've tried to make
> > sure it's called exactly once per patch so as not to be wasteful and
> > inefficient.
> 
> I think you could do this with:
> 
>   size_t len;
>   if (strip_suffix(patch->new_name, GITATTRIBUTES_FILE, &len) &&
>       len > 0 && is_dir_sep(patch->new_name[len-1]))
>           flush_attributes = 1;
> 
> Not sure if that is better or worse. It avoids the extra memory
> boilerplate, but the logic is a slightly more subtle.

Subtle indeed :) because you have to allow len == 0 to catch the toplevel .gitattributes file.

But there is an other subtlety here: when I read the commit message saying "patch that touches a path ending in ".gitattributes"." and saw the new call to strip_path_suffix(), I immediately thought what would happen with a file called 'foo.gitattributes'. Only when I looked into strip_path_suffix() became it clear that it only removes full path components, so such a filename won't cause any trouble (though perhaps the worst thing that could happen is that we unnecessarily flush the attributes cache).