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

[Atharva Raykar <raykar.ath@gmail.com>]

Atharva Raykar <raykar.ath@gmail.com> writes:
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>> 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()'.