git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2
       [not found] <20170321001156.21915-1-sbeller@google.com>
@ 2017-03-21  0:11 ` Stefan Beller
  2017-03-21  0:11 ` [PATCH 2/3] revision machinery: gentle submodule errors Stefan Beller
  2017-03-21  0:11 ` [PATCH 3/3] builtin/describe: introduce --submodule-error-as-dirty flag Stefan Beller
  2 siblings, 0 replies; 3+ messages in thread
From: Stefan Beller @ 2017-03-21  0:11 UTC (permalink / raw)
  To: jrnieder; +Cc: git, mfick, Stefan Beller

Migrate 'is_submodule_modified' to the new porcelain format of
git-status.

As the old porcelain only reported ' M' for submodules, no
matter what happened inside the submodule (untracked files,
changes to tracked files or move of HEAD), the new API
properly reports the different scenarios.

In a followup patch we will make use of these finer grained
reporting for git-status.

While porting this to the new API, add another extension
point that will get used in the future: When a submodule
is broken (e.g. the .git file pointing to a wrong directory,
not containing a git dir, as fallout of e.g. f8eaa0ba98
(submodule--helper, module_clone: always operate on absolute
paths, 2016-03-31)), we can chose to not die and report it
differently.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 85 +++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 49 insertions(+), 36 deletions(-)

diff --git a/submodule.c b/submodule.c
index 3200b7bb2b..81d44cb7e9 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1041,67 +1041,80 @@ int fetch_populated_submodules(const struct argv_array *options,
 
 unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
-	ssize_t len;
 	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *argv[] = {
-		"status",
-		"--porcelain",
-		NULL,
-		NULL,
-	};
 	struct strbuf buf = STRBUF_INIT;
 	unsigned dirty_submodule = 0;
-	const char *line, *next_line;
 	const char *git_dir;
+	int error_code = 0;
 
 	strbuf_addf(&buf, "%s/.git", path);
-	git_dir = read_gitfile(buf.buf);
-	if (!git_dir)
-		git_dir = buf.buf;
-	if (!is_directory(git_dir)) {
-		strbuf_release(&buf);
-		/* The submodule is not checked out, so it is not modified */
-		return 0;
+	git_dir = resolve_gitdir_gently(buf.buf, &error_code);
 
+	if (!git_dir) {
+		switch (error_code) {
+		case READ_GITFILE_ERR_STAT_FAILED:
+		case READ_GITFILE_ERR_NOT_A_FILE:
+			/* We may have an uninitialized repo here */
+			return 0;
+		default:
+		case READ_GITFILE_ERR_OPEN_FAILED:
+		case READ_GITFILE_ERR_READ_FAILED:
+		case READ_GITFILE_ERR_INVALID_FORMAT:
+		case READ_GITFILE_ERR_NO_PATH:
+		case READ_GITFILE_ERR_NOT_A_REPO:
+		case READ_GITFILE_ERR_TOO_LARGE:
+			/*
+			 * All these other error codes are indicating
+			 * a broken submodule. We do not know what is
+			 * right here. Resolve again triggering die()
+			 * inside of the parsing.
+			 */
+			read_gitfile_gently(buf.buf, NULL);
+			die("BUG: read_gitfile_gently should have died.");
+		}
 	}
+
 	strbuf_reset(&buf);
 
+	argv_array_pushl(&cp.args, "status", "--porcelain=2", NULL);
 	if (ignore_untracked)
-		argv[2] = "-uno";
+		argv_array_push(&cp.args, "-uno");
 
-	cp.argv = argv;
 	prepare_submodule_repo_env(&cp.env_array);
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;
 	cp.out = -1;
 	cp.dir = path;
 	if (start_command(&cp))
-		die("Could not run 'git status --porcelain' in submodule %s", path);
+		die("Could not run 'git status --porcelain=2' in submodule %s", path);
 
-	len = strbuf_read(&buf, cp.out, 1024);
-	line = buf.buf;
-	while (len > 2) {
-		if ((line[0] == '?') && (line[1] == '?')) {
+	while (strbuf_getwholeline_fd(&buf, cp.out, '\n') != EOF) {
+		/* regular untracked files */
+		if (buf.buf[0] == '?')
 			dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-			if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
-				break;
-		} else {
-			dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
-			if (ignore_untracked ||
-			    (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))
-				break;
+
+		/* regular unmerged and renamed files */
+		if (buf.buf[0] == 'u' ||
+		    buf.buf[0] == '1' ||
+		    buf.buf[0] == '2') {
+			if (buf.buf[5] == 'S') {
+				/* nested submodule handling */
+				if (buf.buf[6] == 'C' || buf.buf[7] == 'M')
+					dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+				if (buf.buf[8] == 'U')
+					dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+			} else
+				dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
 		}
-		next_line = strchr(line, '\n');
-		if (!next_line)
-			break;
-		next_line++;
-		len -= (next_line - line);
-		line = next_line;
+
+		if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED &&
+		    dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
+				break;
 	}
 	close(cp.out);
 
 	if (finish_command(&cp))
-		die("'git status --porcelain' failed in submodule %s", path);
+		die("'git status --porcelain=2' failed in submodule %s", path);
 
 	strbuf_release(&buf);
 	return dirty_submodule;
-- 
2.12.0.402.g4b3201c2d6.dirty


^ permalink raw reply	[flat|threaded] 3+ messages in thread

* [PATCH 2/3] revision machinery: gentle submodule errors
       [not found] <20170321001156.21915-1-sbeller@google.com>
  2017-03-21  0:11 ` [PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2 Stefan Beller
@ 2017-03-21  0:11 ` Stefan Beller
  2017-03-21  0:11 ` [PATCH 3/3] builtin/describe: introduce --submodule-error-as-dirty flag Stefan Beller
  2 siblings, 0 replies; 3+ messages in thread
From: Stefan Beller @ 2017-03-21  0:11 UTC (permalink / raw)
  To: jrnieder; +Cc: git, mfick, Stefan Beller

When a submodule is broken (e.g. its git directory is deleted or is not
pointed to via the .git linking file), commands that deal with submodules
error out reporting the submodule error.

Sometimes users rather want to see the rest of the command succeed,
ignoring the submodule that is severely broken.

Introduce a flag for the revision machinery '--gentle-submodule-errors'
that will ask 'is_submodule_modified' to return the broken flag instead
of dying().

By switching the argument of is_submodule_modified to an unsigned bit
vector, we can signal more flags, such as the gentle bit introduced
in this patch. When this flag is given, avoid dying inside
'is_submodule_modified' for severe errors and return
DIRTY_SUBMODULE_BROKEN instead.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff-lib.c                                   | 10 ++++++++--
 diff.h                                       |  1 +
 diffcore.h                                   |  3 ++-
 revision.c                                   |  2 ++
 submodule.c                                  |  9 ++++++++-
 submodule.h                                  | 10 +++++++++-
 t/t4060-diff-submodule-option-diff-format.sh | 22 ++++++++++++++++++++++
 7 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 52447466b5..61d33b6d26 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -77,8 +77,14 @@ static int match_stat_with_submodule(struct diff_options *diffopt,
 		if (DIFF_OPT_TST(diffopt, IGNORE_SUBMODULES))
 			changed = 0;
 		else if (!DIFF_OPT_TST(diffopt, IGNORE_DIRTY_SUBMODULES)
-		    && (!changed || DIFF_OPT_TST(diffopt, DIRTY_SUBMODULES)))
-			*dirty_submodule = is_submodule_modified(ce->name, DIFF_OPT_TST(diffopt, IGNORE_UNTRACKED_IN_SUBMODULES));
+		    && (!changed || DIFF_OPT_TST(diffopt, DIRTY_SUBMODULES))) {
+			unsigned flags = 0;
+			if (DIFF_OPT_TST(diffopt, IGNORE_UNTRACKED_IN_SUBMODULES))
+				flags |= IS_SUBMODULE_MODIFIED_IGNORE_UNTRACKED;
+			if (diffopt->gentle_submodule_errors)
+				flags |= IS_SUBMODULE_MODIFIED_GENTLY;
+			*dirty_submodule = is_submodule_modified(ce->name, flags);
+		}
 		diffopt->flags = orig_flags;
 	}
 	return changed;
