Skip to content

[Bug] Remote branch not deleted by usethis::pr_finish() #1150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fh-kpikhart opened this issue Jun 5, 2020 · 5 comments · Fixed by #1144
Closed

[Bug] Remote branch not deleted by usethis::pr_finish() #1150

fh-kpikhart opened this issue Jun 5, 2020 · 5 comments · Fixed by #1144
Labels
git git, GitHub, and CI in general reprex needs a minimal reproducible example

Comments

@fh-kpikhart
Copy link

When using usethis::pr_finish(), my local branch is deleted, but not the remote branch.

I wonder if there is a bug where the == should be > here:
https://github.com/r-lib/usethis/blob/master/R/pr.R#L350

> usethis::pr_finish()
✔ Switching back to 'master' branch
✔ Pulling changes from GitHub source repo 'origin/master'
✔ Deleting local 'xxx' branch
> 

I don't get the expected output from ui_done(), and the branch is not deleted from GitHub

ui_done("Removing remote {ui_value(remote)}")
@jennybc
Copy link
Member

jennybc commented Jun 5, 2020

What version of usethis do you have?

If it's not a very recent dev version, can you please install from GitHub and see if you still have the problem?

If so, a more complete reprex is helpful, as it's very hard for functions like this to work with all possible initial states. I would need to see the situation faced by pr_finish(). We use the pr_*() functions heavily internally, but not everyone shares the same workflow.

@fh-kpikhart
Copy link
Author

> devtools::session_info() %$% packages
 ! package      * version    date       lib source                                 
   assertthat     0.2.1      2019-03-21 [1] CRAN (R 3.6.1)                         
   backports      1.1.7      2020-05-13 [1] CRAN (R 3.6.1)                         
   bit            1.1-15.2   2020-02-10 [1] CRAN (R 3.6.0)                         
   bit64          0.9-7      2017-05-08 [1] CRAN (R 3.6.0)                         
   blob           1.2.1      2020-01-20 [1] CRAN (R 3.6.0)                         
   callr          3.4.3      2020-03-28 [1] CRAN (R 3.6.2)                         
   cli            2.0.2      2020-02-28 [1] CRAN (R 3.6.0)                         
   clisymbols     1.2.0      2017-05-21 [1] CRAN (R 3.6.1)                         
   commonmark     1.7        2018-12-01 [1] CRAN (R 3.6.1)                         
   crayon         1.3.4      2017-09-16 [1] CRAN (R 3.6.1)                         
   curl           4.0        2019-07-22 [1] CRAN (R 3.6.1)                         
   cyclocomp      1.1.0      2016-09-10 [1] CRAN (R 3.6.0)                         
   DBI            1.1.0      2019-12-15 [1] CRAN (R 3.6.0)                         
   dbplyr         1.4.2      2019-06-17 [1] CRAN (R 3.6.1)                         
   desc           1.2.0      2018-05-01 [1] CRAN (R 3.6.1)                         
   devtools       2.1.0      2019-07-06 [1] CRAN (R 3.6.1)                         
   digest         0.6.25     2020-02-23 [1] CRAN (R 3.6.0)                         
   dplyr          0.8.3      2019-07-04 [1] CRAN (R 3.6.1)                         
   fansi          0.4.1      2020-01-08 [1] CRAN (R 3.6.0)                         
   fs             1.4.1      2020-04-04 [1] CRAN (R 3.6.2)                         
   gh             1.0.1      2017-07-16 [1] CRAN (R 3.6.1)                         
   git2r        * 0.27.1     2020-05-03 [1] CRAN (R 3.6.1)                         
   glue         * 1.3.1      2019-03-12 [1] CRAN (R 3.6.1)                         
   httr           1.4.1      2019-08-05 [1] CRAN (R 3.6.1)                         
   jsonlite       1.6        2018-12-07 [1] CRAN (R 3.6.1)                         
   knitr          1.24       2019-08-08 [1] CRAN (R 3.6.1)                         
   lattice        0.20-38    2018-11-04 [1] CRAN (R 3.6.1)                         
   lazyeval       0.2.2      2019-03-15 [1] CRAN (R 3.6.1)                         
   lintr          2.0.1      2020-02-19 [1] CRAN (R 3.6.0)                         
   magrittr     * 1.5        2014-11-22 [1] CRAN (R 3.6.1)                         
   memoise        1.1.0      2017-04-21 [1] CRAN (R 3.6.1)                         
   pillar         1.4.2      2019-06-29 [1] CRAN (R 3.6.1)                         
   pkgbuild       1.0.5      2019-08-26 [1] CRAN (R 3.6.1)                         
   pkgconfig      2.0.3      2019-09-22 [1] CRAN (R 3.6.0)                         
   pkgload        1.0.2      2018-10-29 [1] CRAN (R 3.6.1)                         
   prettyunits    1.0.2      2015-07-13 [1] CRAN (R 3.6.1)                         
   processx       3.4.2      2020-02-09 [1] CRAN (R 3.6.0)                         
   ps             1.3.3      2020-05-08 [1] CRAN (R 3.6.2)                         
   purrr          0.3.4      2020-04-17 [1] CRAN (R 3.6.2)                         
   R6             2.4.1      2019-11-12 [1] CRAN (R 3.6.0)                         
   Rcpp           1.0.4.6    2020-04-09 [1] CRAN (R 3.6.1)                         
   remotes        2.1.0      2019-06-24 [1] CRAN (R 3.6.1)                         
   rex            1.2.0      2020-04-21 [1] CRAN (R 3.6.2)                         
   rlang          0.4.6      2020-05-02 [1] CRAN (R 3.6.2)                         
   roxygen2       7.0.2      2019-12-02 [1] CRAN (R 3.6.1)                         
   RPostgreSQL    0.6-2      2017-06-24 [1] CRAN (R 3.6.1)                         
   rprojroot      1.3-2      2018-01-03 [1] CRAN (R 3.6.1)                         
   RSQLite        2.2.0      2020-01-07 [1] CRAN (R 3.6.0)                         
   rstudioapi     0.10       2019-03-19 [1] CRAN (R 3.6.1)                         
   x      0.1.0      2020-05-13 [1] local                                  
 P x      * 0.1.3.9000 2020-05-29 [?] Github (flatironhealth/x@154f3eb)
   sessioninfo    1.1.1      2018-11-05 [1] CRAN (R 3.6.1)                         
   stringi        1.4.3      2019-03-12 [1] CRAN (R 3.6.1)                         
   stringr        1.4.0      2019-02-10 [1] CRAN (R 3.6.1)                         
   testthat     * 2.2.1      2019-07-25 [1] CRAN (R 3.6.1)                         
   tibble         2.1.3      2019-06-06 [1] CRAN (R 3.6.1)                         
   tidyselect     1.1.0      2020-05-11 [1] CRAN (R 3.6.2)                         
   usethis      * 1.5.1      2019-07-04 [1] CRAN (R 3.6.1)                         
   utf8           1.1.4      2018-05-24 [1] CRAN (R 3.6.1)                         
   vctrs          0.2.4      2020-03-10 [1] CRAN (R 3.6.1)                         
   withr          2.2.0      2020-04-20 [1] CRAN (R 3.6.2)                         
   xfun           0.13       2020-04-13 [1] CRAN (R 3.6.2)                         
   xml2           1.2.2      2019-08-09 [1] CRAN (R 3.6.1)                         
   xmlparsedata   1.0.3      2019-09-27 [1] CRAN (R 3.6.0)                         
   zoo            1.8-6      2019-05-28 [1] CRAN (R 3.6.1)                         

[1] /Library/Frameworks/R.framework/Versions/3.6/Resources/library

 P ── Loaded and on-disk path mismatch.

My workflow is:

usethis::pr_pull_upstream()
usethis::pr_init("<your_branch_name>")
usethis::pr_push()
usethis::pr_finish()

Example:

kpikhart ~/code/x (master) $ git branch -a | grep "reprex"
kpikhart ~/code/x (master) $

> usethis::pr_pull_upstream()
✔ Pulling changes from GitHub source repo 'origin/master'
> usethis::pr_init("usethis_reprex")
✔ Checking that local branch 'master' has the changes in 'origin/master'
✔ Creating local PR branch 'usethis_reprex'
✔ Switching to branch 'usethis_reprex'
● Use `pr_push()` to create PR

kpikhart ~/code/x (usethis_reprex) $ git branch -a | grep "reprex"
* usethis_reprex

> usethis::pr_push()
✔ Pushing local 'usethis_reprex' branch to 'origin:usethis_reprex'
✔ Setting upstream tracking branch for 'usethis_reprex' to 'origin/usethis_reprex'
✔ Create PR at link given below
✔ Opening URL 'https://github.com/flatironhealth/x/compare/usethis_reprex'

kpikhart ~/code/x (usethis_reprex) $ git branch -a | grep "reprex"
* usethis_reprex
  remotes/origin/usethis_reprex

> usethis::pr_finish()
✔ Switching back to 'master' branch
✔ Pulling changes from GitHub source repo 'origin/master'
✔ Deleting local 'usethis_reprex' branch

kpikhart ~/code/x (usethis_reprex) $ git branch -a | grep "reprex"
  remotes/origin/usethis_reprex

> usethis::pr_pull_upstream()
✔ Pulling changes from GitHub source repo 'origin/master'

kpikhart ~/code/x (master) $ git branch -a | grep "reprex"
  remotes/origin/usethis_reprex

Is == the right comparator to use in that line of code?

@jennybc
Copy link
Member

jennybc commented Jun 5, 2020

usethis      * 1.5.1      2019-07-04 [1] CRAN (R 3.6.1)

So you are NOT using the dev version of usethis. Quoting from above:

If it's not a very recent dev version, can you please install from GitHub and see if you still have the problem?

You asked "Is == the right comparator to use in that line of code?" Yes I believe so because if there are other still-existing branches associated with that remote, we should not delete the remote. Note there is a difference between a remote and a remote branch.

@jennybc jennybc added git git, GitHub, and CI in general reprex needs a minimal reproducible example labels Jun 5, 2020
@fh-kpikhart
Copy link
Author

I'm not sure how to identify other branches that are associated with that remote. I just created the dummy remote for the purpose of this reprex, so it seems unlikely that anything else should be associated with it.

I will try the dev version and let you know what I see!

@jennybc
Copy link
Member

jennybc commented Jul 3, 2020

OK I see what this is about now.

I am usually dealing with PRs as maintainer and/or as a participant in an internal PR. In the case of an internal PR, we typically delete the branch at the time we merge (or close) the PR. In the case of an external PR, I don't generally see that, from the PR contributor's POV, pr_finish() does not delete the branch in their fork.

I can reproduce it.

jennybc added a commit that referenced this issue Jul 4, 2020
@jennybc jennybc mentioned this issue Jul 4, 2020
jennybc added a commit that referenced this issue Jul 8, 2020
* Import gert

* uses_git(), check_uses_git()

* Add tests for uses_git(), check_uses_git()

* Create gert_repo() to use like git_repo() during the switchover

* git_config_get() --> git_cfg_get()

* git_config_set() --> gert::git_config_set(), gert::git_config_global_set()

* Take care this myself

If it happens in gert, I can simplify things here.

r-lib/gert#37

* Re-document()

* Rename git.R to use_git.R

* Kill git_config(), update use_git_config()

* git2r::init() --> gert::git_init()

* git2r::branch_delete() --> gert::git_branch_delete()

* git2r::checkout() --> gert::git_branch_checkout()

* git_branch_current(), git_branch(), git_branch_name() --> git_branch()

* Deal with unborn branch edge case

* Add test for git_branch()

* Use git_repo() for the gert version and git2r_repo() for the legacy git2r version

Move all the gert stuff back into git-utils.R

* git_branch_create() --> git_branch_create_and_switch(), git2r::branch_create() --> gert::git_branch_create()

* First, easy step towards being able to delete git_commit_find()

* Install libgit2 for the macOS + r-devel build

* git2r::branches() --> gert::git_branch_list()

* Leave note to revisit this

* Be more flexible re: finding remote for use_dev_package()

* Use dev gert

* git2r::status() --> gert::git_status()

* gitr2::add(), commit() --> gert::git_add(), git_commit()

* Switch over git_ask_commit()

* Recover from file renaming that went sideways

* Delete git_has_commits()

No longer used anywhere

* Whitespace

* git_ask_commit() and friends; moving things

* Add git_status(), no default for `untracked`
* Remove default for `untracked` from `git_ask_commit()`
* Intersect `paths` with status results in `git_ask_commit()`

* Install libgit2 in the test run because dev gert means we must build from source

* No immediate reason we would be installing magick from source

* pkgdown run will also need dev gert and therefore libgit2

* Brain dump some notes

* Remove `path = proj_get()` from from git helpers; prefer git_repo() over proj_get()

Inspired by working on principles.md in 8c8dbdf

Removing `path` as an arguement from internal git helpers where I think I should make implicit, hard-wired use of active project/repo

Prefer git_repo() to proj_get() because of the implicit check for git-repo-hood. The only reason to use proj_get() in the git helpers is when when we helping to establish the active project as a git repo.

Found a few places where I was neglecting to specify `repo = git_repo()` in gert calls. Although our preferred workflow will have user working in working directory that is (or is below) the active project, that is not an absolute requirement.

* Continuing to implement ideas recently recorded in principles.md

* Say what to do for noninteractive commit

* git2r::remotes(), git2r::remote_url() --> gert::git_remote_list()

* Better check for this argument

* git2r::remote_*() --> gert::git_remote_*()

* Inline logic from git_remote_find(), delete it

