Re: [PATCH 5/7] sha1-file: pass git_hash_algo to write_object_file_prepare()

From: Jeff King <peff@peff.net>
Date: Feb 1, 2020, 11:08 UTC
Message-ID: <20200201110836.GB1864948@coredump.intra.peff.net>
In-Reply-To: <20200201100337.u4ikzmrprca4ltvb@tb-raspi4>

On Sat, Feb 01, 2020 at 11:03:37AM +0100, Torsten Bögershausen wrote:

Show 21 quoted lines

> > diff --git a/sha1-file.c b/sha1-file.c
> > index 13b90b1cb1..e789bfd153 100644
> > --- a/sha1-file.c
> > +++ b/sha1-file.c
> > @@ -1647,7 +1647,8 @@ void *read_object_with_reference(struct repository *r,
> >  	}
> >  }
> >
> > -static void write_object_file_prepare(const void *buf, unsigned long len,
> > +static void write_object_file_prepare(const struct git_hash_algo *algo,
> > +				      const void *buf, unsigned long len,
> >  				      const char *type, struct object_id *oid,
> >  				      char *hdr, int *hdrlen)
> >  {
> 
> One minor minor suggestion/nit, may be my own type of style only.
> 
> When adding a parameter to a function, we typically add it at the end of
> the parameter list, rather than at the beginning.
> The other (unwritten) convention, when dealing with "buffer pointer/len",
> seams to be that buffer-ptr is the first paramter and len is is the second.

I'd usually add new options at the end, too, all things being equal. But in this case, I see:

  - some of these are input parameters and some are output (I think oid,
    hdr, and hdrlen are the latter); it makes sense to keep them grouped
    that way

  - in object-oriented functions, we'd often take the object first
    (e.g., strbuf_add). In non-object-oriented functions, we often take
    a "context" argument first that shapes how the function works (e.g.,
    all of the pp_* functions that take a pretty_print_context). I'd
    think of the algorithm a little bit as a "context" argument, though
    I admit it's mostly a gut feeling and quite subjective.

    But if you believe that line of reasoning, then it doesn't seem out
    of place as the first argument.

-Peff