From 4ae6d4699f4d1df7cd3ffac9dd361c86d62eca42 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 31 Jan 2013 19:26:21 -0800 Subject: [PATCH 1/3] git-am: record full index line in the patch used while rebasing 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 --- 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 c682d34094..0e0a0966f4 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 f919c8d34d..8e32f19007 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 From e2afb0be9073fbe9f565ad9e952155c0bf7dd7c6 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 31 Jan 2013 19:19:44 -0800 Subject: [PATCH 2/3] apply: simplify build_fake_ancestor() 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 --- builtin/apply.c | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/builtin/apply.c b/builtin/apply.c index 156b3ce3b7..a1db7b4295 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)) From e28efb1998bb0b73057de13b9568f6aef439a583 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 31 Jan 2013 19:33:27 -0800 Subject: [PATCH 3/3] apply: diagnose incomplete submodule object name better "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 --- builtin/apply.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/builtin/apply.c b/builtin/apply.c index a1db7b4295..1f78e2cfab 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 */