On 22-Feb-19 11:11 PM, Junio C Hamano wrote:
"Slavica Djukic via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
+static int parse_color_slot(const char *slot)
+{
+ if (!strcasecmp(slot, "prompt"))
+ return COLOR_PROMPT;
+ if (!strcasecmp(slot, "header"))
+ return COLOR_HEADER;
+ if (!strcasecmp(slot, "help"))
+ return COLOR_HELP;
+ if (!strcasecmp(slot, "error"))
+ return COLOR_ERROR;
As these are "color.interactive.<name>", matching case-insensitively
is the right thing to do. Good.
If we would end up increasing the number of slots, we may need to
switch to LOOKUP_CONFIG(), but for just four of them, this will do.
@@ -313,3 +357,78 @@ static struct choices *list_and_choose(struct choices *data,
return NULL;
}
}
+
+static struct choice *make_choice(const char *name )
Excess SP before ')'?
+{
+ struct choice *choice;
Style: have a blank line here to delineate decls (at the beginning
of a function) and stmts (at all the decls).
+ FLEXPTR_ALLOC_STR(choice, name, name);
+ return choice;
+}
+
+static struct choice *add_choice(struct choices *choices, const char type,
+ struct file_stat *file, struct command *command)
+{
+ struct choice *choice;
Ditto here.
+ switch (type) {
+ case 'f':
Style: in our codebase, switch and case are indented to the same
level, with the body of each case arm indented one more level.
+ choice = make_choice(file->name);
+ choice->u.file.index.added = file->index.added;
+ choice->u.file.index.deleted = file->index.deleted;
+ choice->u.file.worktree.added = file->worktree.added;
+ choice->u.file.worktree.deleted = file->worktree.deleted;
Would it make sense to make sure that all of file->index,
u.file.index, file->wt, u.file.wt are exactly the same type of
struct by introducing
struct adddel { uintmax_t add, del; };
in a very early part of the series, and embed that structure as a
member in "struct choice" and "struct file_stat"? That way, these
assignments would become two structure assignments, that would be
much easier to read.
Yes, definitely. I worked on this and it will be included in the next
iteration.
+void add_i_status(void)
+{
+ struct collection_status *s;
+ struct list_and_choose_options opts = { 0 };
+ struct hashmap *map;
+ struct hashmap_iter iter;
+ struct choices choices = CHOICES_INIT;
+ struct file_stat *entry;
+ const char *modified_fmt = _("%12s %12s %s");
+ const char type = 'f';
+
+ opts.header = xmalloc(sizeof(char) * (HEADER_MAXLEN + 1));
+ snprintf(opts.header, HEADER_MAXLEN + 1, modified_fmt,
+ _("staged"), _("unstaged"), _("path"));
Is there aversion to use of strbuf among your mentors?
+ s = list_modified(the_repository, NULL);
+ if (s == NULL)
+ return;
+
+ map = &s->file_map;
+ hashmap_iter_init(map, &iter);
+ while ((entry = hashmap_iter_next(&iter))) {
+ add_choice(&choices, type, entry, NULL);
+ }
+
+ list_and_choose(&choices, &opts);
In what order are these filenames given? Whatever random order the
hashmap happens to store them in?
I vaguely recall in an earlier step the code used hashmap to collect
but at the end produced a sorted list out of the final result.
Shouldn't we be iterating over that sorted list instead? Do we even
need the hashmap at this point?
We actually don't need hashmap at this point, I've changed this so that
list_modified returns produced sorted list.
I've also applied all other suggestions in this message.
Thank you.
+ hashmap_free(&s->file_map, 1);
+ free(s);
+ free_choices(&choices);
Did we just leak opt.header?
+}
diff --git a/add-interactive.h b/add-interactive.h
new file mode 100644
index 0000000000..8ef3d2e82b
--- /dev/null
+++ b/add-interactive.h
@@ -0,0 +1,8 @@
+#ifndef ADD_INTERACTIVE_H
+#define ADD_INTERACTIVE_H
+
+int add_i_config(const char *var, const char *value, void *cbdata);
+
+void add_i_status(void);
+
+#endif
diff --git a/builtin/add--helper.c b/builtin/add--helper.c
index 6a97f0e191..464d2245f3 100644
--- a/builtin/add--helper.c
+++ b/builtin/add--helper.c
@@ -1,6 +1,38 @@
+#include "add-interactive.h"
#include "builtin.h"
+#include "config.h"
+#include "revision.h"
+
+static const char * const builtin_add_helper_usage[] = {
+ N_("git add-interactive--helper <command>"),
+ NULL
+};
+
+enum cmd_mode {
+ DEFAULT = 0,
+ STATUS
+};
int cmd_add__helper(int argc, const char **argv, const char *prefix)
{
+ enum cmd_mode mode = DEFAULT;
+
+ struct option options[] = {
+ OPT_CMDMODE(0, "status", &mode,
+ N_("print status information with diffstat"), STATUS),
+ OPT_END()
+ };
+
+ git_config(add_i_config, NULL);
+ argc = parse_options(argc, argv, NULL, options,
+ builtin_add_helper_usage,
+ PARSE_OPT_KEEP_ARGV0);
+
+ if (mode == STATUS)
+ add_i_status();
+ else
+ usage_with_options(builtin_add_helper_usage,
+ options);
+
return 0;
}
