@matheusbsbs / matheus-bagliano / commits / eddd1a411d

merge-recursive: enforce rule that index matches head before merging

builtin/merge.c says that when we are about to perform a merge: ...the index must be in sync with the head commit. The strategies are responsible to ensure this. merge-recursive has always relied on unpack_trees() to enforce this requirement, except in the case of an "Already up to date!" merge. unpack-trees.c does not actually enforce this requirement, though. It allows for a pair of exceptions, in cases which it refers to as #14(ALT) and #2ALT. Documentation/technical/trivial-merge.txt can be consulted for the precise meanings of the various case numbers and their meanings for unpack-trees.c, but we have a high-level description of the intent behind these two exceptions in a combined and summarized form in Documentation/git-merge.txt: ...[merge will] abort if there are any changes registered in the index relative to the `HEAD` commit. (One exception is when the changed index entries are in the state that would result from the merge already.) While this high-level description does describe conditions under which it would be safe to allow the index to diverge from HEAD, it does not match what is actually implemented. In particular, unpack-trees.c has no knowledge of renames, and these two exceptions were written assuming that no renames take place. Once renames get into the mix, it is no longer safe to allow the index to not match for #2ALT. We could modify unpack-trees to only allow #14(ALT) as an exception, but that would be more strict than required for the resolve strategy (since the resolve strategy doesn't handle renames at all). Therefore, unpack_trees.c seems like the wrong place to fix this. Further, if someone fixes the combination of break and rename detection and modifies merge-recursive to take advantage of the combination, then it will also no longer be safe to allow the index to not match for #14(ALT) when the recursive strategy is in use. Therefore, leaving one of the exceptions in place with the recursive merge strategy feels like we are just leaving a latent bug in the code for folks in the future to stumble across. It may be possible to fix both unpack-trees and merge-recursive in a way that implements the exception as stated in Documentation/git-merge.txt, but it would be somewhat complex, possibly also buggy at first, and ultimately, not all that valuable. Instead, just enforce the requirement stated in builtin/merge.c; error out if the index does not match the HEAD commit, just like the 'ours' and 'octopus' strategies do. Some testcase fixups were in order: t7611: had many tests designed to show that `git merge --abort` could not always restore the index and working tree to the state they were in before the merge started. The tests that were associated with having changes in the index before the merge started are no longer applicable, so they have been removed. t7504: had a few tests that had stray staged changes that were not actually part of the test under consideration t6044: We no longer expect stray staged changes to sometimes result in the merge continuing. Also, fix a case where a merge didn't abort but should have. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Jun 30, 2018 at 18:25 UTC eddd1a411d935420e1746108ecfee564114a129f

4 files changed +18 -137

merge-recursive.c

+7 -7

@@ -1974,6 +1974,13 @@ int merge_trees(struct merge_options *o,

1974 struct tree **result)