diff --git a/diff.h b/diff.h
index e9ccb38c26..d1e48de43c 100644
--- a/diff.h
+++ b/diff.h
@@ -164,6 +164,7 @@ struct diff_options {
 	const char *word_regex;
 	enum diff_words_type word_diff;
 	enum diff_submodule_format submodule_format;
+	int gentle_submodule_errors;
 
 	/* this is set by diffcore for DIFF_FORMAT_PATCH */
 	int found_changes;
diff --git a/diffcore.h b/diffcore.h
index 6230241354..ceef783570 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -40,9 +40,10 @@ struct diff_filespec {
 #define DIFF_FILE_VALID(spec) (((spec)->mode) != 0)
 	unsigned should_free : 1; /* data should be free()'ed */
 	unsigned should_munmap : 1; /* data should be munmap()'ed */
-	unsigned dirty_submodule : 2;  /* For submodules: its work tree is dirty */
+	unsigned dirty_submodule : 3;  /* For submodules: its work tree is dirty */
 #define DIRTY_SUBMODULE_UNTRACKED 1
 #define DIRTY_SUBMODULE_MODIFIED  2
+#define DIRTY_SUBMODULE_BROKEN 4
 	unsigned is_stdin : 1;
 	unsigned has_more_entries : 1; /* only appear in combined diff */
 	/* data should be considered "binary"; -1 means "don't know yet" */
diff --git a/revision.c b/revision.c
index 7ff61ff5f7..8ae1179b8d 100644
--- a/revision.c
+++ b/revision.c
@@ -2014,6 +2014,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->limited = 1;
 	} else if (!strcmp(arg, "--ignore-missing")) {
 		revs->ignore_missing = 1;
+	} else if (!strcmp(arg, "--gentle-submodule-errors")) {
+		revs->diffopt.gentle_submodule_errors = 1;
 	} else {
 		int opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);
 		if (!opts)
diff --git a/submodule.c b/submodule.c
index 81d44cb7e9..d477297fab 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1039,13 +1039,18 @@ int fetch_populated_submodules(const struct argv_array *options,
 	return spf.result;
 }
 
-unsigned is_submodule_modified(const char *path, int ignore_untracked)
+/*
+ * Inspects a submodule and returns its state using flags
+ */
+unsigned is_submodule_modified(const char *path, unsigned flags)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	unsigned dirty_submodule = 0;
 	const char *git_dir;
 	int error_code = 0;
+	int ignore_untracked = flags & IS_SUBMODULE_MODIFIED_IGNORE_UNTRACKED;
+	int gently = flags & IS_SUBMODULE_MODIFIED_GENTLY;
 
 	strbuf_addf(&buf, "%s/.git", path);
 	git_dir = resolve_gitdir_gently(buf.buf, &error_code);
@@ -1069,6 +1074,8 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 			 * right here. Resolve again triggering die()
 			 * inside of the parsing.
 			 */
+			if (gently)
+				return DIRTY_SUBMODULE_BROKEN;
 			read_gitfile_gently(buf.buf, NULL);
 			die("BUG: read_gitfile_gently should have died.");
 		}
diff --git a/submodule.h b/submodule.h
index c8a0c9cb29..b7afac5e87 100644
--- a/submodule.h
+++ b/submodule.h
@@ -62,7 +62,15 @@ extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);
 extern int fetch_populated_submodules(const struct argv_array *options,
 			       const char *prefix, int command_line_option,
 			       int quiet, int max_parallel_jobs);
-extern unsigned is_submodule_modified(const char *path, int ignore_untracked);
+
+/* Ignore untracked files */
+#define IS_SUBMODULE_MODIFIED_IGNORE_UNTRACKED 1
+/*
+ * Report severe errors in the submodule (e.g. missing git dir) via
+ * DIRTY_SUBMODULE_BROKEN instead of dying.
+ */
+#define IS_SUBMODULE_MODIFIED_GENTLY 2
+extern unsigned is_submodule_modified(const char *path, unsigned flags);
 extern int submodule_uses_gitfile(const char *path);
 
 #define SUBMODULE_REMOVAL_DIE_ON_ERROR (1<<0)
diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh
index 7e23b55ea4..e5e11a8378 100755
--- a/t/t4060-diff-submodule-option-diff-format.sh
+++ b/t/t4060-diff-submodule-option-diff-format.sh
@@ -746,4 +746,26 @@ test_expect_success 'diff --submodule=diff with .git file' '
 	test_cmp expected actual
 '
 
+test_expect_success 'setup -- break submodule' '
+	# the gitlink in sm2 points at a missing git dir now
+	mv .real .moved_real
+'
+
+test_expect_success 'diff with severely broken submodule' '
+	test_must_fail git diff &&
+
+	sm_hash=$(git rev-parse HEAD:sm2) &&
+	cat >expect <<-EOF &&
+	diff --git a/sm2 b/sm2
+	--- a/sm2
+	+++ b/sm2
+	@@ -1 +1 @@
+	-Subproject commit $sm_hash
+	+Subproject commit $sm_hash-dirty
+	EOF
+
+	git diff --gentle-submodule-errors -- sm2 >out &&
+	test_cmp expect out
+'
+
 test_done
-- 
2.12.0.402.g4b3201c2d6.dirty


^ permalink raw reply	[flat|threaded] 3+ messages in thread

