{"thread":{"id":"29385","subject":"Interactive rebase with submodules","startedAt":"2012-01-17T18:47:08Z","lastAt":"2012-01-19T10:48:37Z","messageCount":5,"participants":["John Keeping","Jens Lehmann"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"182691","messageId":"4F15C22C.3020902@metanate.com","threadId":"29385","inReplyTo":null,"subject":"Interactive rebase with submodules","fromName":"John Keeping","fromEmail":"john@metanate.com","sentAt":"2012-01-17T18:47:08Z","receivedAt":"2012-01-17T18:47:08Z","isPatch":false,"sender":{"key":"john@metanate.com","avatar":null},"body":"I've encountered a scenario where git rebase --interactive drops a \ncommit which contains a modification to a submodule but no other changes.\n\nThis occurs when there is a conflict when applying the commit (for \nexample if the submodule's history has been rewritten and you are \nrewriting the parent repository to match the new version of the submodule).\n\nTo clarify:\n\ngit rebase -i\n# Edit a commit, switching submodule to an unrelated commit\ngit rebase --continue\n# Conflict in submodule, checkout the correct submodule commit\ngit add path/to/submodule\n# Only change in index is updated submodule\ngit rebase --continue\n# No commit is created for the submodule change\n\n\nThis appears to be because the git-rebase--interactive script inspects \nwhether there is anything to commit when `rebase --continue` is invoked \nby running:\n\n     git diff-index --cached --quiet --ignore-submodules HEAD --\n\nIs there a reason for the `--ignore-submodules` in this command? \nRemoving that option results in the expected behaviour.\n\n\nI can understand not updating submodules while running the rebase, but I \nexpected that having resolved a conflict and added my change to the \nindex it would be applied by `git rebase --continue`, as indeed it is if \nthere happen to be other (non-submodule) changes in the same commit.\n\n\n-- \nJohn\n"},{"id":"182700","messageId":"4F15E83D.10509@web.de","threadId":"29385","inReplyTo":"4F15C22C.3020902@metanate.com","subject":"Re: Interactive rebase with submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-01-17T21:29:33Z","receivedAt":"2012-01-17T21:29:33Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 17.01.2012 19:47, schrieb John Keeping:\n> I've encountered a scenario where git rebase --interactive drops a commit which contains a modification to a submodule but no other changes.\n> \n> This occurs when there is a conflict when applying the commit (for example if the submodule's history has been rewritten and you are rewriting the parent repository to match the new version of the submodule).\n> \n> To clarify:\n> \n> git rebase -i\n> # Edit a commit, switching submodule to an unrelated commit\n> git rebase --continue\n> # Conflict in submodule, checkout the correct submodule commit\n> git add path/to/submodule\n> # Only change in index is updated submodule\n> git rebase --continue\n> # No commit is created for the submodule change\n> \n> \n> This appears to be because the git-rebase--interactive script inspects whether there is anything to commit when `rebase --continue` is invoked by running:\n> \n>     git diff-index --cached --quiet --ignore-submodules HEAD --\n\nThanks for pinning that down.\n\n> Is there a reason for the `--ignore-submodules` in this command? Removing that option results in the expected behaviour.\n\nYes, removing it will help your use case but break others. The reason\nfor that is that because submodules are not updated during a rebase\nit doesn't make sense to compare their HEAD to what is recorded in\nthe superproject, as that might have been changed by an earlier\ncommit. And as the submodules HEAD hasn't been updated back then,\nit is stale and will always show up as modified (even if it wasn't).\n\n> I can understand not updating submodules while running the rebase, but I expected that having resolved a conflict and added my change to the index it would be applied by `git rebase --continue`, as indeed it is if there happen to be other (non-submodule) changes in the same commit.\n\nThe irony is that you would have to update submodules (or at least\ntheir HEAD and use \"--ignore-submodules=dirty\") while running rebase\nto make that work in all cases ;-)\n\nBut just updating the HEAD would be dangerous as you would have to be\nvery careful to restore the submodules HEAD after the rebase, or the\nsubmodule's work tree will be out of sync.\n\nI suspect in the long run a rebase should, e.g. when invoked with\n--recurse-submodules, update the submodules too and won't use the\n--ignore-submodule option for diff anymore ... then everything\nshould Just Work. But until that happens, I don't see a solution\nfor your problem.\n"},{"id":"182723","messageId":"4F16AB2E.30706@metanate.com","threadId":"29385","inReplyTo":"4F15E83D.10509@web.de","subject":"Re: Interactive rebase with submodules","fromName":"John Keeping","fromEmail":"john@metanate.com","sentAt":"2012-01-18T11:21:18Z","receivedAt":"2012-01-18T11:21:18Z","isPatch":false,"sender":{"key":"john@metanate.com","avatar":null},"body":"On 17/01/12 21:29, Jens Lehmann wrote:\n> Am 17.01.2012 19:47, schrieb John Keeping:\n>> This appears to be because the git-rebase--interactive script inspects whether there is anything to commit when `rebase --continue` is invoked by running:\n>>\n>>      git diff-index --cached --quiet --ignore-submodules HEAD --\n >\n>> Is there a reason for the `--ignore-submodules` in this command? Removing that option results in the expected behaviour.\n>\n> Yes, removing it will help your use case but break others. The reason\n> for that is that because submodules are not updated during a rebase\n> it doesn't make sense to compare their HEAD to what is recorded in\n> the superproject, as that might have been changed by an earlier\n> commit. And as the submodules HEAD hasn't been updated back then,\n> it is stale and will always show up as modified (even if it wasn't).\n\nIs this worse than the current behaviour?  If I perform a rebase where \nthere is a (non-submodule) conflict in a commit where a submodule \nchanges I can see something like:\n\n# Changes to be committed:\n#     modified:    path/to/submodule\n#\n# Unmerged paths:\n#     both modified:      path/to/file\n#\n# Changes not staged for commit:\n#     modified:    path/to/submodule (new commits)\n\nThis occurs if a later commit in the rebase will modify the submodule. \nIn this case, `rebase --continue` correctly recreates the commit once I \nhave resolved the conflict in the file, ignoring the unstaged submodule \nchanges.\n\n>> I can understand not updating submodules while running the rebase, but I expected that having resolved a conflict and added my change to the index it would be applied by `git rebase --continue`, as indeed it is if there happen to be other (non-submodule) changes in the same commit.\n>\n> The irony is that you would have to update submodules (or at least\n> their HEAD and use \"--ignore-submodules=dirty\") while running rebase\n> to make that work in all cases ;-)\n\nI don't this this is the case, since diff-tree is being invoked with \n--cached won't it ignore changes in the work tree anyway?\n\n> But just updating the HEAD would be dangerous as you would have to be\n> very careful to restore the submodules HEAD after the rebase, or the\n> submodule's work tree will be out of sync.\n\nJust updating HEAD in the submodule without touching its work tree \ndoesn't seem like a good idea.  I think it will cause a lot more \nconfusion when running `git status` which will show unexpected modified \ncontent for the submodule.\n\nSince I did not expect rebase to perform a submodule update, I was not \nsurprised to see unstaged submodule changes when rebasing, but I did \nexpect rebase to commit anything I had added to the index.\n\n\n-- \nJohn\n"},{"id":"182738","messageId":"4F172DBB.4080303@web.de","threadId":"29385","inReplyTo":"4F16AB2E.30706@metanate.com","subject":"Re: Interactive rebase with submodules","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-01-18T20:38:19Z","receivedAt":"2012-01-18T20:38:19Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 18.01.2012 12:21, schrieb John Keeping:\n> On 17/01/12 21:29, Jens Lehmann wrote:\n>> Am 17.01.2012 19:47, schrieb John Keeping:\n>>> This appears to be because the git-rebase--interactive script inspects whether there is anything to commit when `rebase --continue` is invoked by running:\n>>>\n>>>      git diff-index --cached --quiet --ignore-submodules HEAD --\n>>\n>>> Is there a reason for the `--ignore-submodules` in this command? Removing that option results in the expected behaviour.\n>>\n>> Yes, removing it will help your use case but break others. The reason\n>> for that is that because submodules are not updated during a rebase\n>> it doesn't make sense to compare their HEAD to what is recorded in\n>> the superproject, as that might have been changed by an earlier\n>> commit. And as the submodules HEAD hasn't been updated back then,\n>> it is stale and will always show up as modified (even if it wasn't).\n> \n> Is this worse than the current behaviour?\n\nIf we break established behavior that would be worse unless we have\na *very* good reason to do so. But please see below ...\n\n>  If I perform a rebase where there is a (non-submodule) conflict in a commit where a submodule changes I can see something like:\n> \n> # Changes to be committed:\n> #     modified:    path/to/submodule\n> #\n> # Unmerged paths:\n> #     both modified:      path/to/file\n> #\n> # Changes not staged for commit:\n> #     modified:    path/to/submodule (new commits)\n> \n> This occurs if a later commit in the rebase will modify the submodule. In this case, `rebase --continue` correctly recreates the commit once I have resolved the conflict in the file, ignoring the unstaged submodule changes.\n\nYeah, as rebase doesn't touch the submodules, that is expected.\n\n>>> I can understand not updating submodules while running the rebase, but I expected that having resolved a conflict and added my change to the index it would be applied by `git rebase --continue`, as indeed it is if there happen to be other (non-submodule) changes in the same commit.\n>>\n>> The irony is that you would have to update submodules (or at least\n>> their HEAD and use \"--ignore-submodules=dirty\") while running rebase\n>> to make that work in all cases ;-)\n> \n> I don't this this is the case, since diff-tree is being invoked with --cached won't it ignore changes in the work tree anyway?\n\nRight, thanks for nudging me with the clue bat ...\n\nI missed the \"--cached\" option and did not question if the code does\nwhat the commit message of 6848d58c6 (where the --ignore-submodules\noption was introduced) said:\n\n    Ignore dirty submodule states during rebase and stash\n\n    When rebasing or stashing, chances are that you do not care about\n    dirty submodules, since they are not updated by those actions anyway.\n    So ignore the submodules' states.\n\n    Note: the submodule states -- as committed in the superproject --\n    will still be stashed and rebased, it is _just_ the state of the\n    submodule in the working tree which is ignored.\n\nI think this logic misses the case when only submodule changes are left\nin a commit.\n\n>> But just updating the HEAD would be dangerous as you would have to be\n>> very careful to restore the submodules HEAD after the rebase, or the\n>> submodule's work tree will be out of sync.\n> \n> Just updating HEAD in the submodule without touching its work tree doesn't seem like a good idea.  I think it will cause a lot more confusion when running `git status` which will show unexpected modified content for the submodule.\n\nYes, we agree here.\n\n> Since I did not expect rebase to perform a submodule update, I was not surprised to see unstaged submodule changes when rebasing, but I did expect rebase to commit anything I had added to the index.\n\nRight.\n\nI'll have to add some tests for that case, but I doubt I'll manage that\ntoday. Until I can provide a complete patch, this diff should fix your\nproblem (no, I did not test if that change is enough to fix the problem,\nbut at least it does not break the test suite ;-):\n\n---------------8<--------------\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 5812222..4546749 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -672,7 +672,7 @@ rearrange_squash () {\n case \"$action\" in\n continue)\n        # do we have anything to commit?\n-       if git diff-index --cached --quiet --ignore-submodules HEAD --\n+       if git diff-index --cached --quiet HEAD --\n        then\n                : Nothing to commit -- skip this\n        else\n"},{"id":"182782","messageId":"4F17F505.5030204@metanate.com","threadId":"29385","inReplyTo":"4F172DBB.4080303@web.de","subject":"Re: Interactive rebase with submodules","fromName":"John Keeping","fromEmail":"john@metanate.com","sentAt":"2012-01-19T10:48:37Z","receivedAt":"2012-01-19T10:48:37Z","isPatch":false,"sender":{"key":"john@metanate.com","avatar":null},"body":"On 18/01/12 20:38, Jens Lehmann wrote:\n> Am 18.01.2012 12:21, schrieb John Keeping:\n>> On 17/01/12 21:29, Jens Lehmann wrote:\n>>> Am 17.01.2012 19:47, schrieb John Keeping:\n>>>> I can understand not updating submodules while running the rebase, but I expected that having resolved a conflict and added my change to the index it would be applied by `git rebase --continue`, as indeed it is if there happen to be other (non-submodule) changes in the same commit.\n>>>\n>>> The irony is that you would have to update submodules (or at least\n>>> their HEAD and use \"--ignore-submodules=dirty\") while running rebase\n>>> to make that work in all cases ;-)\n>>\n>> I don't this this is the case, since diff-tree is being invoked with --cached won't it ignore changes in the work tree anyway?\n>\n> Right, thanks for nudging me with the clue bat ...\n>\n> I missed the \"--cached\" option and did not question if the code does\n> what the commit message of 6848d58c6 (where the --ignore-submodules\n> option was introduced) said:\n>\n>      Ignore dirty submodule states during rebase and stash\n>\n>      When rebasing or stashing, chances are that you do not care about\n>      dirty submodules, since they are not updated by those actions anyway.\n>      So ignore the submodules' states.\n>\n>      Note: the submodule states -- as committed in the superproject --\n>      will still be stashed and rebased, it is _just_ the state of the\n>      submodule in the working tree which is ignored.\n>\n> I think this logic misses the case when only submodule changes are left\n> in a commit.\n\nYes, this is precisely the case that doesn't work as I expect.\n\n>>> But just updating the HEAD would be dangerous as you would have to be\n>>> very careful to restore the submodules HEAD after the rebase, or the\n>>> submodule's work tree will be out of sync.\n>>\n>> Just updating HEAD in the submodule without touching its work tree doesn't seem like a good idea.  I think it will cause a lot more confusion when running `git status` which will show unexpected modified content for the submodule.\n>\n> Yes, we agree here.\n>\n>> Since I did not expect rebase to perform a submodule update, I was not surprised to see unstaged submodule changes when rebasing, but I did expect rebase to commit anything I had added to the index.\n>\n> Right.\n>\n> I'll have to add some tests for that case, but I doubt I'll manage that\n> today. Until I can provide a complete patch, this diff should fix your\n> problem (no, I did not test if that change is enough to fix the problem,\n> but at least it does not break the test suite ;-):\n>\n> ---------------8<--------------\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index 5812222..4546749 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -672,7 +672,7 @@ rearrange_squash () {\n>   case \"$action\" in\n>   continue)\n>          # do we have anything to commit?\n> -       if git diff-index --cached --quiet --ignore-submodules HEAD --\n> +       if git diff-index --cached --quiet HEAD --\n>          then\n>                  : Nothing to commit -- skip this\n>          else\n\nThis patch does indeed fix the problem I was seeing.\n\n\nThanks,\n\n-- \nJohn\n"}]}