1975 {

1976 int code, clean;

1977 + struct strbuf sb = STRBUF_INIT;

1978 +

1979 + if (!o->call_depth && index_has_changes(&the_index, head, &sb)) {

1980 + err(o, _("Your local changes to the following files would be overwritten by merge:\n %s"),

1981 + sb.buf);

1982 + return -1;

1983 + }

1984

1985 if (o->subtree_shift) {

1986 merge = shift_tree_object(head, merge, o->subtree_shift);

@@ -1981,13 +1988,6 @@ int merge_trees(struct merge_options *o,

1988 }

1989

1990 if (oid_eq(&common->object.oid, &merge->object.oid)) {

1984 - struct strbuf sb = STRBUF_INIT;

1985 -

1986 - if (!o->call_depth && index_has_changes(&the_index, head, &sb)) {

1987 - err(o, _("Your local changes to the following files would be overwritten by merge:\n %s"),

1988 - sb.buf);

1989 - return -1;

1990 - }

1991 output(o, 0, _("Already up to date!"));

1992 *result = head;

1993 return 1;

t/t6044-merge-unrelated-index-changes.sh

+8 -11

@@ -137,7 +137,7 @@ test_expect_success 'merge-recursive, when index==head but head!=HEAD' '

137 test_i18ngrep "Already up to date" out

138 '

139

140 -test_expect_failure 'recursive, when file has staged changes not matching HEAD nor what a merge would give' '

140 +test_expect_success 'recursive, when file has staged changes not matching HEAD nor what a merge would give' '

141 git reset --hard &&

142 git checkout B^0 &&

143

@@ -145,14 +145,12 @@ test_expect_failure 'recursive, when file has staged changes not matching HEAD n

145 test_seq 1 10 >subdir/a &&

146 git add subdir/a &&

147

148 - # HEAD has no subdir/a; merge would write 1..11 to subdir/a;

149 - # Since subdir/a matches neither HEAD nor what the merge would write

150 - # to that file, the merge should fail to avoid overwriting what is

151 - # currently found in subdir/a

152 - test_must_fail git merge -s recursive E^0

148 + # We have staged changes; merge should error out

149 + test_must_fail git merge -s recursive E^0 2>err &&

150 + test_i18ngrep "changes to the following files would be overwritten" err

151 '

152

155 -test_expect_failure 'recursive, when file has staged changes matching what a merge would give' '

153 +test_expect_success 'recursive, when file has staged changes matching what a merge would give' '

154 git reset --hard &&

155 git checkout B^0 &&

156

@@ -160,10 +158,9 @@ test_expect_failure 'recursive, when file has staged changes matching what a mer

158 test_seq 1 11 >subdir/a &&

159 git add subdir/a &&

160

163 - # HEAD has no subdir/a; merge would write 1..11 to subdir/a;

164 - # Since subdir/a matches what the merge would write to that file,

165 - # the merge should be safe to proceed

166 - git merge -s recursive E^0

161 + # We have staged changes; merge should error out

162 + test_must_fail git merge -s recursive E^0 2>err &&

163 + test_i18ngrep "changes to the following files would be overwritten" err

164 '

165

166 test_expect_success 'octopus, unrelated file touched' '

t/t7504-commit-msg-hook.sh

+3 -1

@@ -157,6 +157,7 @@ test_expect_success 'merge bypasses failing hook with --no-verify' '

157 test_when_finished "git branch -D newbranch" &&

158 test_when_finished "git checkout -f master" &&

159 git checkout --orphan newbranch &&

160 + git rm -f file &&

161 : >file2 &&

162 git add file2 &&

163 git commit --no-verify file2 -m in-side-branch &&

@@ -168,7 +169,7 @@ test_expect_success 'merge bypasses failing hook with --no-verify' '

169 chmod -x "$HOOK"

170 test_expect_success POSIXPERM 'with non-executable hook' '

171

171 - echo "content" >> file &&

172 + echo "content" >file &&

173 git add file &&

174 git commit -m "content"

175

@@ -249,6 +250,7 @@ test_expect_success 'hook called in git-merge picks up commit message' '

250 test_when_finished "git branch -D newbranch" &&

251 test_when_finished "git checkout -f master" &&

252 git checkout --orphan newbranch &&

253 + git rm -f file &&

254 : >file2 &&

255 git add file2 &&

256 git commit --no-verify file2 -m in-side-branch &&

t/t7611-merge-abort.sh

-118

@@ -187,31 +187,6 @@ test_expect_success 'Fail clean merge with matching dirty worktree' '

187 test_cmp expect actual

188 '

189

190 -test_expect_success 'Abort clean merge with matching dirty index' '

191 - git add bar &&

192 - git diff --staged > expect &&

193 - git merge --no-commit clean_branch &&

194 - test -f .git/MERGE_HEAD &&

195 - ### When aborting the merge, git will discard all staged changes,

196 - ### including those that were staged pre-merge. In other words,

197 - ### --abort will LOSE any staged changes (the staged changes that

198 - ### are lost must match the merge result, or the merge would not

199 - ### have been allowed to start). Change expectations accordingly:

200 - rm expect &&

201 - touch expect &&

202 - # Abort merge

203 - git merge --abort &&

204 - test ! -f .git/MERGE_HEAD &&

205 - test "$pre_merge_head" = "$(git rev-parse HEAD)" &&

206 - git diff --staged > actual &&

207 - test_cmp expect actual &&

208 - test -z "$(git diff)"

209 -'

210 -

211 -test_expect_success 'Reset worktree changes' '

212 - git reset --hard "$pre_merge_head"

213 -'

214 -

190 test_expect_success 'Fail conflicting merge with matching dirty worktree' '

191 echo barf > bar &&

192 git diff > expect &&

@@ -223,97 +198,4 @@ test_expect_success 'Fail conflicting merge with matching dirty worktree' '

198 test_cmp expect actual

199 '

200

226 -test_expect_success 'Abort conflicting merge with matching dirty index' '

227 - git add bar &&

228 - git diff --staged > expect &&

229 - test_must_fail git merge conflict_branch &&

230 - test -f .git/MERGE_HEAD &&

231 - ### When aborting the merge, git will discard all staged changes,

232 - ### including those that were staged pre-merge. In other words,

233 - ### --abort will LOSE any staged changes (the staged changes that

234 - ### are lost must match the merge result, or the merge would not

235 - ### have been allowed to start). Change expectations accordingly:

236 - rm expect &&

237 - touch expect &&

238 - # Abort merge

239 - git merge --abort &&

240 - test ! -f .git/MERGE_HEAD &&

241 - test "$pre_merge_head" = "$(git rev-parse HEAD)" &&

242 - git diff --staged > actual &&

243 - test_cmp expect actual &&

244 - test -z "$(git diff)"

245 -'

246 -

247 -test_expect_success 'Reset worktree changes' '

248 - git reset --hard "$pre_merge_head"

249 -'

250 -

251 -test_expect_success 'Abort merge with pre- and post-merge worktree changes' '

252 - # Pre-merge worktree changes

253 - echo xyzzy > foo &&

254 - echo barf > bar &&

255 - git add bar &&

256 - git diff > expect &&

257 - git diff --staged > expect-staged &&

258 - # Perform merge

259 - test_must_fail git merge conflict_branch &&

260 - test -f .git/MERGE_HEAD &&

261 - # Post-merge worktree changes

262 - echo yzxxz > foo &&

263 - echo blech > baz &&

264 - ### When aborting the merge, git will discard staged changes (bar)

265 - ### and unmerged changes (baz). Other changes that are neither

266 - ### staged nor marked as unmerged (foo), will be preserved. For

267 - ### these changed, git cannot tell pre-merge changes apart from

268 - ### post-merge changes, so the post-merge changes will be

269 - ### preserved. Change expectations accordingly:

270 - git diff -- foo > expect &&

271 - rm expect-staged &&

272 - touch expect-staged &&

273 - # Abort merge

274 - git merge --abort &&

275 - test ! -f .git/MERGE_HEAD &&

276 - test "$pre_merge_head" = "$(git rev-parse HEAD)" &&

277 - git diff > actual &&

278 - test_cmp expect actual &&

279 - git diff --staged > actual-staged &&

280 - test_cmp expect-staged actual-staged

281 -'

282 -

283 -test_expect_success 'Reset worktree changes' '

284 - git reset --hard "$pre_merge_head"

285 -'

286 -

287 -test_expect_success 'Abort merge with pre- and post-merge index changes' '

288 - # Pre-merge worktree changes

289 - echo xyzzy > foo &&

290 - echo barf > bar &&

291 - git add bar &&

292 - git diff > expect &&

293 - git diff --staged > expect-staged &&

294 - # Perform merge

295 - test_must_fail git merge conflict_branch &&

296 - test -f .git/MERGE_HEAD &&

297 - # Post-merge worktree changes

298 - echo yzxxz > foo &&

299 - echo blech > baz &&

300 - git add foo bar &&

301 - ### When aborting the merge, git will discard all staged changes

302 - ### (foo, bar and baz), and no changes will be preserved. Whether

303 - ### the changes were staged pre- or post-merge does not matter

304 - ### (except for not preventing starting the merge).

305 - ### Change expectations accordingly:

306 - rm expect expect-staged &&

307 - touch expect &&

308 - touch expect-staged &&

309 - # Abort merge

310 - git merge --abort &&

311 - test ! -f .git/MERGE_HEAD &&

312 - test "$pre_merge_head" = "$(git rev-parse HEAD)" &&

313 - git diff > actual &&

314 - test_cmp expect actual &&

315 - git diff --staged > actual-staged &&

316 - test_cmp expect-staged actual-staged

317 -'

318 -

201 test_done