On Mon, Sep 26, 2022 at 10:35 AM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
>
>
> 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.
Done.
>
> > [...]
> > + 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.
Done.
>
>
> > + int i;
>
> ditto size_t above...
>
> > + for (i = 0; i < changes.nr; i++) {
>
> ...for this iteration...
Obsolete, moved to using for_each_string_list_item
>
> > + 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.
>
Done.
> 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')
Done
>
>
> > + }
> > +
> > + 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.
Done.
>
> > +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.
Done, thanks!
