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).
