search

arxanas / git-branchless

High-velocity, monorepo-scale workflow for Git
Apache License 2.0
3.37k stars 80 forks source link

Fix for Bug #1321 - Support symbolic refs in tx lines for git 2.45 #1322

Closed jblebrun closed 2 weeks ago

jblebrun commented 1 month ago

In git 2.45, reference transactions now support symbolic refs (see: https://github.com/git/git/commit/a8ae923f85da6434c3faf9c39719d6d5e5c77e65)

To support this, we add a transaction reference resolver that's capable of looking up the Oid for a named reference. It requires a repository object to do this successfully.

To support the functionality, a new refname_to_id function is added to the Repo object that calls the similarly named method on the inner repository to get the git2::Oid for a given reference name.

jblebrun commented 1 month ago

Also, sorry for force-pushing and destroying all of the conversation threads. It's muscle memory :-(

jblebrun commented 1 month ago

another possible issue:

While this change fixes the issue for git 2.45, I think it also slows things down quite a bit for some reason. A git sl on the currently released version is instantaneous; with my change, it now takes 3-4 seconds every time.

jblebrun commented 1 month ago

another possible issue:

While this change fixes the issue for git 2.45, I think it also slows things down quite a bit for some reason. A git sl on the currently released version is instantaneous; with my change, it now takes 3-4 seconds every time.

Nevermind, just an issue of debug vs release!

arxanas commented 1 month ago

I looked into the failing test_amend case on master, and it might be that the best long-term solution is to delete the reference-transaction hook entirely and rely on working-copy snapshotting instead 😬.

If we wanted to capture the new information provided by the reference-transaction hook, we'd have to update the event log schema (as it currently stores an OID or a reference name), but not both; and even if we did all that, we'd still have all the reference-transaction hook issues as described above.

The main implementation problem with removing the reference-transaction hook is that we'd have to unpack and diff against the most recent working-copy snapshot in order to restore the current undo workflow and show the list of changed references. (That code path is also effectively used in the bug-report subcommand.)

jblebrun commented 1 month ago

I looked into the failing test_amend case on master, and it might be that the best long-term solution is to delete the reference-transaction hook entirely and rely on working-copy snapshotting instead 😬.

Sounds like it might be better to focus on the longer-term approach, then? Since the git commit that causes the issue isn't released yet, I can just use my personal branch with the patch for now, and we can abandon this PR, if you'd like. It was a fun exercise anyway.

If we wanted to capture the new information provided by the reference-transaction hook, we'd have to update the event log schema (as it currently stores an OID or a reference name), but not both; and even if we did all that, we'd still have all the reference-transaction hook issues as described above.

Is there a benefit to this, vs just sticking with the OIDs like now?

The main implementation problem with removing the reference-transaction hook is that we'd have to unpack and diff against the most recent working-copy snapshot in order to restore the current undo workflow and show the list of changed references. (That code path is also effectively used in the bug-report subcommand.)

I could potentially help with this, although it's a larger-scope changed, so I'd have to get up to speed on a few more things first.

arxanas commented 1 month ago

I could potentially help with this, although it's a larger-scope changed, so I'd have to get up to speed on a few more things first.

I posted https://github.com/arxanas/git-branchless/issues/1334 to detail the necessary work.

Is there a benefit to this, vs just sticking with the OIDs like now?

The main problem is that the reference-transaction hook would potentially misbehave when trying to undo/revert a symbolic reference. We treat HEAD specially in most cases (since there are very few other symbolic references that work well in Git) to get around this, so it's probably not a big deal in practice. It would be fine to just stick with the OIDs for now.

Sounds like it might be better to focus on the longer-term approach, then? Since the git commit that causes the issue isn't released yet, I can just use my personal branch with the patch for now, and we can abandon this PR, if you'd like. It was a fun exercise anyway.

It will presumably be released at some point 😉. If you can add a test to expose the current failing behavior, then I'm happy to merge the PR.

jblebrun commented 3 weeks ago

Ok, I've added a test and done a round of cleanup on this.