github-merge: context not isolated

[real-or-random]

One potential pitfall of github-merge is that works in your normal worktree. That is, you can switch branches, change things, etc, between two git commands issued by the script, in particular when it waits for user input. For example, if you switch to a different branch and then type s, you sign the wrong commit. (I haven't tested it but this is what I assume from reading the code.)

Could git worktrees help here?
https://git-scm.com/docs/git-worktree

[laanwj]

When it throws you into a shell there is code to guard against this ("ERROR: Tree hash changed unexpectedly"), but right, not when it's waiting for s. I think you could force it into making a mistake if you switch branches in a different terminal window.

Could add a further check that the git commit --gpg-sign only signs the previously determined top commit. I wouldn't mind some more belts and suspenders here, though I don't think it's possible to protect against a malicious parallel usage of git in general (there's always some window for a race condition).

A worktree wouldn't fully avoid this either. It's possible to switch branches in a worktree. But maybe having a new (temporary) worktree makes it less likely for conflicts like this to arise? Could have this as an option.

[real-or-random]

I wouldn't mind some more belts and suspenders here, though I don't think it's possible to protect against a malicious parallel usage of git in general (there's always some window for a race condition).

Yep, I was more thinking of accidents -- if there's a malicious second process you're lost.

A worktree wouldn't fully avoid this either. It's possible to switch branches in a worktree. But maybe having a new (temporary) worktree makes it less likely for conflicts like this to arise? Could have this as an option.

Right, this was the idea. Of course you could modify this temporary worktree in another shell but then you probably know what you're doing.

[laanwj]

If you're going to implement this, I can think of some complications:

• the user may already be using a worktree (e.g. for bitcoin core I have a (fixed) worktree per branch)
• re-using the build environment; a new worktree always starts empty, so re-bootstrapping and configure and a full rebuild is needed before testing (sometimes this may be desirable, but not always)

[maflcko]

Maybe a warning that no git "write-actions" are allowed while the script is running (only git "read-actions" like show, diff or log) would be sufficient?

[laanwj]

You could add a warning to the "documentation" I guess. But I think everyone within the very small circle of users of this tool is aware they shouldn't interfere with it in that way 😄

Still, mistakes happen, I think adding a check that it's signing (and pushing) the correct commit probably can't hurt.

[real-or-random]

A worktree wouldn't fully avoid this either. It's possible to switch branches in a worktree. But maybe having a new (temporary) worktree makes it less likely for conflicts like this to arise? Could have this as an option.

Right, this was the idea. Of course you could modify this temporary worktree in another shell but then you probably know what you're doing.

I just run into opentimestamps/opentimestamps-client#87, so a worktree is not a good idea here.