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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 18, 2017, 04:35 UTC
Message-ID: <xmqq4lxmfi4b.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1492462296-4990-5-git-send-email-bnmvco@gmail.com>

Daniel Ferreira <bnmvco@gmail.com> writes:

Show 5 quoted lines

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

Show 12 quoted lines

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