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

From: Jeff King <peff@peff.net>
To: "brian m. carlson" <sandals@crustytoothpaste.net>
Cc: Taylor Blau <me@ttaylorr.com>,
	git@vger.kernel.org, Stefan Beller <stefanbeller@gmail.com>,
	Duy Nguyen <pclouds@gmail.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] apply: reload .gitattributes after patching it
Date: Fri, 9 Aug 2019 07:36:14 -0400	[thread overview]
Message-ID: <20190809113613.GA3957@sigill.intra.peff.net> (raw)
In-Reply-To: <20190809112552.GM118825@genre.crustytoothpaste.net>

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.

-Peff