cherry-pick: regression fix for empty commits
The earlier "--keep-redundant-commit" series broke "cherry-pick" that is given a commit whose change is already in the current history. Such a cherry-pick would result in an empty change, and should stop with an error, telling the user that conflict resolution may have made the result empty (which is exactly what is happening), but we silently dropped the change on the floor without any message nor non-zero exit code. Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano committed
May 29, 2012 at 17:14 UTC
ac2b0e8fb77d02f480af3f45a44cb9ea611008a4
2 files changed
+75
-28
sequencer.c
+45
-28
@@ -291,7 +291,8 @@ static int is_index_unchanged(void)
291
* If we are revert, or if our cherry-pick results in a hand merge,
292
* we had better say that the current user is responsible for that.
293
*/
294
-static int run_git_commit(const char *defmsg, struct replay_opts *opts)
294
+static int run_git_commit(const char *defmsg, struct replay_opts *opts,
295
+ int allow_empty)
296
{
297
struct argv_array array;
298
int rc;
@@ -307,7 +308,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts)
308
argv_array_push(&array, defmsg);
309
}
310
310
- if (opts->allow_empty)
311
+ if (allow_empty)
312
argv_array_push(&array, "--allow-empty");
313
314
rc = run_command_v_opt(array.argv, RUN_GIT_CMD);
@@ -335,6 +336,44 @@ static int is_original_commit_empty(struct commit *commit)
336
return !hashcmp(ptree_sha1, commit->tree->object.sha1);
337
}
338
339
+/*
340
+ * Do we run "git commit" with "--allow-empty"?
341
+ */
342
+static int allow_empty(struct replay_opts *opts, struct commit *commit)
343
+{
344
+ int index_unchanged, empty_commit;
345
+
346
+ /*
347
+ * Three cases:
348
+ *
349
+ * (1) we do not allow empty at all and error out.
350
+ *
351
+ * (2) we allow ones that were initially empty, but
352
+ * forbid the ones that become empty;
353
+ *
354
+ * (3) we allow both.
355
+ */
356
+ if (!opts->allow_empty)
357
+ return 0; /* let "git commit" barf as necessary */
358
+
359
+ index_unchanged = is_index_unchanged();
360
+ if (index_unchanged < 0)
361
+ return index_unchanged;
362
+ if (!index_unchanged)
363
+ return 0; /* we do not have to say --allow-empty */
364
+
365
+ if (opts->keep_redundant_commits)
366
+ return 1;
367
+
368
+ empty_commit = is_original_commit_empty(commit);
369
+ if (empty_commit < 0)
370
+ return empty_commit;
371
+ if (!empty_commit)
372
+ return 0;
373
+ else
374
+ return 1;
375
+}
376
+
377
static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
378
{
379
unsigned char head[20];
@@ -344,8 +383,6 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
383
char *defmsg = NULL;
384
struct strbuf msgbuf = STRBUF_INIT;
385
int res;
347
- int empty_commit;
348
- int index_unchanged;
386
387
if (opts->no_commit) {
388
/*
@@ -471,10 +508,6 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
508
free_commit_list(remotes);
509
}
510
474
- empty_commit = is_original_commit_empty(commit);
475
- if (empty_commit < 0)
476
- return empty_commit;
477
-
511
/*
512
* If the merge was clean or if it failed due to conflict, we write
513
* CHERRY_PICK_HEAD for the subsequent invocation of commit to use.
@@ -495,27 +528,11 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
528
print_advice(res == 1, opts);
529
rerere(opts->allow_rerere_auto);
530
} else {
498
- index_unchanged = is_index_unchanged();
499
- /*
500
- * If index_unchanged is less than 0, that indicates we either
501
- * couldn't parse HEAD or the index, so error out here.
502
- */
503
- if (index_unchanged < 0)
504
- return index_unchanged;
505
-
506
- if (!empty_commit && !opts->keep_redundant_commits && index_unchanged)
507
- /*
508
- * The head tree and the index match
509
- * meaning the commit is empty. Since it wasn't created
510
- * empty (based on the previous test), we can conclude
511
- * the commit has been made redundant. Since we don't
512
- * want to keep redundant commits, we can just return
513
- * here, skipping this commit
514
- */
515
- return 0;
516
-
531
+ int allow = allow_empty(opts, commit);
532
+ if (allow < 0)
533
+ return allow;
534
if (!opts->no_commit)
518
- res = run_git_commit(defmsg, opts);
535
+ res = run_git_commit(defmsg, opts, allow);
536
}
537
538
free_message(&msg);
t/t3505-cherry-pick-empty.sh
+30
@@ -71,4 +71,34 @@ test_expect_success 'cherry pick with --keep-redundant-commits' '
71
git cherry-pick --keep-redundant-commits HEAD^
72
'
73
74
+test_expect_success 'cherry-pick a commit that becomes no-op (prep)' '
75
+ git checkout master &&
76
+ git branch fork &&
77
+ echo foo >file2 &&
78
+ git add file2 &&
79
+ test_tick &&
80
+ git commit -m "add file2 on master" &&
81
+
82
+ git checkout fork &&
83
+ echo foo >file2 &&
84
+ git add file2 &&
85
+ test_tick &&
86
+ git commit -m "add file2 on the side"
87
+'
88
+
89
+test_expect_success 'cherry-pick a no-op without --keep-redundant' '
90
+ git reset --hard &&
91
+ git checkout fork^0 &&
92
+ test_must_fail git cherry-pick master
93
+'
94
+
95
+test_expect_success 'cherry-pick a no-op with --keep-redundant' '
96
+ git reset --hard &&
97
+ git checkout fork^0 &&
98
+ git cherry-pick --keep-redundant-commits master &&
99
+ git show -s --format='%s' >actual &&
100
+ echo "add file2 on master" >expect &&
101
+ test_cmp expect actual
102
+'
103
+
104
test_done