"sha1 information is lacking or useless" when rebasing with a submodule pointer conflict

I'm seeing what might be a bug that was introduced in git 1.7.12 (also observed in 1.8.1.2). If not a bug, it's a changed behavior from previous versions that I don't understand.

Here's the scenario:
* I have a remote repo containing a pointer to a submodule.
* Developer A and Developer B clone this repo, and both make a commit
to first the submodule, and then the parent repo, changing some files
and also the submodule pointer at the same time.
* Developer A pushes his changes to both the submodule and the parent
module to the shared remote
* Developer B either does a "git pull --rebase" or a "git fetch && git
rebase origin/master"

Results: When applying Developer B's changes on top of origin/master, the following messages are observed: "fatal: sha1 information is lacking or useless (sub). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge."

Furthermore, an immediate "git status" reports "all conflicts fixed", does not report the submodule pointer as unmerged, and the changes to the other files in the parent module made by Developer B are not applied.

Expected Results: Similar to how 1.7.12.1 behaves. No mention of sha1 information or blobs being lacking. "git status" should report the submodule pointer and any other changed files as being unmerged. Conflicting changes should not be lost.

I'm witnessing this behavior on OS X 10.8.2 with git versions 1.7.12, 1.7.12.1, and 1.8.1.2, installed via homebrew. I do not see this behavior in 1.7.11.5 or any of the 8 previous versions I've tried back to 1.7.4.1.

Workaround: Using an interactive rebase ("git fetch && git rebase -i origin/master") with no changes made to the commit list seems to sidestep this behavior.

Thanks in advance for any help with this.

Reproduce script: Execute the following, then "cd local-b; git pull --rebase"

#!/bin/sh

set -e -v

BASEDIR=`pwd`

# Make bare remote repos mkdir remotes pushd remotes git init --bare super.git git init --bare sub.git popd

# Initialize the submodule repo git clone --no-hardlinks remotes/sub.git sub pushd sub echo subfile > subfile git add subfile git commit -m initial-submodule git push origin master popd rm -Rf sub

# Clone into local-a and initialize supermodule repo git clone --no-hardlinks remotes/super.git local-a pushd local-a git submodule add $BASEDIR/remotes/sub.git sub echo file > file git add sub file git commit -m initial git push origin master popd

# Clone into local-b git clone --no-hardlinks remotes/super.git local-b pushd local-b git submodule init && git submodule update popd

# Make a change to supermodule file and submodule pointer from local-a and push to remote pushd local-a cd sub echo subfile changed from local-a > subfile git add subfile git commit -m subfile-changed-from-local-a git push cd .. echo file changed from local-a > file git add sub file git commit -m change-from-local-a git push popd

# Make a conflicting change to both supermodule file and submodule pointer from local-b pushd local-b cd sub echo subfile changed from local-b > subfile git add subfile git commit -m subfile-changed-from-local-b cd .. echo file changed from local-b > file git add sub file git commit -m change-from-local-b popd

Hi,

On Wed, Jan 30, 2013 at 12:43:31PM -0600, Michael Sims wrote:

Thanks for the detailed bug report and the demo script. I can reproduce the behavior here and will have a look into it. The submodule should be marked as conflict.

Cheers Heiko

Hi,

On Wed, Jan 30, 2013 at 10:56:15PM +0100, Heiko Voigt wrote:

Bisect identified the following commit:

commit a230949409f4a650c6a1a9a5879e2a8b993ba695 (HEAD)
Author: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
Date:   Tue Jun 26 07:51:56 2012 -0700

    am --rebasing: get patch body from commit, not from mailbox
    
    Rebasing a commit that contains a diff in the commit message results
    in a failure with output such as
    
      First, rewinding head to replay your work on top of it...
      Applying: My cool patch.
      fatal: sha1 information is lacking or useless
      (app/controllers/settings_controller.rb).
      Repository lacks necessary blobs to fall back on 3-way merge.
      Cannot fall back to three-way merge.
      Patch failed at 0001 My cool patch.
    
    The reason is that 'git rebase' without -p/-i/-m internally calls 'git
    format-patch' and pipes the output to 'git am --rebasing', which has
    no way of knowing what is a real patch and what is a commit message
    that contains a patch.
    
    Make 'git am' while in --rebasing mode get the patch body from the
    commit object instead of extracting it from the mailbox.
    
    Patch by Junio, test case and commit log message by Martin.
    
    Reported-by: anikey <arty.anikey@gmail.com>
    Helped-by: Junio C Hamano <gitster@pobox.com>
    Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Maybe Martin or Junio immediately see whats going wrong here? I would need to further dig into the git-am code to find out how to fix it.