* [PATCH 3/3] builtin/describe: introduce --submodule-error-as-dirty flag
       [not found] <20170321001156.21915-1-sbeller@google.com>
  2017-03-21  0:11 ` [PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2 Stefan Beller
  2017-03-21  0:11 ` [PATCH 2/3] revision machinery: gentle submodule errors Stefan Beller
@ 2017-03-21  0:11 ` Stefan Beller
  2 siblings, 0 replies; 3+ messages in thread
From: Stefan Beller @ 2017-03-21  0:11 UTC (permalink / raw)
  To: jrnieder; +Cc: git, mfick, Stefan Beller

git-describe tells you the version number you're at, or errors out, e.g.
when you run it outside of a repository, which may happen when downloading
a tar ball instead of using git to obtain the source code.

To keep this property of only erroring out, when not in a repository,
severe submodule errors must be downgraded to reporting them gently
instead of having git-describe error out completely.

This patch helps to fix the root cause in [1], which tries to work around
this situation.

[1] ("Work around git describe bug for build.")
https://gerrit-review.googlesource.com/#/c/99851/

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/describe.c  | 17 ++++++++++-------
 t/t6120-describe.sh | 17 +++++++++++++++++
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 76c18059bf..569fef9ecf 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -31,13 +31,9 @@ static int have_util;
 static struct string_list patterns = STRING_LIST_INIT_NODUP;
 static struct string_list exclude_patterns = STRING_LIST_INIT_NODUP;
 static int always;
+static int gentle_submodule_errors;
 static const char *dirty;
 
-/* diff-index command arguments to check if working tree is dirty. */
-static const char *diff_index_args[] = {
-	"diff-index", "--quiet", "HEAD", "--", NULL
-};
-
 struct commit_name {
 	struct hashmap_entry entry;
 	struct object_id peeled;
@@ -442,6 +438,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			   N_("do not consider tags matching <pattern>")),
 		OPT_BOOL(0, "always",        &always,
 			N_("show abbreviated commit object as fallback")),
+		OPT_BOOL(0, "submodule-error-as-dirty", &gentle_submodule_errors,
+			N_("show abbreviated commit object as fallback")),
 		{OPTION_STRING, 0, "dirty",  &dirty, N_("mark"),
 			N_("append <mark> on dirty working tree (default: \"-dirty\")"),
 			PARSE_OPT_OPTARG, NULL, (intptr_t) "-dirty"},
@@ -496,6 +494,12 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 		if (dirty) {
 			static struct lock_file index_lock;
 			int fd;
+			struct argv_array args = ARGV_ARRAY_INIT;
+
+			argv_array_push(&args, "diff-index");
+			if (gentle_submodule_errors)
+				argv_array_push(&args, "--gentle-submodule-errors");
+			argv_array_pushl(&args, "--quiet", "HEAD", "--", NULL);
 
 			read_cache_preload(NULL);
 			refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,
@@ -504,8 +508,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
 			if (0 <= fd)
 				update_index_if_able(&the_index, &index_lock);
 
-			if (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1,
-					    diff_index_args, prefix))
+			if (!cmd_diff_index(args.argc, args.argv, prefix))
 				dirty = NULL;
 		}
 		describe("HEAD", 1);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 167491fd5b..99e5ba44b7 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -233,4 +233,21 @@ test_expect_success 'describe --contains and --no-match' '
 	test_cmp expect actual
 '
 
+test_expect_success 'setup and absorb a submodule' '
+	test_create_repo sub1 &&
+	test_commit -C sub1 initial &&
+	git submodule add ./sub1 &&
+	git submodule absorbgitdirs &&
+	git commit -a -m "add submodule"
+'
+
+test_expect_success 'describe chokes on severly broken submodules' '
+	mv .git/modules/sub1/ .git/modules/sub_moved &&
+	test_must_fail git describe --dirty
+'
+test_expect_success 'describe ignoring a borken submodule' '
+	git describe --dirty --submodule-error-as-dirty >out &&
+	grep dirty out
+'
+
 test_done
-- 
2.12.0.402.g4b3201c2d6.dirty


^ permalink raw reply	[flat|threaded] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20170321001156.21915-1-sbeller@google.com>
2017-03-21  0:11 ` [PATCH 1/3] submodule.c: port is_submodule_modified to use porcelain 2 Stefan Beller
2017-03-21  0:11 ` [PATCH 2/3] revision machinery: gentle submodule errors Stefan Beller
2017-03-21  0:11 ` [PATCH 3/3] builtin/describe: introduce --submodule-error-as-dirty flag Stefan Beller

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox