Git development
 help / color / mirror / Atom feed
* [PATCH 10/15] update submodules: add submodule_go_from_to
From: Stefan Beller @ 2017-02-16  0:38 UTC (permalink / raw)
  Cc: git, sandals, jrnieder, bmwill, gitster, Stefan Beller
In-Reply-To: <20170216003811.18273-1-sbeller@google.com>

In later patches we introduce the options and flag for commands
that modify the working directory, e.g. git-checkout.

This piece of code will be used universally for
all these working tree modifications as it
* supports dry run to answer the question:
  "Is it safe to change the submodule to this new state?"
  e.g. is it overwriting untracked files or are there local
  changes that would be overwritten?
* supports a force flag that can be used for resetting
  the tree.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 151 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 submodule.h |   5 ++
 2 files changed, 156 insertions(+)

diff --git a/submodule.c b/submodule.c
index b262c5b0ad..84cc62f3bb 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1250,6 +1250,157 @@ int bad_to_remove_submodule(const char *path, unsigned flags)
 	return ret;
 }
 
+static int submodule_has_dirty_index(const struct submodule *sub)
+{
+	ssize_t len;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf buf = STRBUF_INIT;
+	int ret = 0;
+
+	prepare_submodule_repo_env_no_git_dir(&cp.env_array);
+
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "diff-index", "--cached", "HEAD", NULL);
+	cp.no_stdin = 1;
+	cp.out = -1;
+	cp.dir = sub->path;
+	if (start_command(&cp))
+		die("could not recurse into submodule %s", sub->path);
+
+	len = strbuf_read(&buf, cp.out, 1024);
+	if (len > 2)
+		ret = 1;
+
+	close(cp.out);
+	if (finish_command(&cp))
+		die("could not recurse into submodule %s", sub->path);
+
+	strbuf_release(&buf);
+	return ret;
+}
+
+void submodule_clean_index(const char *path)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	prepare_submodule_repo_env_no_git_dir(&cp.env_array);
+
+	cp.git_cmd = 1;
+	cp.no_stdin = 1;
+	cp.dir = path;
+
+	argv_array_pushf(&cp.args, "--super-prefix=%s/", path);
+	argv_array_pushl(&cp.args, "read-tree", "-u", "--reset", NULL);
+
+	argv_array_push(&cp.args, EMPTY_TREE_SHA1_HEX);
+
+	if (run_command(&cp))
+		die("could not clean submodule index");
+}
+
+/**
+ * Moves a submodule at a given path from a given head to another new head.
+ * For edge cases (a submodule coming into existence or removing a submodule)
+ * pass NULL for old or new respectively.
+ *
+ * TODO: move dryrun and forced to flags.
+ */
+int submodule_go_from_to(const char *path,
+			 const char *old,
+			 const char *new,
+			 int dry_run,
+			 int force)
+{
+	int ret = 0;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	const struct submodule *sub;
+
+	sub = submodule_from_path(null_sha1, path);
+
+	if (!sub)
+		die("BUG: could not get submodule information for '%s'", path);
+
+	if (!dry_run) {
+		if (old) {
+			if (!submodule_uses_gitfile(path))
+				absorb_git_dir_into_superproject("", path,
+					ABSORB_GITDIR_RECURSE_SUBMODULES);
+		} else {
+			struct strbuf sb = STRBUF_INIT;
+			strbuf_addf(&sb, "%s/modules/%s",
+				    get_git_common_dir(), sub->name);
+			connect_work_tree_and_git_dir(path, sb.buf);
+			strbuf_release(&sb);
+
+			/* make sure the index is clean as well */
+			submodule_clean_index(path);
+		}
+	}
+
+	if (old && !force) {
+		/* Check if the submodule has a dirty index. */
+		if (submodule_has_dirty_index(sub)) {
+			/* print a thing here? */
+			return -1;
+		}
+	}
+
+	prepare_submodule_repo_env_no_git_dir(&cp.env_array);
+
+	cp.git_cmd = 1;
+	cp.no_stdin = 1;
+	cp.dir = path;
+
+	argv_array_pushf(&cp.args, "--super-prefix=%s/", path);
+	argv_array_pushl(&cp.args, "read-tree", NULL);
+
+	if (dry_run)
+		argv_array_push(&cp.args, "-n");
+	else
+		argv_array_push(&cp.args, "-u");
+
+	if (force)
+		argv_array_push(&cp.args, "--reset");
+	else
+		argv_array_push(&cp.args, "-m");
+
+	argv_array_push(&cp.args, old ? old : EMPTY_TREE_SHA1_HEX);
+	argv_array_push(&cp.args, new ? new : EMPTY_TREE_SHA1_HEX);
+
+	if (run_command(&cp)) {
+		ret = -1;
+		goto out;
+	}
+
+	if (!dry_run) {
+		if (new) {
+			struct child_process cp1 = CHILD_PROCESS_INIT;
+			/* also set the HEAD accordingly */
+			cp1.git_cmd = 1;
+			cp1.no_stdin = 1;
+			cp1.dir = path;
+
+			argv_array_pushl(&cp1.args, "update-ref", "HEAD",
+					 new ? new : EMPTY_TREE_SHA1_HEX, NULL);
+
+			if (run_command(&cp1)) {
+				ret = -1;
+				goto out;
+			}
+		} else {
+			struct strbuf sb = STRBUF_INIT;
+
+			strbuf_addf(&sb, "%s/.git", path);
+			unlink_or_warn(sb.buf);
+			strbuf_release(&sb);
+
+			if (is_empty_dir(path))
+				rmdir_or_warn(path);
+		}
+	}
+out:
+	return ret;
+}
+
 static int find_first_merges(struct object_array *result, const char *path,
 		struct commit *a, struct commit *b)
 {
diff --git a/submodule.h b/submodule.h
index 46d9f0f293..3336607bfc 100644
--- a/submodule.h
+++ b/submodule.h
@@ -102,6 +102,11 @@ extern int push_unpushed_submodules(struct sha1_array *commits,
 extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
 extern int parallel_submodules(void);
 
+extern int submodule_go_from_to(const char *path,
+				const char *old,
+				const char *new,
+				int dry_run, int force);
+
 /*
  * Prepare the "env_array" parameter of a "struct child_process" for executing
  * a submodule by clearing any repo-specific envirionment variables, but
-- 
2.12.0.rc1.16.ge4278d41a0.dirty


^ permalink raw reply related

* [PATCH 06/15] update submodules: add submodule config parsing
From: Stefan Beller @ 2017-02-16  0:38 UTC (permalink / raw)
  Cc: git, sandals, jrnieder, bmwill, gitster, Stefan Beller
In-Reply-To: <20170216003811.18273-1-sbeller@google.com>

Similar to b33a15b08 (push: add recurseSubmodules config option,
2015-11-17) and 027771fcb1 (submodule: allow erroneous values for the
fetchRecurseSubmodules option, 2015-08-17), we add submodule-config code
that is later used to parse whether we are interested in updating
submodules.

We need the `die_on_error` parameter to be able to call this parsing
function for the config file as well, which if incorrect lets Git die.

As we're just touching the header file, also mark all functions extern.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule-config.c | 22 ++++++++++++++++++++++
 submodule-config.h | 17 +++++++++--------
 2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 93453909cf..93f01c4378 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -234,6 +234,28 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)
 	return parse_fetch_recurse(opt, arg, 1);
 }
 
+static int parse_update_recurse(const char *opt, const char *arg,
+				int die_on_error)
+{
+	switch (git_config_maybe_bool(opt, arg)) {
+	case 1:
+		return RECURSE_SUBMODULES_ON;
+	case 0:
+		return RECURSE_SUBMODULES_OFF;
+	default:
+		if (!strcmp(arg, "checkout"))
+			return RECURSE_SUBMODULES_ON;
+		if (die_on_error)
+			die("bad %s argument: %s", opt, arg);
+		return RECURSE_SUBMODULES_ERROR;
+	}
+}
+
+int parse_update_recurse_submodules_arg(const char *opt, const char *arg)
+{
+	return parse_update_recurse(opt, arg, 1);
+}
+
 static int parse_push_recurse(const char *opt, const char *arg,
 			       int die_on_error)
 {
diff --git a/submodule-config.h b/submodule-config.h
index 70f19363fd..d434ecdb45 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -22,16 +22,17 @@ struct submodule {
 	int recommend_shallow;
 };
 
-int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
-int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
-int parse_submodule_config_option(const char *var, const char *value);
-const struct submodule *submodule_from_name(const unsigned char *commit_or_tree,
-		const char *name);
-const struct submodule *submodule_from_path(const unsigned char *commit_or_tree,
-		const char *path);
+extern int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);
+extern int parse_update_recurse_submodules_arg(const char *opt, const char *arg);
+extern int parse_push_recurse_submodules_arg(const char *opt, const char *arg);
+extern int parse_submodule_config_option(const char *var, const char *value);
+extern const struct submodule *submodule_from_name(
+		const unsigned char *commit_or_tree, const char *name);
+extern const struct submodule *submodule_from_path(
+		const unsigned char *commit_or_tree, const char *path);
 extern int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
 				      unsigned char *gitmodules_sha1,
 				      struct strbuf *rev);
-void submodule_free(void);
+extern void submodule_free(void);
 
 #endif /* SUBMODULE_CONFIG_H */
-- 
2.12.0.rc1.16.ge4278d41a0.dirty


^ permalink raw reply related

* [PATCH 13/15] read-cache: remove_marked_cache_entries to wipe selected submodules.
From: Stefan Beller @ 2017-02-16  0:38 UTC (permalink / raw)
  Cc: git, sandals, jrnieder, bmwill, gitster, Stefan Beller
In-Reply-To: <20170216003811.18273-1-sbeller@google.com>

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 read-cache.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/read-cache.c b/read-cache.c
index 9054369dd0..b78a7f02e3 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -18,6 +18,7 @@
 #include "varint.h"
 #include "split-index.h"
 #include "utf8.h"
+#include "submodule.h"
 
 /* Mask for the name length in ce_flags in the on-disk index */
 
@@ -532,6 +533,8 @@ void remove_marked_cache_entries(struct index_state *istate)
 
 	for (i = j = 0; i < istate->cache_nr; i++) {
 		if (ce_array[i]->ce_flags & CE_REMOVE) {
+			if (is_active_submodule_with_strategy(ce_array[i], SM_UPDATE_UNSPECIFIED))
+				submodule_go_from_to(ce_array[i]->name, "HEAD", NULL, 0, 1);
 			remove_name_hash(istate, ce_array[i]);
 			save_or_free_index_entry(istate, ce_array[i]);
 		}
-- 
2.12.0.rc1.16.ge4278d41a0.dirty


^ permalink raw reply related

* [PATCH 15/15] builtin/checkout: add --recurse-submodules switch
From: Stefan Beller @ 2017-02-16  0:38 UTC (permalink / raw)
  Cc: git, sandals, jrnieder, bmwill, gitster, Stefan Beller
In-Reply-To: <20170216003811.18273-1-sbeller@google.com>

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-checkout.txt |  7 +++++++
 builtin/checkout.c             | 28 ++++++++++++++++++++++++++++
 t/lib-submodule-update.sh      | 33 ++++++++++++++++++++++++---------
 t/t2013-checkout-submodule.sh  |  5 +++++
 4 files changed, 64 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index 8e2c0662dd..d6399c0af8 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -256,6 +256,13 @@ section of linkgit:git-add[1] to learn how to operate the `--patch` mode.
 	out anyway. In other words, the ref can be held by more than one
 	worktree.
 
+--[no-]recurse-submodules::
+	Using --recurse-submodules will update the content of all initialized
+	submodules according to the commit recorded in the superproject. If
+	local modifications in a submodule would be overwritten the checkout
+	will fail unless `-f` is used. If nothing (or --no-recurse-submodules)
+	is used, the work trees of submodules will not be updated.
+
 <branch>::
 	Branch to checkout; if it refers to a branch (i.e., a name that,
 	when prepended with "refs/heads/", is a valid ref), then that
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f174f50303..207ce09771 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -21,12 +21,31 @@
 #include "submodule-config.h"
 #include "submodule.h"
 
+static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
+
 static const char * const checkout_usage[] = {
 	N_("git checkout [<options>] <branch>"),
 	N_("git checkout [<options>] [<branch>] -- <file>..."),
 	NULL,
 };
 
+int option_parse_recurse_submodules(const struct option *opt,
+				    const char *arg, int unset)
+{
+	if (unset) {
+		recurse_submodules = RECURSE_SUBMODULES_OFF;
+		return 0;
+	}
+	if (arg)
+		recurse_submodules =
+			parse_update_recurse_submodules_arg(opt->long_name,
+							    arg);
+	else
+		recurse_submodules = RECURSE_SUBMODULES_ON;
+
+	return 0;
+}
+
 struct checkout_opts {
 	int patch_mode;
 	int quiet;
@@ -1163,6 +1182,9 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 				N_("second guess 'git checkout <no-such-branch>'")),
 		OPT_BOOL(0, "ignore-other-worktrees", &opts.ignore_other_worktrees,
 			 N_("do not check if another worktree is holding the given ref")),
+		{ OPTION_CALLBACK, 0, "recurse-submodules", &recurse_submodules,
+			    "checkout", "control recursive updating of submodules",
+			    PARSE_OPT_OPTARG, option_parse_recurse_submodules },
 		OPT_BOOL(0, "progress", &opts.show_progress, N_("force progress reporting")),
 		OPT_END(),
 	};
@@ -1193,6 +1215,12 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
 	}
 
+	if (recurse_submodules != RECURSE_SUBMODULES_OFF) {
+		git_config(submodule_config, NULL);
+		if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
+			set_config_update_recurse_submodules(recurse_submodules);
+	}
+
 	if ((!!opts.new_branch + !!opts.new_branch_force + !!opts.new_orphan_branch) > 1)
 		die(_("-b, -B and --orphan are mutually exclusive"));
 
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index ea838df028..4693ba7a7e 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -756,6 +756,11 @@ test_submodule_forced_switch () {
 
 test_submodule_switch_recursing () {
 	command="$1"
+	RESULT=success
+	if test "$KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS" = 1
+	then
+		RESULT=failure
+	fi
 	######################### Appearing submodule #########################
 	# Switching to a commit letting a submodule appear checks it out ...
 	test_expect_success "$command: added submodule is checked out" '
@@ -865,7 +870,7 @@ test_submodule_switch_recursing () {
 	'
 	# Replacing a submodule with files in a directory must succeeds
 	# when the submodule is clean
-	test_expect_success "$command: replace submodule with a directory" '
+	test_expect_$RESULT "$command: replace submodule with a directory" '
 		prolog &&
 		reset_work_tree_to_interested add_sub1 &&
 		(
@@ -877,7 +882,7 @@ test_submodule_switch_recursing () {
 		)
 	'
 	# ... absorbing a .git directory.
-	test_expect_success "$command: replace submodule containing a .git directory with a directory must absorb the git dir" '
+	test_expect_$RESULT "$command: replace submodule containing a .git directory with a directory must absorb the git dir" '
 		prolog &&
 		reset_work_tree_to_interested add_sub1 &&
 		(
@@ -905,7 +910,7 @@ test_submodule_switch_recursing () {
 	'
 
 	# ... must check its local work tree for untracked files
-	test_expect_success "$command: replace submodule with a file must fail with untracked files" '
+	test_expect_$RESULT "$command: replace submodule with a file must fail with untracked files" '
 		prolog &&
 		reset_work_tree_to_interested add_sub1 &&
 		(
@@ -961,16 +966,21 @@ test_submodule_switch_recursing () {
 		)
 	'
 
+	# This test fails, due to missing setup, we do not clone sub2 into
+	# submodule_update, because it doesn't exist in the 'add_sub1' version
+	#
 	test_expect_success "$command: modified submodule updates submodule recursively" '
 		prolog &&
 		reset_work_tree_to_interested add_sub1 &&
 		(
 			cd submodule_update &&
 			git branch -t modify_sub1_recursively origin/modify_sub1_recursively &&
-			$command modify_sub1_recursively &&
-			test_superproject_content origin/modify_sub1_recursively &&
-			test_submodule_content sub1 origin/modify_sub1_recursively
-			test_submodule_content sub1/sub2
+			test_must_fail $command modify_sub1_recursively &&
+			test_superproject_content origin/add_sub1 &&
+			test_submodule_content sub1 origin/add_sub1
+			# test_superproject_content origin/modify_sub1_recursively &&
+			# test_submodule_content sub1 origin/modify_sub1_recursively &&
+			# test_submodule_content sub1/sub2 no_submodule
 		)
 	'
 }
@@ -980,6 +990,11 @@ test_submodule_switch_recursing () {
 # the superproject as well as the submodule is allowed.
 test_submodule_forced_switch_recursing () {
 	command="$1"
+	RESULT=success
+	if test "$KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS" = 1
+	then
+		RESULT=failure
+	fi
 	######################### Appearing submodule #########################
 	# Switching to a commit letting a submodule appear creates empty dir ...
 	test_expect_success "$command: added submodule is checked out" '
@@ -1074,7 +1089,7 @@ test_submodule_forced_switch_recursing () {
 		)
 	'
 	# Replacing a submodule with files in a directory ...
-	test_expect_success "$command: replace submodule with a directory" '
+	test_expect_$RESULT "$command: replace submodule with a directory" '
 		prolog &&
 		reset_work_tree_to_interested add_sub1 &&
 		(
@@ -1125,7 +1140,7 @@ test_submodule_forced_switch_recursing () {
 	'
 
 	# ... but stops for untracked files that would be lost
-	test_expect_success "$command: replace submodule with a file" '
+	test_expect_$RESULT "$command: replace submodule with a file stops for untracked files" '
 		prolog &&
 		reset_work_tree_to_interested add_sub1 &&
 		(
diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh
index 6847f75822..aa35223369 100755
--- a/t/t2013-checkout-submodule.sh
+++ b/t/t2013-checkout-submodule.sh
@@ -63,6 +63,11 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/
 	! test -s actual
 '
 
+KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
+test_submodule_switch_recursing "git checkout --recurse-submodules"
+
+test_submodule_forced_switch_recursing "git checkout -f --recurse-submodules"
+
 test_submodule_switch "git checkout"
 
 test_submodule_forced_switch "git checkout -f"
-- 
2.12.0.rc1.16.ge4278d41a0.dirty


^ permalink raw reply related

* [PATCH 12/15] unpack-trees: check if we can perform the operation for submodules
From: Stefan Beller @ 2017-02-16  0:38 UTC (permalink / raw)
  Cc: git, sandals, jrnieder, bmwill, gitster, Stefan Beller
In-Reply-To: <20170216003811.18273-1-sbeller@google.com>

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 unpack-trees.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
 unpack-trees.h |  1 +
 2 files changed, 90 insertions(+), 9 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 616a0ae4b2..40af8e9b5f 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -10,6 +10,7 @@
 #include "attr.h"
 #include "split-index.h"
 #include "dir.h"
+#include "submodule.h"
 
 /*
  * Error messages expected by scripts out of plumbing commands such as
@@ -45,6 +46,9 @@ static const char *unpack_plumbing_errors[NB_UNPACK_TREES_ERROR_TYPES] = {
 
 	/* ERROR_WOULD_LOSE_ORPHANED_REMOVED */
 	"Working tree file '%s' would be removed by sparse checkout update.",
+
+	/* ERROR_WOULD_LOSE_UNTRACKED_SUBMODULE */
+	"Submodule '%s' cannot be deleted as it contains untracked files.",
 };
 
 #define ERRORMSG(o,type) \
@@ -161,6 +165,8 @@ void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
 		_("The following working tree files would be overwritten by sparse checkout update:\n%s");
 	msgs[ERROR_WOULD_LOSE_ORPHANED_REMOVED] =
 		_("The following working tree files would be removed by sparse checkout update:\n%s");
+	msgs[ERROR_WOULD_LOSE_UNTRACKED_SUBMODULE] =
+		_("Submodule '%s' cannot be deleted as it contains untracked files.");
 
 	opts->show_all_errors = 1;
 	/* rejected paths may not have a static buffer */
@@ -240,12 +246,44 @@ static void display_error_msgs(struct unpack_trees_options *o)
 		fprintf(stderr, _("Aborting\n"));
 }
 
+static int submodule_check_from_to(const struct cache_entry *ce, const char *old_id, const char *new_id, struct unpack_trees_options *o)
+{
+	if (submodule_go_from_to(ce->name, old_id,
+				 new_id, 1, o->reset))
+		return o->gently ? -1 :
+			add_rejected_path(o, ERROR_WOULD_LOSE_UNTRACKED_SUBMODULE, ce->name);
+	return 0;
+}
+
+static void reload_gitmodules_file(struct index_state *index,
+				   struct checkout *state)
+{
+	int i;
+	for (i = 0; i < index->cache_nr; i++) {
+		struct cache_entry *ce = index->cache[i];
+		if (ce->ce_flags & CE_UPDATE) {
+
+			int r = strcmp(ce->name, ".gitmodules");
+			if (r < 0)
+				continue;
+			else if (r == 0) {
+				checkout_entry(ce, state, NULL);
+			} else
+				break;
+		}
+	}
+	gitmodules_config();
+	git_config(submodule_config, NULL);
+}
+
 /*
  * Unlink the last component and schedule the leading directories for
  * removal, such that empty directories get removed.
  */
 static void unlink_entry(const struct cache_entry *ce)
 {
+	if (is_active_submodule_with_strategy(ce, SM_UPDATE_UNSPECIFIED))
+		submodule_go_from_to(ce->name, "HEAD", NULL, 0, 1);
 	if (!check_leading_path(ce->name, ce_namelen(ce)))
 		return;
 	if (remove_or_warn(ce->ce_mode, ce->name))
@@ -301,6 +339,9 @@ static int check_updates(struct unpack_trees_options *o)
 	remove_marked_cache_entries(index);
 	remove_scheduled_dirs();
 
+	if (touch_submodules_in_worktree() && o->update && !o->dry_run)
+		reload_gitmodules_file(index, &state);
+
 	for (i = 0; i < index->cache_nr; i++) {
 		struct cache_entry *ce = index->cache[i];
 
@@ -1358,17 +1399,27 @@ static int verify_uptodate_1(const struct cache_entry *ce,
 	if (!lstat(ce->name, &st)) {
 		int flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;
 		unsigned changed = ie_match_stat(o->src_index, ce, &st, flags);
+
+		if (is_active_submodule_with_strategy(ce, SM_UPDATE_UNSPECIFIED)) {
+			int r;
+			r = submodule_check_from_to(ce,
+				"HEAD", oid_to_hex(&ce->oid), o);
+			if (r)
+				return o->gently ? -1 :
+					add_rejected_path(o, error_type, ce->name);
+			return 0;
+		}
+
 		if (!changed)
 			return 0;
 		/*
-		 * NEEDSWORK: the current default policy is to allow
-		 * submodule to be out of sync wrt the superproject
-		 * index.  This needs to be tightened later for
-		 * submodules that are marked to be automatically
-		 * checked out.
+		 * Historic default policy was to allow submodule to be out
+		 * of sync wrt the superproject index. If the submodule was
+		 * not considered interesting above, we don't care here.
 		 */
 		if (S_ISGITLINK(ce->ce_mode))
 			return 0;
+
 		errno = 0;
 	}
 	if (errno == ENOENT)
@@ -1412,7 +1463,12 @@ static int verify_clean_submodule(const char *old_sha1,
 				  enum unpack_trees_error_types error_type,
 				  struct unpack_trees_options *o)
 {
-	return 0;
+	if (!is_active_submodule_with_strategy(ce, SM_UPDATE_UNSPECIFIED))
+		return 0;
+
+	return submodule_check_from_to(ce,
+				       old_sha1,
+				       oid_to_hex(&ce->oid), o);
 }
 
 static int verify_clean_subdirectory(const struct cache_entry *ce,
@@ -1578,9 +1634,15 @@ static int verify_absent_1(const struct cache_entry *ce,
 		path = xmemdupz(ce->name, len);
 		if (lstat(path, &st))
 			ret = error_errno("cannot stat '%s'", path);
-		else
-			ret = check_ok_to_remove(path, len, DT_UNKNOWN, NULL,
-						 &st, error_type, o);
+		else {
+			if (is_active_submodule_with_strategy(ce, SM_UPDATE_UNSPECIFIED))
+				ret = submodule_check_from_to(ce,
+							oid_to_hex(&ce->oid),
+							NULL, o);
+			else
+				ret = check_ok_to_remove(path, len, DT_UNKNOWN, NULL,
+							 &st, error_type, o);
+		}
 		free(path);
 		return ret;
 	} else if (lstat(ce->name, &st)) {
@@ -1588,6 +1650,10 @@ static int verify_absent_1(const struct cache_entry *ce,
 			return error_errno("cannot stat '%s'", ce->name);
 		return 0;
 	} else {
+		if (is_active_submodule_with_strategy(ce, SM_UPDATE_UNSPECIFIED))
+			return submodule_check_from_to(ce, oid_to_hex(&ce->oid),
+						       NULL, o);
+
 		return check_ok_to_remove(ce->name, ce_namelen(ce),
 					  ce_to_dtype(ce), ce, &st,
 					  error_type, o);
@@ -1643,6 +1709,16 @@ static int merged_entry(const struct cache_entry *ce,
 			return -1;
 		}
 		invalidate_ce_path(merge, o);
+
+		if (is_active_submodule_with_strategy(ce, SM_UPDATE_UNSPECIFIED)) {
+			int ret = submodule_check_from_to(ce,
+							  NULL,
+							  oid_to_hex(&ce->oid),
+							  o);
+			if (ret)
+				return ret;
+		}
+
 	} else if (!(old->ce_flags & CE_CONFLICTED)) {
 		/*
 		 * See if we can re-use the old CE directly?
@@ -1663,6 +1739,10 @@ static int merged_entry(const struct cache_entry *ce,
 			update |= old->ce_flags & (CE_SKIP_WORKTREE | CE_NEW_SKIP_WORKTREE);
 			invalidate_ce_path(old, o);
 		}
+		if (is_active_submodule_with_strategy(ce, SM_UPDATE_UNSPECIFIED)) {
+			if (submodule_check_from_to(ce, oid_to_hex(&old->oid), oid_to_hex(&ce->oid), o))
+				return -1;
+		}
 	} else {
 		/*
 		 * Previously unmerged entry left as an existence
diff --git a/unpack-trees.h b/unpack-trees.h
index 36a73a6d00..c0427ce082 100644
--- a/unpack-trees.h
+++ b/unpack-trees.h
@@ -21,6 +21,7 @@ enum unpack_trees_error_types {
 	ERROR_SPARSE_NOT_UPTODATE_FILE,
 	ERROR_WOULD_LOSE_ORPHANED_OVERWRITTEN,
 	ERROR_WOULD_LOSE_ORPHANED_REMOVED,
+	ERROR_WOULD_LOSE_UNTRACKED_SUBMODULE,
 	NB_UNPACK_TREES_ERROR_TYPES
 };
 
-- 
2.12.0.rc1.16.ge4278d41a0.dirty


^ permalink raw reply related

* [PATCH 11/15] unpack-trees: pass old oid to verify_clean_submodule
From: Stefan Beller @ 2017-02-16  0:38 UTC (permalink / raw)
  Cc: git, sandals, jrnieder, bmwill, gitster, Stefan Beller
In-Reply-To: <20170216003811.18273-1-sbeller@google.com>

The check (which uses the old oid) is yet to be implemented, but this part
is just a refactor, so it can go separately first.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 unpack-trees.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 3a8ee19fe8..616a0ae4b2 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1407,7 +1407,8 @@ static void invalidate_ce_path(const struct cache_entry *ce,
  * Currently, git does not checkout subprojects during a superproject
  * checkout, so it is not going to overwrite anything.
  */
-static int verify_clean_submodule(const struct cache_entry *ce,
+static int verify_clean_submodule(const char *old_sha1,
+				  const struct cache_entry *ce,
 				  enum unpack_trees_error_types error_type,
 				  struct unpack_trees_options *o)
 {
@@ -1427,16 +1428,18 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
 	struct dir_struct d;
 	char *pathbuf;
 	int cnt = 0;
-	unsigned char sha1[20];
 
-	if (S_ISGITLINK(ce->ce_mode) &&
-	    resolve_gitlink_ref(ce->name, "HEAD", sha1) == 0) {
-		/* If we are not going to update the submodule, then
+	if (S_ISGITLINK(ce->ce_mode)) {
+		unsigned char sha1[20];
+		int sub_head = resolve_gitlink_ref(ce->name, "HEAD", sha1);
+		/*
+		 * If we are not going to update the submodule, then
 		 * we don't care.
 		 */
-		if (!hashcmp(sha1, ce->oid.hash))
+		if (!sub_head && !hashcmp(sha1, ce->oid.hash))
 			return 0;
-		return verify_clean_submodule(ce, error_type, o);
+		return verify_clean_submodule(sub_head ? NULL : sha1_to_hex(sha1),
+					      ce, error_type, o);
 	}
 
 	/*
-- 
2.12.0.rc1.16.ge4278d41a0.dirty


^ permalink raw reply related

* [PATCH 14/15] entry.c: update submodules when interesting
From: Stefan Beller @ 2017-02-16  0:38 UTC (permalink / raw)
  Cc: git, sandals, jrnieder, bmwill, gitster, Stefan Beller
In-Reply-To: <20170216003811.18273-1-sbeller@google.com>

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 entry.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/entry.c b/entry.c
index c6eea240b6..ae40611c97 100644
--- a/entry.c
+++ b/entry.c
@@ -2,6 +2,7 @@
 #include "blob.h"
 #include "dir.h"
 #include "streaming.h"
+#include "submodule.h"
 
 static void create_directories(const char *path, int path_len,
 			       const struct checkout *state)
@@ -203,6 +204,13 @@ static int write_entry(struct cache_entry *ce,
 			return error("cannot create temporary submodule %s", path);
 		if (mkdir(path, 0777) < 0)
 			return error("cannot create submodule directory %s", path);
+		if (is_active_submodule_with_strategy(ce, SM_UPDATE_UNSPECIFIED))
+				/*
+				 * force=1 is ok for any case as we did a dry
+				 * run before with appropriate force setting
+				 */
+				return submodule_go_from_to(ce->name,
+					NULL, oid_to_hex(&ce->oid), 0, 1);
 		break;
 	default:
 		return error("unknown file mode for %s in index", path);
@@ -260,6 +268,26 @@ int checkout_entry(struct cache_entry *ce,
 
 	if (!check_path(path.buf, path.len, &st, state->base_dir_len)) {
 		unsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE);
+		/*
+		 * Needs to be checked before !changed returns early,
+		 * as the possibly empty directory was not changed
+		 */
+		if (is_active_submodule_with_strategy(ce, SM_UPDATE_UNSPECIFIED)) {
+			int err;
+			if (!is_submodule_populated_gently(ce->name, &err)) {
+				struct stat sb;
+				if (lstat(ce->name, &sb))
+					die(_("could not stat file '%s'"), ce->name);
+				if (!(st.st_mode & S_IFDIR))
+					unlink_or_warn(ce->name);
+
+				return submodule_go_from_to(ce->name,
+					NULL, oid_to_hex(&ce->oid), 0, 1);
+			} else
+				return submodule_go_from_to(ce->name,
+					"HEAD", oid_to_hex(&ce->oid), 0, 1);
+		}
+
 		if (!changed)
 			return 0;
 		if (!state->force) {
-- 
2.12.0.rc1.16.ge4278d41a0.dirty


^ permalink raw reply related

* [PATCH 01/15] lib-submodule-update.sh: reorder create_lib_submodule_repo
From: Stefan Beller @ 2017-02-16  0:37 UTC (permalink / raw)
  Cc: git, sandals, jrnieder, bmwill, gitster, Stefan Beller
In-Reply-To: <20170216003811.18273-1-sbeller@google.com>

Redraw the ASCII art describing the setup using more space, such that
it is easier to understand.  The leaf commits are now ordered the same
way the actual code is ordered.

Add empty lines to the setup code separating each of the leaf commits,
each starting with a "checkout -b".

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/lib-submodule-update.sh | 49 ++++++++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 20 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 915eb4a7c6..5df528ea81 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -15,22 +15,27 @@
 # - Tracked file replaced by submodule (replace_sub1_with_file =>
 #   replace_file_with_sub1)
 #
-#                   --O-----O
-#                  /  ^     replace_directory_with_sub1
-#                 /   replace_sub1_with_directory
-#                /----O
-#               /     ^
-#              /      modify_sub1
-#      O------O-------O
-#      ^      ^\      ^
-#      |      | \     remove_sub1
-#      |      |  -----O-----O
-#      |      |   \   ^     replace_file_with_sub1
-#      |      |    \  replace_sub1_with_file
-#      |   add_sub1 --O-----O
-# no_submodule        ^     valid_sub1
-#                     invalid_sub1
+#                     ----O
+#                    /    ^
+#                   /     remove_sub1
+#                  /
+#       add_sub1  /-------O
+#             |  /        ^
+#             | /         modify_sub1
+#             v/
+#      O------O-----------O---------O
+#      ^       \          ^         replace_directory_with_sub1
+#      |        \         replace_sub1_with_directory
+# no_submodule   \
+#                 --------O---------O
+#                  \      ^         replace_file_with_sub1
+#                   \     replace_sub1_with_file
+#                    \
+#                     ----O---------O
+#                         ^         valid_sub1
+#                         invalid_sub1
 #
+
 create_lib_submodule_repo () {
 	git init submodule_update_repo &&
 	(
@@ -49,10 +54,11 @@ create_lib_submodule_repo () {
 		git config submodule.sub1.ignore all &&
 		git add .gitmodules &&
 		git commit -m "Add sub1" &&
-		git checkout -b remove_sub1 &&
+
+		git checkout -b remove_sub1 add_sub1 &&
 		git revert HEAD &&
 
-		git checkout -b "modify_sub1" "add_sub1" &&
+		git checkout -b modify_sub1 add_sub1 &&
 		git submodule update &&
 		(
 			cd sub1 &&
@@ -67,7 +73,7 @@ create_lib_submodule_repo () {
 		git add sub1 &&
 		git commit -m "Modify sub1" &&
 
-		git checkout -b "replace_sub1_with_directory" "add_sub1" &&
+		git checkout -b replace_sub1_with_directory add_sub1 &&
 		git submodule update &&
 		git -C sub1 checkout modifications &&
 		git rm --cached sub1 &&
@@ -75,22 +81,25 @@ create_lib_submodule_repo () {
 		git config -f .gitmodules --remove-section "submodule.sub1" &&
 		git add .gitmodules sub1/* &&
 		git commit -m "Replace sub1 with directory" &&
+
 		git checkout -b replace_directory_with_sub1 &&
 		git revert HEAD &&
 
-		git checkout -b "replace_sub1_with_file" "add_sub1" &&
+		git checkout -b replace_sub1_with_file add_sub1 &&
 		git rm sub1 &&
 		echo "content" >sub1 &&
 		git add sub1 &&
 		git commit -m "Replace sub1 with file" &&
+
 		git checkout -b replace_file_with_sub1 &&
 		git revert HEAD &&
 
-		git checkout -b "invalid_sub1" "add_sub1" &&
+		git checkout -b invalid_sub1 add_sub1 &&
 		git update-index --cacheinfo 160000 0123456789012345678901234567890123456789 sub1 &&
 		git commit -m "Invalid sub1 commit" &&
 		git checkout -b valid_sub1 &&
 		git revert HEAD &&
+
 		git checkout master
 	)
 }
-- 
2.12.0.rc1.16.ge4278d41a0.dirty


^ permalink raw reply related

* Re: [git-for-windows] Re: Continuous Testing of Git on Windows
From: Junio C Hamano @ 2017-02-16  1:33 UTC (permalink / raw)
  To: Philip Oakley; +Cc: Christian Couder, Johannes Schindelin, git-for-windows, git
In-Reply-To: <72ADDC07AFEF4BD88EADCDAF75DFB2CB@PhilipOakley>

"Philip Oakley" <philipoakley@iee.org> writes:

> For an integrator, or especially a CI tool, simply checking the second
> parents of each topic merge (post fail) should at least indicate if
> the basics of the feature actually passed the tests, though it doesn't
> check for interaction issues. This could give direct author feedback!

I think that is essentially what Avery advocated for in the old
thread Christian gave us earlier in the thread, and I agree that is
a useful way to help contributors.

While we are on the topic of testing efficiently and effectively,
there are a few other things worth taking advantage of.

One obvious thing is that not all topic branches get updated every
day, even though the tip of 'pu' will be different every day (or on
days when I run the integration cycle more than once, after every
pushout), because we constantly acquire new topics or updated ones.
The tips of topics that are merged in 'pu^{/^### match next}..pu'
can be enumerated by following the first-parent chain, and they can
be tested all one-by-one, excluding the ones that have been tested
already in the previous runs.  When I counted these merges the other
day I had 27 of them, and as of this writing I have 20 of them
(because some topics were merged to 'next' in the meantime).  Among
them, only 3 are new.  Everything else would have already been
tested if such a test was done daily.  So from that point of view,
testing them all could be less expensive than bisecting; in order to
bisect a first-parent chain of 20 merges, you'd need to test 5 or
so.

Another thing that may help to prevent breakages from seeping into
'next' is that "pu^{/^### match next}..pu" are rebuilt every day,
and the order of topics that are merged are updated.  The topics
that are closer to be merged to 'next' are moved down, so that
testing the merge result would give us closer result to what would
testing 'next' in the near future would give us [*1*].  So paying
closer attention to the merges (not tips of topics) above the commit
marked as "pu^{/^### match next}" when testing would catch potential
breakage about to happen in 'next' due to unexpected interactions of
topics when merged together.

There is another point in the history that may be worth paying
closer attention, which is the tip of 'jch'.  This always is ahead
of "pu^{/^### match next}", and almost always has a handful more
topics, many of which are considered to be merged to 'next' in near
future.  The branch is meant to be at least stable enough for my
personal use (hence its name) in helping me run everyday integration
cycles.


[Footnote]

*1* Suppose there are three topics A, B and C that are not yet in
    'next', and 'pu' merges them in that order.  Further suppose
    that C is a lot better cooked than others.  Merging C directly
    to 'next' however can expose a hidden issue that changes C
    introduces alone is not sufficient and depends on A or B to be
    present.  To avoid such a surprise after merging C to 'next',
    when 'pu' is rebuilt, I try to reorder them so that C is merged
    first on top of an equivalent of 'next', and then A and B.  For
    the same reason, topics in 'master..pu^{/^### match next}' are
    also reordered so that ones that are planned to be merged to
    'master' soon comes at the bottom.

    One natural consequence of this is that 'pu' is rebuilt directly
    on top of 'master' and 'next' does not fast-forwared to 'pu'.
    This arrangement also helps to spot mismerges to 'next' and
    avoid the same mismerge to affect 'master' when topics are
    merged to it.

^ permalink raw reply

* [PATCH 0/1] config.txt: Fix formatting of submodule.alternateErrorStrategy section
From: David Pursehouse @ 2017-02-16  5:05 UTC (permalink / raw)
  To: git; +Cc: David Pursehouse

From: David Pursehouse <dpursehouse@collab.net>

Fixes a minor glitch in the formatting of the documentation.

David Pursehouse (1):
  config.txt: Fix formatting of submodule.alternateErrorStrategy section

 Documentation/config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.11.1


^ permalink raw reply

* [PATCH 1/1] config.txt: Fix formatting of submodule.alternateErrorStrategy section
From: David Pursehouse @ 2017-02-16  5:05 UTC (permalink / raw)
  To: git; +Cc: David Pursehouse
In-Reply-To: <20170216050535.64593-1-david.pursehouse@gmail.com>

From: David Pursehouse <dpursehouse@collab.net>

Add missing `::` after the title.

Signed-off-by: David Pursehouse <dpursehouse@collab.net>
---
 Documentation/config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1fee83ca4..6c378c6fc 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2894,7 +2894,7 @@ submodule.alternateLocation::
 	value is set to `superproject` the submodule to be cloned computes
 	its alternates location relative to the superprojects alternate.
 
-submodule.alternateErrorStrategy
+submodule.alternateErrorStrategy::
 	Specifies how to treat errors with the alternates for a submodule
 	as computed via `submodule.alternateLocation`. Possible values are
 	`ignore`, `info`, `die`. Default is `die`.
-- 
2.11.1


^ permalink raw reply related

* Re: Confusing git messages when disk is full.
From: Andreas Schwab @ 2017-02-16 10:10 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Jáchym Barvínek, git
In-Reply-To: <20170215231832.bzg3ygz4ualcvqlc@sigill.intra.peff.net>

On Feb 15 2017, Jeff King <peff@peff.net> wrote:

> On Wed, Feb 15, 2017 at 02:50:19PM -0800, Junio C Hamano wrote:
>
>> > That works, but the fact that we need a comment is a good sign that it's
>> > kind of gross. It's too bad stdio does not specify the return of fclose
>> > to report an error in the close _or_ any previous error. I guess we
>> > could wrap it with our own function.
>> 
>> Sure.  I am happy to add something like this:
>> 
>> 	/*
>> 	 * closes a FILE *, returns 0 if closing and all the
>> 	 * previous stdio operations on fp were successful,
>> 	 * otherwise non-zero.
>> 	 */
>> 	int xfclose(FILE *fp)
>> 	{
>> 		return ferror(fp) | fclose(fp);
>> 	}
>
> Yes, that's exactly what I had in mind (might be worth calling out the
> bitwise-OR, though, just to make it clear it's not a typo).

Since the order of evaluation is unspecified, it would be better to
force sequencing ferror before fclose.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply

* Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line
From: Lars Schneider @ 2017-02-16 10:30 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jonathan Tan, git, sbeller
In-Reply-To: <xmqqa89n10df.fsf_-_@gitster.mtv.corp.google.com>


> On 16 Feb 2017, at 00:48, Junio C Hamano <gitster@pobox.com> wrote:
> 
> The "git -c <var>=<val> cmd" mechanism is to pretend that a

The problem is also present for gitconfig variables e.g.
git config --local submodule.UPPERSUB.update none

But your patch below fixes that, too!


> configuration variable <var> is set to <val> while the cmd is
> running.  The code to do so however downcased <var> in its entirety,
> which is wrong for a three-level <section>.<subsection>.<variable>.
> 
> The <subsection> part needs to stay as-is.
> 
> Reported-by: Lars Schneider <larsxschneider@gmail.com>
> Diagnosed-by: Jonathan Tan <jonathantanmy@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> config.c | 22 +++++++++++++++++++++-
> 1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/config.c b/config.c
> index 0dfed682b8..e9b93b5304 100644
> --- a/config.c
> +++ b/config.c
> @@ -199,6 +199,26 @@ void git_config_push_parameter(const char *text)
> 	strbuf_release(&env);
> }
> 
> +/*
> + * downcase the <section> and <variable> in <section>.<variable> or
> + * <section>.<subsection>.<variable> and do so in place.  <subsection>
> + * is left intact.
> + */
> +static void canonicalize_config_variable_name(char *varname)
> +{
> +	char *dot, *cp;
> +
> +	dot = strchr(varname, '.');
minor nit:
I think it would improve readability if we call this "first_dot" ...


> +	if (dot)
> +		for (cp = varname; cp < dot; cp++)
> +			*cp = tolower(*cp);
> +	dot = strrchr(varname, '.');
... and this "last_dot"?


> +	if (dot)
> +		for (cp = dot + 1; *cp; cp++)
> +			*cp = tolower(*cp);
> +}
> +
> +
> int git_config_parse_parameter(const char *text,
> 			       config_fn_t fn, void *data)
> {
> @@ -221,7 +241,7 @@ int git_config_parse_parameter(const char *text,
> 		strbuf_list_free(pair);
> 		return error("bogus config parameter: %s", text);
> 	}
> -	strbuf_tolower(pair[0]);
> +	canonicalize_config_variable_name(pair[0]->buf);
> 	if (fn(pair[0]->buf, value, data) < 0) {
> 		strbuf_list_free(pair);
> 		return -1;
> -- 
> 2.12.0-rc1-258-g3d3d1e383b
> 


The patch looks good to me and fixes the problem!

Should we add a test case to this patch?
If not, do you want me to improve my test case patch [1] 
or do you want to ditch the test?

Thank you,
Lars


[1] http://public-inbox.org/git/20170215111704.78320-1-larsxschneider@gmail.com/

^ permalink raw reply

* [PATCH 1/3] add git_psprintf helper function
From: Maxim Moseychuk @ 2017-02-16 11:28 UTC (permalink / raw)
  To: git; +Cc: peff, Maxim Moseychuk
In-Reply-To: <20170216112829.18079-1-franchesko.salias.hudro.pedros@gmail.com>

There are a number of places in the code where we call
xsnprintf(), with the assumption that the output will fit into
the buffer. If the buffer is small, then git die.
In many places buffers have compile-time size, but generated string
depends from current system locale (gettext)and can have size
greater the buffer.
Just run "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8"
on linux sources - it impossible.

git_psprintf is similar to the standard C sprintf() function
but safer, since it calculates the maximum space required
and allocates memory to hold the result.
The returned string should be freed with free() when no longer needed.

Signed-off-by: Maxim Moseychuk <franchesko.salias.hudro.pedros@gmail.com>
---
 git-compat-util.h |  3 +++
 wrapper.c         | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 87237b092..fa98705d0 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -878,6 +878,9 @@ static inline size_t xsize_t(off_t len)
 	return (size_t)len;
 }
 
+__attribute__((format (printf, 1, 2)))
+extern char *git_psprintf(const char *fmt, ...);
+
 __attribute__((format (printf, 3, 4)))
 extern int xsnprintf(char *dst, size_t max, const char *fmt, ...);
 
diff --git a/wrapper.c b/wrapper.c
index e7f197996..deee46d2d 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -635,6 +635,25 @@ char *xgetcwd(void)
 	return strbuf_detach(&sb, NULL);
 }
 
+char *git_psprintf(const char *fmt, ...)
+{
+	va_list ap;
+	int len;
+	char *dst;
+
+	va_start(ap, fmt);
+	len = vsnprintf(NULL, 0, fmt, ap);
+	va_end(ap);
+
+	dst = xmallocz(len);
+
+	va_start(ap, fmt);
+	vsprintf(dst, fmt, ap);
+	va_end(ap);
+
+	return dst;
+}
+
 int xsnprintf(char *dst, size_t max, const char *fmt, ...)
 {
 	va_list ap;
-- 
2.11.1


^ permalink raw reply related

* [PATCH 3/3] stop_progress_msg: convert xsnprintf to git_psprintf
From: Maxim Moseychuk @ 2017-02-16 11:28 UTC (permalink / raw)
  To: git; +Cc: peff, Maxim Moseychuk
In-Reply-To: <20170216112829.18079-1-franchesko.salias.hudro.pedros@gmail.com>

Replace local safe string buffer allocation by git_psprintf.

Signed-off-by: Maxim Moseychuk <franchesko.salias.hudro.pedros@gmail.com>
---
 progress.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/progress.c b/progress.c
index 76a88c573..989d65504 100644
--- a/progress.c
+++ b/progress.c
@@ -243,21 +243,18 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
 	*p_progress = NULL;
 	if (progress->last_value != -1) {
 		/* Force the last update */
-		char buf[128], *bufp;
-		size_t len = strlen(msg) + 5;
+		char *bufp;
 		struct throughput *tp = progress->throughput;
 
-		bufp = (len < sizeof(buf)) ? buf : xmallocz(len);
 		if (tp) {
 			unsigned int rate = !tp->avg_misecs ? 0 :
 					tp->avg_bytes / tp->avg_misecs;
 			throughput_string(&tp->display, tp->curr_total, rate);
 		}
 		progress_update = 1;
-		xsnprintf(bufp, len + 1, ", %s.\n", msg);
+		bufp = git_psprintf(", %s.\n", msg);
 		display(progress, progress->last_value, bufp);
-		if (buf != bufp)
-			free(bufp);
+		free(bufp);
 	}
 	clear_progress_signal();
 	if (progress->throughput)
-- 
2.11.1


^ permalink raw reply related

* [PATCH 2/3] bisect_next_all: convert xsnprintf to git_psprintf
From: Maxim Moseychuk @ 2017-02-16 11:28 UTC (permalink / raw)
  To: git; +Cc: peff, Maxim Moseychuk
In-Reply-To: <20170216112829.18079-1-franchesko.salias.hudro.pedros@gmail.com>

Git can't run bisect between 2048+ commits if use russian translation.
Reproduce "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8" on linux sources.

Dummy solution: just increase buffer size but is not safe.
Size gettext string is a runtime value.

Signed-off-by: Maxim Moseychuk <franchesko.salias.hudro.pedros@gmail.com>
---
 bisect.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/bisect.c b/bisect.c
index 21bc6daa4..8670cc97a 100644
--- a/bisect.c
+++ b/bisect.c
@@ -940,7 +940,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
 	struct commit_list *tried;
 	int reaches = 0, all = 0, nr, steps;
 	const unsigned char *bisect_rev;
-	char steps_msg[32];
+	char *steps_msg;
 
 	read_bisect_terms(&term_bad, &term_good);
 	if (read_bisect_refs())
@@ -990,14 +990,15 @@ int bisect_next_all(const char *prefix, int no_checkout)
 
 	nr = all - reaches - 1;
 	steps = estimate_bisect_steps(all);
-	xsnprintf(steps_msg, sizeof(steps_msg),
-		  Q_("(roughly %d step)", "(roughly %d steps)", steps),
-		  steps);
+
+	steps_msg = git_psprintf(Q_("(roughly %d step)", "(roughly %d steps)",
+		  steps), steps);
 	/* TRANSLATORS: the last %s will be replaced with
 	   "(roughly %d steps)" translation */
 	printf(Q_("Bisecting: %d revision left to test after this %s\n",
 		  "Bisecting: %d revisions left to test after this %s\n",
 		  nr), nr, steps_msg);
+	free(steps_msg);
 
 	return bisect_checkout(bisect_rev, no_checkout);
 }
-- 
2.11.1


^ permalink raw reply related

* [PATCH 0/3] Fix l10n
From: Maxim Moseychuk @ 2017-02-16 11:28 UTC (permalink / raw)
  To: git; +Cc: peff, Maxim Moseychuk

In some places static size buffers can't store formatted string.
If it be happen then git die.

Maxim Moseychuk (3):
  add git_psprintf helper function
  bisect_next_all: convert xsnprintf to git_psprintf
  stop_progress_msg: convert xsnprintf to git_psprintf

 bisect.c          |  9 +++++----
 git-compat-util.h |  3 +++
 progress.c        |  9 +++------
 wrapper.c         | 19 +++++++++++++++++++
 4 files changed, 30 insertions(+), 10 deletions(-)

-- 
2.11.1


^ permalink raw reply

* [PATCH v2 00/16] Remove submodule from files-backend.c
From: Nguyễn Thái Ngọc Duy @ 2017-02-16 11:48 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Michael Haggerty, Johannes Schindelin,
	Ramsay Jones, Stefan Beller, novalis,
	Nguyễn Thái Ngọc Duy
In-Reply-To: <20170213152011.12050-1-pclouds@gmail.com>

[-- Attachment #1: Type: plain, Size: 3039 bytes --]

^ permalink raw reply

* [PATCH v2 01/16] refs-internal.c: make files_log_ref_write() static
From: Nguyễn Thái Ngọc Duy @ 2017-02-16 11:48 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Michael Haggerty, Johannes Schindelin,
	Ramsay Jones, Stefan Beller, novalis,
	Nguyễn Thái Ngọc Duy
In-Reply-To: <20170216114818.6080-1-pclouds@gmail.com>

Created in 5f3c3a4e6f (files_log_ref_write: new function - 2015-11-10)
but probably never used outside refs-internal.c

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs/files-backend.c | 3 +++
 refs/refs-internal.h | 4 ----
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index cdb6b8ff5..75565c3aa 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -165,6 +165,9 @@ static struct ref_entry *create_dir_entry(struct files_ref_store *ref_store,
 					  const char *dirname, size_t len,
 					  int incomplete);
 static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry);
+static int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
+			       const unsigned char *new_sha1, const char *msg,
+			       int flags, struct strbuf *err);
 
 static struct ref_dir *get_ref_dir(struct ref_entry *entry)
 {
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 33adbf93b..59e65958a 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -222,10 +222,6 @@ struct ref_transaction {
 	enum ref_transaction_state state;
 };
 
-int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
-			const unsigned char *new_sha1, const char *msg,
-			int flags, struct strbuf *err);
-
 /*
  * Check for entries in extras that are within the specified
  * directory, where dirname is a reference directory name including
-- 
2.11.0.157.gd943d85


^ permalink raw reply related

* [PATCH v2 02/16] files-backend: convert git_path() to strbuf_git_path()
From: Nguyễn Thái Ngọc Duy @ 2017-02-16 11:48 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Michael Haggerty, Johannes Schindelin,
	Ramsay Jones, Stefan Beller, novalis,
	Nguyễn Thái Ngọc Duy
In-Reply-To: <20170216114818.6080-1-pclouds@gmail.com>

git_path() and friends are going to be killed in files-backend.c in near
future. And because there's a risk with overwriting buffer in
git_path(), let's convert them all to strbuf_git_path(). We'll have
easier time killing/converting strbuf_git_path() then because we won't
have to worry about memory management again.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs/files-backend.c | 111 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 89 insertions(+), 22 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 75565c3aa..f0c878b92 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2169,6 +2169,8 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
 	static int timeout_configured = 0;
 	static int timeout_value = 1000;
 	struct packed_ref_cache *packed_ref_cache;
+	struct strbuf sb = STRBUF_INIT;
+	int ret;
 
 	files_assert_main_repository(refs, "lock_packed_refs");
 
@@ -2177,10 +2179,13 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
 		timeout_configured = 1;
 	}
 
-	if (hold_lock_file_for_update_timeout(
-			    &packlock, git_path("packed-refs"),
-			    flags, timeout_value) < 0)
+	strbuf_git_path(&sb, "packed-refs");
+	ret = hold_lock_file_for_update_timeout(&packlock, sb.buf,
+						flags, timeout_value);
+	strbuf_release(&sb);
+	if (ret < 0)
 		return -1;
+
 	/*
 	 * Get the current packed-refs while holding the lock.  If the
 	 * packed-refs file has been modified since we last read it,
@@ -2335,6 +2340,9 @@ static void try_remove_empty_parents(char *name)
 	for (q = p; *q; q++)
 		;
 	while (1) {
+		struct strbuf sb = STRBUF_INIT;
+		int ret;
+
 		while (q > p && *q != '/')
 			q--;
 		while (q > p && *(q-1) == '/')
@@ -2342,7 +2350,10 @@ static void try_remove_empty_parents(char *name)
 		if (q == p)
 			break;
 		*q = '\0';
-		if (rmdir(git_path("%s", name)))
+		strbuf_git_path(&sb, "%s", name);
+		ret = rmdir(sb.buf);
+		strbuf_release(&sb);
+		if (ret)
 			break;
 	}
 }
@@ -2431,7 +2442,11 @@ static int repack_without_refs(struct files_ref_store *refs,
 		return 0; /* no refname exists in packed refs */
 
 	if (lock_packed_refs(refs, 0)) {
-		unable_to_lock_message(git_path("packed-refs"), errno, err);
+		struct strbuf sb = STRBUF_INIT;
+
+		strbuf_git_path(&sb, "packed-refs");
+		unable_to_lock_message(sb.buf, errno, err);
+		strbuf_release(&sb);
 		return -1;
 	}
 	packed = get_packed_refs(refs);
@@ -2529,8 +2544,10 @@ static int rename_tmp_log(const char *newrefname)
 {
 	int attempts_remaining = 4;
 	struct strbuf path = STRBUF_INIT;
+	struct strbuf tmp_renamed_log = STRBUF_INIT;
 	int ret = -1;
 
+	strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
  retry:
 	strbuf_reset(&path);
 	strbuf_git_path(&path, "logs/%s", newrefname);
@@ -2546,7 +2563,7 @@ static int rename_tmp_log(const char *newrefname)
 		goto out;
 	}
 
-	if (rename(git_path(TMP_RENAMED_LOG), path.buf)) {
+	if (rename(tmp_renamed_log.buf, path.buf)) {
 		if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 0) {
 			/*
 			 * rename(a, b) when b is an existing
@@ -2574,6 +2591,7 @@ static int rename_tmp_log(const char *newrefname)
 	ret = 0;
 out:
 	strbuf_release(&path);
+	strbuf_release(&tmp_renamed_log);
 	return ret;
 }
 
@@ -2614,9 +2632,15 @@ static int files_rename_ref(struct ref_store *ref_store,
 	int flag = 0, logmoved = 0;
 	struct ref_lock *lock;
 	struct stat loginfo;
-	int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
+	struct strbuf sb_oldref = STRBUF_INIT;
+	struct strbuf sb_newref = STRBUF_INIT;
+	struct strbuf tmp_renamed_log = STRBUF_INIT;
+	int log, ret;
 	struct strbuf err = STRBUF_INIT;
 
+	strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
+	log = !lstat(sb_oldref.buf, &loginfo);
+	strbuf_release(&sb_oldref);
 	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldrefname);
 
@@ -2630,7 +2654,12 @@ static int files_rename_ref(struct ref_store *ref_store,
 	if (!rename_ref_available(oldrefname, newrefname))
 		return 1;
 
-	if (log && rename(git_path("logs/%s", oldrefname), git_path(TMP_RENAMED_LOG)))
+	strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
+	strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
+	ret = log && rename(sb_oldref.buf, tmp_renamed_log.buf);
+	strbuf_release(&sb_oldref);
+	strbuf_release(&tmp_renamed_log);
+	if (ret)
 		return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
 			oldrefname, strerror(errno));
 
@@ -2709,13 +2738,19 @@ static int files_rename_ref(struct ref_store *ref_store,
 	log_all_ref_updates = flag;
 
  rollbacklog:
-	if (logmoved && rename(git_path("logs/%s", newrefname), git_path("logs/%s", oldrefname)))
+	strbuf_git_path(&sb_newref, "logs/%s", newrefname);
+	strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
+	if (logmoved && rename(sb_newref.buf, sb_oldref.buf))
 		error("unable to restore logfile %s from %s: %s",
 			oldrefname, newrefname, strerror(errno));
+	strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
 	if (!logmoved && log &&
-	    rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", oldrefname)))
+	    rename(tmp_renamed_log.buf, sb_oldref.buf))
 		error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
 			oldrefname, strerror(errno));
+	strbuf_release(&sb_newref);
+	strbuf_release(&sb_oldref);
+	strbuf_release(&tmp_renamed_log);
 
 	return 1;
 }
@@ -3111,22 +3146,32 @@ int set_worktree_head_symref(const char *gitdir, const char *target)
 static int files_reflog_exists(struct ref_store *ref_store,
 			       const char *refname)
 {
+	struct strbuf sb = STRBUF_INIT;
 	struct stat st;
+	int ret;
 
 	/* Check validity (but we don't need the result): */
 	files_downcast(ref_store, 0, "reflog_exists");
 
-	return !lstat(git_path("logs/%s", refname), &st) &&
-		S_ISREG(st.st_mode);
+	strbuf_git_path(&sb, "logs/%s", refname);
+	ret = !lstat(sb.buf, &st) && S_ISREG(st.st_mode);
+	strbuf_release(&sb);
+	return ret;
 }
 
 static int files_delete_reflog(struct ref_store *ref_store,
 			       const char *refname)
 {
+	struct strbuf sb = STRBUF_INIT;
+	int ret;
+
 	/* Check validity (but we don't need the result): */
 	files_downcast(ref_store, 0, "delete_reflog");
 
-	return remove_path(git_path("logs/%s", refname));
+	strbuf_git_path(&sb, "logs/%s", refname);
+	ret = remove_path(sb.buf);
+	strbuf_release(&sb);
+	return ret;
 }
 
 static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *cb_data)
@@ -3181,7 +3226,9 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
 	/* Check validity (but we don't need the result): */
 	files_downcast(ref_store, 0, "for_each_reflog_ent_reverse");
 
-	logfp = fopen(git_path("logs/%s", refname), "r");
+	strbuf_git_path(&sb, "logs/%s", refname);
+	logfp = fopen(sb.buf, "r");
+	strbuf_release(&sb);
 	if (!logfp)
 		return -1;
 
@@ -3287,7 +3334,9 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,
 	/* Check validity (but we don't need the result): */
 	files_downcast(ref_store, 0, "for_each_reflog_ent");
 
-	logfp = fopen(git_path("logs/%s", refname), "r");
+	strbuf_git_path(&sb, "logs/%s", refname);
+	logfp = fopen(sb.buf, "r");
+	strbuf_release(&sb);
 	if (!logfp)
 		return -1;
 
@@ -3369,12 +3418,15 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st
 {
 	struct files_reflog_iterator *iter = xcalloc(1, sizeof(*iter));
 	struct ref_iterator *ref_iterator = &iter->base;
+	struct strbuf sb = STRBUF_INIT;
 
 	/* Check validity (but we don't need the result): */
 	files_downcast(ref_store, 0, "reflog_iterator_begin");
 
 	base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);
-	iter->dir_iterator = dir_iterator_begin(git_path("logs"));
+	strbuf_git_path(&sb, "logs");
+	iter->dir_iterator = dir_iterator_begin(sb.buf);
+	strbuf_release(&sb);
 	return ref_iterator;
 }
 
@@ -3843,8 +3895,13 @@ static int files_transaction_commit(struct ref_store *ref_store,
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}
-	for_each_string_list_item(ref_to_delete, &refs_to_delete)
-		unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
+	for_each_string_list_item(ref_to_delete, &refs_to_delete) {
+		struct strbuf sb = STRBUF_INIT;
+
+		strbuf_git_path(&sb, "logs/%s", ref_to_delete->string);
+		unlink_or_warn(sb.buf);
+		strbuf_release(&sb);
+	}
 	clear_loose_ref_cache(refs);
 
 cleanup:
@@ -4098,18 +4155,28 @@ static int files_reflog_expire(struct ref_store *ref_store,
 
 static int files_init_db(struct ref_store *ref_store, struct strbuf *err)
 {
+	struct strbuf sb = STRBUF_INIT;
+
 	/* Check validity (but we don't need the result): */
 	files_downcast(ref_store, 0, "init_db");
 
 	/*
 	 * Create .git/refs/{heads,tags}
 	 */
-	safe_create_dir(git_path("refs/heads"), 1);
-	safe_create_dir(git_path("refs/tags"), 1);
+	strbuf_git_path(&sb, "refs/heads");
+	safe_create_dir(sb.buf, 1);
+	strbuf_reset(&sb);
+	strbuf_git_path(&sb, "refs/tags");
+	safe_create_dir(sb.buf, 1);
+	strbuf_reset(&sb);
 	if (get_shared_repository()) {
-		adjust_shared_perm(git_path("refs/heads"));
-		adjust_shared_perm(git_path("refs/tags"));
+		strbuf_git_path(&sb, "refs/heads");
+		adjust_shared_perm(sb.buf);
+		strbuf_reset(&sb);
+		strbuf_git_path(&sb, "refs/tags");
+		adjust_shared_perm(sb.buf);
 	}
+	strbuf_release(&sb);
 	return 0;
 }
 
-- 
2.11.0.157.gd943d85


^ permalink raw reply related

* [PATCH v2 05/16] refs.c: share is_per_worktree_ref() to files-backend.c
From: Nguyễn Thái Ngọc Duy @ 2017-02-16 11:48 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Michael Haggerty, Johannes Schindelin,
	Ramsay Jones, Stefan Beller, novalis,
	Nguyễn Thái Ngọc Duy
In-Reply-To: <20170216114818.6080-1-pclouds@gmail.com>

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs.c               | 6 ------
 refs/refs-internal.h | 6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index 4f845798b..7a474198e 100644
--- a/refs.c
+++ b/refs.c
@@ -489,12 +489,6 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 	return logs_found;
 }
 
-static int is_per_worktree_ref(const char *refname)
-{
-	return !strcmp(refname, "HEAD") ||
-		starts_with(refname, "refs/bisect/");
-}
-
 static int is_pseudoref_syntax(const char *refname)
 {
 	const char *c;
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 59e65958a..f4aed49f5 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -651,4 +651,10 @@ const char *resolve_ref_recursively(struct ref_store *refs,
 				    int resolve_flags,
 				    unsigned char *sha1, int *flags);
 
+static inline int is_per_worktree_ref(const char *refname)
+{
+	return !strcmp(refname, "HEAD") ||
+		starts_with(refname, "refs/bisect/");
+}
+
 #endif /* REFS_REFS_INTERNAL_H */
-- 
2.11.0.157.gd943d85


^ permalink raw reply related

* [PATCH v2 06/16] refs-internal.h: correct is_per_worktree_ref()
From: Nguyễn Thái Ngọc Duy @ 2017-02-16 11:48 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Michael Haggerty, Johannes Schindelin,
	Ramsay Jones, Stefan Beller, novalis,
	Nguyễn Thái Ngọc Duy
In-Reply-To: <20170216114818.6080-1-pclouds@gmail.com>

All refs outside refs/ directory is per-worktree, not just HEAD.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs/refs-internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index f4aed49f5..69d02b6ba 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -653,7 +653,7 @@ const char *resolve_ref_recursively(struct ref_store *refs,
 
 static inline int is_per_worktree_ref(const char *refname)
 {
-	return !strcmp(refname, "HEAD") ||
+	return !starts_with(refname, "refs/") ||
 		starts_with(refname, "refs/bisect/");
 }
 
-- 
2.11.0.157.gd943d85


^ permalink raw reply related

* [PATCH v2 03/16] files-backend: add files_path()
From: Nguyễn Thái Ngọc Duy @ 2017-02-16 11:48 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Michael Haggerty, Johannes Schindelin,
	Ramsay Jones, Stefan Beller, novalis,
	Nguyễn Thái Ngọc Duy
In-Reply-To: <20170216114818.6080-1-pclouds@gmail.com>

This will be the replacement for both git_path() and git_path_submodule()
in this file. The idea is backend takes a git path and use that,
oblivious of submodule, linked worktrees and such.

This is the middle step towards that. Eventually the "submodule" field
in 'struct files_ref_store' should be replace by "gitdir". And a
compound ref_store is created to combine two files backends together,
one represents the shared refs in $GIT_COMMON_DIR, one per-worktree. At
that point, files_path() becomes a wrapper of strbuf_vaddf().

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs/files-backend.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index f0c878b92..abb8a95e0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -930,6 +930,24 @@ struct files_ref_store {
 /* Lock used for the main packed-refs file: */
 static struct lock_file packlock;
 
+__attribute__((format (printf, 3, 4)))
+static void files_path(struct files_ref_store *refs, struct strbuf *sb,
+		       const char *fmt, ...)
+{
+	struct strbuf tmp = STRBUF_INIT;
+	va_list vap;
+
+	va_start(vap, fmt);
+	strbuf_vaddf(&tmp, fmt, vap);
+	va_end(vap);
+	if (refs->submodule)
+		strbuf_git_path_submodule(sb, refs->submodule,
+					  "%s", tmp.buf);
+	else
+		strbuf_git_path(sb, "%s", tmp.buf);
+	strbuf_release(&tmp);
+}
+
 /*
  * Increment the reference count of *packed_refs.
  */
-- 
2.11.0.157.gd943d85


^ permalink raw reply related

* [PATCH v2 04/16] files-backend: replace *git_path*() with files_path()
From: Nguyễn Thái Ngọc Duy @ 2017-02-16 11:48 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Michael Haggerty, Johannes Schindelin,
	Ramsay Jones, Stefan Beller, novalis,
	Nguyễn Thái Ngọc Duy
In-Reply-To: <20170216114818.6080-1-pclouds@gmail.com>

This centralizes all path rewriting of files-backend.c in one place so
we have easier time removing the path rewriting later. There could be
some hidden indirect git_path() though, I didn't audit the code to the
bottom.

Side note: set_worktree_head_symref() is a bad boy and should not be in
files-backend.c (probably should not exist in the first place). But
we'll leave it there until we have better multi-worktree support in refs
before we update it.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs/files-backend.c | 185 ++++++++++++++++++++++++++-------------------------
 1 file changed, 94 insertions(+), 91 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index abb8a95e0..24f5bf7f1 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -165,7 +165,8 @@ static struct ref_entry *create_dir_entry(struct files_ref_store *ref_store,
 					  const char *dirname, size_t len,
 					  int incomplete);
 static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry);
-static int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
+static int files_log_ref_write(struct files_ref_store *refs,
+			       const char *refname, const unsigned char *old_sha1,
 			       const unsigned char *new_sha1, const char *msg,
 			       int flags, struct strbuf *err);
 
@@ -1178,12 +1179,10 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
 static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store *refs)
 {
 	char *packed_refs_file;
+	struct strbuf sb = STRBUF_INIT;
 
-	if (refs->submodule)
-		packed_refs_file = git_pathdup_submodule(refs->submodule,
-							 "packed-refs");
-	else
-		packed_refs_file = git_pathdup("packed-refs");
+	files_path(refs, &sb, "packed-refs");
+	packed_refs_file = strbuf_detach(&sb, NULL);
 
 	if (refs->packed &&
 	    !stat_validity_check(&refs->packed->validity, packed_refs_file))
@@ -1249,10 +1248,7 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
 	size_t path_baselen;
 	int err = 0;
 
-	if (refs->submodule)
-		err = strbuf_git_path_submodule(&path, refs->submodule, "%s", dirname);
-	else
-		strbuf_git_path(&path, "%s", dirname);
+	files_path(refs, &path, "%s", dirname);
 	path_baselen = path.len;
 
 	if (err) {
@@ -1394,10 +1390,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
 	*type = 0;
 	strbuf_reset(&sb_path);
 
-	if (refs->submodule)
-		strbuf_git_path_submodule(&sb_path, refs->submodule, "%s", refname);
-	else
-		strbuf_git_path(&sb_path, "%s", refname);
+	files_path(refs, &sb_path, "%s", refname);
 
 	path = sb_path.buf;
 
@@ -1585,7 +1578,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
 	*lock_p = lock = xcalloc(1, sizeof(*lock));
 
 	lock->ref_name = xstrdup(refname);
-	strbuf_git_path(&ref_file, "%s", refname);
+	files_path(refs, &ref_file, "%s", refname);
 
 retry:
 	switch (safe_create_leading_directories(ref_file.buf)) {
@@ -2052,7 +2045,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,
 	if (flags & REF_DELETING)
 		resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
 
-	strbuf_git_path(&ref_file, "%s", refname);
+	files_path(refs, &ref_file, "%s", refname);
 	resolved = !!resolve_ref_unsafe(refname, resolve_flags,
 					lock->old_oid.hash, type);
 	if (!resolved && errno == EISDIR) {
@@ -2197,7 +2190,7 @@ static int lock_packed_refs(struct files_ref_store *refs, int flags)
 		timeout_configured = 1;
 	}
 
-	strbuf_git_path(&sb, "packed-refs");
+	files_path(refs, &sb, "packed-refs");
 	ret = hold_lock_file_for_update_timeout(&packlock, sb.buf,
 						flags, timeout_value);
 	strbuf_release(&sb);
@@ -2343,7 +2336,7 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)
  * Remove empty parents, but spare refs/ and immediate subdirs.
  * Note: munges *name.
  */
-static void try_remove_empty_parents(char *name)
+static void try_remove_empty_parents(struct files_ref_store *refs, char *name)
 {
 	char *p, *q;
 	int i;
@@ -2368,7 +2361,7 @@ static void try_remove_empty_parents(char *name)
 		if (q == p)
 			break;
 		*q = '\0';
-		strbuf_git_path(&sb, "%s", name);
+		files_path(refs, &sb, "%s", name);
 		ret = rmdir(sb.buf);
 		strbuf_release(&sb);
 		if (ret)
@@ -2377,7 +2370,7 @@ static void try_remove_empty_parents(char *name)
 }
 
 /* make sure nobody touched the ref, and unlink */
-static void prune_ref(struct ref_to_prune *r)
+static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)
 {
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
@@ -2397,13 +2390,13 @@ static void prune_ref(struct ref_to_prune *r)
 	}
 	ref_transaction_free(transaction);
 	strbuf_release(&err);
-	try_remove_empty_parents(r->name);
+	try_remove_empty_parents(refs, r->name);
 }
 
-static void prune_refs(struct ref_to_prune *r)
+static void prune_refs(struct files_ref_store *refs, struct ref_to_prune *r)
 {
 	while (r) {
-		prune_ref(r);
+		prune_ref(refs, r);
 		r = r->next;
 	}
 }
@@ -2426,7 +2419,7 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 	if (commit_packed_refs(refs))
 		die_errno("unable to overwrite old ref-pack file");
 
-	prune_refs(cbdata.ref_to_prune);
+	prune_refs(refs, cbdata.ref_to_prune);
 	return 0;
 }
 
@@ -2462,7 +2455,7 @@ static int repack_without_refs(struct files_ref_store *refs,
 	if (lock_packed_refs(refs, 0)) {
 		struct strbuf sb = STRBUF_INIT;
 
-		strbuf_git_path(&sb, "packed-refs");
+		files_path(refs, &sb, "packed-refs");
 		unable_to_lock_message(sb.buf, errno, err);
 		strbuf_release(&sb);
 		return -1;
@@ -2558,17 +2551,17 @@ static int files_delete_refs(struct ref_store *ref_store,
  */
 #define TMP_RENAMED_LOG  "logs/refs/.tmp-renamed-log"
 
-static int rename_tmp_log(const char *newrefname)
+static int rename_tmp_log(struct files_ref_store *refs, const char *newrefname)
 {
 	int attempts_remaining = 4;
 	struct strbuf path = STRBUF_INIT;
 	struct strbuf tmp_renamed_log = STRBUF_INIT;
 	int ret = -1;
 
-	strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
+	files_path(refs, &tmp_renamed_log, TMP_RENAMED_LOG);
  retry:
 	strbuf_reset(&path);
-	strbuf_git_path(&path, "logs/%s", newrefname);
+	files_path(refs, &path, "logs/%s", newrefname);
 	switch (safe_create_leading_directories_const(path.buf)) {
 	case SCLD_OK:
 		break; /* success */
@@ -2656,7 +2649,7 @@ static int files_rename_ref(struct ref_store *ref_store,
 	int log, ret;
 	struct strbuf err = STRBUF_INIT;
 
-	strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
+	files_path(refs, &sb_oldref, "logs/%s", oldrefname);
 	log = !lstat(sb_oldref.buf, &loginfo);
 	strbuf_release(&sb_oldref);
 	if (log && S_ISLNK(loginfo.st_mode))
@@ -2672,8 +2665,8 @@ static int files_rename_ref(struct ref_store *ref_store,
 	if (!rename_ref_available(oldrefname, newrefname))
 		return 1;
 
-	strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
-	strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
+	files_path(refs, &sb_oldref, "logs/%s", oldrefname);
+	files_path(refs, &tmp_renamed_log, TMP_RENAMED_LOG);
 	ret = log && rename(sb_oldref.buf, tmp_renamed_log.buf);
 	strbuf_release(&sb_oldref);
 	strbuf_release(&tmp_renamed_log);
@@ -2700,7 +2693,7 @@ static int files_rename_ref(struct ref_store *ref_store,
 			struct strbuf path = STRBUF_INIT;
 			int result;
 
-			strbuf_git_path(&path, "%s", newrefname);
+			files_path(refs, &path, "%s", newrefname);
 			result = remove_empty_directories(&path);
 			strbuf_release(&path);
 
@@ -2714,7 +2707,7 @@ static int files_rename_ref(struct ref_store *ref_store,
 		}
 	}
 
-	if (log && rename_tmp_log(newrefname))
+	if (log && rename_tmp_log(refs, newrefname))
 		goto rollback;
 
 	logmoved = log;
@@ -2756,12 +2749,12 @@ static int files_rename_ref(struct ref_store *ref_store,
 	log_all_ref_updates = flag;
 
  rollbacklog:
-	strbuf_git_path(&sb_newref, "logs/%s", newrefname);
-	strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
+	files_path(refs, &sb_newref, "logs/%s", newrefname);
+	files_path(refs, &sb_oldref, "logs/%s", oldrefname);
 	if (logmoved && rename(sb_newref.buf, sb_oldref.buf))
 		error("unable to restore logfile %s from %s: %s",
 			oldrefname, newrefname, strerror(errno));
-	strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
+	files_path(refs, &tmp_renamed_log, TMP_RENAMED_LOG);
 	if (!logmoved && log &&
 	    rename(tmp_renamed_log.buf, sb_oldref.buf))
 		error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
@@ -2817,11 +2810,13 @@ static int commit_ref(struct ref_lock *lock)
  * should_autocreate_reflog returns non-zero.  Otherwise, create it
  * regardless of the ref name.  Fill in *err and return -1 on failure.
  */
-static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)
+static int log_ref_setup(struct files_ref_store *refs, const char *refname,
+			 struct strbuf *logfile, struct strbuf *err,
+			 int force_create)
 {
 	int logfd, oflags = O_APPEND | O_WRONLY;
 
-	strbuf_git_path(logfile, "logs/%s", refname);
+	files_path(refs, logfile, "logs/%s", refname);
 	if (force_create || should_autocreate_reflog(refname)) {
 		if (safe_create_leading_directories(logfile->buf) < 0) {
 			strbuf_addf(err, "unable to create directory for '%s': "
@@ -2864,11 +2859,10 @@ static int files_create_reflog(struct ref_store *ref_store,
 {
 	int ret;
 	struct strbuf sb = STRBUF_INIT;
+	struct files_ref_store *refs =
+		files_downcast(ref_store, 0, "create_reflog");
 
-	/* Check validity (but we don't need the result): */
-	files_downcast(ref_store, 0, "create_reflog");
-
-	ret = log_ref_setup(refname, &sb, err, force_create);
+	ret = log_ref_setup(refs, refname, &sb, err, force_create);
 	strbuf_release(&sb);
 	return ret;
 }
@@ -2899,7 +2893,8 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
 	return 0;
 }
 
-static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
+static int log_ref_write_1(struct files_ref_store *refs,
+			   const char *refname, const unsigned char *old_sha1,
 			   const unsigned char *new_sha1, const char *msg,
 			   struct strbuf *logfile, int flags,
 			   struct strbuf *err)
@@ -2909,7 +2904,7 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
 
-	result = log_ref_setup(refname, logfile, err, flags & REF_FORCE_CREATE_REFLOG);
+	result = log_ref_setup(refs, refname, logfile, err, flags & REF_FORCE_CREATE_REFLOG);
 
 	if (result)
 		return result;
@@ -2933,21 +2928,23 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 	return 0;
 }
 
-static int log_ref_write(const char *refname, const unsigned char *old_sha1,
+static int log_ref_write(struct files_ref_store *refs,
+			 const char *refname, const unsigned char *old_sha1,
 			 const unsigned char *new_sha1, const char *msg,
 			 int flags, struct strbuf *err)
 {
-	return files_log_ref_write(refname, old_sha1, new_sha1, msg, flags,
-				   err);
+	return files_log_ref_write(refs, refname, old_sha1, new_sha1,
+				   msg, flags, err);
 }
 
-int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
+int files_log_ref_write(struct files_ref_store *refs,
+			const char *refname, const unsigned char *old_sha1,
 			const unsigned char *new_sha1, const char *msg,
 			int flags, struct strbuf *err)
 {
 	struct strbuf sb = STRBUF_INIT;
-	int ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb, flags,
-				  err);
+	int ret = log_ref_write_1(refs, refname, old_sha1, new_sha1, msg,
+				  &sb, flags, err);
 	strbuf_release(&sb);
 	return ret;
 }
@@ -3004,7 +3001,8 @@ static int commit_ref_update(struct files_ref_store *refs,
 	files_assert_main_repository(refs, "commit_ref_update");
 
 	clear_loose_ref_cache(refs);
-	if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err)) {
+	if (log_ref_write(refs, lock->ref_name, lock->old_oid.hash,
+			  sha1, logmsg, 0, err)) {
 		char *old_msg = strbuf_detach(err, NULL);
 		strbuf_addf(err, "cannot update the ref '%s': %s",
 			    lock->ref_name, old_msg);
@@ -3035,8 +3033,8 @@ static int commit_ref_update(struct files_ref_store *refs,
 		if (head_ref && (head_flag & REF_ISSYMREF) &&
 		    !strcmp(head_ref, lock->ref_name)) {
 			struct strbuf log_err = STRBUF_INIT;
-			if (log_ref_write("HEAD", lock->old_oid.hash, sha1,
-					  logmsg, 0, &log_err)) {
+			if (log_ref_write(refs, "HEAD", lock->old_oid.hash,
+					  sha1, logmsg, 0, &log_err)) {
 				error("%s", log_err.buf);
 				strbuf_release(&log_err);
 			}
@@ -3068,23 +3066,26 @@ static int create_ref_symlink(struct ref_lock *lock, const char *target)
 	return ret;
 }
 
-static void update_symref_reflog(struct ref_lock *lock, const char *refname,
+static void update_symref_reflog(struct files_ref_store *refs,
+				 struct ref_lock *lock, const char *refname,
 				 const char *target, const char *logmsg)
 {
 	struct strbuf err = STRBUF_INIT;
 	unsigned char new_sha1[20];
 	if (logmsg && !read_ref(target, new_sha1) &&
-	    log_ref_write(refname, lock->old_oid.hash, new_sha1, logmsg, 0, &err)) {
+	    log_ref_write(refs, refname, lock->old_oid.hash,
+			  new_sha1, logmsg, 0, &err)) {
 		error("%s", err.buf);
 		strbuf_release(&err);
 	}
 }
 
-static int create_symref_locked(struct ref_lock *lock, const char *refname,
+static int create_symref_locked(struct files_ref_store *refs,
+				struct ref_lock *lock, const char *refname,
 				const char *target, const char *logmsg)
 {
 	if (prefer_symlink_refs && !create_ref_symlink(lock, target)) {
-		update_symref_reflog(lock, refname, target, logmsg);
+		update_symref_reflog(refs, lock, refname, target, logmsg);
 		return 0;
 	}
 
@@ -3092,7 +3093,7 @@ static int create_symref_locked(struct ref_lock *lock, const char *refname,
 		return error("unable to fdopen %s: %s",
 			     lock->lk->tempfile.filename.buf, strerror(errno));
 
-	update_symref_reflog(lock, refname, target, logmsg);
+	update_symref_reflog(refs, lock, refname, target, logmsg);
 
 	/* no error check; commit_ref will check ferror */
 	fprintf(lock->lk->tempfile.fp, "ref: %s\n", target);
@@ -3121,13 +3122,19 @@ static int files_create_symref(struct ref_store *ref_store,
 		return -1;
 	}
 
-	ret = create_symref_locked(lock, refname, target, logmsg);
+	ret = create_symref_locked(refs, lock, refname, target, logmsg);
 	unlock_ref(lock);
 	return ret;
 }
 
 int set_worktree_head_symref(const char *gitdir, const char *target)
 {
+	/*
+	 * FIXME: this obviously will not work well for future refs
+	 * backends. This function needs to die.
+	 */
+	struct files_ref_store *refs =
+		files_downcast(get_ref_store(NULL), 0, "set_head_symref");
 	static struct lock_file head_lock;
 	struct ref_lock *lock;
 	struct strbuf head_path = STRBUF_INIT;
@@ -3154,7 +3161,7 @@ int set_worktree_head_symref(const char *gitdir, const char *target)
 	lock->lk = &head_lock;
 	lock->ref_name = xstrdup(head_rel);
 
-	ret = create_symref_locked(lock, head_rel, target, NULL);
+	ret = create_symref_locked(refs, lock, head_rel, target, NULL);
 
 	unlock_ref(lock); /* will free lock */
 	strbuf_release(&head_path);
@@ -3164,14 +3171,13 @@ int set_worktree_head_symref(const char *gitdir, const char *target)
 static int files_reflog_exists(struct ref_store *ref_store,
 			       const char *refname)
 {
+	struct files_ref_store *refs =
+		files_downcast(ref_store, 0, "reflog_exists");
 	struct strbuf sb = STRBUF_INIT;
 	struct stat st;
 	int ret;
 
-	/* Check validity (but we don't need the result): */
-	files_downcast(ref_store, 0, "reflog_exists");
-
-	strbuf_git_path(&sb, "logs/%s", refname);
+	files_path(refs, &sb, "logs/%s", refname);
 	ret = !lstat(sb.buf, &st) && S_ISREG(st.st_mode);
 	strbuf_release(&sb);
 	return ret;
@@ -3180,13 +3186,12 @@ static int files_reflog_exists(struct ref_store *ref_store,
 static int files_delete_reflog(struct ref_store *ref_store,
 			       const char *refname)
 {
+	struct files_ref_store *refs =
+		files_downcast(ref_store, 0, "delete_reflog");
 	struct strbuf sb = STRBUF_INIT;
 	int ret;
 
-	/* Check validity (but we don't need the result): */
-	files_downcast(ref_store, 0, "delete_reflog");
-
-	strbuf_git_path(&sb, "logs/%s", refname);
+	files_path(refs, &sb, "logs/%s", refname);
 	ret = remove_path(sb.buf);
 	strbuf_release(&sb);
 	return ret;
@@ -3236,15 +3241,14 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
 					     each_reflog_ent_fn fn,
 					     void *cb_data)
 {
+	struct files_ref_store *refs =
+		files_downcast(ref_store, 0, "for_each_reflog_ent_reverse");
 	struct strbuf sb = STRBUF_INIT;
 	FILE *logfp;
 	long pos;
 	int ret = 0, at_tail = 1;
 
-	/* Check validity (but we don't need the result): */
-	files_downcast(ref_store, 0, "for_each_reflog_ent_reverse");
-
-	strbuf_git_path(&sb, "logs/%s", refname);
+	files_path(refs, &sb, "logs/%s", refname);
 	logfp = fopen(sb.buf, "r");
 	strbuf_release(&sb);
 	if (!logfp)
@@ -3345,14 +3349,13 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,
 				     const char *refname,
 				     each_reflog_ent_fn fn, void *cb_data)
 {
+	struct files_ref_store *refs =
+		files_downcast(ref_store, 0, "for_each_reflog_ent");
 	FILE *logfp;
 	struct strbuf sb = STRBUF_INIT;
 	int ret = 0;
 
-	/* Check validity (but we don't need the result): */
-	files_downcast(ref_store, 0, "for_each_reflog_ent");
-
-	strbuf_git_path(&sb, "logs/%s", refname);
+	files_path(refs, &sb, "logs/%s", refname);
 	logfp = fopen(sb.buf, "r");
 	strbuf_release(&sb);
 	if (!logfp)
@@ -3434,15 +3437,14 @@ static struct ref_iterator_vtable files_reflog_iterator_vtable = {
 
 static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_store)
 {
+	struct files_ref_store *refs =
+		files_downcast(ref_store, 0, "reflog_iterator_begin");
 	struct files_reflog_iterator *iter = xcalloc(1, sizeof(*iter));
 	struct ref_iterator *ref_iterator = &iter->base;
 	struct strbuf sb = STRBUF_INIT;
 
-	/* Check validity (but we don't need the result): */
-	files_downcast(ref_store, 0, "reflog_iterator_begin");
-
 	base_ref_iterator_init(ref_iterator, &files_reflog_iterator_vtable);
-	strbuf_git_path(&sb, "logs");
+	files_path(refs, &sb, "logs");
 	iter->dir_iterator = dir_iterator_begin(sb.buf);
 	strbuf_release(&sb);
 	return ref_iterator;
@@ -3866,8 +3868,8 @@ static int files_transaction_commit(struct ref_store *ref_store,
 
 		if (update->flags & REF_NEEDS_COMMIT ||
 		    update->flags & REF_LOG_ONLY) {
-			if (log_ref_write(lock->ref_name, lock->old_oid.hash,
-					  update->new_sha1,
+			if (log_ref_write(refs, lock->ref_name,
+					  lock->old_oid.hash, update->new_sha1,
 					  update->msg, update->flags, err)) {
 				char *old_msg = strbuf_detach(err, NULL);
 
@@ -3916,7 +3918,7 @@ static int files_transaction_commit(struct ref_store *ref_store,
 	for_each_string_list_item(ref_to_delete, &refs_to_delete) {
 		struct strbuf sb = STRBUF_INIT;
 
-		strbuf_git_path(&sb, "logs/%s", ref_to_delete->string);
+		files_path(refs, &sb, "logs/%s", ref_to_delete->string);
 		unlink_or_warn(sb.buf);
 		strbuf_release(&sb);
 	}
@@ -4078,6 +4080,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 	int status = 0;
 	int type;
 	struct strbuf err = STRBUF_INIT;
+	struct strbuf sb = STRBUF_INIT;
 
 	memset(&cb, 0, sizeof(cb));
 	cb.flags = flags;
@@ -4102,7 +4105,8 @@ static int files_reflog_expire(struct ref_store *ref_store,
 		return 0;
 	}
 
-	log_file = git_pathdup("logs/%s", refname);
+	files_path(refs, &sb, "logs/%s", refname);
+	log_file = strbuf_detach(&sb, NULL);
 	if (!(flags & EXPIRE_REFLOGS_DRY_RUN)) {
 		/*
 		 * Even though holding $GIT_DIR/logs/$reflog.lock has
@@ -4173,25 +4177,24 @@ static int files_reflog_expire(struct ref_store *ref_store,
 
 static int files_init_db(struct ref_store *ref_store, struct strbuf *err)
 {
+	struct files_ref_store *refs =
+		files_downcast(ref_store, 0, "init_db");
 	struct strbuf sb = STRBUF_INIT;
 
-	/* Check validity (but we don't need the result): */
-	files_downcast(ref_store, 0, "init_db");
-
 	/*
 	 * Create .git/refs/{heads,tags}
 	 */
-	strbuf_git_path(&sb, "refs/heads");
+	files_path(refs, &sb, "refs/heads");
 	safe_create_dir(sb.buf, 1);
 	strbuf_reset(&sb);
-	strbuf_git_path(&sb, "refs/tags");
+	files_path(refs, &sb, "refs/tags");
 	safe_create_dir(sb.buf, 1);
 	strbuf_reset(&sb);
 	if (get_shared_repository()) {
-		strbuf_git_path(&sb, "refs/heads");
+		files_path(refs, &sb, "refs/heads");
 		adjust_shared_perm(sb.buf);
 		strbuf_reset(&sb);
-		strbuf_git_path(&sb, "refs/tags");
+		files_path(refs, &sb, "refs/tags");
 		adjust_shared_perm(sb.buf);
 	}
 	strbuf_release(&sb);
-- 
2.11.0.157.gd943d85


^ permalink raw reply related

* [PATCH v2 07/16] files-backend: remove the use of git_path()
From: Nguyễn Thái Ngọc Duy @ 2017-02-16 11:48 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Michael Haggerty, Johannes Schindelin,
	Ramsay Jones, Stefan Beller, novalis,
	Nguyễn Thái Ngọc Duy
In-Reply-To: <20170216114818.6080-1-pclouds@gmail.com>

Given $GIT_DIR and $GIT_COMMON_DIR, files-backend is now in charge of
deciding what goes where. The end goal is to pass $GIT_DIR only. A
refs "view" of a linked worktree is a logical ref store that combines
two files backends together.

(*) Not entirely true since strbuf_git_path_submodule() still does path
translation underneath. But that's for another patch.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 refs/files-backend.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 24f5bf7f1..74e31d041 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -924,6 +924,9 @@ struct files_ref_store {
 	 */
 	const char *submodule;
 
+	struct strbuf gitdir;
+	struct strbuf gitcommondir;
+
 	struct ref_entry *loose;
 	struct packed_ref_cache *packed;
 };
@@ -937,6 +940,7 @@ static void files_path(struct files_ref_store *refs, struct strbuf *sb,
 {
 	struct strbuf tmp = STRBUF_INIT;
 	va_list vap;
+	const char *ref;
 
 	va_start(vap, fmt);
 	strbuf_vaddf(&tmp, fmt, vap);
@@ -944,8 +948,12 @@ static void files_path(struct files_ref_store *refs, struct strbuf *sb,
 	if (refs->submodule)
 		strbuf_git_path_submodule(sb, refs->submodule,
 					  "%s", tmp.buf);
+	else if (is_per_worktree_ref(tmp.buf) ||
+		 (skip_prefix(tmp.buf, "logs/", &ref) &&
+		  is_per_worktree_ref(ref)))
+		strbuf_addf(sb, "%s/%s", refs->gitdir.buf, tmp.buf);
 	else
-		strbuf_git_path(sb, "%s", tmp.buf);
+		strbuf_addf(sb, "%s/%s", refs->gitcommondir.buf, tmp.buf);
 	strbuf_release(&tmp);
 }
 
@@ -1004,7 +1012,15 @@ static struct ref_store *files_ref_store_create(const char *submodule)
 
 	base_ref_store_init(ref_store, &refs_be_files);
 
-	refs->submodule = xstrdup_or_null(submodule);
+	strbuf_init(&refs->gitdir, 0);
+	strbuf_init(&refs->gitcommondir, 0);
+
+	if (submodule) {
+		refs->submodule = xstrdup(submodule);
+	} else {
+		strbuf_addstr(&refs->gitdir, get_git_dir());
+		strbuf_addstr(&refs->gitcommondir, get_git_common_dir());
+	}
 
 	return ref_store;
 }
-- 
2.11.0.157.gd943d85


^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox