Re: [PATCH 04/15] scalar: 'register' sets recommended config and starts maintenance

2 messages, 2 authors, 2021-09-03 · open the first message on its own page

Re: [PATCH 04/15] scalar: 'register' sets recommended config and starts maintenance

From: Junio C Hamano <hidden>
Date: 2021-09-01 16:17:09

"Derrick Stolee via GitGitGadget" [off-list ref] writes:
+static void setup_enlistment_directory(int argc, const char **argv,
+				       const char * const *usagestr,
+				       const struct option *options,
+				       struct strbuf *enlistment_root)
+{
+	struct strbuf path = STRBUF_INIT;
+	char *root;
+	int enlistment_found = 0;
+
+	if (startup_info->have_repository)
+		BUG("gitdir already set up?!?");
+
+	if (argc > 1)
+		usage_with_options(usagestr, options);
+
+	/* find the worktree, determine its corresponding root */
+	if (argc == 1)
+		strbuf_add_absolute_path(&path, argv[0]);
+	else if (strbuf_getcwd(&path) < 0)
+		die(_("need a working directory"));
+
+	strbuf_trim_trailing_dir_sep(&path);
+	do {
+		const size_t len = path.len;
+
+		/* check if currently in enlistment root with src/ workdir */
+		strbuf_addstr(&path, "/src/.git");
+		if (is_git_directory(path.buf)) {
+			strbuf_strip_suffix(&path, "/.git");
+
+			if (enlistment_root)
+				strbuf_add(enlistment_root, path.buf, len);
+
+			enlistment_found = 1;
+			break;
+		}
This special casing of "normally the top of the working tree is
enlisted, but if the repository is called src/, then we enslist
one level up" is a bit of eyesore because

 (1) it is unclear why such a directory with 'src/' subdirectory is
     so special, and

 (2) it fails to serve those who has the same need but named their
     source subdirectory differently (like 'source/').

"The design decisions we made are all part of being opinionated" can
all explain it away, but at least we should let the users know where
the opinionated choices scalar makes want to lead them to, and this
"src/" stuff needs a bit of clarification.  Perhaps a documentation
will be added in later steps?
+	for (i = 0; config[i].key; i++) {
+		if (git_config_get_string(config[i].key, &value)) {
+			trace2_data_string("scalar", the_repository, config[i].key, "created");
+			if (git_config_set_gently(config[i].key,
+						  config[i].value) < 0)
+				return error(_("could not configure %s=%s"),
+					     config[i].key, config[i].value);
+		} else {
+			trace2_data_string("scalar", the_repository, config[i].key, "exists");
+			free(value);
+		}
I wonder if we should have a table of configuration variables and
their default values.  The above code implements a skewed "we only
avoid overriding what is explicitly configured".  A variable that
the user left unconfigured because the user found its default
satisfactory will be overridden, and if the value scalar wants to
use happens to be the default value, we leave an explicit
configuration to that default value in the resulting configuration
file.

But I think the above is the best we can do without such a central
registry of configuration variables.

Re: [PATCH 04/15] scalar: 'register' sets recommended config and starts maintenance

From: Johannes Schindelin <hidden>
Date: 2021-09-03 15:41:41

Hi Junio,

On Wed, 1 Sep 2021, Junio C Hamano wrote:
"Derrick Stolee via GitGitGadget" [off-list ref] writes:
quoted
+static void setup_enlistment_directory(int argc, const char **argv,
+				       const char * const *usagestr,
+				       const struct option *options,
+				       struct strbuf *enlistment_root)
+{
+	struct strbuf path = STRBUF_INIT;
+	char *root;
+	int enlistment_found = 0;
+
+	if (startup_info->have_repository)
+		BUG("gitdir already set up?!?");
+
+	if (argc > 1)
+		usage_with_options(usagestr, options);
+
+	/* find the worktree, determine its corresponding root */
+	if (argc == 1)
+		strbuf_add_absolute_path(&path, argv[0]);
+	else if (strbuf_getcwd(&path) < 0)
+		die(_("need a working directory"));
+
+	strbuf_trim_trailing_dir_sep(&path);
+	do {
+		const size_t len = path.len;
+
+		/* check if currently in enlistment root with src/ workdir */
+		strbuf_addstr(&path, "/src/.git");
+		if (is_git_directory(path.buf)) {
+			strbuf_strip_suffix(&path, "/.git");
+
+			if (enlistment_root)
+				strbuf_add(enlistment_root, path.buf, len);
+
+			enlistment_found = 1;
+			break;
+		}
This special casing of "normally the top of the working tree is
enlisted, but if the repository is called src/, then we enslist
one level up" is a bit of eyesore because

 (1) it is unclear why such a directory with 'src/' subdirectory is
     so special, and

 (2) it fails to serve those who has the same need but named their
     source subdirectory differently (like 'source/').
All true. I wish we had come up with a better way, or with a way to
override this via an option.

Unfortunately, we are now bound by the fact that there are already users
out there...
"The design decisions we made are all part of being opinionated" can
all explain it away, but at least we should let the users know where
the opinionated choices scalar makes want to lead them to, and this
"src/" stuff needs a bit of clarification.  Perhaps a documentation
will be added in later steps?
I had hoped that the initial blurb of the manual page was sufficient, but
you're right, the `register` subcommand is particular in that it allows to
force Scalar to consider the worktree to be identical to the Scalar
enlistment. I added this:

	diff --git a/contrib/scalar/scalar.txt b/contrib/scalar/scalar.txt
	index 1593da45eae..568987064b2 100644
	--- a/contrib/scalar/scalar.txt
	+++ b/contrib/scalar/scalar.txt
	@@ -40,6 +40,10 @@ register [<enlistment>]::
		and starts background maintenance. If `<enlistment>` is not provided,
		then the enlistment associated with the current working directory is
		registered.
	++
	+Note: when this subcommand is called in a worktree that is called `src/`, its
	+parent directory is considered to be the Scalar enlistment. If the worktree is
	+_not_ called `src/`, it itself will be considered to be the Scalar enlistment.
quoted
+	for (i = 0; config[i].key; i++) {
+		if (git_config_get_string(config[i].key, &value)) {
+			trace2_data_string("scalar", the_repository, config[i].key, "created");
+			if (git_config_set_gently(config[i].key,
+						  config[i].value) < 0)
+				return error(_("could not configure %s=%s"),
+					     config[i].key, config[i].value);
+		} else {
+			trace2_data_string("scalar", the_repository, config[i].key, "exists");
+			free(value);
+		}
I wonder if we should have a table of configuration variables and
their default values.  The above code implements a skewed "we only
avoid overriding what is explicitly configured".  A variable that
the user left unconfigured because the user found its default
satisfactory will be overridden, and if the value scalar wants to
use happens to be the default value, we leave an explicit
configuration to that default value in the resulting configuration
file.

But I think the above is the best we can do without such a central
registry of configuration variables.
Even with such a central registry, there would still be the question
whether the user, by staying with the default, wanted Git (or in this
instance, Scalar) to keep using the old default. The intention is
unfortunately not clear just from setting the variable.

So I think this is the best we can do.

Ciao,
Dscho
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help