[5/7] sha1-file: pass git_hash_algo to write_object_file_prepare()
diff mbox series

Message ID f256d2058736091c3d9662788762849e8df794b0.1580413221.git.matheus.bernardino@usp.br
State New
Headers show
Series
  • fix inconsistent uses of the_repo in parse_object()'s call chain
Related show

Commit Message

Matheus Tavares Bernardino Jan. 30, 2020, 8:32 p.m. UTC
Allow write_object_file_prepare() to receive arbitrary 'struct
git_hash_algo's instead of always using the_hash_algo. The added
parameter will be used in the next commit to make hash_object_file() be
able to work with arbitrary git_hash_algo's, as well.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 sha1-file.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Comments

Torsten Bögershausen Feb. 1, 2020, 10:03 a.m. UTC | #1
On Thu, Jan 30, 2020 at 05:32:21PM -0300, Matheus Tavares wrote:
> Allow write_object_file_prepare() to receive arbitrary 'struct
> git_hash_algo's instead of always using the_hash_algo. The added
> parameter will be used in the next commit to make hash_object_file() be
> able to work with arbitrary git_hash_algo's, as well.
>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
>  sha1-file.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
>
> 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.

However, this is my personal note, other opinions and comments are welcome.
What do others think ?
Jeff King Feb. 1, 2020, 11:08 a.m. UTC | #2
On Sat, Feb 01, 2020 at 11:03:37AM +0100, Torsten Bögershausen wrote:

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

Patch
diff mbox series

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)
 {
@@ -1657,10 +1658,10 @@  static void write_object_file_prepare(const void *buf, unsigned long len,
 	*hdrlen = xsnprintf(hdr, *hdrlen, "%s %"PRIuMAX , type, (uintmax_t)len)+1;
 
 	/* Sha1.. */
-	the_hash_algo->init_fn(&c);
-	the_hash_algo->update_fn(&c, hdr, *hdrlen);
-	the_hash_algo->update_fn(&c, buf, len);
-	the_hash_algo->final_fn(oid->hash, &c);
+	algo->init_fn(&c);
+	algo->update_fn(&c, hdr, *hdrlen);
+	algo->update_fn(&c, buf, len);
+	algo->final_fn(oid->hash, &c);
 }
 
 /*
@@ -1718,7 +1719,8 @@  int hash_object_file(const void *buf, unsigned long len, const char *type,
 {
 	char hdr[MAX_HEADER_LEN];
 	int hdrlen = sizeof(hdr);
-	write_object_file_prepare(buf, len, type, oid, hdr, &hdrlen);
+	write_object_file_prepare(the_hash_algo, buf, len, type, oid, hdr,
+				  &hdrlen);
 	return 0;
 }
 
@@ -1876,7 +1878,8 @@  int write_object_file(const void *buf, unsigned long len, const char *type,
 	/* Normally if we have it in the pack then we do not bother writing
 	 * it out into .git/objects/??/?{38} file.
 	 */
-	write_object_file_prepare(buf, len, type, oid, hdr, &hdrlen);
+	write_object_file_prepare(the_hash_algo, buf, len, type, oid, hdr,
+				  &hdrlen);
 	if (freshen_packed_object(oid) || freshen_loose_object(oid))
 		return 0;
 	return write_loose_object(oid, hdr, hdrlen, buf, len, 0);
@@ -1892,7 +1895,8 @@  int hash_object_file_literally(const void *buf, unsigned long len,
 	/* type string, SP, %lu of the length plus NUL must fit this */
 	hdrlen = strlen(type) + MAX_HEADER_LEN;
 	header = xmalloc(hdrlen);
-	write_object_file_prepare(buf, len, type, oid, header, &hdrlen);
+	write_object_file_prepare(the_hash_algo, buf, len, type, oid, header,
+				  &hdrlen);
 
 	if (!(flags & HASH_WRITE_OBJECT))
 		goto cleanup;