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

feat: support profile parameters in pull & clone command #28

Merged

Conversation

EscapeB
Copy link
Member

@EscapeB EscapeB commented Feb 28, 2024

Basic Checks

Have you run rush change for this change?

  • Yes
  • No

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

Summary

Support profile related parameters in pull & clone command

Detail

  • add function ensureSkeletonExistAndUpdated in GitSparseCheckoutService to avoid duplicated initialization for skeleton.
  • add new pull command in sparo with profile related parameters support.
  • add profile related parameters support in clone command.
  • resolveSparoProfileAsync function was moved to SparoProfileService
  • add preprocessProfileArgs and syncProfileState in SparoProfileService for code sharing.

Notice:
Unlike sparo checkout command, clone and pull command will invoke native git command first and try to get profile in local file system.

How to test it

Manually tested.

@EscapeB EscapeB force-pushed the feat/support_profile_parameters_in_pull branch from 06c0b96 to e8691c3 Compare February 28, 2024 12:50
@CLAassistant
Copy link

CLAassistant commented Feb 29, 2024

CLA assistant check
All committers have signed the CLA.

@EscapeB EscapeB changed the title WIP: feat: support profile parameters in pull command WIP: feat: support profile parameters in pull & clone command Feb 29, 2024
@EscapeB EscapeB force-pushed the feat/support_profile_parameters_in_pull branch from c53468f to b3771c3 Compare February 29, 2024 08:33
@EscapeB EscapeB changed the title WIP: feat: support profile parameters in pull & clone command feat: support profile parameters in pull & clone command Feb 29, 2024
@EscapeB EscapeB force-pushed the feat/support_profile_parameters_in_pull branch from 65c81f9 to e3ce816 Compare March 4, 2024 06:55
@EscapeB EscapeB requested a review from chengcyber March 4, 2024 07:33
@EscapeB EscapeB enabled auto-merge March 4, 2024 07:38
@chengcyber
Copy link
Member

chengcyber commented Mar 4, 2024

I had a discussion with Pete, the implementation in this PR actually introduces easy-to-use features. That says sparo clone ... --profile <profile> is a shortcut to run sparo clone ... && sparo checkout --profile.

However, let's see an edge case here, assuming:

  1. T0: sparo checkout --profile A
  2. T1: sparo pull origin/master --profile B

In the implementation in this PR, it can be explained as

  1. T0: sparo checkout --profile A
  2. T1: sparo pull origin/master && sparo checkout --profile B

A noticeable thing in the step 2 is that internal merging/rebasing process is based on the scope of profile A.

What if profile A is deleted in master branch? Or profile A cannot be correctly checkout enough folders when merging/rebasing. In this edge case, the sparo checkout folders are somehow inefficient during merging/rebasing from origin/master. Which cannot reflect the semantic meaning of providing --profile <profile> to sparo pull.

A more intuitive way for this is merging/rebasing from origin/master by using the specified profile (B here). So, we conclude that --profile parameters should be first introduced in sparo merge and sparo rebase. Then, it could be a convenience usage to call sparo pull --profile <profile> as a shortcut to sparo fetch && sparo merge/rebase --profile <profile>.

To this PR,

  1. sparo clone --profile <profile> can be safely merged.
  2. sparo pull --profile <profile> should be introduced after sparo merge|rebase --profile <profile

@EscapeB
Copy link
Member Author

EscapeB commented Mar 5, 2024

I had a discussion with Pete, the implementation in this PR actually introduces easy-to-use features. That says sparo clone ... --profile <profile> is a shortcut to run sparo clone ... && sparo checkout --profile.

However, let's see an edge case here, assuming:

  1. T0: sparo checkout --profile A
  2. T1: sparo pull origin/master --profile B

In the implementation in this PR, it can be explained as

  1. T0: sparo checkout --profile A
  2. T1: sparo pull origin/master && sparo checkout --profile B

A noticeable thing in the step 2 is that internal merging/rebasing process is based on the scope of profile A.

What if profile A is deleted in master branch? Or profile A cannot be correctly checkout enough folders when merging/rebasing. In this edge case, the sparo checkout folders are somehow inefficient during merging/rebasing from origin/master. Which cannot reflect the semantic meaning of providing --profile <profile> to sparo pull.

A more intuitive way for this is merging/rebasing from origin/master by using the specified profile (B here). So, we conclude that --profile parameters should be first introduced in sparo merge and sparo rebase. Then, it could be a convenience usage to call sparo pull --profile <profile> as a shortcut to sparo fetch && sparo merge/rebase --profile <profile>.

To this PR,

  1. sparo clone --profile <profile> can be safely merged.
  2. sparo pull --profile <profile> should be introduced after sparo merge|rebase --profile <profile

I had a discussion with Pete, the implementation in this PR actually introduces easy-to-use features. That says sparo clone ... --profile <profile> is a shortcut to run sparo clone ... && sparo checkout --profile.

However, let's see an edge case here, assuming:

  1. T0: sparo checkout --profile A
  2. T1: sparo pull origin/master --profile B

In the implementation in this PR, it can be explained as

  1. T0: sparo checkout --profile A
  2. T1: sparo pull origin/master && sparo checkout --profile B

A noticeable thing in the step 2 is that internal merging/rebasing process is based on the scope of profile A.

What if profile A is deleted in master branch? Or profile A cannot be correctly checkout enough folders when merging/rebasing. In this edge case, the sparo checkout folders are somehow inefficient during merging/rebasing from origin/master. Which cannot reflect the semantic meaning of providing --profile <profile> to sparo pull.

A more intuitive way for this is merging/rebasing from origin/master by using the specified profile (B here). So, we conclude that --profile parameters should be first introduced in sparo merge and sparo rebase. Then, it could be a convenience usage to call sparo pull --profile <profile> as a shortcut to sparo fetch && sparo merge/rebase --profile <profile>.

To this PR,

  1. sparo clone --profile <profile> can be safely merged.
  2. sparo pull --profile <profile> should be introduced after sparo merge|rebase --profile <profile

Got, I will disable pull command for now and merge this PR first.

@EscapeB
Copy link
Member Author

EscapeB commented Mar 5, 2024

I had a discussion with Pete, the implementation in this PR actually introduces easy-to-use features. That says sparo clone ... --profile <profile> is a shortcut to run sparo clone ... && sparo checkout --profile.

However, let's see an edge case here, assuming:

  1. T0: sparo checkout --profile A
  2. T1: sparo pull origin/master --profile B

In the implementation in this PR, it can be explained as

  1. T0: sparo checkout --profile A
  2. T1: sparo pull origin/master && sparo checkout --profile B

A noticeable thing in the step 2 is that internal merging/rebasing process is based on the scope of profile A.

What if profile A is deleted in master branch? Or profile A cannot be correctly checkout enough folders when merging/rebasing. In this edge case, the sparo checkout folders are somehow inefficient during merging/rebasing from origin/master. Which cannot reflect the semantic meaning of providing --profile <profile> to sparo pull.

A more intuitive way for this is merging/rebasing from origin/master by using the specified profile (B here). So, we conclude that --profile parameters should be first introduced in sparo merge and sparo rebase. Then, it could be a convenience usage to call sparo pull --profile <profile> as a shortcut to sparo fetch && sparo merge/rebase --profile <profile>.

To this PR,

  1. sparo clone --profile <profile> can be safely merged.
  2. sparo pull --profile <profile> should be introduced after sparo merge|rebase --profile <profile

I had a discussion with Pete, the implementation in this PR actually introduces easy-to-use features. That says sparo clone ... --profile <profile> is a shortcut to run sparo clone ... && sparo checkout --profile.

However, let's see an edge case here, assuming:

  1. T0: sparo checkout --profile A
  2. T1: sparo pull origin/master --profile B

In the implementation in this PR, it can be explained as

  1. T0: sparo checkout --profile A
  2. T1: sparo pull origin/master && sparo checkout --profile B

A noticeable thing in the step 2 is that internal merging/rebasing process is based on the scope of profile A.

What if profile A is deleted in master branch? Or profile A cannot be correctly checkout enough folders when merging/rebasing. In this edge case, the sparo checkout folders are somehow inefficient during merging/rebasing from origin/master. Which cannot reflect the semantic meaning of providing --profile <profile> to sparo pull.

A more intuitive way for this is merging/rebasing from origin/master by using the specified profile (B here). So, we conclude that --profile parameters should be first introduced in sparo merge and sparo rebase. Then, it could be a convenience usage to call sparo pull --profile <profile> as a shortcut to sparo fetch && sparo merge/rebase --profile <profile>.

To this PR,

  1. sparo clone --profile <profile> can be safely merged.
  2. sparo pull --profile <profile> should be introduced after sparo merge|rebase --profile <profile

Got, I will revert the changes in pull command and merge this PR first.

1 similar comment
@EscapeB
Copy link
Member Author

EscapeB commented Mar 5, 2024

I had a discussion with Pete, the implementation in this PR actually introduces easy-to-use features. That says sparo clone ... --profile <profile> is a shortcut to run sparo clone ... && sparo checkout --profile.

However, let's see an edge case here, assuming:

  1. T0: sparo checkout --profile A
  2. T1: sparo pull origin/master --profile B

In the implementation in this PR, it can be explained as

  1. T0: sparo checkout --profile A
  2. T1: sparo pull origin/master && sparo checkout --profile B

A noticeable thing in the step 2 is that internal merging/rebasing process is based on the scope of profile A.

What if profile A is deleted in master branch? Or profile A cannot be correctly checkout enough folders when merging/rebasing. In this edge case, the sparo checkout folders are somehow inefficient during merging/rebasing from origin/master. Which cannot reflect the semantic meaning of providing --profile <profile> to sparo pull.

A more intuitive way for this is merging/rebasing from origin/master by using the specified profile (B here). So, we conclude that --profile parameters should be first introduced in sparo merge and sparo rebase. Then, it could be a convenience usage to call sparo pull --profile <profile> as a shortcut to sparo fetch && sparo merge/rebase --profile <profile>.

To this PR,

  1. sparo clone --profile <profile> can be safely merged.
  2. sparo pull --profile <profile> should be introduced after sparo merge|rebase --profile <profile

I had a discussion with Pete, the implementation in this PR actually introduces easy-to-use features. That says sparo clone ... --profile <profile> is a shortcut to run sparo clone ... && sparo checkout --profile.

However, let's see an edge case here, assuming:

  1. T0: sparo checkout --profile A
  2. T1: sparo pull origin/master --profile B

In the implementation in this PR, it can be explained as

  1. T0: sparo checkout --profile A
  2. T1: sparo pull origin/master && sparo checkout --profile B

A noticeable thing in the step 2 is that internal merging/rebasing process is based on the scope of profile A.

What if profile A is deleted in master branch? Or profile A cannot be correctly checkout enough folders when merging/rebasing. In this edge case, the sparo checkout folders are somehow inefficient during merging/rebasing from origin/master. Which cannot reflect the semantic meaning of providing --profile <profile> to sparo pull.

A more intuitive way for this is merging/rebasing from origin/master by using the specified profile (B here). So, we conclude that --profile parameters should be first introduced in sparo merge and sparo rebase. Then, it could be a convenience usage to call sparo pull --profile <profile> as a shortcut to sparo fetch && sparo merge/rebase --profile <profile>.

To this PR,

  1. sparo clone --profile <profile> can be safely merged.
  2. sparo pull --profile <profile> should be introduced after sparo merge|rebase --profile <profile

Got, I will revert the changes in pull command and merge this PR first.

@EscapeB EscapeB merged commit 3a7a42e into tiktok:main Mar 5, 2024
1 check passed
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.

4 participants