{"thread":{"id":"30678","subject":"Bug in git citool when staging symlink as replacement for submodule","startedAt":"2012-06-01T10:31:26Z","lastAt":"2012-06-12T21:18:05Z","messageCount":7,"participants":["Adam Spiers","Heiko Voigt","Junio C Hamano","Jens Lehmann"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"192647","messageId":"CAOkDyE9q1n=oLoEXXxurDjNM0B2+cZ9eoeGE57F9hEQUjK0hZg@mail.gmail.com","threadId":"30678","inReplyTo":null,"subject":"Bug in git citool when staging symlink as replacement for submodule","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2012-06-01T10:31:26Z","receivedAt":"2012-06-01T10:31:26Z","isPatch":false,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"I found some strange behaviour in git citool when trying to stage a\nsymlink into the index as a replacement for a (non-registered) submodule.\nI just reproduced with latest master (1.7.11.rc0.55.gb2478aa):\n\n    mkdir repo\n    cd repo\n    git init\n    echo foo > one\n    git add one\n    git commit -m'first commit'\n\n    mkdir two\n    cd two\n    git init\n    echo foo > submodule-file\n    git add submodule-file\n    git commit -m'first submodule commit'\n\n    cd ..\n\n    git add two\n    git commit -m'second commit'\n\n    rm -rf two\n    ln -s one two\n\n    git citool\n\nAt this point, git citool outputs:\n\n    error: Unhandled submodule diff marker: {@@ }\n    error: Unhandled submodule diff marker: {+on}\n\nNow if I press Control-T to try to stage 'two' into the index, I get a new\ndialog which says:\n\n    Updating the Git index failed. A rescan will be automatically\n    started to resynchronize git-gui.\n\n    error: two is already a gitlink, not replacing\n    fatal: Unable to process path two\n\n\n                              [Unlock Index] [Continue]\n\nI can work around via 'git add two', but it would be nice if citool\nhandled this correctly.\n\nThanks!\nAdam\n"},{"id":"193207","messageId":"20120609134729.GA16268@book.hvoigt.net","threadId":"30678","inReplyTo":"CAOkDyE9q1n=oLoEXXxurDjNM0B2+cZ9eoeGE57F9hEQUjK0hZg@mail.gmail.com","subject":"[PATCH] git-gui: recognize submodule diff when replaced by file","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-06-09T13:47:29Z","receivedAt":"2012-06-09T13:47:29Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"When coloring the diff in submodule mode we previously just looked\nfor the submodule change markers and not normal diff markers. Since\na submodule can be replaced by a file and vice versa lets also support\nhunk, + and - lines.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\nHi,\n\nOn Fri, Jun 01, 2012 at 11:31:26AM +0100, Adam Spiers wrote:\n> At this point, git citool outputs:\n> \n>     error: Unhandled submodule diff marker: {@@ }\n>     error: Unhandled submodule diff marker: {+on}\n\nThis fixes that error message (and is hopefully doing the right thing).\n\nThere will be another patch fixing the error popup.\n\nCheers Heiko\n\n git-gui/lib/diff.tcl | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/git-gui/lib/diff.tcl b/git-gui/lib/diff.tcl\nindex ec44055..a3c997b 100644\n--- a/git-gui/lib/diff.tcl\n+++ b/git-gui/lib/diff.tcl\n@@ -467,8 +467,16 @@ proc read_diff {fd conflict_size cont_info} {\n \t\t\t\t{  >} {set tags d_+}\n \t\t\t\t{  W} {set tags {}}\n \t\t\t\tdefault {\n-\t\t\t\t\tputs \"error: Unhandled submodule diff marker: {$op}\"\n-\t\t\t\t\tset tags {}\n+\t\t\t\t\tset op [string index $line 0]\n+\t\t\t\t\tswitch -- $op {\n+\t\t\t\t\t\t{-} {set tags d_-}\n+\t\t\t\t\t\t{+} {set tags d_+}\n+\t\t\t\t\t\t{@} {set tags d_@}\n+\t\t\t\t\t\tdefault {\n+\t\t\t\t\t\t\tputs \"error: Unhandled submodule diff marker: {$op}\"\n+\t\t\t\t\t\t\tset tags {}\n+\t\t\t\t\t\t}\n+\t\t\t\t\t}\n \t\t\t\t}\n \t\t\t\t}\n \t\t\t}\n-- \n1.7.10.2.522.g93b45fe\n"},{"id":"193208","messageId":"20120609142658.GB16268@book.hvoigt.net","threadId":"30678","inReplyTo":"CAOkDyE9q1n=oLoEXXxurDjNM0B2+cZ9eoeGE57F9hEQUjK0hZg@mail.gmail.com","subject":"[PATCH] update-index: allow overwriting existing submodule index entries","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-06-09T14:27:00Z","receivedAt":"2012-06-09T14:27:00Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"In commit e01105 Linus introduced gitlinks to update-index. He explains\nthat he thinks it is not the right thing to replace a gitlink with\nsomething else.\n\nThat commit is from the very first beginnings of submodule support.\nSince then we have gotten a lot closer to being able to remove a\nsubmodule without losing its history. This check prevents such a use\ncase, so I think this assumption has changed.\n\nAdditionally in the git add codepath we do not have such a check, so for\nconsistency reasons I think removing this check is the correct thing to\ndo.\n\nSigned-off-by: Heiko Voigt <hvoigt@hvoigt.net>\n---\nHi,\n\nOn Fri, Jun 01, 2012 at 11:31:26AM +0100, Adam Spiers wrote:\n> Now if I press Control-T to try to stage 'two' into the index, I get a new\n> dialog which says:\n> \n>     Updating the Git index failed. A rescan will be automatically\n>     started to resynchronize git-gui.\n> \n>     error: two is already a gitlink, not replacing\n>     fatal: Unable to process path two\n> \n> \n>                               [Unlock Index] [Continue]\n> \n> I can work around via 'git add two', but it would be nice if citool\n> handled this correctly.\n\nThis is because git gui uses plumbing (update-index) for updating the\nindex.\n\nThis patch fixes that popup. Testsuite passes here.\n\nIs it ok this way?\n\nCheers Heiko\n\n builtin/update-index.c | 6 ------\n 1 file changed, 6 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 5f038d6..5a4e9ea 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -211,12 +211,6 @@ static int process_path(const char *path)\n \tif (S_ISDIR(st.st_mode))\n \t\treturn process_directory(path, len, &st);\n \n-\t/*\n-\t * Process a regular file\n-\t */\n-\tif (ce && S_ISGITLINK(ce->ce_mode))\n-\t\treturn error(\"%s is already a gitlink, not replacing\", path);\n-\n \treturn add_one_path(ce, path, len, &st);\n }\n \n-- \n1.7.10.2.522.g93b45fe\n"},{"id":"193308","messageId":"7v3961ao1q.fsf@alter.siamese.dyndns.org","threadId":"30678","inReplyTo":"20120609142658.GB16268@book.hvoigt.net","subject":"Re: [PATCH] update-index: allow overwriting existing submodule index entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-11T15:03:29Z","receivedAt":"2012-06-11T15:03:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Heiko Voigt <hvoigt@hvoigt.net> writes:\n\n> In commit e01105 Linus introduced gitlinks to update-index. He explains\n> that he thinks it is not the right thing to replace a gitlink with\n> something else.\n>\n> That commit is from the very first beginnings of submodule support.\n> Since then we have gotten a lot closer to being able to remove a\n> submodule without losing its history. This check prevents such a use\n> case, so I think this assumption has changed.\n\nYeah, I think we should remove it if only to make it consistent with\n\"add\" (if anything, the Porcelain level \"add\" should be the one that\nis more strict and the plumbing level should be able to let you\nshoot in the foot, not the other way around), but we need to make\nsure \"closer to\" becomes reality.  Can we remove and the resurrect\nautomatically when checking out a branch with a submodule when you\nare on a branch with a directory and vice versa?\n"},{"id":"193386","messageId":"4FD661C3.7000105@web.de","threadId":"30678","inReplyTo":"7v3961ao1q.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] update-index: allow overwriting existing submodule index entries","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-06-11T21:23:15Z","receivedAt":"2012-06-11T21:23:15Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 11.06.2012 17:03, schrieb Junio C Hamano:\n> Heiko Voigt <hvoigt@hvoigt.net> writes:\n> \n>> In commit e01105 Linus introduced gitlinks to update-index. He explains\n>> that he thinks it is not the right thing to replace a gitlink with\n>> something else.\n>>\n>> That commit is from the very first beginnings of submodule support.\n>> Since then we have gotten a lot closer to being able to remove a\n>> submodule without losing its history. This check prevents such a use\n>> case, so I think this assumption has changed.\n> \n> Yeah, I think we should remove it if only to make it consistent with\n> \"add\" (if anything, the Porcelain level \"add\" should be the one that\n> is more strict and the plumbing level should be able to let you\n> shoot in the foot, not the other way around), but we need to make\n> sure \"closer to\" becomes reality. Can we remove and the resurrect\n> automatically when checking out a branch with a submodule when you\n> are on a branch with a directory and vice versa?\n\nEven while I suspect most of the time a submodule <=> directory\ntransition will occur (moving directory content into a submodule\nor vice versa; and then there will be no replacement of a gitlink\nwith something else as only the files inside the directory will be\nrecorded) there is no reason why submodule <=> file or submodule\n<=> link transitions shouldn't work just fine. So yes, we can ;-)\n(See the recursive_submodule_checkout branch in my GitHub repo for\ncurrent state of affairs, even though not all transitions work yet\nsome do just fine)\n\nIf you want I can keep this patch in my GitHub repo until \"closer\nto\" becomes reality. Me too thinks that plumbing should not enforce\nthis restriction, but I wouldn't mind to hold this patch back until\nthen.\n"},{"id":"193500","messageId":"20120612203316.GA17053@book.hvoigt.net","threadId":"30678","inReplyTo":"4FD661C3.7000105@web.de","subject":"Re: [PATCH] update-index: allow overwriting existing submodule index entries","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2012-06-12T20:33:19Z","receivedAt":"2012-06-12T20:33:19Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Mon, Jun 11, 2012 at 11:23:15PM +0200, Jens Lehmann wrote:\n> Am 11.06.2012 17:03, schrieb Junio C Hamano:\n> > Heiko Voigt <hvoigt@hvoigt.net> writes:\n> > \n> >> In commit e01105 Linus introduced gitlinks to update-index. He explains\n> >> that he thinks it is not the right thing to replace a gitlink with\n> >> something else.\n> >>\n> >> That commit is from the very first beginnings of submodule support.\n> >> Since then we have gotten a lot closer to being able to remove a\n> >> submodule without losing its history. This check prevents such a use\n> >> case, so I think this assumption has changed.\n> > \n> > Yeah, I think we should remove it if only to make it consistent with\n> > \"add\" (if anything, the Porcelain level \"add\" should be the one that\n> > is more strict and the plumbing level should be able to let you\n> > shoot in the foot, not the other way around), but we need to make\n> > sure \"closer to\" becomes reality. Can we remove and the resurrect\n> > automatically when checking out a branch with a submodule when you\n> > are on a branch with a directory and vice versa?\n> \n> Even while I suspect most of the time a submodule <=> directory\n> transition will occur (moving directory content into a submodule\n> or vice versa; and then there will be no replacement of a gitlink\n> with something else as only the files inside the directory will be\n> recorded) there is no reason why submodule <=> file or submodule\n> <=> link transitions shouldn't work just fine. So yes, we can ;-)\n> (See the recursive_submodule_checkout branch in my GitHub repo for\n> current state of affairs, even though not all transitions work yet\n> some do just fine)\n\nThis is what works currently with submodule update in master:\n\nfile => submodule\nsymlink => submodule\ndirectory => submodule\n\tThese are the simplest cases. It currently works fine with\n\tsubmodule update. The checkout will remove the files of the\n\tdirectory and submodule update will just populate the submodule\n\tif it needs to.\n\n\nsubmodule => file\nsubmodule => symlink\nsubmodule => directory\n\tThese ones are the trickier ones.\n\tA checkout would currently not touch the submodule. It will be\n\tleft in place. I have just tried it and in case the same files\n\texist checkout will stop with an error. If there are no\n\toverlapping files it will happily checkout the files.  Now\n\tstatus displays the files contained in the submodule as\n\tuntracked.\n\n\tThis behavior could use some improvement. *)\n\n\tSo if we were to implement submodule removal with submodule\n\tupdate the first problem here is when transitioning from a\n\tsubmodule to files we first should skip checking out files from\n\tthat path.\n\n\tThen when using submodule update we need to detect whether a\n\tpaths situation was brought to you by a checkout. Remember if\n\tthe files are skipped by checkout the submodule is still in place.\n\tComparing the working copy with whats in the database looks\n\tas if the user has replaced the directory/file with a submodule.\n\n\tTo cleanup the situation with submodule update we could just do\n\tsome security checks**) and in case they are successful remove\n\tthe submodule and checkout the files.\n\n\tOne real problem I see here is when displaying status if some\n\tfiles checkout has been skipped. Now you will potentially\n\t(directory case) see a lot of files looking as if they were\n\tdeleted.\n\n*) I also found that replacing a submodule with a directory using git\nadd does not work directly. The directory and it files will simply be\nignored by add.\n\n**) Check that the .git is a gitfile that points outside of the\nsubmodules directory. Check that there are no untracked changes in the\nsubmodule.\n\nThe current plan to solve the submodule => file(s) transition is to\nextend checkout with a --recurse-submodules option and then let it do\nthe above transition during checkout.\n\nBut unless we are going to have recursive checkout for populated\nsubmodules always on (no config) we need to be able to deal with the\nnon-recursive outdated submodules situation.\n\nHere a quick idea of what we could do:\n\nWe could mark a path as transitioned from submodule (similar to the\nassume unchanged bit) if files were skipped due to removal of a\nsubmodule and have submodule update clear that flag. That way we could\nteach diff, status and so on to only show one entry for a submodule to\nbe removed and replaced with something else.\n\nThinking further: We could actually prevent adding such an out-of-date\nsubmodule as a safeguard. Which in fact was something which happened by\nmistake to some of our users. The story is that when you see a *changed*\nsubmodule in a merge conflict it can be easily mistaken for needing\nresolution and added to the merge. Most times this rewinds your\nsubmodule to some old state\n\nIf we agree on how to handle the submodule => file(s) cases I think the\nimplementation in the submodule script possibly requires less work than\nthe fully fledged recursive checkout and could also be used for gaining\nsome experience with such transitions.\n\nSo the first step would be to extend submodule update to support the\nremoval of submodules. The next step is then that we finish the fully\nautomatic recursive submodule checkout.\n\nWhat do you think of such a two step plan?\n\nCheers Heiko\n"},{"id":"193511","messageId":"4FD7B20D.9080803@web.de","threadId":"30678","inReplyTo":"20120612203316.GA17053@book.hvoigt.net","subject":"Re: [PATCH] update-index: allow overwriting existing submodule index entries","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-06-12T21:18:05Z","receivedAt":"2012-06-12T21:18:05Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 12.06.2012 22:33, schrieb Heiko Voigt:\n> Here a quick idea of what we could do:\n> \n> We could mark a path as transitioned from submodule (similar to the\n> assume unchanged bit) if files were skipped due to removal of a\n> submodule and have submodule update clear that flag. That way we could\n> teach diff, status and so on to only show one entry for a submodule to\n> be removed and replaced with something else.\n> \n> Thinking further: We could actually prevent adding such an out-of-date\n> submodule as a safeguard. Which in fact was something which happened by\n> mistake to some of our users. The story is that when you see a *changed*\n> submodule in a merge conflict it can be easily mistaken for needing\n> resolution and added to the merge. Most times this rewinds your\n> submodule to some old state\n> \n> If we agree on how to handle the submodule => file(s) cases I think the\n> implementation in the submodule script possibly requires less work than\n> the fully fledged recursive checkout and could also be used for gaining\n> some experience with such transitions.\n> \n> So the first step would be to extend submodule update to support the\n> removal of submodules. The next step is then that we finish the fully\n> automatic recursive submodule checkout.\n> \n> What do you think of such a two step plan?\n\nHmm, I suspect the real problem here is that \"git submodule update\" is\nrun *after* the actual checkout, merge, reset or whatever. So if for\nexample you want to replace a submodule with a plain directory containing\nthe same files the submodule update would not only have to remove the now\nobsolete submodule but also has to remember to check out all files in the\nformer submodule directory again. IMO this should be part of the initial\nunpack_trees(), not done in a later script. Imagine a submodule having\nlocal modifications which would be overwritten by the checkout, then the\nlater submodule update would have to fail (when used without -f) to\nprotect the users changes. The only sane thing to do would be to not allow\nsuch a checkout in the first place (when the user chose to automatically\nupdate submodules that is) but abort just like we do for changes to\nregular files right now telling the user to save his changes.\n\nAnd I suspect you would have to teach at least status and diff to give\nmeaningful output in that \"submodule should be removed and replaced with\nsomething else but submodule update hasn't run yet\" case, and apart from\nthe change to add you mentioned above some other commands might need\nsafeguards too.\n\nSo me thinks we should skip step one and implement recursive checkout\nright away. Adding much more complexity to the submodule script for an\nintermediate solution doesn't sound like a good idea to me.\n"}]}