Homebrew / homebrew-cask

🍻 A CLI workflow for the administration of macOS applications distributed as binaries
https://brew.sh
BSD 2-Clause "Simplified" License
20.95k stars 10.73k forks source link

Improve git merge practices #6958

Closed federicobond closed 9 years ago

federicobond commented 10 years ago

When comparing the commit history of homebrew-cask with that of the homebrew project, one thing that stands out a lot is the sheer amount of git merge bubbles we are generating due to our current practices.

I understand that it is easy to be able to merge commits right from the web interface, but there are several disadvantages with this method that others have carefully pointed out before. In our case, it also pollutes the history with lots of unnecessary cruft, wasting space in every user's computer, and makes it harder to find particular changes by obscuring genuine commits.

I would like to hear your opinions and see if there is a better way we can handle the merge process.

cc/ @caskroom/maintainers

vitorgalvao commented 10 years ago

Agreed. This is ideally something that could be done by Github, but lacking that, do you know how they do it in homebrew?

Partly inspired by the post you link to, I’ve made a small script to help with making corrections to PRs. It doesn’t require an external tool (hub), which is to me a plus. The end result is still a PR, though, but that could easily (the script would be smaller, even) be changed.

If we go the script route (which is not great, but works), it’d be trivial to build an Alfred workflow (for those of us that use it) to perform the relevant actions.

rolandwalker commented 10 years ago

We have discussed this on IRC before. It is certainly true that the merge history is complex, but it is also true that the PR system increases productivity (I frequently process Cask submissions when on the train).

Momentum is supremely important.

When a submission needs more work (as in https://github.com/caskroom/homebrew-cask/pull/6891#issuecomment-60078883 or https://github.com/caskroom/homebrew-cask/pull/6886#issuecomment-60077996) I do drop to the CLI and use a workflow similar to what the Homebrew project does — omitting the merge commit. Since we are sharing scripts, my method uses some aliases defined in ~/.gitconfig.

However, that approach only results in a tidier git history if all collaborators maintain CLI discipline and forgo the web interface for all merges. I'm not willing to do that myself, much less require everyone else to do the same.

In addition, GitHub is adding convenient features to PRs. I also use the merge commits to generate the changelog for releases. So it's not as if PRs and merge commits are useless or contain zero information, there are just so many of them.

Since the complex merge history is only a distraction to developers on the backend code, and since the source of the complexity is Cask PRs, a simpler solution would be to separate the codebases.

After this discussion re: mackup interop, I took a closer look at submodules. Contrary to what I thought at that time, Homebrew should support submodules in our Formula. So the backend could be migrated to a separate repo without affecting end-users.

If doing so increases contributions on the backend, I am all for it.

federicobond commented 10 years ago

Thanks for the answers. It is nice to have the conversation here so that we can revisit the arguments at any time. I agree that momentum is important, and lacking support from GitHub for any other kind of workflow, there is not much we can do right now without complicating things for every other maintainer.

For the moment, I don't think that migrating the backend to a different repo would bring much benefit.

rolandwalker commented 10 years ago

Hopefully these are not so much answers as a list of plusses and minuses. The log topology does annoy me, on a daily basis.

I'll keep an open mind about it, especially if the prevailing habits of the most productive maintainers were to change.

tapeinosyne commented 10 years ago

This repository has much churn: small, trivial patches in high numbers. The web interface works well because there is almost no need for review; each step beyond the 1-click merge feels all the more burdensome as it is made in service of something so banal.

The commit history generated by this process is indeed very polluted, and might be sufficient reason to merge PRs manually. (Increased repository size does seem a bit inconsiderate on our part, but is likely not problematic on modern macs.)

If other maintainers feel so inclined, I am amenable to new practices. An alternative would be to write (or fork) a merge bot.

petere commented 10 years ago

As an interested outsider, I don't think this is a real problem. The commit history looks perfectly sane when viewed by git log --no-merges, which is the only sane way to look at any git commit history. The merge commits are an artifact of how git works and why git is useful, but you don't have to look at them.

As far as disk space is concerned, merge commits are at worst a constant-factor overhead on the order of tens of bytes. Repacking will reduce that even further. Total disk space here is about 24M. If you are concerned about disk space for ordinary end users, provide a shallow clone option (or Homebrew proper should provide one).

L2G commented 9 years ago

@petere :+1:

vitorgalvao commented 9 years ago

Closing.

I have been following (and prefer) the “cleaner history” approach, as have @Ngrd and @jawshooah. However, I’m not opposed to merging via the web interface, and may do so myself occasionally (if on a phone, for example). Ideally github would let us have the best of both worlds, but that is currently not the case.

I also have a few custom-built scripts I use to make the work easier, depending on if the PR can be merged as is, or needs fixes and everyone is absolutely free to use and improve on them, if they wish.

In the (no-so-near) future it may be wise to have an agreement so we’re all consistent, but for now its important everyone does what they find works best personally, to get things moving.