Re: [PATCH 07/10] evolve: implement the git change command

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 26, 2022, 08:25 UTC
Message-ID: <220926.8635ce4jox.gmgdl@evledraar.gmail.com>
In-Reply-To: <914028341842a4d57e02ec42a7426d3aa83640f9.1663959325.git.gitgitgadget@gmail.com>

On Fri, Sep 23 2022, Stefan Xenos via GitGitGadget wrote:

> From: Stefan Xenos <sxenos@google.com>

Show 9 quoted lines

> +static const char * const builtin_change_usage[] = {
> +	N_("git change update [--force] [--replace <treeish>...] [--origin <treesih>...] [--content <newtreeish>]"),
> +	NULL
> +};
> +
> +static const char * const builtin_update_usage[] = {
> +	N_("git change update [--force] [--replace <treeish>...] [--origin <treesih>...] [--content <newtreeish>]"),
> +	NULL
> +};

This (and the corresponding later *.txt version) should indent the overly long -h line, probably after "[--replace <treeish>...]".

> +struct update_state {
> +	int options;

I think this should be an enum in your earlier 06/10. Makes things more

> +		die(_("Failed to resolve '%s' as a valid revision."), committish);

This and other error should start with a lower-case letter, see CodingGuidelines on errors.

> [...]
> +		die(_("Could not parse object '%s'."), committish);

Ditto etc.

> +	int i;
> +	for (i = 0; i < commitsish_list->nr; i++) {

A string_list uses a size_t for a nr, not int, so lets make that "size_t i".

This both makes things more obvious, and helps some compilers spot unsigned v.s. signed issues.

> +	int i;

ditto size_t above...

> +	for (i = 0; i < changes.nr; i++) {

...for this iteration...

> +		struct string_list_item *it = &changes.items[i];

...but actually don't you just want for_each_string_list_item() instead?

> +		if (it->util)
> +			fprintf(stdout, N_("Updated change %s\n"), name);
> +		else
> +			fprintf(stdout, N_("Created change %s\n"), name);

The use of N_() here is wrong, you should use _(), N_() just marks things for translation, but doesn't use it.

We also tend to try to avoid adding \n in translations needlessly. And since you're printing to stdout this can be:

	if (...)
		printf(_("Updated change %s"), name);
	...
	putchar('\n')

Show 40 quoted lines

> +	}
> +
> +	string_list_clear(&changes, 0);
> +	change_table_clear(&chtable);
> +	clear_metacommit_data(&metacommit);
> +
> +	return ret;
> +}
> +
> +static int change_update(int argc, const char **argv, const char* prefix)
> +{
> +	int result;
> +	int force = 0;
> +	int newchange = 0;
> +	struct strbuf err = STRBUF_INIT;
> +	struct update_state state;
> +	struct option options[] = {
> +		{ OPTION_CALLBACK, 'r', "replace", &state, N_("commit"),
> +			N_("marks the given commit as being obsolete"),
> +			0, update_option_parse_replace },
> +		{ OPTION_CALLBACK, 'o', "origin", &state, N_("commit"),
> +			N_("marks the given commit as being the origin of this commit"),
> +			0, update_option_parse_origin },
> +		OPT_BOOL('F', "force", &force,
> +			N_("overwrite an existing change of the same name")),
> +		OPT_STRING('c', "content", &state.content, N_("commit"),
> +				 N_("identifies the new content commit for the change")),
> +		OPT_STRING('g', "change", &state.change, N_("commit"),
> +				 N_("name of the change to update")),
> +		OPT_BOOL('n', "new", &newchange,
> +			N_("create a new change - do not append to any existing change")),
> +		OPT_END()
> +	};
> +
> +	init_update_state(&state);
> +
> +	argc = parse_options(argc, argv, prefix, options, builtin_update_usage, 0);
> +
> +	if (force) state.options |= UPDATE_OPTION_FORCE;
> +	if (newchange) state.options |= UPDATE_OPTION_NOAPPEND;

Just use OPT_SET_INT_F() and skip the indirection thorugh OPT_BOOL(), that macro itself is a thin wrapper for OPT_SET_INT_F().

I.e. you can drop these "force" and "newchange" variables, andjust set your state.options directly.

Show 19 quoted lines

> +int cmd_change(int argc, const char **argv, const char *prefix)
> +{
> +	/* No options permitted before subcommand currently */
> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	int result = 1;
> +
> +	argc = parse_options(argc, argv, prefix, options, builtin_change_usage,
> +		PARSE_OPT_STOP_AT_NON_OPTION);
> +
> +	if (argc < 1)
> +		usage_with_options(builtin_change_usage, options);
> +	else if (!strcmp(argv[0], "update"))
> +		result = change_update(argc, argv, prefix);
> +	else {
> +		error(_("Unknown subcommand: %s"), argv[0]);
> +		usage_with_options(builtin_change_usage, options);
> +	}

This was presumably written before the recent OPT_SUBCOMMAND(), and should instead use that API.