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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement cherry-pick #1849

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

IAmSSH
Copy link

@IAmSSH IAmSSH commented Dec 29, 2023

I'm adding a parameter to an existing command X:

  • add parameter to the function in src/api/X.js (and src/commands/X.js if necessary)
  • document the parameter in the JSDoc comment above the function
  • add a test case in __tests__/test-X.js if possible
  • if this is your first time contributing, run npm run add-contributor and follow the prompts to add yourself to the README
  • squash merge the PR with commit message "feat(X): Added 'bar' parameter"

I'm adding a new command:

  • add as a new file in src/api (and src/commands if necessary)
  • add command to src/index.js
  • update __tests__/test-exports.js
  • create a test in src/__tests__
  • document the command with a JSDoc comment
  • add page to the Docs Sidebar website/sidebars.json
  • add page to the v1 Docs Sidebar website/versioned_sidebars/version-1.x-sidebars.json
  • if this is your first time contributing, run npm run add-contributor and follow the prompts to add yourself to the README
  • squash merge the PR with commit message "feat: Added 'X' command"

Should close: #1736

Hey guys, at the moment this is a very basic and initial implementation of cherry-pick. I am yet to complete the implementation and update the documentation, but before that just wanted to get an opinion from you if the approach is indeed correct. I didn't want to spend effort in adding the tests and updating the documentation just to find out in the end that the basic approach itself is incorrect 馃槄.
I have added the cherryPick command in api and command, and have also added a very simple and basic test case. I would really appreciate your feedback on the implementation so far.
Thanks.

@IAmSSH
Copy link
Author

IAmSSH commented Dec 29, 2023

@mojavelinux , @jcubic , @araknast Requesting your comments.

@jcubic
Copy link
Contributor

jcubic commented Dec 29, 2023

First of all the tests didn't pass, you have Lint errors. Fix lint errors so we can see if the tests pass. They are executed with && in one line.

@jcubic
Copy link
Contributor

jcubic commented Dec 29, 2023

Thanks for your contribution, I need to check the code locally how the function works on real repo.

@IAmSSH
Copy link
Author

IAmSSH commented Dec 30, 2023

First of all the tests didn't pass, you have Lint errors. Fix lint errors so we can see if the tests pass. They are executed with && in one line.

Done. All tests passing now.

@jcubic
Copy link
Contributor

jcubic commented Dec 30, 2023

Do you really need those options:

      ours: 'a',
      theirs: 'd7676dfe102cd28ec794b25f9a6231af0cdfd16d',

Can you just name them oid and branch like in other API methods?

@IAmSSH
Copy link
Author

IAmSSH commented Dec 30, 2023

Do you really need those options:

      ours: 'a',
      theirs: 'd7676dfe102cd28ec794b25f9a6231af0cdfd16d',

Can you just name them oid and branch like in other API methods?

I still have some refactoring to do. Those were left-overs from the code for merge. I will be using proper names and will be completing the refactoring soon.
For this PR, I just wanted to get your opinion if the initial implementation/algorithm looks fine?

@jcubic
Copy link
Contributor

jcubic commented Dec 30, 2023

I'm not sure if the algorithm is correct or not. I don't know what actually cherry-pick should do. I will test locally and see what is the effect.

@IAmSSH
Copy link
Author

IAmSSH commented Jan 3, 2024

Hey @jcubic , any updates?

@jcubic
Copy link
Contributor

jcubic commented Jan 3, 2024

Sorry, I was busy. Didn't have time to test it locally.

@jcubic
Copy link
Contributor

jcubic commented Jan 3, 2024

I just test it locally and it doesn't work correctly. I've got the message in the log but the file that I've modified didn't change in the repo. I think that you need to add the code to check if the current branch is ours and if so update the working tree.

I end up with this state:

On branch test
Changes to be committed:
  (use "git restore --staged <file>..." to unstage)
	modified:   README.md

and the same changes are in the commit.

if I run git reset I get diff that the changes (from cherry-pick) got removed.

I was testing using canonical git after calling cherry-pick to the current branch.

Also, I would add a unit test to check if the working tree is clean and up to date after cherry pick.

@IAmSSH
Copy link
Author

IAmSSH commented Jan 3, 2024

Got it. It works when the target branch is not the current branch. But, you are correct, I need to update the working tree when it is the current branch. I'll make the necessary changes.
Thanks for the feedback 馃憤.

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.

Support cherry-pick
2 participants