Should we auto-close PRs on git/git?

Should we auto-close PRs on git/git?

[Emily Shaffer]

Hi all,

It seems to me that the friendly template text we prefill when someone
opens a pull request in github.com/git/git isn't being fully appreciated
by many interested contributors. For some time now, Johannes has been
slogging through the list to try to narrow it down to folks who are
still interested in contributing, and yesterday on #git-devel said he
was pretty happy with the progress so far.

But to me, this seems like a sort of Sisyphean task - more folks will
want to make contributions and not read the template text, and we will
have more PRs being ignored forever, especially if Johannes decides he
doesn't want to shepherd those changes anymore (I would have decided
that long ago, in his shoes).

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.

Here's the prefilled template now:

  Thanks for taking the time to contribute to Git! Please be advised
  that the Git community does not use github.com for their
  contributions. Instead, we use a mailing list (git@vger.kernel.org)
  for code submissions, code reviews, and bug reports. Nevertheless, you
  can use GitGitGadget (https://gitgitgadget.github.io/) to conveniently
  send your Pull Requests commits to our mailing list.

  Please read the "guidelines for contributing" linked above!

Maybe we can close PRs with something like this:

  Thank you for taking the time to submit a patch!

  However, Git does not accept submissions via GitHub pull requests.

  You can open an identical pull request to this one against
  https://github.com/gitgitgadget/git and follow the instructions there
  to submit it to the Git mailing list, where reviews are performed.

  If you don't want to subscribe to the mailing list, you can keep an
  eye on your patch at https://public-inbox.org/git, or by watching
  comments on your GitGitGadget pull request.

  More info on GitGitGadget: https://gitgitgadget.github.io

I was aiming for "same message, but firmer", and "write down something
so we have a place to start". I look forward to the discussion.

 - Emily

PS: Today we have 17 PRs open against git/git, and I think all of them
have been nudged by dscho in comments to open against GGG instead. Many
are in a state where dscho is sending a ping every few weeks to see if
the committer is interested in following through.

https://github.com/git/git/pulls

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

[Junio C Hamano]

Emily Shaffer <emilyshaffer@google.com> writes:

> It seems to me that the friendly template text we prefill when someone
> opens a pull request in github.com/git/git isn't being fully appreciated
> by many interested contributors. For some time now, Johannes has been
> slogging through the list to try to narrow it down to folks who are
> still interested in contributing, and yesterday on #git-devel said he
> was pretty happy with the progress so far.
>
> But to me, this seems like a sort of Sisyphean task ...

Yeah, I would not stop Dscho if he likes doing so, but it does sound
like a waste of talent.

> ... want to make contributions and not read the template text, and we will
> have more PRs being ignored forever, especially if Johannes decides he
> doesn't want to shepherd those changes anymore (I would have decided
> that long ago, in his shoes).
>
> 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.

Personally, I think that it would not help, it would be a waste of
our time to set up, and it would be a waste of our attention having
to worry about giving yet another external read/write access to PRs
to a third-party tool.

I've looked at those PRs, and noticed that the issues that the ones
with unedited prefilled template try to address are mostly those
that would cost more to give help polishing the patch into an
acceptable shape than some of us redo them outselves (more
clarifications below).

Quite honestly, "drive-by contribution" is overrated.  Surely it is
nice if those little typoes and forgotten free()s and off-by-ones
got fixed by somebody without taking too much of our attention, and
it would be nicer if we can help those who started from "drive-by"
status eventually grow to full fledged contributors.

But step back and think about these two a bit.

Those tiny typoes, missing calls to free(), etc. that are low
hanging fruits tend to be "bugs" that have only one obvious way to
"fix", without leaving much room to express the patch in any other
way.  It's not like that they now own the bug and the right to make
a patch to fix it because they found and sent a PR first.  If
somebody else makes the same fix with patch text that happens to be
identical, that is perfectly fine.  The _only_ real contribution to
us made by such a PR is to let us know where such a trivial problem
resides; once that is identified, anybody would fix it the same way.

