[RFC,v3,00/13] example implementation of revwalk tutorial
mbox series

Message ID 20190701202014.34480-1-emilyshaffer@google.com
Headers show
Series
  • example implementation of revwalk tutorial
Related show

Message

Emily Shaffer July 1, 2019, 8:20 p.m. UTC
Since v2, mostly reworded comments, plus fixed the issues mentioned in
the tutorial itself. Thanks Eric for the review.

Emily Shaffer (13):
  walken: add infrastructure for revwalk demo
  walken: add usage to enable -h
  walken: add placeholder to initialize defaults
  walken: add handler to git_config
  walken: configure rev_info and prepare for walk
  walken: perform our basic revision walk
  walken: filter for authors from gmail address
  walken: demonstrate various topographical sorts
  walken: demonstrate reversing a revision walk list
  walken: add unfiltered object walk from HEAD
  walken: add filtered object walk
  walken: count omitted objects
  walken: reverse the object walk order

 Makefile         |   1 +
 builtin.h        |   1 +
 builtin/walken.c | 297 +++++++++++++++++++++++++++++++++++++++++++++++
 git.c            |   1 +
 4 files changed, 300 insertions(+)
 create mode 100644 builtin/walken.c

Comments

Johannes Schindelin July 25, 2019, 9:25 a.m. UTC | #1
Hi Emily,

On Mon, 1 Jul 2019, Emily Shaffer wrote:

> Since v2, mostly reworded comments, plus fixed the issues mentioned in
> the tutorial itself. Thanks Eric for the review.
>
> Emily Shaffer (13):
>   walken: add infrastructure for revwalk demo
>   walken: add usage to enable -h
>   walken: add placeholder to initialize defaults
>   walken: add handler to git_config
>   walken: configure rev_info and prepare for walk
>   walken: perform our basic revision walk
>   walken: filter for authors from gmail address
>   walken: demonstrate various topographical sorts
>   walken: demonstrate reversing a revision walk list
>   walken: add unfiltered object walk from HEAD
>   walken: add filtered object walk
>   walken: count omitted objects
>   walken: reverse the object walk order
>
>  Makefile         |   1 +
>  builtin.h        |   1 +
>  builtin/walken.c | 297 +++++++++++++++++++++++++++++++++++++++++++++++

Since this is not really intended to be an end user-facing command, I
think it should not become a built-in, to be carried into every Git
user's setup.

Instead, I would recommend to implement this as a test helper.

This would have the following advantages:

- it won't clutter the end user installations,

- it will still be compile-tested with every build (guaranteeing that
  the tutorial won't become stale over time as so many other tutorials),

- it really opens the door very wide to follow up with another tutorial
  to guide new contributors to write stellar regression tests.

Thanks,
Dscho

>  git.c            |   1 +
>  4 files changed, 300 insertions(+)
>  create mode 100644 builtin/walken.c
>
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
>
>
Emily Shaffer Aug. 6, 2019, 11:13 p.m. UTC | #2
On Thu, Jul 25, 2019 at 11:25:02AM +0200, Johannes Schindelin wrote:
> Hi Emily,
> 
> On Mon, 1 Jul 2019, Emily Shaffer wrote:
> 
> > Since v2, mostly reworded comments, plus fixed the issues mentioned in
> > the tutorial itself. Thanks Eric for the review.
> >
> > Emily Shaffer (13):
> >   walken: add infrastructure for revwalk demo
> >   walken: add usage to enable -h
> >   walken: add placeholder to initialize defaults
> >   walken: add handler to git_config
> >   walken: configure rev_info and prepare for walk
> >   walken: perform our basic revision walk
> >   walken: filter for authors from gmail address
> >   walken: demonstrate various topographical sorts
> >   walken: demonstrate reversing a revision walk list
> >   walken: add unfiltered object walk from HEAD
> >   walken: add filtered object walk
> >   walken: count omitted objects
> >   walken: reverse the object walk order
> >
> >  Makefile         |   1 +
> >  builtin.h        |   1 +
> >  builtin/walken.c | 297 +++++++++++++++++++++++++++++++++++++++++++++++
> 
> Since this is not really intended to be an end user-facing command, I
> think it should not become a built-in, to be carried into every Git
> user's setup.

It's not intended to be checked into Git source as-is.

> 
> Instead, I would recommend to implement this as a test helper.

I'm not sure I follow how you imagine this looking, but the drawback I
see of implementing this in a different way than you would typically do
when writing a real feature for the project is that it becomes less
useful as a reference for new contributors.

> 
> This would have the following advantages:
> 
> - it won't clutter the end user installations,
> 
> - it will still be compile-tested with every build (guaranteeing that
>   the tutorial won't become stale over time as so many other tutorials),

This part of your suggestion appeals to me; so I'm really curious how
you would do it. Do you have something else written in the way you're
suggesting in mind?

> 
> - it really opens the door very wide to follow up with another tutorial
>   to guide new contributors to write stellar regression tests.
> 
> Thanks,
> Dscho
> 
> >  git.c            |   1 +
> >  4 files changed, 300 insertions(+)
> >  create mode 100644 builtin/walken.c
> >
> > --
> > 2.22.0.410.gd8fdbe21b5-goog
> >
> >
> >
Johannes Schindelin Aug. 8, 2019, 7:19 p.m. UTC | #3
Hi Emily,

On Tue, 6 Aug 2019, Emily Shaffer wrote:

> On Thu, Jul 25, 2019 at 11:25:02AM +0200, Johannes Schindelin wrote:
> >
> > On Mon, 1 Jul 2019, Emily Shaffer wrote:
> >
> > > Since v2, mostly reworded comments, plus fixed the issues mentioned in
> > > the tutorial itself. Thanks Eric for the review.
> > >
> > > Emily Shaffer (13):
> > >   walken: add infrastructure for revwalk demo
> > >   walken: add usage to enable -h
> > >   walken: add placeholder to initialize defaults
> > >   walken: add handler to git_config
> > >   walken: configure rev_info and prepare for walk
> > >   walken: perform our basic revision walk
> > >   walken: filter for authors from gmail address
> > >   walken: demonstrate various topographical sorts
> > >   walken: demonstrate reversing a revision walk list
> > >   walken: add unfiltered object walk from HEAD
> > >   walken: add filtered object walk
> > >   walken: count omitted objects
> > >   walken: reverse the object walk order
> > >
> > >  Makefile         |   1 +
> > >  builtin.h        |   1 +
> > >  builtin/walken.c | 297 +++++++++++++++++++++++++++++++++++++++++++++++
> >
> > Since this is not really intended to be an end user-facing command, I
> > think it should not become a built-in, to be carried into every Git
> > user's setup.
>
> It's not intended to be checked into Git source as-is.

Then it runs the very real danger of becoming stale: we do _not_
guarantee a stable API, not even an internal one.

> > Instead, I would recommend to implement this as a test helper.
>
> I'm not sure I follow how you imagine this looking, but the drawback I
> see of implementing this in a different way than you would typically do
> when writing a real feature for the project is that it becomes less
> useful as a reference for new contributors.

To the contrary. Some code in `t/helper/` is intended to test
functionality in a way that is copy-editable.

Your use case strikes me a perfect example for such a test helper:

- It guarantees that the example is valid,
- It demonstrates how to use the API,
- In case the API changes, the changes to the helper will inform
  contributors how to change their copy-edited versions

> > This would have the following advantages:
> >
> > - it won't clutter the end user installations,
> >
> > - it will still be compile-tested with every build (guaranteeing that
> >   the tutorial won't become stale over time as so many other tutorials),
>
> This part of your suggestion appeals to me; so I'm really curious how
> you would do it. Do you have something else written in the way you're
> suggesting in mind?

I looked at `t/helper/test-hashmap.c` and it looks _almost_ like a
perfect example for what I have in mind: it uses a given API,
demonstrates how to use it properly, and is copy-editable.

Ciao,
Dscho