Support PRs over at https://github.com/git/git, too #148

dscho · 2019-11-18T18:30:09Z

@peff asked for this a couple of times, e.g. at https://public-inbox.org/git/20191113011020.GB20431@sigill.intra.peff.net/

This PR should do that trick, together with a couple of changes to GitGitGadget's Azure Pipelines (and a new one).

Note that the TO-DROP patches at the end are there only for testing; I did send a test patch series (and a second iteration) to myself, and the resulting tags can be seen here: https://github.com/gitgitgadget/git/releases/tag/pr-dscho-12%2Fdscho%2Ffix-rebase-r-labels-v1 and https://github.com/gitgitgadget/git/releases/tag/pr-dscho-12%2Fdscho%2Ffix-rebase-r-labels-v2.

I would appreciate all the code reviews I can get, although I am expecting having to work out the kinks after merging this PR.

dscho · 2019-11-18T18:53:11Z

As usual, GitHub's UI juggles the commits around because it orders by date rather than by topology. And the test is failing because I removed the Cc: for my testing ;-)

dscho · 2019-11-18T20:44:31Z

And the test is failing because I removed the Cc: for my testing ;-)

Note: I pushed those TO-DROP commits to the work-on-git.git-debug branch in my fork, and dropped from this here PR.

webstech · 2019-11-19T05:56:51Z

Lots to look at here. I will post comments as I go through it. Hopefully I won't add too many more as I circle between sources.

script/misc-helper.ts

lib/github-glue.ts

azure-function/index.js

lib/ci-helper.ts

dscho · 2019-11-19T16:50:52Z

@webstech @sgn could you have another look, please? In particular at the interdiff: https://github.com/gitgitgadget/gitgitgadget/compare/f8026c1a05c04760f35f95ddc216014e58bc62c7..374319ab56bce3729b70f360ecc357686c72dab6

script/misc-helper.ts

lib/gitgitgadget.ts

webstech · 2019-11-20T06:44:35Z

I have now reviewed the commits and the set. Think I may have commented on something earlier in a single commit that was updated in a later commit, so sorry about that. I have done a basic scan of the project looking for anything else that may have been missed but you are much more familiar with it than I. Just had a couple more comments above.

dscho · 2019-11-20T08:28:50Z

I have done a basic scan of the project looking for anything else that may have been missed but you are much more familiar with it than I.

@webstech thank you so much. The more eyes we have on these changes, the less likely bugs can hide.

dscho · 2019-11-20T12:08:40Z

@webstech does this interdiff look good to you?

webstech · 2019-11-20T18:05:06Z

@webstech does this interdiff look good to you?

The old code relied on the git config setting and the new code overrides the git config setting. Is that what is wanted? Maybe not since this is the git work dir and not the gitgitgadget work dir. Maybe defer this for now? It could break things. Maybe --work-dir is not needed and the config should be relied on. What do you think?

May not matter but the default for commander.workDir is "." so the || "." seems redundant.

…tree For many operations, GitGitGadget needs a `git.git` worktree to interact with the commits and the notes stored at https://github.com/gitgitgadget/git. To that end, GitGitGadget looks at the config setting `gitgitgadget.workDir`. The reason why this is stored in a Git config is so that the Azure Pipelines can be configured conveniently to share a single per-agent `git.git` worktree. The `misc-helper` script has a `--work-dir` option that was supposed to help with that, defaulting to the current directory. But that does not make sense! Because the current directory is typically a worktree cloned from https://github.com/gitgitgadget/gitgitgadget, i.e. a _GitGitGadget_ worktree instead of a git.git one. And the `--work-dir` option was never respected anyway, providing more evidence that it was introduced without any clear idea what its purpose should be: specify the GitGitGadget worktree? Or the git.git one? The former would be needed only to access the config setting specifying the latter one. So let's just clear things up and declare that the only workdir that is relevant is the git.git worktree, and `--work-dir` can override `gitgittgadget.workDir` (if it was set to begin with). The confusion was pointed out by Chris Webster in gitgitgadget#148 (comment) Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Suggested by Chris Webster. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

