* Should we auto-close PRs on git/git? @ 2019-11-09 2:00 Emily Shaffer 2019-11-09 4:55 ` Junio C Hamano 2019-11-12 19:11 ` Johannes Schindelin 0 siblings, 2 replies; 22+ messages in thread From: Emily Shaffer @ 2019-11-09 2:00 UTC (permalink / raw) To: git; +Cc: peff 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Should we auto-close PRs on git/git? 2019-11-09 2:00 Should we auto-close PRs on git/git? Emily Shaffer @ 2019-11-09 4:55 ` Junio C Hamano 2019-11-13 5:29 ` Stephen Smith 2019-11-12 19:11 ` Johannes Schindelin 1 sibling, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2019-11-09 4:55 UTC (permalink / raw) To: Emily Shaffer; +Cc: git, peff 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Should we auto-close PRs on git/git? 2019-11-09 4:55 ` Junio C Hamano @ 2019-11-13 5:29 ` Stephen Smith 0 siblings, 0 replies; 22+ messages in thread From: Stephen Smith @ 2019-11-13 5:29 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Emily Shaffer, peff 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Should we auto-close PRs on git/git? 2019-11-09 2:00 Should we auto-close PRs on git/git? Emily Shaffer 2019-11-09 4:55 ` Junio C Hamano @ 2019-11-12 19:11 ` Johannes Schindelin 2019-11-13 1:10 ` Jeff King 2019-11-13 21:09 ` Emily Shaffer 1 sibling, 2 replies; 22+ messages in thread From: Johannes Schindelin @ 2019-11-12 19:11 UTC (permalink / raw) To: Emily Shaffer; +Cc: git, peff 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Should we auto-close PRs on git/git? 2019-11-12 19:11 ` Johannes Schindelin @ 2019-11-13 1:10 ` Jeff King 2019-11-13 12:04 ` Johannes Schindelin 2019-11-13 21:09 ` Emily Shaffer 1 sibling, 1 reply; 22+ messages in thread From: Jeff King @ 2019-11-13 1:10 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Emily Shaffer, git 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Should we auto-close PRs on git/git? 2019-11-13 1:10 ` Jeff King @ 2019-11-13 12:04 ` Johannes Schindelin 2019-11-14 7:41 ` Jeff King 0 siblings, 1 reply; 22+ messages in thread From: Johannes Schindelin @ 2019-11-13 12:04 UTC (permalink / raw) To: Jeff King; +Cc: Emily Shaffer, git 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Should we auto-close PRs on git/git? 2019-11-13 12:04 ` Johannes Schindelin @ 2019-11-14 7:41 ` Jeff King 2019-11-14 23:03 ` Johannes Schindelin 0 siblings, 1 reply; 22+ messages in thread From: Jeff King @ 2019-11-14 7:41 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Emily Shaffer, git 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Should we auto-close PRs on git/git? 2019-11-14 7:41 ` Jeff King @ 2019-11-14 23:03 ` Johannes Schindelin 2019-11-18 18:37 ` GitGitGadget on git/git, was " Johannes Schindelin 0 siblings, 1 reply; 22+ messages in thread From: Johannes Schindelin @ 2019-11-14 23:03 UTC (permalink / raw) To: Jeff King; +Cc: Emily Shaffer, git 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* GitGitGadget on git/git, was Re: Should we auto-close PRs on git/git? 2019-11-14 23:03 ` Johannes Schindelin @ 2019-11-18 18:37 ` Johannes Schindelin 2019-11-21 10:54 ` Jeff King 0 siblings, 1 reply; 22+ messages in thread From: Johannes Schindelin @ 2019-11-18 18:37 UTC (permalink / raw) To: Jeff King; +Cc: Emily Shaffer, git 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: GitGitGadget on git/git, was Re: Should we auto-close PRs on git/git? 2019-11-18 18:37 ` GitGitGadget on git/git, was " Johannes Schindelin @ 2019-11-21 10:54 ` Jeff King 2019-11-22 13:50 ` Johannes Schindelin 0 siblings, 1 reply; 22+ messages in thread From: Jeff King @ 2019-11-21 10:54 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Emily Shaffer, git 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: GitGitGadget on git/git, was Re: Should we auto-close PRs on git/git? 2019-11-21 10:54 ` Jeff King @ 2019-11-22 13:50 ` Johannes Schindelin 2019-11-22 14:43 ` Johannes Schindelin 2019-11-25 14:30 ` Jeff King 0 siblings, 2 replies; 22+ messages in thread From: Johannes Schindelin @ 2019-11-22 13:50 UTC (permalink / raw) To: Jeff King; +Cc: Emily Shaffer, git 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: GitGitGadget on git/git, was Re: Should we auto-close PRs on git/git? 2019-11-22 13:50 ` Johannes Schindelin @ 2019-11-22 14:43 ` Johannes Schindelin 2019-11-25 14:30 ` Jeff King 1 sibling, 0 replies; 22+ messages in thread From: Johannes Schindelin @ 2019-11-22 14:43 UTC (permalink / raw) To: Jeff King; +Cc: Emily Shaffer, git 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: GitGitGadget on git/git, was Re: Should we auto-close PRs on git/git? 2019-11-22 13:50 ` Johannes Schindelin 2019-11-22 14:43 ` Johannes Schindelin @ 2019-11-25 14:30 ` Jeff King 2019-11-26 20:55 ` Johannes Schindelin 1 sibling, 1 reply; 22+ messages in thread From: Jeff King @ 2019-11-25 14:30 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Emily Shaffer, git 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: GitGitGadget on git/git, was Re: Should we auto-close PRs on git/git? 2019-11-25 14:30 ` Jeff King @ 2019-11-26 20:55 ` Johannes Schindelin 2019-11-26 21:56 ` Eric Wong 0 siblings, 1 reply; 22+ messages in thread From: Johannes Schindelin @ 2019-11-26 20:55 UTC (permalink / raw) To: Jeff King; +Cc: Emily Shaffer, git 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: GitGitGadget on git/git, was Re: Should we auto-close PRs on git/git? 2019-11-26 20:55 ` Johannes Schindelin @ 2019-11-26 21:56 ` Eric Wong 2019-11-26 22:22 ` Johannes Schindelin 2019-11-27 1:52 ` Junio C Hamano 0 siblings, 2 replies; 22+ messages in thread From: Eric Wong @ 2019-11-26 21:56 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Jeff King, Emily Shaffer, git 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 :> ) ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: GitGitGadget on git/git, was Re: Should we auto-close PRs on git/git? 2019-11-26 21:56 ` Eric Wong @ 2019-11-26 22:22 ` Johannes Schindelin 2019-11-26 22:40 ` Eric Wong 2019-11-27 1:52 ` Junio C Hamano 1 sibling, 1 reply; 22+ messages in thread From: Johannes Schindelin @ 2019-11-26 22:22 UTC (permalink / raw) To: Eric Wong; +Cc: Jeff King, Emily Shaffer, git 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: GitGitGadget on git/git, was Re: Should we auto-close PRs on git/git? 2019-11-26 22:22 ` Johannes Schindelin @ 2019-11-26 22:40 ` Eric Wong 2019-11-26 22:52 ` Johannes Schindelin 0 siblings, 1 reply; 22+ messages in thread From: Eric Wong @ 2019-11-26 22:40 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Jeff King, Emily Shaffer, git 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). ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: GitGitGadget on git/git, was Re: Should we auto-close PRs on git/git? 2019-11-26 22:40 ` Eric Wong @ 2019-11-26 22:52 ` Johannes Schindelin 2019-11-26 23:58 ` Eric Wong 0 siblings, 1 reply; 22+ messages in thread From: Johannes Schindelin @ 2019-11-26 22:52 UTC (permalink / raw) To: Eric Wong; +Cc: Jeff King, Emily Shaffer, git 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: GitGitGadget on git/git, was Re: Should we auto-close PRs on git/git? 2019-11-26 22:52 ` Johannes Schindelin @ 2019-11-26 23:58 ` Eric Wong 0 siblings, 0 replies; 22+ messages in thread From: Eric Wong @ 2019-11-26 23:58 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Jeff King, Emily Shaffer, git 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: GitGitGadget on git/git, was Re: Should we auto-close PRs on git/git? 2019-11-26 21:56 ` Eric Wong 2019-11-26 22:22 ` Johannes Schindelin @ 2019-11-27 1:52 ` Junio C Hamano 2019-11-27 2:37 ` Eric Wong 1 sibling, 1 reply; 22+ messages in thread From: Junio C Hamano @ 2019-11-27 1:52 UTC (permalink / raw) To: Eric Wong; +Cc: Johannes Schindelin, Jeff King, Emily Shaffer, git 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? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: GitGitGadget on git/git, was Re: Should we auto-close PRs on git/git? 2019-11-27 1:52 ` Junio C Hamano @ 2019-11-27 2:37 ` Eric Wong 0 siblings, 0 replies; 22+ messages in thread From: Eric Wong @ 2019-11-27 2:37 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin, Jeff King, Emily Shaffer, git 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... ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: Should we auto-close PRs on git/git? 2019-11-12 19:11 ` Johannes Schindelin 2019-11-13 1:10 ` Jeff King @ 2019-11-13 21:09 ` Emily Shaffer 1 sibling, 0 replies; 22+ messages in thread From: Emily Shaffer @ 2019-11-13 21:09 UTC (permalink / raw) To: Johannes Schindelin; +Cc: git, peff 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-11-27 2:37 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-09 2:00 Should we auto-close PRs on git/git? Emily Shaffer 2019-11-09 4:55 ` Junio C Hamano 2019-11-13 5:29 ` Stephen Smith 2019-11-12 19:11 ` Johannes Schindelin 2019-11-13 1:10 ` Jeff King 2019-11-13 12:04 ` Johannes Schindelin 2019-11-14 7:41 ` Jeff King 2019-11-14 23:03 ` Johannes Schindelin 2019-11-18 18:37 ` GitGitGadget on git/git, was " Johannes Schindelin 2019-11-21 10:54 ` Jeff King 2019-11-22 13:50 ` Johannes Schindelin 2019-11-22 14:43 ` Johannes Schindelin 2019-11-25 14:30 ` Jeff King 2019-11-26 20:55 ` Johannes Schindelin 2019-11-26 21:56 ` Eric Wong 2019-11-26 22:22 ` Johannes Schindelin 2019-11-26 22:40 ` Eric Wong 2019-11-26 22:52 ` Johannes Schindelin 2019-11-26 23:58 ` Eric Wong 2019-11-27 1:52 ` Junio C Hamano 2019-11-27 2:37 ` Eric Wong 2019-11-13 21:09 ` Emily Shaffer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).