Re: [RFC PATCH v2 10/13] walken: add unfiltered object walk from HEAD
- From
Emily Shaffer <emilyshaffer@google.com>
- Date
- Jun 27, 2019, 22:31 UTC
- Message-ID
- <20190627223110.GH245941@google.com>
- In-Reply-To
- <CAPig+cR7rHokaTtZcJJg8trJ14xO3hdeWHqLnB4aOnEUak051w@mail.gmail.com>
On Thu, Jun 27, 2019 at 01:37:58AM -0400, Eric Sunshine wrote:
Show 32 quoted lines
> 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()).
Show 8 quoted lines
>
> > + 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.)
Show 18 quoted lines
> > > + } > > +}> @@ -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