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

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Aug 9, 2019, 11:25 UTC
Message-ID: <20190809112552.GM118825@genre.crustytoothpaste.net>
In-Reply-To: <20190809111452.GA93559@syl.local>

On 2019-08-09 at 11:14:52, Taylor Blau wrote:

Show 23 quoted lines

> > diff --git a/apply.c b/apply.c
> > index cde95369bb..b959b88b8e 100644
> > --- a/apply.c
> > +++ b/apply.c
> > @@ -4643,6 +4643,7 @@ static int apply_patch(struct apply_state *state,
> >  	struct patch *list = NULL, **listp = &list;
> >  	int skipped_patch = 0;
> >  	int res = 0;
> > +	int flush_attributes = 0;
> >
> >  	state->patch_input_file = filename;
> >  	if (read_patch_file(&buf, fd) < 0)
> > @@ -4670,6 +4671,14 @@ static int apply_patch(struct apply_state *state,
> >  			patch_stats(state, patch);
> >  			*listp = patch;
> >  			listp = &patch->next;
> > +
> > +			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.

Show 5 quoted lines

> But, I'm not sure if 'dummy' is the best name for this variable or not.
> My first thought was that I'd expect this to be named 'suffix' (or the
> less descriptive 'p'), but I don't know if the change is warranted.
> What *are* we supposed to call this variable?
> "path_after_gitattributes" ;-)?

I struggled with this variable name, as I'm sure you guessed. I could call it "has_suffix" or something similar. I have mixed feelings about naming pointer variables like booleans since it makes things prone to misreading, but I think that might be the sanest thing to do here. I can certainly do "p" as well.

I'll see if there are other comments about directions to take here, and then reroll.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204