Re: Should we auto-close PRs on git/git?

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Peff,

On Tue, 12 Nov 2019, Jeff King wrote:

> On Tue, Nov 12, 2019 at 08:11:06PM +0100, Johannes Schindelin wrote:
>
> > > To that end, I wonder if we should add an Action to automatically
> > > close PRs on that repo. It looks like
> > > https://github.com/dessant/repo-lockdown would do the trick. We could
> > > close incoming PRs automatically with a kind, maybe more succinct or
> > > prescriptive version of the prefill text encouraging folks to open the
> > > exact same PR against gitgitgadget/git instead.
> >
> > I am rather certain that that would not be a good thing to do.
> >
> > There are some people who open git/git PRs solely for the PR builds,
> > others to facilitate code review, and yet others just because it is the
> > intuitively obvious way to contribute to Git.
>
> We talked a while ago about having GitGitGadget operate on git/git,
> rather than on a separate mirror. That would automatically help at least
> one class of PR-opener: people who want their patches to reach the list
> but didn't realize they should be using gitgitgadget/git.
>
> I don't remember what the technical blockers are for getting that set
> up, but it seems like a strictly nicer outcome than auto-closing their
> PR.

Okay, here are a couple of technical challenges, off the top of my head:

# The permission problem

GitGitGadget needs code write permission on
https://github.com/gitgitgadget/git so that it can push those tags that
correspond to delivered patch series iterations. It also needs
permission to write to Pull Requests (so that it can comment and add
labels).

But on https://github.com/git/git, Junio offered a strong preference
for restricting access so that GitGitGadget cannot just push code. I
do agree with this, but there is the complication that we cannot ask
for a different permission sets depending on which repository we
install the GitHub App.

I just verified that I cannot add a PR comment on git/git using the
existing App (which is installed only on gitgitgadget/git).

Possible workaround: I could register a second GitGitGadget app
(e.g. gitgitgadget2) and install that on git/git, then use that set of
permissions to interact with PRs on git/git.

This, however, will require a change in GitGitGadget's code,
as it now potentially needs to use either the GitGitGadget App's token
or the GitGitGadget2's. And for pushing the tags it always needs to use
the GitGitGadget's token.

BTW I do not like the name `gitgitgadget2` very much (it suggests an
upgraded version to me), if you have any ideas, I'm all ears.

# Disentangling the tagging part from the rest

As I said, GitGitGadget pushes tags to gitgitgadget/git that correspond
to each sent iteration. This is not only to allow for fetching directly
(rather than trying to find an appropriate base commit and then applying
the patches manually, which I find very tedious) but also for the
range-diff for v2 and later.

I would like to keep doing this even when letting GitGitGadget handle
git/git's PRs.

To avoid clashes, I would suggest to invent a new tag format. The
current one is `pr-<number>/<author>/<branch-name>-v<iteration>`.
Instead of the prefix `pr-`, we could easily use `git-` and be fine.

However, that requires changes in GitGitGadget: so far, the URL prefix
`https://github.com/gitgitgadget/git/` is pretty hard-coded. In theory,
it would matter only when fetching the commits that need to be
`/submit`ed, of course, but that will take some careful analysis right
there.

# The Checks

To have the same nice integration with the GitHub Checks, where you can
easily see when GitGitGadget is running, and get to the logs, we will
need to install a separate CI/PR pipeline.

For technical reasons, this will have to live in
https://dev.azure.com/gitgitgadget/git/_build, as I want to have only
one available agent for these runs: GitGitGadget is not _really_ able to
run concurrently. Neither does it have to. It's not like contributors
try to send multiple patch series in parallel. This saves me a lot of
headache about locking.

# The commit mappings

One of the things that irks me the most with the mailing list driven
development is that it is super hard to go from mail to commit, or for
that matter, from commit to commit: _my_ commit in _my_ PR will have a
completely different hash than Junio's commit in gitster/git. To help
with that, GitGitGadget adds "Checks" to the commits in the PR that
link to the corresponding ones in gitster/git.

This should still be possible even in git/git, I think, provided that
the second App would be equipped with permissions to write those checks.

# The PR labels

Whenever `pu` is updated, GitGitGadget tries to figure out what has been
merged where, and add labels `pu`, `next`, `master` or `maint` to the
PRs, closing the ones that made it to `master`.

This should be equally possible on git/git, again contingent on the
appropriate permission.

# Reacting to `/submit`, `/preview`, etc

We can probably reuse the same Azure Function that we have right now,
provided that GitHub Apps can share the same webhook URL with other
Apps.

That's just the stuff off the top of my head.

To start with this project, if I had the time, I would probably register
that second app, install it on my fork and then pretend that my fork is
git/git, and start testing what goes wrong (trying to re-route the mails
away from the Git mailing list, of course).

Not an easy, nor a small project, I am afraid.

Ciao,
Dscho