On 2019-08-09 at 11:14:52, Taylor Blau wrote:
> > 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.
> 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
Attachment:
signature.asc
Description: PGP signature
