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

From: Junio C Hamano <gitster@pobox.com>
To: "Slavica Djukic via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Phillip Wood" <phillip.wood@dunelm.org.uk>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Slavica Djukic" <slawica92@hotmail.com>
Subject: Re: [PATCH v5 03/10] add-interactive.c: implement list_modified
Date: Thu, 21 Feb 2019 12:27:45 -0800	[thread overview]
Message-ID: <xmqqef80ubsu.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <xmqqtvgxt0ze.fsf@gitster-ct.c.googlers.com> (Junio C. Hamano's message of "Thu, 21 Feb 2019 11:06:45 -0800")

Junio C Hamano <gitster@pobox.com> writes:

A few things I missed in the previous message.

>> +	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.

... so that this can become

	if (s->collecting_from_index) {
		entry->index.added = stat.files[i]->added;
		entry->index.deleted = stat.files[i]->deleted;
	} else {
		...
	}

without "else if" and without having to worry about "what if phase
is neither?".

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

And before such a clean-up happens, the implementation of the helper
would want to be improved.  "Does 'rev-parse --verify HEAD' work?"
was an easiest way to see if we are on an unborn branch from a
script that works most of the time, but as we are rewriting it in C,
we should use the more direct and correct API to see if "HEAD" is
a symref, and if it points at a branch that does not yet exist,
which is available in the refs API as resolve_ref_unsafe().

One issue with lazy use of get_oid("HEAD") is that the function
dwims and tries to find HEAD in common hierarchies like
.git/refs/heads/HEAD etc. when .git/HEAD does not work.  We do not
want such a dwimmery when seeing "are we on an unborn branch?".

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

The non-error codepath of this function does not do any output, but
we see two "just emit newline" before returning NUULL to signal an
error to the caller" in the above.  Such a printing from this level
in the callchain (although we haven't seen callers of this function
yet at this point in the series) is wrong.  Presumably, the caller, 
when it obtains a non-NULL 's', does something useful and maybe as a
part of the "useful" thing prints something to the standard output.
Then the caller is also responsible for handling a NULL return.  I.e.
upon seeing such a NULL collection status, if the party that invoked
the caller wants to see a single empty line for whatever reason (which
in turn is a questionable practice, if you ask me, but at this point
in the series we haven't seen what that invoker is doing, so for now
lets assume that it is sane to want to see an empty line), the caller
should do the putchar('\n').

Also, these two "return NULL" leaks 's'.