Cheers Heiko

Heiko Voigt <hvoigt@hvoigt.net> writes:

"am -3" has never worked on a patch that describes changes to any non-blobs; the underlyihng "apply --fake-ancestor" is not prepared to see anything other than blobs.

Heiko Voigt <hvoigt@hvoigt.net> writes:

Thanks. I think the minimum fix is this, but there are some nits to pick around this area as well.

"git rebase" used to use "format-patch --full-index" to generate a series of patches and let "git am --rebase" reconstruct a fake preimage tree by reading the object names from the "index" lines.

With a230949 (am --rebasing: get patch body from commit, not from mailbox, 2012-06-26), we switched to use "diff-tree", but forgot that without "--full-index", the information generated on the "index" line is insufficient to recreate a preimage tree that contains a submodule.

The first one is the real fix to this issue.

The other two falls into the niceties category; they are to issue a better error messge when the machinery is fed an abbreviated object name on the "index" line for a submodule.

Junio C Hamano (3):
  git-am: record full index line in the patch used while rebasing
  apply: simplify build_fake_ancestor()
  apply: diagnose incomplete submodule object name better

 builtin/apply.c             | 30 ++++++++++++++++--------------
 git-am.sh                   |  2 +-
 t/t7402-submodule-rebase.sh | 30 ++++++++++++++++++++++++++++--
 3 files changed, 45 insertions(+), 17 deletions(-)

Earlier, a230949 (am --rebasing: get patch body from commit, not from mailbox, 2012-06-26) learned to regenerate patch body from the commit object while rebasing, instead of reading from the rebase-am front-end. While doing so, it used "git diff-tree" but without giving it the "--full-index" option.

This does not matter for in-repository objects; during rebasing, any abbreviated object name should uniquely identify them.

But we may be rebasing a commit that contains a change to a gitlink, in which case we usually should not have the object (it names a commit in the submodule). A full object name is necessary to later reconstruct a fake ancestor index for them.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-am.sh                   |  2 +-
 t/t7402-submodule-rebase.sh | 30 ++++++++++++++++++++++++++++--
 2 files changed, 29 insertions(+), 3 deletions(-)

On Thu, Jan 31, 2013 at 8:32 PM, Junio C Hamano <gitster@pobox.com> wrote:

emails about the breakage until now, but I wouldn't have known where to even start looking anyway, so thanks a lot for taking care of it.

The local variable sha1_ptr in the build_fake_ancestor() function used to either point at the null_sha1[] (if the ancestor did not have the path) or at sha1[] (if we read the object name into the local array), but 7a98869 (apply: get rid of --index-info in favor of --build-fake-ancestor, 2007-09-17) made the "missing in the ancestor" case unnecessary, hence sha1_ptr, when used, always points at the local array.

Get rid of the unneeded variable, and restructure the if/else cascade a bit to make it easier to read. There should be no behaviour change.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

"git am -3" uses this function to build a tree that records how the preimage the patch was created from would have looked like. An abbreviated object name on the index line is ordinarily sufficient for us to figure out the object name the preimage tree would have contained, but a change to a submodule by definition shows an object name of a submodule commit which our repository should not have, and get_sha1_blob() is not an appropriate way to read it (or get_sha1() for that matter).

Use get_sha1_hex() and complain if we do not find a full object name there.

We could read from the payload part of the patch to learn the full object name of the commit, but the primary user "git rebase" has been fixed to give us a full object name, so this should suffice for now.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Junio C Hamano <gitster@pobox.com> writes:

And the patch on top to do so looks like this. With this patch in place, we could drop 1/3 of this series, but there is no strong reason to do so. After all, 1/3 is all about internal implementation details.

-- >8 --
Subject: [PATCH 4/3] apply: verify submodule commit object name better

A textual patch also records the submodule commit object name in full. Make the parsing more robust by reading from there and verifying the (possibly abbreviated) name on the index line matches.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/apply.c | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)