| Message ID | 20190701202014.34480-1-emilyshaffer@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | example implementation of revwalk tutorial | expand |
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 > > >
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 > > > > > >
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