On Fri, Sep 23 2022, Stefan Xenos via GitGitGadget wrote:
From: Stefan Xenos <redacted>
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+ 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')
quoted hunk ↗ jump to hunk
+ }
+
+ 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.
quoted hunk ↗ jump to hunk
+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.