Best way to submit patches

I have reworked the utils/alive.py (branches: master and maintenance/gramps52), fixing several bugs and, I think, improved the algorithm to give more reliable estimates in some cases.
I cloned the repo locally and made a new branch off the maintenance52 line with about 20 commits.
I have:

  • tiny change to lib/eventtype.py: added probate as a death-fallback
  • small changes to lib/date.py - one bug, one improvement and documentation
  • utils/alive.py: bug fix, refactoring (duplicate code into functions), improving algorithm, until the patch file is bigger than the source.

I’ve done the black and pylint requirements.

I have now read the stuff about pull requests (not having done one like this before) and it seems I can’t get there from here. As far as I can see it needs to be a forked repo on Github.
As you could tell, my git usage has been only for simple stuff.

Is there a way I can realign my local copy with a github fork while retaining my branch and commits?, or do I need to request push access to Gramps?

Would you like separate bug reports for everything I have “fixed”?

If so, and I reference my commit hash, do the hashes remain constant across repos?

Hi @CameronD73

Thanks for taking the time to jump into Gramps code make improvements. A couple of general thoughts to help get started:

  1. File bugs reports for issues that you are fixing. This makes it easy for triagers to confirm bugs, and test your fix. Link to the bug in your PR.
  2. For new feature work, file a feature requests. It aids in determining whether the changes can go in a minor release, or may need to wait for a major release.

Nice to see you’ve used pylint and black already, which makes it more likely that your code changes won’t be rejected by the automatic checks. Consider adding unit tests. About Git and GitHub

  1. Forking adds a bit more work but isn’t too complicated, it’s just a way to get your changes into a repo where you don’t have write access. Instead of describing the process here, refer to the ProGit book which covers it in the Distributed Git chapter, under Contributing to a Project > Forked Public Project section. It will answer your question about how to get your changes into your fork and ready for a Pull Request.
  2. Open one PR for each independent issue. This makes changes easy to test, and PRs easy to accept/reject, and again, decide which release they go into.

If you haven’t already, I’d recommend reading through Committing policies and Developer Policies.

Hope this helps.

1 Like

The other benefit of having an issue in MantisBT bug tracking system is: it includes the change in the Roadmap report used for the next version’s Release Note.

1 Like

Yes. Bug fixes should be submitted against the current maintenance branch. They should have a corresponding bug report in our bug tracking system. Keep the changes as concise as possible.

Enhancements, refactoring and code quality improvements should be submitted against the master branch. Keep the code quality changes in separate commits to the enhancements so that we can see what has changes more easily.

Extra unit tests are always welcome.

1 Like

Thanks both of you.
I was worried that was going to be the answer.
Many of the bug fixes are quite trivial, but I think a few are tied in with the restructure - certainly some only came to light towards the end. Sticking some in maintenance and some in master might be a bit beyond me.

It may sound overwhelming but you’ve already done the hard part (fixing the bugs!). Happy to walk you through this. Pick one of the issues you want to start with and we’ll go over it one step at a time. Feel free to DM me.

1 Like

thanks, I’ll start with some easy ones. Then see how it goes.

3 Likes

It looks like I am not allowed to DM anyone.
So I now have a local repo with a commit, which failed the unittests when I pushed it to my GitHub fork
I can edit the unit-test code as well, but somehow I have to back out of the commit, presumably so that I can submit both changes at once.
Do I just do a hard reset on the local repo? Or a revert?

What kind of Private Messaging problem are you experiencing? The account on this forum was less than 30 days old and had restrictions. A moderator saw the complaint and manually boosted the account above “New User” status.

(Although publicly discussing hitches in the workflow can spotlight things to improve. Or at least, help others find a path through. The GitHub workflow feels convoluted to me.)

@CameronD73 Let me see if I understand correctly - you pushed a commit (was it in a branch, or your master?) to your fork of Gramps, and you now want to add more changes to fix UT failures.

With git there are many ways to accomplish the same thing, some easier than others.

If you really wanted to, you could use reset, make the fixes and create one new commit. But you don’t necessarily need to back out your original commit - instead, fix the code and push another commit. Now you have two commits to fully fix the issue. This can be merged to one commit later in two ways (1) Once you create the Pull Request, ask the person who merges your Pull Request to Gramps to “squash and merge” your two commits into one, which is easy to do and no additional work for you, or (2) you could use git rebase -i locally to do the squashing yourself and force push to your branch. At this point it’s easiest to let the merger deal with it. Just continue to add commits to your branch.

If I misunderstood, feel free to share your repo/branch/PR someone can jump in and help. There are many experienced developers here.

Keep code quality and refactoring in separate commits. This helps us see what changes were made to implement your enhancement or bug fix.

1 Like

OK, it turns out my github repo was a copy and not a fork, so I ended up wiping the GitHub content and starting again. Now one PR has gone through (without too many mistakes)

However, I am still confused and looking for a bit of guidance.

I created a branch, based on maintenance/gramps52, named for the bugID, committed to that and issued the PR. So far, so good.

For the next trivial bug I merged the original bug branch back into maintenance/gramps52, created a new branch, committed changes against that bug id and issued another PR.
It looks like my PR includes not just the branch, but also the merges to maint…52 - which is probably not what is wanted.

Should I be basing my changes on a single branch, and gradually issuing PR’s against commits on that?
Or what?

Yes! Since you’re working on an independent bug, you would start by creating a branch from maintenance/gramps52 just as you did for the first bug fix. This allows each PR to be merged in independently. So start by creating a new branch based on gramps52 and work on the second bug fix.

Only if the first bug fix is needed for the second fix and they aren’t truly independent issues, then they need to be in the same PR.

Another helpful wiki: Brief Introduction to Git - Gramps. It covers a lot of topics, just pick the topics that are relevant to you at this point.

Thanks. Hmm, that ship has sailed.

I started with bug 1, call it that for the sake of brevity (which I have not yet logged in Mantis).
I then found my fix did not work, because of bug 2. The commit of the fix for bug 2 failed because the unit test needed changing (bug 3).

So I have already done the PR for bugs 2 and 3 combined.

But the fix for bug 1 does not work without bug 2 having been fixed, so I presume I should add that to my branch currently named “bugs_2and3”. But then I guess it will include the earlier PR content again.

I have read most of the Gramps wiki - the content is not quite detailed enough to cover the holes I have dug myself into.

You’ve done the right thing combining bugs 2 and 3 - they aren’t independent issues. You can upload the PR and request that it be merged in. This would be one way forward because then you would pull in the merged code, and continue to work on bug 1 with the dependency.

If on the other hand you have to work on bug 1 before bugs_2and3 is merged (more likely scenario) then one way I can think of is to -

  1. Create a local branch for bug 1 from your bugs_2and3 branch, make the fix for bug 1 and verify the complete fix.
  2. Next, make a new branch for bug 1 from gramps52 and copy only the changed file(s) from the branch in step 1 to this new branch, commit (review carefully!) and create a PR. This PR should not have the files from bugs_2and3. Limitation is that this would work only if the files for bug 1 are different from bug_2and3. So, YMMV.
  3. Now push up the PR for bug 1 branch from step 2, and make a note that the PR for bugs_2and3 must be merged before it.

I’m sure there are other ways…git gives you tremendous flexibility. Hope to see your first contribution soon!

Thanks - that path was the conclusion I was coming to.

Sure does - too much for my old brain to absorb.

First two PRs are in the system.