Re: [PATCH v2 0/7] reflog: introduce subcommand to list reflogs

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

Patrick Steinhardt <ps@pks.im> writes:

>       struct dir_iterator_level {
>       	DIR *dir;
>     + 
>     ++	/*
>     ++	 * The directory entries of the current level. This list will only be
>     ++	 * populated when the iterator is ordered. In that case, `dir` will be
>     ++	 * set to `NULL`.
>     ++	 */
>      +	struct string_list entries;
>      +	size_t entries_idx;

Reads well.  Nice.

>     ++static int next_directory_entry(DIR *dir, const char *path,
>     ++				struct dirent **out)
>     ++{
>     ++	struct dirent *de;
>     ++
>     ++repeat:
>     ++	errno = 0;
>     ++	de = readdir(dir);
>     ++	if (!de) {
>     ++		if (errno) {
>     ++			warning_errno("error reading directory '%s'",
>     ++				      path);
>     ++			return -1;
>     ++		}
>     ++
>     ++		return 1;
>     ++	}
>     ++
>     ++	if (is_dot_or_dotdot(de->d_name))
>     ++		goto repeat;
>     ++
>     ++	*out = de;
>     ++	return 0;
>     ++}

Very nice to encapsulate the common readdir() loop into this helper.

> 3:  e4e4fac05c ! 3:  32b24a3d4b refs/files: sort reflogs returned by the reflog iterator
>     @@ refs/files-backend.c: static struct ref_iterator *reflog_iterator_begin(struct r
>       	iter->dir_iterator = diter;
>       	iter->ref_store = ref_store;
>       	strbuf_release(&sb);
>     +@@ refs/files-backend.c: static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st
>     + 		return reflog_iterator_begin(ref_store, refs->gitcommondir);
>     + 	} else {
>     + 		return merge_ref_iterator_begin(
>     +-			0, reflog_iterator_begin(ref_store, refs->base.gitdir),
>     ++			1, reflog_iterator_begin(ref_store, refs->base.gitdir),
>     + 			reflog_iterator_begin(ref_store, refs->gitcommondir),
>     + 			reflog_iterator_select, refs);
>     + 	}

This hunk is new.  Is there a downside to force merged iterators to
always be sorted?  The ones that are combined are all sorted so it
is natural to force sorting like this code does?  It might deserve
explaining, and would certainly help future readers who runs "blame"
on this code to figure out what made us think always sorting is a
good direction forward.

> -:  ---------- > 4:  4254f23fd4 refs: always treat iterators as ordered

This one is new, and deserves a separate review.

> 4:  be512ef268 ! 5:  240334df6c refs: drop unused params from the reflog iterator callback
>     @@ refs/reftable-backend.c: static int reftable_reflog_iterator_advance(struct ref_
>       	}
>      @@ refs/reftable-backend.c: static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl
>       	iter = xcalloc(1, sizeof(*iter));
>     - 	base_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable, 1);
>     + 	base_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable);
>       	iter->refs = refs;
>      -	iter->base.oid = &iter->oid;
>       
> 5:  a7459b9483 = 6:  7928661318 refs: stop resolving ref corresponding to reflogs
> 6:  cddb2de939 = 7:  d7b9cff4c3 builtin/reflog: introduce subcommand to list reflogs

Looking good from a cursory read.

Thanks for a quick reroll.