Submitting PRs for packages in the pfSense/FreeBSD-ports repo?
-
Thanks for your tips - very helpful. :)
-
Thanks for your tips - very helpful. :)
Another helpful tip that benefits the pfSense guys during code review is to break your package changes into a series of commits instead of one giant commit. For instance, let's say I'm fixing four bugs in the Snort GUI code and I plan to include them all in the same update. I will fix bug #1, then do a commit so that only the files touched while fixing bug #1 get marked. Next I fix bug #2 and commit again. Rinse and repeat for the other two bugs. At the end I have a chain of four commits for bug fixes. I will then do a final commit that includes only the file changes needed to bump the package version info (typically just one or two edited lines in the Makefile). So the entire package update would consist of five separate commits into the same branch.
Now as folks review the changes, everything is clearly laid out and easy to follow with one commit per change. Take a look at some of the closed pull requests on the Git Hub site for the pfSense/FreeBSD-ports repository for examples.
Bill
-
Another helpful tip that benefits the pfSense guys during code review is to break your package changes into a series of commits instead of one giant commit.
While that is nice for review, it makes it more difficult for us to cherry-pick that change to other branches, such as copying the commit(s) from devel to RELENG_2_4_0, RELENG_2_3, and RELENG_2_3_4.
It's nice to have them all squashed into a single commit before it's merged, but we have some scripts that help us deal with that so it's not the end of the world.
-
Thanks jimp, so - single unified commits are preferred for PRs then?
-
Since we don't merge on github it's a little more complicated, but IIRC you can squash the commits in your PR after review so you can have multiple commits while you work, and then it can be merged as one.
https://github.com/blog/2141-squash-your-commits
http://blog.steveklabnik.com/posts/2012-11-08-how-to-squash-commits-in-a-github-pull-request -
but IIRC you can squash the commits in your PR after review
That's what you would need to activate, none of the people submitting PRs can use that. It can only be used on merge. You have kind of an issue with the workflow you are using.
-
but IIRC you can squash the commits in your PR after review
That's what you would need to activate, none of the people submitting PRs can use that. It can only be used on merge. You have kind of an issue with the workflow you are using.
When I'm working locally, I can squash several commits down to one. I think the same thing can be done in a PR by the submitter, but I'm not 100% on that. I know for certain it can be done before submitting the PR. There are lots of how-tos out there on squashing, such as https://github.com/todotxt/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit – Yes, github can do that automatically when doing a merge in their GUI, but that doesn't help us.
The workflow has to be the way it is because everything has to be sourced from our local gitlab instance. Aside from it being a better flow for us internally, we don't like being stopped dead in our tracks if github goes down, which happens a lot. Some people don't notice how often it happens, but when you're working in git all day, every day, it's more apparent.
There may also be something we can cook up eventually with local scripts to squash but I'm not sure any of us have looked at it.
-
Well yes you can squash things locally… what you linked regarding GitHub squash feature however is unusable for anyone without merge (and apparently for people with merge privs as well due to the workflow).
-
Yeah but the question is if you can squash to your own PR after creating the PR, before it gets merged. I'll have to try to test that out.
-
Another helpful tip that benefits the pfSense guys during code review is to break your package changes into a series of commits instead of one giant commit.
While that is nice for review, it makes it more difficult for us to cherry-pick that change to other branches, such as copying the commit(s) from devel to RELENG_2_4_0, RELENG_2_3, and RELENG_2_3_4.
It's nice to have them all squashed into a single commit before it's merged, but we have some scripts that help us deal with that so it's not the end of the world.
Didn't realize it complicated the cherry picking process. I'm not a GitHub guru, though. Based on the conversations that followed in this thread, it looks like there are pros and cons each way (multiple commits versus single giant commit).
Bill
-
Those giant commits definitely suck when searching for regressions. Plus mostly unusable with GitHub web interface, it just won't show anything lots of times, instead complaining about the query taking too much time etc.
-
Yeah but the question is if you can squash to your own PR after creating the PR, before it gets merged. I'll have to try to test that out.
Yes you can, that's what I do now after initially having a load of commits for typos and additions in my earlier PR's. Now I do a git rebase -i HEAD~x locally, where x is the number of commits to squash and then a git push origin whatever-im-doing –force
That syncs it all up nicely. I'm 99.9% certain the PR still points at the branch if I've issued a PR from, that's what the a quick Google of Git says too. My testers end up with one nice patch ID and you end up with a single commit.
-
…
I ran into the exact same problem with the Windows Git client. It just does not like the FreeBSD-ports repository for some reason (or at least it never worked for me).
...
Bill
It doesn't work because there's files in the repo with names reserved by Windows (specifically 'LPR' IIRC) I think its probably a holdover from the DOS days but maybe they're still used internally in Windows.
-
You can do all of the pfSense stuff itself in Windows Git Desktop, the FreeBSD ports fails due to the Japanese folder which windows does not like due to some of the folder names.
I use a VM FreeBSD with GUI for the FreeBSD ports. Doing the commits on that is a bit more long winded as it all has to be done from a shell.
-
Ideally anything dealing with FreeBSD-ports should happen on a FreeBSD system at some point (either directly or in a secondary capacity) because then you have access to tools like portlint which can check the port structure to ensure it's complete and correct before committing changes, for example it will flag problems with the formatting or content of the Makefile.
