[PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit

* [PATCH/RFD] builtin-revert.c: release index lock when cherry-picking an empty commit
@ 2009-03-07  9:30 Chris Johnsen
  2009-03-07 11:15 ` Johannes Schindelin
                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Chris Johnsen @ 2009-03-07  9:30 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Miklos Vajna, Chris Johnsen

When a cherry-pick of an empty commit is done, release the lock
held on the index.

The fix is the same as was applied to similar code in 4271666046.

Signed-off-by: Chris Johnsen <chris_johnsen@pobox.com>
---

RELEVANT HISTORY

The previous code was introduced in 6eb1b43793.

It seems to have been copied from builtin-merge.c, where it was
introduced in 18668f5319 (6eb1's parent).

The code introduced in 1866 was fixed with the addition of
rollback_lock_file (the same fix as applied here) in 4271666046.

CONTEXT

I ran into the code path that this patch modifies while using
'git rebase -i' on a branch that had empty commits. When rebase
tried to cherry-pick an empty commit, the cherry-pick returned an
error and failed to unlocking the index. While this patch does
not allow 'git rebase -i' to "correctly" process empty commits,
it does prevent 'git cherry-pick' from exiting without unlocking
the index.

	T="$(mktemp -d -t empty-cherry)"
	cd "$T"
	git init
	echo a > a
	git add a
	git commit -m 'add a'
	git checkout -b empty
	git commit --allow-empty -m 'empty'
	git checkout master
	git cherry-pick empty

> Finished one cherry-pick.
> fatal: Unable to create '.git/index.lock': File exists.
> 
> If no other git process is currently running, this probably means a
> git process crashed in this repository earlier. Make sure no other git
> process is running and remove the file manually to continue.

I was originally appending empty commits with descriptive
messages to mark "interesting" points in a stream of otherwise
automatic commits (made by an Xcode build script). My idea was to
use these commits as markers that would flow with the rest of the
commits through a subsequent series of cleanups done with 'git
rebase -i'.  After encountering this problem, however, I moved
away from using empty commits (I have since been using 'git
commit --amend' to rewrite the last (generic, automatic) commit
message), but the bug left me wondering about the the status of
emtpy commits.

UNEVEN TREATMENT OF EMPTY CHANGES

It seems that empty commits suffer uneven treatment under various
patch-transport/history-rewriting mechanisms. They seem to be
handled okay in the most of the core (fetch, push, bundle all
seem to preserve empty commits, though I have not done rigorous
testing). But there are various problems in other areas:
'format-patch', 'send-email', 'apply', 'am', 'rebase' (automatic,
non-fast-forward; and --interactive).

DISCUSSION

36863af16e (git-commit --allow-empty) says "This is primarily for
use by foreign scm interface scripts.". Is this the only case
where empty commits _should_ be used? Or is the uneven treatment
just a matter nobody having an itch to use empty commits in
situations where the above commands would interact with them?

I played with having builtin-revert.c always use 'git commit
--allow-empty ...', but I was not confident that such behavior
would match "official policy for empty commits" (if such policy
even exists). Should empty commits be preserved (by default) once
they are in the commit stream? Should commands that do "patch
manipulation" only preserve empty commits if they are explicitly
instructed to do so (with their own --allow-emtpy option)? Should
something completely different happen?

I realize that rebasing and cherry-picking need to have special
consideration for "effectively empty" patches (those that
introduce changes already present; usually because one side
already picked some changes from the other), but maybe "actually
empty, yet informational" commits also deserve some
consideration.

If such "actually empty, yet informational" commits (no content
changes, but a useful commit messages) are a valid use of empty
commits, then maybe "actually empty" commits in general deserve
treatment equal to that of normal commits.

---
 builtin-revert.c             |    1 +
 t/t3505-cherry-pick-empty.sh |   33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 0 deletions(-)
 create mode 100755 t/t3505-cherry-pick-empty.sh

diff --git a/builtin-revert.c b/builtin-revert.c
index d210150..3f2614e 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -376,6 +376,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	    (write_cache(index_fd, active_cache, active_nr) ||
 	     commit_locked_index(&index_lock)))
 		die("%s: Unable to write new index file", me);
+	rollback_lock_file(&index_lock);
 
 	if (!clean) {
 		add_to_msg("\nConflicts:\n\n");
diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
new file mode 100755
index 0000000..9aaeabd
--- /dev/null
+++ b/t/t3505-cherry-pick-empty.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='test cherry-picking an empty commit'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	echo first > file1 &&
+	git add file1 &&
+	test_tick &&
+	git commit -m "first" &&
+
+	git checkout -b empty-branch &&
+	test_tick &&
+	git commit --allow-empty -m "empty"
+
+'
+
+test_expect_code 1 'cherry-pick an empty commit' '
+
+	git checkout master &&
+	git cherry-pick empty-branch
+
+'
+
+test_expect_success 'index lockfile was removed' '
+
+	test ! -f .git/index.lock
+
+'
+
+test_done
-- 
1.6.2

^ permalink raw reply related	[flat|nested] 21+ messages in thread