git: show fetch progress with `git.subprocess = true` #5519

bsdinis · 2025-01-30T13:17:41Z

Checklist

If applicable:

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

lib/src/git_subprocess.rs

…out tty With this and "git --progress" PR jj-vcs#5519, remote git progress will be displayed. The sideband callback is disabled in git2 code path if progress is not enabled. If this were enabled, most "git fetch" tests would fail with git2 due to remote progress messages. Since there's no git2 API to turn the remote progress off, and the git2 code path is supposed to be phased out, I just inserted ad-hoc workaround.

…out tty With this and "git --progress" PR #5519, remote git progress will be displayed. The sideband callback is disabled in git2 code path if progress is not enabled. If this were enabled, most "git fetch" tests would fail with git2 due to remote progress messages. Since there's no git2 API to turn the remote progress off, and the git2 code path is supposed to be phased out, I just inserted ad-hoc workaround.

bsdinis · 2025-01-31T02:40:58Z

@yuja there is some weird interplay here with #5521

Basically, in the case that the remote sends a message while the progress bar is updating, the progress bar will be split and not disappear, as it should.

In particular, there is one message, like this:

remote: Total 72578 (delta 173), reused 124 (delta 79), pack-reused 72267 (from 2)

I'm going to work on a fix in front of this. It will be hacky, but we can iterate over it

bsdinis · 2025-01-31T03:00:38Z

How it looked before:

Fetching into new repo in "/tmp/tmp.usqsXuL1dA/jj"
remote: Enumerating objects: 56033, done.
remote: Counting objects: 100% (7/7), done.
remote: Compressing objects: 100% (4/4), done.
 98% [████████████████████████████████████████████████████████████████████████████████████████████████████████████▊  ]remote: Total 56033 (delta 5), reused 3 (delta 3), pack-reused 56026 (from 1)

How it looks now (bar disappears when it's complete, as it should):

remote: Enumerating objects: 56034, done.
remote: Total 56034 (delta 6), reused 3 (delta 3), pack-reused 56026 (from 1)

yuja · 2025-01-31T03:12:27Z

Basically, in the case that the remote sends a message while the progress bar is updating, the progress bar will be split and not disappear, as it should.

Doesn't it an issue of the frontend? I think the UI will have to hide progress bar when sideband message is received, or something like that.

bsdinis · 2025-01-31T03:20:03Z

Basically, in the case that the remote sends a message while the progress bar is updating, the progress bar will be split and not disappear, as it should.

Doesn't it an issue of the frontend? I think the UI will have to hide progress bar when sideband message is received, or something like that.

I mean, it would require a more involved frontend here. The progress bar is hidden in the end by issuing a \r.
I tried looking for a clean solution there but couldn't find one. To be fair, there is a single message here.
Happy to leave a comment about it, but unless I'm missing something really obvious we could use, it seems like that should be its own PR

yuja · 2025-01-31T03:35:00Z

Doesn't it an issue of the frontend? I think the UI will have to hide progress bar when sideband message is received, or something like that.

I mean, it would require a more involved frontend here. The progress bar is hidden in the end by issuing a \r. I tried looking for a clean solution there but couldn't find one.

Yeah, the frontend will have to do something based on both prgoress and sideband messages, but I think that's the correct way to fix the problem. It's weird that two separate machinery is writing to the same output stream.

EDIT: fwiw, I think these callbacks can be merged into one once we drop support for git2.

it seems like that should be its own PR

Yes. If the current state is annoying, I would revert my patch.

bsdinis · 2025-01-31T03:37:10Z

What do you think of the solution here of holding the sideband messages while progress is going on? It seems to be the best compromise (send sideband immediately when available, hold them for later when not).

Agree that it's not the cleanest, but sounds like it wouldn't be to bad.

yuja · 2025-01-31T03:39:08Z

What do you think of the solution here of holding the sideband messages while progress is going on? It seems to be the best compromise (send sideband immediately when available, hold them for later when not).

I suspect that the git2 impl would be already broken (if progress and sideband messages are processed in the same way as git command.) I haven't test that.

bsdinis · 2025-01-31T03:42:24Z

% ~/dev/jj/target/debug/jj --config 'git.subprocess=false' git clone https://github.com/jj-vcs/jj
Fetching into new repo in "/tmp/tmp.xjeT5o8aH9/jj"
remote: Enumerating objects: 72578, done.
remote: Counting objects: 100% (315/315), done.
remote: Compressing objects: 100% (227/227), done.
 22%  35.3 MiB at  11.9 MiB/s [████████████████████████████████████████████▉                                                                                                                                                                 ]remote: Total 72578 (delta 176), reused 135 (delta 86), pack-reused 72263 (from 2)
 31% [████████████████████████████████████████████████████████████████████████▏                                                                                                                                                              ]

It is

yuja · 2025-01-31T03:42:34Z

I suspect that the git2 impl would be already broken (if progress and sideband messages are processed in the same way as git command.) I haven't test that.

It is. I'll take a look later.

bsdinis · 2025-01-31T03:42:58Z

Now that I remember, the sideband callback for git fetch operations were always set to none.

This helps sideband writer overwrite the progress bar. Suppose progress information is less important than sideband messages, it should be okay to always overwrite the progress bar. The sideband writer will erase the trailing characters and move the cursor back to the first column, and/or put "\n" accordingly.

I think this is copy-paste error from C implementation.

yuja · 2025-01-31T07:15:24Z

I suspect that the git2 impl would be already broken

#5530

bsdinis · 2025-01-31T17:52:21Z

Tested both this PR and git2 on top of #5530 .
Works like a charm.
Thanks

lib/src/git_subprocess.rs

yuja

Thanks.

lib/src/git_subprocess.rs

yuja reviewed Jan 30, 2025

View reviewed changes

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

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

bsdinis force-pushed the bsdinis/rowomzsmyxok branch 3 times, most recently from a597be4 to 229553d Compare January 30, 2025 18:24

bsdinis force-pushed the bsdinis/rowomzsmyxok branch from 229553d to 3f758a9 Compare January 31, 2025 03:05

yuja added 2 commits January 31, 2025 16:10

cli: remove handling of null terminated string from git sideband writer

007bb80

I think this is copy-paste error from C implementation.

bsdinis force-pushed the bsdinis/rowomzsmyxok branch from 3f758a9 to e2ddeb1 Compare January 31, 2025 18:27

steadmon reviewed Feb 1, 2025

View reviewed changes

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

bsdinis force-pushed the bsdinis/rowomzsmyxok branch from e2ddeb1 to 54955c3 Compare February 1, 2025 00:45

yuja approved these changes Feb 1, 2025

View reviewed changes

lib/src/git_subprocess.rs Show resolved Hide resolved

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

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

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

lib/src/git_subprocess.rs Show resolved Hide resolved

git: show fetch progress with git.subprocess = true

3fbef62

bsdinis force-pushed the bsdinis/rowomzsmyxok branch from 54955c3 to 3fbef62 Compare February 1, 2025 01:33

bsdinis enabled auto-merge February 1, 2025 01:35

bsdinis added this pull request to the merge queue Feb 1, 2025

Merged via the queue into main with commit b35d503 Feb 1, 2025
36 checks passed

bsdinis deleted the bsdinis/rowomzsmyxok branch February 1, 2025 02:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

git: show fetch progress with `git.subprocess = true` #5519

git: show fetch progress with `git.subprocess = true` #5519

bsdinis commented Jan 30, 2025

bsdinis commented Jan 31, 2025 •

edited

Loading

bsdinis commented Jan 31, 2025

yuja commented Jan 31, 2025

bsdinis commented Jan 31, 2025

yuja commented Jan 31, 2025 •

edited

Loading

bsdinis commented Jan 31, 2025

yuja commented Jan 31, 2025

bsdinis commented Jan 31, 2025

yuja commented Jan 31, 2025

bsdinis commented Jan 31, 2025

yuja commented Jan 31, 2025

bsdinis commented Jan 31, 2025

yuja left a comment

git: show fetch progress with git.subprocess = true #5519

git: show fetch progress with git.subprocess = true #5519

Conversation

bsdinis commented Jan 30, 2025

Checklist

bsdinis commented Jan 31, 2025 • edited Loading

bsdinis commented Jan 31, 2025

yuja commented Jan 31, 2025

bsdinis commented Jan 31, 2025

yuja commented Jan 31, 2025 • edited Loading

bsdinis commented Jan 31, 2025

yuja commented Jan 31, 2025

bsdinis commented Jan 31, 2025

yuja commented Jan 31, 2025

bsdinis commented Jan 31, 2025

yuja commented Jan 31, 2025

bsdinis commented Jan 31, 2025

yuja left a comment

Choose a reason for hiding this comment

git: show fetch progress with `git.subprocess = true` #5519

git: show fetch progress with `git.subprocess = true` #5519

bsdinis commented Jan 31, 2025 •

edited

Loading

yuja commented Jan 31, 2025 •

edited

Loading