It would be far more effective use of the time of the community to
make the same fix by any member who already knows how such a patch
should look like, while giving a proper credit for discovering the
issue.

I can already hear people saying that by investing to educate the
original drive-by contributors (instead of "stealing" their patch
and doing it outselves) would yield a larger value in the longer
term, as it could help grow the drive-by contributors into our
community.  I would agree with that argument in principle, but
I do not think that would apply to the drive-by stuff with unedited
prefilled template still intact.

The thing is, I do not think we can expect those who do not even
bother to read the prefilled template to grow to full fledged
contributors.  Certainly before they start paying attention to what
are told to them.

So, I would certainly not veto auto-closing, but I do not think it
would help.

Thanks.

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

[Johannes Schindelin]

Hi Emily,

On Fri, 8 Nov 2019, Emily Shaffer wrote:

> It seems to me that the friendly template text we prefill when someone
> opens a pull request in github.com/git/git isn't being fully appreciated
> by many interested contributors.

That is probably due to our confusing use of the template as a stop sign
;-)

> For some time now, Johannes has been slogging through the list to try
> to narrow it down to folks who are still interested in contributing,
> and yesterday on #git-devel said he was pretty happy with the progress
> so far.

I don't mind it, and quite honestly, it does not take a lot of time,
most of the time.

> But to me, this seems like a sort of Sisyphean task - more folks will
> want to make contributions and not read the template text, and we will
> have more PRs being ignored forever, especially if Johannes decides he
> doesn't want to shepherd those changes anymore (I would have decided
> that long ago, in his shoes).

The PRs are not bad. What is bad is all those comments on commits coming
in as of recent, some developers thinking that they do not need to
research the best way to reach the Git contributor community and instead
just assuming that adding comments via GitHub's UI is a valid way.

I should probably refrain from trying to help those developers because
it makes me very cranky, but I just don't want Git to be an unfriendly
project.

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

Even some long-running PRs are worth keeping open, e.g. the Plan 9
support (which will just take time), the GET_OID_GENTLY one or the one
clarifying the documentation of `git submodule update` (which both need
to wait for a time when the respective contributor is less busy), and
the likes.

