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

From: Jeff King <peff@peff.net>
Date: Aug 9, 2019, 13:51 UTC
Message-ID: <20190809135148.GA7495@sigill.intra.peff.net>
In-Reply-To: <20190809124318.GJ20404@szeder.dev>

On Fri, Aug 09, 2019 at 02:43:18PM +0200, SZEDER Gábor wrote:

Show 12 quoted lines

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

I'll pretend I was leaving that as a puzzle for the readers. :)

Show 8 quoted lines

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

Right. I think the term we want here is really "basename". So in fact:

  if (!strcmp(basename(patch->new_name), GITATTRIBUTES_FILE))

would do what we want, except for the annoying caveat that basename() is allowed to modify its parameter (to remove trailing directory separators, but we know we wouldn't have them here).

-Peff