Re: [PATCH v4] documentation: add tutorial for revision walking

From: Emily Shaffer <emilyshaffer@google.com>
Date: Aug 14, 2019, 18:33 UTC
Message-ID: <20190814183328.GA40797@google.com>
In-Reply-To: <xmqqwofohjgv.fsf@gitster-ct.c.googlers.com>

On Wed, Aug 07, 2019 at 12:19:12PM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> Emily Shaffer <emilyshaffer@google.com> writes:
> 
> > Since v3, only a couple of minor changes from Jonathan Tan - thanks.
> >
> > I'm dropping the updates for the RFC set, since they're incremental from
> > now. Next time you all see them they will be in a form which we would
> > hope to maintain over a long period of time, checked into source -
> > likely in the form of an mbox or dir of .patch file.
> 
> Sure.
> 
> > I think the tutorial itself is pretty much ready...
> 
> A few comments after skimming this round; none of them may be a show
> stopper, but others may have different opinions.
> 
>  - There is still a leftover "TODO: checking CLI options"; is that
>    something we postpone teaching?

Yeah, I think it would make more sense to include it into the other one (my first contribution) instead.

Show 5 quoted lines

> 
>  - This is an offtopic tangent, but "my first contribution" being an
>    addition of an entire command probably mistakenly raised the bar
>    to contributors a bit too much.  A typical first contribution is
>    a typofix, fix to a small (e.g. off-by-one) bug, etc.

Sure, I agree with that; but I think the larger (though less common) project of new command allowed me to explain more about the architecture of the project overall than a typo fix would. Maybe there's a clearer name to use?

Show 30 quoted lines

> 
>  - For a revision walk tutorial, not seeing any mention of pathspec
>    filtering and associated history simplification is somewhat
>    unsatisfying.  On the other hand, I expect that enumeration of
>    objects contained within commits is (hence various --filter
>    options are) totally uninteresting for end users who run the
>    command interactively and view the output of the command on
>    screen.
> 
>  - Enumeration of objects is useful at least in three places in Git:
>    (1) enumerate objects to be packed, with some filtering based on
>    various criteria; (2) enumerate objects that are reachable from
>    anchor points like refs, index, reflog, etc., to discover what
>    are not reachable and can be discarded; (3) enumerate objects
>    that still matter (i.e. the opposite of (2)) so that they can be
>    fed to validation mechanisms (e.g. "cat-file --batch-check").  If
>    this were titled "My first object enumeration", the reaction led
>    to the latter half of the previous point may not have occurred
>    (pathspec filtering would still be relevant, but not as
>    much---for packing to create a narrow clone, you do not want to
>    use pathspec with history simplification, but you would want to
>    use something more like "root and intermediate trees that are
>    necessary to cover these paths" filter in the
>    list--objects-filter layer).
> 
> And from the point of view of the last item, I would think the new
> document is covering a need that is different from what we
> traditionally would call "revision walk", which is more about "git
> log", not the upstream of "git pack-objects", which this new
> document is more geared towards.

Hmmm. It sounds like you're saying:

- This object covers walking objects, which is surprising since it's
  titled about "revision walks". Revision walks are more about commits
  ("git log").
- Using grep on objects doesn't make any sense.
- Other filters (like pathspecs) which do make sense for object walks
  aren't covered.

Show 6 quoted lines

> 
> Unless "git walken" is an exercise of how to write code that does
> random thing, use of --grep filter however may be out of place,
> though.  I do not offhand think of a use case where --grep would be
> useful in the revision walk/object enumeration that is placed
> upstream of "pack-objects".

In this case, it might make sense to do one of these things:

- Apply the grep filter to the commit walk, and apply a more interesting
  object filter to the object walk.

Or,

- Choose a different kind of filter which is interesting when applied to
  commits alone _and_ all objects.

In the interest of covering more ground with this kind of tutorial, I'd lean more towards the former.

It's possible that the added scope will make the document large enough that we'd rather split it into two (one for "git log"-ish, one for "git pack-objects"-ish). I think that's fine if we end up there.

Thanks. I hope to get back to this soon...

 - Emily