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

[Slavica Djukic <slavicadj.ip2018@gmail.com>]

On 21-Feb-19 9:27 PM, Junio C Hamano wrote:
> 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').


Caller of list_modified is add_i_status in the later commit, and yes
it should definitely take care of this newline.
It should print it after calling list_modified *and* list_and_choose.

It is done in the same way in the Perl script:
sub status_cmd {
     list_and_choose({ LIST_ONLY => 1, HEADER => $status_head },
             list_modified());
     print "\n";
}

I overlooked this because print_modified (now list_modified) in
the previous iteration was equivalent of status command, and
everything was handled there.
After refactoring, I missed to pick up those printfs.


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


I guess I missed some more things. Thank you.


>