patchbuiltin-revert.c: release index lock when cherry-picking an empty commit

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

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

Hi,

On Sat, 7 Mar 2009, Chris Johnsen wrote:

Thanks for the detailed explanation, and the patch!

I wonder, though, if the real root of the problem is that there is copied code. IOW I think it would be better to introduce a global function that writes the index to a tree. A quick "git grep commit_locked_index" reveals quite a few code sites...

Thanks, Dscho

On 2009 Mar 7, Johannes Schindelin wrote:

Agreed.

I am not quite sure I follow your meaning here.

We have write_cache_as_tree in cache-tree.c. Is something like that what you had in mind for "write the index to a tree"?

Could you elaborate on how an "index to tree" function could be applied to the problem of the inconsistent lock releasing around commit_locked_index calls? Sorry, for my lack of a clue, I am fairly new to the code base. Or are you seeing a different code duplication problem here?

The general form of the code around calls to commit_locked_index seems to be of the this form:

	if (some_condition) /* not all call sites use this */
		if (write_cache(...) || commit_locked_index(...))
			die(...); /* or return error(...) */
	rollback_lock_file(...); /* sometimes missing or distant */

In most cases some_condition is active_cache_changed, but not always (builtin-apply.c, rerere.c).

The problem with cherry-picking empty commits was that active_cache_changed (used as some_condition in the general pattern shown above) would be false, so the write and commit was skipped, but there was also never any rollback. Later, when cherry-pick exec-ed commit, the lock file still existed and commit dies.

To make sure a commit or a rollback always happens at every call site, each one would have to unconditionally call some global function (write_and_commit_or_rollback_locked_index?, ick) that conditionally did the write and commit, but unconditionally did the rollback (basically a no-op if the commit went OK).

Indeed, would such a cleanup be worth the churn? I do not have any modifications for which this cleanup could be considered preparatory.

Thank you for your help!

Hi,

On Sat, 7 Mar 2009, Chris Johnsen wrote:

Well, my thinking was that instead of imitating what merge-recursive does, you could refactor that very code into a function that gets called from both merge-recursive and revert.

Ciao, Dscho

Chris Johnsen <chris_johnsen@pobox.com> writes:

Thanks. Will apply. We should handle possible refactoring as a separate topic.

They just transfer an existing history from one place to another without modifying, so it is unfortunately true that they preserve such a broken history with empty commits.

These are all about creating history afresh, and they actively discourage empty commits to be (re)created.

There is no "uneven treatment".

If foreign scm recorded an empty commit, it would be nice to be able to recreate such a broken history _if the user wanted to_, and that is where the --allow-empty option can be used.

On 2009 Mar 7, at 22:14, Junio C Hamano wrote:

Thank you for the clarification. I will explain the source of my confusion.

The current documentation "Usually recording [an empty commit] is a mistake... This option ... is primarily for use by foreign scm interface scripts." implied to me that there was room outside foreign scm interface for "normal" use of empty commits.

My confusion was that I took "usually a mistake" to refer to the case where the user meant to commit content changes but forgot to first stage any changed content. But your clarification shows that "usually a mistake" really means that making any empty commit, intentional or not, is (considered to be) a fundamental misuse of SCM machinery.

Would it be acceptable to strengthen the language in the documentation for --allow-empty? Possibly something like the following paragraph:

Empty commits are a broken concept and should never be made during normal usage. By default, the command prevents you from making such a commit. This option bypasses the safety, and is primarily for use by foreign scm interface scripts.

If such a change is acceptable, I will send a patch.

Chris Johnsen <chris_johnsen@pobox.com> writes:

The empty commits in your example a few messages ago are used as "piss in the snow" marking. If you did not have tags (and commit notes), it may be the only workaround to say "here is an interesting point", but even then such a workaround can only be made while the commit is at the tip, and be made useful only by forcing all the other commits on the branch be on top of that "piss in the snow" commit, so it is a flawed workaround.

Suppose you have this history.

 ---A---B---C

You found that the point C is interesting in some way, so you mark it:

 ---A---B---C---P

But somebody else may have developed on top of C bypassing P

              D---E---F
             /
 ---A---B---C---P

What would you do in such a case? You cannot leave P dangling, as that would mean P will not participate in future rebases (and you do not want to rebase P on top of F because C is the point that is interesting to you, not F). Do you merge F and P only to make P not dangling? What does such a merge mean?

Worse yet, if you stared from the original history with three commits, how would you mark that B is interesting?

          P   D---E---F
         /   /
 ---A---B---C

The facility git and other SCM offer you to leave such mark (possibly after the fact) is to use tags.

So in your particular "piss in the snow" usage, I would agree that such an empty commit is a misuse.

I am not however claiming that all uses of an empty commit are fundamental misuses here, though. Somebody else may have other valid uses.

On Sat, Mar 07, 2009 at 03:30:51AM -0600, Chris Johnsen wrote:

Hmm. This test fails for me on FreeBSD. However, it seems to be related to a shell portability issue with the test suite. The extra newline inside the shell snippet seems to lose the last status. The following works fine for me with bash or dash:

-- >8 -- #!/bin/sh

test_description='test that shell handles whitespace in eval' . ./test-lib.sh

test_expect_code 1 'no newline' 'false' test_expect_code 1 'one newline' 'false ' test_expect_code 1 'extra newline' 'false

'

test_done -- 8< --

but on a FreeBSD 6.1 box generates:

  *   ok 1: no newline
  *   ok 2: one newline
  * FAIL 3: 1
          extra newline false

With this minimal example, you can see that the problem is not some subtle bug in the test suite:

-- >8 -- #!/bin/sh

eval 'false' echo status is $? eval 'false ' echo status is $? eval 'false

' echo status is $? -- 8< --