The footer of the cover letter of the patch series sent by GitGitGadget contain a link back to the originating PR. Rather than deconstructing the URL into bits and pieces only to put back together the identical URL, let's use the original URL to begin with. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Originally, the idea was to develop GitGitGadget to see whether it works, whether it helps, and eventually just move things to https://github.com/git/git. That is the reason why in many places, GitGitGadget stores _full_ Pull Request URLs instead of just numbers. Somewhere along the line, this developer became less sure about this course of action and instead focused on supporting https://github.com/gitgitgadget/git well, so in some code paths, that repository owner "gitgitgadget" is hard-coded. One of the reasons for this trepidation was the fact that there seemed to be consensus that GitGitGadget should never be allowed write access to the git/git repository. It does need to push the tagged iterations, though, to be able to generate range-diffs between patch series iterations. When the idea arose that the tags could still be pushed to https://github.com/gitgitgadget/git (with a different prefix than the current tags, i.e. something like `pr-git-<number>` instead of `pr-<number>`), the plan to support git/git natively picked up steam again, but there was still the permission problem. The solution is to have _two_ GitHub Apps: one with write permission to the gitgitgadget/git repository (and this App is intended to be installed _only_ on that repository), and a newly-created one: GitGitGadget-Git (which is intended to be installed on the git/git repository, and has only write permission on the PR comments and labels). With this commit, GitGitGadget learns to pick up the credentials for installations of that new App, e.g. `git`, and generate a token to do its work. The idea is that the private key is available under the Git config key `gitgitgadget-git.privateKey` (typically specified via the command-line, in an Azure Pipeline via a secret variable), and the resulting token is stored under `gitgitgadget.<owner>.githubToken` where `<owner>` corresponds to the repository owner (i.e. the repository URL is https://github.com/<owner>/git). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

The idea for tagged iterations was to keep generating `pr-<number>/*` tags for PRs at gitgitgadget/git, and to generate `pr-git-<number>/*` ones for PRs at git/git. We still want to push the tags to gitgitgadget/git, whether they correspond to PRs at gitgitgadget/git or not. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

In preparation for opening up GitGitGadget to handle PRs at https://github.com/git/git (where we want GitGitGadget to be read-only except for PR labels and comments), let's just tell GitGitGadget to skip annotating commits (for the original <-> gitster/git commit mapping) unless we're working on a PR that was opened at https://github.com/gitgitgadget/git. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

To allow GitGitGadget to be used with PRs other than at https://github.com/gitgitgadget/git, i.e. to support https://github.com/git/git, let's teach this `misc-helper` command about (optionally) using a different repository. The Azure Pipeline "Mirror Git List to GitGitGadget's PRs" at https://dev.azure.com/gitgitgadget/git/_build?definitionId=5 uses this command, so we want to be careful not to break things (such as changing the command-line syntax). Technically, we do not actually prepare _this command_ for git/git, but the underlying methods that already accept full Pull Request URLs (which already contain all the information necessary). This patch assumes that the config setting `gitgitgadget.githubToken` has the value necessary for https://github.com/gitgitgadget/git, i.e. the token generated for the GitHub App "GitGitGadget", and this patch assumes that for every Pull Request URL thrown at it that has the form https://github.com/<owner>/git/pull/<number> for owners other than "gitgitgadget", there exists a corresponding config setting `gitgitgadget.<owner>.githubToken`, i.e. tokens generated for the GitHub App "GitGitGadget-Git". The reason for this split between the owner "gitgitgadget" and all others is that the GitHub App "GitGitGadget" really needs full write permissions on the repository (the gitgitgadget/git one, to push tags), while the GitHub App "GitGitGadget-Git" only needs permissions to add PR comments and edit PR labels. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

While it is true that we only annotate commits with commit statuses in the repository at https://github.com/gitgitgadget/git (because we do not want to allow GitGitGadget too much write access to, say, git/git), it might be possible in the future, so let's prepare `updateCommitMapping()` for that possibility. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

To allow GitGitGadget to be used with PRs other than at https://github.com/gitgitgadget/git, i.e. to support https://github.com/git/git, let's teach the `misc-helper` script about (optionally) using a different repository. It has to be optional so to keep the Azure Pipeline "GitGitGadget PR Handler" at https://dev.azure.com/gitgitgadget/git/_build?definitionId=3 working. This pipeline services gitgitgadget/git and needs to keep running both with this patch and without it because we will not merge this patch to `master` immediately. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

To allow GitGitGadget to be used with PRs other than at https://github.com/gitgitgadget/git, i.e. to support https://github.com/git/git, let's teach this `misc-helper` command about (optionally) looking at additional repositories. This is a companion patch to the previous one, but it has to handle _all_ of the repositories GitGitGadget is servicing because that command is wired up in an Azure Pipeline: "Update GitGitGadget's PRs" at https://dev.azure.com/gitgitgadget/git/_build?definitionId=2. Therefore, the command-line syntax of the command actually does not change. In preparation to servicing PRs at https://github.com/git/git (and for debugging purposes, also https://github.com/dscho/git), let's handle those "repository owners" specifically. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

To allow GitGitGadget to be used with PRs other than at https://github.com/gitgitgadget/git, i.e. to support https://github.com/git/git, let's teach these `misc-helper` commands about (optionally) using a different repository. This is a companion patch to the previous two, but unlike them, these commands are not wired up in any active Azure Pipeline. Therefore, we technically do not have to be careful to make the repository owner optional. We still do because why not. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

This needs adjusting in preparation for teaching GitGitGadget to work also on PRs at https://github.com/git/git. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

This is necessary to avoid any (really unlikely, but hey!) potential conflicts between the Message-IDs of cover letters from PRs sent via different forks. Example: the format for PRs in https://github.com/gitgitgadget/git is still `pull.<number>.git.<timestamp>.<email>`, but for PRs in https://github.com/git/git, the format will be `pull.<number>.git.git.<timestamp>.<email>`, i.e. an extra `git.` after the `<number>.`. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

As GitGitGadget is tied down not to run outside the `gitgitgadget` org, we need to tell it explicitly that `git` is okay, too, if we want to service the PRs at https://github.com/git/git. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

This is the most important step to let GitGitGadget support PRs at https://github.com/git/git: do not reject webhooks originating from there, and trigger a different pipeline than for events being sent over from gitgitgadget/git. The different Azure Pipeline is important because the pipeline has to be registered as a CI build for the respective repository, so that it shows up in the "Checks" section of the Pull Requests in question. While at it, let's document the relationship between the (single) Azure Function and the (multiple) Azure Pipelines that need to work together to implement the backend of GitGitGadget's PR comment handling. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Rather than testing with git/git right away, during the development of the patches that teach GitGitGadget to handle git/git PRs, the GitGitGadget-Git app has been installed on dscho/git, and a new PR pipeline has been added there, too, so that this thing can be tested a little. This setup might come in handy later, for other stunts similar in complexity to expanding GitGitGadget to handle PRs outside of https://github.com/gitgitgadget/git, so let's keep it around. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho · 2019-11-22T11:54:20Z

@webstech does this interdiff look good to you?

The old code relied on the git config setting and the new code overrides the git config setting.

Oh, but all that was changed is that we now look in the specified workDir for that config setting. By default, it is still the same: ., i.e. the current directory.

Is that what is wanted? Maybe not since this is the git work dir and not the gitgitgadget work dir.

I know, it is confusing. There is commander.workDir, which is supposed to be the GitGitGadget worktree, and commander.gitGitWorkDir, which is the git.git worktree.

The config setting gitgitgadget.workDir is actually expected to be configured in the GitGitGadget worktree, and it points to the git.git worktree. Not the best naming on my part, I admit.

Maybe defer this for now? It could break things. Maybe --work-dir is not needed and the config should be relied on. What do you think?

Sure, I should not have snuck it in. But I still think it is the right thing to do.

May not matter but the default for commander.workDir is "." so the || "." seems redundant.

True. And it also nudged me into looking what commander.workDir is actually used for. And the answer is: nothing.

I opened #156 to re-define what --work-dir means, and to make it actually do something in the first place ;-)

dscho · 2019-11-22T12:36:12Z

@webstech I dropped that workdir patch again. Do you have any more concerns?

…tree For many operations, GitGitGadget needs a `git.git` worktree to interact with the commits and the notes stored at https://github.com/gitgitgadget/git. To that end, GitGitGadget looks at the config setting `gitgitgadget.workDir`. The reason why this is stored in a Git config is so that the Azure Pipelines can be configured conveniently to share a single per-agent `git.git` worktree. The `misc-helper` script has a `--work-dir` option that was supposed to help with that, defaulting to the current directory. But that does not make sense! Because the current directory is typically a worktree cloned from https://github.com/gitgitgadget/gitgitgadget, i.e. a _GitGitGadget_ worktree instead of a git.git one. And the `--work-dir` option was never respected anyway, providing more evidence that it was introduced without any clear idea what its purpose should be: specify the GitGitGadget worktree? Or the git.git one? The former would be needed only to access the config setting specifying the latter one. So let's just clear things up and declare that the only workdir that is relevant is the git.git worktree, and `--work-dir` can override `gitgittgadget.workDir` (if it was set to begin with). The confusion was pointed out by Chris Webster in gitgitgadget#148 (comment) Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho · 2019-11-22T15:07:25Z

FWIW I installed GitGitGadget-Git on git/git, and tested with a first patch series: https://lore.kernel.org/git/pull.670.git.git.1574433665.gitgitgadget@gmail.com/. So far, it seems to work fine.

webstech · 2019-11-22T17:43:15Z

The old code relied on the git config setting and the new code overrides the git config setting.

Oh, but all that was changed is that we now look in the specified workDir for that config setting. By default, it is still the same: ., i.e. the current directory.

Sorry, my mistake confusing the workDir parameter and the gitConfig() as a setter function when given a second parm. Note to self: Drink ☕ before commenting.

Is that what is wanted? Maybe not since this is the git work dir and not the gitgitgadget work dir.

I know, it is confusing. There is commander.workDir, which is supposed to be the GitGitGadget worktree, and commander.gitGitWorkDir, which is the git.git worktree.

The config setting gitgitgadget.workDir is actually expected to be configured in the GitGitGadget worktree, and it points to the git.git worktree. Not the best naming on my part, I admit.

Agree on the confusing part when --work-dir was not used.

I opened #156 to re-define what --work-dir means, and to make it actually do something in the first place ;-)

This ties in to issue #140 and the need to pass --work-dir to CIHelper (and thus GitGitGadget).

First of all, this fixes the bug where passing `--work-dir` to the script does exactly nothing at all. Further, the option name was outright confusing: GitGitGadget accesses _two_ worktrees, the git.git clone (needed to interact with the commits and the notes stored at https://github.com/gitgitgadget/git) and the GitGitGadget worktree that is expected to contain a Git config with further information (and it also contains the `res/WELCOME.md` message we show new users). The confusion was pointed out by Chris Webster in gitgitgadget#148 (comment) Let's replace the `--work-dir` option by two options: --git-work-dir: This points to the git.git working directory to work with (it can actually be a bare clone, it does not require a worktree at all). --gitgitgadget-work-dri: This option points to the worktree to use for the `CIHelper`: it contains the `res/WELCOME.md` file we show to prospective GitGitGadget users, and it also contains the Git config where we store settings such as the SMTP configuration for sending the patch series to the Git mailing list. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

dscho requested a review from webstech November 18, 2019 18:30

dscho force-pushed the work-on-git.git branch from 73ba50b to f8026c1 Compare November 18, 2019 20:42