Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Pull requests close themselves immediately #257

Closed
benbalter opened this Issue · 9 comments

6 participants

Ben Balter Michael Aufreiter jbenton iyshannon Roman Komarov Dave Cole
Ben Balter

This has happened twice now with two separate users. Non-contributor user gets pushed to prose, makes changes, clicks submit patch. Pull request is generated, but immediately closed.

The repo is properly forked and the change exists as a blob on the forked repo, but it does not appear to be attached to the tree (e.g., looking in history doesn't show the change, trying to cherry pick the SHA results in an error). Not sure if this is a GitHub API issue or a prose issue.

Steps to reproduce

  1. Log in to Github as user without commit access to a repo
  2. Access that repo via Prose (by changing github.com/ to prose.io# in the URL)
  3. Make changes
  4. Click submit patch

Expected

  1. Repo would be forked to user
  2. Changes committed to forked repo and visible in changelog
  3. Pull request opened
  4. Pull request remains open

Actual

  1. Repo forked
  2. Changes committed and accessible via URL but not otherwise visible (e.g., changelog)
  3. Pull request opened
  4. Pull request immediately closed

Examples

If it's relevant, the primary branch is a gh-pages branch in both examples

Michael Aufreiter
Owner

Thanks a lot for this detailed error report. Short on time at the moment but I'll look into this asap.

Michael Aufreiter
Owner

Or even better, if you could help out and look into the corresponding code yourself. Now that you can reproduce the error you should have a good chance to spot what's going wrong.

Here's the corresponding fork/patch/pullrequest logic:

https://github.com/prose/prose/blob/gh-pages/_includes/model.js#L280

jbenton

Hey — I'm in no position to help, but (a) I'm very excited by Prose and want to use it to make a section of my site user-editable, and (b) for some reason, patch requests from non-me users aren't showing up as pull requests for me. I assume it might be related to the issue Ben's talking about, but not sure.

Ben Balter

@michael will take a look.

Believe the issue is related to the branch not properly creating (the branch does not appear in the branches menu of the fork). The fork and the pull request seem to be working as expected.

iyshannon

Just wanted to note that this just happened (#259) when I edited your "Eventually Consistent" document. I came here to the repo to see how the pull requests came through and saw that it was closed. (It says that I closed it, actually).

EDIT: Looking at your code, and my "activity" page, I'm guessing that when you delete the branch, GitHub automatically closes the outstanding Pull Request. My activity page shows that I created the Pull Request, then deleted the branch, and then closed the Pull Request. I haven't tested this, so it could be incorrect, but it makes sense (I think).

Ben Balter benbalter referenced this issue from a commit in benbalter/prose
Ben Balter benbalter Don't delete `prose-patch` branch after submitting patch, fixes #257
When a non-collaborator user clicks commit, prose silently forks the repo, creates a branch, submits a pull request, and deletes the branch. Deleting the branch triggers GitHub to close the pull request, making a subsequent merge impossible.

This patch removes the logic from `_includes/model.js` that deletes the branch following a pull request. However, further refinement of the patching workflow may be required (e.g., to create a new branch for each pull request to avoid smushing commits together).
e872070
Ben Balter

@iyshannon you are correct. The problem stemmed from closing deleting the branch after PR was generated. Submitted #260 which fixed the issue when tested locally.

This however creates a new workflow problem. If I (a non-collaborator) submit a PR, it sits, I go back a week later, and submit another PR (or think I am) it will simply add the new commit to the old pull request (because I am now committing to an existing branch).

One solution may be to uniquely name each branch, e.g., by using the date and time (prose-patch-201211171400), or to simply increment e.g., prose-patch-1, prose-patch-2, etc. Thoughts?

Ben Balter benbalter referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
Ben Balter benbalter referenced this issue from a commit in benbalter/prose
Ben Balter benbalter Auto-increment patch names
Rather than naming all pull requests `prose-patch`, name it `prose-patch-N` where `N` is the lowest possible number not already in use.

This generates the patch name in `forkRepo` and must also retrieve the patch name (from the refs passed to the callback) in `sendPatch`.

See #257 for additional discussion on the problem.
f95ac0c
Roman Komarov

This just happened for me, so it would be nice if this issue would be fixed someday. I'd like to provide link like “edit in prose” for my articles, but it wouldn't be that convenient if the pull requests would be closed right after creating.

So, as I can see, there are already two pull requests regarding this: #260 and #261. @benbalter, the first one seems to have incorrect history, could you try to rebase it so those two pull requests would be easy to merge into Prose?

I also think that it would be nice to have branches not deleted, so one could continue the work on the made changes after he did them. However, it's another issue. The bug that's in this issue should be fixed first.

Roman Komarov kizu referenced this issue in kizu/kizu.github.com
Closed

Updated _posts/issues/2013-01-17-restart.md #46

Ben Balter

For the time being, if you don't mind running prose yourself (you'll need to standup Gatekeeper and create an App on GitHub), this bug is patched in this fork.

You can see an example of it in action at edit.benbalter.com. Pull requests / improvements welcome.

(@kizu thanks for the heads up, will clean up history on the PR)

Dave Cole
Owner

Tracking in #260

Dave Cole dhcole closed this
Dave Cole dhcole referenced this issue from a commit
Ben Balter benbalter Auto-increment patch names
Rather than naming all pull requests `prose-patch`, name it `prose-patch-N` where `N` is the lowest possible number not already in use.

This generates the patch name in `forkRepo` and must also retrieve the patch name (from the refs passed to the callback) in `sendPatch`.

See #257 for additional discussion on the problem.
6ef7388
Dave Cole dhcole referenced this issue from a commit
Ben Balter benbalter Don't delete `prose-patch` branch after submitting patch, fixes #257
When a non-collaborator user clicks commit, prose silently forks the repo, creates a branch, submits a pull request, and deletes the branch. Deleting the branch triggers GitHub to close the pull request, making a subsequent merge impossible.

This patch removes the logic from `_includes/model.js` that deletes the branch following a pull request. However, further refinement of the patching workflow may be required (e.g., to create a new branch for each pull request to avoid smushing commits together).
3cf3be8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.