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@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. 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.