* Delete git_remref()

* Delete git_remote_exists()

* Eliminate git2r from git_parse_remref()

* Renaming github related files

* More renaming

* Remove interactivity that accomplishes very little

If user is not happy with the repo_name or repo_desc, there's no easy remedy offered anyway.

The defaults are quite good and both name and description can be modified later, so nothing is being written in stone here.

* check_no_uncommitted_changes() is a more accurate name

* Fix git_ask_commit()

* Work on use_github() docs

* Return invisibly

* Use lifecycle

* Switch use_github() over to gert

* Update and run use_github() manual test

* Tweak docs

* Don't need to import deprecate_soft(), since I legit need to import deprecated()

* Reminder to stay informed whether git_push() continues to automatically set tracking branch

* Go back to original file names

Now that the utils are in utils-git.R and utils-github.R, I see that it's better to NOT have a content-free `use_` prefix all over the place

* Work on use_github() docs

* Import glue::glue_data()

* length() should be sum()

* Introduce helpers that analyze the github remote setup

* Chipping away ...

* Error handling for bad GitHub config

* Doc tweaks

* Remotes need to know their own name

* Work on format and print methods for github_config()

* Fix wordo

* That was unnecessary

* Introduce ui_unset()

* More work on presentations of GitHub remote config

* Explicit args for git_push()

The defaults are (or will be) what I need / want and yet it still seems wise to be explicit here. For robustness and also self-documentation.

* Using dev roxygen2

* Line length

* Beef up `desc` for GitHub remote configs we discourage

* Swith use_github_links() over

* Need to strip the `glue` class to make desc happy

* Update manual test re: use_github()

* Allow for the use of glue::glue() to produce `value`

Also whitespace fix

* Catch error from use_github_links() to avoid partial setup

Fixes #1137

* Fix whitespace, linebreaks

* Tidy up and trim down to one helper

* Add `add = TRUE` to some on.exit()s

* Rename create_github_token() (AGAIN); incorporate credentials::set_github_pat()

Closes #1154

* Another place to alter advice re: storing GITHUB_PAT

* Small improvements to use_github_links()

* Whitespace / linebreaks

* Why can't this be NULL?

If it can't, I guess I'll find out.

* Move this to a helper I can use elsewhere

* Start to replace check_uses_github()

* This needs to truly exit

* Tweak whitespace/linebreaks

* Account for a project that lacks a README

* Switch over use_travis(), use_travis_badge()

* Switch over appveyor functions

* Re-document()

* Switch over Circle CI functions

* Extend get_github_primary()

Re-conceive "is_fork" as "in_fork"

Re-work config data for "fork_no_upstream"

* Beef up `desc` for supported configs that can lead to errors

These configs are untenable for specific functions, but not in general.

* Retrospectively apply `in_fork` idea, don't claim primary is `upstream`

* Switch over release functions

* Fix test

* Switch over github label functions

* Switch over test coverage functions

* Style, reflow roxygen comments

* Update wordlist

* `repo_spec` is guaranteed to exist

* Add repo_spec argument to use_coverage()

* Switch over github actions functions

* Switch create_from_github() over

* Remove use_git_credentials()

* Remove this documented argument

* Moving towards a new repo_spec helper

* Housekeeping pass on GitHub Actions stuff

* Update travis functions

* Update appveyor functions

* Groom non-GHA CI stuff

Lots of uses_*() and check_uses_*() helpers are never called. I deleted them. I suspect they were added by PR contributors following what they saw for, e.g., travis.

* Clean up coverage

* Update github labels functions

* Another helper to get primary repo spec; update use_tidy_thanks()

* Update use_binder_badge()

* Update release-related functions

* Make it possible to exit github_remotes2() w/o hitting the API

* Switch uses_github() over to github_remotes2()

* Switch over project_data()

* Update use_readme_[r]md()

* Delete get_github_primary()

* More touches to get_repo_spec() and

* '4.0' --> 'release' in GHA

* More internal documentation

* Install imagemagick in test coverage workflow

This is needed whenever magick updates on CRAN and there's no binary

* Fix mistakes left from previous refactoring

* Introduce check_pr_readiness(), stop_unsupported_pr_config()

* Work on `pr_*` docs

* Add git_branch_upstream()

* Refactor git_branch_compare()

Can't quite eliminate git2r until I gert gets something like ahead_behind()

* Refactor check_branch_pulled()

Now we always assume we're dealing with the current local branch and there is the option to specify the remote branch. But if unspecified, we honor upstream tracking branch.

* Switch over pr_init()

* Introduce pr_resume()

* Edit error message

* Switch over `git_pull()`

