Re: [GSoC] [PATCH] submodule--helper: run update procedures from C

Atharva Raykar [off-list ref] writes:

quoted

Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

static void update_submodule(struct update_clone_data *ucd)
{
	fprintf(stdout, "dummy %s %d\t%s\n",

@@ -2379,6 +2552,79 @@ static int update_clone(int argc, const char **argv, const char *prefix)

	return update_submodules(&suc);
}

+static int run_update_procedure(int argc, const char **argv, const char *prefix)
+{
+	int force = 0, quiet = 0, nofetch = 0, just_cloned = 0;
+	char *prefixed_path, *update = NULL;
+	char *sha1 = NULL, *subsha1 = NULL;
+	struct update_data update_data = UPDATE_DATA_INIT;
+
+	struct option options[] = {
+		OPT__QUIET(&quiet, N_("suppress output for update by rebase or merge")),
+		OPT__FORCE(&force, N_("force checkout updates"), 0),
+		OPT_BOOL('N', "no-fetch", &nofetch,
+			 N_("don't fetch new objects from the remote site")),
+		OPT_BOOL(0, "just-cloned", &just_cloned,
+			 N_("overrides update mode in case the repository is a fresh clone")),
+		OPT_INTEGER(0, "depth", &update_data.depth, N_("depth for shallow fetch")),
+		OPT_STRING(0, "prefix", &prefix,
+			   N_("path"),
+			   N_("path into the working tree")),
+		OPT_STRING(0, "update", &update,
+			   N_("string"),
+			   N_("rebase, merge, checkout or none")),
+		OPT_STRING(0, "recursive-prefix", &update_data.recursive_prefix, N_("path"),
+			   N_("path into the working tree, across nested "
+			      "submodule boundaries")),
+		OPT_STRING(0, "sha1", &sha1, N_("string"),
+			   N_("SHA1 expected by superproject")),
+		OPT_STRING(0, "subsha1", &subsha1, N_("string"),
+			   N_("SHA1 of submodule's HEAD")),
+		OPT_END()
+	};
+
+	const char *const usage[] = {
+		N_("git submodule--helper run-update-procedure [<options>] <path>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (argc != 1)
+		usage_with_options(usage, options);
+	update_data.force = !!force;
+	update_data.quiet = !!quiet;
+	update_data.nofetch = !!nofetch;
+	update_data.just_cloned = !!just_cloned;

For all of these just pass the reference to the update_data variable
directly in the OPT_*(). No need to set an "int force", only to copy it
over to update_data.force. Let's just use the latter only.

Hmm, I'm trying to remember why the single bit values are treated this way
in this whole file...

...there seems to be no good reason for it. The API docs for parse options
state that OPT_BOOL() is guaranteed to return either zero or one, so that
double negation does look unnecessary.

I forgot to mention why I did not address this change in my v3 patch.
The reason why we are handling boolean values this way is because they
are declared as bitfields in the 'update_data' struct. Since we cannot
take the address of bitfields, we have to use a different variable to
store when using 'parse_options()'.