{"thread":{"id":"9077","subject":"[PATCH] unpack-trees.c: assume submodules are clean during check-out","startedAt":"2007-07-17T18:28:28Z","lastAt":"2007-08-08T11:39:52Z","messageCount":16,"participants":["Sven Verdoolaege","Junio C Hamano","Lars Hjemli","Eran Tromer"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"47658","messageId":"20070717182828.GA4583MdfPADPa@greensroom.kotnet.org","threadId":"9077","inReplyTo":null,"subject":"[PATCH] unpack-trees.c: assume submodules are clean during check-out","fromName":"Sven Verdoolaege","fromEmail":"skimo@kotnet.org","sentAt":"2007-07-17T18:28:28Z","receivedAt":"2007-07-17T18:28:28Z","isPatch":true,"sender":{"key":"skimo@kotnet.org","avatar":null},"body":"In particular, when moving back to a commit without a given submodule\nand then moving back forward to a commit with the given submodule,\nwe shouldn't complain that updating would lose untracked file in\nthe submodule, because git currently does not checkout subprojects\nduring superproject check-out.\n\nSigned-off-by: Sven Verdoolaege \n---\n\nI'm not sure if t7400-submodule-basic is the best place\nfor the test, but it has the right context for the test.\n\nskimo\n\n t/t7400-submodule-basic.sh | 9 +++++\n unpack-trees.c | 75 +++++++++++++++++++++++++++++--------------\n 2 files changed, 59 insertions(+), 25 deletions(-)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 5e91db6..e8ce7cd 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -21,6 +21,10 @@ subcommands of git-submodule.\n # -add an entry to .gitmodules for submodule 'example'\n #\n test_expect_success 'Prepare submodule testing' '\n+\t: > t &&\n+\tgit-add t &&\n+\tgit-commit -m \"initial commit\" &&\n+\tgit branch initial HEAD &&\n \tmkdir lib &&\n \tcd lib &&\n \tgit init &&\n@@ -166,4 +170,9 @@ test_expect_success 'status should be \"up-to-date\" after update' '\n \tgit-submodule status | grep \"^ $rev1\"\n '\n \n+test_expect_success 'checkout superproject with subproject already present' '\n+\tgit-checkout initial &&\n+\tgit-checkout master\n+'\n+\n test_done\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 89dd279..7cc029e 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -5,6 +5,7 @@\n #include \"cache-tree.h\"\n #include \"unpack-trees.h\"\n #include \"progress.h\"\n+#include \"refs.h\"\n \n #define DBRT_DEBUG 1\n \n@@ -425,11 +426,24 @@ static void invalidate_ce_path(struct cache_entry *ce)\n \t\tcache_tree_invalidate_path(active_cache_tree, ce->name);\n }\n \n-static int verify_clean_subdirectory(const char *path, const char *action,\n+/*\n+ * Check that checking out ce->sha1 in subdir ce->name is not\n+ * going to overwrite any working files.\n+ *\n+ * Currently, git does not checkout subprojects during a superproject\n+ * checkout, so it is not going to overwrite anything.\n+ */\n+static int verify_clean_submodule(struct cache_entry *ce, const char *action,\n+\t\t\t\t struct unpack_trees_options *o)\n+{\n+\treturn 0;\n+}\n+\n+static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,\n \t\t\t\t struct unpack_trees_options *o)\n {\n \t/*\n-\t * we are about to extract \"path\"; we would not want to lose\n+\t * we are about to extract \"ce->name\"; we would not want to lose\n \t * anything in the existing directory there.\n \t */\n \tint namelen;\n@@ -437,13 +451,24 @@ static int verify_clean_subdirectory(const char *path, const char *action,\n \tstruct dir_struct d;\n \tchar *pathbuf;\n \tint cnt = 0;\n+\tunsigned char sha1[20];\n+\n+\tif (S_ISGITLINK(ntohl(ce->ce_mode)) &&\n+\t resolve_gitlink_ref(ce->name, \"HEAD\", sha1) == 0) {\n+\t\t/* If we are not going to update the submodule, then\n+\t\t * we don't care.\n+\t\t */\n+\t\tif (!hashcmp(sha1, ce->sha1))\n+\t\t\treturn 0;\n+\t\treturn verify_clean_submodule(ce, action, o);\n+\t}\n \n \t/*\n \t * First let's make sure we do not have a local modification\n \t * in that directory.\n \t */\n-\tnamelen = strlen(path);\n-\tpos = cache_name_pos(path, namelen);\n+\tnamelen = strlen(ce->name);\n+\tpos = cache_name_pos(ce->name, namelen);\n \tif (0 <= pos)\n \t\treturn cnt; /* we have it as nondirectory */\n \tpos = -pos - 1;\n@@ -451,7 +476,7 @@ static int verify_clean_subdirectory(const char *path, const char *action,\n \t\tstruct cache_entry *ce = active_cache[i];\n \t\tint len = ce_namelen(ce);\n \t\tif (len < namelen ||\n-\t\t strncmp(path, ce->name, namelen) ||\n+\t\t strncmp(ce->name, ce->name, namelen) ||\n \t\t ce->name[namelen] != '/')\n \t\t\tbreak;\n \t\t/*\n@@ -469,16 +494,16 @@ static int verify_clean_subdirectory(const char *path, const char *action,\n \t * present file that is not ignored.\n \t */\n \tpathbuf = xmalloc(namelen + 2);\n-\tmemcpy(pathbuf, path, namelen);\n+\tmemcpy(pathbuf, ce->name, namelen);\n \tstrcpy(pathbuf+namelen, \"/\");\n \n \tmemset(&d, 0, sizeof(d));\n \tif (o->dir)\n \t\td.exclude_per_dir = o->dir->exclude_per_dir;\n-\ti = read_directory(&d, path, pathbuf, namelen+1, NULL);\n+\ti = read_directory(&d, ce->name, pathbuf, namelen+1, NULL);\n \tif (i)\n \t\tdie(\"Updating '%s' would lose untracked files in it\",\n-\t\t path);\n+\t\t ce->name);\n \tfree(pathbuf);\n \treturn cnt;\n }\n@@ -487,7 +512,7 @@ static int verify_clean_subdirectory(const char *path, const char *action,\n * We do not want to remove or overwrite a working tree file that\n * is not tracked, unless it is ignored.\n */\n-static void verify_absent(const char *path, const char *action,\n+static void verify_absent(struct cache_entry *ce, const char *action,\n \t\tstruct unpack_trees_options *o)\n {\n \tstruct stat st;\n@@ -495,15 +520,15 @@ static void verify_absent(const char *path, const char *action,\n \tif (o->index_only || o->reset || !o->update)\n \t\treturn;\n \n-\tif (has_symlink_leading_path(path, NULL))\n+\tif (has_symlink_leading_path(ce->name, NULL))\n \t\treturn;\n \n-\tif (!lstat(path, &st)) {\n+\tif (!lstat(ce->name, &st)) {\n \t\tint cnt;\n \n-\t\tif (o->dir && excluded(o->dir, path))\n+\t\tif (o->dir && excluded(o->dir, ce->name))\n \t\t\t/*\n-\t\t\t * path is explicitly excluded, so it is Ok to\n+\t\t\t * ce->name is explicitly excluded, so it is Ok to\n \t\t\t * overwrite it.\n \t\t\t */\n \t\t\treturn;\n@@ -515,7 +540,7 @@ static void verify_absent(const char *path, const char *action,\n \t\t\t * files that are in \"foo/\" we would lose\n \t\t\t * it.\n \t\t\t */\n-\t\t\tcnt = verify_clean_subdirectory(path, action, o);\n+\t\t\tcnt = verify_clean_subdirectory(ce, action, o);\n \n \t\t\t/*\n \t\t\t * If this removed entries from the index,\n@@ -543,7 +568,7 @@ static void verify_absent(const char *path, const char *action,\n \t\t * delete this path, which is in a subdirectory that\n \t\t * is being replaced with a blob.\n \t\t */\n-\t\tcnt = cache_name_pos(path, strlen(path));\n+\t\tcnt = cache_name_pos(ce->name, strlen(ce->name));\n \t\tif (0 <= cnt) {\n \t\t\tstruct cache_entry *ce = active_cache[cnt];\n \t\t\tif (!ce_stage(ce) && !ce->ce_mode)\n@@ -551,7 +576,7 @@ static void verify_absent(const char *path, const char *action,\n \t\t}\n \n \t\tdie(\"Untracked working tree file '%s' \"\n-\t\t \"would be %s by merge.\", path, action);\n+\t\t \"would be %s by merge.\", ce->name, action);\n \t}\n }\n \n@@ -575,7 +600,7 @@ static int merged_entry(struct cache_entry *merge, struct cache_entry *old,\n \t\t}\n \t}\n \telse {\n-\t\tverify_absent(merge->name, \"overwritten\", o);\n+\t\tverify_absent(merge, \"overwritten\", o);\n \t\tinvalidate_ce_path(merge);\n \t}\n \n@@ -590,7 +615,7 @@ static int deleted_entry(struct cache_entry *ce, struct cache_entry *old,\n \tif (old)\n \t\tverify_uptodate(old, o);\n \telse\n-\t\tverify_absent(ce->name, \"removed\", o);\n+\t\tverify_absent(ce, \"removed\", o);\n \tce->ce_mode = 0;\n \tadd_cache_entry(ce, ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE);\n \tinvalidate_ce_path(ce);\n@@ -707,18 +732,18 @@ int threeway_merge(struct cache_entry **stages,\n \tif (o->aggressive) {\n \t\tint head_deleted = !head && !df_conflict_head;\n \t\tint remote_deleted = !remote && !df_conflict_remote;\n-\t\tconst char *path = NULL;\n+\t\tstruct cache_entry *ce = NULL;\n \n \t\tif (index)\n-\t\t\tpath = index->name;\n+\t\t\tce = index;\n \t\telse if (head)\n-\t\t\tpath = head->name;\n+\t\t\tce = head;\n \t\telse if (remote)\n-\t\t\tpath = remote->name;\n+\t\t\tce = remote;\n \t\telse {\n \t\t\tfor (i = 1; i < o->head_idx; i++) {\n \t\t\t\tif (stages[i] && stages[i] != o->df_conflict_entry) {\n-\t\t\t\t\tpath = stages[i]->name;\n+\t\t\t\t\tce = stages[i];\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t}\n@@ -733,8 +758,8 @@ int threeway_merge(struct cache_entry **stages,\n \t\t (remote_deleted && head && head_match)) {\n \t\t\tif (index)\n \t\t\t\treturn deleted_entry(index, index, o);\n-\t\t\telse if (path && !head_deleted)\n-\t\t\t\tverify_absent(path, \"removed\", o);\n+\t\t\telse if (ce && !head_deleted)\n+\t\t\t\tverify_absent(ce, \"removed\", o);\n \t\t\treturn 0;\n \t\t}\n \t\t/*\n-- \n1.5.3.rc2.10.g148b\n"},{"id":"47710","messageId":"7vy7he6ufj.fsf@assigned-by-dhcp.cox.net","threadId":"9077","inReplyTo":"20070717182828.GA4583MdfPADPa@greensroom.kotnet.org","subject":"Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-18T07:29:52Z","receivedAt":"2007-07-18T07:29:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Passing of ce instead of path in the unpack-trees callchain\nlooks like the right thing to do. Good job.\n\nThanks.\n"},{"id":"49310","messageId":"20070801140532.GC31114MdfPADPa@greensroom.kotnet.org","threadId":"9077","inReplyTo":"7vy7he6ufj.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out","fromName":"Sven Verdoolaege","fromEmail":"skimo@kotnet.org","sentAt":"2007-08-01T14:05:32Z","receivedAt":"2007-08-01T14:05:32Z","isPatch":true,"sender":{"key":"skimo@kotnet.org","avatar":null},"body":"On Wed, Jul 18, 2007 at 12:29:52AM -0700, Junio C Hamano wrote:\n> Passing of ce instead of path in the unpack-trees callchain\n> looks like the right thing to do. Good job.\n\nActually, my patch only fixes the tip of the iceberg.\nIf you have a submodule checked out and you go back (or forward)\nto a revision of the supermodule that contains a different\nrevision of the submodule and then switch to another revision,\nit will complain that the submodule is not uptodate, because\ngit simply didn't update the submodule in the first move.\n\nNow, you may say that I simply need to run 'git submodule update'\nafter every such move, but this is very inconvenient, especially\nif you're doing a bisect or a rebase.\n\nHow do other people deal with this problem?\n\nHow about just replacing the body of ce_compare_gitlink\nwith \"return 0\" until git actually (optionally) updates\nthe submodules during an update of the supermodule?\n\nskimo\n"},{"id":"49662","messageId":"7v643vj316.fsf@assigned-by-dhcp.cox.net","threadId":"9077","inReplyTo":"20070801140532.GC31114MdfPADPa@greensroom.kotnet.org","subject":"Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-04T05:13:09Z","receivedAt":"2007-08-04T05:13:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sven Verdoolaege writes:\n\n> If you have a submodule checked out and you go back (or forward)\n> to a revision of the supermodule that contains a different\n> revision of the submodule and then switch to another revision,\n> it will complain that the submodule is not uptodate, because\n> git simply didn't update the submodule in the first move.\n>\n> Now, you may say that I simply need to run 'git submodule update'\n> after every such move, but this is very inconvenient, especially\n> if you're doing a bisect or a rebase.\n>\n> How do other people deal with this problem?\n>\n> How about just replacing the body of ce_compare_gitlink\n> with \"return 0\" until git actually (optionally) updates\n> the submodules during an update of the supermodule?\n\nLet me understand the problem first. If your first checkout\ndoes not check out the submodule, switching between revisions\nthat has different commit of the submodule there would not fail,\nbut once you checkout the submodule, switching without updating\nthe submodule would be Ok (because by design updating the\nsubmodule is optional) but then further switching out of that\nstate will fail because submodule in the supermodule tree and\nchecked-out submodule repository are now out of sync. Is that\nthe problem?\n\nIn any case, I doubt ce_compare_gitlink() is the right layer to\nwork this around -- it is not about \"can we switch\" but is about\n\"is it different\". It is at too low a level.\n\nThe current policy is to consider it is perfectly normal that\nchecked-out submodule is out-of-sync wrt the supermodule index,\nif I am reading you right. I think it is a good policy, at\nleast until we introduce a superproject repository configuration\noption that says \"in this repository, I do care about this\nsubmodule and at any time I move around in the superproject,\nrecursively check out the submodule to match\". The most extreme\ncase of this policy is that the superproject index knows about\nthe submodule but the subdirectory does not even have to be\nchecked out, which is what we have now.\n\nWhere does the \"No you are not up-to-date, I wouldn't let you\nswitch\" come from? Is that verify_uptodate() called from\nmerged_entry() called from twoway_merge()? I think the right\napproach to deal with this is to teach verify_uptodate() about\nthe policy. The function is about \"make sure the filesystem\nentity that corresponds to this cache entry is up to date, lest\nwe lose the local modifications\". As we explicitly allow\nsubmodule checkout to drift from the supermodule index entry,\nthe check should say \"Ok, for submodules, not matching is the\nnorm\" for now. Later when we have the ability to mark \"I care\nabout this submodule to be always in sync with the superproject\"\n(thereby implementing automatic recursive checkout and perhaps\ndiff, among other things), we should check if the submodule in\nquestion is marked as such and perform the current test.\n\nHow about doing something like this instead?\n\n unpack-trees.c | 9 +++++++++\n 1 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 3b32718..dfd985b 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -407,6 +407,15 @@ static void verify_uptodate(struct cache_entry *ce,\n \t\tunsigned changed = ce_match_stat(ce, &st, 1);\n \t\tif (!changed)\n \t\t\treturn;\n+\t\t/*\n+\t\t * NEEDSWORK: the current default policy is to allow\n+\t\t * submodule to be out of sync wrt the supermodule\n+\t\t * index. This needs to be tightened later for\n+\t\t * submodules that are marked to be automatically\n+\t\t * checked out.\n+\t\t */\n+\t\tif (S_ISGITLINK(ntohl(ce->ce_mode)))\n+\t\t\treturn;\n \t\terrno = 0;\n \t}\n \tif (errno == ENOENT)\n"},{"id":"49699","messageId":"8c5c35580708040441ue1c3ef8qc022912a5af4883e@mail.gmail.com","threadId":"9077","inReplyTo":"7v643vj316.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2007-08-04T11:41:06Z","receivedAt":"2007-08-04T11:41:06Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On 8/4/07, Junio C Hamano wrote:\n> As we explicitly allow\n> submodule checkout to drift from the supermodule index entry,\n> the check should say \"Ok, for submodules, not matching is the\n> norm\" for now. Later when we have the ability to mark \"I care\n> about this submodule to be always in sync with the superproject\"\n> (thereby implementing automatic recursive checkout and perhaps\n> diff, among other things), we should check if the submodule in\n> question is marked as such and perform the current test.\n\nYes, this sounds like a sane plan (and a good explanation of the\ncurrent semantics: maybe something to include in the release notes for\n1.5.3?)\n\nBtw: I've applied your patch to rc-4 and tested the result in my cgit\nrepo: very nice, and very ack'd ;-)\n\n--\nlarsh\n"},{"id":"49744","messageId":"46B4A350.9060806@tromer.org","threadId":"9077","inReplyTo":"7v643vj316.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out","fromName":"Eran Tromer","fromEmail":"git2eran@tromer.org","sentAt":"2007-08-04T16:03:28Z","receivedAt":"2007-08-04T16:03:28Z","isPatch":true,"sender":{"key":"git2eran@tromer.org","avatar":null},"body":"On 2007-08-04 01:13, Junio C Hamano wrote:\n> Let me understand the problem first. If your first checkout\n> does not check out the submodule, switching between revisions\n> that has different commit of the submodule there would not fail,\n> but once you checkout the submodule, switching without updating\n> the submodule would be Ok (because by design updating the\n> submodule is optional) but then further switching out of that\n> state will fail because submodule in the supermodule tree and\n> checked-out submodule repository are now out of sync. Is that\n> the problem?\n> \n[snip]\n\n> Where does the \"No you are not up-to-date, I wouldn't let you\n> switch\" come from? Is that verify_uptodate() called from\n> merged_entry() called from twoway_merge()? I think the right\n> approach to deal with this is to teach verify_uptodate() about\n> the policy. The function is about \"make sure the filesystem\n> entity that corresponds to this cache entry is up to date, lest\n> we lose the local modifications\". As we explicitly allow\n> submodule checkout to drift from the supermodule index entry,\n> the check should say \"Ok, for submodules, not matching is the\n> norm\" for now. Later when we have the ability to mark \"I care\n> about this submodule to be always in sync with the superproject\"\n> (thereby implementing automatic recursive checkout and perhaps\n> diff, among other things), we should check if the submodule in\n> question is marked as such and perform the current test.\n> \n> How about doing something like this instead?\n> \n> unpack-trees.c | 9 +++++++++\n\nWorks here: it silences the check and allows switching branches. Still,\nleaving the working tree dirty can inadvertently affect subsequent\ncommits. Consider the most ordinary of sequences:\n\n$ git checkout experimental-death-ray\n$ git submodules update\n(return a week later, woozy from the vacation.)\n$ git checkout master\n(hack hack hack)\n$ git commit -a -m \"fixed typos\"\n$ git push\n(Oops. You've just accidentally committed the wrong submodule heads.)\n\nSo to safely make new commits you must remember to always run \"git\nsubmodule update\", or forgo use of \"git commit -a\", whenever submodules\nmight be involved.\n\nI guess you can hack around this by excluding submodules from \"commit\n-a\" and (for scripts) \"ls-files -m\" too...\n\nAnother approach is for pull, checkout etc. to automatically update the\nsubmodule' head ref, but no more. In this case the supermodule always\nsees a consistent state with traditional semantics, but the *submodule*\nends up with a dirty working tree and a head referring to a\npossibly-missing commit; \"git submodule update\" would need to clean that up.\n\n Eran\n"},{"id":"49828","messageId":"7vbqdmh63q.fsf@assigned-by-dhcp.cox.net","threadId":"9077","inReplyTo":"8c5c35580708040441ue1c3ef8qc022912a5af4883e@mail.gmail.com","subject":"Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-05T06:02:01Z","receivedAt":"2007-08-05T06:02:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Lars Hjemli\" writes:\n\n> On 8/4/07, Junio C Hamano wrote:\n>> As we explicitly allow\n>> submodule checkout to drift from the supermodule index entry,\n>> the check should say \"Ok, for submodules, not matching is the\n>> norm\" for now. Later when we have the ability to mark \"I care\n>> about this submodule to be always in sync with the superproject\"\n>> (thereby implementing automatic recursive checkout and perhaps\n>> diff, among other things), we should check if the submodule in\n>> question is marked as such and perform the current test.\n>\n> Yes, this sounds like a sane plan (and a good explanation of the\n> current semantics: maybe something to include in the release notes for\n> 1.5.3?)\n\nThe submodule Porcelain is a new thing in 1.5.3, so we would\nneed a good description of what the current rules are and what\nour vision for future enhancement will be.\n\nI am not certain however the above is the accurate description\nof the current design and the direction we would want to go,\nthough. Somebody needs to sanity check me.\n"},{"id":"49829","messageId":"7v4pjeh5lv.fsf@assigned-by-dhcp.cox.net","threadId":"9077","inReplyTo":"46B4A350.9060806@tromer.org","subject":"Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-05T06:12:44Z","receivedAt":"2007-08-05T06:12:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eran Tromer writes:\n\n> Another approach is for pull, checkout etc. to automatically update the\n> submodule' head ref, but no more. In this case the supermodule always\n> sees a consistent state with traditional semantics, but the *submodule*\n> ends up with a dirty working tree and a head referring to a\n> possibly-missing commit; \"git submodule update\" would need to clean that up.\n\nThat however would introduce the same problem as \"pushing into\nlive repository to update its HEAD while the index and working\ntree are looking the other way\". We would need to somehow make\nit possible for the subdirectory to remember which commit the\nindex and the working tree are derived from, so that the local\nchanges can be merged back to the revision the submodule HEAD\npoints at.\n"},{"id":"49890","messageId":"20070805135529.GA999MdfPADPa@greensroom.kotnet.org","threadId":"9077","inReplyTo":"8c5c35580708040441ue1c3ef8qc022912a5af4883e@mail.gmail.com","subject":"Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out","fromName":"Sven Verdoolaege","fromEmail":"skimo@kotnet.org","sentAt":"2007-08-05T13:55:29Z","receivedAt":"2007-08-05T13:55:29Z","isPatch":true,"sender":{"key":"skimo@kotnet.org","avatar":null},"body":"On Fri, Aug 03, 2007 at 10:13:09PM -0700, Junio C Hamano wrote:\n> Let me understand the problem first. If your first checkout\n> does not check out the submodule, switching between revisions\n> that has different commit of the submodule there would not fail,\n> but once you checkout the submodule, switching without updating\n> the submodule would be Ok (because by design updating the\n> submodule is optional) but then further switching out of that\n> state will fail because submodule in the supermodule tree and\n> checked-out submodule repository are now out of sync. Is that\n> the problem?\n\nYes.\n\n> In any case, I doubt ce_compare_gitlink() is the right layer to\n> work this around -- it is not about \"can we switch\" but is about\n> \"is it different\". It is at too low a level.\n\nYou are right. I followed the logic down to ce_compare_gitlink,\nbut I should have backed up again.\n\n> The current policy is to consider it is perfectly normal that\n> checked-out submodule is out-of-sync wrt the supermodule index,\n> if I am reading you right. I think it is a good policy, at\n> least until we introduce a superproject repository configuration\n> option that says \"in this repository, I do care about this\n> submodule and at any time I move around in the superproject,\n> recursively check out the submodule to match\". The most extreme\n> case of this policy is that the superproject index knows about\n> the submodule but the subdirectory does not even have to be\n> checked out, which is what we have now.\n\nYes. Alex Riesen convinced me that having the submodule checked\nout is a good indication that you care about the submodule being\nin sync (although you then apparently convinced him that this is\nnot a good idea). You didn't take any patch that implements this\nyet, so I'll probably try again after you release 1.5.3.\n\nSince some people seem to like the current situation, I'd only do\nthe updating of checked-out submodules if some \"autoUpdateSubmodules\"\nis set.\n\n> How about doing something like this instead?\n\nWorks like a charm.\n\nOn Sat, Aug 04, 2007 at 01:41:06PM +0200, Lars Hjemli wrote:\n> Btw: I've applied your patch to rc-4 and tested the result in my cgit\n> repo: very nice, and very ack'd ;-)\n\nIf it matters, a definite\n\nAcked-by: Sven Verdoolaege \n\ntoo.\n\nskimo\n"},{"id":"49898","messageId":"20070805144632.GB999MdfPADPa@greensroom.kotnet.org","threadId":"9077","inReplyTo":"46B4A350.9060806@tromer.org","subject":"Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out","fromName":"Sven Verdoolaege","fromEmail":"skimo@kotnet.org","sentAt":"2007-08-05T14:46:32Z","receivedAt":"2007-08-05T14:46:32Z","isPatch":true,"sender":{"key":"skimo@kotnet.org","avatar":null},"body":"On Sat, Aug 04, 2007 at 12:03:28PM -0400, Eran Tromer wrote:\n> Works here: it silences the check and allows switching branches. Still,\n> leaving the working tree dirty can inadvertently affect subsequent\n> commits. Consider the most ordinary of sequences:\n> \n> $ git checkout experimental-death-ray\n> $ git submodules update\n> (return a week later, woozy from the vacation.)\n> $ git checkout master\n\nHere, it'll warn that your submodule isn't up-to-date.\n\n> (hack hack hack)\n> $ git commit -a -m \"fixed typos\"\n\nAnd if you run \"git status\" first, it'll tell you that the submodule\n(still) isn't up-to-date.\n\n> $ git push\n> (Oops. You've just accidentally committed the wrong submodule heads.)\n\nYou always have to be careful when doing \"git commit -a\".\n\n> Another approach is for pull, checkout etc. to automatically update the\n> submodule' head ref, but no more.\n\nThen everything, including \"git submodule update\", would assume\nthat the submodule is up-to-date.\n\nskimo\n"},{"id":"50037","messageId":"46B76B8C.9050905@tromer.org","threadId":"9077","inReplyTo":"20070805144632.GB999MdfPADPa@greensroom.kotnet.org","subject":"Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out","fromName":"Eran Tromer","fromEmail":"git2eran@tromer.org","sentAt":"2007-08-06T18:42:20Z","receivedAt":"2007-08-06T18:42:20Z","isPatch":true,"sender":{"key":"git2eran@tromer.org","avatar":null},"body":"On 2007-08-05 10:46, Sven Verdoolaege wrote:\n>> $ git checkout experimental-death-ray\n>> $ git submodules update\n>> (return a week later, woozy from the vacation.)\n>> $ git checkout master\n> \n> Here, it'll warn that your submodule isn't up-to-date.\n> \n>> (hack hack hack)\n>> $ git commit -a -m \"fixed typos\"\n> \n> And if you run \"git status\" first, it'll tell you that the submodule\n> (still) isn't up-to-date.\n> \n>> $ git push\n>> (Oops. You've just accidentally committed the wrong submodule heads.)\n> \n> You always have to be careful when doing \"git commit -a\".\n\nExactly. You now have to be very careful, whereas previously\n$ git checkout master && vi foo && git commit -a -m \"fixed typos\"\nwas perfectly safe.\n\nWorse yet, it could also be a script making similar assumptions. For\nexample, consider the tree filter in git-filter-branch. It used to be\nfine, but will now corrupt the rewritten trees when submodules are\ninvolved. Here's the relevant code from git-filter-branch.sh:\n\n-----------------------------------------------------------------\nwhile read commit parents; do\n...\n\t\tgit read-tree -i -m $commit\n...\n\t\tgit checkout-index -f -u -a ||\n\t\t\tdie \"Could not checkout the index\"\n...\n\t\teval \"$filter_tree\" < /dev/null ||\n\t\t\tdie \"tree filter failed: $filter_tree\"\n\n\t\tgit diff-index -r $commit | cut -f 2- | tr '\\n' '\\0' | \\\n\t\t\txargs -0 git update-index --add --replace --remove\n...\n\tsh -c \"$filter_commit\" \"git commit-tree\" \\\n\t\t$(git write-tree) $parentstr < ../message > ../map/$commit\ndone <../revs\n-----------------------------------------------------------------\n\n\n>> Another approach is for pull, checkout etc. to automatically update the\n>> submodule' head ref, but no more.\n> \n> Then everything, including \"git submodule update\", would assume\n> that the submodule is up-to-date.\n\nWith that approach, \"git submodule update\" would fetch the submodule's\nhead commit (which could be missing), and then check it against the\nsubmodule's index (and maybe its work tree).\n\n Eran\n"},{"id":"50038","messageId":"20070806190344.GF999MdfPADPa@greensroom.kotnet.org","threadId":"9077","inReplyTo":"46B76B8C.9050905@tromer.org","subject":"Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out","fromName":"Sven Verdoolaege","fromEmail":"skimo@kotnet.org","sentAt":"2007-08-06T19:03:44Z","receivedAt":"2007-08-06T19:03:44Z","isPatch":true,"sender":{"key":"skimo@kotnet.org","avatar":null},"body":"On Mon, Aug 06, 2007 at 02:42:20PM -0400, Eran Tromer wrote:\n> On 2007-08-05 10:46, Sven Verdoolaege wrote:\n> > You always have to be careful when doing \"git commit -a\".\n> \n> Exactly. You now have to be very careful, whereas previously\n> $ git checkout master && vi foo && git commit -a -m \"fixed typos\"\n> was perfectly safe.\n\nI don't see the difference. If you forgot you changed something\n(be it a submodule or a file) you will commit something you\ndidn't plan to commit.\n\n bash-3.00$ git init; touch a b c; git add .; git commit -m 1\n Initialized empty Git repository in .git/\n Created initial commit 4e6da45: 1\n 0 files changed, 0 insertions(+), 0 deletions(-)\n create mode 100644 a\n create mode 100644 b\n create mode 100644 c\n bash-3.00$ git checkout -b branch\n Switched to a new branch \"branch\"\n bash-3.00$ echo \"foo\" > a; git add a; git commit -m 2\n Created commit fe87123: 2\n 1 files changed, 1 insertions(+), 0 deletions(-)\n bash-3.00$ echo \"bar\" > c\n bash-3.00$ git checkout master && echo \"test\" > b && git commit -a -m 'change b'\n M c\n Switched to branch \"master\"\n Created commit 657c5b1: change b\n 2 files changed, 2 insertions(+), 0 deletions(-)\n\n> >> Another approach is for pull, checkout etc. to automatically update the\n> >> submodule' head ref, but no more.\n> > \n> > Then everything, including \"git submodule update\", would assume\n> > that the submodule is up-to-date.\n> \n> With that approach, \"git submodule update\" would fetch the submodule's\n> head commit (which could be missing), and then check it against the\n> submodule's index (and maybe its work tree).\n\nAnd how is anyone supposed to figure out what HEAD the submodule's\nindex and working tree correspond to?\nI can only hope that \"git submodule update\" would never blindly assume\nthat the submodule is clean and so the user would have to manually\nsync the HEAD and the working tree.\n\nskimo\n"},{"id":"50083","messageId":"46B7E5FE.7050006@tromer.org","threadId":"9077","inReplyTo":"20070806190344.GF999MdfPADPa@greensroom.kotnet.org","subject":"Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out","fromName":"Eran Tromer","fromEmail":"git2eran@tromer.org","sentAt":"2007-08-07T03:24:46Z","receivedAt":"2007-08-07T03:24:46Z","isPatch":true,"sender":{"key":"git2eran@tromer.org","avatar":null},"body":"On 2007-08-06 15:03, Sven Verdoolaege wrote:\n> I don't see the difference. If you forgot you changed something\n> (be it a submodule or a file) you will commit something you\n> didn't plan to commit.\n...\n> bash-3.00$ git checkout master && echo \"test\" > b && git commit -a -m 'change b'\n> M c\n> Switched to branch \"master\"\n> Created commit 657c5b1: change b\n> 2 files changed, 2 insertions(+), 0 deletions(-)\n\nYes, you're right. (When I tried this, checkout complained about the\ndirty working tree because a *merge* was needed.)\n\nSo let's try to explicitly reset the index and work tree:\n\n$ git reset --hard master\n$ vi foo\n$ git commit -a -m 'fixed typos'\n\nOops, still a corrupt commit.\n\n\n>>>> Another approach is for pull, checkout etc. to automatically update the\n>>>> submodule' head ref, but no more.\n>>> Then everything, including \"git submodule update\", would assume\n>>> that the submodule is up-to-date.\n>> With that approach, \"git submodule update\" would fetch the submodule's\n>> head commit (which could be missing), and then check it against the\n>> submodule's index (and maybe its work tree).\n> And how is anyone supposed to figure out what HEAD the submodule's\n> index and working tree correspond to?\n\nWhat HEAD corresponds to any other dirty index or dirty working tree?\nIt's irrelevant and may not exist. You just have some random dirty state.\n\nIf it's the yet-to-exist submodule merging you're worried about, the\nsubmodule's old head can be saved in ORIG_HEAD or some such during the\nsupermodule checkout.\n\n\n> I can only hope that \"git submodule update\" would never blindly assume\n> that the submodule is clean and so the user would have to manually\n> sync the HEAD and the working tree.\n\nWhy would it assume that? In this approach, and ignoring submodule\nmerging for now, \"git submodule update\" should mean roughly \"cd\nsubmodule && git fetch HEAD && git reset --hard HEAD\". After all, this\nis really the only way to end up with the prescribed commit sha1.\n\nI agree that for safety it makes sense to warn or abort if the index\ndoesn't match ORIG_HEAD (saved by the supermodule checkout) or if the\nindex doesn't match the work tree.\n\n Eran\n"},{"id":"50105","messageId":"20070807085149.GH999MdfPADPa@greensroom.kotnet.org","threadId":"9077","inReplyTo":"46B7E5FE.7050006@tromer.org","subject":"Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out","fromName":"Sven Verdoolaege","fromEmail":"skimo@kotnet.org","sentAt":"2007-08-07T08:51:49Z","receivedAt":"2007-08-07T08:51:49Z","isPatch":true,"sender":{"key":"skimo@kotnet.org","avatar":null},"body":"On Mon, Aug 06, 2007 at 11:24:46PM -0400, Eran Tromer wrote:\n> On 2007-08-06 15:03, Sven Verdoolaege wrote:\n> >>>> Another approach is for pull, checkout etc. to automatically update the\n> >>>> submodule' head ref, but no more.\n> >>> Then everything, including \"git submodule update\", would assume\n> >>> that the submodule is up-to-date.\n> >> With that approach, \"git submodule update\" would fetch the submodule's\n> >> head commit (which could be missing), and then check it against the\n> >> submodule's index (and maybe its work tree).\n> > And how is anyone supposed to figure out what HEAD the submodule's\n> > index and working tree correspond to?\n> \n> What HEAD corresponds to any other dirty index or dirty working tree?\n> It's irrelevant and may not exist. You just have some random dirty state.\n\nThe only way to know that it's dirty is if you know the HEAD.\nHow can that not be relevant.\n\n> > I can only hope that \"git submodule update\" would never blindly assume\n> > that the submodule is clean and so the user would have to manually\n> > sync the HEAD and the working tree.\n> \n> Why would it assume that? In this approach, and ignoring submodule\n> merging for now, \"git submodule update\" should mean roughly \"cd\n> submodule && git fetch HEAD && git reset --hard HEAD\".\n\nIf you're doing that, then that is exactly what you are assuming.\n\n> After all, this\n> is really the only way to end up with the prescribed commit sha1.\n\nThat's the best way of losing all you precious changes in the submodule.\nAnd there is no way to get them back!\nSurely this is a lot worse than occasionally committing something you\ndidn't plan to commit, and only if you are performing a known \"dangerous\"\noperation.\n\n> I agree that for safety it makes sense to warn or abort if the index\n> doesn't match ORIG_HEAD (saved by the supermodule checkout) or if the\n> index doesn't match the work tree.\n\nYou may have done several supermodule checkouts since you last changed\nthe submodule.\n\nskimo\n"},{"id":"50163","messageId":"46B91F4E.8050008@tromer.org","threadId":"9077","inReplyTo":"20070807085149.GH999MdfPADPa@greensroom.kotnet.org","subject":"Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out","fromName":"Eran Tromer","fromEmail":"git2eran@tromer.org","sentAt":"2007-08-08T01:41:34Z","receivedAt":"2007-08-08T01:41:34Z","isPatch":true,"sender":{"key":"git2eran@tromer.org","avatar":null},"body":"On 2007-08-07 04:51, Sven Verdoolaege wrote:\n> Surely this is a lot worse than occasionally committing something you\n> didn't plan to commit, and only if you are performing a known \"dangerous\"\n> operation.\n> \nAre you saying that\n$ git reset --hard HEAD && vi foo && git commit -a\nis a \"known dangerous\" operation that can record corrupted content even\nthough you didn't touch it? This is very bad news indeed! I don't see\nany such warnings in the documentation.\n\nSo, when I'm sure all the edits I did in the work tree are fine, how\n*do* I safely make a commit without manually inspecting the changed\nfiles list, or manually listing the changed files for \"git add\", or\nmanually running \"git submodule update\", or manually checking whether\nthere happens to be some submodules in this project, some other such\ncumbersome measure?\n\n\n> You may have done several supermodule checkouts since you last changed\n> the submodule.\n\nTrue, that approach won't work. I can imagine some logic to\nconditionally update ORIG_HEAD, but it gets messy and fragile. Looks\nlike brokenness is just inevitable when you let the state get stale and\nthen merrily read it out as if it's fresh.\n\n\nSo.... Maybe we can tackle this head-on? Let index entries be explicitly\nmarked as \"adrift\", meaning we just don't touch the work tree for these\nentries -- neither reads nor writes. It's used when the piece of\ncontent, say a submodule, is allowed to drift arbitrarily in the work\ntree in a way that doesn't represent meaningful edits that should be\nreflected in commits, diffs, etc.\n\nFor example:\n- \"git checkout\" sets the \"adrift\" flag on all (modified?) submodules\n- \"git submodule update\" undrifts (\"moores?\") the submodules\n- \"git commit -a\" skips files that are adrift, and likewise \"git add .\",\n \"git diff\" etc. (perhaps with some warning?)\n- \"git add \" undrifts and proceeds as usual\n- \"git status\" reports drifting files as such and doesn't bother to\n check them in the work tree\n- When merging into the work tree, drifting files are left as such\n\nAnd why stop at submodules? If there's a large blob you don't want to\ncheck out, just \"git drift \" it. To set whole *directories*\nadrift, we can piggybacking on the empty-directory support (i.e., add a\ndirectory entry to the index and set it adrift). So this could be the\nbasis of partial-checkout support.\n\nDoes this sound reasonable?\n\n Eran\n"},{"id":"50189","messageId":"20070808113952.GN999MdfPADPa@greensroom.kotnet.org","threadId":"9077","inReplyTo":"46B91F4E.8050008@tromer.org","subject":"Re: [PATCH] unpack-trees.c: assume submodules are clean during check-out","fromName":"Sven Verdoolaege","fromEmail":"skimo@kotnet.org","sentAt":"2007-08-08T11:39:52Z","receivedAt":"2007-08-08T11:39:52Z","isPatch":true,"sender":{"key":"skimo@kotnet.org","avatar":null},"body":"On Tue, Aug 07, 2007 at 09:41:34PM -0400, Eran Tromer wrote:\n> On 2007-08-07 04:51, Sven Verdoolaege wrote:\n> > Surely this is a lot worse than occasionally committing something you\n> > didn't plan to commit, and only if you are performing a known \"dangerous\"\n> > operation.\n> > \n> Are you saying that\n> $ git reset --hard HEAD && vi foo && git commit -a\n> is a \"known dangerous\" operation that can record corrupted content even\n> though you didn't touch it?\n\nI'm only saying that \"git commit -a\" will commit anything that has been\nmodified, so you have to be careful when using it and it just so happens\nthat git reset may leave submodules modified. This should probably\nbe documented.\nAnd I agree with you that this is not ideal (personally, I'd want\nall checked-out submodules to get updated automatically), but it's\ncertainly better than your earlier proposal.\n\n> So, when I'm sure all the edits I did in the work tree are fine, how\n> *do* I safely make a commit without manually inspecting the changed\n> files list, or manually listing the changed files for \"git add\", or\n> manually running \"git submodule update\", or manually checking whether\n> there happens to be some submodules in this project, some other such\n> cumbersome measure?\n\nIf you've ever done a \"git submodule update\" in the project, you should\nknow that there are submodules and if you haven't then there are no\nchecked-out submodules that can get out of sync.\nIf you're talking about tools, then they should indeed be extra careful.\n\n[another proposal]\n> \n> Does this sound reasonable?\n\nI'll leave it to others to comment on that one.\n\nskimo\n"}]}