Re: [RFC PATCH v2 10/13] walken: add unfiltered object walk from HEAD

[Emily Shaffer <emilyshaffer@google.com>]

On Thu, Jun 27, 2019 at 01:37:58AM -0400, Eric Sunshine wrote:
> On Wed, Jun 26, 2019 at 7:51 PM Emily Shaffer <emilyshaffer@google.com> wrote:
> > Provide a demonstration of a revision walk which traverses all types of
> > object, not just commits. This type of revision walk is used for
> > operations such as creating packfiles and performing fetches or clones,
> > so it's useful to teach new developers how it works. For starters, only
> > demonstrate the unfiltered version, as this will make the tutorial
> > easier to follow.
> > [...]
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> > diff --git a/builtin/walken.c b/builtin/walken.c
> > @@ -103,6 +109,65 @@ static int git_walken_config(const char *var, const char *value, void *cb)
> > +static void walken_show_commit(struct commit *cmt, void *buf)
> > +{
> > +       commit_count++;
> > +}
> > +
> > +static void walken_show_object(struct object *obj, const char *str, void *buf)
> > +{
> > +       switch (obj->type) {
> > +       [...]
> > +       case OBJ_TAG:
> > +               tag_count++;
> > +               break;
> > +       case OBJ_COMMIT:
> > +               die(_("unexpectedly encountered a commit in "
> > +                     "walken_show_object\n"));
> > +               commit_count++;
> 
> The "commit_count++" line is not only dead code, but it also confuses
> the reader (or makes the reader think that the author of this code
> doesn't understand C programming). You should drop this line.

Ow, yes. Removed. This is stale (pre-die()).

> 
> > +               break;
> > +       default:
> > +               die(_("unexpected object type %s\n"), type_name(obj->type));
> > +               break;
> 
> Likewise, this "break" (and the one in the OBJ_COMMIT case) are dead
> code which should be dropped to avoid confusing the reader.

Done.

> 
> Don't localize the die() message via _() here or in the preceding
> OBJ_COMMIT case.

I'm a little surprised by that. Is it because die() is expected to only
be seen by the developer? It seems like a poor user experience if
someone in non-English locale encounters a bug that Git team didn't
find, and needed to try to translate the English die() string and figure
out if a workaround is possible.

> 
> The two die() messages are unnecessarily dissimilar. Try to unify them
> so that they read in the same way.

I'm a little surprised by this too; it seems to me the root cause of
each would be different. In the former case, I'd guess that
traverse_commit_list()'s behavior changed, and in the latter case I'd
guess that a new object type was recently added to the model. Can you
help me understand the motivation for making the messages similar?

(I don't think, though, that I did a good job of indicating either root
cause in the die() messages as they are now.)

> 
> > +       }
> > +}> @@ -113,15 +178,15 @@ static void walken_commit_walk(struct rev_info *rev)
> >         /*
> > -         * prepare_revision_walk() gets the final steps ready for a revision
> > +        * prepare_revision_walk() gets the final steps ready for a revision
> >          * walk. We check the return value for errors.
> > -         */
> > +        */
> >         /*
> > -         * Now we can start the real commit walk. get_revision grabs the next
> > +        * Now we can start the real commit walk. get_revision grabs the next
> >          * revision based on the contents of rev.
> >          */
> 
> I think these are just correcting whitespace/indentation errors I
> pointed out in an earlier patch (so they are unnecessary noise in this
> patch).

ACK