Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Add branch tracking to development workflow instructions#28575

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

juanis2112
Copy link
Contributor

PR summary

(This change was done during SciPy2024 sprints)

It's a good practice to set up the branch tracking when pushing changes so I have changed the push command to have the tracking. However, this doesn't work in git versions lower than 1.7 so I added a tab without this for previous versions. This is how it looks like:

Screenshot 2024-07-14 at 11 14 53 AMScreenshot 2024-07-14 at 11 14 46 AM

PR checklist

@github-actionsgithub-actionsbot added the Documentation: devdocsfiles in doc/devel labelJul 14, 2024
Copy link

@github-actionsgithub-actionsbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

@timhoffm
Copy link
Member

timhoffm commentedJul 14, 2024
edited
Loading

Thanks for the PR! I believe, the current form is historic and should be reconsidered fundamentally.

git 1.7 was released in 2010. Do we really have to make things more complicated by distinguisheing and explaining workflows for software that is 14 years old? I'd rather just assume that everybody has a newer version.

But irrespectively, I would not recommend--set-upstream here. Yes,--set-upstream has its justification, but if you are not a git expert,--set-upstream is more complicated:

  • you have to learn what it does (or just use it without questioning, but that makes git look even more complicated)
  • you have to learn what the benefits are for subsequent commands (mostly git psuh) are and how they look. Otherwise you've gained nothing. . But for a beginner, I think it's still more complicated to learn: usegit push --set-upstream origin my-branch for the first push andgit push afterwards; rather than just learn alwaysgit push origin my-branch.

I propose to just delete this block:
grafik

Note also thathttps://learn.scientific-python.org/contributors/first-contribution/ does not mention--set-upstream. I consider that as the canonical workflow guideline. If--set-upstream should be discussed, I would vote to get it in there and link to it from the matplotlib docs "for a more refined workflow".

Additionally, I propose to put the following paragraph into a.. hint:: to set it apart from the standard workflow:
grafik

jklymak reacted with thumbs up emoji

@story645
Copy link
Member

Do we really have to make things more complicated by distinguisheing and explaining workflows for software that is 14 years old? I'd rather just assume that everybody has a newer version.

To be fair to@juanis2112, she said the same thing and I was the one who said to put it in just in case there was someone who had very strong opinions about us keeping it. If there aren't any, then 🤷‍♀️ I don't have an issue with deleting it.

@juanis2112
Copy link
ContributorAuthor

Thanks for your comments@timhoffm. I was actually the one who wrote the scientific python guide and now I'm reconsidering some stuff there based on the experience I've had at the SciPy sprints. I will probably review it and make some edits after the sprints.

I agree there's no need to add workflow explanations for old versions of git so I'll go ahead and remove the tab.

About tracking the branches, I added the 'set-upstream' part because that's what git suggests when you only do 'git push' but it does require further understanding. It's definitely not needed for doing the 'first PR' but if there's extra changes needed within the same branch, this is something that people will eventually have to do so maybe it is good to have it somewhere after. For example in thesprints guide it is mentioned later. In the Matplotlib guide it probably belongs to the section 'Update a pull request'. Let me know if this makes sense and I'll do it that way.

Also I'm happy to implement the hint too.

@rcomer
Copy link
Member

I’m confused by this as I always just dogit push origin and don’t usually have a problem. I have only occasionally seen the error about not having an upstream branch. I’m not certain, but I think it happens when I branch off a branch that I subsequently delete.

@juanis2112
Copy link
ContributorAuthor

juanis2112 commentedJul 14, 2024
edited
Loading

These are my two recent changes. If you all agree, I can update the PR description and screenshots!
Screenshot 2024-07-14 at 2 25 37 PM

Screenshot 2024-07-14 at 2 25 28 PM
timhoffm and rcomer reacted with thumbs up emoji

@QuLogicQuLogic added the mentored: sprintIssues intended and suitable for sprints labelJul 14, 2024
@timhoffm
Copy link
Member

I will probably review it and make some edits after the sprints.

🚀

I would find a diagram of the workflow helpful. There are many visualization variants out there. I liktethis one, tough I would make a stronger visual connection between the the two occurences (main + feature branch) of the local and of the remote repos.

@rcomer
Copy link
Member

Oh, I get it now. Thanks!

@scottshambaugh
Copy link
Contributor

It looks like some exception handling changes accidentally made it into this PR branch?

rcomer reacted with eyes emoji

@rcomer
Copy link
Member

It looks like some exception handling changes accidentally made it into this PR branch?

Those are onmain - a rebase should fix this.

@@ -0,0 +1,26 @@
Exception handling control
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Something does not look right with the git history here.

@juanis2112
Copy link
ContributorAuthor

Thanks for your suggestions@timhoffm. I just made a commit with the changes.

I'll also look into the diagram idea once I start working on the SP guides.

Not sure what the 'Exeption handling control' changes are. I did do a rebase on this PR after opening since there were some changes pushed to main while I was working on it.@tacaswell please let me know what to do or how to fix this.

@story645
Copy link
Member

Not sure what the 'Exeption handling control' changes are. I did do a rebase on this PR after opening since there were some changes pushed to main while I was working on it

Could you have accidentally pulled main into your feature branch when rebasing? That exception came in from#28573 and another rebase should fix it.

@story645story645 mentioned this pull requestJul 18, 2024
11 tasks
@QuLogic
Copy link
Member

QuLogic commentedJul 18, 2024
edited
Loading

There are two copies of this "Add tabs for push command depending on git version" commit (c8440972d55730dfe5dc016e56a10edb49aeee32 andffdda7e590cd2180284ccf236fe06bd02552e7fd):
Screenshot from 2024-07-18 15-08-37
Likely what happened is that you rebased, and then you followed git's advice to do agit pull due to branch divergence. However, after a rebase, you've moved the commit (and it thus has a new hash), so youwant to delete the old copy and do a force push to the remote. To be safer, you can do--force-with-lease to have git verify that the commit you are replacing on the remote is the same one that git thought it was locally.

