Re: [PATCH v3 03/11] built-in add -i: implement the `status` command

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 27, 2019, 10:04 UTC
Message-ID: <nycvar.QRO.7.76.6.1908271123570.46@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqsgqmoyz1.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Wed, 31 Jul 2019, Junio C Hamano wrote:

Show 34 quoted lines

> "Daniel Ferreira via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +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.

Yep.

Show 13 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...

Ah, that's my mistake. I had converted the `add` and `del` fields from the `unsigned long` data type that the 20th century wanted back.

Show 19 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.

Yep, this is the Perl version:

sub status_cmd {
        list_and_choose({ LIST_ONLY => 1, HEADER => $status_head },
			list_modified());
	print "\n";
}

Show 45 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?

Yep, I added a call to the now-public `free_diffstat_info()` function.

Thanks, Dscho