On Thu, Jun 27, 2019 at 01:37:58AM -0400, Eric Sunshine wrote:
> On Wed, Jun 26, 2019 at 7:51 PM Emily Shaffer <[email protected]> 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 <[email protected]>
> > ---
> > 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
