Submitting PRs for packages in the pfSense/FreeBSD-ports repo?

bmeeks

Since Mac is an OpenBSD derivative, I would think it should work but maybe not. If you have access to a Linux machine or a FreeBSD virtual machine, try that using the CLI version of Git. One issue may be the huge number of directories and files in the repository. That's what appeared to be tripping up Windows.

Bill

luckman212

Git didn't throw any errors when cloning (the error happened on GitHub's site actually). But I'll spin up a VM and do some more testing. Thanks

bmeeks

@luckman212:

I'm a relative beginner at git/GitHub and still learning new things every day. So forgive my ignorance on this!

I was trying to submit a small patch for the HAProxy package today and ran into some trouble. I worked around it but am not sure if I've done so correctly. My typical workflow when working on patches to the main pfsense/pfsense repo has been:

fork the main repo on GitHub

clone my fork of the repo

hack hack hack

commit + push

When I tried that on the pfsense/FreeBSD-ports repo, I ended up with a clone that was missing all of the files I needed. Specifically, all of the pfSense-pkg-xxxx directories which contained the files I was trying to update were just not there. What I ended up doing instead was cloning the original repo, hacking on that, and then pushing the commits, like this:
git clone git@github.com:pfsense/FreeBSD-ports.git my-foobar-patch
cd my-foobar-patch
git remote set-url origin git@github.com:luckman212/FreeBSD-ports.git
git checkout -b my-foobar-patch

~ HACK HACK HACK ~

git ls-files --modified | xargs git add
git commit
git push origin my-foobar-patch
That worked. So, questions are:

why did that happen?

have I done this the "right" way?

Thanks all

As for question #2 –

What you have is fine with a couple of tweaks as follows. First, I usually just use "git commit -a" instead of searching for modified files manually. Git knows what files have been touched and the "-a" switch will automatically include them in the commit.

The bigger tweak you need is to create a new branch each time you start a patch stream. For example, when I am getting ready to create an update for the Snort package I do the following --


git pull upstream devel
git checkout -b pfSense-pkg-snort-3.2.9.5_2

This first pulls any updates from the main FreeBSD-ports repository into my local copy. The next step creates a new branch in my local copy to match the package name and package version I will be submitting changes against.

When I'm ready to commit my updates I do this:


git commit -a
git push origin pfSense-pkg-snort-3.2.9.5_2

These two lines will commit all my modified files and then push them as a new branch to my Git hub fork of the FreeBSD-ports repository. I then go to the Git Hub web site and my new branch will show up in remote repository along with a button to create a Pull Request against the upstream FreeBSD-ports repository of pfSense.

The above examples assume pfSense-pkg-snort-3.2.9.5_1 is the current devel version and pfSense-pkg-snort-3.2.9.5_2 is my updated package to be submitted as a Pull Request.

Bill

luckman212

Thanks for your tips - very helpful. :)

bmeeks

@luckman212:

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

jimp

@bmeeks:

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.

luckman212

Thanks jimp, so - single unified commits are preferred for PRs then?

jimp

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

doktornotor

@jimp:

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.

jimp

@doktornotor:

@jimp:

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.

doktornotor

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).

jimp

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.

bmeeks

@jimp:

@bmeeks:

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

doktornotor

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.

@jimp:

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.

loonylion

@bmeeks:

…

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.

jimp

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.