On Wed, Jun 19, 2019 at 08:17:29AM -0700, Junio C Hamano wrote:
> Emily Shaffer <emilyshaffer@xxxxxxxxxx> 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.
- Emily
