The workspace head commit is sometimes signed if gitbutler.signCommits is enabled

[slarse]

Version

master

Operating System

Linux

Distribution Method

N/A

Describe the issue

I've noted that when gitbutler.signCommits is enabled, I'm prompted to sign twice whenever I make a commit. This is both confusing and rather annoying as I sign with an external key, which is rather slow.

I've tracked the underlying reason down to the fact that the workspace tip (along with the new commit) is updated with the rebase engine here:

gitbutler/crates/but-workspace/src/legacy/commit_engine/mod.rs

Lines 300 to 305 in 508016c

	// We can assume the workspace tip is connected to a pick (or else the rebase will fail)
	builder.steps([but_rebase::RebaseStep::Pick {
	commit_id,
	new_message: None,
	}])?;
	match builder.rebase() {

And that eventually ends up creating a signed workspace commit with an underlying create library function, that signs based on the repo settings

gitbutler/crates/but-rebase/src/commit.rs

Line 113 in 508016c

if settings.gitbutler_sign_commits.unwrap_or(false) {

I've found several other forms of updating the workspace tip that do not result in signing. For example, if you edit the commit message of a commit or uncommit, the resulting workspace commit is not signed.

How to reproduce (Optional)

Turn on gitbutler.signCommits, then make a commit.

Run git cat-file -p HEAD to see the signed commit.

Expected behavior (Optional)

In my opinion, the workspace commit should never be signed and the bug is that there's (at least) one case where it is signed. The cases where it isn't signed are IMO to be considered "correct" behavior. It doesn't make much sense to sign as it's never pushed to any remote, it's purely a local commit with the sole purpose of facilitating GitButler's local functionality.

I'd love to help out addressing this, but I'm not sure how to go about it. It seems a bit inelegant to temporarily edit the repo settings. My first thought was to perhaps be able to attach specific settings to a rebase step, or at least annotating the rebase step to say that it's the workspace tip so the rebase engine can know to treat it a bit differently.

Any thoughts?

[Byron]

Thanks a lot for reporting!

This must be a long-standing issue in the old rebase implementation, which probably doesn't special-case workspace commits and thus may end up signing them.

For reference, there is a new implementation here that never signs (or runs hooks) as it just creates the workspace commit itself.

gitbutler/crates/but-workspace/src/commit/mod.rs

Lines 320 to 331 in fc0bce6

	let mut ws_commit =
	Self::new_from_stacks(stacks.iter().cloned(), repo.object_hash());
	ws_commit.tree = merge_tree_id;
	Self::fixup_times(&mut ws_commit, repo);
	let workspace_commit_id = repo.write_object(&ws_commit)?.detach();
	return Ok(Outcome {
	workspace_commit_id,
	stacks,
	missing_stacks: vec![], /* this is never set here as all tips are already resolved */
	conflicting_stacks,
	});

One way of dealing with this in the rebase engine, at least the new one, is to only sign when the commit was signed before. That should naturally avoid signing workspace commits, even if otherwise they don't have to be special.

I'd love to help out addressing this, but I'm not sure how to go about it. It seems a bit inelegant to temporarily edit the repo settings. My first thought was to perhaps be able to attach specific settings to a rebase step, or at least annotating the rebase step to say that it's the workspace tip so the rebase engine can know to treat it a bit differently.

I think not signing unsigned commits would do the trick.

Also CC @Caleb-T-Owens for similar considerations in the new rebase engine.

PS: It would be interesting to see if there is a way to not show up the password prompt each time a commit is signed, which seems very unwieldy. This is usually nothing GitButler can or should influence, but maybe there is a way to help users who find themselves in such a situation.

[slarse]

Thanks a lot for reporting!

This must be a long-standing issue in the old rebase implementation, which probably doesn't special-case workspace commits and thus may end up signing them.

For reference, there is a new implementation here that never signs (or runs hooks) as it just creates the workspace commit itself.

gitbutler/crates/but-workspace/src/commit/mod.rs

Ah, so I gather that this particular issue will more or less work itself out once the new workspace implementation is more widely used?

One way of dealing with this in the rebase engine, at least the new one, is to only sign when the commit was signed before. That should naturally avoid signing workspace commits, even if otherwise they don't have to be special.

I was going to bring this up separately as it's a broader issue, but this kind of skids into the territory so why not: the way GitButler works with signing commits is unwieldy in conjunction with the frequent history rewriting. @krlvi and I had a brief discussion about this, and pondered the idea of signing commits only when pushing, otherwise leaving them unsigned.

I think the sign-on-push should be skippable even if it's "enabled". There are cases where you just don't care.

I've also been pondering adding a context action to the lane context menu that's something like "Sign all unsigned commits", but I'm not sure it's useful if there's sign-on-push functionality. I don't really see a usecase where I'd like to sign without also publishing my commits.

Any thoughts on that?

I think not signing unsigned commits would do the trick.

This is probably orthogonal to the ideas I've expressed above, but as stated I don't actually want to sign a commit locally until I'm ready to push it. Even if it was previously signed. I might be doing a lot of history rewriting between two pushes depending on the workflow.

Even for those that don't need to enter a passphrase for each sign, signing makes committing a heckofalot slower so it's probably an overall UX win to "only sign when necessary", which in my mind is on push.

PS: It would be interesting to see if there is a way to not show up the password prompt each time a commit is signed, which seems very unwieldy. This is usually nothing GitButler can or should influence, but maybe there is a way to help users who find themselves in such a situation.

I feel there is a misunderstanding here. The password prompt showing up for me is correct behavior. My signing key is on an external (card) device, which needs to be unlocked for each sign. That's precisely how I want it, I take signing stuff rather seriously :)

[Byron]

Ah, so I gather that this particular issue will more or less work itself out once the new workspace implementation is more widely used?

That would be my expectation. The question if there is a simple fix now remains, and in theory it should be no problem to make such a fix. It would probably be good to have a reproduction, as it doesn't trivially reproduce for me.

I was going to bring this up separately as it's a broader issue, but this kind of skids into the territory so why not: the way GitButler works with signing commits is unwieldy in conjunction with the frequent history rewriting. @krlvi and I had a brief discussion about this, and pondered the idea of signing commits only when pushing, otherwise leaving them unsigned.

This could be a game-changer!

As a matter of fact, we now have a complication in the new rebase-engine because we can't have side-effects until the changes are actually materialised, so we don't sign until we actually materialise.

Being able to even skip that and resign only before it's actually needed would be great, also for performance.

Something that would make me not want to tackle this is the UX implications - doing a mutation right before a push seems like a big thing to do, and it completely affects what the remote will see.
Implementing this correctly should be very possible though.

However, it also seems to be an optimisation as it should reasonably work well even when we keep everything signed at all times (except for the workspace commit).

I feel there is a misunderstanding here. The password prompt showing up for me is correct behavior. My signing key is on an external (card) device, which needs to be unlocked for each sign. That's precisely how I want it, I take signing stuff rather seriously :)

Right, no help needed here :D.
And in that case, even signing-on-push would be quite a chore, but less so than signing each time something happens.

[slarse]

Something that would make me not want to tackle this is the UX implications - doing a mutation right before a push seems like a big thing to do, and it completely affects what the remote will see.

This is where my tiny idea for a "Sign all unsigned commits" context action is a bit cleaner. But I feel like it might be cleaner from our perspective more so than from a UX perspective. I'm a bit uncertain there.

That being said, the context action is purely additive to the current functionality and requires little to no new UX. It also has the added benefit that it doesn't impose any complexity on a core workflow component (pushing, in this case). As commit signing is (regrettably) still kind of fringe, I think it's a very valid to trade-off to add that extra button click before every push for the few users that will make use of it to reduce core workflow complexity.

I personally would not be bothered at all by having to click once to sign when I feel it's time, as I prefer signing being very deliberate. It could perhaps be adapted wholesale into the CLI as well, unless the corresponding functionality is cleanly baked into something else.

[Byron]

That being said, the context action is purely additive to the current functionality and requires little to no new UX. It also has the added benefit that it doesn't impose any complexity on a core workflow component (pushing, in this case). As commit signing is (regrettably) still kind of fringe, I think it's a very valid to trade-off to add that extra button click before every push for the few users that will make use of it to reduce core workflow complexity.

That I can imagine, purely additive. Finding a good spot for the button to not clutter, and to only show if signing is configured, that seems to be a place to start.

It could perhaps be adapted wholesale into the CLI as well, unless the corresponding functionality is cleanly baked into something else.

Definitely, there'd probably be a new function in but-api which can then be called by the CLI as well.

[slarse]

Perfect, I think it's time to just implement something and see how it feels. I'll give it a go as soon as I'm able, hopefully next week :)