git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Emily Shaffer <emilyshaffer@google.com>
To: Josh Steadmon <steadmon@google.com>,
	Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org
Subject: Re: [PATCH] documentation: add tutorial for revision walking
Date: Mon, 15 Jul 2019 17:06:28 -0700	[thread overview]
Message-ID: <20190716000628.GF113966@google.com> (raw)
In-Reply-To: <20190713003948.GA43313@google.com>

On Fri, Jul 12, 2019 at 05:39:48PM -0700, Josh Steadmon wrote:
> On 2019.06.20 14:06, Emily Shaffer wrote:
> > On Wed, Jun 19, 2019 at 08:17:29AM -0700, Junio C Hamano wrote:
> > > Emily Shaffer <emilyshaffer@google.com> writes:
> > > 
> > > > Maybe there's a case for storing them as a set of patch files that are
> > > > revision-controlled somewhere within Documentation/? There was some
> > > > discussion on the IRC a few weeks ago about trying to organize these
> > > > tutorials into their own directory to form a sort of "Git Contribution
> > > > 101" course, maybe it makes sense to store there?
> > > >
> > > >   Documentation/contributing/myfirstcontrib/MyFirstContrib.txt
> > > >   Documentation/contributing/myfirstcontrib/sample/*.patch
> > > >   Documentation/contributing/myfirstrevwalk/MyFirstRevWalk.txt
> > > >   Documentation/contributing/myfirstrevwalk/sample/*.patch
> > > >
> > > > I don't love the idea of maintaining text patches with the expectation
> > > > that they should cleanly apply always,...
> > > 
> > > Well, I actually think the above organization does match the intent
> > > of the "My first contribution codelab" perfectly.  When the codebase,
> > > the workflow used by the project, and/or the coding or documentation
> > > guideline gets updated, the text that documents how to contribute to
> > > the project as well as the sample patches must be updated to match
> > > the updated reality.
> > > 
> > > I agree with you that maintaining the *.patch files to always
> > > cleanly apply is less than ideal.  A topic to update the sample
> > > patches and tutorial text may be competing with another topic that
> > > updates the very API the tutorials are teaching, and the sample
> > > patches may not apply cleanly when two topics are merged together,
> > > even if the "update sample patches and tutorial text" topic does
> > > update them to match the API at the tip of the topic branch itself.
> > > One thing we _could_ do is to pin the target version of the codebase
> > > for the sake of tutorial.  IOW, the sample/*.patch may not apply
> > > cleanly to the version of the tree these patches were taken from,
> > > but would always apply cleanly to the most recent released version
> > > before the last update to the tutorial, or something like that.
> > > 
> > > Also having to review the patch to sample/*.patch files will be
> > > unpleasant.
> > 
> > I wonder if we can ease some pain for both of the above issues by
> > including some scripts to "inflate" the patch files into a topic branch,
> > or figure out some more easily-reviewed (but more complicated, I
> > suppose) method for sending updates to the sample/*.patch files.
> > 
> > Imagining workflows like this:
> > 
> > Doing the tutorial:
> >  - In worktree a/.
> >  - Run a magic script which creates a worktree with the sample code, b/.
> >  - Read through a/Documentation/MyFirstContribution.txt and generate
> >    a/builtins/psuh.c, referring to b/builtins/psuh.c if confused.
> > 
> > Rebasing the tutorial patches:
> >  - In worktree a/.
> >  - Run a magic script which checks out a new branch at the last known
> >    good base for the patchset, then applies all the patches.
> >  - Now faced with, likely, a topic branch based on v<n-1> (where n is
> >    latest release).
> >  - `git rebase v<n> -x (make && ./bin-wrappers/git psuh)`
> >  - Interactively fix conflicts
> >  - Run a script to generate a magic interdiff from the old version of
> >    patches
> >  - Mail out magic interdiff to list and get approval
> >  - (Maybe maintainer does this when interdiff is happy? Maybe updater
> >    does this when review looks good?) Run a magic script to regenerate
> >    patches from rebased branch, and note somewhere they are based on
> >    v<n>
> >  - Mail sample/*.patch (based on v<n>) to list (if maintainer rolled the
> >    patches after interdiff approval, this step can be skipped)
> > 
> > (This seems to still be a lot of steps, even with the magic script..)
> > 
> > Alternatively, for the same process:
> >  Updater: Run a magic script to create topic branch based on v<n-1>
> >    (like before)
> >  U: `git rebase v<n> -x (make && ./bin-wrappers/git psuh)`
> >  U: Interactively fix conflicts
> >  U: Run a script to turn topic branch back into sample/*.patch
> >  U: Send email with changes to sample/*.patch (this will be ugly and
> >     unreadable) - message ID <M1>
> >  Reviewer: Run a magic script, providing <M1> argument, which grabs the
> >     diff-of-.patch and generates an interdiff, or a topic branch based
> >     on v<n>
> >  R: Send comments explaining where issue is (tricky to find where to
> >     inline in the diff-of-.patch)
> >  U: Reroll diff-of-.patch email
> >  R: Accepts
> >  Maintainer: Applies diff-of-.patch email normally
> > 
> >  I suppose for the first suggestion, there ends up being quite a lot of
> >  onus on the maintainer, and a lot of trust that there is no difference
> >  between the RFC easy-to-read interdiff patchset. For the second
> >  suggestion, there ends up being onus on the reviewers to run some
> >  magical script. Maybe we can split the difference by expecting Updater
> >  to provide the interdiff below the --- line? Maybe in practice the
> >  diff-of-.patch isn't so unreadable, if it's only minor changes needed
> >  to bring the tutorial up to latest?
> > 
> >  I'm not sure there's a way to make this totally painless using email
> >  tools.
> 
> Random thought about the "magic scripts": if we keep an mbox instead of
> a directory of *.patch files, then it seems like git-format-patch and
> git-am would solve the bulk of this. I don't think dealing with
> diffs-of-patches-in-mbox is much worse than dealing with
> diffs-of-patches-in-multiple-files. And for the "Doing the tutorial"
> workflow, it nudges the new contributor to learn git-am.
> 
> But I guess the hard part here is the reviewing diffs-of-diffs part.
> I'm leaning towards the second option here; I personally would not feel
> too troubled as a reviewer by having to run an extra script. And as you
> say, diff-of-diffs may not be so bad in practice. Reviewers already see
> these whenever someone includes a range-diff in their v>=2 emails.

There was also some suggestion of instead checking in ed scripts or
similar to populate the changes. On one hand, it might be nicer, as
there aren't diff markers on the front of all the code... but on the
other hand, I'm not sure how many folks are familiar with ed (I know I'm
not) and it might be complex to indicate where to insert changes.

I have been in a position of reviewing diff-of-.patch in a past life,
albeit via Gerrit, and it's not the worst when the code is simple (as we
should always hope this example tutorial code would be).

 - Emily

  reply	other threads:[~2019-07-16  0:06 UTC|newest]

Thread overview: 102+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-07  1:07 [PATCH] documentation: add tutorial for revision walking Emily Shaffer
2019-06-07  1:07 ` [RFC PATCH 00/13] example implementation of revwalk tutorial Emily Shaffer
2019-06-07  1:07   ` [RFC PATCH 01/13] walken: add infrastructure for revwalk demo Emily Shaffer
2019-06-07  1:08   ` [RFC PATCH 02/13] walken: add usage to enable -h Emily Shaffer
2019-06-07  1:08   ` [RFC PATCH 03/13] walken: add placeholder to initialize defaults Emily Shaffer
2019-06-07  1:08   ` [RFC PATCH 04/13] walken: add handler to git_config Emily Shaffer
2019-06-07  1:08   ` [RFC PATCH 05/13] walken: configure rev_info and prepare for walk Emily Shaffer
2019-06-07  1:08   ` [RFC PATCH 06/13] walken: perform our basic revision walk Emily Shaffer
2019-06-07  1:08   ` [RFC PATCH 07/13] walken: filter for authors from gmail address Emily Shaffer
2019-06-07  1:08   ` [RFC PATCH 08/13] walken: demonstrate various topographical sorts Emily Shaffer
2019-06-07  1:08   ` [RFC PATCH 09/13] walken: demonstrate reversing a revision walk list Emily Shaffer
2019-06-07  1:08   ` [RFC PATCH 10/13] walken: add unfiltered object walk from HEAD Emily Shaffer
2019-06-07  1:08   ` [RFC PATCH 11/13] walken: add filtered object walk Emily Shaffer
2019-06-07 19:15     ` Jeff Hostetler
2019-06-17 20:30       ` Emily Shaffer
2019-06-07  1:08   ` [RFC PATCH 12/13] walken: count omitted objects Emily Shaffer
2019-06-07  1:08   ` [RFC PATCH 13/13] walken: reverse the object walk order Emily Shaffer
2019-06-07  6:21 ` [PATCH] documentation: add tutorial for revision walking Eric Sunshine
2019-06-10 21:26   ` Junio C Hamano
2019-06-10 21:38     ` Eric Sunshine
2019-06-17 23:19   ` Emily Shaffer
2019-06-19  8:13     ` Eric Sunshine
2019-06-19 23:35       ` Emily Shaffer
2019-06-23 18:54         ` Eric Sunshine
2019-06-10 20:25 ` Junio C Hamano
2019-06-17 23:50   ` Emily Shaffer
2019-06-19 15:17     ` Junio C Hamano
2019-06-20 21:06       ` Emily Shaffer
2019-07-13  0:39         ` Josh Steadmon
2019-07-16  0:06           ` Emily Shaffer [this message]
2019-07-16 17:24             ` Junio C Hamano
2019-06-10 20:49 ` Junio C Hamano
2019-06-17 23:33   ` Emily Shaffer
2019-06-26 23:49 ` [PATCH v2] " Emily Shaffer
2019-06-26 23:50   ` [RFC PATCH v2 00/13] example implementation of revwalk tutorial Emily Shaffer
2019-06-26 23:50     ` [RFC PATCH v2 01/13] walken: add infrastructure for revwalk demo Emily Shaffer
2019-06-26 23:50     ` [RFC PATCH v2 02/13] walken: add usage to enable -h Emily Shaffer
2019-06-27  4:47       ` Eric Sunshine
2019-06-27 18:40         ` Emily Shaffer
2019-06-27  4:50       ` Eric Sunshine
2019-06-26 23:50     ` [RFC PATCH v2 03/13] walken: add placeholder to initialize defaults Emily Shaffer
2019-06-26 23:50     ` [RFC PATCH v2 04/13] walken: add handler to git_config Emily Shaffer
2019-06-27  4:54       ` Eric Sunshine
2019-06-27 18:47         ` Emily Shaffer
2019-06-26 23:50     ` [RFC PATCH v2 05/13] walken: configure rev_info and prepare for walk Emily Shaffer
2019-06-27  5:06       ` Eric Sunshine
2019-06-27 18:56         ` Emily Shaffer
2019-06-26 23:50     ` [RFC PATCH v2 06/13] walken: perform our basic revision walk Emily Shaffer
2019-06-27  5:16       ` Eric Sunshine
2019-06-27 20:54         ` Emily Shaffer
2019-06-26 23:50     ` [RFC PATCH v2 07/13] walken: filter for authors from gmail address Emily Shaffer
2019-06-27  5:20       ` Eric Sunshine
2019-06-27 20:58         ` Emily Shaffer
2019-06-26 23:50     ` [RFC PATCH v2 08/13] walken: demonstrate various topographical sorts Emily Shaffer
2019-06-27  5:22       ` Eric Sunshine
2019-06-27 22:12         ` Emily Shaffer
2019-06-26 23:50     ` [RFC PATCH v2 09/13] walken: demonstrate reversing a revision walk list Emily Shaffer
2019-06-27  5:26       ` Eric Sunshine
2019-06-27 22:20         ` Emily Shaffer
2019-06-26 23:50     ` [RFC PATCH v2 10/13] walken: add unfiltered object walk from HEAD Emily Shaffer
2019-06-27  5:37       ` Eric Sunshine
2019-06-27 22:31         ` Emily Shaffer
2019-06-28  0:48           ` Eric Sunshine
2019-07-01 19:19             ` Emily Shaffer
2019-06-26 23:50     ` [RFC PATCH v2 11/13] walken: add filtered object walk Emily Shaffer
2019-06-27  5:42       ` Eric Sunshine
2019-06-27 22:33         ` Emily Shaffer
2019-06-26 23:50     ` [RFC PATCH v2 12/13] walken: count omitted objects Emily Shaffer
2019-06-27  5:44       ` Eric Sunshine
2019-06-26 23:50     ` [RFC PATCH v2 13/13] walken: reverse the object walk order Emily Shaffer
2019-06-27 22:56     ` [RFC PATCH v2 00/13] example implementation of revwalk tutorial Emily Shaffer
2019-07-01 20:19   ` [PATCH v3] documentation: add tutorial for revision walking Emily Shaffer
2019-07-01 20:20     ` [RFC PATCH v3 00/13] example implementation of revwalk tutorial Emily Shaffer
2019-07-01 20:20       ` [RFC PATCH v3 01/13] walken: add infrastructure for revwalk demo Emily Shaffer
2019-07-01 20:20       ` [RFC PATCH v3 02/13] walken: add usage to enable -h Emily Shaffer
2019-07-01 20:20       ` [RFC PATCH v3 03/13] walken: add placeholder to initialize defaults Emily Shaffer
2019-07-01 20:20       ` [RFC PATCH v3 04/13] walken: add handler to git_config Emily Shaffer
2019-07-01 20:20       ` [RFC PATCH v3 05/13] walken: configure rev_info and prepare for walk Emily Shaffer
2019-07-01 20:20       ` [RFC PATCH v3 06/13] walken: perform our basic revision walk Emily Shaffer
2019-07-01 20:20       ` [RFC PATCH v3 07/13] walken: filter for authors from gmail address Emily Shaffer
2019-07-01 20:20       ` [RFC PATCH v3 08/13] walken: demonstrate various topographical sorts Emily Shaffer
2019-07-01 20:20       ` [RFC PATCH v3 09/13] walken: demonstrate reversing a revision walk list Emily Shaffer
2019-07-01 20:20       ` [RFC PATCH v3 10/13] walken: add unfiltered object walk from HEAD Emily Shaffer
2019-07-01 20:20       ` [RFC PATCH v3 11/13] walken: add filtered object walk Emily Shaffer
2019-07-01 20:20       ` [RFC PATCH v3 12/13] walken: count omitted objects Emily Shaffer
2019-07-01 20:20       ` [RFC PATCH v3 13/13] walken: reverse the object walk order Emily Shaffer
2019-07-25  9:25       ` [RFC PATCH v3 00/13] example implementation of revwalk tutorial Johannes Schindelin
2019-08-06 23:13         ` Emily Shaffer
2019-08-08 19:19           ` Johannes Schindelin
2019-07-24 23:11     ` [PATCH v3] documentation: add tutorial for revision walking Josh Steadmon
2019-07-24 23:32     ` Jonathan Tan
2019-08-06 23:10       ` Emily Shaffer
2019-08-06 23:19     ` [PATCH v4] " Emily Shaffer
2019-08-07 19:19       ` Junio C Hamano
2019-08-14 18:33         ` Emily Shaffer
2019-08-14 19:18           ` Junio C Hamano
2019-10-10 15:19       ` [PATCH v5] documentation: add tutorial for object walking Emily Shaffer
2019-10-11  5:50         ` Junio C Hamano
2019-10-11 23:26           ` Emily Shaffer
2019-10-11 17:50         ` SZEDER Gábor
2019-10-11 23:33           ` Emily Shaffer
2019-10-11 23:55         ` [PATCH v6] " Emily Shaffer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190716000628.GF113966@google.com \
    --to=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=steadmon@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).