Fix bug where a forgotten branch couldn't be fetched (v2) #1771

ilyagr · 2023-06-29T02:49:41Z

New approach to fix the bug described in the (now closed) PR #1714

Checklist

If applicable:

I have updated CHANGELOG.md
(n/a) I have updated the documentation (README.md, docs/, demos/)
(n/a) I have updated the config schema (src/config-schema.json)
I have added tests to cover my changes

This is a bit unwieldy; see a comment in the code for a detailed explanation. We'll have to decide whether we prefer this, or we prefer the approach of martinvonz#1771, or we have a better idea.

ilyagr · 2023-06-30T03:09:16Z

Update: This approach is now replaced by the "export before fetch" approach discussed below.

@martinvonz @yuja Are we OK with the "branch forget: force jj git export of relevant branches" commit? It's inelegant, but I think it's OK for the users and seems essential to this approach. In other ways, this approach is a lot cleaner than #1714.

This is a bit unwieldy; see a comment in the code for a detailed explanation. We'll have to decide whether we prefer this, or we prefer the approach of martinvonz#1771, or we have a better idea.

ilyagr · 2023-06-30T04:02:00Z

Another radical possibility would be to do automatic import/export in all repositories. This would remove a lot of test duplication and special cases in tests as well.

The cost is:

Some performance
More race conditions when different jj instances update the branches in the git repo all the time (TBH, now that's it's written down, this seems like a deal-breaker to me)
Potential users who want to really garden their local git repo branches in a normal jj repo. I don't know that such users exist.

yuja · 2023-06-30T04:11:39Z

Are we OK with the "branch forget: force jj git export of relevant branches" commit? It's inelegant, but I think it's OK for the users and seems essential to this approach.

I disagree at least for now. We have @git branch in jj branch list, which indicates that the forgotten branch isn't exported yet. I think we can add something similar for remote refs.

Maybe jj branch forget should be moved under jj git subcommand, but I don't have concrete idea.

ilyagr · 2023-06-30T04:26:42Z

I think there's another option that @yuja may in fact have mentioned before: instead of doing jj git export after jj branch forget, we could do some part of it (just the remote-tracking branches, perhaps?) before jj git fetch. I'm not 100% sure, but it seems like that should be sufficient.

yuja · 2023-06-30T13:28:30Z

instead of doing jj git export after jj branch forget, we could do some part of it (just the remote-tracking branches, perhaps?) before jj git fetch.

It sounds less scary as fetch is supposed to move remote-tracking branches, but I prefer to address this edge case separately. Without a workaround, I think local and remote-tracking branches behave consistently in that jj git export propagates the "forgot" operation to git backend.

ilyagr · 2023-06-30T21:05:13Z

It sounds less scary as fetch is supposed to move remote-tracking branches, but I prefer to address this edge case separately.

Do you have an idea in mind?

IMO, the export before fetch idea becomes quite similar to #1714. In a way, I could call the latter "addressing this edge case separately". I'm not sure I prefer #1714 to export-before-fetch; I currently think export-before-fetch is slightly cleaner (though slightly less optimized), but I'd be OK with either. I don't have a third idea at the moment.

yuja · 2023-07-01T03:15:33Z

Do you have an idea in mind?

No. "export before fetch" would probably work, but I'm not pretty sure.

IMO, the export before fetch idea becomes quite similar to #1714. In a way, I could call the latter "addressing this edge case separately". I'm not sure I prefer #1714 to export-before-fetch; I currently think export-before-fetch is slightly cleaner (though slightly less optimized), but I'd be OK with either. I don't have a third idea at the moment.

#1714 is bad in that it introduces inconsistent handling of git_refs. #1711 respects the current git_refs design, removes inconsistency from export_refs(), and will probably fix fetch & undo issue thanks to #1700. That's why I like this approach.

I don't think "export before fetch" is a hard requirement (the behavior should be logically correct without it), but it will solve UX problem without breaking the internal design, I suppose.

ilyagr · 2023-07-01T03:48:50Z

No. "export before fetch" would probably work, but I'm not pretty sure.

At the moment, I'm pretty comfortable with "export before fetch". It sounds like it's worth implementing it, especially since most of the work is already done. So, I'll do that, but let me know if you get a better idea or if you'd like me to wait.

I also feel that the "export before fetch" approach is more likely to improve other strange fetch-undo-import-export interactions, but I haven't thought it though.

I do think of the UX problem as pretty serious. I think a user in a non-colocated repo should never need to run jj git export for things to work as expected. It would be very confusing for there to be a situation where a user is told that if jj git fetch does not work for them, they should "just" do jj git export; jj git fetch. The user might no longer remember that they did jj branch forget in the past, and they certainly shouldn't have to keep track of whether they did or not.

yuja · 2023-07-01T04:11:49Z

At the moment, I'm pretty comfortable with "export before fetch". It sounds like it's worth implementing it, especially since most of the work is already done. So, I'll do that, but let me know if you get a better idea or if you'd like me to wait.

Sounds good. I just don't wanna reject the whole PR just because the latter part could introduce another problem. (It's more like a UX problem of github ;)

BTW, do you think git_ref_filter should take &RefName in place of &str? I have such patch, but I'm not sure if that's good idea.

This is a bit unwieldy; see a comment in the code for a detailed explanation. We'll have to decide whether we prefer this, or we prefer the approach of martinvonz#1771, or we have a better idea.

ilyagr · 2023-07-01T22:25:20Z

OK, here's export-before-fetch. PTAL, @yuja.

This is a bit unwieldy; see a comment in the code for a detailed explanation. We'll have to decide whether we prefer this, or we prefer the approach of martinvonz#1771, or we have a better idea.

yuja

Thanks!

lib/src/git.rs

This follows 3779b45, but in this case the refactor makes the logic more complicated. The main goal here is to prepare for the next commit.

I now believe that jj will need to store git-tracking refs for both local and remote-tracking branches of the git repo for the long term. See martinvonz#1666 (comment) More refactoring will likely happen when that bug is fixed.

ilyagr · 2023-07-03T04:50:03Z

Thank you Yuya for the review and the many helpful suggestions!

I'm planning to submit this ~~in an hour or so~~ in 10 hours or so, unless I hear from you that I should wait. One of your suggestions turned out to be a bit messy to fix, so I added another commit at the end. If it's possible to make it cleaner, and it doesn't happen in this PR, we can do it later.

🎉

ilyagr force-pushed the export-del-remote branch from da8c61f to 468308d Compare June 29, 2023 02:54

ilyagr changed the title ~~Export del remote~~ Fix bug where a forgotten branch couldn't be fetched (v2) Jun 29, 2023

ilyagr force-pushed the export-del-remote branch 2 times, most recently from b6416b1 to 26e5947 Compare June 30, 2023 00:41

ilyagr mentioned this pull request Jun 30, 2023

jj op undo/restore no longer restore git-tracking branches by default #1700

Merged

2 tasks

ilyagr force-pushed the export-del-remote branch from 26e5947 to 9d78105 Compare June 30, 2023 01:15

ilyagr mentioned this pull request Jun 30, 2023

FR: better design for ignoring branches #1734

Closed

6 tasks

ilyagr force-pushed the export-del-remote branch from 9d78105 to 106e3d6 Compare June 30, 2023 01:44

ilyagr force-pushed the export-del-remote branch from 106e3d6 to 51940ba Compare June 30, 2023 03:05

ilyagr marked this pull request as ready for review June 30, 2023 03:05

ilyagr force-pushed the export-del-remote branch from 51940ba to 33c8d72 Compare June 30, 2023 03:06

ilyagr force-pushed the export-del-remote branch from 33c8d72 to 29cca97 Compare June 30, 2023 03:16

ilyagr force-pushed the export-del-remote branch from 29cca97 to 9b54e48 Compare June 30, 2023 03:18

ilyagr force-pushed the export-del-remote branch from 9b54e48 to e29ff72 Compare July 1, 2023 20:57

ilyagr force-pushed the export-del-remote branch 3 times, most recently from 900a81b to 7ef607c Compare July 1, 2023 22:20

ilyagr force-pushed the export-del-remote branch 2 times, most recently from cc7cbf1 to ccfa922 Compare July 1, 2023 22:48

yuja approved these changes Jul 2, 2023

View reviewed changes

lib/src/git.rs Show resolved Hide resolved

lib/src/git.rs Outdated Show resolved Hide resolved

lib/src/git.rs Show resolved Hide resolved

lib/src/git.rs Outdated Show resolved Hide resolved

ilyagr force-pushed the export-del-remote branch 3 times, most recently from 96227f4 to 794747a Compare July 3, 2023 03:40

git export: (almost) no-op refactor to export_refs to use RefName

7c916f8

This follows 3779b45, but in this case the refactor makes the logic more complicated. The main goal here is to prepare for the next commit.

ilyagr force-pushed the export-del-remote branch from 794747a to b372e08 Compare July 3, 2023 03:49

ilyagr added 3 commits July 2, 2023 21:03

git export: export deletion of forgotten remote-tracking branches

8131cb1

lib/git.rs: inline functions used only once, rename others

39d0d3f

I now believe that jj will need to store git-tracking refs for both local and remote-tracking branches of the git repo for the long term. See martinvonz#1666 (comment) More refactoring will likely happen when that bug is fixed.

git.rs: create export_some_refs

a4acfa7

ilyagr force-pushed the export-del-remote branch 2 times, most recently from 47fb598 to da89112 Compare July 3, 2023 04:45

ilyagr mentioned this pull request Jul 3, 2023

Fix bug where a forgotten branch couldn't be fetched (v1) #1714

Closed

2 tasks

git fetch: do a git export of deleted branches before fetch

cc90f45

ilyagr force-pushed the export-del-remote branch 2 times, most recently from f2c2439 to be52132 Compare July 3, 2023 05:46

git fetch: limit export of deleted refs to provided globs

8eef8cd

ilyagr force-pushed the export-del-remote branch from be52132 to 8eef8cd Compare July 3, 2023 17:48

ilyagr enabled auto-merge (rebase) July 3, 2023 17:52

ilyagr merged commit 597a74d into martinvonz:main Jul 3, 2023

ilyagr deleted the export-del-remote branch July 3, 2023 18:13

chenrui333 mentioned this pull request Jul 16, 2023

jj 0.8.0 Homebrew/homebrew-core#136764

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug where a forgotten branch couldn't be fetched (v2) #1771

Fix bug where a forgotten branch couldn't be fetched (v2) #1771

ilyagr commented Jun 29, 2023 •

edited

Loading

ilyagr commented Jun 30, 2023 •

edited

Loading

ilyagr commented Jun 30, 2023 •

edited

Loading

yuja commented Jun 30, 2023

ilyagr commented Jun 30, 2023

yuja commented Jun 30, 2023

ilyagr commented Jun 30, 2023

yuja commented Jul 1, 2023

ilyagr commented Jul 1, 2023

yuja commented Jul 1, 2023

ilyagr commented Jul 1, 2023

yuja left a comment

ilyagr commented Jul 3, 2023 •

edited

Loading

Fix bug where a forgotten branch couldn't be fetched (v2) #1771

Fix bug where a forgotten branch couldn't be fetched (v2) #1771

Conversation

ilyagr commented Jun 29, 2023 • edited Loading

Checklist

ilyagr commented Jun 30, 2023 • edited Loading

ilyagr commented Jun 30, 2023 • edited Loading

yuja commented Jun 30, 2023

ilyagr commented Jun 30, 2023

yuja commented Jun 30, 2023

ilyagr commented Jun 30, 2023

yuja commented Jul 1, 2023

ilyagr commented Jul 1, 2023

yuja commented Jul 1, 2023

ilyagr commented Jul 1, 2023

yuja left a comment

Choose a reason for hiding this comment

ilyagr commented Jul 3, 2023 • edited Loading

ilyagr commented Jun 29, 2023 •

edited

Loading

ilyagr commented Jun 30, 2023 •

edited

Loading

ilyagr commented Jun 30, 2023 •

edited

Loading

ilyagr commented Jul 3, 2023 •

edited

Loading