diff mbox series

[11/15] scalar: allow reconfiguring an existing enlistment

Message ID 13056f02018542f8143e4933fbe180a0a9f77004.1630359290.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Upstreaming the Scalar command | expand

Commit Message

Johannes Schindelin Aug. 30, 2021, 9:34 p.m. UTC
From: Johannes Schindelin <johannes.schindelin@gmx.de>

This comes in handy during Scalar upgrades, or when config settings were
messed up by mistake.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 contrib/scalar/scalar.c          | 81 ++++++++++++++++++++------------
 contrib/scalar/scalar.txt        |  8 ++++
 contrib/scalar/t/t9099-scalar.sh |  8 ++++
 3 files changed, 68 insertions(+), 29 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Aug. 31, 2021, 8:29 a.m. UTC | #1
On Mon, Aug 30 2021, Johannes Schindelin via GitGitGadget wrote:

> This comes in handy during Scalar upgrades, or when config settings were
> messed up by mistake.

> [...]
>  		const char *key;
>  		const char *value;
> +		int overwrite_on_reconfigure;

If you make this a "keep_on_reconfigure", then ...

>  	} config[] = {
> -		{ "am.keepCR", "true" },
> -		{ "core.FSCache", "true" },
> -		{ "core.multiPackIndex", "true" },
> -		{ "core.preloadIndex", "true" },
> +		/* Required */
> +		{ "am.keepCR", "true", 1 },
> +		{ "core.FSCache", "true", 1 },
> +		{ "core.multiPackIndex", "true", 1 },
> +		{ "core.preloadIndex", "true", 1 },

You won't need the churn/boilerplate of adding "1" to everything here,
but can just change the initial patch to use designated initializers.

That along with a throwaway macro like:

#define SCALAR_CFG_TRUE(k) (.key = k, .value = "true")
#define SCALAR_CFG_FALSE(k) (.key = k, .value = "false")

Might (or might not) make this even easier to eyeball...
Johannes Schindelin Sept. 3, 2021, 3:53 p.m. UTC | #2
Hi Ævar,

On Tue, 31 Aug 2021, Ævar Arnfjörð Bjarmason wrote:

>
> On Mon, Aug 30 2021, Johannes Schindelin via GitGitGadget wrote:
>
> > This comes in handy during Scalar upgrades, or when config settings were
> > messed up by mistake.
>
> > [...]
> >  		const char *key;
> >  		const char *value;
> > +		int overwrite_on_reconfigure;
>
> If you make this a "keep_on_reconfigure", then ...

I do not think that this would be a better name, or that renaming this
field would do anything except cause more work for me.

>
> >  	} config[] = {
> > -		{ "am.keepCR", "true" },
> > -		{ "core.FSCache", "true" },
> > -		{ "core.multiPackIndex", "true" },
> > -		{ "core.preloadIndex", "true" },
> > +		/* Required */
> > +		{ "am.keepCR", "true", 1 },
> > +		{ "core.FSCache", "true", 1 },
> > +		{ "core.multiPackIndex", "true", 1 },
> > +		{ "core.preloadIndex", "true", 1 },
>
> You won't need the churn/boilerplate of adding "1" to everything here,
> but can just change the initial patch to use designated initializers.
>
> That along with a throwaway macro like:
>
> #define SCALAR_CFG_TRUE(k) (.key = k, .value = "true")
> #define SCALAR_CFG_FALSE(k) (.key = k, .value = "false")
>
> Might (or might not) make this even easier to eyeball...

To me, it makes things less readable. There is an entire section with the
header `/* Optional */` below, and I want this list to stay as readable as
it is now.

Ciao,
Dscho
Ævar Arnfjörð Bjarmason Sept. 6, 2021, 1:01 a.m. UTC | #3
On Fri, Sep 03 2021, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Tue, 31 Aug 2021, Ævar Arnfjörð Bjarmason wrote:
>
>>
>> On Mon, Aug 30 2021, Johannes Schindelin via GitGitGadget wrote:
>>
>> > This comes in handy during Scalar upgrades, or when config settings were
>> > messed up by mistake.
>>
>> > [...]
>> >  		const char *key;
>> >  		const char *value;
>> > +		int overwrite_on_reconfigure;
>>
>> If you make this a "keep_on_reconfigure", then ...
>
> I do not think that this would be a better name, or that renaming this
> field would do anything except cause more work for me.

It would also result in more readable code, i.e. why add boilerplate ",
1" to a boolean field in this case if every single setting is set to
"1"? Doesn't it make more sense to invert the variable name & save on
the verbosity?

>>
>> >  	} config[] = {
>> > -		{ "am.keepCR", "true" },
>> > -		{ "core.FSCache", "true" },
>> > -		{ "core.multiPackIndex", "true" },
>> > -		{ "core.preloadIndex", "true" },
>> > +		/* Required */
>> > +		{ "am.keepCR", "true", 1 },
>> > +		{ "core.FSCache", "true", 1 },
>> > +		{ "core.multiPackIndex", "true", 1 },
>> > +		{ "core.preloadIndex", "true", 1 },
>>
>> You won't need the churn/boilerplate of adding "1" to everything here,
>> but can just change the initial patch to use designated initializers.
>>
>> That along with a throwaway macro like:
>>
>> #define SCALAR_CFG_TRUE(k) (.key = k, .value = "true")
>> #define SCALAR_CFG_FALSE(k) (.key = k, .value = "false")
>>
>> Might (or might not) make this even easier to eyeball...
>
> To me, it makes things less readable. There is an entire section with the
> header `/* Optional */` below, and I want this list to stay as readable as
> it is now.

Yeah, I think those macros are probably less readable too. I should have
phrased that as a "one could even...", but just the smaller change of
avoiding the ", 1" everywhere seems worthwhile.
diff mbox series

Patch

diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
index d5d38a1afeb..4eff3464a13 100644
--- a/contrib/scalar/scalar.c
+++ b/contrib/scalar/scalar.c
@@ -115,18 +115,20 @@  static int run_git(const char *arg, ...)
 	return res;
 }
 
-static int set_recommended_config(void)
+static int set_recommended_config(int reconfigure)
 {
 	struct {
 		const char *key;
 		const char *value;
+		int overwrite_on_reconfigure;
 	} config[] = {
-		{ "am.keepCR", "true" },
-		{ "core.FSCache", "true" },
-		{ "core.multiPackIndex", "true" },
-		{ "core.preloadIndex", "true" },
+		/* Required */
+		{ "am.keepCR", "true", 1 },
+		{ "core.FSCache", "true", 1 },
+		{ "core.multiPackIndex", "true", 1 },
+		{ "core.preloadIndex", "true", 1 },
 #ifndef WIN32
-		{ "core.untrackedCache", "true" },
+		{ "core.untrackedCache", "true", 1 },
 #else
 		/*
 		 * Unfortunately, Scalar's Functional Tests demonstrated
@@ -140,29 +142,30 @@  static int set_recommended_config(void)
 		 * Therefore, with a sad heart, we disable this very useful
 		 * feature on Windows.
 		 */
-		{ "core.untrackedCache", "false" },
+		{ "core.untrackedCache", "false", 1 },
 #endif
-		{ "core.bare", "false" },
-		{ "core.logAllRefUpdates", "true" },
-		{ "credential.https://dev.azure.com.useHttpPath", "true" },
-		{ "credential.validate", "false" }, /* GCM4W-only */
-		{ "gc.auto", "0" },
-		{ "gui.GCWarning", "false" },
-		{ "index.threads", "true" },
-		{ "index.version", "4" },
-		{ "merge.stat", "false" },
-		{ "merge.renames", "false" },
-		{ "pack.useBitmaps", "false" },
-		{ "pack.useSparse", "true" },
-		{ "receive.autoGC", "false" },
-		{ "reset.quiet", "true" },
-		{ "feature.manyFiles", "false" },
-		{ "feature.experimental", "false" },
-		{ "fetch.unpackLimit", "1" },
-		{ "fetch.writeCommitGraph", "false" },
+		{ "core.bare", "false", 1 },
+		{ "core.logAllRefUpdates", "true", 1 },
+		{ "credential.https://dev.azure.com.useHttpPath", "true", 1 },
+		{ "credential.validate", "false", 1 }, /* GCM4W-only */
+		{ "gc.auto", "0", 1 },
+		{ "gui.GCWarning", "false", 1 },
+		{ "index.threads", "true", 1 },
+		{ "index.version", "4", 1 },
+		{ "merge.stat", "false", 1 },
+		{ "merge.renames", "false", 1 },
+		{ "pack.useBitmaps", "false", 1 },
+		{ "pack.useSparse", "true", 1 },
+		{ "receive.autoGC", "false", 1 },
+		{ "reset.quiet", "true", 1 },
+		{ "feature.manyFiles", "false", 1 },
+		{ "feature.experimental", "false", 1 },
+		{ "fetch.unpackLimit", "1", 1 },
+		{ "fetch.writeCommitGraph", "false", 1 },
 #ifdef WIN32
-		{ "http.sslBackend", "schannel" },
+		{ "http.sslBackend", "schannel", 1 },
 #endif
+		/* Optional */
 		{ "status.aheadBehind", "false" },
 		{ "commitGraph.generationVersion", "1" },
 		{ "core.autoCRLF", "false" },
@@ -173,7 +176,8 @@  static int set_recommended_config(void)
 	char *value;
 
 	for (i = 0; config[i].key; i++) {
-		if (git_config_get_string(config[i].key, &value)) {
+		if ((reconfigure && config[i].overwrite_on_reconfigure) ||
+		    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)
@@ -238,7 +242,7 @@  static int register_dir(void)
 	int res = add_or_remove_enlistment(1);
 
 	if (!res)
-		res = set_recommended_config();
+		res = set_recommended_config(0);
 
 	if (!res)
 		res = toggle_maintenance(1);
@@ -425,7 +429,7 @@  static int cmd_clone(int argc, const char **argv)
 	    (res = run_git("sparse-checkout", "init", "--cone", NULL)))
 		goto cleanup;
 
-	if (set_recommended_config())
+	if (set_recommended_config(0))
 		return error(_("could not configure '%s'"), dir);
 
 	if ((res = run_git("fetch", "--quiet", "origin", NULL))) {
@@ -490,6 +494,24 @@  static int cmd_register(int argc, const char **argv)
 	return register_dir();
 }
 
+static int cmd_reconfigure(int argc, const char **argv)
+{
+	struct option options[] = {
+		OPT_END(),
+	};
+	const char * const usage[] = {
+		N_("scalar reconfigure [<enlistment>]"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, NULL, options,
+			     usage, 0);
+
+	setup_enlistment_directory(argc, argv, usage, options, NULL);
+
+	return set_recommended_config(1);
+}
+
 static int cmd_run(int argc, const char **argv)
 {
 	struct option options[] = {
@@ -626,6 +648,7 @@  static struct {
 	{ "register", cmd_register },
 	{ "unregister", cmd_unregister },
 	{ "run", cmd_run },
+	{ "reconfigure", cmd_reconfigure },
 	{ NULL, NULL},
 };
 
diff --git a/contrib/scalar/scalar.txt b/contrib/scalar/scalar.txt
index 9aadaf6323f..227e3542a07 100644
--- a/contrib/scalar/scalar.txt
+++ b/contrib/scalar/scalar.txt
@@ -13,6 +13,7 @@  scalar list
 scalar register [<enlistment>]
 scalar unregister [<enlistment>]
 scalar run ( all | config | commit-graph | fetch | loose-objects | pack-files ) [<enlistment>]
+scalar reconfigure <enlistment>
 
 DESCRIPTION
 -----------
@@ -113,6 +114,13 @@  opinionated default settings that make Git work more efficiently with
 large repositories. As this task is run as part of `scalar clone`
 automatically, explicit invocations of this task are rarely needed.
 
+Reconfigure
+~~~~~~~~~~~
+
+After a Scalar upgrade, or when the configuration of a Scalar enlistment
+was somehow corrupted or changed by mistake, this subcommand allows to
+reconfigure the enlistment.
+
 SEE ALSO
 --------
 linkgit:git-clone[1], linkgit:git-maintenance[1].
diff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh
index 9a35ab4fde6..e6d74a06ca0 100755
--- a/contrib/scalar/t/t9099-scalar.sh
+++ b/contrib/scalar/t/t9099-scalar.sh
@@ -65,4 +65,12 @@  test_expect_success 'scalar clone' '
 	)
 '
 
+test_expect_success 'scalar reconfigure' '
+	git init one/src &&
+	scalar register one &&
+	git -C one/src config core.preloadIndex false &&
+	scalar reconfigure one &&
+	test true = "$(git -C one/src config core.preloadIndex)"
+'
+
 test_done