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

Re: [PATCH 3/3] commit-graph: error out on invalid commit oids in 'write --stdin-commits'

From
Derrick Stolee <stolee@gmail.com>
Date
Aug 5, 2019, 13:57 UTC
Message-ID
<ecdd6c01-5ee2-6869-4eb3-1eb45f4c335c@gmail.com>
In-Reply-To
<20190805080240.30892-4-szeder.dev@gmail.com>
On 8/5/2019 4:02 AM, SZEDER Gábor wrote:
Show 23 quoted lines
> While 'git commit-graph write --stdin-commits' expects commit object
> ids as input, it accepts and silently skips over any invalid commit
> object ids, and still exits with success:
> 
>   # nonsense
>   $ echo not-a-commit-oid | git commit-graph write --stdin-commits
>   $ echo $?
>   0
>   # sometimes I forgot that refs are not good...
>   $ echo HEAD | git commit-graph write --stdin-commits
>   $ echo $?
>   0
>   # valid tree OID, but not a commit OID
>   $ git rev-parse HEAD^{tree} | git commit-graph write --stdin-commits
>   $ echo $?
>   0
>   $ ls -l .git/objects/info/commit-graph
>   ls: cannot access '.git/objects/info/commit-graph': No such file or directory
> 
> Check that all input records are indeed valid commit object ids and
> return with error otherwise, the same way '--stdin-packs' handles
> invalid input; see e103f7276f (commit-graph: return with errors during
> write, 2019-06-12).
Consistency is good. We should definitely make these modes match.
Show 8 quoted lines
> Note that it should only return with error when encountering an
> invalid commit object id coming from standard input.  However,
> '--reachable' uses the same code path to process object ids pointed to
> by all refs, and that includes tag object ids as well, which should
> still be skipped over.  Therefore add a new flag to 'enum
> commit_graph_write_flags' and a corresponding field to 'struct
> write_commit_graph_context', so we can differentiate between those two
> cases.
Thank you for the care here.
[snip]
Show 27 quoted lines
> @@ -1215,20 +1216,21 @@ static void fill_oids_from_commit_hex(struct write_commit_graph_context *ctx,
>  		struct commit *result;
>  
>  		display_progress(ctx->progress, i + 1);
> -		if (commit_hex->items[i].string &&
> -		    parse_oid_hex(commit_hex->items[i].string, &oid, &end))
> -			continue;
> -
> -		result = lookup_commit_reference_gently(ctx->r, &oid, 1);
> -
> -		if (result) {
> +		if (!parse_oid_hex(commit_hex->items[i].string, &oid, &end) &&
> +		    (result = lookup_commit_reference_gently(ctx->r, &oid, 1))) {
>  			ALLOC_GROW(ctx->oids.list, ctx->oids.nr + 1, ctx->oids.alloc);
>  			oidcpy(&ctx->oids.list[ctx->oids.nr], &(result->object.oid));
>  			ctx->oids.nr++;
> +		} else if (ctx->check_oids) {
> +			error(_("invalid commit object id: %s"),
> +			    commit_hex->items[i].string);
> +			return -1;
>  		}
>  	}
>  	stop_progress(&ctx->progress);
>  	strbuf_release(&progress_title);
> +
> +	return 0;
>  }

This is the critical bit. I notice that you are not checking commit_hex->items[i].string for NULL, but it should never be NULL here anyway.

Show 6 quoted lines
> @@ -1775,6 +1777,7 @@ int write_commit_graph(const char *obj_dir,
>  	ctx->append = flags & COMMIT_GRAPH_WRITE_APPEND ? 1 : 0;
>  	ctx->report_progress = flags & COMMIT_GRAPH_WRITE_PROGRESS ? 1 : 0;
>  	ctx->split = flags & COMMIT_GRAPH_WRITE_SPLIT ? 1 : 0;
> +	ctx->check_oids = flags & COMMIT_GRAPH_WRITE_CHECK_OIDS ? 1 : 0;
>  	ctx->split_opts = split_opts;

Using the enum for the function and the bitfield for internal logic matches the existing pattern. Thanks.

Show 10 quoted lines
> @@ -1829,8 +1832,10 @@ int write_commit_graph(const char *obj_dir,
>  			goto cleanup;
>  	}
>  
> -	if (commit_hex)
> -		fill_oids_from_commit_hex(ctx, commit_hex);
> +	if (commit_hex) {
> +		if ((res = fill_oids_from_commit_hex(ctx, commit_hex)))
> +			goto cleanup;
> +	}
And this links the low-level error to a return code.

Thanks for this! The changes here look good and justify the two cleanup patches.

-Stolee
Previous: SZEDER GáborNext: SZEDER Gábor
Message 5 of 19 in “commit-graph: error out on invalid commit oids in 'write --stdin-commits'”
  1. 0/3 commit-graph: error out on invalid commit oids in 'write --stdin-commits'SZEDER Gábor, Aug 5, 2019
  2. 1/3 t5318-commit-graph: use 'test_expect_code'SZEDER Gábor, Aug 5, 2019
  3. 2/3 commit-graph: turn a group of write-related macro flags into an enumSZEDER Gábor, Aug 5, 2019
  4. 3/3 commit-graph: error out on invalid commit oids in 'write --stdin-commits'SZEDER Gábor, Aug 5, 2019
  5. Derrick StoleeAug 5, 2019
  6. SZEDER GáborAug 5, 2019
  7. Jeff KingApr 3, 2020
  8. Taylor BlauApr 3, 2020
  9. SZEDER GáborApr 3, 2020
  10. Jeff KingApr 3, 2020
  11. SZEDER GáborApr 3, 2020
  12. Jeff KingApr 3, 2020
  13. Taylor BlauApr 13, 2020
  14. Jeff KingApr 13, 2020
  15. Taylor BlauApr 14, 2020
  16. Taylor BlauApr 3, 2020
  17. Junio C HamanoApr 3, 2020
  18. Taylor BlauApr 3, 2020
  19. SZEDER GáborAug 5, 2019

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.