Re: [PATCH v9 4/5] dir_iterator: refactor state machine model

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

Daniel Ferreira <bnmvco@gmail.com> writes:

> Perform major refactor of dir_iterator_advance(). dir_iterator has
> ceased to rely on a convoluted state machine mechanism of two loops and
> two state variables (level.initialized and level.dir_state). This serves
> to ease comprehension of the iterator mechanism and ease addition of new
> features to the iterator.

I called 3/5 a "refactoring"; on the other hand, I wouldn't call
this refactoring--it is a major rewrite and/or enhancement.

Drop "convoluted...loops and variables"; instead spend the same
number of bits to explain how updated code is easier to follow.

> Add an option for the dir_iterator API to iterate over the root
> directory (the one it was initialized with) as well.

I had trouble understanding what you wanted to say with "iterate
over the root" because the phrase sounded, at least to me, to mean
"opendir the root directory and repeatedly issue readdir() calls,
returning each entry returned by them", and making that optional did
not make any sense to me.  I had to cheat by looking at the test
script to realize that you meant that the root directory that the
iterator is told to iterate over itself is also included in the
result, as opposed to the usual expectation that iterator would just
iterate over the contents of that directory recursively.  Perhaps

    Add an option for the API to also include the initial directory
    in the result.

or something like that?

By the way, some other topic already took t0065, so I moved your
t0065 over to t0066 while queuing.

> +/*
> + * This function uses a state machine with the following states:
> + * -> DIR_STATE_PUSHED: the directory has been pushed to the
> + * iterator traversal tree.
> + * -> DIR_STATE_PRE_ITERATION: the directory is *NOT* initialized. The
> + * dirpath has already been returned if pre-order traversal is set.
> + * -> DIR_STATE_ITERATING: the directory is initialized. We are traversing
> + * through it.
> + * -> DIR_STATE_POST_ITERATION: the directory has been iterated through.
> + * We are ready to close it.
> + * -> DIR_STATE_EXHAUSTED: the directory is closed and ready to be popped.
> + */

These "->" somehow makes it very hard to read; are these meant to be
bullets in a 5-item bulleted list, or do they mean more than that
(e.g. as an arrow, you are trying to say from state PUSHED there is
a transition to PRE_ITERATION, etc.)?

PRE_ITERATION says "*NOT* initialized"; has the directory been
already initialized in PUSHED state?  Instead of saying "*NOT*", it
may make it clearer if you say when it is initialized instead.