Re: [PATCH v4 03/19] builtin/diff-tree: convert to struct object_id

From: Jeff King <peff@peff.net>
Date: Feb 20, 2017, 08:09 UTC
Message-ID: <20170220080902.vkexezd5solnhrhb@sigill.intra.peff.net>
In-Reply-To: <20170220001031.559931-4-sandals@crustytoothpaste.net>

On Mon, Feb 20, 2017 at 12:10:15AM +0000, brian m. carlson wrote:

Show 26 quoted lines

>  /* Diff one or more commits. */
> -static int stdin_diff_commit(struct commit *commit, char *line, int len)
> +static int stdin_diff_commit(struct commit *commit, const char *p)
>  {
> -	unsigned char sha1[20];
> -	if (isspace(line[40]) && !get_sha1_hex(line+41, sha1)) {
> +	struct object_id oid;
> +	if (isspace(*p++) && !parse_oid_hex(p, &oid, &p)) {
>  		/* Graft the fake parents locally to the commit */
> -		int pos = 41;
>  		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 (isspace(*p++) && !parse_oid_hex(p, &oid, &p)) {
> +			struct commit *parent = lookup_commit(oid.hash);
>  			if (parent) {
>  				pptr = &commit_list_insert(parent, pptr)->next;
>  			}
> -			pos += 41;
>  		}
>  	}

Are you sure this is right? The first "if" will advance the "p" pointer, and we'll miss it in the inner loop.

IOW, the original looked something like:

  1. see if we have any parents after the initial commit sha1

  2. if so, then free the original parent list, so we can parse the new
     ones

  3. starting at pos 41 (the same one we parsed in the conditional!),
     loop and parse each parent sha1

The conditional in step 1 can't advance our pointer, or we miss the first parent in step 3.

It's silly to parse the same sha1 twice, though. You could solve it by adding the first "oid" from the conditional to the new parent list. In my "something like this" patch, I solved it by dropping the conditional, and just having the inner loop. It lazily drops the old parent list on the first iteration.

It's a little disturbing that we do not seem to have even a basic test of:

  git rev-list --parents HEAD | git diff-tree --stdin

which would exercise this code.

-Peff