Skip to content

Git Improvements #1458

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

Merged
merged 6 commits into from
Oct 31, 2023
Merged

Conversation

avinizhanov
Copy link
Contributor

Description

I did a few git improvements:

  1. In GitClient changed all operations to async and changed all calls wrapped in Task, some operations was blocking main thread. Also changed GitClient.runLive from publisher to async stream, for some reason publisher failed for me in the middle of large cloning and I didn't find a reason why. Async stream works fine.
  2. SourceControlManager/GitClient is now single object per workspace. Except for clone.

Screencasts with changes:

Git cloning progress and cancelation (for large git repos). When you cancel cloning it will remove folder previously created for this repo.

Parsing of cloning output and progress calculation inspired by https://github.com/microsoft/vscode/blob/main/extensions/git/src/git.ts

Screen.Recording.2023-10-21.at.2.07.54.PM.mov

Cloning and branch selection (if more than one branch)

Screen.Recording.2023-10-21.at.2.12.29.PM.mov

Git status in project navigator/source control navigator and discarding changes.
If file opened in a tab, it will not reload content. I think this requires changes to CodeEditTextView.

Screen.Recording.2023-10-21.at.2.16.29.PM.mov

Commit and push.

Screen.Recording.2023-10-21.at.2.21.42.PM.mov

Branches view, new branch, delete branch, checkout (local and remote).

Screen.Recording.2023-10-21.at.2.24.06.PM.mov

Design used inspired by VS Code, Intellij and Xcode.
If you have any suggestions I can change it.

Related Issues

Checklist

  • I read and understood the contributing guide as well as the code of conduct
  • The issues this PR addresses are related to each other
  • My changes generate no new warnings
  • My code builds and runs on my machine
  • My changes are all related to the related issue above
  • I documented my code

Not sure about warning, it doesn't generate any new in Xcode for me 🙂

@Angelk90
Copy link
Contributor

@avinizhanov : Some considerations:

  1. Having some information about what it is doing during cloning would be helpful.

Check it out here: #593 (comment)

  1. If a repo contains more than one branch but I don't want to clone just a specific branch, therefore only one, but all of them, as happens in normal cloning, does it work?

2b) Specific case:
What if I wanted to clone only some specific branches, not all of them, but some, for example branch1 and a branch2?

Solution shell, specific case To clone specific branches from a Git repository using the `git` command, you can follow these steps:
  1. Open your terminal or command prompt.
  2. Navigate to the directory where you want to clone the repository.
  3. Run the following command to clone the branch1:

git clone --single-branch --branch branch1 https://github.com/example/repository.git

This command clones only the branch1 and not the entire repository.

  1. Repeat step 3 for branch2 by running the following command:

git clone --single-branch --branch branch2 https://github.com/example/repository.git

This command clones only the branch2 and not the entire repository.

  1. If the file is modified on vscode, does the update happen on CE?
    You can see that the file is being modified, is there a modification?
    Either at the commit level.

  2. By Sync Changes do you mean push?

@avinizhanov
Copy link
Contributor Author

@Angelk90

Having some information about what it is doing during cloning would be helpful.

Ok, I'll add it.

If the file is modified on vscode, does the update happen on CE?

Yes. It will update git status for a file. Here is example:

Screen.Recording.2023-10-23.at.7.17.27.AM.mov

By Sync Changes do you mean push?

Yes, copied label from VS Code (Sync Changes/Publish Branch). I can rename it.

What if I wanted to clone only some specific branches, not all of them, but some, for example branch1 and a branch2?

I just checked VS Code, PhpStorm and Xcode. Only xcode asking branch before cloning.
I guess we can get branches with git ls-remote --heads and show branch selector before cloning.

@Angelk90
Copy link
Contributor

@avinizhanov :

  1. You can do this test by opening the same file on both your CE and VS, updating the file on Vs.
    Put the CE tab, the one where it allows you to commit and push.
    To see if it updates.

  2. For Sync Changes, I mean there are changes in the remote repository so I want to sync mine, so I think.

Screenshot 2023-10-23 alle 15 18 18 Screenshot 2023-10-23 alle 15 18 55 Screenshot 2023-10-23 alle 15 19 03
  1. For cloning I do it this way, you can tell me what you think.
    After user put the link to clone, do this:

Check how many branches there are, if there is only one branch there are no problems, a git clone is fine.
The user is not asked anything.

If there are more than one branch, the user is asked which branches he wants, the possible cases are:

a) to everyone
git clone...

b) A list of branches, then a multiple select, array of branch names.
Execute one after the other or all together (We need to see which solution is better) a series of this command specifying the name of the branch.

git clone --single-branch --branch NAME_BRANCH https://github.com/example/repository.git

c) Only one branch, we fall back into case b) with an array consisting only of a branch name.

@avinizhanov
Copy link
Contributor Author

@Angelk90

You can do this test by opening the same file on both your CE and VS, updating the file on Vs.
Put the CE tab, the one where it allows you to commit and push.
To see if it updates.

Do you mean if you commit file in VS, will it update status in CE?

For Sync Changes, I mean there are changes in the remote repository so I want to sync mine, so I think.

For now it will only push. Pull is not supported yet.

For cloning I do it this way, you can tell me what you think.

I'll try to do this.

@Angelk90
Copy link
Contributor

@avinizhanov : That's what I meant:

Registrazione.schermo.2023-10-23.alle.16.42.58.mov

Copy link
Member

@tom-ludwig tom-ludwig left a comment

Choose a reason for hiding this comment

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

Great work! I was only able to quickly skim through the code, but it looks pretty good overall. Just a few small improvements to consider: better error handling, more informative comments, and clearer function names.

@Angelk90
Copy link
Contributor

@avinizhanov : Some considerations:

  1. Cloning via ssh doesn't work.
Screenshot 2023-10-26 alle 15 27 39
  1. If I clone a repository that contains only one branch, for example the main one, it tells me this:
Screenshot 2023-10-26 alle 15 32 27
  1. It's okay that after cloning it asks you which branch you want to checkout in.
    But there is no step where there are multiple branches, it asks you if you want to clone a single branch or multiple branches, discussed previously.

@thecoolwinter
Copy link
Collaborator

@Angelk90 To address your 3rd point, when the user switches to a remote branch, it'll be up to them to pull changes. This is the same as gh desktop. I've addressed your 2nd point in a review I'm finishing up now. Lets leave ssh cloning to a different PR. We'll need to add support for keys and other authentication methods to fully support that.

Copy link
Collaborator

@thecoolwinter thecoolwinter left a comment

Choose a reason for hiding this comment

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

I've left a bunch of suggestions, they're all small changes to either code style or a small UI change and a few bug fixes. This is a large PR and I think it's needed to have a good base for our git integration, so I think we'll hold off on any further UI changes in this PR. I have more UI things I want changed but I think we'll merge them in a different PR due to this PR already being large. For now the UI works and can be improved on.

@avinizhanov
Copy link
Contributor Author

@thecoolwinter thanks for suggestions. I fixed most of them and added comments for 2.

@Angelk90
Copy link
Contributor

@thecoolwinter , @activcoding : For point 2) if there is only one branch there is no need for the checkout request.

@thecoolwinter: For point 3) I didn't understand from your message whether it will be implemented or not.
If it will be implemented, how will it be done can you explain the details?

@thecoolwinter
Copy link
Collaborator

thecoolwinter commented Oct 29, 2023

@thecoolwinter , @ activcoding : For point 2) if there is only one branch there is no need for the checkout request.

Yes you're right, maybe I misunderstood the original problem. @avinizhanov There should be a check in WelcomeView#L119 to make sure there's more than one branch before showing the sheet.

@thecoolwinter: For point 3) I didn't understand from your message whether it will be implemented or not. If it will be implemented, how will it be done can you explain the details?

Yes, it will be implemented. I'd like to make sure it supports cases where a passkey is needed to access an SSH key from keychain. For now though in my testing using SSH to clone a repo did work. If you clone using SSH via terminal do you get any prompts or errors?

@Angelk90
Copy link
Contributor

Angelk90 commented Oct 29, 2023

The reason I think is that you only have one ssh key.
I have more than one, I think it's something CE needs to consider.

When I clone from the terminal I have to do:
git@github.com-USER:NAME/PROJECT.git

But every time I have to fix url.
I think we need to do a check up, if you want to discuss it I'm available whenever you want.

.ssh/config:

# Personal account - default config
Host github.com-user1
   HostName github.com
   User git
   IdentityFile ~/.ssh/id_rsa_user1
# Official account
Host github.com-user2
   HostName github.com
   User git
   IdentityFile ~/.ssh/id_rsa_user2

@avinizhanov
Copy link
Contributor Author

Yes you're right, maybe I misunderstood the original problem. @avinizhanov There should be a check in WelcomeView#L119 to make sure there's more than one branch before showing the sheet.

@Angelk90 @thecoolwinter Fixed this one.

Copy link
Collaborator

@thecoolwinter thecoolwinter left a comment

Choose a reason for hiding this comment

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

One last change and then I think this is good to go.

@avinizhanov
Copy link
Contributor Author

@thecoolwinter works good. thanks

@austincondiff austincondiff requested review from austincondiff and removed request for tom-ludwig October 31, 2023 00:55
Copy link
Collaborator

@austincondiff austincondiff left a comment

Choose a reason for hiding this comment

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

Great work @avinizhanov!

@austincondiff austincondiff merged commit 25bfeb2 into CodeEditApp:main Oct 31, 2023
@thecoolwinter thecoolwinter mentioned this pull request Nov 4, 2023
@thecoolwinter thecoolwinter linked an issue Nov 6, 2023 that may be closed by this pull request
@thecoolwinter thecoolwinter removed a link to an issue Nov 6, 2023
@thecoolwinter thecoolwinter added the enhancement New feature or request label Dec 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants