All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Daniel Ferreira (theiostream)" <bnmvco@gmail.com>
To: Michael Haggerty <mhagger@alum.mit.edu>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Stefan Beller <sbeller@google.com>,
	Duy Nguyen <pclouds@gmail.com>
Subject: Re: [PATCH v7 4/5] dir_iterator: refactor state machine model
Date: Mon, 3 Apr 2017 15:03:43 -0300	[thread overview]
Message-ID: <CAEA2_R+EBbrD1rbcrP598AKJAEZDZfGY-ED8g+ehgAaL9tLG8A@mail.gmail.com> (raw)
In-Reply-To: <76c92397-8e15-b2fb-432f-0ae3d987ffa8@alum.mit.edu>

On Mon, Apr 3, 2017 at 12:36 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> As far as I can tell, you got the logic in this complicated big loop
> correct on the first try (well, if we ignore v6 :-) ), even as you added
> new features. I think that's good evidence that the new structure is
> more comprehensible than the old, plus the new tests probably helped.
> That's a big win!

Thanks for ignoring v6 ;)

Another gain is that the other proposed features (only iterate over
directories, do not recurse into subdirectories) are also quite easy
to add with this new structure.

> It's not ideal that the test code depends on the numerical values of the
> flag constants, and it makes the tests harder to understand. It would be
> better if this program were to accept options like `--pre-order`,
> `--post-order`, etc., as I suggested in an earlier round of review.

Although it does make tests harder to understand, if we were to
specify how to iterate with human-readable flags we'd add the getopt()
+ flag configuration overhead to this helper program to be able to
handle all cases properly. Additionally, new flags added to
dir_iterator would have to edit the test program as well, generating
extra work.

I personally think that the string describing the test in the script
is enough to explain what the flag-as-argument is doing for the sake
of readability. The only gain I see in your suggestion is that we
avoid the programmer committing errors calculating the flag by hand
when writing the test.

That said, I'd appreciate some more thought on this.

> Michael
>

Thanks for the review. I agree with all other points and I'll address
them in a next series.

  reply	other threads:[~2017-04-03 18:04 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-02 20:03 [PATCH v7 0/5] [GSoC] remove_subtree(): reimplement using iterators Daniel Ferreira
2017-04-02 20:03 ` [PATCH v7 1/5] dir_iterator: add tests for dir_iterator API Daniel Ferreira
2017-04-03 22:31   ` Stefan Beller
2017-04-02 20:03 ` [PATCH v7 2/5] remove_subtree(): test removing nested directories Daniel Ferreira
2017-04-03 22:35   ` Stefan Beller
2017-04-02 20:03 ` [PATCH v7 3/5] dir_iterator: add helpers to dir_iterator_advance Daniel Ferreira
2017-04-03 22:48   ` Stefan Beller
2017-04-02 20:03 ` [PATCH v7 4/5] dir_iterator: refactor state machine model Daniel Ferreira
2017-04-03  3:36   ` Michael Haggerty
2017-04-03 18:03     ` Daniel Ferreira (theiostream) [this message]
2017-04-04  6:36       ` Michael Haggerty
2017-04-02 20:03 ` [PATCH v7 5/5] remove_subtree(): reimplement using iterators Daniel Ferreira
2017-04-03  3:42   ` Michael Haggerty

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAEA2_R+EBbrD1rbcrP598AKJAEZDZfGY-ED8g+ehgAaL9tLG8A@mail.gmail.com \
    --to=bnmvco@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=mhagger@alum.mit.edu \
    --cc=pclouds@gmail.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.