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

Re: [PATCH v3 02/19] builtin/diff-tree: convert to struct object_id

From
Ramsay Jones <ramsay@ramsayjones.plus.com>
Date
Feb 18, 2017, 01:18 UTC
Message-ID
<3630da01-5af3-bc02-3a8c-1e3495512279@ramsayjones.plus.com>
In-Reply-To
<20170218000652.375129-3-sandals@crustytoothpaste.net>
On 18/02/17 00:06, brian m. carlson wrote:
Show 65 quoted lines
> Convert most leaf functions to struct object_id.  Rewrite several
> hardcoded numbers in terms of GIT_SHA1_HEXSZ, using an intermediate
> variable where that makes sense.
> 
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  builtin/diff-tree.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
> index 8ce00480cd..1f1573bb2a 100644
> --- a/builtin/diff-tree.c
> +++ b/builtin/diff-tree.c
> @@ -7,9 +7,9 @@
>  
>  static struct rev_info log_tree_opt;
>  
> -static int diff_tree_commit_sha1(const unsigned char *sha1)
> +static int diff_tree_commit_sha1(const struct object_id *oid)
>  {
> -	struct commit *commit = lookup_commit_reference(sha1);
> +	struct commit *commit = lookup_commit_reference(oid->hash);
>  	if (!commit)
>  		return -1;
>  	return log_tree_commit(&log_tree_opt, commit);
> @@ -18,22 +18,22 @@ static int diff_tree_commit_sha1(const unsigned char *sha1)
>  /* Diff one or more commits. */
>  static int stdin_diff_commit(struct commit *commit, char *line, int len)
>  {
> -	unsigned char sha1[20];
> -	if (isspace(line[40]) && !get_sha1_hex(line+41, sha1)) {
> +	struct object_id oid;
> +	if (isspace(line[GIT_SHA1_HEXSZ]) && !get_oid_hex(line+GIT_SHA1_HEXSZ+1, &oid)) {
>  		/* Graft the fake parents locally to the commit */
> -		int pos = 41;
> +		int pos = GIT_SHA1_HEXSZ + 1;
>  		struct commit_list **pptr;
>  
>  		/* Free the real parent list */
>  		free_commit_list(commit->parents);
>  		commit->parents = NULL;
>  		pptr = &(commit->parents);
> -		while (line[pos] && !get_sha1_hex(line + pos, sha1)) {
> -			struct commit *parent = lookup_commit(sha1);
> +		while (line[pos] && !get_oid_hex(line + pos, &oid)) {
> +			struct commit *parent = lookup_commit(oid.hash);
>  			if (parent) {
>  				pptr = &commit_list_insert(parent, pptr)->next;
>  			}
> -			pos += 41;
> +			pos += GIT_SHA1_HEXSZ + 1;
>  		}
>  	}
>  	return log_tree_commit(&log_tree_opt, commit);
> @@ -42,11 +42,13 @@ static int stdin_diff_commit(struct commit *commit, char *line, int len)
>  /* Diff two trees. */
>  static int stdin_diff_trees(struct tree *tree1, char *line, int len)
>  {
> -	unsigned char sha1[20];
> +	struct object_id oid;
>  	struct tree *tree2;
> -	if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))
> +	const int chunksz = GIT_SHA1_HEXSZ + 1;
> +	if (len != 2 * chunksz || !isspace(line[chunksz-1]) ||
> +		get_sha1_hex(line + chunksz, oid.hash))

I'm not sure that this is an improvement. The input expected in 'line' is supposed to look like: '<sha1> + <space> + <sha1> + <\n>'. So your 'chunk' would be a <sha1> plus one 'char' of some sort. Except that the caller of this function has already replaced the newline character with a '\0' char (so strlen(line) would return 81), but still passes the original line length! Also, note that this (and other functions in this file) actually test for 'isspace(char)' rather than for a ' ' char!

Hmm, maybe just:
if (len < (2 * GIT_SHA1_HEXSZ + 1) || line[GIT_SHA1_HEXSZ] != ' ' ||
    get_sha1_hex(line + GIT_SHA1_HEXSZ + 1, oid.hash))
(or, perhaps, still call isspace() in this patch ...)

ATB, Ramsay Jones

Show 56 quoted lines
>  		return error("Need exactly two trees, separated by a space");
> -	tree2 = lookup_tree(sha1);
> +	tree2 = lookup_tree(oid.hash);
>  	if (!tree2 || parse_tree(tree2))
>  		return -1;
>  	printf("%s %s\n", oid_to_hex(&tree1->object.oid),
> @@ -60,15 +62,15 @@ static int stdin_diff_trees(struct tree *tree1, char *line, int len)
>  static int diff_tree_stdin(char *line)
>  {
>  	int len = strlen(line);
> -	unsigned char sha1[20];
> +	struct object_id oid;
>  	struct object *obj;
>  
>  	if (!len || line[len-1] != '\n')
>  		return -1;
>  	line[len-1] = 0;
> -	if (get_sha1_hex(line, sha1))
> +	if (get_oid_hex(line, &oid))
>  		return -1;
> -	obj = parse_object(sha1);
> +	obj = parse_object(oid.hash);
>  	if (!obj)
>  		return -1;
>  	if (obj->type == OBJ_COMMIT)
> @@ -76,7 +78,7 @@ static int diff_tree_stdin(char *line)
>  	if (obj->type == OBJ_TREE)
>  		return stdin_diff_trees((struct tree *)obj, line, len);
>  	error("Object %s is a %s, not a commit or tree",
> -	      sha1_to_hex(sha1), typename(obj->type));
> +	      oid_to_hex(&oid), typename(obj->type));
>  	return -1;
>  }
>  
> @@ -141,7 +143,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
>  		break;
>  	case 1:
>  		tree1 = opt->pending.objects[0].item;
> -		diff_tree_commit_sha1(tree1->oid.hash);
> +		diff_tree_commit_sha1(&tree1->oid);
>  		break;
>  	case 2:
>  		tree1 = opt->pending.objects[0].item;
> @@ -164,9 +166,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
>  			opt->diffopt.setup |= (DIFF_SETUP_USE_SIZE_CACHE |
>  					       DIFF_SETUP_USE_CACHE);
>  		while (fgets(line, sizeof(line), stdin)) {
> -			unsigned char sha1[20];
> +			struct object_id oid;
>  
> -			if (get_sha1_hex(line, sha1)) {
> +			if (get_oid_hex(line, &oid)) {
>  				fputs(line, stdout);
>  				fflush(stdout);
>  			}
>
Previous: brian m. carlsonNext: brian m. carlson
Message 3 of 28 in “object_id part 6”
  1. 00/19 object_id part 6brian m. carlson, Feb 18, 2017
  2. 02/19 builtin/diff-tree: convert to struct object_idbrian m. carlson, Feb 18, 2017
  3. Ramsay JonesFeb 18, 2017
  4. brian m. carlsonFeb 18, 2017
  5. Jeff KingFeb 18, 2017
  6. Jeff KingFeb 18, 2017
  7. brian m. carlsonFeb 18, 2017
  8. Jeff KingFeb 18, 2017
  9. 08/19 builtin/clone: convert to struct object_idbrian m. carlson, Feb 18, 2017
  10. 05/19 builtin/fmt-merge-message: convert to struct object_idbrian m. carlson, Feb 18, 2017
  11. 07/19 builtin/branch: convert to struct object_idbrian m. carlson, Feb 18, 2017
  12. 06/19 builtin/grep: convert to struct object_idbrian m. carlson, Feb 18, 2017
  13. 18/19 builtin/merge-base: convert to struct object_idbrian m. carlson, Feb 18, 2017
  14. 15/19 refs: simplify parsing of reflog entriesbrian m. carlson, Feb 18, 2017
  15. 17/19 Convert object iteration callbacks to struct object_idbrian m. carlson, Feb 18, 2017
  16. 19/19 wt-status: convert to struct object_idbrian m. carlson, Feb 18, 2017
  17. 13/19 refs: convert each_reflog_ent_fn to struct object_idbrian m. carlson, Feb 18, 2017
  18. 11/19 builtin/replace: convert to struct object_idbrian m. carlson, Feb 18, 2017
  19. 14/19 hex: introduce parse_oid_hexbrian m. carlson, Feb 18, 2017
  20. 12/19 reflog-walk: convert struct reflog_info to struct object_idbrian m. carlson, Feb 18, 2017
  21. 16/19 sha1_file: introduce an nth_packed_object_oid functionbrian m. carlson, Feb 18, 2017
  22. Ramsay JonesFeb 18, 2017
  23. brian m. carlsonFeb 18, 2017
  24. 01/19 builtin/commit: convert to struct object_idbrian m. carlson, Feb 18, 2017
  25. 04/19 builtin/fast-export: convert to struct object_idbrian m. carlson, Feb 18, 2017
  26. 03/19 builtin/describe: convert to struct object_idbrian m. carlson, Feb 18, 2017
  27. 10/19 Convert remaining callers of resolve_refdup to object_idbrian m. carlson, Feb 18, 2017
  28. 09/19 builtin/merge: convert to struct object_idbrian m. carlson, Feb 18, 2017

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.