generates:

  status is 1
  status is 1
  status is 0

which means that any tests of the form

  test_expect_success description '

      contents

  '

are not actually having their exit code checked properly, and are just claiming success regardless of what happened. So this definitely needs to be addressed, I think. I'm not sure of the best course of action, though. Our options include:

  1. Declare appended newline a forbidden style, fix all existing cases
     in the test suite, and be on the lookout for new ones.

     The biggest problem with this option is that we have no automated
     way of policing. Such tests will just silently pass on the broken
     platform.

  2. Have test_run_ canonicalize the snippet by removing trailing
     newlines.

  3. Declare FreeBSD's /bin/sh unfit for git consumption, and require
     bash for the test suite.

I think (2) is the most reasonable option of those choices.

We could also try to convince FreeBSD that it's a bug, but that doesn't change the fact that the tests are broken on every existing version.

-Peff

On Sun, Mar 08, 2009 at 10:42:40AM -0400, Jeff King wrote:

And here's what that would look like:

Jeff King <peff@peff.net> writes:

If this part from your analysis is true for a shell:

I would be very tempted to declare that shell is unfit for any serious use, not just for test suite. Removing the empty line at the end of a scriptlet that such a broken shell misinterprets as an empty command that is equivalent to ":" (or "true") might hide breakages in the test suite, but

 (1) eval "$string" is used outside of test suite, most notably "am" and
     "bisect".  I think "am"'s use is safe, but I wouldn't be surprised if
     the scriptlet "bisect" internally creates has empty lines if only for
     debuggability; and more importantly

 (2) who knows what _other_ things may be broken in such a shell?

On Sun, Mar 08, 2009 at 12:45:55PM -0700, Junio C Hamano wrote:

OK, good points. I was just hoping not to cause people on FreeBSD undue pain. What is the best way to make such a declaration? I can think of:

  1. A mention in the release notes.

  2. A test in the Makefile similar to the $(:) test.

  3. Getting in touch with the freebsd ports maintainer for git and
     suggesting a dependency on bash (and/or seeing if he wants to push
     through a fix for /bin/sh).

     I don't know if the same problem exists on other BSD-influenced systems,
     or how closely they share the ports collection (it's been quite a
     while since I've really admin'd a freebsd box). For that matter, I
     wonder if this is also a problem on OS X. Can somebody with an OS X
     box try:

       $ /bin/sh
       $ eval 'false

         '
       $ echo $?

     It should print '1'; if it prints '0', the shell is broken.

-Peff

On Mar 10, 2009, at 7:17 PM, Jeff King wrote:

prints '1' here (10.5.6)

tom

On Mar 10, 2009, at 7:17 PM, Jeff King wrote:

prints '1' here (10.5.6)

tom

On 2009 Mar 10, at 13:17, Jeff King wrote:

I wrote t3505 on a Mac OS X 10.4.11 system. On that system, /bin/sh is a copy of bash v2.05b. Your test code prints 1 here.

On Tue, Mar 10, 2009 at 06:57:55PM -0500, Chris Johnsen wrote:

OK, then nothing to worry about there. I have no idea which shell OpenBSD and NetBSD use these days, and I don't have access to a box. Anybody?

-Peff

2009/3/11 Jeff King <peff@peff.net>:

OpenBSD uses pdksh in Bourne shell mode for non-root shells (ksh mode for root) [1].

NetBSD >=4 uses a Bourne shell but I don't know the exact provenance. [2] "A sh command appeared in Version 1 AT&T UNIX. It was, however, unmaintainable so we wrote this one."

[1] http://www.openbsd.org/faq/faq10.html#ksh [2] http://www.netbsd.org/docs/misc/index.html#shells

2009/3/11 Mike Ralphson <mike.ralphson@gmail.com>

... and isn't broken in this instance (OpenBSD v4.1)

Weird test failures though, so now I'm looking at that 8-)

[this is a follow-up on the "eval 'false\n\n'" returns 0 issue on FreeBSD]

On Wed, Mar 11, 2009 at 11:08:06AM +0000, Mike Ralphson wrote:

Thanks for looking this up, Mike. It sounds like FreeBSD is probably the only problematic one. I confirmed that the problem still exists in FreeBSD 7.1, and I've mailed the git ports maintainer off-list to make him aware of the issue. So we'll see what happens.

Junio, do you want to put anything in the release notes warning people who build from source that this is a potential issue? Do you want something in the Makefile detecting that the shell is broken?

-Peff

Jeff King <peff@peff.net> writes:

Thanks for keeping track of this one.

A sentence or two in INSTALL will not hurt.

I would not worry too much about the test scripts, but I would worry more about getting phantom bug reports for our shell script Porcelains that get hit by this. Earlier I mentioned bisect is the only heavy user, but the issue is more severe with filter-branch that is designed to eval end user scripts (calls to 'eval "$filter_frotz"' check the exit status and die on failure---with trailing blank lines the failure the filter reports will not get caught).

On Sun, Mar 22, 2009 at 02:58:35PM -0700, Junio C Hamano wrote:

Agreed. The good news is that the /bin/sh people are treating it like a bug:

  http://lists.freebsd.org/pipermail/freebsd-standards/2009-March/001721.html

so it will hopefully be fixed soon. It might still be worth warning users of older releases in INSTALL, though.

-Peff

Jeff King wrote:

Even /bin/sh (which is unfit for git consumption) on Solaris 7 produces non-zero for all three tests:

   status is 255
   status is 255
   status is 255

I set SHELL_PATH=/usr/xpg4/bin/sh aka ksh when compiling git which also handles your test correctly:

   status is 1
   status is 1
   status is 1

IRIX6.5 /bin/sh and /bin/ksh produce the correct results also.

-brandon