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@xxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html