* Don't assume remote and local branch have same name

* Message when we delegate to pr_resume()

* Create pr_pull_from_primary()

* Better message when we can't pull

* Messages

* Stop passing credentials to git_pull()

* Working on git push/pull and tracking branch

* Progress on pr_fetch()

* Tweak message

* More work on pr_fetch()

* Delete git_branch_remote()

* git_push() and pr_push()

* Delete git_branch_track()

* Make pr_finish() at least pass R CMD check

* Re-document()

* Delete git_commit_find()

* Delete git_branch_tracking() and git_branch_tracking_FIXME()

* Update WORDLIST

* Update pr_create_gh()

* Notes and tweaks

* Reclaim git_branch_tracking()

* Rename this to pr_pull_primary_default()

Meant to evoke:
git pull upstream master

But where we generalize:
upstream --> primary
master --> default (aspirational)

* Switch over pr_finish()

* Fix URL

* Nonexistent tracking branch is NA, not NULL

* Fill in URL

* Whitespace

* Make R-devel happy re: Rd links

* Remove git2r

* Preferred vocabulary

* Tweak manual test

* Work on pr_url()

* Work on pr_pull_upstream()

* Add a fetch to pr_fetch()

* Define `branch`

* Set remote tracking branch

* Fix the refspec

* Remove a usage of git_pull_upstream()

* Revert a previous change, now that roxygen2 7.1.1 does the link the way R-devel wants

* Comments and docs tweaks

* Set upstream tracking here

* Account for the glue class

Glue vs not clue makes `identical()` fail, when it should not

* Re-document()

* Continue to clear my head about various git pulls

* Working on PR docs

* Remove the `owner` argument of `pr_fetch()`

* No longer seems weird to me

* Be consistent with style elsewhere

* No need to incur all the overhead and extra verbiage of pr_pull()

* Use an object that actually exists

* Redundant with the messaging from git_pull()

* Guard again the NA

Can't use `identical()` because of glue class

* This is only meaningful for external PRs

`maintainer_can_modify` is `FALSE` for internal PRs, but maintainers can push into the branch, by definition

* Inline all git pushes

* Rename: pr_pull_upstream() --> pr_merge_main()

* Use the right remote

* Deal with docs

* More re: the rename

* NEWS updates

* Delete github_owner_upstream()

* Update pkgdown functions and docs

* WIP Delete github_owner(), github_repo()

* Deleting more old helpers

* Delete github_repo_spec()

* Stop tracking this

* Refactor browse.R

* Update WORDLIST

* Better comment

* Refactor GitHub URL parsing

* Complete the switch over of github_remotes()

* Rework old uses_github(), check_uses_github()

* Not used anywhere

* This check is really specific to PRs

* Say less

I either need to say less or also cover branch switching via RStudio or gert.

* Fixes bugs revealed by manual testing of pr_push()

* Delete the remote branch in pr_finish()

Closes #1150

* Add NEWS bullet

* Work on NEWS

* Update principles.md

* Use the new gert::git_remote_set_url()

* Share auth docs between use_github() and create_from_github()

* Work on deprecations

* Docs for create_github_token()

* Update wordlist

* Refactor github token helpers

Closes #1133 "Error message from github_user() is concealed". Any uncaught error is now revealed in the error we throw.

We also challenge "accidental non-forks" in interactive sessions, i.e. when someone uses `create_from_github()` on a repo they can't push to, without a GitHub PAT.

We used to silently just clone, which lead to lots of pain at tidyverse dev days, when people did not complete the setup, but hoped to make a PR.

* Better, earlier error when no PAT available

* Whitespace

* check_github_token() no longer returns the token

Stop using its return value as a PAT

* Update manual tests

* Add TODO

* Update manual tests

* Unbork use_git_remote()

It is VERY important to use gert::git_remote_add() to set up a new remote, as opposed to get_remote_set_url(). The latter appears to work, but it does not fully set up the remote. In particular the fetch refspec is not configured, so operations like setting upstream tracking branch and merging fail with very mysterious errors.

* More edits to manual tests

* Don't run this example

* Bump gert version

This was an artificial version number for totally unrelated reasons, but I want people to have the "latest". So this needs to at least reflect the highest version number there is.

* Re-document()

* Groom this file

* Bring the internal github helpers back under one roof

* Fix cfg_upstream_only(); add test

Part of #1175

* Fix cfg_upstream_but_origin_is_not_fork(); add tests

Closes #1175

* The next version will be 2.0.0

* Offer to abort a conflicted merge

Closes #1168
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
git git, GitHub, and CI in general reprex needs a minimal reproducible example
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants