arxanas / git-branchless

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

`git sync` should be more controllable #574

Closed cloudhan closed 1 year ago

cloudhan commented 2 years ago

Description of the bug

git sync will move all branches to be based on origin/main which is intrusive. As the users (me, particularly) might not want to fiddle around with upstream main. For example, upstream main may has CI break that needs to be fixed or testing failure after merge. And once you pulled, there is no easy way to fix it, maybe reset origin/main. Sync with local main solves the problem by leaving to control to users. As we can easily pick one commit from the main branch history just by examine the CI status of them. sync all local branches onto it and keep working and wait for upstream fix and then pull and sync again.

So, in my opinion, sync should not fetch, should not have network activities involved. It should just automatically move all my working commit to a dest.

Expected behavior

git sync should be controllable

Actual behavior

git sync is not controllable

Version of rustc

not apply

Version of git-branchless

not apply

Version of git

not apply

arxanas commented 2 years ago

So, in my opinion, sync should not fetch, should not have network activities involved. It should just automatically move all my working commit to a dest.

git sync already shouldn't do network activity unless you pass -p/--pull. If you want to use your local copy of main as the main branch instead of the remote copy, then you should just need to configure your main branch as main instead of origin/main. Can you provide specific reproduction steps?

Maybe you actually want https://github.com/arxanas/git-branchless/issues/355 to be implemented, so that git sync -p moves the local main branch to the remote origin/main branch?

cloudhan commented 2 years ago

I accidentally fetched and now it tries to sync with my (remote origin/main)

The following smartlog is after

git fetch --all
git sync --merge
git undo
◇ e3bdba3 3d (main, origin/main) Mitigate prefast static analysis warnings (#13032)
┣━┓
⋮ ◯ a835c37 16h Use tunable GEMM
⋮ ┣━┓
⋮ ┃ ◯ cd712f6 16h Use composable_kernel unconditionally
⋮ ┃ ┃
⋮ ┃ ◯ a2d3f11 16h Exclude glitched CK gemm impl and enable more tests for CK's GEMM
⋮ ┃ ┃
⋮ ┃ ◯ 75c5042 16h Update Status propagation for gemm_rocblas.h
⋮ ┃ ┃
⋮ ┃ ◯ 997160d 1h Improve the verbose error reporting for kernel_explorer's gemm_test
⋮ ┃ ┃
⋮ ┃ ◯ 767639b 1h (guangyunhan/ort-use-tunable-gemm) Skip GEMM tuning if beta is non-zero
⋮ ┃ ┃
⋮ ┃ ◯ da0e952 1h Add tuning switch
⋮ ┃ ┃
⋮ ┃ ◯ 7cde73d 1h Allow enable or disable TunableOp via session options and environment variable
⋮ ┃ ┃
⋮ ┃ ◯ 4409a8e 1h (guangyunhan/ort-gemm-use-tunable-switch) Allow enable tunbale op with provider options
⋮ ┃ ┃
⋮ ┃ ◯ bd71e82 1h Add verbose level log for TunableOp
⋮ ┃ ┃
⋮ ┃ ◯ ea5c1d8 1h Fix unsound hipify in ROCm EP
⋮ ┃ ┃
⋮ ┃ ◯ cc6188d 1h [DONT MERGE] Backup my workspace config
⋮ ┃ ┃
⋮ ┃ ◯ 39cc474 1h [DONT MERGE] Fix
⋮ ┃ ┃
⋮ ┃ ◯ b76894f 1h (guangyunhan/main) Fix clang-tidy(cppcoreguidelines-pro-bounds-array-to-pointer-decay)
⋮ ┃ ┃
⋮ ┃ ◯ 42c3fbc 1h Move code closer to its usage
⋮ ┃ ┃
⋮ ┃ ◯ 6930bba 1h Factor out SetEpForAllNodes
⋮ ┃ ┃
⋮ ┃ ◯ f0b557c 1h Factor out ExecuteModelForEps
⋮ ┃ ┃
⋮ ┃ ◯ c28dccf 1h Allow tests construct RocmExecutionProvider with TunableOp enabled
⋮ ┃ ┃
⋮ ┃ ◯ c268c6e 1h (guangyunhan/refactor-provider-test-utils) Skip instead of error when SetEpsForAllNodes failed
⋮ ┃ ┃
⋮ ┃ ◯ f53cd54 1h (guangyunhan/test-tunable-gemm) Enable TunableOp test for ROCm execution provider by adding an additional EP with TunableOp enabled
⋮ ┃
⋮ ◯ 86d7480 21s fix rocm ci pipeline build failure
⋮ ┃
⋮ ◯ 8d4aed5 21s (guangyunhan/update-rocm-ci) use --build
⋮
✕ 4d85106 20h (rewritten as a835c374) (remote origin/main) Update find_optimizer_opset_version_updates_required.py to use the ONNX headers to determine the latest opset.  (#12484)
┃
✕ 8fb1a01 53s (rewritten as a2d3f114) Exclude glitched CK gemm impl and enable more tests for CK's GEMM
┃
✕ f4252c1 53s (rewritten as 75c5042d) Update Status propagation for gemm_rocblas.h
┃
✕ 57a0e8b 53s (rewritten as 997160da) Improve the verbose error reporting for kernel_explorer's gemm_test
┃
✕ 29ded01 52s (rewritten as 767639b5) Skip GEMM tuning if beta is non-zero
┃
✕ 5f9f0c9 52s (rewritten as da0e952e) Add tuning switch
┃
✕ 0d5fbfa 52s (rewritten as 7cde73d1) Allow enable or disable TunableOp via session options and environment variable
┃
✕ 9caad3d 52s (rewritten as 4409a8e2) Allow enable tunbale op with provider options
┃
✕ 091a65a 52s (rewritten as bd71e821) Add verbose level log for TunableOp
┃
✕ 9737607 51s (rewritten as ea5c1d8a) Fix unsound hipify in ROCm EP
┃
✕ 0e0247e 51s (rewritten as cc6188d6) [DONT MERGE] Backup my workspace config
┃
✕ ec9e4b7 51s (rewritten as 39cc4746) [DONT MERGE] Fix
┃
✕ b110b80 51s (rewritten as b76894fa) Fix clang-tidy(cppcoreguidelines-pro-bounds-array-to-pointer-decay)
┃
✕ e1a0134 51s (rewritten as 42c3fbca) Move code closer to its usage
┃
✕ 93d5780 50s (rewritten as 6930bba5) Factor out SetEpForAllNodes
┃
✕ c94d540 50s (rewritten as f0b557c3) Factor out ExecuteModelForEps
┃
✕ 3426998 50s (rewritten as c28dccfa) Allow tests construct RocmExecutionProvider with TunableOp enabled
┃
✕ a5fe3c3 50s (rewritten as c268c6ef) Skip instead of error when SetEpsForAllNodes failed
┃
⦻ 2b9c259 49s (rewritten as f53cd54a) Enable TunableOp test for ROCm execution provider by adding an additional EP with TunableOp enabled
> git config --list  | grep main
branch.main.remote=origin
branch.main.merge=refs/heads/main
branchless.core.mainbranch=main
cloudhan commented 2 years ago

I want something like git sync --dest <a-commit-hash> so that I can precisely set the sync point.

arxanas commented 2 years ago

I see. I was mistaken: git sync does sync to the remote branch (if there is one). That being said, I think the solution to this problem is still to implement #355, as then we could sync on top of the main branch directly.

In the meantime, you could run something like git move -s 'draft()' -d <dest> to sync your commits (or, if there are merge conflicts, you can sync one stack at a time).

cloudhan commented 2 years ago

Nope, #355 is about sync with updated main branch. And it is quite easy to achieve with git fetch origin main:main && git sync --merge, done. This is about sync with outdated main branch, and it is not achievable.

cloudhan commented 2 years ago

I am also a bit confused here, what is the remote origin/main, when compared to origin/main? I never saw that with stock git, you either pull it or not pull it.

arxanas commented 2 years ago

Nope, https://github.com/arxanas/git-branchless/issues/355 is about sync with updated main branch. And it is quite easy to achieve with git fetch origin main:main && git sync --merge, done. This is about sync with outdated main branch, and it is not achievable.

Yes, #355 isn't about syncing with an outdated main branch. I just suspect that fixing #355 properly will also make it possible to do the behavior specified in this issue and sync with an outdated main branch. Currently, the way we handle git sync is by using the remote branch origin/main if it's available, which we shouldn't be doing. Fixing that issue is probably necessary in order to solve #355.

I am also a bit confused here, what is the remote origin/main, when compared to origin/main? I never saw that with stock git, you either pull it or not pull it.

Apparently you have a local branch called origin/main and also a remote branch called origin/main, i.e. you have both refs/heads/origin/main and refs/remotes/origin/main. This is typically a bad idea, since then the branch names are ambiguous. If you didn't create the origin/main branch yourself, then this is probably a bug in git-branchless; if you find a set of reproduction steps, it would be helpful if you opened an issue.

cloudhan commented 1 year ago

Apparently you have a local branch called origin/main

This is a weird truth.