Re: [PATCH v5 03/10] add-interactive.c: implement list_modified

[Junio C Hamano <gitster@pobox.com>]

"Slavica Djukic via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/add-interactive.c b/add-interactive.c
> new file mode 100644
> index 0000000000..7d096207d4
> --- /dev/null
> +++ b/add-interactive.c
> @@ -0,0 +1,188 @@
> +#include "cache.h"
> +#include "commit.h"
> +#include "color.h"
> +#include "config.h"
> +#include "diffcore.h"
> +#include "revision.h"
> +
> +enum collection_phase {
> +	WORKTREE,
> +	INDEX
> +};
> +
> +struct file_stat {
> +	struct hashmap_entry ent;
> +	struct {
> +		uintmax_t added, deleted;
> +	} index, worktree;
> +	char name[FLEX_ARRAY];
> +};
> +
> +struct collection_status {
> +	enum collection_phase phase;
> +
> +	const char *reference;
> +	struct pathspec pathspec;
> +
> +	struct hashmap file_map;
> +};
> +
> +static int hash_cmp(const void *unused_cmp_data, const void *entry,
> +		    const void *entry_or_key, const void *keydata)
> +{
> +	const struct file_stat *e1 = entry, *e2 = entry_or_key;
> +	const char *name = keydata ? keydata : e2->name;
> +
> +	return strcmp(e1->name, name);
> +}

This one is used with hashmap API, which gives unfortunate name
"cmp_fn" to its callback, which does not necessarily need to
compare.  It only needs to say "are they different (yes/no)?"
without having to order them (iow, it would not break anything if
the above is changed to return strcmp(name, e1->name)).

Rather than naming the callback after the name of the API that calls
it (i.e. "hash"), it would make it more descriptive if it were named
after what is being compared for equality, in this case, pathname.
Perhaps "pathname_equal()" or something?

> +static int alphabetical_cmp(const void *a, const void *b)
> +{
> +	struct file_stat *f1 = *((struct file_stat **)a);
> +	struct file_stat *f2 = *((struct file_stat **)b);
> +
> +	return strcmp(f1->name, f2->name);
> +}

This one is used with qsort(), and there are two things you may want
the function name to tell the readers.  "what is compared" and "how
it is compared".  Alphabetical is a bit fuzzy (different people sort
"a.txt", "A.txt", and "b.txt" in different order) and it gives a
piece of information of secondary importance (i.e. it only starts to
make sense after you know you are ordering pathnames), so perhaps
"pathname_cmp()" or something may be a better name.

> +
> +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);

Remember what I said on patch 01/10?  This callback function is
designed to take arbitrary diff_queue_struct q as its parameter and
work on it, but compute_diffstat() is misdesigned and cannot work on
anything but the singleton diff_queued_diff, which is an impedance
mismatch that can easily be corrected by going back to 01/10 and
fixing it.

> +	for (i = 0; i < stat.nr; i++) {
> +		struct file_stat *entry;
> +		const char *name = stat.files[i]->name;
> +		unsigned int hash = strhash(name);
> +
> +		entry = hashmap_get_from_hash(&s->file_map, hash, name);
> +		if (!entry) {
> +			FLEX_ALLOC_STR(entry, name, name);
> +			hashmap_entry_init(entry, hash);
> +			hashmap_add(&s->file_map, entry);
> +		}

The path may already be in the collection_status.file_map from the
previous run when "diff-index --cached" is run, in which case we avoid
adding it twice, which makes sense.

> +		if (s->phase == WORKTREE) {
> +			entry->worktree.added = stat.files[i]->added;
> +			entry->worktree.deleted = stat.files[i]->deleted;
> +		} else if (s->phase == INDEX) {
> +			entry->index.added = stat.files[i]->added;
> +			entry->index.deleted = stat.files[i]->deleted;
> +		}

As the set of phases will not going to grow, not having the final
'else BUG("phase is neither WORKTREE nor INDEX");' here is OK.

But stepping back a bit, if we know we will not grow the phases,
then it may be simpler *and* equally descriptive to rename .phase
field to a boolean ".collecting_from_index" that literally means
"are we collecting from the index?" and that way we can also get rid
of the enum.

> +	}
> +}
> +
> +static void collect_changes_worktree(struct collection_status *s)
> +{
> +	struct rev_info rev;
> +
> +	s->phase = WORKTREE;
> +
> +	init_revisions(&rev, NULL);
> +	setup_revisions(0, NULL, &rev, NULL);
> +
> +	rev.max_count = 0;

The convention to (ab)use the max_count field to specify the
unmerged stage, against which the working tree file is compared for
an unmerged path, is one of the older quirks of the run_diff_files()
API.  "git diff-files" itself when running cmd_diff_files() leaves
rev.max_count untouched to get a normal output (as opposed to the
case when it is told to do --base/--ours/--theirs), so it ends up
passing -1 in this field in such a case.

This explicit assignment to 0 must be explained with in-code comment
why we want to do so.

> +	rev.diffopt.flags.ignore_dirty_submodules = 1;

OK, this matches what "sub list_modified" does.

> +	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
> +	rev.diffopt.format_callback = collect_changes_cb;
> +	rev.diffopt.format_callback_data = s;
> +	run_diff_files(&rev, 0);
> +}
> +
> +static void collect_changes_index(struct collection_status *s)
> +{
> +	struct rev_info rev;
> +	struct setup_revision_opt opt = { 0 };
> +
> +	s->phase = INDEX;
> +
> +	init_revisions(&rev, NULL);
> +	opt.def = s->reference;
> +	setup_revisions(0, NULL, &rev, &opt);
> +
> +	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
> +	rev.diffopt.format_callback = collect_changes_cb;
> +	rev.diffopt.format_callback_data = s;
> +
> +	run_diff_index(&rev, 1);
> +}
> +
> +static int is_inital_commit(void)
> +{
> +	struct object_id sha1;
> +	if (get_oid("HEAD", &sha1))

Using the newer get_oid() API (as opposed to get_sha1() API) while
calling the variable that receives its result "sha1" is an
oxymoron.  Most users of the API call it "oid", I think.

> +		return 1;
> +	return 0;
> +}

inital???

Grep for "unborn" in the codebase.  It probably makes sense to call
it on_unborn_branch() instead.

	static int on_unborn_branch(void)
	{
		struct object_id oid;
		return !!get_oid("HEAD", &oid);
	}

Eventually, the users of "unborn" in sequencer.c and builtin/reset.c
may want to share the implementation but the helper is so small that
we probably should not worry about it until the topic is done and
leave it for a later clean-up.

> +static const char *get_diff_reference(void)
> +{
> +	if(is_inital_commit())

Style.  Do not forget SP between "if" and "(".

> +		return empty_tree_oid_hex();
> +	return "HEAD";
> +}

Although probably

	return on_unborn_branch() ? empty_tree_oid_hex() : "HEAD";

is clear enough.

> +static void filter_files(const char *filter, struct hashmap *file_map,
> +			 struct file_stat **files)
> +{
> +
> +	for (int i = 0; i < hashmap_get_size(file_map); i++) {

The declaration of "int i" there is one of the C99ism we do not use,
IIRC ("git grep 'for (int ' \*.c" comes up empty).

> +		struct file_stat *f = files[i];
> +
> +		if ((!(f->worktree.added || f->worktree.deleted)) &&
> +		   (!strcmp(filter, "file-only")))
> +				hashmap_remove(file_map, f, NULL);
> +
> +		if ((!(f->index.added || f->index.deleted)) &&
> +		   (!strcmp(filter, "index-only")))
> +				hashmap_remove(file_map, f, NULL);
> +	}

As part of the "interactive" machinery, obvious inefficiency in the
code may not matter that much, but having to make exact string
comparison for an unmodified parameter twice for each iteration does
not look like showing a good taste.

What is the earliest time the code can decide that it is asked to do
a "file-only" filtering?  What I am trying to get at is that if we
know early enough, instead of grabbing outputs from "diff-files" and
"diff-index --cached", only to drop such entries that do not have
output in "diff-files", we could run only "diff-index --cached"
without spending any cycle to run "diff-files" in the first place.

Even if such a reorganizing of the callflow is impossible (I didn't
check), at least a micro optimization would be:

	int i;
	int file_only = !strcmp(filter, "file-only");
	int index_only = !strcmp(filter, "index-only");

	for (i = 0; i < hashmap_get_size(file_map), i++) {
		struct file_stat *f = files[i];
		if (file_only && !(f->worktree.added || f->worktree.deleted))
			hashmap_remove(file_map, f, NULL);
		...
	}



> +}
> +
> +static struct collection_status *list_modified(struct repository *r, const char *filter)
> +{
> +	int i = 0;
> +	struct collection_status *s = xcalloc(1, sizeof(*s));
> +	struct hashmap_iter iter;
> +	struct file_stat **files;
> +	struct file_stat *entry;
> +
> +	if (repo_read_index(r) < 0) {
> +		printf("\n");
> +		return NULL;
> +	}
> +
> +	s->reference = get_diff_reference();
> +	hashmap_init(&s->file_map, hash_cmp, NULL, 0);
> +
> +	collect_changes_worktree(s);
> +	collect_changes_index(s);
> +
> +	if (hashmap_get_size(&s->file_map) < 1) {
> +		printf("\n");
> +		return NULL;
> +	}
> +
> +	hashmap_iter_init(&s->file_map, &iter);
> +
> +	files = xcalloc(hashmap_get_size(&s->file_map), sizeof(struct file_stat *));
> +	while ((entry = hashmap_iter_next(&iter))) {
> +		files[i++] = entry;
> +	}
> +	QSORT(files, hashmap_get_size(&s->file_map), alphabetical_cmp);
> +
> +	if (filter)
> +		filter_files(filter, &s->file_map, files);

Ahh, this answers my previous question, no?  The "filter" parameter
given to this function will never change, so we could inspect it
even before we call collect_chagnes_{worktree,index}().  If the
caller says "I want to see only the paths that appear in the
diff-files output", there is no need to call collect_changes_index()
at all, no?

> +
> +	free(files);
> +	return s;
> +}