Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 15, 2024

Conversation

jblebrun
Copy link
Contributor

@jblebrun jblebrun commented May 22, 2024

In git 2.45, reference transactions now support symbolic refs (see:
git/git@a8ae923)

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 jblebrun force-pushed the fix-symbolic-tx-refs branch 2 times, most recently from 12b3ad0 to a63852a Compare May 22, 2024 20:45
@jblebrun jblebrun force-pushed the fix-symbolic-tx-refs branch 17 times, most recently from 759212e to 85dc225 Compare May 22, 2024 23:56
@jblebrun jblebrun changed the title Fix for Bug #1321 - Support symbolic refs in tx lines Fix for Bug #1321 - Support symbolic refs in tx lines for git 2.45 May 22, 2024
@jblebrun jblebrun force-pushed the fix-symbolic-tx-refs branch 2 times, most recently from 89f4ab0 to 0961aa8 Compare May 23, 2024 00:16
@jblebrun jblebrun marked this pull request as ready for review May 23, 2024 00:37
@jblebrun jblebrun force-pushed the fix-symbolic-tx-refs branch 6 times, most recently from 22e3766 to aa612db Compare May 23, 2024 01:17
@jblebrun
Copy link
Contributor Author

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

@jblebrun
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

arxanas commented May 26, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

arxanas commented Jun 1, 2024

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 #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
Copy link
Contributor Author

jblebrun commented Jun 8, 2024

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

@jblebrun jblebrun force-pushed the fix-symbolic-tx-refs branch 5 times, most recently from a6aacce to a2af993 Compare June 10, 2024 00:22
In git 2.45, reference transactions now support symbolic refs (see:
git/git@a8ae923)

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.
Copy link
Owner

@arxanas arxanas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to reproduce the test failure on my end. Thanks for the fix!

@arxanas arxanas merged commit 8651f92 into arxanas:master Jun 15, 2024
13 checks passed
@mochja
Copy link

mochja commented Aug 6, 2024

When may we expect a new release containing this fix?

arxanas added a commit that referenced this pull request Sep 7, 2024
This is the most recent version of Git that reproduces the "Could not parse reference-transaction-line" issue as discussed in #1388 and hopefully fixed in #1322.
@arxanas
Copy link
Owner

arxanas commented Oct 9, 2024

@mochja unfortunately I haven't been able to release yet, but you can follow #1388 for updates.

@arxanas arxanas added this to the v0.10.0 milestone Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants