search

pytest-dev / pytest

The pytest framework makes it easy to write small tests, yet scales to support complex functional testing
https://pytest.org
MIT License
12.06k stars 2.67k forks source link

GitHub: enable squash merging / better Git history #4361

Closed blueyed closed 4 years ago

blueyed commented 5 years ago

I would really like to enable squash-merging via the GitHub UI.

  1. The main benefit here would be keeping the Git history cleaner, especially since it allows for editing commit message while squashing. This would allow to have something more meaningful instead of the following easily:

    M─┼─┴─┘ Merge pull request #4307 from fzarifian/fzarifian-pr4304
│ o Update AUTHORS
│ o Fix linting
│ o Improve changelog a bit
│ o Update __init__.py
│ o Create 4304.bugfix.rst
│ o Update __init__.py
│ o Add test to branch
│ o Update __init__.py

  2. It makes git-bisect behave better (less commits in general, no commits that need to be skipped because of failed tests (e.g. if https://github.com/pytest-dev/pytest/pull/4299 gets merged as-is)

While you can rebase/squash things yourself and force-push, it will a) trigger an extra CI build and b) it cannot be done for other contributors PRs easily.

RonnyPfannschmidt commented 5 years ago

iim strictly against committing modified history - for one it makes local work flows crappier, and git is not good enough to tell the commit-er that the actual commit was merged

i'd be all for this if git wasn't this barren and incapable ^^

blueyed commented 5 years ago

Yes, it's true that git branch --merged will not help here.

OTOH

  1. git fetch will tell you if you deleted the branch yourself via the GitHub UI (the remote branch is deleted)
  2. I think it is acceptable in general, since normally people are not juggling with dozens of active branches/PRs here, and can delete it manually after it was merged
  3. squashing does not have to be enforced/used always/by default of course - it is totally OK to not do it by default, and e.g. if you would not like to do it in general

I would just like to use it for myself for example, and in cases like cited above: I think it is more important to keep the project's history in good shape.

blueyed commented 5 years ago

Would you rather like to rebase/squash the commit yourself, force-push it, wait for it to get merged, and then delete it more confidently?

My main argument here is that commit history should become better, and squash-merging is a shortcut here.

I think most contributors would rather have it squash-merged than being asked to force-push again, no?

(just came back to this after some intermediate "linting" commit when git-blaming, so it's not just git-bisect (of course))

RonnyPfannschmidt commented 5 years ago

i would much rather like if git had grouping merge commits that would collapse all the elements of the second parents into a single displayed commit ^^

unfortunately thats not existing yet

asottile commented 5 years ago

Please no. The ability to use git branch --merged is very important to me.

@blueyed if you need a linear history, use git log --first-parent

blueyed commented 5 years ago

@asottile It is not about linear history, but rather reducing the number of commits in total, and having a good log messages for those, and the squash-merge-UI is a great fit here: it allows the one that merges it to adjust the message for anything that has changed while the PR was developed/reviewed, too.

The ability to use git branch --merged is very important to me.

I get this. But is it that hard to manually delete a branch nonetheless?

For bulk-cleanup I've created a (Zsh) function that compares branches and will find squash-merged/cherry-picked stuff: https://github.com/blueyed/oh-my-zsh/blob/b2d46dbcf1a4e7b012dbc5d38fe7e2ea53be0839/plugins/git/git.plugin.zsh#L234

git log --first-parent

This just results in something like this:

o {origin/features} Merge pull request #4360 from blueyed/merge-master
o [features] Merge pull request #4271 from blueyed/pytest_cache
o Merge pull request #4336 from blueyed/cwd2
o Merge pull request #4338 from blueyed/merge-master
o Merge pull request #4317 from blueyed/cwd
o Merge pull request #4314 from RonnyPfannschmidt/bestrelpath-cache-move-to-session
o Merge pull request #4301 from nicoddemus/merge-master-into-features
o Merge pull request #4237 from boxed/master
o Merge pull request #4272 from blueyed/cache-non-default
o Merge pull request #4247 from blueyed/lru
o Merge pull request #4277 from blueyed/pdb-set_trace-capture-msg

Still I am not requiring to use it in general, but would only like to use it for my own PRs in the first place, and then would like to enforce something like a "every commit should pass tests by itself, and commits should be meaningful by themselves". If that means to rebase/force-push things yourself - fine.

Another benefit of squash-merging from GitHub is btw that you still have the history there (e.g. failing builds, adjustments), and then the squash-merged commit in master (also expected to be green).

asottile commented 5 years ago

It is not about linear history, but rather reducing the number of commits in total

why do you care about this?

and having a good log messages for those

I'd rather see what actually happened.

and the squash-merge-UI is a great fit here: it allows the one that merges it to adjust the message for anything that has changed while the PR was developed/reviewed, too

You can already adjust the merge message, how is that different?

But is it that hard to manually delete a branch nonetheless?

It's not possible to know whether your exact change was applied when using squash merge. There is no "safe" deletion of branches, you must force delete when the histories are different. Your 150 line function for handling squash merged is a 3 line bash pipeline for merges. (!!!)

Still I am not requiring to use it in general, but would only like to use it for my own PRs in the first place

If you're worried about your history, just rebase on your feature branch and merge your one tidy commit. This keeps the workflow the same for everyone (and the revert workflow the same for everyone!). It's especially common (at least in this project) for each of us to merge others' branches -- once you're squash-merging your own branches github will default that workflow for everyone elses'. You're going to end up accidentally squash merging something unintentionally :/.

Another benefit of squash-merging from GitHub is btw that you still have the history there (e.g. failing builds, adjustments),

Don't you lose the history because you're deleting commits?

blueyed commented 5 years ago

[number of commits]

why do you care about this?

git-bisect and git-blame.

With git-bisect it takes longer in general, and git-blame needs more steps (to skip over linting etc). Additionally, git-bisect might fail (needs to be skipped) due to some intermediate commit introducing a failure that was fixed later in the same PR (and should have been squashed).

and having a good log messages for those

I'd rather see what actually happened.

Sure, it is still valid to have multiple commits in a PR that are being kept, but there is no reason for unnecessary "Update $file" commits from when someone uses the GitHub UI to edit PR files.

Also it is not worth having a "whoops, I've meant to use not" as an extra commit, is it?

and the squash-merge-UI is a great fit here: it allows the one that merges it to adjust the message for anything that has changed while the PR was developed/reviewed, too

You can already adjust the merge message, how is that different?

Adjusting the merge message is a good point, and we/I should use this more probably.

But is it that hard to manually delete a branch nonetheless?

It's not possible to know whether your exact change was applied when using squash merge. There is no "safe" deletion of branches, you must force delete when the histories are different.

Yes. But you usually know a) that you had pushed your local branch, and b) that it has been merged now, so you can git branch -D it.

Still I am not requiring to use it in general, but would only like to use it for my own PRs in the first place

If you're worried about your history, just rebase on your feature branch and merge your one tidy commit.

I usually do this, but rather would like the UI for it directly. And it will still create a merge commit for this single commit then.

Also force-pushing will a) trigger a new CI build (but you can merge nonetheless), and b) remove the existing history on the GitHub remote (because you've force-pushed).

When squash-merging from the GitHub UI the history is kept on the PR page. The history is only "lost" on the branch where it was squash-merged into.

This keeps the workflow the same for everyone (and the revert workflow the same for everyone!).

What is the "revert workflow"? (I think GitHub UI allows to revert in either case, and it typically easier to revert a single commit than a merge.)

It's especially common (at least in this project) for each of us to merge others' branches -- once you're squash-merging your own branches github will default that workflow for everyone elses'. You're going to end up accidentally squash merging something unintentionally :/.

Well, I pay attention to the state of the button in general.

Another benefit of squash-merging from GitHub is btw that you still have the history there (e.g. failing builds, adjustments),

Don't you lose the history because you're deleting commits?

The commits in the PR (remote branch, which is "archived") are not deleted, but only if you force-push to it because you're squashing manually.

I usually like to just keep on pushing "fixup!" and "squash!" commits, and only if it should be kept in separate commits would git rebase -i and force-push it. If it is only a few fixups that would be squashed anyway, I then just squash-merge it from GitHub's UI. This keeps track of the history (build failures, changes, discussion) in the PR for reference, but only has the result then in master.

asottile commented 5 years ago

I'm not clever enough to reply and maintain the quotes, so you'll have to make do with a new post 😭

at least with git blame there's a --first-parent option. It's fairly easy to tell git bisect to skip any other non-merge commit by scripting | xargs git bisect skip

I absolutely think it's important to keep all the history, even the small fixups. If someone cares about keeping their branch tidy, then by all means let them do that.

It's insufficient to know that I pushed a branch, especially for me -- I frequently use four different computers and without being able to query --merged I'm dead in the water guessing whether the history was integrated (work forces me to rebase-squash and the number of times I've lost code due to "oh I definitely pushed this branch already" in the last year is 10+)

reverting a merge is git revert -m1 {sha}, if you have a mix of merge / squash commits then you have two ways to revert (with or without -m1)

and lastly, I don't think relying on the github ui to maintain history is a good idea -- if we were to switch to another service all that information would be lost (but would be kept if we didn't delete commits with rebase-squash)

nicoddemus commented 5 years ago

FWIW, I rebase my own branches (and only my own branches) all the time, removing/squashing commits, and pushing-force back to GitHub.

IIUC, all @blueyed is asking is to enable the option in the GitHub UI that allows merges to be squashed so he can use in his own branches, not to enforce us to use on all branches. As has been pointed out, he is already allowed to do so manually (and does), so why not make his life easier and allow him to do it on his own branches directly on the GitHub UI?

I get @asottile's point about tracking what has been merged or not, but I understand @asottile uses this for his own branches, no? If that's the case, that workflow won't be affected.

We might be derailing the discussion to personal preferences regarding merging strategies, but I think the original issue is just to enable someone to do squash-merges in the UI when merging their own branches, which they already can and do themselves manually anyway.

asottile commented 5 years ago

It's still a problem there too, there isn't a "squash and then merge" option (which I'd be totally fine with -- not on my branches) in the UI so the overall history will be a mix of merges and squashed commits. This is worse than all-merges or all-squashes as you have to then determine "ok how was this merged" before being able to take an action (such as revert / diff / etc.) against that commit.

nicoddemus commented 5 years ago

Oh I see, squash in the UI is not the same operation as squash-force-push-then-merge, is that it?

asottile commented 5 years ago

Oh I see, squash in the UI is not the same operation as squash-force-push-then-merge, is that it?

Correct, it's essentially "add one commit on master with the squashed contents of the branch"

nicoddemus commented 5 years ago

OK, thanks for the clarification.

I'm neutral on this then, as I understand it would make @blueyed life's easier, it would make @asottile's life a little harder too.

I myself prefer "clean but inaccurate history" vs "accurate but dirty history", but I'm just saying that because that everyone stated their preferences here, not to add to that discussion. 😁

RonnyPfannschmidt commented 5 years ago

i want to be very clear here - for me the only person that should be allowed to edit the history of a pull request is the person making it - anything else completely unacceptable in my book

im entirely ok with having a process and/or expectations for cleaner history, but i am absolutely opposed to enforcing edited history on contributions of anyone

nicoddemus commented 5 years ago

i want to be very clear here - for me the only person that should be allowed to edit the history of a pull request is the person making it - anything else completely unacceptable in my book

I agree. 👍

blueyed commented 5 years ago

Yes, @nicoddemus it is basically for me, and/or when asking contributors if it is OK to squash their commits, or if they want to do it themselves (as I would have asked for in the first example).

@asottile I am not sure why having non-merge commits in the history is confusing for you? Because of having to use -m or not? If it's only that, I would argue that reverting a whole merge via CLI is not very common here, is it?

at least with git blame there's a --first-parent option. It's fairly easy to tell git bisect to skip any other non-merge commit by scripting | xargs git bisect skip

I do not want to handle only merges with blame/bisect, but find the bad commit - just having a bunch of extra commits that should have been squashed makes bisecting take longer, and blaming a bit more annoying (since you have to go unnecessarily to parents again).

i want to be very clear here - for me the only person that should be allowed to edit the history of a pull request is the person making it - anything else completely unacceptable in my book

Yes, but you could also allow others to do it, i.e. state that it is OK to squash-merge, if you do not want to do it yourself.

I can see that I am a bit pedantic here, but I really do not like "bloated" history like in the first example, where none of the commits says what it is really doing.

Off-topic: @asottile

I'm not clever enough to reply and maintain the quotes, so you'll have to make do with a new post 😭

You can "edit" a comment, and then copy from there.. ;) You just have to add a level of quoting then (I am using qutebrowser with Vim as external editor, which makes this easy).

asottile commented 5 years ago

@asottile I am not sure why having non-merge commits in the history is confusing for you? Because of having to use -m or not? If it's only that, I would argue that reverting a whole merge via CLI is not very common here, is it?

I'm essentially advocating these things:

And from that it follows: since a mixed history is problematic and we shouldn't force rebase-squash on people, we probably shouldn't enable rebase-squash at all.

blueyed commented 5 years ago

@asottile

  1. I am not sure how relevant this is for "scripting" really - what would require you to know the used "workflow"?

bisect will be not-any-longer by only considering mainline commits, and blame will point you exactly at the PR that introduced the change using --first-parent

git blame --first-parent is something I might try. Not sure about your git-bisect comment? Do you mean that it prefers mainline commits? If so, yeah, but the more commits the longer it takes afterwards (i.e. given 7 commits (that should be one really) instead of 1 for a PR).

Forcing (or even suggesting to) contributors to squash is unacceptable

That's why we could just do it ourselves for them (if they are OK with it).

Anyway, I am resting my case.. :)

blueyed commented 5 years ago

@asottile

Please no. The ability to use git branch --merged is very important to me.

Like said before there is tooling for when you need this often, but in general (for the occasional PR) it is fine to use git branch -D afterwards.

But especially given your number of repos etc you should rather not require merge commits for gb --merged to work, when you should have tools to deleted merged branches anyway.

You could also delete local branches after the remote one has been deleted, which you usually do after merging a PR.

@blueyed if you need a linear history, use git log --first-parent

This does not really work unfortunaltely easily. I tend to use an alias for tig HEAD@{1}..HEAD@{0}, which ends up in something similar to what git log --oneline 0822a1e53..0f11a7a73 provides now for me:

0f11a7a73 (origin/features) Merge master into features (#5744)
1049a38ce Fix wording as suggested in review of #5741
d7f082519 Merge remote-tracking branch 'upstream/master' into mm
2d613a03b Async result warn (#5742)
28c6c5bb7 check that tests that are partial staticmethods are supported (#5701)
6b9d729ed also warn on awaitable or async iterable test results
0ba774a7c warn for async generator functions (#5734)
0a62c4ac0 Merge pull request #5729 from Stranger6667/issue-5115
137255816 Fix collection of staticmethods defined with functools.partial
2f1b192fe Issue a warning for async gen functions
7183335e6 Capture warnings during ``pytest_configure``
a295a3dda test_non_ascii_paste_text: mock call to urlopen (#5728)
3b3ce0e79 remove %s formatting from docs (#5733)
3eb497306 remove %s formatting from docs
0db9dade6 (blueyed/test_non_ascii_paste_text, test_non_ascii_paste_text) test_non_ascii_paste_text: mock call to urlopen
a77c83a4c Merge pull request #5726 from jdufresne/black
0767f080a Update URL: python/black → psf/black
f7e81dab9 (Stranger6667/master) 5669: pytester: add docstrings for Testdir.copy_example (#5719)
ee936b27a pytester: fix docstrings
e0ce8b79d pytester: add docstrings for Testdir.copy_example
8ffa3aa65 Improve docs of pytest.importorskip (#5718)
b095e0de4 Improve docs of pytest.importorskip

With --first-parent it is just:

0f11a7a73 (origin/features) Merge master into features (#5744)

git log --merges:

0f11a7a73 (origin/features) Merge master into features (#5744)
d7f082519 Merge remote-tracking branch 'upstream/master' into mm
2d613a03b Async result warn (#5742)
28c6c5bb7 check that tests that are partial staticmethods are supported (#5701)
0ba774a7c warn for async generator functions (#5734)
0a62c4ac0 Merge pull request #5729 from Stranger6667/issue-5115
a295a3dda test_non_ascii_paste_text: mock call to urlopen (#5728)
3b3ce0e79 remove %s formatting from docs (#5733)
a77c83a4c Merge pull request #5726 from jdufresne/black
f7e81dab9 (Stranger6667/master) 5669: pytester: add docstrings for Testdir.copy_example (#5719)
8ffa3aa65 Improve docs of pytest.importorskip (#5718)

git log --no-merges is actually useful in this case:

1049a38ce Fix wording as suggested in review of #5741
6b9d729ed also warn on awaitable or async iterable test results
137255816 Fix collection of staticmethods defined with functools.partial
2f1b192fe Issue a warning for async gen functions
7183335e6 Capture warnings during ``pytest_configure``
3eb497306 remove %s formatting from docs
0db9dade6 (blueyed/test_non_ascii_paste_text, test_non_ascii_paste_text) test_non_ascii_paste_text: mock call to urlopen
0767f080a Update URL: python/black → psf/black
ee936b27a pytester: fix docstrings
e0ce8b79d pytester: add docstrings for Testdir.copy_example
b095e0de4 Improve docs of pytest.importorskip

Still it feels to me like extra tooling is used for people to see a clean history (which still means that there is an unclean one).

blueyed commented 5 years ago

As for "better history": I think it is good to see WIP / tries on the PR, but have the clean result merged then. Given that squash-merging is not enabled it means to either have all the WIP things (like reverting a previously commit all by itself) in the main branch (forever!), or remove the history of it in GitHub then by first force-pushing before merging (creating another CI build for basically unchanged files).

GitHub by now keeps track of forced pushes, but that is merely a note, and it is still better to see all the WIP commits that lead to the final result.

I am not saying that everything should be squashed, but it certainly makes sense for things that should be (i.e. fixup commits etc), and also for single-commit PRs.

RonnyPfannschmidt commented 4 years ago

i'm not willing to engage on the basis of this framing which i perceive as pretty passive-aggressive and needlessly annoying - feel free to rephrase the core of your message in a way that has communicative success as key target and i'm happy to engage about limited positive use of the feature

blueyed commented 4 years ago

@RonnyPfannschmidt I've deleted my ranting comment - happy to hear from you with regard to my earlier comments.

RonnyPfannschmidt commented 4 years ago

I'm not going to forgive and forget, for me this is part of the implicit negotiation of what kinds of behaviors are acceptable

It's time this happens explicitly because the actual pattern is a problem and I expect that we engage to solve that problem

As far as I'm concerned, my good will is exhausted and it's time for a number of uncomfortable non-technical negotiations about behavior

blueyed commented 4 years ago

@RonnyPfannschmidt I am very sorry for having exhausted your good will and coming across as passive-aggressive - this was not my intention, and came out of me being frustrated and annoyed so much in the first place myself. But that does not excuse my behavior, and I should not have let that come into that comment! I especially apologize for wrongly stating that you would be blocking this, since after re-reading all comments it was clear that you were not opposed to it in general in the end.

I also appologize to all others possibly being rightfully offended directly (@asottile), or indirectly affected by this (@nicoddemus, and others reading this).

As for speaking about behaviors and reactions in this regard I will follow-up privately via email.

So, again, I'm very sorry for what I've done and caused here, and I hope you will accept my apologies.

I will do what I've just should have done instead in the first place: pinging @asottile (so that we can have this sub-thread then being hidden as off-topic hopefully).

blueyed commented 4 years ago

@asottile Are you still opposed to enabling the squash-merge button in the first place, to be used for myself, and helping contributors that agree for it to be used?

Can you answer my remaining/previous comments otherwise (but also in general maybe), please? The main (only?) reason seems to be that you "can only script for a particular workflow, not a "mixed one" (easily?)" (from what I've understood), and that it would be confusing to have merge commits and (basically) fast-forward merges being mixed in the history.

asottile commented 4 years ago

yes, my original comment is unchanged

RonnyPfannschmidt commented 4 years ago

Thanks for the reply and I'm eager to sort those details out in a positive manner

As for the issue at hand, I am still opposed to general squashing, But I am open to consent based squash merging in the cases where it helps the contributor

That way there is control in place so contributors like @asottile and me get the control over the history as we want while we still have a reasonably simple process for helping people less fluent with git

asottile commented 4 years ago

I'd still rather see merge commits on the main branch -- at least with consent we can push a squash to the contributor's branch (assuming they left the checkbox checked when creating a PR that allows maintainers to push to their branch)

RonnyPfannschmidt commented 4 years ago

@asottile how strong do you feel about others using a squash based work-flow (for both new contributors or own work-flows)

personally i don't mind, as the content delivered is roughly the same, i only want the history control for myself for my own utility

asottile commented 4 years ago

I'm fine with users personally using squash (I do myself) -- I don't want squash as a "merge strategy" as stated above

nicoddemus commented 4 years ago

I personally also don't mind enabling squash merging

blueyed commented 4 years ago

@nicoddemus @RonnyPfannschmidt Gentle ping.

(2 months have passed. Please either enable it finally, or close the issue)

RonnyPfannschmidt commented 4 years ago

enabled