From: Slavica Djukic Date: Fri, 22 Feb 2019 12:18:01 GMT Subject: Re: [PATCH v5 03/10] add-interactive.c: implement list_modified Message-ID: In-Reply-To: On 21-Feb-19 9:27 PM, Junio C Hamano wrote: > Junio C Hamano 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. >