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

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

[Michael Sims]

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

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

[Heiko Voigt]

Hi,

On Wed, Jan 30, 2013 at 12:43:31PM -0600, Michael Sims wrote:
> 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"

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

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

[Heiko Voigt]

Hi,

On Wed, Jan 30, 2013 at 10:56:15PM +0100, Heiko Voigt wrote:
> On Wed, Jan 30, 2013 at 12:43:31PM -0600, Michael Sims wrote:
> > 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"
> 
> 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.

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

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

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> 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.

"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.

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

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> 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.

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


diff --git b/git-am.sh a/git-am.sh
index b4d95f5..202130f 100755
--- b/git-am.sh
+++ a/git-am.sh
@@ -664,7 +664,7 @@ do
 			sed -e '1,/^$/d' >"$dotest/msg-clean"
 			echo "$commit" >"$dotest/original-commit"
 			get_author_ident_from_commit "$commit" >"$dotest/author-script"
-			git diff-tree --root --binary "$commit" >"$dotest/patch"
+			git diff-tree --root --binary --full-index "$commit" >"$dotest/patch"
 		else
 			git mailinfo $keep $no_inbody_headers $scissors $utf8 "$dotest/msg" "$dotest/patch" \
 				<"$dotest/$msgnum" >"$dotest/info" ||

[PATCH 0/3] rebasing changes that update submodules

[Junio C Hamano]

"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(-)

-- 
1.8.1.2.612.g09f4be5

[PATCH 1/3] git-am: record full index line in the patch used while rebasing

[Junio C Hamano]

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(-)

diff --git a/git-am.sh b/git-am.sh
index c682d34..0e0a096 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -664,7 +664,7 @@ do
 			sed -e '1,/^$/d' >"$dotest/msg-clean"
 			echo "$commit" >"$dotest/original-commit"
 			get_author_ident_from_commit "$commit" >"$dotest/author-script"
-			git diff-tree --root --binary "$commit" >"$dotest/patch"
+			git diff-tree --root --binary --full-index "$commit" >"$dotest/patch"
 		else
 			git mailinfo $keep $no_inbody_headers $scissors $utf8 "$dotest/msg" "$dotest/patch" \
 				<"$dotest/$msgnum" >"$dotest/info" ||
diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
index f919c8d..8e32f19 100755
--- a/t/t7402-submodule-rebase.sh
+++ b/t/t7402-submodule-rebase.sh
@@ -3,7 +3,7 @@
 # Copyright (c) 2008 Johannes Schindelin
 #
 
-test_description='Test rebasing and stashing with dirty submodules'
+test_description='Test rebasing, stashing, etc. with submodules'
 
 . ./test-lib.sh
 
@@ -20,7 +20,8 @@ test_expect_success setup '
 	echo second line >> file &&
 	(cd submodule && git pull) &&
 	test_tick &&
-	git commit -m file-and-submodule -a
+	git commit -m file-and-submodule -a &&
+	git branch added-submodule
 
 '
 
@@ -89,4 +90,29 @@ test_expect_success 'stash with a dirty submodule' '
 
 '
 
+test_expect_success 'rebasing submodule that should conflict' '
+	git reset --hard &&
+	git checkout added-submodule &&
+	git add submodule &&
+	test_tick &&
+	git commit -m third &&
+	(
+		cd submodule &&
+		git commit --allow-empty -m extra
+	) &&
+	git add submodule &&
+	test_tick &&
+	git commit -m fourth &&
+
+	test_must_fail git rebase --onto HEAD^^ HEAD^ HEAD^0 &&
+	git ls-files -s submodule >actual &&
+	(
+		cd submodule &&
+		echo "160000 $(git rev-parse HEAD^) 1	submodule" &&
+		echo "160000 $(git rev-parse HEAD^^) 2	submodule" &&
+		echo "160000 $(git rev-parse HEAD) 3	submodule"
+	) >expect &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.1.2.612.g09f4be5

[PATCH 2/3] apply: simplify build_fake_ancestor()

