Re: [PATCH v3 03/11] built-in add -i: implement the `status` command
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 31, 2019, 18:12 UTC
- Message-ID
- <xmqqsgqmoyz1.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <b93b055ebeef98be11a1658453df6f85fed87347.1563289115.git.gitgitgadget@gmail.com>
"Daniel Ferreira via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 28 quoted lines
> +struct item {
> + const char *name;
> +};
> +
> +struct list_options {
> + const char *header;
> + void (*print_item)(int i, struct item *item, void *print_item_data);
> + void *print_item_data;
> +};
> +
> +struct adddel {
> + uintmax_t add, del;
> + unsigned seen:1, binary:1;
> +};
> +
> +struct file_list {
> + struct file_item {
> + struct item item;
> + struct adddel index, worktree;
> + } **file;
> + size_t nr, alloc;
> +};
> +
> +struct pathname_entry {
> + struct hashmap_entry ent;
> + size_t index;
> + char pathname[FLEX_ARRAY];
> +};All of the above are named too generic but assuming that add-i will stay to be a single file and these names will never leak outside the file to become global, it would be perfectly fine.
Show 11 quoted lines
> +static void populate_wi_changes(struct strbuf *buf,
> + struct adddel *ad, const char *no_changes)
> +{
> + if (ad->binary)
> + strbuf_addstr(buf, _("binary"));
> + else if (ad->seen)
> + strbuf_addf(buf, "+%"PRIuMAX"/-%"PRIuMAX,
> + (uintmax_t)ad->add, (uintmax_t)ad->del);
> + else
> + strbuf_addstr(buf, no_changes);
> +}I offhand do not see the need for (uintmax_t) casts here...
Show 11 quoted lines
> +static int run_status(struct repository *r, const struct pathspec *ps,
> + struct file_list *files, struct list_options *opts)
> +{
> + reset_file_list(files);
> +
> + if (get_modified_files(r, files, ps) < 0)
> + return -1;
> +
> + if (files->nr)
> + list((struct item **)files->file, files->nr, opts);
> + putchar('\n');So, if there is anything to list, we show list() and then add an empty line; if there is nothing to list, we show an empty line anyway?
As long as that matches the current scripted "add -i", it's perfectly fine. It's just that the code structure above looked somewhat odd.
Show 42 quoted lines
> +static void collect_changes_cb(struct diff_queue_struct *q,
> + struct diff_options *options,
> + void *data)
> +{
> + struct collection_status *s = data;
> + struct diffstat_t stat = { 0 };
> + int i;
> +
> + if (!q->nr)
> + return;
> +
> + compute_diffstat(options, &stat, q);
> +
> + for (i = 0; i < stat.nr; i++) {
> + const char *name = stat.files[i]->name;
> + int hash = strhash(name);
> + struct pathname_entry *entry;
> + size_t file_index;
> + struct file_item *file;
> + struct adddel *adddel;
> +
> + entry = hashmap_get_from_hash(&s->file_map, hash, name);
> + if (entry)
> + file_index = entry->index;
> + else {
> + FLEX_ALLOC_STR(entry, pathname, name);
> + hashmap_entry_init(entry, hash);
> + entry->index = file_index = s->list->nr;
> + hashmap_add(&s->file_map, entry);
> +
> + add_file_item(s->list, name);
> + }
> + file = s->list->file[file_index];
> +
> + adddel = s->phase == FROM_INDEX ? &file->index : &file->worktree;
> + adddel->seen = 1;
> + adddel->add = stat.files[i]->added;
> + adddel->del = stat.files[i]->deleted;
> + if (stat.files[i]->is_binary)
> + adddel->binary = 1;
> + }
> +}Would resources held in the "stat" structure leak at the end of this function?