Re: [PATCH v2 3/3] fast-import: disallow "merge $itself" command
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:54:19
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 <redacted>
Yes, this one still looks good. [...]
quoted hunk ↗ jump to hunk
--- 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 ...");
[...]quoted hunk ↗ jump to hunk
--- 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