{"thread":{"id":"10815","subject":"[PATCH] Fix rev-list when showing objects involving submodules","startedAt":"2007-11-11T23:35:23Z","lastAt":"2007-11-12T21:25:22Z","messageCount":5,"participants":["Johannes Schindelin","Sam Vilain","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"59400","messageId":"Pine.LNX.4.64.0711112335020.4362@racer.site","threadId":"10815","inReplyTo":null,"subject":"[PATCH] Fix rev-list when showing objects involving submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-11T23:35:23Z","receivedAt":"2007-11-11T23:35:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nThe function mark_tree_uninteresting() assumed that the tree entries\nare blob when they are not trees.  This is not so.  Since we do\nnot traverse into submodules (yet), the gitlinks should be ignored.\n\nNoticed by Ilari on IRC.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n revision.c                    |    2 +-\n t/t6008-rev-list-submodule.sh |   42 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 1 deletions(-)\n create mode 100755 t/t6008-rev-list-submodule.sh\n\ndiff --git a/revision.c b/revision.c\nindex 931f978..81b5a93 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -69,7 +69,7 @@ void mark_tree_uninteresting(struct tree *tree)\n \twhile (tree_entry(&desc, &entry)) {\n \t\tif (S_ISDIR(entry.mode))\n \t\t\tmark_tree_uninteresting(lookup_tree(entry.sha1));\n-\t\telse\n+\t\telse if (!S_ISGITLINK(entry.mode))\n \t\t\tmark_blob_uninteresting(lookup_blob(entry.sha1));\n \t}\n \ndiff --git a/t/t6008-rev-list-submodule.sh b/t/t6008-rev-list-submodule.sh\nnew file mode 100755\nindex 0000000..88e96fb\n--- /dev/null\n+++ b/t/t6008-rev-list-submodule.sh\n@@ -0,0 +1,42 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2007 Johannes E. Schindelin\n+#\n+\n+test_description='git rev-list involving submodules that this repo has'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\t: > file &&\n+\tgit add file &&\n+\ttest_tick &&\n+\tgit commit -m initial &&\n+\techo 1 > file &&\n+\ttest_tick &&\n+\tgit commit -m second file &&\n+\techo 2 > file &&\n+\ttest_tick &&\n+\tgit commit -m third file &&\n+\n+\trm .git/index &&\n+\n+\t: > super-file &&\n+\tgit add super-file &&\n+\tgit submodule add . sub &&\n+\tgit symbolic-ref HEAD refs/heads/super &&\n+\ttest_tick &&\n+\tgit commit -m super-initial &&\n+\techo 1 > super-file &&\n+\ttest_tick &&\n+\tgit commit -m super-first super-file &&\n+\techo 2 > super-file &&\n+\ttest_tick &&\n+\tgit commit -m super-second super-file\n+'\n+\n+test_expect_success \"Ilari's test\" '\n+\tgit rev-list --objects super master ^super^\n+'\n+\n+test_done\n-- \n1.5.3.5.1695.g146d07\n"},{"id":"59559","messageId":"4738AF60.90207@vilain.net","threadId":"10815","inReplyTo":"Pine.LNX.4.64.0711112335020.4362@racer.site","subject":"Re: [PATCH] Fix rev-list when showing objects involving submodules","fromName":"Sam Vilain","fromEmail":"sam@vilain.net","sentAt":"2007-11-12T19:54:08Z","receivedAt":"2007-11-12T19:54:08Z","isPatch":true,"sender":{"key":"sam@vilain.net","avatar":"https://gravatar.com/avatar/8fc840ca854dbf6f7065b4335e3b934951c1dca3b11db688e95e471901f8f4a8?d=mp&s=160"},"body":"Johannes Schindelin wrote:\n> The function mark_tree_uninteresting() assumed that the tree entries\n> are blob when they are not trees.  This is not so.  Since we do\n> not traverse into submodules (yet), the gitlinks should be ignored.\n>\n> diff --git a/revision.c b/revision.c\n> index 931f978..81b5a93 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -69,7 +69,7 @@ void mark_tree_uninteresting(struct tree *tree)\n>  \twhile (tree_entry(&desc, &entry)) {\n>  \t\tif (S_ISDIR(entry.mode))\n>  \t\t\tmark_tree_uninteresting(lookup_tree(entry.sha1));\n> -\t\telse\n> +\t\telse if (!S_ISGITLINK(entry.mode))\n>  \t\t\tmark_blob_uninteresting(lookup_blob(entry.sha1));\n>  \t}\n>  \n>   \n\nWouldn't it be better to check for what it is, rather than what it is not?\n\nSam.\n"},{"id":"59562","messageId":"Pine.LNX.4.64.0711122020270.4362@racer.site","threadId":"10815","inReplyTo":"4738AF60.90207@vilain.net","subject":"Re: [PATCH] Fix rev-list when showing objects involving submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-12T20:21:05Z","receivedAt":"2007-11-12T20:21:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 13 Nov 2007, Sam Vilain wrote:\n\n> Johannes Schindelin wrote:\n> > The function mark_tree_uninteresting() assumed that the tree entries\n> > are blob when they are not trees.  This is not so.  Since we do\n> > not traverse into submodules (yet), the gitlinks should be ignored.\n> >\n> > diff --git a/revision.c b/revision.c\n> > index 931f978..81b5a93 100644\n> > --- a/revision.c\n> > +++ b/revision.c\n> > @@ -69,7 +69,7 @@ void mark_tree_uninteresting(struct tree *tree)\n> >  \twhile (tree_entry(&desc, &entry)) {\n> >  \t\tif (S_ISDIR(entry.mode))\n> >  \t\t\tmark_tree_uninteresting(lookup_tree(entry.sha1));\n> > -\t\telse\n> > +\t\telse if (!S_ISGITLINK(entry.mode))\n> >  \t\t\tmark_blob_uninteresting(lookup_blob(entry.sha1));\n> >  \t}\n> >  \n> >   \n> \n> Wouldn't it be better to check for what it is, rather than what it is not?\n\nYou mean something like\n\n\t\telse if (S_ISREG(entry.mod) || S_ISLNK(entry.mod))\n\nHmm?  Sure, I have no preference there.\n\nCiao,\nDscho\n"},{"id":"59566","messageId":"alpine.LFD.0.9999.0711121231570.3062@woody.linux-foundation.org","threadId":"10815","inReplyTo":"Pine.LNX.4.64.0711122020270.4362@racer.site","subject":"Re: [PATCH] Fix rev-list when showing objects involving submodules","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-11-12T20:43:57Z","receivedAt":"2007-11-12T20:43:57Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 12 Nov 2007, Johannes Schindelin wrote:\n> \n> You mean something like\n> \n> \t\telse if (S_ISREG(entry.mod) || S_ISLNK(entry.mod))\n> \n> Hmm?  Sure, I have no preference there.\n\nMaybe more along the line of something like this?\n\nIn general, I suspect we should try to start moving away from using the \n\"S_ISLNK()\" like things for internal git state. It was a mistake to just \nassume the numbers all were same across all systems in the first place.\n\nSo this just converts to the \"object_type\", and then uses a case \nstatement.\n\n\t\tLinus\n\n---\n builtin-pack-objects.c |    2 +-\n revision.c             |   11 +++++++++--\n tree-walk.h            |    7 +++++++\n 3 files changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 545ece5..4f44658 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -990,7 +990,7 @@ static void add_pbase_object(struct tree_desc *tree,\n \t\t\treturn;\n \t\tif (name[cmplen] != '/') {\n \t\t\tadd_object_entry(entry.sha1,\n-\t\t\t\t\t S_ISDIR(entry.mode) ? OBJ_TREE : OBJ_BLOB,\n+\t\t\t\t\t object_type(entry.mode),\n \t\t\t\t\t fullname, 1);\n \t\t\treturn;\n \t\t}\ndiff --git a/revision.c b/revision.c\nindex 931f978..c054a82 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -67,10 +67,17 @@ void mark_tree_uninteresting(struct tree *tree)\n \n \tinit_tree_desc(&desc, tree->buffer, tree->size);\n \twhile (tree_entry(&desc, &entry)) {\n-\t\tif (S_ISDIR(entry.mode))\n+\t\tswitch (object_type(entry.mode)) {\n+\t\tcase OBJ_TREE:\n \t\t\tmark_tree_uninteresting(lookup_tree(entry.sha1));\n-\t\telse\n+\t\t\tbreak;\n+\t\tcase OBJ_BLOB:\n \t\t\tmark_blob_uninteresting(lookup_blob(entry.sha1));\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\t/* Subproject commit - not in this repository */\n+\t\t\tbreak;\n+\t\t}\n \t}\n \n \t/*\ndiff --git a/tree-walk.h b/tree-walk.h\nindex db0fbdc..903a7b0 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -7,6 +7,13 @@ struct name_entry {\n \tunsigned int mode;\n };\n \n+static inline enum object_type object_type(unsigned int mode)\n+{\n+\treturn S_ISDIR(mode) ? OBJ_TREE :\n+\t\tS_ISGITLINK(mode) ? OBJ_COMMIT :\n+\t\tOBJ_BLOB;\n+}\n+\n struct tree_desc {\n \tconst void *buffer;\n \tstruct name_entry entry;\n"},{"id":"59574","messageId":"Pine.LNX.4.64.0711122124500.4362@racer.site","threadId":"10815","inReplyTo":"alpine.LFD.0.9999.0711121231570.3062@woody.linux-foundation.org","subject":"Re: [PATCH] Fix rev-list when showing objects involving submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-11-12T21:25:22Z","receivedAt":"2007-11-12T21:25:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 12 Nov 2007, Linus Torvalds wrote:\n\n> On Mon, 12 Nov 2007, Johannes Schindelin wrote:\n> > \n> > You mean something like\n> > \n> > \t\telse if (S_ISREG(entry.mod) || S_ISLNK(entry.mod))\n> > \n> > Hmm?  Sure, I have no preference there.\n> \n> Maybe more along the line of something like this?\n> \n> In general, I suspect we should try to start moving away from using the \n> \"S_ISLNK()\" like things for internal git state. It was a mistake to just \n> assume the numbers all were same across all systems in the first place.\n> \n> So this just converts to the \"object_type\", and then uses a case \n> statement.\n\nSure.  Just add the test from my patch, and we're fixed.\n\nCiao,\nDscho\n"}]}