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

Unexpected rebase after jj git fetch when git.subprocess=true #5700

Open
tw4452852 opened this issue Feb 14, 2025 · 36 comments · May be fixed by #5721
Open

Unexpected rebase after jj git fetch when git.subprocess=true #5700

tw4452852 opened this issue Feb 14, 2025 · 36 comments · May be fixed by #5721
Assignees

Comments

@tw4452852
Copy link

tw4452852 commented Feb 14, 2025

Description

Steps to Reproduce the Problem

  1. Working-copy is based on one of remote branches.
  2. jj git fetch
  3. Working-copy is rebased on the latest git_head().

Expected Behavior

No rebase happens after jj git fetch

Actual Behavior

Working-copy is rebased on the latest git_head().

Specifications

Reproduce steps on my local environment:

~/data/code/xx> cat .git/HEAD
c7d0c233fa822a621a94eda34eee4c21cd4e9ca7                                                                                                           

~/data/code/xx> jj --config 'git.subprocess=true' git fetch
Nothing changed.

~/data/code/xx> cat .git/HEAD
ref: refs/heads/develop

~/data/code/xx> git rev-parse refs/heads/develop
feb29c4f7bd658a25f11275e7796ce597bde3ab2

~/data/code/xx> jj log
Reset the working copy parent to the new Git HEAD.
...

  • Platform: linux
  • Version: jj 0.26.0-37992412b64f828d078789b2db84d1d291412ab0
@tw4452852 tw4452852 changed the title Unexpected rebase on git HEAD when git.subprocess=true Unexpected rebase after jj git fetch when git.subprocess=true Feb 14, 2025
@yuja
Copy link
Contributor

yuja commented Feb 14, 2025

~/data/code/xx> cat .git/HEAD
c7d0c233fa822a621a94eda34eee4c21cd4e9ca7

~/data/code/xx> jj --config 'git.subprocess=true' git fetch
Nothing changed.

~/data/code/xx> cat .git/HEAD
ref: refs/heads/develop

Just to clarify, does that mean .git/HEAD is rewritten by noop jj git fetch? Do you know some configuration that makes git fetch update HEAD? I thought git fetch would only update FETCH_HEAD.

@tw4452852
Copy link
Author

tw4452852 commented Feb 14, 2025

I might find the rootcause: As this is just one of repositories managed by repo. So the $PWD/.git is just a symbolic link. But jj just treat it as git directory as it invokes git with git --bare --git-dir $pwd/.git .... Possible fix might be remove --bare.

@bsdinis bsdinis self-assigned this Feb 14, 2025
@bsdinis
Copy link
Contributor

bsdinis commented Feb 14, 2025

I might find the rootcause: As this is just one of repositories managed by repo. So the $PWD/.git is just a symbolic link. But jj just treat it as git directory as it invokes git with git --bare --git-dir $pwd/.git .... Possible fix might be remove --bare.

I don't quite understand how this would be the issue. --git-dir should still work with a symbolic link.
Nevertheless, will try adding a test for this to help us triage

@bsdinis
Copy link
Contributor

bsdinis commented Feb 14, 2025

@tw4452852 I implemented the test, you can see it in https://github.com/bsdinis/jj/blob/a835fcee21f8753fcb9fb27a66cb56266e4a4b91/cli/tests/test_git_fetch.rs#L1935

I couldn't find this behaviour. Question though: do you know if repo activates git hooks (and maybe they are moving HEAD)? That's a pretty big difference between with the git subprocess code.

We've already had to disable it for push (#5612), maybe we also have to do so for fetch too

@yuja
Copy link
Contributor

yuja commented Feb 15, 2025

cc @bnjmnt4n as he is the git expert.

fwiw, it might be good to set --no-write-fetch-head so jj's fetch request is invisible to external parties.

@tw4452852
Copy link
Author

tw4452852 commented Feb 15, 2025

@bsdinis
I did an experiment on my reproducing repo:

~/data/code/primus/Hypervisor/os-framework> ls -l .git
lrwxrwxrwx 1 tw tw 48 Sep  3 16:10 .git -> ../../.repo/projects/Hypervisor/os-framework.git
~/data/code/primus/Hypervisor/os-framework> git --version
git version 2.48.1
~/data/code/primus/Hypervisor/os-framework> cat .git/HEAD
c7d0c233fa822a621a94eda34eee4c21cd4e9ca7⏎                                          
~/data/code/primus/Hypervisor/os-framework> git fetch
~/data/code/primus/Hypervisor/os-framework> cat .git/HEAD
c7d0c233fa822a621a94eda34eee4c21cd4e9ca7⏎ 
~/data/code/primus/Hypervisor/os-framework> git --git-dir $pwd/.git fetch
~/data/code/primus/Hypervisor/os-framework> cat .git/HEAD
c7d0c233fa822a621a94eda34eee4c21cd4e9ca7⏎                                         
~/data/code/primus/Hypervisor/os-framework> git --bare --git-dir $pwd/.git fetch
~/data/code/primus/Hypervisor/os-framework> cat .git/HEAD
ref: refs/heads/develop

Seems HEAD is only be rewritten in --bare case.

@tw4452852
Copy link
Author

do you know if repo activates git hooks

@bsdinis I think so.

@martinvonz
Copy link
Member

Weird! Doesn't seem to reproduce for me, though.

@martinvonz
Copy link
Member

Oh, but does that experiment work only on that particular repo for you? Are you able to reproduce it on a new repo?

@tw4452852
Copy link
Author

tw4452852 commented Feb 15, 2025

@martinvonz
Yes, this issue only happens on the repos which are managed by repo. I couldn't reproduce it on a normal standalone git repo either.

@martinvonz
Copy link
Member

We have some other git experts from Google who might know that's going on, especially since repo is a Google tool. @nasamuffin, @jonathantanmy, @steadmon.

@tw4452852
Copy link
Author

tw4452852 commented Feb 15, 2025

@yuja @martinvonz @bsdinis
Out of curiosity, I did some debugging on the internal of git fetch, it will update HEAD according to https://github.com/git/git/blob/e2067b49ecaef9b7f51a17ce251f9207f72ef52d/builtin/fetch.c#L1927.

Especially for a bare repo:

https://github.com/git/git/blob/e2067b49ecaef9b7f51a17ce251f9207f72ef52d/builtin/fetch.c#L1660-L1662

This explains why --bare makes a difference.

Here's stacktrace when updating happens:

#0  rename (
    old=0x555555a14750 "/home/tw/data/code/primus/Hypervisor/os-framework/.git/HEAD.lock",
    new=new@entry=0x5555559a28c0 "/home/tw/data/code/primus/Hypervisor/os-framework/.git/HEAD") at ../sysdeps/unix/sysv/linux/rename.c:29
#1  0x00005555557dedae in rename_tempfile (
    tempfile_p=tempfile_p@entry=0x555555a0eb18,
    path=path@entry=0x5555559a28c0 "/home/tw/data/code/primus/Hypervisor/os-framework/.git/HEAD") at ./tempfile.c:346
#2  0x00005555557006ba in commit_lock_file_to (
    path=0x5555559a28c0 "/home/tw/data/code/primus/Hypervisor/os-framework/.git/HEAD", lk=0x555555a0eb18) at ./lockfile.h:317
#3  commit_lock_file (lk=lk@entry=0x555555a0eb18) at ./lockfile.c:210
#4  0x0000555555780dce in commit_ref (lock=lock@entry=0x555555a0eb10)
    at refs/files-backend.c:1763
#5  0x00005555557844d5 in files_transaction_finish (ref_store=<optimized out>,
    transaction=0x555555a0e920, err=0x7fffffffc9c0) at refs/files-backend.c:3204
#6  0x000055555577de3f in ref_transaction_commit (
    transaction=transaction@entry=0x555555a0e920, err=err@entry=0x7fffffffc9c0)
    at ./refs.c:2457
#7  0x000055555577e2cc in refs_update_symref_extended (
    refs=refs@entry=0x555555956b00, ref=0x5555559db380 "HEAD",
    target=<optimized out>, logmsg=logmsg@entry=0x55555583e373 "fetch",
    referent=referent@entry=0x7fffffffcb90, create_only=create_only@entry=0)
    at ./refs.c:2267
#8  0x00005555555c5a13 in set_head (no_warn_branch=<optimized out>,
    follow_remote_head=0, remote_refs=<optimized out>) at builtin/fetch.c:1672
#9  do_fetch (config=0x7fffffffcbd0, rs=0x7fffffffcb10,
    transport=<optimized out>) at builtin/fetch.c:1927
#10 fetch_one (config=0x7fffffffcbd0, use_stdin_refspecs=<optimized out>,
    prune_tags_ok=<optimized out>, argv=<optimized out>, argc=<optimized out>,
    remote=<optimized out>) at builtin/fetch.c:2276
#11 cmd_fetch (argc=<optimized out>, argv=<optimized out>,
    prefix=<optimized out>, repo=<optimized out>) at builtin/fetch.c:2585
#12 0x0000555555573522 in run_builtin (repo=0x5555559495e0 <the_repo>,
    argv=<optimized out>, argc=<optimized out>, p=0x5555559173f8 <commands+984>)
    at ./git.c:480
#13 handle_builtin (args=args@entry=0x7fffffffdee0) at ./git.c:740
#14 0x0000555555574642 in run_argv (args=args@entry=0x7fffffffdee0)
    at ./git.c:807
#15 0x000055555557518c in cmd_main (argc=<optimized out>, argc@entry=6,
    argv=<optimized out>, argv@entry=0x7fffffffe118) at ./git.c:947
#16 0x00005555555731fa in main (argc=6, argv=0x7fffffffe118)
    at ./common-main.c:64

@bnjmnt4n
Copy link
Member

That whole function was introduced in Git v2.48, which is relatively new and might be why others are not able to reproduce. I'll see if I can download and test this out later. (I also thought the new behavior was to update refs/remotes/[remote]/HEAD when fetching remotes, not .git/HEAD, but I might be wrong.)

@yuja
Copy link
Contributor

yuja commented Feb 15, 2025

Oh, so it's the feature introduced recently? One weird thing the repo tool does is that it doesn't set core.bare = true|false. So if we pass --bare, the repository might be processed as a real bare repo?

@tw4452852
Copy link
Author

I also thought the new behavior was to update refs/remotes/[remote]/HEAD when fetching remotes, not .git/HEAD, but I might be wrong

@bnjmnt4n For the non-bare repo this is the case, but not for bare repo though, at least from the implementation (https://github.com/git/git/blob/e2067b49ecaef9b7f51a17ce251f9207f72ef52d/builtin/fetch.c#L1660-L1666):

	if (is_bare) {
		strbuf_addstr(&b_head, "HEAD");
		strbuf_addf(&b_remote_head, "refs/heads/%s", head_name);
	} else {
		strbuf_addf(&b_head, "refs/remotes/%s/HEAD", remote);
		strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", remote, head_name);
	}

@bnjmnt4n
Copy link
Member

It seems like this might be an implementation bug given the commit description in git/git@b1b713f, which seems to say that this should only happen for mirrored bare repositories.

@bsdinis
Copy link
Contributor

bsdinis commented Feb 15, 2025

My thoughts about this:

  • We can't really avoid --bare for non colocated repos. The embedded git repo inside .jj is bare
  • We should probably avoid writing the fetch head like @yuja suggested. I suspect it would solve the issue

@tw4452852
Copy link
Author

tw4452852 commented Feb 15, 2025

Maybe another option is adding --work-tree ? As https://github.com/git/git/blob/e2067b49ecaef9b7f51a17ce251f9207f72ef52d/environment.c#L147-L151

@yuja
Copy link
Contributor

yuja commented Feb 15, 2025

fwiw, all tests passed without "--bare". It's nice that we enforce fetch/push does never touch the work tree, though.

Maybe we can also disable followremotehead until the bug is fixed in upstream.

@bsdinis
Copy link
Contributor

bsdinis commented Feb 15, 2025

@tw4452852
Copy link
Author

tw4452852 commented Feb 16, 2025

@bsdinis

I'm afraid it doesn't fix this problem, here's log:

~/data/code/primus/Hypervisor/os-framework> cat .git/HEAD
c7d0c233fa822a621a94eda34eee4c21cd4e9ca7⏎                                         
~/data/code/primus/Hypervisor/os-framework> jj --version
jj 0.26.0-37992412b64f828d078789b2db84d1d291412ab0
~/data/code/primus/Hypervisor/os-framework> jj --config 'git.subprocess=true' git fetch
~/data/code/primus/Hypervisor/os-framework> cat .git/HEAD
ref: refs/heads/develop

~/data/code/primus/Hypervisor/os-framework> cat .git/HEAD
c7d0c233fa822a621a94eda34eee4c21cd4e9ca7⏎                                         
~/data/code/primus/Hypervisor/os-framework> git --git-dir .git --bare fetch --no-write-fetch-head
~/data/code/primus/Hypervisor/os-framework> cat .git/HEAD
ref: refs/heads/develop

IIRC, --no-write-fetch-head only prevents FETCH_HEAD from being updated.

@tw4452852
Copy link
Author

I also did some spelunkings to find out why this issue can't be reproduced on standalone git repo. The reason is that git will treat these repos as non-bare, even when we specify --bare on command line, as it sets in .git/config explicitly:

[core]
       ...
       bare = false
...

While this is not the case for the repos managed by repo wherebare configuration is not specified in .git/config, so git treats them as bare when we specify --bare command line option.

bsdinis added a commit that referenced this issue Feb 16, 2025
This causes some issues (e.g., #5700) and is not actively needed.
@bsdinis
Copy link
Contributor

bsdinis commented Feb 16, 2025

@tw4452852 yeah, removing --bare might be the option. In that same branch I've added that option. So sorry to keep asking you to triage this but I can't reproduce the issue locally. Could you give it another go?

@tw4452852
Copy link
Author

@bsdinis
It works like a charm again, thanks!

~/data/code/primus/Hypervisor/os-framework> cat .git/HEAD
c7d0c233fa822a621a94eda34eee4c21cd4e9ca7⏎                                         
~/data/code/primus/Hypervisor/os-framework> jj version
jj 0.26.0-b64ad68ad9b593ac9e41ea365be87666b280aef4
~/data/code/primus/Hypervisor/os-framework> jj --config 'git.subprocess=true' git fetch
Nothing changed.
~/data/code/primus/Hypervisor/os-framework> cat .git/HEAD
c7d0c233fa822a621a94eda34eee4c21cd4e9ca7⏎ 

@martinvonz
Copy link
Member

Maybe we can also disable followremotehead until the bug is fixed in upstream.

I don't see a bug report for Git. I suspect no one has reported it there. Let's see what the Git experts in this thread say. Maybe they can confirm that we've understood correctly what bare repos are (in which case it does seem like a bug), or maybe bare repos is a more complicated concept than we think and the new behavior is actually expected.

@bsdinis
Copy link
Contributor

bsdinis commented Feb 18, 2025

@bnjmnt4n did you manage to figure out if this is a bug in git?

@bnjmnt4n
Copy link
Member

Yes, it appears that this was a bug in Git. The following patch was published: https://lore.kernel.org/git/[email protected]/, which looks to have been accepted in https://lore.kernel.org/git/[email protected]/.

@bsdinis
Copy link
Contributor

bsdinis commented Feb 18, 2025

So is it ok to close this issue and the corresponding PR? Given that if git fixes itself we should be fine?

@ilyagr
Copy link
Contributor

ilyagr commented Feb 18, 2025

I haven't thought about it in detail, but would it make sense to merge it until people move off buggy Git versions? Or make it conditional on the installed Git version?

Update: This was a very naive question. I think it was correctly received as such, but I am adding this edit to make sure. I didn't realize it was a single buggy git version, for example.

@bsdinis
Copy link
Contributor

bsdinis commented Feb 18, 2025

I haven't thought about it in detail, but would it make sense to merge it until people move off buggy Git versions? Or make it conditional on the installed Git version?

I think the original patch can actually be split. The first commit I think solves your problem (updating the fetch head). The second one is the one that tries to solve this

I don't think it's good policy to be merging patches intended for a single buggy git version.

@yuja
Copy link
Contributor

yuja commented Feb 19, 2025

I think it's also good to merge the fix at our side, given --bare isn't required. I don't have strong opinion about that.

@bsdinis
Copy link
Contributor

bsdinis commented Feb 19, 2025

I think it's also good to merge the fix at our side, given --bare isn't required. I don't have strong opinion about that.

One way I was thinking --bare was "helping" us was ensuring that it wouldn't ever touch the worktree (especially in the case of the colocated repo). But maybe because our operations don't touch the working tree it doesn't really matter?

bsdinis added a commit that referenced this issue Feb 19, 2025
This causes some issues (e.g., #5700) and is not actively needed.
@yuja
Copy link
Contributor

yuja commented Feb 19, 2025

One way I was thinking --bare was "helping" us was ensuring that it wouldn't ever touch the worktree

Yes. OTOH, it could trigger edge case bug like this, so I'm not sure if which is better overall.

@bsdinis
Copy link
Contributor

bsdinis commented Feb 19, 2025

I think that's fair. The other PR is still ready, can be merged whenever

@martinvonz
Copy link
Member

I think it's unfortunate that we can't pass the --bare flag but I agree that it's better to skip to to work around this bug. We could perhaps tell git to use an empty temporary directory as worktree but it's probably not worth it.

@bsdinis
Copy link
Contributor

bsdinis commented Feb 19, 2025

We could perhaps tell git to use an empty temporary directory as worktree but it's probably not worth it.

I think the hassle of creating and removing temp dirs is probably not worth it. Again, fetch and push should never touch the worktree anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants