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

[Emily Shaffer <emilyshaffer@google.com>]

On Thu, Jun 27, 2019 at 08:48:31PM -0400, Eric Sunshine wrote:
> On Thu, Jun 27, 2019 at 6:31 PM Emily Shaffer <emilyshaffer@google.com> wrote:
> > On Thu, Jun 27, 2019 at 01:37:58AM -0400, Eric Sunshine wrote:
> > > 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?
> 
> Sorry, I was reading those as BUG(), not die(), and we don't localize
> BUG() text. But, why aren't those BUG()? Can those cases arise in
> practice? (Genuine question; I haven't familiarized myself with that
> code yet.)
> 
> If they legitimately should be die(), then ignore my comment about not
> localizing them.

Hmmm. Yeah, I'll switch them to BUG() - I think there are other
instances of die() in the example and it'd be good to describe yet
another way of reporting behavior.

> 
> > > 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?
> 
> Both causes you describe here sound like BUG() cases, not die(). If
> I'm understanding correctly, they could only trigger if someone made
> some breaking or behavior changing modifications within Git and failed
> to update all the code in the project impacted by the change. In other
> words, these can't be triggered by user input, hence they would be
> BUG()s indicating that a Git developer needs to fix the code.
> 
> As for the messages themselves, I was referring to the grammatical
> dissimilarity of "unexpectedly" and "unexpected", and I also don't
> understand why one messages mentions walken_show_object() explicitly,
> whereas the other does not.

I see - ok, I have reworded.


Thanks!
 - Emily