On Fri, Feb 03, 2023 at 01:42:07PM +0100, Omar Polo wrote:
> On 2023/02/03 12:44:04 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> > On Fri, Feb 03, 2023 at 11:52:20AM +0100, Omar Polo wrote:
> > > add a function that's analogous to got_object_id_str but writes to the
> > > given buffer.  at the moment is just a wrapper around got_sha1_digest_to_str
> > > but will gain some more logic in the future.
> > > 
> > > i've avoided all the calls to got_sha1_digest_to_str that were in the
> > > pack code, the network code and gotd.  There are a few calls to build
> > > a GOT_ERR_OBJ_CSUM error that i'm going to deal with in a follow-up
> > > diff.  The two calls in the patch code were kept too, will revisit
> > > them when we'll start to have some real initial sha256 support.
> > > 
> > > diffstat -s /home/op/w/got
> > >  M  include/got_object.h  |  7+  0-
> > 
> > Does the function need to be declared in exactly this headers?
> > 
> > Would lib/got_lib_object.h work, too? This would reduce visibility.
> > Code outside lib libexec (and now, gotd) isn't really supposed
> > to be using headers from lib/. For new functions it would be better
> > to start out with limited visibility if possible.
> 
> sure; i went with got_object.h only because it's where
> got_object_id_str was defined.
> 
> > >  M  lib/error.c           |  1+  1-
> > >  M  lib/object.c          |  1+  1-
> > >  M  lib/object_parse.c    |  7+  1-
> > > 
> > > 4 files changed, 16 insertions(+), 3 deletions(-)
> > > 
> > > diff -s /home/op/w/got
> > > commit - 09cbb981df84d9c12f0fa371cb6b85d3f1b615c0
> > > path + /home/op/w/got (staged changes)
> > > blob - 9d11c8f4f518f5f911295671d3aafcb494a7ec10
> > > blob + 3dea2df3656c85f425a01b9e66471da1fc10fdaf
> > > --- include/got_object.h
> > > +++ include/got_object.h
> > > @@ -82,6 +82,13 @@ const struct got_error *got_object_id_str(char **, str
> > >  const struct got_error *got_object_id_str(char **, struct got_object_id *);
> > >  
> > >  /*
> > > + * Write the string representation of an object ID in the given buffer.
> > > + * The output depends on the hash function used by the repository format
> > > + * (currently SHA1).
> > > + */
> > > +char *got_object_id_serialize(struct got_object_id *, char *, size_t);
> > 
> > "serialize" sounds as if it was creating a bytes string representation.
> > I would suggest using a different term.
> > For example: got_object_id_to_hex(struct got_object_id *, char *, size_t);
> 
> i didn't like serialize neither; thanks for the suggestion!
> 
> > There should be a constant which callers could use to create a
> > buffer of an appropriate size. For now, we could define it like:
> > 
> > #define GOT_OBJECT_ID_HEX_MAXLEN SHA1_DIGEST_LENGTH
> 
> eheh, this was my next step but why don't do it now?  I've also
> tweaked the callers of got_object_blob_id_str.
> 
> > and adjust this constant later as needed. The docs could say:
> > 
> >  * Write the string representation of an object ID in the given buffer.
> >  * This buffer must be at least GOT_OBJECT_ID_HEX_MAXLEN bytes in size.
> >  * The output depends on the hash function used by the repository format.
> > 
> >  And I would drop (currently SHA1). We don't need to document this
> >  because callers shouldn't have to care.
> 
> agreed!

Thanks, I like this much better. ok

> diff -s /home/op/w/got
> commit - 09cbb981df84d9c12f0fa371cb6b85d3f1b615c0
> path + /home/op/w/got (staged changes)
> blob - 9d11c8f4f518f5f911295671d3aafcb494a7ec10
> blob + 6a161bc20df5aaa7789395593ab2ffc6fbe4b1fb
> --- include/got_object.h
> +++ include/got_object.h
> @@ -14,6 +14,8 @@ struct got_object_id {
>   * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>   */
>  
> +#define GOT_OBJECT_ID_HEX_MAXLEN SHA1_DIGEST_STRING_LENGTH
> +
>  struct got_object_id {
>  	u_int8_t sha1[SHA1_DIGEST_LENGTH];
>  };
> blob - 3be41277d7a85a185d21bb7faa43bebe3b8eae07
> blob + 26ad0c1921d76c50b48885cce2fa226b137793d7
> --- lib/diff.c
> +++ lib/diff.c
> @@ -136,8 +136,8 @@ diff_blobs(struct got_diff_line **lines, size_t *nline
>      enum got_diff_algorithm diff_algo)
>  {
>  	const struct got_error *err = NULL, *free_err;
> -	char hex1[SHA1_DIGEST_STRING_LENGTH];
> -	char hex2[SHA1_DIGEST_STRING_LENGTH];
> +	char hex1[GOT_OBJECT_ID_HEX_MAXLEN];
> +	char hex2[GOT_OBJECT_ID_HEX_MAXLEN];
>  	const char *idstr1 = NULL, *idstr2 = NULL;
>  	char *modestr1 = NULL, *modestr2 = NULL;
>  	off_t size1, size2;
> @@ -344,7 +344,7 @@ diff_blob_file(struct got_diffreg_result **resultp,
>      int force_text_diff, struct got_diffstat_cb_arg *diffstat, FILE *outfile)
>  {
>  	const struct got_error *err = NULL, *free_err;
> -	char hex1[SHA1_DIGEST_STRING_LENGTH];
> +	char hex1[GOT_OBJECT_ID_HEX_MAXLEN];
>  	const char *idstr1 = NULL;
>  	struct got_diffreg_result *result = NULL;
>  
> blob - 3c84ba7eeb9c88175a7652495f934263b6fd2328
> blob + 95d7ae9cb011916744442c02343dafa9e4901bb1
> --- lib/error.c
> +++ lib/error.c
> @@ -32,7 +32,7 @@
>  #include "got_lib_delta.h"
>  #include "got_lib_inflate.h"
>  #include "got_lib_object.h"
> -#include "got_lib_sha1.h"
> +#include "got_lib_object_parse.h"
>  
>  #ifndef nitems
>  #define nitems(_a) (sizeof(_a) / sizeof((_a)[0]))
> @@ -367,11 +367,11 @@ got_error_no_obj(struct got_object_id *id)
>  const struct got_error *
>  got_error_no_obj(struct got_object_id *id)
>  {
> -	char msg[sizeof("object   not found") + SHA1_DIGEST_STRING_LENGTH];
> -	char id_str[SHA1_DIGEST_STRING_LENGTH];
> +	char id_str[GOT_OBJECT_ID_HEX_MAXLEN];
> +	char msg[sizeof("object   not found") + sizeof(id_str)];
>  	int ret;
>  
> -	if (!got_sha1_digest_to_str(id->sha1, id_str, sizeof(id_str)))
> +	if (!got_object_id_hex(id, id_str, sizeof(id_str)))
>  		return got_error(GOT_ERR_NO_OBJ);
>  
>  	ret = snprintf(msg, sizeof(msg), "object %s not found", id_str);
> blob - 37def8a54e88bcdfbc49e4cc2f7a41fd8175f3b5
> blob + 4b96e21f407d84ac399adbccb5d7e21cf86dad1f
> --- lib/got_lib_object_parse.h
> +++ lib/got_lib_object_parse.h
> @@ -16,6 +16,13 @@ const struct got_error *got_object_qid_alloc_partial(s
>  
>  struct got_pathlist_head;
>  
> +/*
> + * Write the string representation fo an object ID in the given buffer.
> + * This buffer must be at least GOT_OBJECT_ID_HEX_MAXLEN bytes in size.
> + * The output depneds on the hash function used by the repository format.
> + */
> +char *got_object_id_hex(struct got_object_id *, char *, size_t);
> +
>  const struct got_error *got_object_qid_alloc_partial(struct got_object_qid **);
>  struct got_commit_object *got_object_commit_alloc_partial(void);
>  struct got_tree_entry *got_alloc_tree_entry_partial(void);
> blob - 52e7c967c9cc041ec94d6baa8ab706eb7434b845
> blob + 57876d8fb763d7736c0cb4fbb6967718793d2cea
> --- lib/object.c
> +++ lib/object.c
> @@ -374,7 +374,7 @@ got_object_blob_id_str(struct got_blob_object *blob, c
>  char *
>  got_object_blob_id_str(struct got_blob_object *blob, char *buf, size_t size)
>  {
> -	return got_sha1_digest_to_str(blob->id.sha1, buf, size);
> +	return got_object_id_hex(&blob->id, buf, size);
>  }
>  
>  size_t
> blob - c9a711f7c6066bf89187c9058634f85a5a61c2e1
> blob + b17d5a9b339c525ac55deed48e604e2b049d96ca
> --- lib/object_parse.c
> +++ lib/object_parse.c
> @@ -88,13 +88,13 @@ got_object_id_str(char **outbuf, struct got_object_id 
>  const struct got_error *
>  got_object_id_str(char **outbuf, struct got_object_id *id)
>  {
> -	static const size_t len = SHA1_DIGEST_STRING_LENGTH;
> +	static const size_t len = GOT_OBJECT_ID_HEX_MAXLEN;
>  
>  	*outbuf = malloc(len);
>  	if (*outbuf == NULL)
>  		return got_error_from_errno("malloc");
>  
> -	if (got_sha1_digest_to_str(id->sha1, *outbuf, len) == NULL) {
> +	if (got_object_id_hex(id, *outbuf, len) == NULL) {
>  		free(*outbuf);
>  		*outbuf = NULL;
>  		return got_error(GOT_ERR_BAD_OBJ_ID_STR);
> @@ -103,6 +103,12 @@ void
>  	return NULL;
>  }
>  
> +char *
> +got_object_id_hex(struct got_object_id *id, char *buf, size_t len)
> +{
> +	return got_sha1_digest_to_str(id->sha1, buf, len);
> +}
> +
>  void
>  got_object_close(struct got_object *obj)
>  {
>