> Here's the prefilled template now:
>
>   Thanks for taking the time to contribute to Git! Please be advised
>   that the Git community does not use github.com for their
>   contributions. Instead, we use a mailing list (git@vger.kernel.org)
>   for code submissions, code reviews, and bug reports. Nevertheless, you
>   can use GitGitGadget (https://gitgitgadget.github.io/) to conveniently
>   send your Pull Requests commits to our mailing list.
>
>   Please read the "guidelines for contributing" linked above!
>
> Maybe we can close PRs with something like this:
>
>   Thank you for taking the time to submit a patch!
>
>   However, Git does not accept submissions via GitHub pull requests.
>
>   You can open an identical pull request to this one against
>   https://github.com/gitgitgadget/git and follow the instructions there
>   to submit it to the Git mailing list, where reviews are performed.
>
>   If you don't want to subscribe to the mailing list, you can keep an
>   eye on your patch at https://public-inbox.org/git, or by watching
>   comments on your GitGitGadget pull request.
>
>   More info on GitGitGadget: https://gitgitgadget.github.io
>
> I was aiming for "same message, but firmer", and "write down something
> so we have a place to start". I look forward to the discussion.
>
>  - Emily
>
> PS: Today we have 17 PRs open against git/git, and I think all of them
> have been nudged by dscho in comments to open against GGG instead. Many
> are in a state where dscho is sending a ping every few weeks to see if
> the committer is interested in following through.
>
> https://github.com/git/git/pulls

They all have been nudged, sometimes to clean up the patch first, or to
suggest that maybe the goal of the PR might not be all that desirable.

Some of the PRs probably can be closed, but as I said, I would like to
think of Git as a friendly project, a helpful one, so I want to err in
favor of talking to the contributors rather than shutting the door in
their face, so to say.

Ciao,
Dscho

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

[Jeff King]

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.

-Peff

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

[Stephen Smith]

On Friday, November 8, 2019 9:55:32 PM MST Junio C Hamano wrote:
> > But to me, this seems like a sort of Sisyphean task ...
> 
> Yeah, I would not stop Dscho if he likes doing so, but it does sound
> like a waste of talent.
> 

I contribute when I can for small projects, it looks I could help with some of 
those sparing Dscho some brain cells.

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

[Johannes Schindelin]

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

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

[Emily Shaffer]

On Tue, Nov 12, 2019 at 08:11:06PM +0100, Johannes Schindelin wrote:
> Hi Emily,
> 
> On Fri, 8 Nov 2019, Emily Shaffer wrote:
> 
> > It seems to me that the friendly template text we prefill when someone
> > opens a pull request in github.com/git/git isn't being fully appreciated
> > by many interested contributors.
> 
> That is probably due to our confusing use of the template as a stop sign
> ;-)
> 
> > For some time now, Johannes has been slogging through the list to try
> > to narrow it down to folks who are still interested in contributing,
> > and yesterday on #git-devel said he was pretty happy with the progress
> > so far.
> 
> I don't mind it, and quite honestly, it does not take a lot of time,
> most of the time.
> 
> > But to me, this seems like a sort of Sisyphean task - more folks will
> > want to make contributions and not read the template text, and we will
> > have more PRs being ignored forever, especially if Johannes decides he
> > doesn't want to shepherd those changes anymore (I would have decided
> > that long ago, in his shoes).
> 
> The PRs are not bad. What is bad is all those comments on commits coming
> in as of recent, some developers thinking that they do not need to
> research the best way to reach the Git contributor community and instead
> just assuming that adding comments via GitHub's UI is a valid way.
> 
> I should probably refrain from trying to help those developers because
> it makes me very cranky, but I just don't want Git to be an unfriendly
> project.

I guess my concern is this: when I reply to some code review, email,
whatever, when I am cranky, it makes me seem unfriendly; when I do so
while wearing a maintainership hat (I maintain another project
elsewhere) it makes my project seem unfriendly :) Besides, I don't think
that anybody wants a contributor to be regularly doing work that makes
them cranky.

> > PS: Today we have 17 PRs open against git/git, and I think all of them
> > have been nudged by dscho in comments to open against GGG instead. Many
> > are in a state where dscho is sending a ping every few weeks to see if
> > the committer is interested in following through.
> >
> > https://github.com/git/git/pulls

> They all have been nudged, sometimes to clean up the patch first, or to
> suggest that maybe the goal of the PR might not be all that desirable.
> 
> Some of the PRs probably can be closed, but as I said, I would like to
> think of Git as a friendly project, a helpful one, so I want to err in
> favor of talking to the contributors rather than shutting the door in
> their face, so to say.

I do agree that meeting a patient human instead of silence is a good
contributor experience, and I appreciate all the work you're putting in
that direction.

 - Emily

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

[Jeff King]

On Wed, Nov 13, 2019 at 01:04:35PM +0100, Johannes Schindelin wrote:

> > 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:
> [...]
> Not an easy, nor a small project, I am afraid.

Yow. That's a lot more involved than I was hoping for.

Thanks for writing it up. Some of the points raised were interesting. I
do think we'd want git/git (the repository) to remain read-only if
possible. If GitHub's permissions model is a limiting factor here, let
me know and I can try to bring it to the attention of the right people.

-Peff

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

[Johannes Schindelin]

Hi Peff,

On Thu, 14 Nov 2019, Jeff King wrote:

> On Wed, Nov 13, 2019 at 01:04:35PM +0100, Johannes Schindelin wrote:
>
> > > 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:
> > [...]
> > Not an easy, nor a small project, I am afraid.
>
> Yow. That's a lot more involved than I was hoping for.
>
> Thanks for writing it up. Some of the points raised were interesting. I
> do think we'd want git/git (the repository) to remain read-only if
> possible.

I guess you're right.

We should probably try to restrict the permissions as much as possible,
not only deny write access to the repository.

For example, one thing GitGitGadget does is to add these "Checks" to the
commits of the PRs which contain links to the corresponding commits in
gitster/git (if any). Those can actually not be removed, there is not
even any API for that. So it would probably make sense to avoid that in
git/git.

This would mean that the git/git part of GitGitGadget does not install
those commit mappings. I guess that's okay, they _are_ kinda hard to
use.

> If GitHub's permissions model is a limiting factor here, let me know
> and I can try to bring it to the attention of the right people.

I actually don't think that my use case fits any sane permission model
;-) After all, I want the GitHub App to _span_ repositories (even orgs),
and that's not really the idea of Apps.

After sleeping over it, I don't actually think that it is such a bad
idea to add a second GitHub App with a more limited permission set.

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Peff,

On Fri, 15 Nov 2019, Johannes Schindelin wrote:

> On Thu, 14 Nov 2019, Jeff King wrote:
>
> > On Wed, Nov 13, 2019 at 01:04:35PM +0100, Johannes Schindelin wrote:
> >
> > > > 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:
> > > [...]
> > > Not an easy, nor a small project, I am afraid.
> >
> > Yow. That's a lot more involved than I was hoping for.

Yeah, it wasn't easy. But then, who does not like a little challenge,
especially the challenge to test things outside of production? So here
is a PR: https://github.com/gitgitgadget/gitgitgadget/pull/148

I trust everybody with even rudimentary Javascript skills to be able to
provide useful feedback on that PR.

To build some confidence in my patches (as you probably know, I do not
trust reviews as much as I trust real-life testing, although I do prefer
to have both) I "kind of" activated it on my fork, limited to act only
on comments _I_ made on PRs (and sending only to me instead of the
list), and it seems to work all right, so far. I cannot say for sure
whether it handles the PR labels correctly, but I guess time will tell,
and I will fix bugs as quickly as I can.

Question is: should I turn this thing on? I.e. install that
GitGitGadget-Git App on https://github.com/git/git? This would allow
GitHub users to `/submit` directly from PRs opened in that repository. I
am sure that there are a few kinks to work out, but I do think that it
should not take long to stabilize.

> > Thanks for writing it up. Some of the points raised were interesting. I
> > do think we'd want git/git (the repository) to remain read-only if
> > possible.
>
> I guess you're right.
>
> We should probably try to restrict the permissions as much as possible,
> not only deny write access to the repository.
>
> For example, one thing GitGitGadget does is to add these "Checks" to the
> commits of the PRs which contain links to the corresponding commits in
> gitster/git (if any). Those can actually not be removed, there is not
> even any API for that. So it would probably make sense to avoid that in
> git/git.
>
> This would mean that the git/git part of GitGitGadget does not install
> those commit mappings. I guess that's okay, they _are_ kinda hard to
> use.

I made it so. The GitGitGadget-Git App only requires write permission to
add PR comments and labels, which I think should be okay. It
specifically has _no_ permission to push to git/git.

> > If GitHub's permissions model is a limiting factor here, let me know
> > and I can try to bring it to the attention of the right people.
>
> I actually don't think that my use case fits any sane permission model
> ;-) After all, I want the GitHub App to _span_ repositories (even orgs),
> and that's not really the idea of Apps.
>
> After sleeping over it, I don't actually think that it is such a bad
> idea to add a second GitHub App with a more limited permission set.

The name _was_ bad, but I did settle for GitGitGadget-Git in the end.
Not the most elegant name, but hey, it works so far.

Ciao,
Dscho

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

[Jeff King]

On Mon, Nov 18, 2019 at 07:37:57PM +0100, Johannes Schindelin wrote:

> Yeah, it wasn't easy. But then, who does not like a little challenge,
> especially the challenge to test things outside of production? So here
> is a PR: https://github.com/gitgitgadget/gitgitgadget/pull/148
> 
> I trust everybody with even rudimentary Javascript skills to be able to
> provide useful feedback on that PR.

Wow, thanks for working on this! I don't know that I'd call my
javascript skills even rudimentary, but I did give it a look. The real
challenge to me is not the individual lines of code, but understanding
how the Azure Pipelines and GitHub App systems fit together. So I didn't
see anything wrong, but I also know very little about those systems.

Likewise, the explanations in your comments and commit messages all made
sense to me. But that may also be a false sense of security. You nicely
led me through reading the patches, but the likely bug would probably be
one you did not even anticipate. ;)

> To build some confidence in my patches (as you probably know, I do not
> trust reviews as much as I trust real-life testing, although I do prefer
> to have both) I "kind of" activated it on my fork, limited to act only
> on comments _I_ made on PRs (and sending only to me instead of the
> list), and it seems to work all right, so far. I cannot say for sure
> whether it handles the PR labels correctly, but I guess time will tell,
> and I will fix bugs as quickly as I can.

Yeah, that makes sense to me. Going from one repo to three is not much
worse than going to two, so it's good to have a testing area, too.

Do you want any third-party testing there (e.g., a user who isn't you
making a PR against dscho/git)?

> Question is: should I turn this thing on? I.e. install that
> GitGitGadget-Git App on https://github.com/git/git? This would allow
> GitHub users to `/submit` directly from PRs opened in that repository. I
> am sure that there are a few kinks to work out, but I do think that it
> should not take long to stabilize.

I'd say "yes". The status quo is probably worse than a system with a few
bugs. The worst case if it's disastrously wasting submitter's time is
that we turn it back off, but I have faith that you'd just fix the bugs
before then anyway.

Is the existing Pipelines integration enough for you to turn it on for
git/git, or do I need to tweak any settings?

-Peff

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

[Johannes Schindelin]

Hi Peff,

On Thu, 21 Nov 2019, Jeff King wrote:

> On Mon, Nov 18, 2019 at 07:37:57PM +0100, Johannes Schindelin wrote:
>
> > Yeah, it wasn't easy. But then, who does not like a little challenge,
> > especially the challenge to test things outside of production? So here
> > is a PR: https://github.com/gitgitgadget/gitgitgadget/pull/148
> >
> > I trust everybody with even rudimentary Javascript skills to be able
> > to provide useful feedback on that PR.
>
> Wow, thanks for working on this! I don't know that I'd call my
> javascript skills even rudimentary, but I did give it a look. The real
> challenge to me is not the individual lines of code, but understanding
> how the Azure Pipelines and GitHub App systems fit together. So I didn't
> see anything wrong, but I also know very little about those systems.

I actually spent some quality time with the wiki in the past days to
remedy that. You can adore the result in all its beauty here:

https://github.com/gitgitgadget/gitgitgadget/wiki/GitGitGadget's-Azure-Function-and-Azure-Pipelines

> Likewise, the explanations in your comments and commit messages all made
> sense to me. But that may also be a false sense of security. You nicely
> led me through reading the patches, but the likely bug would probably be
> one you did not even anticipate. ;)

Right, but it does help to have somebody cross-check the ideas.

You probably also realized that Chris Webster and Danh looked over them
and provided useful suggestions, which I incorporated. One of those
suggestions was to document the involved Azure Pipelines ;-)

> > To build some confidence in my patches (as you probably know, I do not
> > trust reviews as much as I trust real-life testing, although I do
> > prefer to have both) I "kind of" activated it on my fork, limited to
> > act only on comments _I_ made on PRs (and sending only to me instead
> > of the list), and it seems to work all right, so far. I cannot say for
> > sure whether it handles the PR labels correctly, but I guess time will
> > tell, and I will fix bugs as quickly as I can.
>
> Yeah, that makes sense to me. Going from one repo to three is not much
> worse than going to two, so it's good to have a testing area, too.
>
> Do you want any third-party testing there (e.g., a user who isn't you
> making a PR against dscho/git)?

While that would be nice, my fork is a mess and not really set up to
provide any useful target branch...

The real proof of the concept will come when the first git/git PR will be
submitted.

> > Question is: should I turn this thing on? I.e. install that
> > GitGitGadget-Git App on https://github.com/git/git? This would allow
> > GitHub users to `/submit` directly from PRs opened in that repository. I
> > am sure that there are a few kinks to work out, but I do think that it
> > should not take long to stabilize.
>
> I'd say "yes". The status quo is probably worse than a system with a few
> bugs. The worst case if it's disastrously wasting submitter's time is
> that we turn it back off, but I have faith that you'd just fix the bugs
> before then anyway.

Yes, I hope to be quick enough to fix things.

> Is the existing Pipelines integration enough for you to turn it on for
> git/git, or do I need to tweak any settings?

All I need is to install the app:

	Install GitGitGadget-Git

	Install on your organization Git @git
	All repositories

	This applies to all current and future repositories.
	Only select repositories
	Selected 1 repository.
	git/git

	...with these permissions:

	Read access to code
	Read access to checks and metadata
	Read and write access to issues and pull requests

... which I just did.

Thanks,
Dscho

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

[Johannes Schindelin]

Hi Peff,

On Fri, 22 Nov 2019, Johannes Schindelin wrote:

> On Thu, 21 Nov 2019, Jeff King wrote:
>
> > On Mon, Nov 18, 2019 at 07:37:57PM +0100, Johannes Schindelin wrote:
> >
> > > To build some confidence in my patches (as you probably know, I do
> > > not trust reviews as much as I trust real-life testing, although I
> > > do prefer to have both) I "kind of" activated it on my fork, limited
> > > to act only on comments _I_ made on PRs (and sending only to me
> > > instead of the list), and it seems to work all right, so far. I
> > > cannot say for sure whether it handles the PR labels correctly, but
> > > I guess time will tell, and I will fix bugs as quickly as I can.
> >
> > Yeah, that makes sense to me. Going from one repo to three is not much
> > worse than going to two, so it's good to have a testing area, too.
> >
> > Do you want any third-party testing there (e.g., a user who isn't you
> > making a PR against dscho/git)?
>
> While that would be nice, my fork is a mess and not really set up to
> provide any useful target branch...
>
> The real proof of the concept will come when the first git/git PR will
> be submitted.

Seems to have worked:
https://public-inbox.org/git/pull.670.git.git.1574433665.gitgitgadget@gmail.com/

Ciao,
Dscho

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

[Jeff King]

On Fri, Nov 22, 2019 at 02:50:05PM +0100, Johannes Schindelin wrote:

> > Wow, thanks for working on this! I don't know that I'd call my
> > javascript skills even rudimentary, but I did give it a look. The real
> > challenge to me is not the individual lines of code, but understanding
> > how the Azure Pipelines and GitHub App systems fit together. So I didn't
> > see anything wrong, but I also know very little about those systems.
> 
> I actually spent some quality time with the wiki in the past days to
> remedy that. You can adore the result in all its beauty here:
> 
> https://github.com/gitgitgadget/gitgitgadget/wiki/GitGitGadget's-Azure-Function-and-Azure-Pipelines

Thanks, this was very informative. I have a feeling that some of this
could be done via the new Actions stuff that GitHub has been shipping,
but I have no idea if it would make any of it easier (and certainly I'm
not advocating dropping a working system to chase a new shiny toy).

> All I need is to install the app:
> [...]
> ... which I just did.

Very cool. :)

-Peff

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

[Johannes Schindelin]

Hi Peff,

On Mon, 25 Nov 2019, Jeff King wrote:

> On Fri, Nov 22, 2019 at 02:50:05PM +0100, Johannes Schindelin wrote:
>
> > > Wow, thanks for working on this! I don't know that I'd call my
> > > javascript skills even rudimentary, but I did give it a look. The real
> > > challenge to me is not the individual lines of code, but understanding
> > > how the Azure Pipelines and GitHub App systems fit together. So I didn't
> > > see anything wrong, but I also know very little about those systems.
> >
> > I actually spent some quality time with the wiki in the past days to
> > remedy that. You can adore the result in all its beauty here:
> >
> > https://github.com/gitgitgadget/gitgitgadget/wiki/GitGitGadget's-Azure-Function-and-Azure-Pipelines
>
> Thanks, this was very informative. I have a feeling that some of this
> could be done via the new Actions stuff that GitHub has been shipping,
> but I have no idea if it would make any of it easier (and certainly I'm
> not advocating dropping a working system to chase a new shiny toy).

It is tempting all right.

The biggest obstacle is that at least one of those Pipelines requires
access to a clone of public-inbox.org/git, and cloning that is rather
expensive. Even a shallow fetch would be super expensive, by virtue of
_all_ the mails being blobs reachable from the tip commit's tree.

Further, GitHub Actions' triggers are a bit too limited: I want this
Pipeline to trigger when public-inbox.org/git is updated, not when any
branch in gitgitgadget/git is updated.

So yes, while it is tempting, it is also not possible right now to use
GitHub Actions.

Ciao,
Dscho

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

[Eric Wong]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> The biggest obstacle is that at least one of those Pipelines requires
> access to a clone of public-inbox.org/git, and cloning that is rather
> expensive. Even a shallow fetch would be super expensive, by virtue of
> _all_ the mails being blobs reachable from the tip commit's tree.

Fwiw, lore.kernel.org/git/$EPOCH.git ought to be somewhat cheaper,
but it's a different (more scalable) format which requires SQLite:

	https://public-inbox.org/public-inbox-v2-format.html

https://lore.kernel.org/git

(and I'm not going to pay extortionist .org fees to keep
 public-inbox.org when it comes up for renewal in 2023,
 maybe everyone can use Tor .onions by then :> )

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

[Johannes Schindelin]

Hi Eric,

On Tue, 26 Nov 2019, Eric Wong wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > The biggest obstacle is that at least one of those Pipelines requires
> > access to a clone of public-inbox.org/git, and cloning that is rather
> > expensive. Even a shallow fetch would be super expensive, by virtue of
> > _all_ the mails being blobs reachable from the tip commit's tree.
>
> Fwiw, lore.kernel.org/git/$EPOCH.git ought to be somewhat cheaper,
> but it's a different (more scalable) format which requires SQLite:
>
> 	https://public-inbox.org/public-inbox-v2-format.html

Is this incremental? GitGitGadget needs this to be incremental ;-)

Ciao,
Dscho

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

[Eric Wong]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Tue, 26 Nov 2019, Eric Wong wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > > The biggest obstacle is that at least one of those Pipelines requires
> > > access to a clone of public-inbox.org/git, and cloning that is rather
> > > expensive. Even a shallow fetch would be super expensive, by virtue of
> > > _all_ the mails being blobs reachable from the tip commit's tree.
> >
> > Fwiw, lore.kernel.org/git/$EPOCH.git ought to be somewhat cheaper,
> > but it's a different (more scalable) format which requires SQLite:
> >
> > 	https://public-inbox.org/public-inbox-v2-format.html
> 
> Is this incremental? GitGitGadget needs this to be incremental ;-)

Incremental as far as "git fetch" goes?  Of course :>
The "m" file is overwritten with every commit, so the tree size
stays at 1 (tree growth was a major scalability problem in v1).

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

[Johannes Schindelin]

Hi Eric,

On Tue, 26 Nov 2019, Eric Wong wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > On Tue, 26 Nov 2019, Eric Wong wrote:
> > > Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > > > The biggest obstacle is that at least one of those Pipelines requires
> > > > access to a clone of public-inbox.org/git, and cloning that is rather
> > > > expensive. Even a shallow fetch would be super expensive, by virtue of
> > > > _all_ the mails being blobs reachable from the tip commit's tree.
> > >
> > > Fwiw, lore.kernel.org/git/$EPOCH.git ought to be somewhat cheaper,
> > > but it's a different (more scalable) format which requires SQLite:
> > >
> > > 	https://public-inbox.org/public-inbox-v2-format.html
> >
> > Is this incremental? GitGitGadget needs this to be incremental ;-)
>
> Incremental as far as "git fetch" goes?  Of course :>
> The "m" file is overwritten with every commit, so the tree size
> stays at 1 (tree growth was a major scalability problem in v1).

Let me try again:

GitGitGadget "reads" the mail via the incremental clone, remembering the
hash of the latest processed commit. When the Azure Pipeline runs, it
first fetches, and if the commit is still the same, does nothing but exit
with success. If the commit is different, it looks at the mails that were
added, via `git log -p <previous-tip-commit>..<tip-commit>`.

Is that possible with the v2 format?

Ciao,
Dscho

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

[Eric Wong]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi Eric,
> 
> On Tue, 26 Nov 2019, Eric Wong wrote:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > > On Tue, 26 Nov 2019, Eric Wong wrote:
> > > > Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > > > > The biggest obstacle is that at least one of those Pipelines requires
> > > > > access to a clone of public-inbox.org/git, and cloning that is rather
> > > > > expensive. Even a shallow fetch would be super expensive, by virtue of
> > > > > _all_ the mails being blobs reachable from the tip commit's tree.
> > > >
> > > > Fwiw, lore.kernel.org/git/$EPOCH.git ought to be somewhat cheaper,
> > > > but it's a different (more scalable) format which requires SQLite:
> > > >
> > > > 	https://public-inbox.org/public-inbox-v2-format.html
> > >
> > > Is this incremental? GitGitGadget needs this to be incremental ;-)
> >
> > Incremental as far as "git fetch" goes?  Of course :>
> > The "m" file is overwritten with every commit, so the tree size
> > stays at 1 (tree growth was a major scalability problem in v1).
> 
> Let me try again:
> 
> GitGitGadget "reads" the mail via the incremental clone, remembering the
> hash of the latest processed commit. When the Azure Pipeline runs, it
> first fetches, and if the commit is still the same, does nothing but exit
> with success. If the commit is different, it looks at the mails that were
> added, via `git log -p <previous-tip-commit>..<tip-commit>`.
> 
> Is that possible with the v2 format?

Of course, yes.  The Xapian and SQLite indexing also works the
same way "git log prev..tip" and storing the latest commit hash.

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

[Junio C Hamano]

Eric Wong <e@80x24.org> writes:

> (and I'm not going to pay extortionist .org fees to keep
>  public-inbox.org when it comes up for renewal in 2023,
>  maybe everyone can use Tor .onions by then :> )

Just on this tangent.  Would you be willing to keep the domain and
keep the service running, if Git Project Leadership Committee pays
the fee out of the funds we keep at Software Freedom Conservancy?

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

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> writes:
> 
> > (and I'm not going to pay extortionist .org fees to keep
> >  public-inbox.org when it comes up for renewal in 2023,
> >  maybe everyone can use Tor .onions by then :> )
> 
> Just on this tangent.  Would you be willing to keep the domain and
> keep the service running, if Git Project Leadership Committee pays
> the fee out of the funds we keep at Software Freedom Conservancy?

Maybe...  I'm against the *principle* of paying extortionists;
and I don't think the Git project should encourage them, either.

Promoting + developing a Distributed Hash Table (DHT) for
Message-ID (and git OID) lookups to fight against centralization
would be a better use of time and funds :>

However, if EFF and other .orgs prove effective in keeping
prices reasonable then that's fine, I guess.  I personally
expect to be financially worse off in 2023 than I was in 2013
when I bought the domain, so some help there could be nice :)

The actual cost of running a service is only $20/month in VPS
hosting.  It's a business expense at the moment as I hack on
that machine for clients, and I'm trying to make public-inbox
cheaper and easier to host, too.

But, https://lore.kernel.org/git/ has professionals behind it
and is more scalable.  It's currently missing syntax
highlighting and blob regeneration because that's a PITA to
configure, though...