On Fri, Sep 23 2022, Stefan Xenos via GitGitGadget wrote:
> From: Stefan Xenos <sxenos@xxxxxxxxxx>
> +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')
> + }
> +
> + 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.
> +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.
