I was thinking about how to organise commits for pushing to the ROOL gitlab
and had a few questions.

Let’s say I’m working on !Draw, so I’ve done my day-to-day work on branch devel.
Then I’m ready to make a merge request so I do

git branch DrawMenu
git reset --mixed

and then use

git add -p
git commit

etc. to write some commits breaking down the changes. But say I then want to do
more work on this feature, should I carry on working on devel, or on top
of DrawMenu instead?

Once I’ve finished my further work, I need to add the changes to the commits
in DrawMenu, but I don’t really want to have to do the work with add/commit
all over again. So do you have some tips on how to fold new changes into
commits in the middle of a merge request?

It depends on what you’re doing – and remember that none of the git workflow is specific to RISC OS development, so any other advice you find on the Internet will be as pertinent.

Let me open by saying that how you initially suggest working isn’t how I work – though it is entirely valid, and it comes down to what your working style is. My working style has been brought through from the style that I used on the original RISC OS sources, and then working at a number of companies and on my own things – there may be a particular way that ROOL likes things managed, but this is how I do things…

How I start out…

I would usually start my work by creating the branch for the specific thing I’m trying to address:

git checkout -b fix-ifc-error-on-load

That lets me start out the work on the branch and ensures that if I make a mistake and commit without branching, I don’t have to juggle around to move my commit from master to a branch (which is all very simple but it’s an extra faff). This sets me up for working and I can do whatever I like from there.

Then I make some changes, to do what I need.

In my example I’m addressing a change that fixes the loading of files with IFC in !Draw. I do some investigation and add extra debug code to identify the problem, build, test, rinse and repeat. At any time I might decide that the code that I’ve added is good. So I might commit the files that I’ve changed.

git commit c.DrawFileIO

And I give a git log message – with a summary, and some details – the details should always contain a brief outline of what you’re trying to address and what you’ve done. The important thing here is what you’re trying to achieve and what you did – if you don’t say what you’re trying to do, then anyone reviewing the change (either on submission or in the future) won’t know /why/ you were doing it.

Add diagnostics to investigate load problems with IFC.

Loading files through IFC is sometimes crashing Draw with data aborts.
It's not clear why that is, so extra debug has been added to give more
information in the file conversion code.

If I add more diagnostic code, I can update that change with the new details with an amend

git commit -a --amend

The amend updates the existing change. This is safe because nobody else has seen it – it only lives on my machine, and I’m just adding more useful content which still does what the description said. If the changes I was adding were unrelated, then I might create a new commit instead of amending. OR I might do the amend but update the commit description (or summary) to reflect what I’m doing instead.

Actual fixes

Finally I find the problem, and I have a choice. I can either amend to the existing change and add in the fix to it – updating the description to say that I’m now fixing the problem as well, or I create a new commit that just contains the fixed code.

Why would I amend to the existing change? Because then it is easier to see in one go what I’ve done and how I fixed it.

Why would I create a new change? Because then the fix itself could be isolated out if it wasn’t the actual issue.

Which you might do is kinda up to you and how you feel about the changes.

VersionNum management

If you’ve fixed a specific problem that has material differences to the code base, you should also include an update to the version number as well. I presume there’s tooling that exists to do that within the ROOL toolchain, but I use vmanage which is an extension of my original commit tool, which was based on descriptions of the srccommit tool that Acorn used. (https://github.com/gerph/riscos-vmanage)

vmanage inc

This could be its own change, or part of the actual fixed code itself.

At this point you can push the change to the server and then it’s nicely preserved. Other people might download it at that point, so you have to be careful about what you do with amend (or rebase).

What to do when completing a feature/fix?

This is the point at which you ask the question about how you continue. It really depends on what you’re trying to do.

Raising a PR

If the feature is complete (which depends on what your definition of complete is, obviously), then you could create a pull request (merge request in many systems, but colloqially just a PR). I would do this if the feature is complete and you want someone to review it for inclusion in the target branch.

However, you might not feel it is complete, and may want to continue work. In which case, just keep working on the branch and push more changes as necessary – and then create the PR when you’re ready.
OR you could create the PR as a draft, and keep working on it. Only when you’re ready do you need to take the PR out of draft for people to review. Whilst in ‘draft’ state, it’s generally expected that the content is in flux, so whilst people might review it, the entire thing could change at any time.

If you feel the feature it is complete and you want people to review it, you may not wish to go down the draft route, and instead make it ready for review directly. You should only do this if the changes made are complete and usable on their own. If the feature is incomplete, and you want to continue working on it, you either shouldn’t make a PR or should leave it in draft.

Continuing development

Whilst that change is being reviewed, if you want to continue working on a follow up feature (ie not the original feature but a development of it), then create a new branch off the original branch. You can then continue working on that branch entirely independant of the review process for the initial change. If you finish that development of the change, you can then push the new branch, and create a PR, but instead of setting the branch target as the master / main / devel (or whatever your main line is called), you set the target to your initial branch, and you keep the PR in draft. Once the first PR has been reviewed and had fixes made to it, your second PR will change its target to be master (or whatever), because the branch it was targetting has been merged. And then you can move that PR out of draft.

You can chain PRs in this way as much as you like. However, the greater number of PRs that you have queued up, the more work you will need to do to rebase any changes that are requested on the preceding PRs.

Independant development

An alternative possibility is that after submitting the first PR for review, you are working on an independent feature – something that doesn’t depend on the original PR. In which case, start a new branch from your mainline branch (master / main / devel or whatever), and just continue working. This will allow your secondary work to be merged independant of the first – if the first isn’t suitable, or takes a lot longer to be accepted, or has contentous elements, your secondary work can go in first. It’s also easier to review smaller, independant changes, so this may accelerate the review process.

Never underestimate the ability to split work on features into multiple independant parts that improve things, before finally tying themtogether later. If you’re working on a feature that adds a new way of importing and exporting a document, you might create one set of changes that does the import, one set that does an export. Mostly those two parts of the code changes won’t intersect so you can do them largely independantly.

Incidental fixes

What about ‘incidental fixes’ ? Sometimes called ‘drive-by fixes’, these are things that you see along the way whilst you’re adding a feature or addressing an independant bug. Let’s say that you spotted a typo in the documentation, or the error handling had an off by one error. The former case could probably be dropped into the change that you’re addressing without too much contention. However, the latter is a functional change that might be wrong (and may be independant of what you’re trying to do as your primary work).

In this case, it’s really good to capture those fixes, but not within the unrelated work. If you’re using git add -p then it’s quite possible that you’ll be able to leave out those changes that aren’t relevant to your primary work, but that’s not the way that I tend to do things – so take this with a pinch of salt.

My usual way of handling such fixes is to git stash everything I am currently working on (or commit it to the current branch, if it’s in a good state), then check out a new branch for just that one problem.

git stash
git checkout -b fix-off-by-one-error-in-widgets master

Then I’d make the change to address the issue and (where possible) update the test code to exercise the change. The change can then be pushed to the origin and a PR created for that change. Finally, I return to the original branch and unstash the changes that I had made, leaving me back where I was.

git checkout -
git stash pop

Addressing PR comments

Another way that your question could be interpreted (I’m not sure exactly what scenario you’re thinking about, so I’ve given a few cases) is that you’re trying to fix up things that were broken in your PR, either because you’d not tested them and they were found by CI, or because people commented on them.

There’s a number of ways to handle this – and it kinda depends on the project and the team’s policies (and how comfortable you are with git) as to what works.

Commits to fix things.

If people give you comments on your PR and you want to fix them, you can just make changes and commit them as independant commits. That’s not a terrible thing – it’ll make it clear that they’re fixes and why the original code wasn’t quite correct. If you’re doing this, make sure you note that you’re fixing PR comments or that at least be explicit that you’re fixing the changes you made previously.

This is the simplest way to address things because all you are doing is adding new commits on top of the existing ones. It’ll keep the history sane and anyone reviewing, or who has pulled your branch will just see the updated changes.

For sanity, I’d suggest trying to address a bunch of related things in each commit if you do things that way – like fixing a typos in a single commit, or renaming functions etc in one commit.

“I’ll do it later” (or not at all!)

Some comments are just that – they don’t actually need to be addressed and you can agree with your reviewer to not bother. Or to defer addressing to a separate change. Just make sure that things that you’ve agreed to actually get done – create an issue or even create a branch and PR with no code in, just as a placeholder to make sure it gets done.

On the other hand, some things are necessary, and the reviewer should be clear that you /need/ to do something, rather than that they’d like you to! :-)

Amending the last commit

You can make an amendment to the last commit with the —amend option, as I suggested earlier. This works, and you can usually push to your branch (you usually cannot push such changes to the main line branch this way, though). However, it does mean that PR comments may refer to a change that no longer exists in the history, or people who have pulled your branch for testing may be affected by your handing re-written history. Do this only if you feel confident that you understand the effects of doing force pushes and what that means to others.

‘fixup’ commits.

Fixup commits are getting more advanced still. In this case, you create a git commit with the special —fixup option. Let’s say that someone noticed that a commit you made on your branch had a mistake and you want to fix that and not leave a broken version in the history.

First you locate the SHA of the change that you want to update – use git log -p, or take the SHA from the history in the PR interface. Then you make the change to the existing code. Then do a commit using that SHA, eg:

git commit --fixup 7795982eb52d3276 -a

This will create a special commit that can be automatically resolved when you do a rebase.

Because it is a deferred record that history needs to be re-written, but isn’t yet doing so, anyone who has your branch isn’t going to get force pushes overwriting the branches and the comment history isn’t going to be affected.

If you do use this, then before the change is merged, you will need to rebase the branch with the automatic fixups applied. To do this you need to use:

git rebase -i --autosquash master

It’s not for the faint of heart, so only do this if you know how to get out of it if things go wrong (and know a little about git reflog, for preference).

I’m not sure if that’s all that useful to you, but that’s how I work and what has worked for me. ROOL’s policies might differ in rewriting history and how you should address review comments, but I suspect they won’t be too far off this lot.

Fix-up sounds like it might be what I’m thinking about thanks, it makes sense that if I change a commit in the middle of a bunch then there’s going to be a rebase involved somewhere!

Be aware, though, that neither git commit --fixup SHA or git rebase are present in the current ROOL Git client – you’ll need to use those on other systems, at the current time.

(nor, for that matter is git stash present in the ROOL Git client, IIRC)

It’s probably worth mentioning that you can also merge using ‘—squash’ (or via the same option on most PR systems).

This allows you to have a branch with many commits (either because it’s a long running feature or that’s just how you work; or because of some back and forth in a review), but merge that branch as a single commit to the target branch.

This allows you to commit to fix mistakes (which is preferable to re-writing history usually), but then merge those commits with a single relevant commit message to the target branch so you keep it’s history free from the noise that can be generated by many commits.