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

[Chris P <christophe.poucet@gmail.com>]

On Mon, Sep 26, 2022 at 10:35 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Fri, Sep 23 2022, Stefan Xenos via GitGitGadget wrote:
>
> > From: Stefan Xenos <sxenos@google.com>
>
> > +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!