[Junio C Hamano]

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(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 156b3ce..a1db7b4 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3598,7 +3598,6 @@ static void build_fake_ancestor(struct patch *list, const char *filename)
 	 * worth showing the new sha1 prefix, but until then...
 	 */
 	for (patch = list; patch; patch = patch->next) {
-		const unsigned char *sha1_ptr;
 		unsigned char sha1[20];
 		struct cache_entry *ce;
 		const char *name;
@@ -3606,20 +3605,19 @@ static void build_fake_ancestor(struct patch *list, const char *filename)
 		name = patch->old_name ? patch->old_name : patch->new_name;
 		if (0 < patch->is_new)
 			continue;
-		else if (get_sha1_blob(patch->old_sha1_prefix, sha1))
-			/* git diff has no index line for mode/type changes */
-			if (!patch->lines_added && !patch->lines_deleted) {
-				if (get_current_sha1(patch->old_name, sha1))
-					die("mode change for %s, which is not "
-						"in current HEAD", name);
-				sha1_ptr = sha1;
-			} else
-				die("sha1 information is lacking or useless "
-					"(%s).", name);
-		else
-			sha1_ptr = sha1;
 
-		ce = make_cache_entry(patch->old_mode, sha1_ptr, name, 0, 0);
+		if (!get_sha1_blob(patch->old_sha1_prefix, sha1)) {
+			; /* ok */
+		} else if (!patch->lines_added && !patch->lines_deleted) {
+			/* mode-only change: update the current */
+			if (get_current_sha1(patch->old_name, sha1))
+				die("mode change for %s, which is not "
+				    "in current HEAD", name);
+		} else
+			die("sha1 information is lacking or useless "
+			    "(%s).", name);
+
+		ce = make_cache_entry(patch->old_mode, sha1, name, 0, 0);
 		if (!ce)
 			die(_("make_cache_entry failed for path '%s'"), name);
 		if (add_index_entry(&result, ce, ADD_CACHE_OK_TO_ADD))
-- 
1.8.1.2.612.g09f4be5

[PATCH 3/3] apply: diagnose incomplete submodule object name better

[Junio C Hamano]

"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(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index a1db7b4..1f78e2c 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3606,7 +3606,11 @@ static void build_fake_ancestor(struct patch *list, const char *filename)
 		if (0 < patch->is_new)
 			continue;
 
-		if (!get_sha1_blob(patch->old_sha1_prefix, sha1)) {
+		if (S_ISGITLINK(patch->old_mode)) {
+			if (get_sha1_hex(patch->old_sha1_prefix, sha1))
+				die("submoule change for %s without full index name",
+				    name);
+		} else if (!get_sha1_blob(patch->old_sha1_prefix, sha1)) {
 			; /* ok */
 		} else if (!patch->lines_added && !patch->lines_deleted) {
 			/* mode-only change: update the current */
-- 
1.8.1.2.612.g09f4be5

Re: [PATCH 1/3] git-am: record full index line in the patch used while rebasing

[Martin von Zweigbergk]

On Thu, Jan 31, 2013 at 8:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

>From what I can understand, this all makes sense. I didn't notice the
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.

Re* [PATCH 3/3] apply: diagnose incomplete submodule object name better

[Junio C Hamano]

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

> 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.

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(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index 1f78e2c..e0f1474 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3587,6 +3587,40 @@ static int get_current_sha1(const char *path, unsigned char *sha1)
 	return 0;
 }
 
+static int preimage_sha1_in_gitlink_patch(struct patch *p, unsigned char sha1[20])
+{
+	/*
+	 * A usable gitlink patch has only one fragment (hunk) that looks like:
+	 * @@ -1 +1 @@
+	 * -Subproject commit <old sha1>
+	 * +Subproject commit <new sha1>
+	 * or
+	 * @@ -1 +0,0 @@
+	 * -Subproject commit <old sha1>
+	 * for a removal patch.
+	 */
+	struct fragment *hunk = p->fragments;
+	static const char heading[] = "-Subproject commit ";
+	char *preimage;
+
+	if (/* does the patch have only one hunk? */
+	    hunk && !hunk->next &&
+	    /* is its preimage one line? */
+	    hunk->oldpos == 1 && hunk->oldlines == 1 &&
+	    /* does preimage begin with the heading? */
+	    (preimage = memchr(hunk->patch, '\n', hunk->size)) != NULL &&
+	    !prefixcmp(++preimage, heading) &&
+	    /* does it record full SHA-1? */
+	    !get_sha1_hex(preimage + sizeof(heading) - 1, sha1) &&
+	    preimage[sizeof(heading) + 40 - 1] == '\n' &&
+	    /* does the abbreviated name on the index line agree with it? */
+	    !prefixcmp(preimage + sizeof(heading) - 1, p->old_sha1_prefix))
+		return 0; /* it all looks fine */
+
+	/* we may have full object name on the index line */
+	return get_sha1_hex(p->old_sha1_prefix, sha1);
+}
+
 /* Build an index that contains the just the files needed for a 3way merge */
 static void build_fake_ancestor(struct patch *list, const char *filename)
 {
@@ -3607,8 +3641,10 @@ static void build_fake_ancestor(struct patch *list, const char *filename)
 			continue;
 
 		if (S_ISGITLINK(patch->old_mode)) {
-			if (get_sha1_hex(patch->old_sha1_prefix, sha1))
-				die("submoule change for %s without full index name",
+			if (!preimage_sha1_in_gitlink_patch(patch, sha1))
+				; /* ok, the textual part looks sane */
+			else
+				die("sha1 information is lacking or useless for submoule %s",
 				    name);
 		} else if (!get_sha1_blob(patch->old_sha1_prefix, sha1)) {
 			; /* ok */
-- 
1.8.1.2.639.g9428735