{"thread":{"id":"17232","subject":"[PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules","startedAt":"2009-01-18T10:53:16Z","lastAt":"2009-01-20T07:04:36Z","messageCount":26,"participants":["Lars Hjemli","Johannes Schindelin","René Scharfe","Junio C Hamano","Keith Cascio"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"100944","messageId":"1232275999-14852-1-git-send-email-hjemli@gmail.com","threadId":"17232","inReplyTo":null,"subject":"[PATCH 0/3] Implement 'git archive --submodules'","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2009-01-18T10:53:16Z","receivedAt":"2009-01-18T10:53:16Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"This series teaches read_tree_recursive() how to traverse into gitlinked\nrepositories by automatically adding submodule object databases as\nalternates during traversal. It is still perfectly legal for a submodule\nnot to be checked out, in which case the submodule will be ignored.\n\nOn top of this, the implementation of 'git archive --submodules' simply\nactivates the new feature in read_tree_recursive().\n\nLars Hjemli (3):\n sha1_file: add function to insert alternate object db\n Teach read_tree_recursive() how to traverse into submodules\n git-archive: add support for --submodules\n\n Documentation/git-archive.txt | 7 +++-\n archive.c | 4 ++\n cache.h | 3 ++\n environment.c | 12 ++++++\n sha1_file.c | 5 +++\n t/t5001-archive-submodules.sh | 78 +++++++++++++++++++++++++++++++++++++++\n tree.c | 80 +++++++++++++++++++++++++++++++++++++++++\n 7 files changed, 187 insertions(+), 2 deletions(-)\n create mode 100755 t/t5001-archive-submodules.sh\n"},{"id":"100945","messageId":"1232275999-14852-2-git-send-email-hjemli@gmail.com","threadId":"17232","inReplyTo":"1232275999-14852-1-git-send-email-hjemli@gmail.com","subject":"[PATCH 1/3] sha1_file: add function to insert alternate object db","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2009-01-18T10:53:17Z","receivedAt":"2009-01-18T10:53:17Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"This function will be used when implementing traversal into submodules.\n\nSigned-off-by: Lars Hjemli \n---\n cache.h | 1 +\n sha1_file.c | 5 +++++\n 2 files changed, 6 insertions(+), 0 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 8e1af26..daa2d4e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -724,6 +724,7 @@ extern struct alternate_object_database {\n \tchar base[FLEX_ARRAY]; /* more */\n } *alt_odb_list;\n extern void prepare_alt_odb(void);\n+extern int add_alt_odb(const char *path);\n extern void add_to_alternates_file(const char *reference);\n typedef int alt_odb_fn(struct alternate_object_database *, void *);\n extern void foreach_alt_odb(alt_odb_fn, void*);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f08493f..19f9725 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -356,6 +356,11 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,\n \t}\n }\n \n+int add_alt_odb(const char *path)\n+{\n+\treturn link_alt_odb_entry(path, strlen(path), NULL, 0);\n+}\n+\n static void read_info_alternates(const char * relative_base, int depth)\n {\n \tchar *map;\n-- \n1.6.1.150.g5e733b\n"},{"id":"100943","messageId":"1232275999-14852-3-git-send-email-hjemli@gmail.com","threadId":"17232","inReplyTo":"1232275999-14852-2-git-send-email-hjemli@gmail.com","subject":"[PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2009-01-18T10:53:18Z","receivedAt":"2009-01-18T10:53:18Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"The traversal of submodules is only triggered if the current submodule\nHEAD commit object is accessible. To this end, read_tree_recursive()\nwill try to insert the submodule odb as an alternate odb but the lack\nof such an odb is not treated as an error since it is then assumed that\nthe user is not interested in the submodule content. However, if the\nsubmodule odb is found it is treated as an error if the HEAD commit\nobject is missing.\n\nSigned-off-by: Lars Hjemli \n---\n cache.h | 2 +\n environment.c | 12 ++++++++\n tree.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 94 insertions(+), 0 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex daa2d4e..6728467 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -382,6 +382,8 @@ extern int set_git_dir(const char *path);\n extern const char *get_git_work_tree(void);\n extern const char *read_gitfile_gently(const char *path);\n extern void set_git_work_tree(const char *tree);\n+extern int get_traverse_gitlinks();\n+extern void set_traverse_gitlinks(int traverse);\n \n #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\n \ndiff --git a/environment.c b/environment.c\nindex e278bce..35cc557 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -53,6 +53,8 @@ static char *work_tree;\n static const char *git_dir;\n static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;\n \n+static int traverse_gitlinks = 0;\n+\n static void setup_git_env(void)\n {\n \tgit_dir = getenv(GIT_DIR_ENVIRONMENT);\n@@ -159,3 +161,13 @@ int set_git_dir(const char *path)\n \tsetup_git_env();\n \treturn 0;\n }\n+\n+int get_traverse_gitlinks()\n+{\n+\treturn traverse_gitlinks;\n+}\n+\n+void set_traverse_gitlinks(int traverse)\n+{\n+\ttraverse_gitlinks = traverse;\n+}\ndiff --git a/tree.c b/tree.c\nindex 03e782a..87cf309 100644\n--- a/tree.c\n+++ b/tree.c\n@@ -5,6 +5,7 @@\n #include \"commit.h\"\n #include \"tag.h\"\n #include \"tree-walk.h\"\n+#include \"refs.h\"\n \n const char *tree_type = \"tree\";\n \n@@ -89,6 +90,61 @@ static int match_tree_entry(const char *base, int baselen, const char *path, uns\n \treturn 0;\n }\n \n+/* Try to add the objectdb of a submodule */\n+int add_gitlink_odb(char *relpath)\n+{\n+\tconst char *odbpath;\n+\tstruct stat st;\n+\n+\todbpath = read_gitfile_gently(mkpath(\"%s/.git\", relpath));\n+\tif (!odbpath)\n+\t\todbpath = mkpath(\"%s/.git/objects\", relpath);\n+\n+\tif (stat(odbpath, &st))\n+\t\treturn 1;\n+\n+\treturn add_alt_odb(odbpath);\n+}\n+\n+/* Check if we should recurse into the specified submodule */\n+int traverse_gitlink(char *path, const unsigned char *commit_sha1,\n+\t\t struct tree **subtree)\n+{\n+\tunsigned char sha1[20];\n+\tint linked_odb = 0;\n+\tstruct commit *commit;\n+\tvoid *buffer;\n+\tenum object_type type;\n+\tunsigned long size;\n+\n+\thashcpy(sha1, commit_sha1);\n+\tif (!add_gitlink_odb(path)) {\n+\t\tlinked_odb = 1;\n+\t\tif (resolve_gitlink_ref(path, \"HEAD\", sha1))\n+\t\t\tdie(\"Unable to lookup HEAD in %s\", path);\n+\t}\n+\n+\tbuffer = read_sha1_file(sha1, &type, &size);\n+\tif (!buffer) {\n+\t\tif (linked_odb)\n+\t\t\tdie(\"Unable to read object %s in submodule %s\",\n+\t\t\t sha1_to_hex(sha1), path);\n+\t\telse\n+\t\t\treturn 0;\n+\t}\n+\n+\tcommit = lookup_commit(sha1);\n+\tif (!commit)\n+\t\tdie(\"traverse_gitlink(): internal error\");\n+\n+\tif (parse_commit_buffer(commit, buffer, size))\n+\t\tdie(\"Failed to parse commit %s in submodule %s\",\n+\t\t sha1_to_hex(sha1), path);\n+\n+\t*subtree = commit->tree;\n+\treturn 1;\n+}\n+\n int read_tree_recursive(struct tree *tree,\n \t\t\tconst char *base, int baselen,\n \t\t\tint stage, const char **match,\n@@ -132,6 +188,30 @@ int read_tree_recursive(struct tree *tree,\n \t\t\t\treturn -1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (S_ISGITLINK(entry.mode) && get_traverse_gitlinks()) {\n+\t\t\tint retval;\n+\t\t\tchar *newbase;\n+\t\t\tstruct tree *subtree;\n+\t\t\tunsigned int pathlen = tree_entry_len(entry.path, entry.sha1);\n+\n+\t\t\tnewbase = xmalloc(baselen + 1 + pathlen);\n+\t\t\tmemcpy(newbase, base, baselen);\n+\t\t\tmemcpy(newbase + baselen, entry.path, pathlen);\n+\t\t\tnewbase[baselen + pathlen] = 0;\n+\t\t\tif (!traverse_gitlink(newbase, entry.sha1, &subtree)) {\n+\t\t\t\tfree(newbase);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tnewbase[baselen + pathlen] = '/';\n+\t\t\tretval = read_tree_recursive(subtree,\n+\t\t\t\t\t\t newbase,\n+\t\t\t\t\t\t baselen + pathlen + 1,\n+\t\t\t\t\t\t stage, match, fn, context);\n+\t\t\tfree(newbase);\n+\t\t\tif (retval)\n+\t\t\t\treturn -1;\n+\t\t\tcontinue;\n+\t\t}\n \t}\n \treturn 0;\n }\n-- \n1.6.1.150.g5e733b\n"},{"id":"100946","messageId":"1232275999-14852-4-git-send-email-hjemli@gmail.com","threadId":"17232","inReplyTo":"1232275999-14852-3-git-send-email-hjemli@gmail.com","subject":"[PATCH 3/3] git-archive: add support for --submodules","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2009-01-18T10:53:19Z","receivedAt":"2009-01-18T10:53:19Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"Signed-off-by: Lars Hjemli \n---\n Documentation/git-archive.txt | 7 +++-\n archive.c | 4 ++\n t/t5001-archive-submodules.sh | 78 +++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 87 insertions(+), 2 deletions(-)\n create mode 100755 t/t5001-archive-submodules.sh\n\ndiff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt\nindex 41cbf9c..84e0b43 100644\n--- a/Documentation/git-archive.txt\n+++ b/Documentation/git-archive.txt\n@@ -10,8 +10,8 @@ SYNOPSIS\n --------\n [verse]\n 'git archive' --format= [--list] [--prefix=/] []\n-\t [--remote= [--exec=]] \n-\t [path...]\n+\t [--remote= [--exec=]] [--submodules]\n+\t [path...]\n \n DESCRIPTION\n -----------\n@@ -59,6 +59,9 @@ OPTIONS\n \tUsed with --remote to specify the path to the\n \t'git-upload-archive' on the remote side.\n \n+--submodules::\n+\tInclude files from checked out submodules.\n+\n ::\n \tThe tree or commit to produce an archive for.\n \ndiff --git a/archive.c b/archive.c\nindex 9ac455d..0c024b8 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -255,6 +255,7 @@ static int parse_archive_args(int argc, const char **argv,\n \tconst char *exec = NULL;\n \tint compression_level = -1;\n \tint verbose = 0;\n+\tint submodules = 0;\n \tint i;\n \tint list = 0;\n \tstruct option opts[] = {\n@@ -262,6 +263,8 @@ static int parse_archive_args(int argc, const char **argv,\n \t\tOPT_STRING(0, \"format\", &format, \"fmt\", \"archive format\"),\n \t\tOPT_STRING(0, \"prefix\", &base, \"prefix\",\n \t\t\t\"prepend prefix to each pathname in the archive\"),\n+\t\tOPT_BOOLEAN(0, \"submodules\", &submodules,\n+\t\t\t\"recurse into submodules\"),\n \t\tOPT__VERBOSE(&verbose),\n \t\tOPT__COMPR('0', &compression_level, \"store only\", 0),\n \t\tOPT__COMPR('1', &compression_level, \"compress faster\", 1),\n@@ -320,6 +323,7 @@ static int parse_archive_args(int argc, const char **argv,\n \targs->base = base;\n \targs->baselen = strlen(base);\n \n+\tset_traverse_gitlinks(submodules);\n \treturn argc;\n }\n \ndiff --git a/t/t5001-archive-submodules.sh b/t/t5001-archive-submodules.sh\nnew file mode 100755\nindex 0000000..5a499a4\n--- /dev/null\n+++ b/t/t5001-archive-submodules.sh\n@@ -0,0 +1,78 @@\n+#!/bin/sh\n+\n+test_description='git archive can include submodule content'\n+\n+. ./test-lib.sh\n+\n+add_file()\n+{\n+\tgit add $1 &&\n+\tgit commit -m \"added $1\"\n+}\n+\n+add_submodule()\n+{\n+\tmkdir $1 && (\n+\t\tcd $1 &&\n+\t\tgit init &&\n+\t\techo \"File $2\" >$2 &&\n+\t\tadd_file $2\n+\t) &&\n+\tadd_file $1\n+}\n+\n+test_expect_success 'setup submodules' '\n+\techo \"File 1\" >1 &&\n+\tadd_file 1 &&\n+\tadd_submodule 2 3 &&\n+\tadd_submodule 4 5 &&\n+\t(cd 4 && add_submodule 6 7)\n+'\n+\n+test_expect_success 'git archive usually ignores submodules' '\n+\tcat <expected &&\n+1\n+2/\n+4/\n+EOF\n+\tgit archive HEAD >normal.tar &&\n+\ttar -tf normal.tar >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git archive includes submodules when requested' '\n+\tcat <expected &&\n+1\n+2/\n+2/3\n+4/\n+4/5\n+4/6/\n+4/6/7\n+EOF\n+\tgit archive --submodules HEAD >full.tar &&\n+\ttar -tf full.tar >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git archive ignores uninteresting submodules' '\n+\tcat <expected &&\n+1\n+2/\n+4/\n+4/5\n+4/6/\n+4/6/7\n+EOF\n+\trm -rf 2/.git &&\n+\tgit archive --submodules HEAD >partial.tar &&\n+\ttar -tf partial.tar >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git archive fails on missing object in interesting submodule' '\n+\tfind 4/.git/objects -type f | xargs rm &&\n+\ttest_must_fail git archive --submodules HEAD\n+'\n+\n+test_done\n-- \n1.6.1.150.g5e733b\n"},{"id":"101003","messageId":"alpine.DEB.1.00.0901181629590.3586@pacific.mpi-cbg.de","threadId":"17232","inReplyTo":"1232275999-14852-2-git-send-email-hjemli@gmail.com","subject":"Re: [PATCH 1/3] sha1_file: add function to insert alternate object db","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-18T15:32:28Z","receivedAt":"2009-01-18T15:32:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 18 Jan 2009, Lars Hjemli wrote:\n\n> diff --git a/cache.h b/cache.h\n> index 8e1af26..daa2d4e 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -724,6 +724,7 @@ extern struct alternate_object_database {\n> \tchar base[FLEX_ARRAY]; /* more */\n> } *alt_odb_list;\n> extern void prepare_alt_odb(void);\n> +extern int add_alt_odb(const char *path);\n> extern void add_to_alternates_file(const char *reference);\n> typedef int alt_odb_fn(struct alternate_object_database *, void *);\n> extern void foreach_alt_odb(alt_odb_fn, void*);\n> diff --git a/sha1_file.c b/sha1_file.c\n> index f08493f..19f9725 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -356,6 +356,11 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,\n> \t}\n> }\n> \n> +int add_alt_odb(const char *path)\n> +{\n> +\treturn link_alt_odb_entry(path, strlen(path), NULL, 0);\n\nThis function can return the error message \"object directory %s does not \nexist; check .git/objects/info/alternates.\" Maybe you want to change \nthat, even if the user you are introducing might not hit that code path.\n\nCiao,\nDscho\n"},{"id":"101006","messageId":"alpine.DEB.1.00.0901181635290.3586@pacific.mpi-cbg.de","threadId":"17232","inReplyTo":"1232275999-14852-3-git-send-email-hjemli@gmail.com","subject":"Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-18T15:48:23Z","receivedAt":"2009-01-18T15:48:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 18 Jan 2009, Lars Hjemli wrote:\n\n> diff --git a/environment.c b/environment.c\n> index e278bce..35cc557 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -53,6 +53,8 @@ static char *work_tree;\n> static const char *git_dir;\n> static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;\n> \n> +static int traverse_gitlinks = 0;\n> +\n> static void setup_git_env(void)\n> {\n> \tgit_dir = getenv(GIT_DIR_ENVIRONMENT);\n> @@ -159,3 +161,13 @@ int set_git_dir(const char *path)\n> \tsetup_git_env();\n> \treturn 0;\n> }\n> +\n> +int get_traverse_gitlinks()\n> +{\n> +\treturn traverse_gitlinks;\n> +}\n> +\n> +void set_traverse_gitlinks(int traverse)\n> +{\n> +\ttraverse_gitlinks = traverse;\n> +}\n\nIf you have full accessors anyway, it is much easier and cleaner to make \nthis a global variable to begin with.\n\nHowever, environment.c is reserved for things that come from the config \nand can be overridden by the user. That is certainly not the case for \ntraverse_gitlinks.\n\nBut let's think about it again: should traverse_gitlinks be a global \nvarible at all? I think not. It should be a per-call decision.\n\n > diff --git a/tree.c b/tree.c\n> index 03e782a..87cf309 100644\n> --- a/tree.c\n> +++ b/tree.c\n> @@ -5,6 +5,7 @@\n> #include \"commit.h\"\n> #include \"tag.h\"\n> #include \"tree-walk.h\"\n> +#include \"refs.h\"\n> \n> const char *tree_type = \"tree\";\n> \n> @@ -89,6 +90,61 @@ static int match_tree_entry(const char *base, int baselen, const char *path, uns\n> \treturn 0;\n> }\n> \n> +/* Try to add the objectdb of a submodule */\n> +int add_gitlink_odb(char *relpath)\n\nThis wants to be static.\n\n> +{\n> +\tconst char *odbpath;\n> +\tstruct stat st;\n> +\n> +\todbpath = read_gitfile_gently(mkpath(\"%s/.git\", relpath));\n> +\tif (!odbpath)\n> +\t\todbpath = mkpath(\"%s/.git/objects\", relpath);\n> +\n> +\tif (stat(odbpath, &st))\n> +\t\treturn 1;\n> +\n> +\treturn add_alt_odb(odbpath);\n> +}\n> +\n> +/* Check if we should recurse into the specified submodule */\n> +int traverse_gitlink(char *path, const unsigned char *commit_sha1,\n\nThis, too.\n\n> +\t\t struct tree **subtree)\n> +{\n> +\tunsigned char sha1[20];\n> +\tint linked_odb = 0;\n> +\tstruct commit *commit;\n> +\tvoid *buffer;\n> +\tenum object_type type;\n> +\tunsigned long size;\n> +\n> +\thashcpy(sha1, commit_sha1);\n> +\tif (!add_gitlink_odb(path)) {\n> +\t\tlinked_odb = 1;\n> +\t\tif (resolve_gitlink_ref(path, \"HEAD\", sha1))\n> +\t\t\tdie(\"Unable to lookup HEAD in %s\", path);\n> +\t}\n\nWhy would you want to continue if add_gitlink_odb() did not find a checked \nout submodule?\n\nSeems you want to fall back to look in the superproject's object database. \nBut I think that is wrong, as I have a superproject with many platform \ndependent submodules, only one of which is checked out, and for \nconvenience, the submodules all live in the superproject's repository.\n\nBut I might misunderstand your code.\n\n> +\tcommit = lookup_commit(sha1);\n> +\tif (!commit)\n> +\t\tdie(\"traverse_gitlink(): internal error\");\n\ns/internal error/could not access commit '%s' of submodule '%s'\",\n\t\t\tsha1_to_hex(sha1), path);/\n\n> @@ -132,6 +188,30 @@ int read_tree_recursive(struct tree *tree,\n> \t\t\t\treturn -1;\n> \t\t\tcontinue;\n> \t\t}\n> +\t\tif (S_ISGITLINK(entry.mode) && get_traverse_gitlinks()) {\n\nLike I said, traverse_gitlinks should be a flag to read_tree_recursive. \nSo preferably, you should add a parameter 'flags' and make that option an \nenum.\n\n> +\t\t\tint retval;\n> +\t\t\tchar *newbase;\n> +\t\t\tstruct tree *subtree;\n> +\t\t\tunsigned int pathlen = tree_entry_len(entry.path, entry.sha1);\n\nNit: Long line.\n\n> +\n> +\t\t\tnewbase = xmalloc(baselen + 1 + pathlen);\n> +\t\t\tmemcpy(newbase, base, baselen);\n> +\t\t\tmemcpy(newbase + baselen, entry.path, pathlen);\n> +\t\t\tnewbase[baselen + pathlen] = 0;\n\nWe have strbufs for that.\n\n> +\t\t\tif (!traverse_gitlink(newbase, entry.sha1, &subtree)) {\n> +\t\t\t\tfree(newbase);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t\tnewbase[baselen + pathlen] = '/';\n\n... to avoid this off-by-one.\n\n> +\t\t\tretval = read_tree_recursive(subtree,\n> +\t\t\t\t\t\t newbase,\n> +\t\t\t\t\t\t baselen + pathlen + 1,\n> +\t\t\t\t\t\t stage, match, fn, context);\n> +\t\t\tfree(newbase);\n> +\t\t\tif (retval)\n> +\t\t\t\treturn -1;\n> +\t\t\tcontinue;\n> +\t\t}\n> \t}\n> \treturn 0;\n> }\n\nCiao,\nDscho\n"},{"id":"101007","messageId":"alpine.DEB.1.00.0901181650340.3586@pacific.mpi-cbg.de","threadId":"17232","inReplyTo":"1232275999-14852-4-git-send-email-hjemli@gmail.com","subject":"Re: [PATCH 3/3] git-archive: add support for --submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-18T15:51:52Z","receivedAt":"2009-01-18T15:51:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 18 Jan 2009, Lars Hjemli wrote:\n\n> @@ -320,6 +323,7 @@ static int parse_archive_args(int argc, const char **argv,\n> \targs->base = base;\n> \targs->baselen = strlen(base);\n> \n> +\tset_traverse_gitlinks(submodules);\n\nAs I said, this is a per-call thing. So you need to add that option to \nthe archiver_args struct and use it in write_archive_entries().\n\nCiao,\nDscho\n"},{"id":"101009","messageId":"1232294130-11409-1-git-send-email-hjemli@gmail.com","threadId":"17232","inReplyTo":"alpine.DEB.1.00.0901181629590.3586@pacific.mpi-cbg.de","subject":"[PATCH] sha1_file: add function to insert alternate object db","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2009-01-18T15:55:30Z","receivedAt":"2009-01-18T15:55:30Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"This function will be used when implementing traversal into submodules.\n\nSigned-off-by: Lars Hjemli \n---\n\nOn Sun, Jan 18, 2009 at 16:32, Johannes Schindelin wrote:\n>\n> On Sun, 18 Jan 2009, Lars Hjemli wrote:\n>> +int add_alt_odb(const char *path)\n>> +{\n>> + return link_alt_odb_entry(path, strlen(path), NULL, 0);\n>\n> This function can return the error message \"object directory %s does not\n> exist; check .git/objects/info/alternates.\" Maybe you want to change\n> that, even if the user you are introducing might not hit that code path.\n\nSomething like this, maybe?\n\n\n cache.h | 1 +\n sha1_file.c | 17 ++++++++++++-----\n 2 files changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 8e1af26..0dbe2a6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -724,6 +724,7 @@ extern struct alternate_object_database {\n \tchar base[FLEX_ARRAY]; /* more */\n } *alt_odb_list;\n extern void prepare_alt_odb(void);\n+extern int add_alt_odb(const char *path, int quiet);\n extern void add_to_alternates_file(const char *reference);\n typedef int alt_odb_fn(struct alternate_object_database *, void *);\n extern void foreach_alt_odb(alt_odb_fn, void*);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f08493f..4b7e691 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -252,7 +252,8 @@ static void read_info_alternates(const char * alternates, int depth);\n * SHA1, an extra slash for the first level indirection, and the\n * terminating NUL.\n */\n-static int link_alt_odb_entry(const char * entry, int len, const char * relative_base, int depth)\n+static int link_alt_odb_entry(const char * entry, int len,\n+\t\t\t const char * relative_base, int depth, int quiet)\n {\n \tconst char *objdir = get_object_directory();\n \tstruct alternate_object_database *ent;\n@@ -285,9 +286,10 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative\n \n \t/* Detect cases where alternate disappeared */\n \tif (!is_directory(ent->base)) {\n-\t\terror(\"object directory %s does not exist; \"\n-\t\t \"check .git/objects/info/alternates.\",\n-\t\t ent->base);\n+\t\tif (!quiet)\n+\t\t\terror(\"object directory %s does not exist; \"\n+\t\t\t \"check .git/objects/info/alternates.\",\n+\t\t\t ent->base);\n \t\tfree(ent);\n \t\treturn -1;\n \t}\n@@ -347,7 +349,7 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,\n \t\t\t\t\t\trelative_base, last);\n \t\t\t} else {\n \t\t\t\tlink_alt_odb_entry(last, cp - last,\n-\t\t\t\t\t\trelative_base, depth);\n+\t\t\t\t\t\trelative_base, depth, 0);\n \t\t\t}\n \t\t}\n \t\twhile (cp < ep && *cp == sep)\n@@ -356,6 +358,11 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,\n \t}\n }\n \n+int add_alt_odb(const char *path, int quiet)\n+{\n+\treturn link_alt_odb_entry(path, strlen(path), NULL, 0, quiet);\n+}\n+\n static void read_info_alternates(const char * relative_base, int depth)\n {\n \tchar *map;\n-- \n1.6.1.150.g5e733b\n"},{"id":"101012","messageId":"49735530.4090901@lsrfire.ath.cx","threadId":"17232","inReplyTo":"1232275999-14852-3-git-send-email-hjemli@gmail.com","subject":"Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2009-01-18T16:13:36Z","receivedAt":"2009-01-18T16:13:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Lars Hjemli schrieb:\n> The traversal of submodules is only triggered if the current submodule\n> HEAD commit object is accessible. To this end, read_tree_recursive()\n> will try to insert the submodule odb as an alternate odb but the lack\n> of such an odb is not treated as an error since it is then assumed that\n> the user is not interested in the submodule content. However, if the\n> submodule odb is found it is treated as an error if the HEAD commit\n> object is missing.\n\nCallers of read_tree_recursive() specify a tree to traverse.\nUnconditionally using the HEAD of submodules feels a bit restrictive,\nbut I don't use submodules, so I have no idea what I'm actually talking\nabout here. :)\n\n> int read_tree_recursive(struct tree *tree,\n> \t\t\tconst char *base, int baselen,\n> \t\t\tint stage, const char **match,\n> @@ -132,6 +188,30 @@ int read_tree_recursive(struct tree *tree,\n> \t\t\t\treturn -1;\n> \t\t\tcontinue;\n> \t\t}\n> +\t\tif (S_ISGITLINK(entry.mode) && get_traverse_gitlinks()) {\n> +\t\t\tint retval;\n> +\t\t\tchar *newbase;\n> +\t\t\tstruct tree *subtree;\n> +\t\t\tunsigned int pathlen = tree_entry_len(entry.path, entry.sha1);\n> +\n> +\t\t\tnewbase = xmalloc(baselen + 1 + pathlen);\n> +\t\t\tmemcpy(newbase, base, baselen);\n> +\t\t\tmemcpy(newbase + baselen, entry.path, pathlen);\n> +\t\t\tnewbase[baselen + pathlen] = 0;\n> +\t\t\tif (!traverse_gitlink(newbase, entry.sha1, &subtree)) {\n> +\t\t\t\tfree(newbase);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t\tnewbase[baselen + pathlen] = '/';\n> +\t\t\tretval = read_tree_recursive(subtree,\n> +\t\t\t\t\t\t newbase,\n> +\t\t\t\t\t\t baselen + pathlen + 1,\n> +\t\t\t\t\t\t stage, match, fn, context);\n> +\t\t\tfree(newbase);\n> +\t\t\tif (retval)\n> +\t\t\t\treturn -1;\n> +\t\t\tcontinue;\n> +\t\t}\n> \t}\n> \treturn 0;\n> }\n\nYou don't need to call get_traverse_gitlinks() in the if statement above\nif you make all read_tree_recursive() callback functions return 0 for\ngitlinks that they don't want to follow and READ_TREE_RECURSIVE for\nthose they do. It's cleaner without the static variable and its\naccessors and more flexible, too: the callbacks might decide to traverse\nonly certain submodules.\n\nRené\n"},{"id":"101016","messageId":"8c5c35580901180837i6e835d98ob8875ce1b8ad3011@mail.gmail.com","threadId":"17232","inReplyTo":"49735530.4090901@lsrfire.ath.cx","subject":"Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2009-01-18T16:37:39Z","receivedAt":"2009-01-18T16:37:39Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On Sun, Jan 18, 2009 at 17:13, René Scharfe wrote:\n> Lars Hjemli schrieb:\n>> The traversal of submodules is only triggered if the current submodule\n>> HEAD commit object is accessible. To this end, read_tree_recursive()\n>> will try to insert the submodule odb as an alternate odb but the lack\n>> of such an odb is not treated as an error since it is then assumed that\n>> the user is not interested in the submodule content. However, if the\n>> submodule odb is found it is treated as an error if the HEAD commit\n>> object is missing.\n>\n> Callers of read_tree_recursive() specify a tree to traverse.\n> Unconditionally using the HEAD of submodules feels a bit restrictive,\n> but I don't use submodules, so I have no idea what I'm actually talking\n> about here. :)\n\nFor bare repositories (where the submodule repo is added to\nobjects/info/alternates), following the tree of the linked commit is\nthe only option. And for non-bare repositories with the submodule\nchecked out, I think we should honor the users choice of checked out\nHEAD in the submodule (especially since we don't have any other way to\nspecify which submodule commit to follow).\n\n\n>\n>> int read_tree_recursive(struct tree *tree,\n>> const char *base, int baselen,\n>> int stage, const char **match,\n>> @@ -132,6 +188,30 @@ int read_tree_recursive(struct tree *tree,\n>> return -1;\n>> continue;\n>> }\n>> + if (S_ISGITLINK(entry.mode) && get_traverse_gitlinks()) {\n>> + int retval;\n>> + char *newbase;\n>> + struct tree *subtree;\n>> + unsigned int pathlen = tree_entry_len(entry.path, entry.sha1);\n>> +\n>> + newbase = xmalloc(baselen + 1 + pathlen);\n>> + memcpy(newbase, base, baselen);\n>> + memcpy(newbase + baselen, entry.path, pathlen);\n>> + newbase[baselen + pathlen] = 0;\n>> + if (!traverse_gitlink(newbase, entry.sha1, &subtree)) {\n>> + free(newbase);\n>> + continue;\n>> + }\n>> + newbase[baselen + pathlen] = '/';\n>> + retval = read_tree_recursive(subtree,\n>> + newbase,\n>> + baselen + pathlen + 1,\n>> + stage, match, fn, context);\n>> + free(newbase);\n>> + if (retval)\n>> + return -1;\n>> + continue;\n>> + }\n>> }\n>> return 0;\n>> }\n>\n> You don't need to call get_traverse_gitlinks() in the if statement above\n> if you make all read_tree_recursive() callback functions return 0 for\n> gitlinks that they don't want to follow and READ_TREE_RECURSIVE for\n> those they do. It's cleaner without the static variable and its\n> accessors and more flexible, too: the callbacks might decide to traverse\n> only certain submodules.\n\nI like the idea, but it will require thorough review of all\nread_tree_recursive() consumers. So now we've got three different\napproaches:\n* me: global setting\n* dscho: parameter to read_tree_recursive()\n* you: accept the return value from the callback function\n\nJunio, what would you prefer?\n\n--\nlarsh\n"},{"id":"101030","messageId":"8c5c35580901180945u17a69140vff2736765ee6073@mail.gmail.com","threadId":"17232","inReplyTo":"alpine.DEB.1.00.0901181635290.3586@pacific.mpi-cbg.de","subject":"Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2009-01-18T17:45:40Z","receivedAt":"2009-01-18T17:45:40Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On Sun, Jan 18, 2009 at 16:48, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Sun, 18 Jan 2009, Lars Hjemli wrote:\n>\n>> diff --git a/environment.c b/environment.c\n>> @@ -159,3 +161,13 @@ int set_git_dir(const char *path)\n>> setup_git_env();\n>> return 0;\n>> }\n>> +\n>> +int get_traverse_gitlinks()\n>> +{\n>> + return traverse_gitlinks;\n>> +}\n>> +\n>> +void set_traverse_gitlinks(int traverse)\n>> +{\n>> + traverse_gitlinks = traverse;\n>> +}\n>\n> If you have full accessors anyway, it is much easier and cleaner to make\n> this a global variable to begin with.\n>\n> However, environment.c is reserved for things that come from the config\n> and can be overridden by the user. That is certainly not the case for\n> traverse_gitlinks.\n>\n> But let's think about it again: should traverse_gitlinks be a global\n> varible at all? I think not. It should be a per-call decision.\n\nYes, it might be a cleaner solution to add an extra parameter to\nread_tree_recursive(), and since it currently has only 7 callsites the\npatch wouldn't be too big either. Junio, what do you think?\n\n\n>> +/* Try to add the objectdb of a submodule */\n>> +int add_gitlink_odb(char *relpath)\n>\n> This wants to be static.\n\nThanks\n\n>\n>> +{\n>> + const char *odbpath;\n>> + struct stat st;\n>> +\n>> + odbpath = read_gitfile_gently(mkpath(\"%s/.git\", relpath));\n>> + if (!odbpath)\n>> + odbpath = mkpath(\"%s/.git/objects\", relpath);\n>> +\n>> + if (stat(odbpath, &st))\n>> + return 1;\n>> +\n>> + return add_alt_odb(odbpath);\n>> +}\n>> +\n>> +/* Check if we should recurse into the specified submodule */\n>> +int traverse_gitlink(char *path, const unsigned char *commit_sha1,\n>\n> This, too.\n\nThanks again ;-)\n\n>\n>> + struct tree **subtree)\n>> +{\n>> + unsigned char sha1[20];\n>> + int linked_odb = 0;\n>> + struct commit *commit;\n>> + void *buffer;\n>> + enum object_type type;\n>> + unsigned long size;\n>> +\n>> + hashcpy(sha1, commit_sha1);\n>> + if (!add_gitlink_odb(path)) {\n>> + linked_odb = 1;\n>> + if (resolve_gitlink_ref(path, \"HEAD\", sha1))\n>> + die(\"Unable to lookup HEAD in %s\", path);\n>> + }\n>\n> Why would you want to continue if add_gitlink_odb() did not find a checked\n> out submodule?\n>\n> Seems you want to fall back to look in the superproject's object database.\n> But I think that is wrong, as I have a superproject with many platform\n> dependent submodules, only one of which is checked out, and for\n> convenience, the submodules all live in the superproject's repository.\n\nActually, I want this to work for bare repositories by specifying the\nsubmodule odbs in the alternates file. So if the current submodule odb\nwasn't found my plan was to check if the commit object was accessible\nanyways but don't die() if it wasn't.\n\n\n>> + commit = lookup_commit(sha1);\n>> + if (!commit)\n>> + die(\"traverse_gitlink(): internal error\");\n>\n> s/internal error/could not access commit '%s' of submodule '%s'\",\n> sha1_to_hex(sha1), path);/\n\nOk (I belive this codepath is virtually impossible to hit, hence the\n\"internal error\", but I could of course be mistaken).\n\n\n>> @@ -132,6 +188,30 @@ int read_tree_recursive(struct tree *tree,\n>> return -1;\n>> continue;\n>> }\n>> + if (S_ISGITLINK(entry.mode) && get_traverse_gitlinks()) {\n>\n> Like I said, traverse_gitlinks should be a flag to read_tree_recursive.\n> So preferably, you should add a parameter 'flags' and make that option an\n> enum.\n>\n>> + int retval;\n>> + char *newbase;\n>> + struct tree *subtree;\n>> + unsigned int pathlen = tree_entry_len(entry.path, entry.sha1);\n>\n> Nit: Long line.\n\nWill fix\n\n>\n>> +\n>> + newbase = xmalloc(baselen + 1 + pathlen);\n>> + memcpy(newbase, base, baselen);\n>> + memcpy(newbase + baselen, entry.path, pathlen);\n>> + newbase[baselen + pathlen] = 0;\n>\n> We have strbufs for that.\n>\n>> + if (!traverse_gitlink(newbase, entry.sha1, &subtree)) {\n>> + free(newbase);\n>> + continue;\n>> + }\n>> + newbase[baselen + pathlen] = '/';\n>\n> ... to avoid this off-by-one.\n\nActually, I don't think this is off-by-one, since (baselen + pathlen +\n1) is passed along to read_tree_recursive() as the new baselen. But\nusing strbufs might be cleaner anyways.\n\nThanks for the review.\n\n--\nlarsh\n"},{"id":"101033","messageId":"alpine.DEB.1.00.0901181929220.3586@pacific.mpi-cbg.de","threadId":"17232","inReplyTo":"8c5c35580901180945u17a69140vff2736765ee6073@mail.gmail.com","subject":"Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-18T18:33:14Z","receivedAt":"2009-01-18T18:33:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 18 Jan 2009, Lars Hjemli wrote:\n\n> On Sun, Jan 18, 2009 at 16:48, Johannes Schindelin\n> wrote:\n>\n> > On Sun, 18 Jan 2009, Lars Hjemli wrote:\n> >\n> >> + struct tree **subtree)\n> >> +{\n> >> + unsigned char sha1[20];\n> >> + int linked_odb = 0;\n> >> + struct commit *commit;\n> >> + void *buffer;\n> >> + enum object_type type;\n> >> + unsigned long size;\n> >> +\n> >> + hashcpy(sha1, commit_sha1);\n> >> + if (!add_gitlink_odb(path)) {\n> >> + linked_odb = 1;\n> >> + if (resolve_gitlink_ref(path, \"HEAD\", sha1))\n> >> + die(\"Unable to lookup HEAD in %s\", path);\n> >> + }\n> >\n> > Why would you want to continue if add_gitlink_odb() did not find a checked\n> > out submodule?\n> >\n> > Seems you want to fall back to look in the superproject's object database.\n> > But I think that is wrong, as I have a superproject with many platform\n> > dependent submodules, only one of which is checked out, and for\n> > convenience, the submodules all live in the superproject's repository.\n> \n> Actually, I want this to work for bare repositories by specifying the\n> submodule odbs in the alternates file. So if the current submodule odb\n> wasn't found my plan was to check if the commit object was accessible\n> anyways but don't die() if it wasn't.\n\nPlease make that an explicit option (cannot think of a good name, though), \notherwise I will not be able to use your feature. Making it the default \nwould be inconsistent with the rest of our submodules framework.\n\n> >> + commit = lookup_commit(sha1);\n> >> + if (!commit)\n> >> + die(\"traverse_gitlink(): internal error\");\n> >\n> > s/internal error/could not access commit '%s' of submodule '%s'\",\n> > sha1_to_hex(sha1), path);/\n> \n> Ok (I belive this codepath is virtually impossible to hit, hence the\n> \"internal error\", but I could of course be mistaken).\n\nYou make it a function that is exported to other parts of Git in cache.h. \nSo you might just as well expect it to be used by other parts at some \nstage.\n\nCiao,\nDscho\n"},{"id":"101035","messageId":"7vfxjgxwv7.fsf@gitster.siamese.dyndns.org","threadId":"17232","inReplyTo":"8c5c35580901180837i6e835d98ob8875ce1b8ad3011@mail.gmail.com","subject":"Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-18T19:00:44Z","receivedAt":"2009-01-18T19:00:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Hjemli writes:\n\n> I like the idea, but it will require thorough review of all\n> read_tree_recursive() consumers. So now we've got three different\n> approaches:\n> * me: global setting\n> * dscho: parameter to read_tree_recursive()\n> * you: accept the return value from the callback function\n>\n> Junio, what would you prefer?\n\nAs usual René has the best taste in designing things ;-)\n"},{"id":"101047","messageId":"8c5c35580901181145x2e14fe0fq4ab0e94c13bad38a@mail.gmail.com","threadId":"17232","inReplyTo":"alpine.DEB.1.00.0901181929220.3586@pacific.mpi-cbg.de","subject":"Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2009-01-18T19:45:00Z","receivedAt":"2009-01-18T19:45:00Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On Sun, Jan 18, 2009 at 19:33, Johannes Schindelin\n wrote:\n> On Sun, 18 Jan 2009, Lars Hjemli wrote:\n>> Actually, I want this to work for bare repositories by specifying the\n>> submodule odbs in the alternates file. So if the current submodule odb\n>> wasn't found my plan was to check if the commit object was accessible\n>> anyways but don't die() if it wasn't.\n>\n> Please make that an explicit option (cannot think of a good name, though),\n> otherwise I will not be able to use your feature. Making it the default\n> would be inconsistent with the rest of our submodules framework.\n\nWould a test on is_bare_repository() suffice for your use-case? That\nis, something like this:\n\n\tif (!add_gitlink_odb(path->buf)) {\n\t\tlinked_odb = 1;\n\t\tif (resolve_gitlink_ref(path->buf, \"HEAD\", sha1))\n\t\t\tdie(\"Unable to lookup HEAD in %s\", path->buf);\n\t} else if (!is_bare_repository())\n\t\treturn 0;\n\nIf this isn't good enough, how do you propose it be solved?\n\n\n>\n>> >> + commit = lookup_commit(sha1);\n>> >> + if (!commit)\n>> >> + die(\"traverse_gitlink(): internal error\");\n>> >\n>> > s/internal error/could not access commit '%s' of submodule '%s'\",\n>> > sha1_to_hex(sha1), path);/\n>>\n>> Ok (I belive this codepath is virtually impossible to hit, hence the\n>> \"internal error\", but I could of course be mistaken).\n>\n> You make it a function that is exported to other parts of Git in cache.h.\n> So you might just as well expect it to be used by other parts at some\n> stage.\n\nThis function is local to tree.c, but your point is still valid.\n\n--\nlarsh\n"},{"id":"101048","messageId":"8c5c35580901181150t1827455elbe2be224a33f1b36@mail.gmail.com","threadId":"17232","inReplyTo":"7vfxjgxwv7.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2009-01-18T19:50:06Z","receivedAt":"2009-01-18T19:50:06Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On Sun, Jan 18, 2009 at 20:00, Junio C Hamano wrote:\n> Lars Hjemli writes:\n>\n>> I like the idea, but it will require thorough review of all\n>> read_tree_recursive() consumers. So now we've got three different\n>> approaches:\n>> * me: global setting\n>> * dscho: parameter to read_tree_recursive()\n>> * you: accept the return value from the callback function\n>>\n>> Junio, what would you prefer?\n>\n> As usual René has the best taste in designing things ;-)\n\nAgreed. I've pushed an updated series using his approach to the\nlh/traverse-gitlinks branch in git://hjemli.net/pub/git/git\n(http://hjemli.net/git/git/log/?h=lh/traverse-gitlinks), but without a\nfix for Dscho's concern about the behaviour in\ntree.c::traverse_gitlink(). Hopefully I'll get to send a reworked\nseries later tonight.\n\n--\nlarsh\n"},{"id":"101057","messageId":"alpine.DEB.1.00.0901182201140.3586@pacific.mpi-cbg.de","threadId":"17232","inReplyTo":"8c5c35580901181145x2e14fe0fq4ab0e94c13bad38a@mail.gmail.com","subject":"Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-18T21:02:27Z","receivedAt":"2009-01-18T21:02:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 18 Jan 2009, Lars Hjemli wrote:\n\n> On Sun, Jan 18, 2009 at 19:33, Johannes Schindelin\n> wrote:\n> > On Sun, 18 Jan 2009, Lars Hjemli wrote:\n> >> Actually, I want this to work for bare repositories by specifying the\n> >> submodule odbs in the alternates file. So if the current submodule odb\n> >> wasn't found my plan was to check if the commit object was accessible\n> >> anyways but don't die() if it wasn't.\n> >\n> > Please make that an explicit option (cannot think of a good name, though),\n> > otherwise I will not be able to use your feature. Making it the default\n> > would be inconsistent with the rest of our submodules framework.\n> \n> Would a test on is_bare_repository() suffice for your use-case?\n\nNo. Inconsistent is inconsistent.\n\n> If this isn't good enough, how do you propose it be solved?\n\nAs I said, with an extra option that you _have_ to pass when you want \nthat behavior.\n\n> >> >> + commit = lookup_commit(sha1);\n> >> >> + if (!commit)\n> >> >> + die(\"traverse_gitlink(): internal error\");\n> >> >\n> >> > s/internal error/could not access commit '%s' of submodule '%s'\",\n> >> > sha1_to_hex(sha1), path);/\n> >>\n> >> Ok (I belive this codepath is virtually impossible to hit, hence the\n> >> \"internal error\", but I could of course be mistaken).\n> >\n> > You make it a function that is exported to other parts of Git in cache.h.\n> > So you might just as well expect it to be used by other parts at some\n> > stage.\n> \n> This function is local to tree.c, but your point is still valid.\n\nMy point is still valid because I never talked about the static function, \nbut the non-static one which calls the static one.\n\nCiao,\nDscho\n"},{"id":"101065","messageId":"8c5c35580901181331v5e54f82fxc6a042962ff1cd06@mail.gmail.com","threadId":"17232","inReplyTo":"alpine.DEB.1.00.0901182201140.3586@pacific.mpi-cbg.de","subject":"Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2009-01-18T21:31:28Z","receivedAt":"2009-01-18T21:31:28Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On Sun, Jan 18, 2009 at 22:02, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Sun, 18 Jan 2009, Lars Hjemli wrote:\n>\n>> On Sun, Jan 18, 2009 at 19:33, Johannes Schindelin\n>> wrote:\n>> > On Sun, 18 Jan 2009, Lars Hjemli wrote:\n>> >> Actually, I want this to work for bare repositories by specifying the\n>> >> submodule odbs in the alternates file. So if the current submodule odb\n>> >> wasn't found my plan was to check if the commit object was accessible\n>> >> anyways but don't die() if it wasn't.\n>> >\n>> > Please make that an explicit option (cannot think of a good name, though),\n>> > otherwise I will not be able to use your feature. Making it the default\n>> > would be inconsistent with the rest of our submodules framework.\n>>\n>> Would a test on is_bare_repository() suffice for your use-case?\n>\n> No. Inconsistent is inconsistent.\n>\n>> If this isn't good enough, how do you propose it be solved?\n>\n> As I said, with an extra option that you _have_ to pass when you want\n> that behavior.\n\nMy concern is how to discern between wanted and unwanted submodules in\na bare repository.\n\nWith my proposed solution `git archive --submodules HEAD` in a bare\nrepository would only include the content of the submodule repos\nlisted in objects/info/alternates (since the commit referenced by the\ngitlink would then be reachable).\n\nBut you mentioned that you had a repository where all the objects of\nall the submodules where stored in the odb of the superproject. With\nmy solution, `git archive --submodules HEAD` in your (bare) repo would\nthen always include the content of all the submodules (since all the\nobjects would always be reachable), and I believe this is the behavior\nyou don't like.\n\nSo, would you rather have something like `git archive --submodules=foo\n--submodules=bar HEAD` to explicitly tell which submodule paths to\ninclude in the archive when executed in a bare repo?\n\n--\nlarsh\n"},{"id":"101067","messageId":"alpine.DEB.1.00.0901182244310.3586@pacific.mpi-cbg.de","threadId":"17232","inReplyTo":"8c5c35580901181331v5e54f82fxc6a042962ff1cd06@mail.gmail.com","subject":"Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-18T21:55:34Z","receivedAt":"2009-01-18T21:55:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 18 Jan 2009, Lars Hjemli wrote:\n\n> So, would you rather have something like `git archive --submodules=foo \n> --submodules=bar HEAD` to explicitly tell which submodule paths to \n> include in the archive when executed in a bare repo?\n\nThat does not quite say what you tried to do, does it? You tried to \ntraverse submodules whose commit can be found in the object database.\n\nSetting aside the fact that we usually try to avoid accessing unreachable \nobjects, which your handling does not do, our \"git submodule\" does not do \nthat either; it only handles submodules that are checked out.\n\nNow, this behavior might be wanted, in bare as well as in non-bare \nrepositories, but I think it should be triggered by an option, such as \n\"--submodules=look-in-superprojects-odb\".\n\nI know, I know, the naming is horrible, but I find it just wrong to \nintroduce a behavior that would only confuse users because it introduces \ninconsistent behavior. As it is, we see too many confused users in #git \nalready with consistent behavior [*1*].\n\nCiao,\nDscho\n\n[*1*] Maybe we should allow cloning empty repositories (with no default \nbranch at all), disable pushing into checked out branches by default, and \nmake \"git add empty-dir/\" add a .gitignore and add that -- to squash at \nleast half of the questions inside #git so that we can go back to fooling \naround there.\n"},{"id":"101069","messageId":"8c5c35580901181446n3c36a345m5d8e78764a85c123@mail.gmail.com","threadId":"17232","inReplyTo":"alpine.DEB.1.00.0901182244310.3586@pacific.mpi-cbg.de","subject":"Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules","fromName":"Lars Hjemli","fromEmail":"hjemli@gmail.com","sentAt":"2009-01-18T22:46:00Z","receivedAt":"2009-01-18T22:46:00Z","isPatch":true,"sender":{"key":"hjemli@gmail.com","avatar":null},"body":"On Sun, Jan 18, 2009 at 22:55, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Sun, 18 Jan 2009, Lars Hjemli wrote:\n>\n>> So, would you rather have something like `git archive --submodules=foo\n>> --submodules=bar HEAD` to explicitly tell which submodule paths to\n>> include in the archive when executed in a bare repo?\n>\n> That does not quite say what you tried to do, does it? You tried to\n> traverse submodules whose commit can be found in the object database.\n>\n> Setting aside the fact that we usually try to avoid accessing unreachable\n> objects, which your handling does not do, our \"git submodule\" does not do\n> that either; it only handles submodules that are checked out.\n>\n> Now, this behavior might be wanted, in bare as well as in non-bare\n> repositories, but I think it should be triggered by an option, such as\n> \"--submodules=look-in-superprojects-odb\".\n\nSorry, but if your concern is whether to traverse a submodule in a\nbare repo when the submodule isn't checked out (yeah, contradiction in\nterms), I just don't see the point.\n\nFor non-bare repositories the policy has always been to ignore\nsubmodules which isn't checked out, but for bare repositories there is\nno obvious way (for me, at least) to apply the same policy. Therefore\nI proposed to traverse all submodules where the linked commit is\nreachable, but as you pointed out this would be wrong for non-bare\nrepositories.\n\nI then modified my proposal to include a check on\nis_bare_repository(): If we're not in a bare repository,\nread_tree_recursive() is only allowed to recurse into checked out\nsubmodules. But if we're in a bare repository, read_tree_recursive()\nis allowed to recurse into any submodule with a reachable commit.\n\nNow then, if `--submodules=look-in-superprojects-odb` should be\nrequired to trigger the latter behavior, running `git archive\n--submodules HEAD` in a bare repository would always produce identical\noutput as `git archive HEAD` and this is why I don't understand the\ngain of 'look-in-superprojects-odb' (I thought you wanted to limit\nwhich of the reachable submodules to recurse into).\n\n--\nlarsh\n"},{"id":"101075","messageId":"alpine.DEB.1.00.0901190218470.3586@pacific.mpi-cbg.de","threadId":"17232","inReplyTo":"8c5c35580901181446n3c36a345m5d8e78764a85c123@mail.gmail.com","subject":"Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-19T01:24:23Z","receivedAt":"2009-01-19T01:24:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 18 Jan 2009, Lars Hjemli wrote:\n\n> Sorry, but if your concern is whether to traverse a submodule in a bare \n> repo when the submodule isn't checked out (yeah, contradiction in \n> terms), I just don't see the point.\n\nObviously.\n\n> For non-bare repositories the policy has always been to ignore\n> submodules which isn't checked out, but for bare repositories there is\n> no obvious way (for me, at least) to apply the same policy.\n\nThere is one: we never traverse them in bare repositories.\n\nNever.\n\nYou are introducing that contradicts that on purpose. Which I do not \nlike at all.\n\nSure, what you want is a nifty feature, but you'll have to do it right.\n\nFor example, your handling for bare repositories precludes everybody from \nspecifying -- just for this particular call to git archive -- what \nsubmodules they want to include. And you preclude anybody from excluding \n-- just for this particular call to git archive -- certain submodules \nwhose commits just so happen to be present in the superproject.\n\nFor me, that is a sign of a bad user interface design.\n\nCiao,\nDscho\n\nP.S.: if you still don't get the point, I will just shut up, until the \nquestion crops up, and redirect every person confused by that behavior to \nyou. Be prepared.\n"},{"id":"101077","messageId":"alpine.GSO.2.00.0901181754190.9333@kiwi.cs.ucla.edu","threadId":"17232","inReplyTo":"alpine.DEB.1.00.0901190218470.3586@pacific.mpi-cbg.de","subject":"[PATCH/RFC v1 1/1] bug fix, diff whitespace ignore options","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-01-19T02:01:47Z","receivedAt":"2009-01-19T02:01:47Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":" Fixed bug in diff whitespace ignore options.\n It is now OK to specify more than one whitespace ignore option\n on the command line. In unit test 4015, expect success rather\n than failure for 4 cases.\n Note: I do not fully understand why this fix works, but it passes\n all 68 t4???-* diff test scripts.\n\nThe semantics of the three whitespace ignore flags\n{ -w, -b, --ignore-space-at-eol }\nobey a relation of transitive implication, i.e. the stronger\noptions imply the weaker options:\n-w implies the other two\n-b implies --ignore-space-at-eol\n--ignore-space-at-eol implies only itself\n\nTherefore it is never necessary to specify more than one of these\non the command line. Yet we imagine scenarios where software\nwrappers (e.g. GUIs, etc) generate command lines that switch on\nmore than one of these flags simultaneously. It is unreasonable\nto prohibit specifying more than one, since a new user might not\nimmediately discern the implication relation. Now we call such\na command line valid and legal.\n\nSigned-off-by: Keith Cascio \n---\n t/t4015-diff-whitespace.sh | 8 ++++----\n xdiff/xutils.c | 22 ++++++++++++----------\n 2 files changed, 16 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex dbb608c..6d13da3 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -99,11 +99,11 @@ EOF\n git diff -w > out\n test_expect_success 'another test, with -w' 'test_cmp expect out'\n git diff -w -b > out\n-test_expect_failure 'another test, with -w -b' 'test_cmp expect out'\n+test_expect_success 'another test, with -w -b' 'test_cmp expect out'\n git diff -w --ignore-space-at-eol > out\n-test_expect_failure 'another test, with -w --ignore-space-at-eol' 'test_cmp expect out'\n+test_expect_success 'another test, with -w --ignore-space-at-eol' 'test_cmp expect out'\n git diff -w -b --ignore-space-at-eol > out\n-test_expect_failure 'another test, with -w -b --ignore-space-at-eol' 'test_cmp expect out'\n+test_expect_success 'another test, with -w -b --ignore-space-at-eol' 'test_cmp expect out'\n\n tr 'Q' '\\015' << EOF > expect\n diff --git a/x b/x\n@@ -123,7 +123,7 @@ EOF\n git diff -b > out\n test_expect_success 'another test, with -b' 'test_cmp expect out'\n git diff -b --ignore-space-at-eol > out\n-test_expect_failure 'another test, with -b --ignore-space-at-eol' 'test_cmp expect out'\n+test_expect_success 'another test, with -b --ignore-space-at-eol' 'test_cmp expect out'\n\n tr 'Q' '\\015' << EOF > expect\n diff --git a/x b/x\ndiff --git a/xdiff/xutils.c b/xdiff/xutils.c\nindex d7974d1..b9bda86 100644\n--- a/xdiff/xutils.c\n+++ b/xdiff/xutils.c\n@@ -245,17 +245,19 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,\n \t\t\twhile (ptr + 1 < top && isspace(ptr[1])\n \t\t\t\t\t&& ptr[1] != '\\n')\n \t\t\t\tptr++;\n-\t\t\tif (flags & XDF_IGNORE_WHITESPACE_CHANGE\n-\t\t\t\t\t&& ptr[1] != '\\n') {\n-\t\t\t\tha += (ha << 5);\n-\t\t\t\tha ^= (unsigned long) ' ';\n-\t\t\t}\n-\t\t\tif (flags & XDF_IGNORE_WHITESPACE_AT_EOL\n-\t\t\t\t\t&& ptr[1] != '\\n') {\n-\t\t\t\twhile (ptr2 != ptr + 1) {\n+\t\t\tif( ! ( flags & XDF_IGNORE_WHITESPACE )){\n+\t\t\t\tif( flags & XDF_IGNORE_WHITESPACE_CHANGE\n+\t\t\t\t\t\t&& ptr[1] != '\\n') {\n \t\t\t\t\tha += (ha << 5);\n-\t\t\t\t\tha ^= (unsigned long) *ptr2;\n-\t\t\t\t\tptr2++;\n+\t\t\t\t\tha ^= (unsigned long) ' ';\n+\t\t\t\t}\n+\t\t\t\telse if( flags & XDF_IGNORE_WHITESPACE_AT_EOL\n+\t\t\t\t\t\t&& ptr[1] != '\\n') {\n+\t\t\t\t\twhile (ptr2 != ptr + 1) {\n+\t\t\t\t\t\tha += (ha << 5);\n+\t\t\t\t\t\tha ^= (unsigned long) *ptr2;\n+\t\t\t\t\t\tptr2++;\n+\t\t\t\t\t}\n \t\t\t\t}\n \t\t\t}\n \t\t\tcontinue;\n-- \n1.6.1.203.ga83c8.dirty\n"},{"id":"101085","messageId":"7v7i4st2vb.fsf@gitster.siamese.dyndns.org","threadId":"17232","inReplyTo":"8c5c35580901180945u17a69140vff2736765ee6073@mail.gmail.com","subject":"Re: [PATCH 2/3] Teach read_tree_recursive() how to traverse into submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-19T03:02:16Z","receivedAt":"2009-01-19T03:02:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Hjemli writes:\n\n> On Sun, Jan 18, 2009 at 16:48, Johannes Schindelin\n> wrote:\n> ...\n>> Seems you want to fall back to look in the superproject's object database.\n>> But I think that is wrong, as I have a superproject with many platform\n>> dependent submodules, only one of which is checked out, and for\n>> convenience, the submodules all live in the superproject's repository.\n>\n> Actually, I want this to work for bare repositories by specifying the\n> submodule odbs in the alternates file. So if the current submodule odb\n> wasn't found my plan was to check if the commit object was accessible\n> anyways but don't die() if it wasn't.\n\nThe current submodule design is \"do not recurse into them by default\nwithout being told\" throughout the Porcelain. We can think of various\nways for the users to tell which submodules are of interest and which are\nuninteresting.\n\nThe most general solution would be to give a list of submodules you are\ninterested in recursing into from the command line, something like:\n\n $ git command --with-submodule=path1 --with-submodule=path2...\n\nThat approach would work equally \"well\" with a bare repository or with a\nnon-bare repository, but if you have N submodules, you need to give up to\nN extra options, which may be cumbersome (meaning, \"works equally well\"\nabove actually may mean \"works equally awkwardly\"). One way to solve\nawkwardness may be to support a mechanism that allows you to use\nconfiguration variables to name a group of submodules.\n\nIn addition to such configuration variables, you already have one default\ngroup of submodules, defined by the way you set up your work tree, when\nyour superproject does have a work tree. Some submodules have\nrepositories cloned in the work tree, while some don't, and the ones\nwithout clones can be defined as \"uninteresting ones\" (to the work tree\nowner) that are outside the default group. For many work tree oriented\noperations, it may even make sense to allow that group to be used with a\nsingle \"git command --with-submodule\" (i.e. when you do not say which\nsubmodule you mean, that can default to \"cloned\" group).\n\nI do not know \"has an entry in the superproject's alternate list that\npoints to its object store\" is a good basis to define another default\ngroup useful especially in a bare repository setting; you seem to be\nsuggesting that, and you might be correct.\n\nFor \"git archive\", however, I suspect the \"default group based on work\ntree checkout state\" may make the least sense. \"git archive HEAD\" is\nexpected to give a reproducible dump of the state recorded by the HEAD\ncommit no matter who runs it in what repository, and I think there should\nbe a conscious and explicit instruction from the end user that says \"Here\nis a dump from this commit in the superproject, *BUT* it was made together\nwith contents from this and that submodule\". Command line options that\nlist \"this and that submodule\" is explicit enough, and a configured\nnickname given to a known group of submodules from the command line may be\nso as well, but the group based on the checkout state feels a bit too\nimplicit and magical to my taste. The group based on the \"has an entry in\nsuperproject's alternates\" criterion is not much better in this regard,\nmethinks.\n\nAnother worrysome thing about \"git archive\" is that it marks the resulting\narchive with the commit object name the tarball was taken from. If you\nallow recursing into an arbitrary subset of submodule, a project with N\nsubmodules can produce 2^N different varieties of archive, all marked with\nthe same commit object name from the superproject. That might be a bit\ntoo confusing.\n"},{"id":"101093","messageId":"alpine.DEB.1.00.0901190446480.3586@pacific.mpi-cbg.de","threadId":"17232","inReplyTo":"alpine.GSO.2.00.0901181754190.9333@kiwi.cs.ucla.edu","subject":"Re: [PATCH/RFC v1 1/1] bug fix, diff whitespace ignore options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-19T03:53:22Z","receivedAt":"2009-01-19T03:53:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 18 Jan 2009, Keith Cascio wrote:\n\n> Fixed bug in diff whitespace ignore options.\n> It is now OK to specify more than one whitespace ignore option\n> on the command line. In unit test 4015, expect success rather\n> than failure for 4 cases.\n> Note: I do not fully understand why this fix works, but it passes\n> all 68 t4???-* diff test scripts.\n> \n> The semantics of the three whitespace ignore flags\n> { -w, -b, --ignore-space-at-eol }\n> obey a relation of transitive implication, i.e. the stronger\n> options imply the weaker options:\n> -w implies the other two\n> -b implies --ignore-space-at-eol\n> --ignore-space-at-eol implies only itself\n> \n> Therefore it is never necessary to specify more than one of these\n> on the command line. Yet we imagine scenarios where software\n> wrappers (e.g. GUIs, etc) generate command lines that switch on\n> more than one of these flags simultaneously. It is unreasonable\n> to prohibit specifying more than one, since a new user might not\n> immediately discern the implication relation. Now we call such\n> a command line valid and legal.\n> \n> Signed-off-by: Keith Cascio \n> ---\n\nThis does not really look all that similar to other commit messages.\n\nFor example, \"Note: I do not fully understand why this fix works, but it \npasses all 68 t4???-* diff test scripts.\" is rather discouraging. If you \nare not convinced, how should we be?\n\nHowever, I almost can excuse that, but...\n\n> t/t4015-diff-whitespace.sh | 8 ++++----\n> xdiff/xutils.c | 22 ++++++++++++----------\n> 2 files changed, 16 insertions(+), 14 deletions(-)\n> \n> diff --git a/xdiff/xutils.c b/xdiff/xutils.c\n> index d7974d1..b9bda86 100644\n> --- a/xdiff/xutils.c\n> +++ b/xdiff/xutils.c\n> @@ -245,17 +245,19 @@ static unsigned long\n> xdl_hash_record_with_whitespace(char const **data,\n> \t\t\twhile (ptr + 1 < top && isspace(ptr[1])\n> \t\t\t\t\t&& ptr[1] != '\\n')\n> \t\t\t\tptr++;\n> -\t\t\tif (flags & XDF_IGNORE_WHITESPACE_CHANGE\n> -\t\t\t\t\t&& ptr[1] != '\\n') {\n> -\t\t\t\tha += (ha << 5);\n> -\t\t\t\tha ^= (unsigned long) ' ';\n> -\t\t\t}\n> -\t\t\tif (flags & XDF_IGNORE_WHITESPACE_AT_EOL\n> -\t\t\t\t\t&& ptr[1] != '\\n') {\n> -\t\t\t\twhile (ptr2 != ptr + 1) {\n> +\t\t\tif( ! ( flags & XDF_IGNORE_WHITESPACE\n\n... this is just plain ugly, not to mention breaking the coding style of \nthe surrounding code in a rather blatant way.\n\n> )){\n> +\t\t\t\tif( flags & XDF_IGNORE_WHITESPACE_CHANGE\n> +\t\t\t\t\t\t&& ptr[1] != '\\n') {\n> \t\t\t\t\tha += (ha << 5);\n> -\t\t\t\t\tha ^= (unsigned long) *ptr2;\n> -\t\t\t\t\tptr2++;\n> +\t\t\t\t\tha ^= (unsigned long) ' ';\n> +\t\t\t\t}\n> +\t\t\t\telse if( flags & XDF_IGNORE_WHITESPACE_AT_EOL\n> +\t\t\t\t\t\t&& ptr[1] != '\\n') {\n> +\t\t\t\t\twhile (ptr2 != ptr + 1) {\n> +\t\t\t\t\t\tha += (ha << 5);\n> +\t\t\t\t\t\tha ^= (unsigned long) *ptr2;\n> +\t\t\t\t\t\tptr2++;\n> +\t\t\t\t\t}\n\nBesides, I think what you actually wanted is\n\n\t\tif (flags & XDF_IGNORE_WHITESPACE)\n\t\t\t; /* already handled */\n\t\telse if (flags & XDF_IGNORE_WHITESPACE_CHANGE)\n\t\t\t...\n\t\telse if (flags & XDF_IGNORE_WHITESPACE_AT_EOL)\n\t\t\t...\n\nfor improved readability both of the code and the patch.\n\nCiao,\nDscho\n"},{"id":"101155","messageId":"alpine.GSO.2.00.0901191000520.25883@kiwi.cs.ucla.edu","threadId":"17232","inReplyTo":"alpine.DEB.1.00.0901190446480.3586@pacific.mpi-cbg.de","subject":"[PATCH/RFC v2 1/1] bug fix, diff whitespace ignore options","fromName":"Keith Cascio","fromEmail":"keith@cs.ucla.edu","sentAt":"2009-01-19T18:03:04Z","receivedAt":"2009-01-19T18:03:04Z","isPatch":true,"sender":{"key":"keith@cs.ucla.edu","avatar":"https://gravatar.com/avatar/c5ec3a8f1cd1f449fdf8bdb7125fdbfd10b729507f32cbf0aa4ad07b4f7127ae?d=mp&s=160"},"body":" Fixed bug in diff whitespace ignore options. It is now\n OK to specify more than one whitespace ignore option\n on the command line.\n\nSigned-off-by: Keith Cascio \n---\nDscho,\nYou are right. The code and the patch are more readable this way.\n -- Keith\n\n t/t4015-diff-whitespace.sh | 8 ++++----\n xdiff/xutils.c | 6 ++++--\n 2 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex dbb608c..6d13da3 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -99,11 +99,11 @@ EOF\n git diff -w > out\n test_expect_success 'another test, with -w' 'test_cmp expect out'\n git diff -w -b > out\n-test_expect_failure 'another test, with -w -b' 'test_cmp expect out'\n+test_expect_success 'another test, with -w -b' 'test_cmp expect out'\n git diff -w --ignore-space-at-eol > out\n-test_expect_failure 'another test, with -w --ignore-space-at-eol' 'test_cmp expect out'\n+test_expect_success 'another test, with -w --ignore-space-at-eol' 'test_cmp expect out'\n git diff -w -b --ignore-space-at-eol > out\n-test_expect_failure 'another test, with -w -b --ignore-space-at-eol' 'test_cmp expect out'\n+test_expect_success 'another test, with -w -b --ignore-space-at-eol' 'test_cmp expect out'\n\n tr 'Q' '\\015' << EOF > expect\n diff --git a/x b/x\n@@ -123,7 +123,7 @@ EOF\n git diff -b > out\n test_expect_success 'another test, with -b' 'test_cmp expect out'\n git diff -b --ignore-space-at-eol > out\n-test_expect_failure 'another test, with -b --ignore-space-at-eol' 'test_cmp expect out'\n+test_expect_success 'another test, with -b --ignore-space-at-eol' 'test_cmp expect out'\n\n tr 'Q' '\\015' << EOF > expect\n diff --git a/x b/x\ndiff --git a/xdiff/xutils.c b/xdiff/xutils.c\nindex d7974d1..04ad468 100644\n--- a/xdiff/xutils.c\n+++ b/xdiff/xutils.c\n@@ -245,12 +245,14 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,\n \t\t\twhile (ptr + 1 < top && isspace(ptr[1])\n \t\t\t\t\t&& ptr[1] != '\\n')\n \t\t\t\tptr++;\n-\t\t\tif (flags & XDF_IGNORE_WHITESPACE_CHANGE\n+\t\t\tif (flags & XDF_IGNORE_WHITESPACE)\n+\t\t\t\t; /* already handled */\n+\t\t\telse if (flags & XDF_IGNORE_WHITESPACE_CHANGE\n \t\t\t\t\t&& ptr[1] != '\\n') {\n \t\t\t\tha += (ha << 5);\n \t\t\t\tha ^= (unsigned long) ' ';\n \t\t\t}\n-\t\t\tif (flags & XDF_IGNORE_WHITESPACE_AT_EOL\n+\t\t\telse if (flags & XDF_IGNORE_WHITESPACE_AT_EOL\n \t\t\t\t\t&& ptr[1] != '\\n') {\n \t\t\t\twhile (ptr2 != ptr + 1) {\n \t\t\t\t\tha += (ha << 5);\n-- \n1.6.1.213.g28da8.dirty\n"},{"id":"101156","messageId":"alpine.DEB.1.00.0901191936170.3586@pacific.mpi-cbg.de","threadId":"17232","inReplyTo":"alpine.GSO.2.00.0901191000520.25883@kiwi.cs.ucla.edu","subject":"Re: [PATCH/RFC v2 1/1] bug fix, diff whitespace ignore options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-01-19T18:36:30Z","receivedAt":"2009-01-19T18:36:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 19 Jan 2009, Keith Cascio wrote:\n\n> Fixed bug in diff whitespace ignore options. It is now\n> OK to specify more than one whitespace ignore option\n> on the command line.\n> \n> Signed-off-by: Keith Cascio \n\nACK,\nDscho\n"},{"id":"101225","messageId":"7vmydmlapn.fsf@gitster.siamese.dyndns.org","threadId":"17232","inReplyTo":"alpine.GSO.2.00.0901191000520.25883@kiwi.cs.ucla.edu","subject":"Re: [PATCH/RFC v2 1/1] bug fix, diff whitespace ignore options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-01-20T07:04:36Z","receivedAt":"2009-01-20T07:04:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Keith Cascio writes:\n\n> Fixed bug in diff whitespace ignore options. It is now\n> OK to specify more than one whitespace ignore option\n> on the command line.\n>\n> Signed-off-by: Keith Cascio \n> ---\n> Dscho,\n> You are right. The code and the patch are more readable this way.\n\nThanks; I've fixed it up so there is no need to resend but your patch was\nwhitespace mangled (format=flowed), by the way.\n"}]}