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

cli: Refactor out get_fetch_remotes into git_util.rs #4768

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

essiene
Copy link
Collaborator

@essiene essiene commented Nov 4, 2024

Similarly to git_fetch, this will be used in jj git sync, so
moving this out to maintain the same behaviour across fetch command and sync.

  • Refactor out get_fetch_remotes to git_util.rs.
  • inline the different get_*_remote* functions into get_fetch_remotes;
    The resulting function is still small enough to be easily readable.
  • Move get_single_remote into git_util.rs and update call sites.
  • Update use references

All tests still pass.

Issue: #1039

@essiene essiene force-pushed the essiene/pmwupzxmwsqz branch 2 times, most recently from fa6e402 to e53870d Compare November 4, 2024 21:23
@essiene essiene requested a review from yuja November 4, 2024 21:24
@essiene essiene force-pushed the essiene/pmwupzxmwsqz branch 2 times, most recently from 1591633 to 0b60444 Compare November 4, 2024 22:45
cli/src/git_util.rs Outdated Show resolved Hide resolved
cli/src/git_util.rs Show resolved Hide resolved
Base automatically changed from essiene/vutlmutyytky to main November 5, 2024 12:47
@essiene essiene force-pushed the essiene/pmwupzxmwsqz branch 6 times, most recently from c9ebe82 to ae98431 Compare November 6, 2024 21:14
cli/src/git_util.rs Outdated Show resolved Hide resolved
@essiene essiene force-pushed the essiene/pmwupzxmwsqz branch 4 times, most recently from c2adc4e to 084797c Compare November 11, 2024 10:29
branch: args.fetch.branch.clone(),
remotes: remotes.clone(),
all_remotes: args.fetch.all_remotes,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, we can't use the original args.fetch? That makes args.fetch useless..

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe FetchRemoteArgs can be extracted instead of all-in FetchArgs, but I'm not pretty sure if that makes sense. Sorry for the noise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No worries at all, I may have jumped the gun on the Refactor before finalizing how it will be used. For sync it turns out we need to fetch all branches for all configured remotes[1], so the help text will be different after all, and I don't need to flatten them in. I've reverted that last change, back to just passing FetchArgs{} around with branch, remotes and all_remotes as the fields.

[1] - https://docs.google.com/document/d/1K4j1xQPFxtINhyOyKuCMTZ_iNrw8g-5_giBFMGoLjaI/edit?tab=t.0#bookmark=id.y91cr92o9czf

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not add branch to the args struct then. It doesn't make sense to pass branch in to get_fetch_remotes(), and all_remotes to git_fetch().

It might also be simpler to just make both get_all_remotes() and get_default_fetch_remotes() public.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I think I finally have something that looks descent:

  • git_fetch takes FetchArgs; which only has fields for branch and remotes. This is useful since FetchArgs disambiguated the fields and makes it easier to know what goes where.
  • get_fetch_remotes doesn't need FetchArgs and since remotes and all_remotes are different types completely, it doesn't look terrible if I just pass them in directly

wdyt?

Copy link
Collaborator

@yuja yuja Nov 24, 2024

Choose a reason for hiding this comment

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

Seems okay.

FWIW, I wonder if git_fetch() can actually be reused in "git sync". Suppose "git sync" does rebase based on the stats returned by git::fetch(), "git sync" will have to use lower-level API.

Regarding args.all_remotes, copy-pasting if args.all_remotes { ... } block wouldn't be so bad.

@essiene essiene force-pushed the essiene/pmwupzxmwsqz branch 4 times, most recently from 5f24508 to 9b0981d Compare November 17, 2024 22:08
@essiene essiene force-pushed the essiene/pmwupzxmwsqz branch 8 times, most recently from 4d59421 to 4802573 Compare November 24, 2024 02:32
Similarly to `git_fetch`, this will be used in `jj git sync`, so
moving this out to maintain the same behaviour across `fetch` command and `sync`.

* Refactor out `get_fetch_remotes` to `git_util.rs`.
* inline the different `get_*_remote*` functions into `get_fetch_remotes`;
  The resulting function is still small enough to be easily readable.
* Move `get_single_remote` into `git_util.rs` and update call sites.
* Use common FetchArgs to pass arguments both to `get_fetch_remotes` and `git_fetch`
* Update use references

All tests still pass.

Issue: #1039
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 this pull request may close these issues.

2 participants