git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 2/6] Accept object data in the fsck_object() function

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 28, 2014, 20:47 UTC
Message-ID
<xmqqvbpc8hon.fsf@gitster.dls.corp.google.com>
In-Reply-To
<alpine.DEB.1.00.1408281646400.990@s15462909.onlinehome-server.info>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 16 quoted lines
> When fsck'ing an incoming pack, we need to fsck objects that cannot be
> read via read_sha1_file() because they are not local yet (and might even
> be rejected if transfer.fsckobjects is set to 'true').
>
> For commits, there is a hack in place: we basically cache commit
> objects' buffers anyway, but the same is not true, say, for tag objects.
>
> By refactoring fsck_object() to take the object buffer and size as
> optional arguments -- optional, because we still fall back to the
> previous method to look at the cached commit objects if the caller
> passes NULL -- we prepare the machinery for the upcoming handling of tag
> objects.
>
> The assumption that such buffers are inherently NUL terminated is now
> wrong, of course, hence we pass the size of the buffer so that we can
> add a sanity check later, to prevent running past the end of the buffer.

A nice side effect may be that we can now check (and perhaps warn) a commit buffer with a NUL inside, perhaps? I am not suggesting to add such a check to this series, but mentioning the possibilty here may have a merit.

Show 155 quoted lines
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/fsck.c           |  2 +-
>  builtin/index-pack.c     |  3 ++-
>  builtin/unpack-objects.c | 14 ++++++++++----
>  fsck.c                   | 24 +++++++++++++++---------
>  fsck.h                   |  4 +++-
>  5 files changed, 31 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/fsck.c b/builtin/fsck.c
> index d42a27d..d9f4e6e 100644
> --- a/builtin/fsck.c
> +++ b/builtin/fsck.c
> @@ -298,7 +298,7 @@ static int fsck_obj(struct object *obj)
>  
>  	if (fsck_walk(obj, mark_used, NULL))
>  		objerror(obj, "broken links");
> -	if (fsck_object(obj, check_strict, fsck_error_func))
> +	if (fsck_object(obj, NULL, 0, check_strict, fsck_error_func))
>  		return -1;
>  
>  	if (obj->type == OBJ_TREE) {
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 5568a5b..f2465ff 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -773,7 +773,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,
>  			if (!obj)
>  				die(_("invalid %s"), typename(type));
>  			if (do_fsck_object &&
> -			    fsck_object(obj, 1, fsck_error_function))
> +			    fsck_object(obj, buf, size, 1,
> +				    fsck_error_function))
>  				die(_("Error in object"));
>  			if (fsck_walk(obj, mark_link, NULL))
>  				die(_("Not all child objects of %s are reachable"), sha1_to_hex(obj->sha1));
> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
> index 99cde45..855d94b 100644
> --- a/builtin/unpack-objects.c
> +++ b/builtin/unpack-objects.c
> @@ -164,10 +164,10 @@ static unsigned nr_objects;
>   * Called only from check_object() after it verified this object
>   * is Ok.
>   */
> -static void write_cached_object(struct object *obj)
> +static void write_cached_object(struct object *obj, struct obj_buffer *obj_buf)
>  {
>  	unsigned char sha1[20];
> -	struct obj_buffer *obj_buf = lookup_object_buffer(obj);
> +
>  	if (write_sha1_file(obj_buf->buffer, obj_buf->size, typename(obj->type), sha1) < 0)
>  		die("failed to write object %s", sha1_to_hex(obj->sha1));
>  	obj->flags |= FLAG_WRITTEN;
> @@ -180,6 +180,8 @@ static void write_cached_object(struct object *obj)
>   */
>  static int check_object(struct object *obj, int type, void *data)
>  {
> +	struct obj_buffer *obj_buf;
> +
>  	if (!obj)
>  		return 1;
>  
> @@ -198,11 +200,15 @@ static int check_object(struct object *obj, int type, void *data)
>  		return 0;
>  	}
>  
> -	if (fsck_object(obj, 1, fsck_error_function))
> +	obj_buf = lookup_object_buffer(obj);
> +	if (!obj_buf)
> +		die("Whoops! Cannot find object '%s'", sha1_to_hex(obj->sha1));
> +	if (fsck_object(obj, obj_buf->buffer, obj_buf->size, 1,
> +			fsck_error_function))
>  		die("Error in object");
>  	if (fsck_walk(obj, check_object, NULL))
>  		die("Error on reachable objects of %s", sha1_to_hex(obj->sha1));
> -	write_cached_object(obj);
> +	write_cached_object(obj, obj_buf);
>  	return 0;
>  }
>  
> diff --git a/fsck.c b/fsck.c
> index 56156ff..dd77628 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -277,7 +277,7 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f
>  }
>  
>  static int fsck_commit_buffer(struct commit *commit, const char *buffer,
> -			      fsck_error error_func)
> +	unsigned long size, fsck_error error_func)
>  {
>  	unsigned char tree_sha1[20], sha1[20];
>  	struct commit_graft *graft;
> @@ -322,15 +322,18 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,
>  	return 0;
>  }
>  
> -static int fsck_commit(struct commit *commit, fsck_error error_func)
> +static int fsck_commit(struct commit *commit, const char *data,
> +	unsigned long size, fsck_error error_func)
>  {
> -	const char *buffer = get_commit_buffer(commit, NULL);
> -	int ret = fsck_commit_buffer(commit, buffer, error_func);
> -	unuse_commit_buffer(commit, buffer);
> +	const char *buffer = data ?  data : get_commit_buffer(commit, &size);
> +	int ret = fsck_commit_buffer(commit, buffer, size, error_func);
> +	if (!data)
> +		unuse_commit_buffer(commit, buffer);
>  	return ret;
>  }
>  
> -static int fsck_tag(struct tag *tag, fsck_error error_func)
> +static int fsck_tag(struct tag *tag, const char *data,
> +	unsigned long size, fsck_error error_func)
>  {
>  	struct object *tagged = tag->tagged;
>  
> @@ -339,7 +342,8 @@ static int fsck_tag(struct tag *tag, fsck_error error_func)
>  	return 0;
>  }
>  
> -int fsck_object(struct object *obj, int strict, fsck_error error_func)
> +int fsck_object(struct object *obj, void *data, unsigned long size,
> +	int strict, fsck_error error_func)
>  {
>  	if (!obj)
>  		return error_func(obj, FSCK_ERROR, "no valid object to fsck");
> @@ -349,9 +353,11 @@ int fsck_object(struct object *obj, int strict, fsck_error error_func)
>  	if (obj->type == OBJ_TREE)
>  		return fsck_tree((struct tree *) obj, strict, error_func);
>  	if (obj->type == OBJ_COMMIT)
> -		return fsck_commit((struct commit *) obj, error_func);
> +		return fsck_commit((struct commit *) obj, (const char *) data,
> +			size, error_func);
>  	if (obj->type == OBJ_TAG)
> -		return fsck_tag((struct tag *) obj, error_func);
> +		return fsck_tag((struct tag *) obj, (const char *) data,
> +			size, error_func);
>  
>  	return error_func(obj, FSCK_ERROR, "unknown type '%d' (internal fsck error)",
>  			  obj->type);
> diff --git a/fsck.h b/fsck.h
> index 1e4f527..d1e6387 100644
> --- a/fsck.h
> +++ b/fsck.h
> @@ -28,6 +28,8 @@ int fsck_error_function(struct object *obj, int type, const char *fmt, ...);
>   *    0		everything OK
>   */
>  int fsck_walk(struct object *obj, fsck_walk_func walk, void *data);
> -int fsck_object(struct object *obj, int strict, fsck_error error_func);
> +/* If NULL is passed for data, we assume the object is local and read it. */
> +int fsck_object(struct object *obj, void *data, unsigned long size,
> +	int strict, fsck_error error_func);
>  
>  #endif
Previous: Johannes SchindelinNext: Jeff King
Message 5 of 68 in “Improve tag checking in fsck and with transfer.fsckobjects”
  1. 0/6 Improve tag checking in fsck and with transfer.fsckobjectsJohannes Schindelin, Aug 28, 2014
  2. 1/6 Refactor type_from_string() to avoid die()ing in case of errorsJohannes Schindelin, Aug 28, 2014
  3. Junio C HamanoAug 28, 2014
  4. 2/6 Accept object data in the fsck_object() functionJohannes Schindelin, Aug 28, 2014
  5. Junio C HamanoAug 28, 2014
  6. Jeff KingAug 29, 2014
  7. Jeff KingAug 29, 2014
  8. 3/6 Make sure fsck_commit_buffer() does not run out of the bufferJohannes Schindelin, Aug 28, 2014
  9. Junio C HamanoAug 28, 2014
  10. Jeff KingAug 29, 2014
  11. 4/6 fsck: check tag objects' headersJohannes Schindelin, Aug 28, 2014
  12. Junio C HamanoAug 28, 2014
  13. Junio C HamanoAug 28, 2014
  14. Jeff KingAug 29, 2014
  15. Junio C HamanoAug 31, 2014
  16. Jeff KingSep 3, 2014
  17. Junio C HamanoSep 3, 2014
  18. Jeff KingSep 4, 2014
  19. Jeff KingAug 29, 2014
  20. Junio C HamanoSep 2, 2014
  21. Jeff KingSep 3, 2014
  22. 5/6 Add regression tests for stricter tag fsck'ingJohannes Schindelin, Aug 28, 2014
  23. 6/6 Make sure that index-pack --strict fails upon invalid tag objectsJohannes Schindelin, Aug 28, 2014
  24. 0/6 Improve tag checking in fsck and with transfer.fsckobjectsJohannes Schindelin, Sep 10, 2014
  25. Johannes SchindelinSep 10, 2014
  26. Junio C HamanoSep 10, 2014
  27. Junio C HamanoSep 10, 2014
  28. Johannes SchindelinSep 11, 2014
  29. 0/6 Improve tag checking in fsck and with transfer.fsckobjectsJohannes Schindelin, Sep 11, 2014
  30. 1/6 Refactor type_from_string() to avoid die()ing in case of errorsJohannes Schindelin, Sep 11, 2014
  31. 2/6 Accept object data in the fsck_object() functionJohannes Schindelin, Sep 11, 2014
  32. 3/6 Make sure fsck_commit_buffer() does not run out of the bufferJohannes Schindelin, Sep 11, 2014
  33. 4/6 fsck: check tag objects' headersJohannes Schindelin, Sep 11, 2014
  34. 5/6 Add regression tests for stricter tag fsck'ingJohannes Schindelin, Sep 11, 2014
  35. 6/6 Make sure that index-pack --strict checks tag objectsJohannes Schindelin, Sep 11, 2014
  36. Junio C HamanoSep 11, 2014
  37. Junio C HamanoSep 11, 2014
  38. 0/3 hash-object --literallyJunio C Hamano, Sep 11, 2014
  39. 1/3 hash-object: reduce file-scope staticsJunio C Hamano, Sep 11, 2014
  40. 2/3 hash-object: pass 'write_object' as a flagJunio C Hamano, Sep 11, 2014
  41. 3/3 hash-object: add --literally optionJunio C Hamano, Sep 11, 2014
  42. Johannes SchindelinSep 12, 2014
  43. 0/6 Improve tag checking in fsck and with transfer.fsckobjectsJohannes Schindelin, Sep 12, 2014
  44. 1/6 Refactor type_from_string() to avoid die()ing in case of errorsJohannes Schindelin, Sep 12, 2014
  45. 2/6 Accept object data in the fsck_object() functionJohannes Schindelin, Sep 12, 2014
  46. 3/6 Make sure fsck_commit_buffer() does not run out of the bufferJohannes Schindelin, Sep 12, 2014
  47. 4/6 fsck: check tag objects' headersJohannes Schindelin, Sep 12, 2014
  48. 5/6 Add regression tests for stricter tag fsck'ingJohannes Schindelin, Sep 12, 2014
  49. 6/6 Make sure that index-pack --strict checks tag objectsJohannes Schindelin, Sep 12, 2014
  50. Junio C HamanoSep 12, 2014
  51. Johannes SchindelinSep 13, 2014
  52. 1/6 Refactor type_from_string() to avoid die()ing in case of errorsJohannes Schindelin, Sep 10, 2014
  53. 2/6 Accept object data in the fsck_object() functionJohannes Schindelin, Sep 10, 2014
  54. 3/6 Make sure fsck_commit_buffer() does not run out of the bufferJohannes Schindelin, Sep 10, 2014
  55. Junio C HamanoSep 10, 2014
  56. Johannes SchindelinSep 11, 2014
  57. Junio C HamanoSep 11, 2014
  58. Eric SunshineSep 10, 2014
  59. 4/6 fsck: check tag objects' headersJohannes Schindelin, Sep 10, 2014
  60. Junio C HamanoSep 10, 2014
  61. 5/6 Add regression tests for stricter tag fsck'ingJohannes Schindelin, Sep 10, 2014
  62. Junio C HamanoSep 10, 2014
  63. Johannes SchindelinSep 11, 2014
  64. 6/6 Make sure that index-pack --strict fails upon invalid tag objectsJohannes Schindelin, Sep 10, 2014
  65. Junio C HamanoSep 10, 2014
  66. Johannes SchindelinSep 11, 2014
  67. Junio C HamanoSep 11, 2014
  68. Johannes SchindelinSep 11, 2014

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.