On Thu, Aug 01, 2024 at 05:40:12PM +0200, Omar Polo wrote:
> This is mostly cosmetic.  it doesn't cause any practical harm on sha256
> repos.  Since we hardcode the sha1 digest length (which should be
> _STRING too...) we just truncate the object id in some cases, which in
> turns means we have to search for a matching object.
> 
> Since we don't know in this libexec the object format, and this is just
> an hint anyway[*], let's drop the pedantic check.
> 
> ok?

Not sure if we should simply delete this check. It does seem useful,
even if just to avoid sending an invalid object ID to callers of
this function.

What about trying to parse a sha2 hash first, and a sha1 hash if
sha2 failed to parse, and only fail the entire check if both of
these parsing attempts fail?

> [*]: i'm considering to change how the merge strategy works so that if
>      we fail to patch the provided blob, we retry using the local file
>      instead.  This will also fix some strange error messages like the
>      one Lucas hit by manually editing a patch.

Sure, seems unrelated to this parsing sanity check though.

> diff /home/op/w/got
> commit - d9048d75a8da25621fe5e1facf2b6923258311a3
> path + /home/op/w/got
> blob - 9b5c6b818d19c41eb3e9e4ffede13cd4a6fb81f5
> file + libexec/got-read-patch/got-read-patch.c
> --- libexec/got-read-patch/got-read-patch.c
> +++ libexec/got-read-patch/got-read-patch.c
> @@ -184,7 +184,6 @@ filexbit(const char *line)
>  static const struct got_error *
>  blobid(const char *line, char **blob, int git)
>  {
> -	uint8_t digest[SHA1_DIGEST_LENGTH];
>  	size_t len;
>  
>  	*blob = NULL;
> @@ -193,11 +192,6 @@ blobid(const char *line, char **blob, int git)
>  	if ((*blob = strndup(line, len)) == NULL)
>  		return got_error_from_errno("strndup");
>  
> -	if (!git && !got_parse_hash_digest(digest, *blob, GOT_HASH_SHA1)) {
> -		/* silently ignore invalid blob ids */
> -		free(*blob);
> -		*blob = NULL;
> -	}
>  	return NULL;
>  }
>  
> 
>