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

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Fri, Aug 09, 2019 at 07:36:14AM -0400, Jeff King wrote:
> 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).