From: Jonathan Nieder <jrnieder@gmail.com>
Date: Wed, 27 Jun 2012 21:33:55 GMT
Subject: Re: [PATCH v2 2/3] fast-import: allow "merge $null_sha1" command
Message-ID: <20120627213355.GO12774@burratino>
In-Reply-To: <1340818825-13754-3-git-send-email-divanorama@gmail.com>

Dmitry Ivankov wrote:

> "from $null_sha1" and "merge $empty_branch" are already allowed so
> allow "merge $null_sha1" command too.

The reader might not realize that null_sha1 means
0000000000000000000000000000000000000000 until she reads the test
script.  Is it possible to help her save time?

[...]
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -2631,12 +2631,14 @@ static struct hash_list *parse_merge(unsigned int *count)
>  				die("Mark :%" PRIuMAX " not a commit", idnum);
>  			hashcpy(n->sha1, oe->idx.sha1);
>  		} else if (!get_sha1(from, n->sha1)) {
> -			unsigned long size;
> -			char *buf = read_object_with_reference(n->sha1,
> -				commit_type, &size, n->sha1);
> -			if (!buf || size < 46)
> -				die("Not a valid commit: %s", from);
> -			free(buf);
> +			if (!is_null_sha1(n->sha1)) {
> +				unsigned long size;
> +				char *buf = read_object_with_reference(n->sha1,
> +					commit_type, &size, n->sha1);
> +				if (!buf || size < 46)
> +					die("Not a valid commit: %s", from);
> +				free(buf);
> +			}

Hm, ok.  Maybe the "peel onion" call guarded by this "if" could be a
separate function to make this cleaner (and avoid some duplication of
code with other functions while at it)?

e.g.,

	static int peel_to_commit(unsigned char sha1[20])
	{
		unsigned long size;

		char *buf = read_object_with_reference(...);
		if (!buf)
			return -1;
		free(buf);
		if (size < strlen("commit ") + 40)
			return -1;
		return 0;
	}

	...
		if (is_null_sha1(n->sha1))
			; /* ok */
		else if (peel_to_commit(n->sha1))
			die("Not a valid commit: %s", from);

I like the direction, but as it is, this patch feels kind of "meh" to
me.

Thanks again and hope that helps,
Jonathan