From: Jonathan Nieder <jrnieder@gmail.com>
Date: Tue, 24 Jul 2012 19:40:47 GMT
Subject: Re: [PATCH v2 3/3] fast-import: disallow "merge $itself" command
Message-ID: <20120724194046.GA14351@burratino>
In-Reply-To: <1340818825-13754-4-git-send-email-divanorama@gmail.com>

Hi,

In June, Dmitry Ivankov wrote:

> In presence of "from $some" command "merge $itself" acts the same as
> "merge $some" would. Which is completely undocumented and looks like
> a bug (caused by parse_from() temporarily rewriting b->sha1 with $some).

Could you give an example?

> Just deny "merge $itself" for now. It was a bit broken and btw "from
> $itself" was and is a forbidden command too.
>
> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>

Yes, this one still looks good.

[...]
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -2611,7 +2611,7 @@ static int parse_from(struct branch *b)
>  	return 1;
>  }
>  
> -static struct hash_list *parse_merge(unsigned int *count)
> +static struct hash_list *parse_merge(unsigned int *count, struct branch *b)
>  {
>  	struct hash_list *list = NULL, *n, *e = e;
>  	const char *from;
> @@ -2622,7 +2622,13 @@ static struct hash_list *parse_merge(unsigned int *count)
>  		from = strchr(command_buf.buf, ' ') + 1;
>  		n = xmalloc(sizeof(*n));
>  		s = lookup_branch(from);
> -		if (s)
> +		if (b == s)

Style: "if (s == b)" would make it clearer that b is known (the current
branch) and s unknown.  Giving the 'b' parameter a meaningful name
like 'this_branch' would help even more.

> +			/*
> +			 * Also if there were a 'from' command, b will point to
> +			 * 'from' commit, because parse_from stores it there.
> +			 */
> +			die("Can't merge a branch with itself: %s", b->name);

It's not clear to me what the "Also" is referring to here.  How
about:

			/*
			 * If there was a 'from' command, b->sha1 refers to
			 * that commit instead of the previous commit on the
			 * current branch, which is probably what no one
			 * expected.
			 *
			 * Let's just reject attempts to merge a branch into
			 * itself.
			 */
			die("Can't merge a ...");

[...]
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -871,6 +871,19 @@ test_expect_success \
>  	'git fast-import <input &&
>  	git rev-parse --verify J5 &&
>  	test_must_fail git rev-parse --verify J5^'
> +
> +cat >input <<INPUT_END
> +commit refs/heads/J5
> +committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
> +data <<COMMIT
> +Merge J5 with itself.
> +COMMIT
> +merge refs/heads/J5
> +
> +INPUT_END
> +test_expect_success \
> +	'J: disallow merge with itself' \
> +	'test_must_fail git fast-import <input'

Looks sensible.

If the changes suggested above look good to you, I can amend locally.
Otherwise, I'll be happy to see what you come up with next.

Thanks,
Jonathan