To fix the PR, you can do the following:

$ git switch main$ git pull$ git switch fix-development-workflow$ git rebase --interactive main

In the editor that opens, remove the line with the old copy of the duplicate commit (ffdda7e590), and if there are any, also remove any commits that you didn't do (but I don't think this should happen). This should end up as:

pick c8440972d5 Add tabs for push command depending on git versionpick 2603c1f5b1 Delete tab for branch tracking workflowpick 54df55604c Apply suggestions from code review

Save, quit the editor, and the rebase should complete without issue. Then you need to force push to delete the extra commits on the branch on GitHub:

$ git push --force-with-lease origin fix-development-workflow

(You can leave out the remote and branch name if you have tracking set up, but it's nice to be explicit when force pushing.)

If you want to do a little extra, as part of the interactive rebase, you can change the second and third lines frompick tosquash (because the second commit basically reverts most of the first), and then update the commit message.

story645 reacted with thumbs up emoji

@juanis2112
Copy link
ContributorAuthor

Thanks@QuLogic. I followed the steps, including squashing the commits and updating the commit message. However I don't see the new commit message reflected.

Also, the circle ci tests are still failing but it doesn't seem related to any files I changed. Thoughts?

@story645
Copy link
Member

However I don't see the new commit message reflected.

Since it looks like the actual commit (at least on here) is fine, you can trygit amend to change the message, thengit push (or maybegit push --force-with-lease to change it.

@QuLogic
Copy link
Member

QuLogic commentedJul 20, 2024
edited
Loading

To fix the PR, you can do the following:

$ git switch main$ git pull

Also, the circle ci tests are still failing but it doesn't seem related to any files I changed. Thoughts?

Ah sorry, in the instructions quoted above, I assumed yourmain branch was tracking theupstream remote, but it seems to be tracking yourorigin fork (you can check withgit branch -vvv), so things went backwards a bit. You can either fix it by redoing the rebase and ensuring you merge withupstream:

$ git switch main$ git pull upstream main  # Explicitly merging with upstream's main.$ git switch fix-development-workflow$ git rebase --interactive main

Or you can rebase against the upstream branch directly:

$ git fetch upstream$ git switch fix-development-workflow  # If necessary.$ git rebase --interactive upstream/main  # Rebase against the upstream main branch.

As part of the rebase, you can changepick toreword to get your editor and change the commit message if you want to do that as well.

@juanis2112juanis2112force-pushed thefix-development-workflow branch from315fbb3 tob488524CompareJuly 24, 2024 23:57
@juanis2112juanis2112 changed the titleAdd tabs for push command depending on git versionAdd branch tracking to development workflow instructionsJul 25, 2024
Delete tab for branch tracking workflowApply suggestions from code reviewCo-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@juanis2112juanis2112force-pushed thefix-development-workflow branch fromb488524 to9961c6fCompareJuly 25, 2024 02:07
@@ -167,6 +161,17 @@ You can achieve this by using
git commit -a --amend --no-edit
git push [your-remote-repo] [your-branch] --force-with-lease

.. tip::
Instead of typying your branch name every time, you can once do::
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggested change
Instead oftypying your branch name every time, you can once do::
Instead oftyping your branch name every time, you can once do::

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

"you can once do" is transposed construction, should be "you can do once" but really something like "instead of typing your branch name every time, you only need to type the following once to link the remote branch to the local branch"

Comment on lines -83 to +88
``--set-upstream`` option::
.. hint::

git push --set-upstream origin my-new-feature

From now on git will know that ``my-new-feature`` is related to the
``my-new-feature`` branch in the GitHub repo.

If you first opened the pull request from your ``main`` branch and then
converted it to a feature branch, you will need to close the original pull
request and open a new pull request from the renamed branch. See
`GitHub: working with branches
<https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-branches#working-with-branches>`_.
If you first opened the pull request from your ``main`` branch and then
converted it to a feature branch, you will need to close the original pull
request and open a new pull request from the renamed branch. See
`GitHub: working with branches
<https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-branches#working-with-branches>`_.
Copy link
Member

@timhoffmtimhoffmJul 29, 2024
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I know you just converted the text to a hint, which is an improvment. But reading this, it feels out of context. We're discussing making a new feature branch. Already having a PR from main means the user has gone two steps in a different direction. Also it's not really relevant to the feature branch.

I'm slightly in favor of removing this completely. Alternatively, if you think, this is still valuable (Do we expect users to find this when they need it? Is a PR from main really that bad that we need to discuss recovering from this non-standard workflow?), you can move this down to the section "Open a pull request".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Is a PR from main really that bad that we need to discuss recovering from this non-standard workflow?

So this language has been in there for a while b/c from what I remember maintainers can't easily take over a PR made to main.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ok let's get this in as an incremental improvement. We can discuss this separately.

story645 reacted with thumbs up emoji
@timhoffmtimhoffm added this to thev3.10.0 milestoneJul 29, 2024
@timhoffmtimhoffm merged commit30f803b intomatplotlib:mainJul 29, 2024
22 checks passed
@timhoffm
Copy link
Member

Thanks@juanis2112!

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell left review comments

@QuLogicQuLogicQuLogic left review comments

@story645story645story645 left review comments

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Labels
Documentation: devdocsfiles in doc/develmentored: sprintIssues intended and suitable for sprints
Projects
None yet
Milestone
v3.10.0
Development

Successfully merging this pull request may close these issues.

7 participants
@juanis2112@timhoffm@story645@rcomer@scottshambaugh@QuLogic@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp