{"thread":{"id":"45448","subject":"[PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2","startedAt":"2017-03-21T00:18:29Z","lastAt":"2017-03-22T21:50:14Z","messageCount":15,"participants":["Stefan Beller","Junio C Hamano","Jakub Narębski"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"314747","messageId":"20170321001156.21915-2-sbeller@google.com","threadId":"45448","inReplyTo":"20170321001156.21915-1-sbeller@google.com","subject":"[PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-21T00:11:54Z","receivedAt":"2017-03-21T00:18:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Migrate 'is_submodule_modified' to the new porcelain format of\ngit-status.\n\nAs the old porcelain only reported ' M' for submodules, no\nmatter what happened inside the submodule (untracked files,\nchanges to tracked files or move of HEAD), the new API\nproperly reports the different scenarios.\n\nIn a followup patch we will make use of these finer grained\nreporting for git-status.\n\nWhile porting this to the new API, add another extension\npoint that will get used in the future: When a submodule\nis broken (e.g. the .git file pointing to a wrong directory,\nnot containing a git dir, as fallout of e.g. f8eaa0ba98\n(submodule--helper, module_clone: always operate on absolute\npaths, 2016-03-31)), we can chose to not die and report it\ndifferently.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n submodule.c | 85 +++++++++++++++++++++++++++++++++++--------------------------\n 1 file changed, 49 insertions(+), 36 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 3200b7bb2b..81d44cb7e9 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1041,67 +1041,80 @@ int fetch_populated_submodules(const struct argv_array *options,\n \n unsigned is_submodule_modified(const char *path, int ignore_untracked)\n {\n-\tssize_t len;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tconst char *argv[] = {\n-\t\t\"status\",\n-\t\t\"--porcelain\",\n-\t\tNULL,\n-\t\tNULL,\n-\t};\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned dirty_submodule = 0;\n-\tconst char *line, *next_line;\n \tconst char *git_dir;\n+\tint error_code = 0;\n \n \tstrbuf_addf(&buf, \"%s/.git\", path);\n-\tgit_dir = read_gitfile(buf.buf);\n-\tif (!git_dir)\n-\t\tgit_dir = buf.buf;\n-\tif (!is_directory(git_dir)) {\n-\t\tstrbuf_release(&buf);\n-\t\t/* The submodule is not checked out, so it is not modified */\n-\t\treturn 0;\n+\tgit_dir = resolve_gitdir_gently(buf.buf, &error_code);\n \n+\tif (!git_dir) {\n+\t\tswitch (error_code) {\n+\t\tcase READ_GITFILE_ERR_STAT_FAILED:\n+\t\tcase READ_GITFILE_ERR_NOT_A_FILE:\n+\t\t\t/* We may have an uninitialized repo here */\n+\t\t\treturn 0;\n+\t\tdefault:\n+\t\tcase READ_GITFILE_ERR_OPEN_FAILED:\n+\t\tcase READ_GITFILE_ERR_READ_FAILED:\n+\t\tcase READ_GITFILE_ERR_INVALID_FORMAT:\n+\t\tcase READ_GITFILE_ERR_NO_PATH:\n+\t\tcase READ_GITFILE_ERR_NOT_A_REPO:\n+\t\tcase READ_GITFILE_ERR_TOO_LARGE:\n+\t\t\t/*\n+\t\t\t * All these other error codes are indicating\n+\t\t\t * a broken submodule. We do not know what is\n+\t\t\t * right here. Resolve again triggering die()\n+\t\t\t * inside of the parsing.\n+\t\t\t */\n+\t\t\tread_gitfile_gently(buf.buf, NULL);\n+\t\t\tdie(\"BUG: read_gitfile_gently should have died.\");\n+\t\t}\n \t}\n+\n \tstrbuf_reset(&buf);\n \n+\targv_array_pushl(&cp.args, \"status\", \"--porcelain=2\", NULL);\n \tif (ignore_untracked)\n-\t\targv[2] = \"-uno\";\n+\t\targv_array_push(&cp.args, \"-uno\");\n \n-\tcp.argv = argv;\n \tprepare_submodule_repo_env(&cp.env_array);\n \tcp.git_cmd = 1;\n \tcp.no_stdin = 1;\n \tcp.out = -1;\n \tcp.dir = path;\n \tif (start_command(&cp))\n-\t\tdie(\"Could not run 'git status --porcelain' in submodule %s\", path);\n+\t\tdie(\"Could not run 'git status --porcelain=2' in submodule %s\", path);\n \n-\tlen = strbuf_read(&buf, cp.out, 1024);\n-\tline = buf.buf;\n-\twhile (len > 2) {\n-\t\tif ((line[0] == '?') && (line[1] == '?')) {\n+\twhile (strbuf_getwholeline_fd(&buf, cp.out, '\\n') != EOF) {\n+\t\t/* regular untracked files */\n+\t\tif (buf.buf[0] == '?')\n \t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n-\t\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)\n-\t\t\t\tbreak;\n-\t\t} else {\n-\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n-\t\t\tif (ignore_untracked ||\n-\t\t\t    (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))\n-\t\t\t\tbreak;\n+\n+\t\t/* regular unmerged and renamed files */\n+\t\tif (buf.buf[0] == 'u' ||\n+\t\t    buf.buf[0] == '1' ||\n+\t\t    buf.buf[0] == '2') {\n+\t\t\tif (buf.buf[5] == 'S') {\n+\t\t\t\t/* nested submodule handling */\n+\t\t\t\tif (buf.buf[6] == 'C' || buf.buf[7] == 'M')\n+\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n+\t\t\t\tif (buf.buf[8] == 'U')\n+\t\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;\n+\t\t\t} else\n+\t\t\t\tdirty_submodule |= DIRTY_SUBMODULE_MODIFIED;\n \t\t}\n-\t\tnext_line = strchr(line, '\\n');\n-\t\tif (!next_line)\n-\t\t\tbreak;\n-\t\tnext_line++;\n-\t\tlen -= (next_line - line);\n-\t\tline = next_line;\n+\n+\t\tif (dirty_submodule & DIRTY_SUBMODULE_MODIFIED &&\n+\t\t    dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n+\t\t\t\tbreak;\n \t}\n \tclose(cp.out);\n \n \tif (finish_command(&cp))\n-\t\tdie(\"'git status --porcelain' failed in submodule %s\", path);\n+\t\tdie(\"'git status --porcelain=2' failed in submodule %s\", path);\n \n \tstrbuf_release(&buf);\n \treturn dirty_submodule;\n-- \n2.12.0.402.g4b3201c2d6.dirty\n\n"},{"id":"314748","messageId":"20170321001156.21915-4-sbeller@google.com","threadId":"45448","inReplyTo":"20170321001156.21915-1-sbeller@google.com","subject":"[PATCH 3/3] builtin/describe: introduce --submodule-error-as-dirty flag","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-21T00:11:56Z","receivedAt":"2017-03-21T00:19:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"git-describe tells you the version number you're at, or errors out, e.g.\nwhen you run it outside of a repository, which may happen when downloading\na tar ball instead of using git to obtain the source code.\n\nTo keep this property of only erroring out, when not in a repository,\nsevere submodule errors must be downgraded to reporting them gently\ninstead of having git-describe error out completely.\n\nThis patch helps to fix the root cause in [1], which tries to work around\nthis situation.\n\n[1] (\"Work around git describe bug for build.\")\nhttps://gerrit-review.googlesource.com/#/c/99851/\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n builtin/describe.c  | 17 ++++++++++-------\n t/t6120-describe.sh | 17 +++++++++++++++++\n 2 files changed, 27 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 76c18059bf..569fef9ecf 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -31,13 +31,9 @@ static int have_util;\n static struct string_list patterns = STRING_LIST_INIT_NODUP;\n static struct string_list exclude_patterns = STRING_LIST_INIT_NODUP;\n static int always;\n+static int gentle_submodule_errors;\n static const char *dirty;\n \n-/* diff-index command arguments to check if working tree is dirty. */\n-static const char *diff_index_args[] = {\n-\t\"diff-index\", \"--quiet\", \"HEAD\", \"--\", NULL\n-};\n-\n struct commit_name {\n \tstruct hashmap_entry entry;\n \tstruct object_id peeled;\n@@ -442,6 +438,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t\t   N_(\"do not consider tags matching <pattern>\")),\n \t\tOPT_BOOL(0, \"always\",        &always,\n \t\t\tN_(\"show abbreviated commit object as fallback\")),\n+\t\tOPT_BOOL(0, \"submodule-error-as-dirty\", &gentle_submodule_errors,\n+\t\t\tN_(\"show abbreviated commit object as fallback\")),\n \t\t{OPTION_STRING, 0, \"dirty\",  &dirty, N_(\"mark\"),\n \t\t\tN_(\"append <mark> on dirty working tree (default: \\\"-dirty\\\")\"),\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t) \"-dirty\"},\n@@ -496,6 +494,12 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\tif (dirty) {\n \t\t\tstatic struct lock_file index_lock;\n \t\t\tint fd;\n+\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n+\n+\t\t\targv_array_push(&args, \"diff-index\");\n+\t\t\tif (gentle_submodule_errors)\n+\t\t\t\targv_array_push(&args, \"--gentle-submodule-errors\");\n+\t\t\targv_array_pushl(&args, \"--quiet\", \"HEAD\", \"--\", NULL);\n \n \t\t\tread_cache_preload(NULL);\n \t\t\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,\n@@ -504,8 +508,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t\tif (0 <= fd)\n \t\t\t\tupdate_index_if_able(&the_index, &index_lock);\n \n-\t\t\tif (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1,\n-\t\t\t\t\t    diff_index_args, prefix))\n+\t\t\tif (!cmd_diff_index(args.argc, args.argv, prefix))\n \t\t\t\tdirty = NULL;\n \t\t}\n \t\tdescribe(\"HEAD\", 1);\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 167491fd5b..99e5ba44b7 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -233,4 +233,21 @@ test_expect_success 'describe --contains and --no-match' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup and absorb a submodule' '\n+\ttest_create_repo sub1 &&\n+\ttest_commit -C sub1 initial &&\n+\tgit submodule add ./sub1 &&\n+\tgit submodule absorbgitdirs &&\n+\tgit commit -a -m \"add submodule\"\n+'\n+\n+test_expect_success 'describe chokes on severly broken submodules' '\n+\tmv .git/modules/sub1/ .git/modules/sub_moved &&\n+\ttest_must_fail git describe --dirty\n+'\n+test_expect_success 'describe ignoring a borken submodule' '\n+\tgit describe --dirty --submodule-error-as-dirty >out &&\n+\tgrep dirty out\n+'\n+\n test_done\n-- \n2.12.0.402.g4b3201c2d6.dirty\n\n"},{"id":"314749","messageId":"20170321001156.21915-3-sbeller@google.com","threadId":"45448","inReplyTo":"20170321001156.21915-1-sbeller@google.com","subject":"[PATCH 2/3] revision machinery: gentle submodule errors","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-21T00:11:55Z","receivedAt":"2017-03-21T00:19:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When a submodule is broken (e.g. its git directory is deleted or is not\npointed to via the .git linking file), commands that deal with submodules\nerror out reporting the submodule error.\n\nSometimes users rather want to see the rest of the command succeed,\nignoring the submodule that is severely broken.\n\nIntroduce a flag for the revision machinery '--gentle-submodule-errors'\nthat will ask 'is_submodule_modified' to return the broken flag instead\nof dying().\n\nBy switching the argument of is_submodule_modified to an unsigned bit\nvector, we can signal more flags, such as the gentle bit introduced\nin this patch. When this flag is given, avoid dying inside\n'is_submodule_modified' for severe errors and return\nDIRTY_SUBMODULE_BROKEN instead.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n diff-lib.c                                   | 10 ++++++++--\n diff.h                                       |  1 +\n diffcore.h                                   |  3 ++-\n revision.c                                   |  2 ++\n submodule.c                                  |  9 ++++++++-\n submodule.h                                  | 10 +++++++++-\n t/t4060-diff-submodule-option-diff-format.sh | 22 ++++++++++++++++++++++\n 7 files changed, 52 insertions(+), 5 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 52447466b5..61d33b6d26 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -77,8 +77,14 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n \t\tif (DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES))\n \t\t\tchanged = 0;\n \t\telse if (!DIFF_OPT_TST(diffopt, IGNORE_DIRTY_SUBMODULES)\n-\t\t    && (!changed || DIFF_OPT_TST(diffopt, DIRTY_SUBMODULES)))\n-\t\t\t*dirty_submodule = is_submodule_modified(ce->name, DIFF_OPT_TST(diffopt, IGNORE_UNTRACKED_IN_SUBMODULES));\n+\t\t    && (!changed || DIFF_OPT_TST(diffopt, DIRTY_SUBMODULES))) {\n+\t\t\tunsigned flags = 0;\n+\t\t\tif (DIFF_OPT_TST(diffopt, IGNORE_UNTRACKED_IN_SUBMODULES))\n+\t\t\t\tflags |= IS_SUBMODULE_MODIFIED_IGNORE_UNTRACKED;\n+\t\t\tif (diffopt->gentle_submodule_errors)\n+\t\t\t\tflags |= IS_SUBMODULE_MODIFIED_GENTLY;\n+\t\t\t*dirty_submodule = is_submodule_modified(ce->name, flags);\n+\t\t}\n \t\tdiffopt->flags = orig_flags;\n \t}\n \treturn changed;\ndiff --git a/diff.h b/diff.h\nindex e9ccb38c26..d1e48de43c 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -164,6 +164,7 @@ struct diff_options {\n \tconst char *word_regex;\n \tenum diff_words_type word_diff;\n \tenum diff_submodule_format submodule_format;\n+\tint gentle_submodule_errors;\n \n \t/* this is set by diffcore for DIFF_FORMAT_PATCH */\n \tint found_changes;\ndiff --git a/diffcore.h b/diffcore.h\nindex 6230241354..ceef783570 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -40,9 +40,10 @@ struct diff_filespec {\n #define DIFF_FILE_VALID(spec) (((spec)->mode) != 0)\n \tunsigned should_free : 1; /* data should be free()'ed */\n \tunsigned should_munmap : 1; /* data should be munmap()'ed */\n-\tunsigned dirty_submodule : 2;  /* For submodules: its work tree is dirty */\n+\tunsigned dirty_submodule : 3;  /* For submodules: its work tree is dirty */\n #define DIRTY_SUBMODULE_UNTRACKED 1\n #define DIRTY_SUBMODULE_MODIFIED  2\n+#define DIRTY_SUBMODULE_BROKEN 4\n \tunsigned is_stdin : 1;\n \tunsigned has_more_entries : 1; /* only appear in combined diff */\n \t/* data should be considered \"binary\"; -1 means \"don't know yet\" */\ndiff --git a/revision.c b/revision.c\nindex 7ff61ff5f7..8ae1179b8d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2014,6 +2014,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->limited = 1;\n \t} else if (!strcmp(arg, \"--ignore-missing\")) {\n \t\trevs->ignore_missing = 1;\n+\t} else if (!strcmp(arg, \"--gentle-submodule-errors\")) {\n+\t\trevs->diffopt.gentle_submodule_errors = 1;\n \t} else {\n \t\tint opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);\n \t\tif (!opts)\ndiff --git a/submodule.c b/submodule.c\nindex 81d44cb7e9..d477297fab 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1039,13 +1039,18 @@ int fetch_populated_submodules(const struct argv_array *options,\n \treturn spf.result;\n }\n \n-unsigned is_submodule_modified(const char *path, int ignore_untracked)\n+/*\n+ * Inspects a submodule and returns its state using flags\n+ */\n+unsigned is_submodule_modified(const char *path, unsigned flags)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned dirty_submodule = 0;\n \tconst char *git_dir;\n \tint error_code = 0;\n+\tint ignore_untracked = flags & IS_SUBMODULE_MODIFIED_IGNORE_UNTRACKED;\n+\tint gently = flags & IS_SUBMODULE_MODIFIED_GENTLY;\n \n \tstrbuf_addf(&buf, \"%s/.git\", path);\n \tgit_dir = resolve_gitdir_gently(buf.buf, &error_code);\n@@ -1069,6 +1074,8 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)\n \t\t\t * right here. Resolve again triggering die()\n \t\t\t * inside of the parsing.\n \t\t\t */\n+\t\t\tif (gently)\n+\t\t\t\treturn DIRTY_SUBMODULE_BROKEN;\n \t\t\tread_gitfile_gently(buf.buf, NULL);\n \t\t\tdie(\"BUG: read_gitfile_gently should have died.\");\n \t\t}\ndiff --git a/submodule.h b/submodule.h\nindex c8a0c9cb29..b7afac5e87 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -62,7 +62,15 @@ extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);\n extern int fetch_populated_submodules(const struct argv_array *options,\n \t\t\t       const char *prefix, int command_line_option,\n \t\t\t       int quiet, int max_parallel_jobs);\n-extern unsigned is_submodule_modified(const char *path, int ignore_untracked);\n+\n+/* Ignore untracked files */\n+#define IS_SUBMODULE_MODIFIED_IGNORE_UNTRACKED 1\n+/*\n+ * Report severe errors in the submodule (e.g. missing git dir) via\n+ * DIRTY_SUBMODULE_BROKEN instead of dying.\n+ */\n+#define IS_SUBMODULE_MODIFIED_GENTLY 2\n+extern unsigned is_submodule_modified(const char *path, unsigned flags);\n extern int submodule_uses_gitfile(const char *path);\n \n #define SUBMODULE_REMOVAL_DIE_ON_ERROR (1<<0)\ndiff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh\nindex 7e23b55ea4..e5e11a8378 100755\n--- a/t/t4060-diff-submodule-option-diff-format.sh\n+++ b/t/t4060-diff-submodule-option-diff-format.sh\n@@ -746,4 +746,26 @@ test_expect_success 'diff --submodule=diff with .git file' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'setup -- break submodule' '\n+\t# the gitlink in sm2 points at a missing git dir now\n+\tmv .real .moved_real\n+'\n+\n+test_expect_success 'diff with severely broken submodule' '\n+\ttest_must_fail git diff &&\n+\n+\tsm_hash=$(git rev-parse HEAD:sm2) &&\n+\tcat >expect <<-EOF &&\n+\tdiff --git a/sm2 b/sm2\n+\t--- a/sm2\n+\t+++ b/sm2\n+\t@@ -1 +1 @@\n+\t-Subproject commit $sm_hash\n+\t+Subproject commit $sm_hash-dirty\n+\tEOF\n+\n+\tgit diff --gentle-submodule-errors -- sm2 >out &&\n+\ttest_cmp expect out\n+'\n+\n test_done\n-- \n2.12.0.402.g4b3201c2d6.dirty\n\n"},{"id":"314750","messageId":"20170321001156.21915-1-sbeller@google.com","threadId":"45448","inReplyTo":null,"subject":"[PATCH 0/3] git-describe deals gracefully with broken submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-21T00:11:53Z","receivedAt":"2017-03-21T00:21:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Our own version generation in GIT-VERSION-GEN is somewhat sane by testing\nif we have a .git dir, and use that as a signal whether the obtained\ncopy of git was obtained using git (clone/fetch) or if it is just a\ndownloaded tar ball.\n\nOther scripts to generate a version are not as cautious and just run\n\"git describe\". An error from git-describe is treated as a sufficient\nsignal to assume it is not a git repository.\n\nWhen submodules come into play, this is not true, as a submodule\nmay be damaged instead, such that we're still in a git repository\nbut error out for the sake of reporting a severly broken submodule.\n\nAdd a flag to git-describe that instructs it to treat severe submodule\nerrors as \"dirty\" instead of erroring out.\n\nThanks,\nStefan\n\nStefan Beller (3):\n  submodule.c: port is_submodule_modified to use porcelain 2\n  revision machinery: gentle submodule errors\n  builtin/describe: introduce --submodule-error-as-dirty flag\n\n builtin/describe.c                           | 17 ++---\n diff-lib.c                                   | 10 ++-\n diff.h                                       |  1 +\n diffcore.h                                   |  3 +-\n revision.c                                   |  2 +\n submodule.c                                  | 94 +++++++++++++++++-----------\n submodule.h                                  | 10 ++-\n t/t4060-diff-submodule-option-diff-format.sh | 22 +++++++\n t/t6120-describe.sh                          | 17 +++++\n 9 files changed, 128 insertions(+), 48 deletions(-)\n\n-- \n2.12.0.402.g4b3201c2d6.dirty\n\n"},{"id":"314779","messageId":"xmqq7f3jta7o.fsf@gitster.mtv.corp.google.com","threadId":"45448","inReplyTo":"20170321001156.21915-1-sbeller@google.com","subject":"Re: [PATCH 0/3] git-describe deals gracefully with broken submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-21T06:28:27Z","receivedAt":"2017-03-21T06:28:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Our own version generation in GIT-VERSION-GEN is somewhat sane by testing\n> if we have a .git dir, and use that as a signal whether the obtained\n> copy of git was obtained using git (clone/fetch) or if it is just a\n> downloaded tar ball.\n>\n> Other scripts to generate a version are not as cautious and just run\n> \"git describe\". An error from git-describe is treated as a sufficient\n> signal to assume it is not a git repository.\n>\n> When submodules come into play, this is not true, as a submodule\n> may be damaged instead, such that we're still in a git repository\n> but error out for the sake of reporting a severly broken submodule.\n>\n> Add a flag to git-describe that instructs it to treat severe submodule\n> errors as \"dirty\" instead of erroring out.\n\nI do not have a strong preference for or against the \"treat a broken\nrepository as if nothing is wrong with the revision, but just mark\nit as dirty\" idea.  I would be more receptive if it substituted the\n\"-dirty\" marker with something else, e.g. \"-broken\", though.\n\nMy knee-jerk reaction to the code change is that treating submodule\nas something very special is probably not a good idea.  Even if you\ndo not use submodules, if some of the objects referenced from your\nindex and/or HEAD are damaged or otherwise causes some error while\naccessing, the diff machinery would die, wouldn't it?\n\nI saw that some new symbolic constants in the code to tell the\nmachinery to \"gracefully die\" (or \"hide the breakage under the rug\")\nare named with SUBMODULE in them, which is probably a bad sign that\nthe design is being too centric to submodules.  The implementation\nthat covers only breakages in submodule as its first step may be OK\n(you have to start somewhere, after all), but I think the aspiration\nshould be to cover all kinds of breakages in the end and turn them\nto be \"graceful\", and if you had that goal in mind, you wouldn't be\nnaming these constants with SUBMODULE in them.\n\nIf \"treat a broken repository as just being 'dirty'\" were a good\nidea, I'd suspect that we would want to see all breakages, not just\nones related to submodules, to be treated the same way.  \n\nBut it is possible there may be a reason why submodules are special.\nI do not think the third paragraph quoted above is a good\njustification.  A repository with broken submodule is just as broken\nand untrustworthy as a broken repository without a submodule, and if\nyou want to allow such a checkout with broken submodule to call\nitself v2.0-dirty, you would also want to allow a broken checkout\nwithout any submodule to do so, too.\n\n"},{"id":"314780","messageId":"xmqq37e7t902.fsf@gitster.mtv.corp.google.com","threadId":"45448","inReplyTo":"xmqq7f3jta7o.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/3] git-describe deals gracefully with broken submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-21T06:54:37Z","receivedAt":"2017-03-21T06:58:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I do not have a strong preference for or against the \"treat a broken\n> repository as if nothing is wrong with the revision, but just mark\n> it as dirty\" idea.  I would be more receptive if it substituted the\n> \"-dirty\" marker with something else, e.g. \"-broken\", though.\n> ...\n> But it is possible there may be a reason why submodules are special.\n> I do not think the third paragraph quoted above is a good\n> justification.  A repository with broken submodule is just as broken\n> and untrustworthy as a broken repository without a submodule, and if\n> you want to allow such a checkout with broken submodule to call\n> itself v2.0-dirty, you would also want to allow a broken checkout\n> without any submodule to do so, too.\n\nHaving said all that, if I thought it were a good idea to optionally\nallow people to treat \"repository is corrupt in some ways to make it\nimpossible for us if the checkout is dirty or not, even though we\nare fairly certain what commit .git/HEAD points at is\" as a state\nthat is describable, I probably would\n\n - introduce a new \"git describe --possibly-broken\" option;\n\n - instead of running \"diff-index\" internally to decide the \"-dirty\"\n   state, spawn \"diff-index\" as a separate process;\n\n - observe the exit status from \"diff-index\" and add \"-dirty\" suffix\n   when it is _known_ to be dirty, add \"-broken\" suffix when it\n   died, and leave out the suffix when we know that the checkout is\n   clean.\n\nThat way, we wouldn't have to contaminate the generic codepath with\na \"treat broken and modified states as if they are the same\" logic,\nonly to support such a niche feature that we wouldn't want to use\nanywhere else.\n"},{"id":"314823","messageId":"CAGZ79kYRgG2CBpTv9qtH0_XZcziqF5rGLyDTXV8=pLLaSGukTA@mail.gmail.com","threadId":"45448","inReplyTo":"xmqq7f3jta7o.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/3] git-describe deals gracefully with broken submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-21T17:46:42Z","receivedAt":"2017-03-21T17:47:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Mar 20, 2017 at 11:28 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Our own version generation in GIT-VERSION-GEN is somewhat sane by testing\n>> if we have a .git dir, and use that as a signal whether the obtained\n>> copy of git was obtained using git (clone/fetch) or if it is just a\n>> downloaded tar ball.\n>>\n>> Other scripts to generate a version are not as cautious and just run\n>> \"git describe\". An error from git-describe is treated as a sufficient\n>> signal to assume it is not a git repository.\n>>\n>> When submodules come into play, this is not true, as a submodule\n>> may be damaged instead, such that we're still in a git repository\n>> but error out for the sake of reporting a severly broken submodule.\n>>\n>> Add a flag to git-describe that instructs it to treat severe submodule\n>> errors as \"dirty\" instead of erroring out.\n>\n> I do not have a strong preference for or against the \"treat a broken\n> repository as if nothing is wrong with the revision, but just mark\n> it as dirty\" idea.  I would be more receptive if it substituted the\n> \"-dirty\" marker with something else, e.g. \"-broken\", though.\n\nok. That was my initial plan. I plan on also incorporating this into\ngit-status.\n\n>\n> My knee-jerk reaction to the code change is that treating submodule\n> as something very special is probably not a good idea.  Even if you\n> do not use submodules, if some of the objects referenced from your\n> index and/or HEAD are damaged or otherwise causes some error while\n> accessing, the diff machinery would die, wouldn't it?\n\nYes; ideally we would want to have a generic \"error with working tree\"\nfor all these kinds of errors. Note that the probability of submodule errors\nis way higher. (This has a couple of reasons: (1) a submodule has more\ncomplexity than a file, (2) that opens up new areas in which errors can occur\nand (3) submodules are less popular so we have fewer\nbug reports and people working on it, hence less mature code.\n\nAs an example for (2), we had relative/absolute path issues in Git 2.7\nfor submodules, which is what Martin was complaining about.\n\n> I saw that some new symbolic constants in the code to tell the\n> machinery to \"gracefully die\" (or \"hide the breakage under the rug\")\n> are named with SUBMODULE in them, which is probably a bad sign that\n> the design is being too centric to submodules.  The implementation\n> that covers only breakages in submodule as its first step may be OK\n> (you have to start somewhere, after all), but I think the aspiration\n> should be to cover all kinds of breakages in the end and turn them\n> to be \"graceful\", and if you had that goal in mind, you wouldn't be\n> naming these constants with SUBMODULE in them.\n\nok\n\n>\n> If \"treat a broken repository as just being 'dirty'\" were a good\n> idea, I'd suspect that we would want to see all breakages, not just\n> ones related to submodules, to be treated the same way.\n>\n> But it is possible there may be a reason why submodules are special.\n> I do not think the third paragraph quoted above is a good\n> justification.  A repository with broken submodule is just as broken\n> and untrustworthy as a broken repository without a submodule, and if\n> you want to allow such a checkout with broken submodule to call\n> itself v2.0-dirty, you would also want to allow a broken checkout\n> without any submodule to do so, too.\n\nagreed.\n\n> I probably would\n>\n> - introduce a new \"git describe --possibly-broken\" option;\n>\n> - instead of running \"diff-index\" internally to decide the \"-dirty\"\n>   state, spawn \"diff-index\" as a separate process;\n>\n> - observe the exit status from \"diff-index\" and add \"-dirty\" suffix\n>   when it is _known_ to be dirty, add \"-broken\" suffix when it\n>   died, and leave out the suffix when we know that the checkout is\n>   clean.\n>\n> That way, we wouldn't have to contaminate the generic codepath with\n> a \"treat broken and modified states as if they are the same\" logic,\n> only to support such a niche feature that we wouldn't want to use\n> anywhere else.\n\nI planned on a follow-up that adds similar behavior to git-status,\nsuch that git-status can be used to get the big picture before\ndiagnosing the individual breakages of a submodule.\n\nThat is the reason why I started with the first patch porting\n'is_submodule_modified' to porcelain2 of git-status, as that\nallows way better reporting on submodules. Sorry for intermixing\nthese two things.\n\nThanks,\nStefan\n"},{"id":"314841","messageId":"20170321185139.8300-1-sbeller@google.com","threadId":"45448","inReplyTo":"xmqq37e7t902.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] builtin/describe: introduce --broken flag","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-21T18:51:39Z","receivedAt":"2017-03-21T18:52:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"git-describe tells you the version number you're at, or errors out, e.g.\nwhen you run it outside of a repository, which may happen when downloading\na tar ball instead of using git to obtain the source code.\n\nTo keep this property of only erroring out, when not in a repository,\nsevere (submodule) errors must be downgraded to reporting them gently\ninstead of having git-describe error out completely.\n\nTo achieve that a flag '--broken' is introduced, which is in the same\nvein as '--dirty' but uses an actual child process to check for dirtiness.\nWhen that child dies unexpectedly, we'll append '-broken' instead of\n'-dirty'.\n\nThis patch helps to fix the root cause in [1], which tries to work around\nthis situation.\n\n[1] (\"Work around git describe bug for build.\")\nhttps://gerrit-review.googlesource.com/#/c/99851/\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n> I probably would\n> \n> - introduce a new \"git describe --possibly-broken\" option;\n> \n> - instead of running \"diff-index\" internally to decide the \"-dirty\"\n>    state, spawn \"diff-index\" as a separate process;\n> \n>  - observe the exit status from \"diff-index\" and add \"-dirty\" suffix\n\nThis is what this patch does. It doesn't need any preceeding refactor\npatches, but comes on its own; developed on origin/master.\n\nThanks,\nStefan\n\n Documentation/git-describe.txt |  7 +++++\n builtin/describe.c             | 59 ++++++++++++++++++++++++++++++------------\n t/t6120-describe.sh            | 20 ++++++++++++++\n 3 files changed, 70 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt\nindex 8755f3af7b..b71fa7a4ad 100644\n--- a/Documentation/git-describe.txt\n+++ b/Documentation/git-describe.txt\n@@ -34,6 +34,13 @@ OPTIONS\n \tIt means describe HEAD and appends <mark> (`-dirty` by\n \tdefault) if the working tree is dirty.\n \n+--broken[=<mark>]::\n+\tDescribe the working tree.\n+\tIt means describe HEAD and appends <mark> (`-broken` by\n+\tdefault) if the working tree cannot be examined for dirtiness.\n+\tThis implies `--dirty`, which is the fallback behavior when\n+\tthe working tree examination works.\n+\n --all::\n \tInstead of using only the annotated tags, use any ref\n \tfound in `refs/` namespace.  This option enables matching\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 76c18059bf..37a83520c9 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -9,6 +9,7 @@\n #include \"diff.h\"\n #include \"hashmap.h\"\n #include \"argv-array.h\"\n+#include \"run-command.h\"\n \n #define SEEN\t\t(1u << 0)\n #define MAX_TAGS\t(FLAG_BITS - 1)\n@@ -31,12 +32,7 @@ static int have_util;\n static struct string_list patterns = STRING_LIST_INIT_NODUP;\n static struct string_list exclude_patterns = STRING_LIST_INIT_NODUP;\n static int always;\n-static const char *dirty;\n-\n-/* diff-index command arguments to check if working tree is dirty. */\n-static const char *diff_index_args[] = {\n-\t\"diff-index\", \"--quiet\", \"HEAD\", \"--\", NULL\n-};\n+static const char *append, *dirty, *broken;\n \n struct commit_name {\n \tstruct hashmap_entry entry;\n@@ -292,8 +288,8 @@ static void describe(const char *arg, int last_one)\n \t\tdisplay_name(n);\n \t\tif (longformat)\n \t\t\tshow_suffix(0, n->tag ? &n->tag->tagged->oid : &oid);\n-\t\tif (dirty)\n-\t\t\tprintf(\"%s\", dirty);\n+\t\tif (append)\n+\t\t\tprintf(\"%s\", append);\n \t\tprintf(\"\\n\");\n \t\treturn;\n \t}\n@@ -369,8 +365,8 @@ static void describe(const char *arg, int last_one)\n \t\tstruct object_id *oid = &cmit->object.oid;\n \t\tif (always) {\n \t\t\tprintf(\"%s\", find_unique_abbrev(oid->hash, abbrev));\n-\t\t\tif (dirty)\n-\t\t\t\tprintf(\"%s\", dirty);\n+\t\t\tif (append)\n+\t\t\t\tprintf(\"%s\", append);\n \t\t\tprintf(\"\\n\");\n \t\t\treturn;\n \t\t}\n@@ -413,8 +409,8 @@ static void describe(const char *arg, int last_one)\n \tdisplay_name(all_matches[0].name);\n \tif (abbrev)\n \t\tshow_suffix(all_matches[0].depth, &cmit->object.oid);\n-\tif (dirty)\n-\t\tprintf(\"%s\", dirty);\n+\tif (append)\n+\t\tprintf(\"%s\", append);\n \tprintf(\"\\n\");\n \n \tif (!last_one)\n@@ -445,6 +441,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t{OPTION_STRING, 0, \"dirty\",  &dirty, N_(\"mark\"),\n \t\t\tN_(\"append <mark> on dirty working tree (default: \\\"-dirty\\\")\"),\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t) \"-dirty\"},\n+\t\t{OPTION_STRING, 0, \"broken\",  &broken, N_(\"mark\"),\n+\t\t\tN_(\"append <mark> on broken working tree (default: \\\"-broken\\\")\"),\n+\t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t) \"-broken\"},\n \t\tOPT_END(),\n \t};\n \n@@ -493,10 +492,35 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"No names found, cannot describe anything.\"));\n \n \tif (argc == 0) {\n-\t\tif (dirty) {\n+\t\tif (broken) {\n+\t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\t\targv_array_pushl(&cp.args, \"diff-index\", \"--quiet\", \"HEAD\", \"--\", NULL);\n+\t\t\tcp.git_cmd = 1;\n+\t\t\tcp.no_stdin = 1;\n+\t\t\tcp.no_stdout = 1;\n+\n+\t\t\tif (!dirty)\n+\t\t\t\tdirty = \"-dirty\";\n+\n+\t\t\tswitch (run_command(&cp)) {\n+\t\t\tcase 0:\n+\t\t\t\tappend = NULL;\n+\t\t\t\tbreak;\n+\t\t\tcase 1:\n+\t\t\t\t/* keep dirty as is */\n+\t\t\t\tappend = dirty;\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\t/* diff-index aborted abnormally */\n+\t\t\t\tappend = broken;\n+\t\t\t}\n+\t\t} else if (dirty) {\n+\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n \t\t\tstatic struct lock_file index_lock;\n \t\t\tint fd;\n \n+\t\t\targv_array_pushl(&args, \"diff-index\", \"--quiet\", \"HEAD\", \"--\", NULL);\n+\n \t\t\tread_cache_preload(NULL);\n \t\t\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,\n \t\t\t\t      NULL, NULL, NULL);\n@@ -504,13 +528,16 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t\tif (0 <= fd)\n \t\t\t\tupdate_index_if_able(&the_index, &index_lock);\n \n-\t\t\tif (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1,\n-\t\t\t\t\t    diff_index_args, prefix))\n-\t\t\t\tdirty = NULL;\n+\t\t\tif (cmd_diff_index(args.argc, args.argv, prefix))\n+\t\t\t\tappend = dirty;\n+\t\t\telse\n+\t\t\t\tappend = NULL;\n \t\t}\n \t\tdescribe(\"HEAD\", 1);\n \t} else if (dirty) {\n \t\tdie(_(\"--dirty is incompatible with commit-ishes\"));\n+\t} else if (broken) {\n+\t\tdie(_(\"--broken is incompatible with commit-ishes\"));\n \t} else {\n \t\twhile (argc-- > 0)\n \t\t\tdescribe(*argv++, argc == 0);\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 167491fd5b..16952e44fc 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -233,4 +233,24 @@ test_expect_success 'describe --contains and --no-match' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup and absorb a submodule' '\n+\ttest_create_repo sub1 &&\n+\ttest_commit -C sub1 initial &&\n+\tgit submodule add ./sub1 &&\n+\tgit submodule absorbgitdirs &&\n+\tgit commit -a -m \"add submodule\" &&\n+\tgit describe --dirty >expect &&\n+\tgit describe --broken >out &&\n+\ttest_cmp expect out\n+'\n+\n+test_expect_success 'describe chokes on severly broken submodules' '\n+\tmv .git/modules/sub1/ .git/modules/sub_moved &&\n+\ttest_must_fail git describe --dirty\n+'\n+test_expect_success 'describe ignoring a borken submodule' '\n+\tgit describe --broken >out &&\n+\tgrep broken out\n+'\n+\n test_done\n-- \n2.12.0.402.g0501f7a28e.dirty\n\n"},{"id":"314870","messageId":"xmqq7f3invr6.fsf@gitster.mtv.corp.google.com","threadId":"45448","inReplyTo":"20170321185139.8300-1-sbeller@google.com","subject":"Re: [PATCH] builtin/describe: introduce --broken flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-21T21:51:57Z","receivedAt":"2017-03-21T21:52:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> This patch helps to fix the root cause in [1], which tries to work around\n> this situation.\n\nI do not necessarily think it is reasonable to give $version-dirty\nand proceed when a repository corruption is detected; if there is a\nbreakage in the repository, \"git describe\" is correct to die when a\npopulated submodule is broken.  IOW, I do not agree that [1] below\nis working with a sensible expectation.\n\nThis is a tangent, but how does the Gerrit repository get corrupted\nin the way described in [1] in the first place?  That might be what\nneeds to be corrected, perhaps?\n\n>  Documentation/git-describe.txt |  7 +++++\n>  builtin/describe.c             | 59 ++++++++++++++++++++++++++++++------------\n>  t/t6120-describe.sh            | 20 ++++++++++++++\n>  3 files changed, 70 insertions(+), 16 deletions(-)\n>\n> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt\n> index 8755f3af7b..b71fa7a4ad 100644\n> --- a/Documentation/git-describe.txt\n> +++ b/Documentation/git-describe.txt\n> @@ -34,6 +34,13 @@ OPTIONS\n>  \tIt means describe HEAD and appends <mark> (`-dirty` by\n>  \tdefault) if the working tree is dirty.\n>  \n> +--broken[=<mark>]::\n> +\tDescribe the working tree.\n> +\tIt means describe HEAD and appends <mark> (`-broken` by\n> +\tdefault) if the working tree cannot be examined for dirtiness.\n> +\tThis implies `--dirty`, which is the fallback behavior when\n> +\tthe working tree examination works.\n\nThe wording for the \"--dirty\" is already awkward, but this one is\neven more so (\"Describe the working tree. It means\" conveys no\nuseful information).  I however cannot come up with something much\nbetter.  This is the best I could come up with:\n\n\tDescribe the state of the working tree.  When the working\n\ttree matches HEAD, the output is the same as \"git describe\n\tHEAD\" and \"-dirty\" is appended to it if the working tree has\n\tlocal modification.  When a repository is corrupt and Git\n\tcannot determine if there is local modification, instead of\n\tdying, append \"-broken\" instead.\n\nThe last sentence can be tweaked to replace the description for\n\"--dirty\".\n\n> diff --git a/builtin/describe.c b/builtin/describe.c\n> index 76c18059bf..37a83520c9 100644\n> --- a/builtin/describe.c\n> +++ b/builtin/describe.c\n> @@ -9,6 +9,7 @@\n>  #include \"diff.h\"\n>  #include \"hashmap.h\"\n>  #include \"argv-array.h\"\n> +#include \"run-command.h\"\n>  \n>  #define SEEN\t\t(1u << 0)\n>  #define MAX_TAGS\t(FLAG_BITS - 1)\n> @@ -31,12 +32,7 @@ static int have_util;\n>  static struct string_list patterns = STRING_LIST_INIT_NODUP;\n>  static struct string_list exclude_patterns = STRING_LIST_INIT_NODUP;\n>  static int always;\n> -static const char *dirty;\n> -\n> -/* diff-index command arguments to check if working tree is dirty. */\n> -static const char *diff_index_args[] = {\n> -\t\"diff-index\", \"--quiet\", \"HEAD\", \"--\", NULL\n> -};\n> +static const char *append, *dirty, *broken;\n\nPerhaps call it \"suffix\" or something?\n\n> +\t\tif (broken) {\n> +\t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\t\t\targv_array_pushl(&cp.args, \"diff-index\", \"--quiet\", \"HEAD\", \"--\", NULL);\n> ...\n> +\t\t\t}\n> +\t\t} else if (dirty) {\n> +\t\t\tstruct argv_array args = ARGV_ARRAY_INIT;\n>  \t\t\tstatic struct lock_file index_lock;\n>  \t\t\tint fd;\n>  \n> +\t\t\targv_array_pushl(&args, \"diff-index\", \"--quiet\", \"HEAD\", \"--\", NULL);\n\n\nSomehow it looks like the patch is making the code a lot worse by\nlosing diff_index_args[] and duplicating the same command line twice\nin the code.  Wouldn't argv_array_pushv() into these two different args\narray from the same template work better?\n"},{"id":"314879","messageId":"CAGZ79kbjsm1p+Ag5Q8fii3ncbxSsVLYRwGP=Va=btx8Tfy3aOA@mail.gmail.com","threadId":"45448","inReplyTo":"xmqq7f3invr6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] builtin/describe: introduce --broken flag","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-21T22:27:56Z","receivedAt":"2017-03-21T22:37:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 21, 2017 at 2:51 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> This patch helps to fix the root cause in [1], which tries to work around\n>> this situation.\n>\n> I do not necessarily think it is reasonable to give $version-dirty\n> and proceed when a repository corruption is detected; if there is a\n> breakage in the repository, \"git describe\" is correct to die when a\n> populated submodule is broken.  IOW, I do not agree that [1] below\n> is working with a sensible expectation.\n\nok, so I won't quote it in the commit message\n\n>\n> This is a tangent, but how does the Gerrit repository get corrupted\n> in the way described in [1] in the first place?  That might be what\n> needs to be corrected, perhaps?\n\nAFAICT, someone is (was?) using a version of Git that doesn't contain\nf8eaa0ba98 (submodule--helper, module_clone: always operate\non absolute paths, 2016-03-31). So then the submodule paths were\nmade absolute paths on creation of the Gerrit repo.\n\nAnd then someone moved the repo and the absolute paths broke.\n\nEven after an upgrade of Git to its latest and greatest version, the underlying\nissue of having broken submodule paths remains in that case.\n\nSo there are a couple of ways forward\n0) as an immediate fix, manually fix the absolute path or make them relative\n\n1A) have more error resilient tools in Git\n1B) have a tool in git (e.g. \"git submodule fsck-setup\") that rewrites\n    the .git file link and the core.worktree setting to be relative and correct.\n\nI think we should do both A and B, I decided to go with A first, specifically\n\"git-describe\" as that was reported to not work well in this situation with the\ngiven broken data\n\n> The wording for the \"--dirty\" is already awkward, but this one is\n> even more so (\"Describe the working tree. It means\" conveys no\n> useful information).  I however cannot come up with something much\n> better.  This is the best I could come up with:\n>\n>         Describe the state of the working tree.  When the working\n>         tree matches HEAD, the output is the same as \"git describe\n>         HEAD\" and \"-dirty\" is appended to it if the working tree has\n>         local modification.  When a repository is corrupt and Git\n>         cannot determine if there is local modification, instead of\n>         dying, append \"-broken\" instead.\n\nok, I'll reuse parts of it.\n\n>> +static const char *append, *dirty, *broken;\n>\n> Perhaps call it \"suffix\" or something?\n\ndone\n\n\n>> +                     argv_array_pushl(&args, \"diff-index\", \"--quiet\", \"HEAD\", \"--\", NULL);\n..\n>  Wouldn't argv_array_pushv() into these two different args\n> array from the same template work better?\n\nyes, looks much saner. Thanks\n"},{"id":"314881","messageId":"xmqqvar2mevi.fsf@gitster.mtv.corp.google.com","threadId":"45448","inReplyTo":"CAGZ79kbjsm1p+Ag5Q8fii3ncbxSsVLYRwGP=Va=btx8Tfy3aOA@mail.gmail.com","subject":"Re: [PATCH] builtin/describe: introduce --broken flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-21T22:41:53Z","receivedAt":"2017-03-21T22:42:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> AFAICT, someone is (was?) using a version of Git that doesn't contain\n> f8eaa0ba98 (submodule--helper, module_clone: always operate\n> on absolute paths, 2016-03-31). So then the submodule paths were\n> made absolute paths on creation of the Gerrit repo.\n>\n> And then someone moved the repo and the absolute paths broke.\n>\n> Even after an upgrade of Git to its latest and greatest version, the underlying\n> issue of having broken submodule paths remains in that case.\n>\n> So there are a couple of ways forward\n> 0) as an immediate fix, manually fix the absolute path or make them relative\n>\n> 1A) have more error resilient tools in Git\n> 1B) have a tool in git (e.g. \"git submodule fsck-setup\") that rewrites\n>     the .git file link and the core.worktree setting to be relative and correct.\n>\n> I think we should do both A and B, I decided to go with A first, specifically\n> \"git-describe\" as that was reported to not work well in this situation with the\n> given broken data\n\nSounds sensible.  I would say that we should do less 1A (i.e. hiding\nproblems under the rug) and more 1B.  Of course, making the problem\nless likely to happen in the first place would be more important ;-)\n\nThanks.\n"},{"id":"314883","messageId":"CAGZ79kYfrKRbs40UH73VCgMsJgFyh1aG5ZrYMj3O_wGdu6rSUA@mail.gmail.com","threadId":"45448","inReplyTo":"xmqqvar2mevi.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] builtin/describe: introduce --broken flag","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-21T22:50:54Z","receivedAt":"2017-03-21T22:51:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Mar 21, 2017 at 3:41 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> AFAICT, someone is (was?) using a version of Git that doesn't contain\n>> f8eaa0ba98 (submodule--helper, module_clone: always operate\n>> on absolute paths, 2016-03-31). So then the submodule paths were\n>> made absolute paths on creation of the Gerrit repo.\n>>\n>> And then someone moved the repo and the absolute paths broke.\n>>\n>> Even after an upgrade of Git to its latest and greatest version, the underlying\n>> issue of having broken submodule paths remains in that case.\n>>\n>> So there are a couple of ways forward\n>> 0) as an immediate fix, manually fix the absolute path or make them relative\n>>\n>> 1A) have more error resilient tools in Git\n>> 1B) have a tool in git (e.g. \"git submodule fsck-setup\") that rewrites\n>>     the .git file link and the core.worktree setting to be relative and correct.\n>>\n>> I think we should do both A and B, I decided to go with A first, specifically\n>> \"git-describe\" as that was reported to not work well in this situation with the\n>> given broken data\n>\n> Sounds sensible.  I would say that we should do less 1A (i.e. hiding\n> problems under the rug) and more 1B.  Of course, making the problem\n> less likely to happen in the first place would be more important ;-)\n\nAgreed on not introducing bugs as often. For (B), I wonder what the\nright place is.\n\nMaybe \"reset --hard\", in combination with \"--recurse-submodules\" would also\nfix these submodule path issues. The made up \"git submodule fsck-setup\"\nsounds like it would only detect (and not fix) the problems. But these absolute\npaths issues are known how to fix, so a \"fsck\"-y tool may convey a wrong\nimpression.\n"},{"id":"314884","messageId":"20170321225718.18633-1-sbeller@google.com","threadId":"45448","inReplyTo":"CAGZ79kYfrKRbs40UH73VCgMsJgFyh1aG5ZrYMj3O_wGdu6rSUA@mail.gmail.com","subject":"[PATCH v2] builtin/describe: introduce --broken flag","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-03-21T22:57:18Z","receivedAt":"2017-03-21T22:57:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"git-describe tells you the version number you're at, or errors out, e.g.\nwhen you run it outside of a repository, which may happen when downloading\na tar ball instead of using git to obtain the source code.\n\nTo keep this property of only erroring out, when not in a repository,\nsevere (submodule) errors must be downgraded to reporting them gently\ninstead of having git-describe error out completely.\n\nTo achieve that a flag '--broken' is introduced, which is in the same\nvein as '--dirty' but uses an actual child process to check for dirtiness.\nWhen that child dies unexpectedly, we'll append '-broken' instead of\n'-dirty'.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/git-describe.txt | 11 +++++++---\n builtin/describe.c             | 47 ++++++++++++++++++++++++++++++++++--------\n t/t6120-describe.sh            | 20 ++++++++++++++++++\n 3 files changed, 66 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt\nindex 8755f3af7b..26f19d3b07 100644\n--- a/Documentation/git-describe.txt\n+++ b/Documentation/git-describe.txt\n@@ -30,9 +30,14 @@ OPTIONS\n \tCommit-ish object names to describe.  Defaults to HEAD if omitted.\n \n --dirty[=<mark>]::\n-\tDescribe the working tree.\n-\tIt means describe HEAD and appends <mark> (`-dirty` by\n-\tdefault) if the working tree is dirty.\n+--broken[=<mark>]::\n+\tDescribe the state of the working tree.  When the working\n+\ttree matches HEAD, the output is the same as \"git describe\n+\tHEAD\".  If the working tree has local modification \"-dirty\"\n+\tis appended to it.  If a repository is corrupt and Git\n+\tcannot determine if there is local modification, Git will\n+\terror out, unless `--broken' is given, which appends\n+\tthe suffix \"-broken\" instead.\n \n --all::\n \tInstead of using only the annotated tags, use any ref\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 76c18059bf..45adbf67d5 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -9,6 +9,7 @@\n #include \"diff.h\"\n #include \"hashmap.h\"\n #include \"argv-array.h\"\n+#include \"run-command.h\"\n \n #define SEEN\t\t(1u << 0)\n #define MAX_TAGS\t(FLAG_BITS - 1)\n@@ -31,7 +32,7 @@ static int have_util;\n static struct string_list patterns = STRING_LIST_INIT_NODUP;\n static struct string_list exclude_patterns = STRING_LIST_INIT_NODUP;\n static int always;\n-static const char *dirty;\n+static const char *suffix, *dirty, *broken;\n \n /* diff-index command arguments to check if working tree is dirty. */\n static const char *diff_index_args[] = {\n@@ -292,8 +293,8 @@ static void describe(const char *arg, int last_one)\n \t\tdisplay_name(n);\n \t\tif (longformat)\n \t\t\tshow_suffix(0, n->tag ? &n->tag->tagged->oid : &oid);\n-\t\tif (dirty)\n-\t\t\tprintf(\"%s\", dirty);\n+\t\tif (suffix)\n+\t\t\tprintf(\"%s\", suffix);\n \t\tprintf(\"\\n\");\n \t\treturn;\n \t}\n@@ -369,8 +370,8 @@ static void describe(const char *arg, int last_one)\n \t\tstruct object_id *oid = &cmit->object.oid;\n \t\tif (always) {\n \t\t\tprintf(\"%s\", find_unique_abbrev(oid->hash, abbrev));\n-\t\t\tif (dirty)\n-\t\t\t\tprintf(\"%s\", dirty);\n+\t\t\tif (suffix)\n+\t\t\t\tprintf(\"%s\", suffix);\n \t\t\tprintf(\"\\n\");\n \t\t\treturn;\n \t\t}\n@@ -413,8 +414,8 @@ static void describe(const char *arg, int last_one)\n \tdisplay_name(all_matches[0].name);\n \tif (abbrev)\n \t\tshow_suffix(all_matches[0].depth, &cmit->object.oid);\n-\tif (dirty)\n-\t\tprintf(\"%s\", dirty);\n+\tif (suffix)\n+\t\tprintf(\"%s\", suffix);\n \tprintf(\"\\n\");\n \n \tif (!last_one)\n@@ -445,6 +446,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\t{OPTION_STRING, 0, \"dirty\",  &dirty, N_(\"mark\"),\n \t\t\tN_(\"append <mark> on dirty working tree (default: \\\"-dirty\\\")\"),\n \t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t) \"-dirty\"},\n+\t\t{OPTION_STRING, 0, \"broken\",  &broken, N_(\"mark\"),\n+\t\t\tN_(\"append <mark> on broken working tree (default: \\\"-broken\\\")\"),\n+\t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t) \"-broken\"},\n \t\tOPT_END(),\n \t};\n \n@@ -493,7 +497,28 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"No names found, cannot describe anything.\"));\n \n \tif (argc == 0) {\n-\t\tif (dirty) {\n+\t\tif (broken) {\n+\t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\t\targv_array_pushv(&cp.args, diff_index_args);\n+\t\t\tcp.git_cmd = 1;\n+\t\t\tcp.no_stdin = 1;\n+\t\t\tcp.no_stdout = 1;\n+\n+\t\t\tif (!dirty)\n+\t\t\t\tdirty = \"-dirty\";\n+\n+\t\t\tswitch (run_command(&cp)) {\n+\t\t\tcase 0:\n+\t\t\t\tsuffix = NULL;\n+\t\t\t\tbreak;\n+\t\t\tcase 1:\n+\t\t\t\tsuffix = dirty;\n+\t\t\t\tbreak;\n+\t\t\tdefault:\n+\t\t\t\t/* diff-index aborted abnormally */\n+\t\t\t\tsuffix = broken;\n+\t\t\t}\n+\t\t} else if (dirty) {\n \t\t\tstatic struct lock_file index_lock;\n \t\t\tint fd;\n \n@@ -506,11 +531,15 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \n \t\t\tif (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1,\n \t\t\t\t\t    diff_index_args, prefix))\n-\t\t\t\tdirty = NULL;\n+\t\t\t\tsuffix = NULL;\n+\t\t\telse\n+\t\t\t\tsuffix = dirty;\n \t\t}\n \t\tdescribe(\"HEAD\", 1);\n \t} else if (dirty) {\n \t\tdie(_(\"--dirty is incompatible with commit-ishes\"));\n+\t} else if (broken) {\n+\t\tdie(_(\"--broken is incompatible with commit-ishes\"));\n \t} else {\n \t\twhile (argc-- > 0)\n \t\t\tdescribe(*argv++, argc == 0);\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 167491fd5b..16952e44fc 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -233,4 +233,24 @@ test_expect_success 'describe --contains and --no-match' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup and absorb a submodule' '\n+\ttest_create_repo sub1 &&\n+\ttest_commit -C sub1 initial &&\n+\tgit submodule add ./sub1 &&\n+\tgit submodule absorbgitdirs &&\n+\tgit commit -a -m \"add submodule\" &&\n+\tgit describe --dirty >expect &&\n+\tgit describe --broken >out &&\n+\ttest_cmp expect out\n+'\n+\n+test_expect_success 'describe chokes on severly broken submodules' '\n+\tmv .git/modules/sub1/ .git/modules/sub_moved &&\n+\ttest_must_fail git describe --dirty\n+'\n+test_expect_success 'describe ignoring a borken submodule' '\n+\tgit describe --broken >out &&\n+\tgrep broken out\n+'\n+\n test_done\n-- \n2.12.1.385.gd4d53dfa89.dirty\n\n"},{"id":"314928","messageId":"xmqqinn1mdlq.fsf@gitster.mtv.corp.google.com","threadId":"45448","inReplyTo":"20170321225718.18633-1-sbeller@google.com","subject":"Re: [PATCH v2] builtin/describe: introduce --broken flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-22T17:21:37Z","receivedAt":"2017-03-22T17:22:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> git-describe tells you the version number you're at, or errors out, e.g.\n> when you run it outside of a repository, which may happen when downloading\n> a tar ball instead of using git to obtain the source code.\n>\n> To keep this property of only erroring out, when not in a repository,\n> severe (submodule) errors must be downgraded to reporting them gently\n> instead of having git-describe error out completely.\n>\n> To achieve that a flag '--broken' is introduced, which is in the same\n> vein as '--dirty' but uses an actual child process to check for dirtiness.\n> When that child dies unexpectedly, we'll append '-broken' instead of\n> '-dirty'.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  Documentation/git-describe.txt | 11 +++++++---\n>  builtin/describe.c             | 47 ++++++++++++++++++++++++++++++++++--------\n>  t/t6120-describe.sh            | 20 ++++++++++++++++++\n>  3 files changed, 66 insertions(+), 12 deletions(-)\n\nI admit that my suggestion to use pushv() was done without trying it\nout myself, but I do agree that the result looks better, especially\nbecause the \"dirty\" (not \"broken\") side does not even have to use a\nseparate argv_array to prepare the command line in the first place\n(which I failed to spot exactly because I didn't try it myself ;-).\n\nWe probably _could_ use cp.argv to point at the diff_index_args()\nwithout doing pushv(&cp.args), and that would save a bit of\nallocation, but it would not matter in practice as this is not a\nperformance critical codepath.\n\nThanks.\n> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> index 167491fd5b..16952e44fc 100755\n> --- a/t/t6120-describe.sh\n> +++ b/t/t6120-describe.sh\n> @@ -233,4 +233,24 @@ test_expect_success 'describe --contains and --no-match' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'setup and absorb a submodule' '\n> +\ttest_create_repo sub1 &&\n> +\ttest_commit -C sub1 initial &&\n> +\tgit submodule add ./sub1 &&\n> +\tgit submodule absorbgitdirs &&\n> +\tgit commit -a -m \"add submodule\" &&\n> +\tgit describe --dirty >expect &&\n> +\tgit describe --broken >out &&\n> +\ttest_cmp expect out\n> +'\n> +\n> +test_expect_success 'describe chokes on severly broken submodules' '\n> +\tmv .git/modules/sub1/ .git/modules/sub_moved &&\n> +\ttest_must_fail git describe --dirty\n> +'\n> +test_expect_success 'describe ignoring a borken submodule' '\n> +\tgit describe --broken >out &&\n> +\tgrep broken out\n> +'\n> +\n>  test_done\n"},{"id":"314986","messageId":"b418a7ec-a8e5-1702-1a1f-3e9c8b8a2f7e@gmail.com","threadId":"45448","inReplyTo":"20170321225718.18633-1-sbeller@google.com","subject":"Re: [PATCH v2] builtin/describe: introduce --broken flag","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2017-03-22T21:50:01Z","receivedAt":"2017-03-22T21:50:14Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 21.03.2017 o 23:57, Stefan Beller pisze:\n\n> --- a/Documentation/git-describe.txt\n> +++ b/Documentation/git-describe.txt\n> @@ -30,9 +30,14 @@ OPTIONS\n>  \tCommit-ish object names to describe.  Defaults to HEAD if omitted.\n>  \n>  --dirty[=<mark>]::\n> -\tDescribe the working tree.\n> -\tIt means describe HEAD and appends <mark> (`-dirty` by\n> -\tdefault) if the working tree is dirty.\n> +--broken[=<mark>]::\n> +\tDescribe the state of the working tree.  When the working\n> +\ttree matches HEAD, the output is the same as \"git describe\n> +\tHEAD\".  If the working tree has local modification \"-dirty\"\n> +\tis appended to it.  If a repository is corrupt and Git\n> +\tcannot determine if there is local modification, Git will\n> +\terror out, unless `--broken' is given, which appends\n> +\tthe suffix \"-broken\" instead.\n\nThe common description reads better... but unfortunately it lost\ninformation about the optional parameter, namely <mark>.  The\n'-dirty' is just the default for <dirty-mark>, and '-broken' is\nthe default for <broken-mark>.\n\nMaybe /the suffix \"-broken\"/<broken-mark> suffix ('-broken' by default)/\nand similarly for \"-dirty\"?\n\nBest,\n-- \nJakub Narębski\n\n"}]}