Add Branch Selection for `coffee_install()` #242

tareknaser · 2024-02-23T21:01:22Z

Description

Add branch selection for coffee_install and display plugin git HEAD in coffee list.
Fixes #205

Changes

Added Repository::git_head() to retrieve the repository's git HEAD.
Added an optional branch parameter to coffee_install() for branch selection during installation.
Implemented git_upgrade_with_git_head() for checkout using a specific git HEAD.
Updated cmd, coffee_plugin, and coffee httpd to pass the branch value.
Modified coffee list output to display the git HEAD of plugins.

Testing

coffee install <plugin_name> --branch <branch_name>

This function will come in handy when we want to restore the state of a repository on disk after upgrading a plugin Signed-off-by: Tarek <[email protected]>

This commit adds an optional parameter to coffee_install() method that makes it possible to specify a branch when installing a plugin. The logic is: - get information about current status before switching (repoisitory branch and plugin git head) - switch to the branch - restore old information and rollback A function git_upgrade_with_git_head was implemented to handle checking out using a specfic git_head we are using the repository git head to save current status and roll back to it since it's safer than using just the branch (we fetch origin so maybe the branch was updated. we want to be back to the same commit) also, cmd, coffee_plugin and coffee httpd were upgraded to pass the branch value Signed-off-by: Tarek <[email protected]>

Since we now may have separate git heads for plugin and repository, we print the git HEAD of the plugin to the user Signed-off-by: Tarek <[email protected]>

Signed-off-by: Tarek <[email protected]>

netlify · 2024-02-23T21:01:40Z

✅ Deploy Preview for coffee-docs ready!

Name	Link
🔨 Latest commit	`d77053b`
🔍 Latest deploy log	https://app.netlify.com/sites/coffee-docs/deploys/65d907a4608e570008c22b65
😎 Deploy Preview	https://deploy-preview-242--coffee-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

vincenzopalazzo

I think that this implementation is a little bit too complex for what we need to do

vincenzopalazzo · 2024-02-26T09:11:34Z

coffee_lib/src/repository.rs

+    /// return the git head of the repository.
+    fn git_head(&self) -> Option<String>;
+


does not make sense to have this inside the repository trait

good point. I will update it

vincenzopalazzo · 2024-02-26T09:12:03Z

coffee_github/src/repository.rs

+    /// return the git head of the repository.
+    fn git_head(&self) -> Option<String> {
+        self.git_head.clone()
+    }


Move this as a struct method and not an a trait implementation

vincenzopalazzo · 2024-02-26T09:14:10Z

coffee_core/src/coffee.rs

+                // if the branch is specified, we need to save current git_head
+                // and checkout the branch
+                let current_git_head = repo.git_head();
+                let current_plugin_commit = plugin.commit.clone();
+                if let Some(branch) = branch.clone() {
+                    log::debug!("checking out branch: {branch}");
+                    if current_git_head.is_none() {
+                        return Err(error!("unable to get the current git head"));
+                    }
+                    let updated_status =
+                        git_upgrade_with_branch(repo.url().path_string.as_str(), &branch, verbose)
+                            .await?;
+                    log::debug!("updated status: {:?}", updated_status);
+
+                    let repository = git2::Repository::open(repo.url().path_string.as_str())
+                        .map_err(|err| error!("{}", err.message()))?;
+
+                    // get commit id after the checkout
+                    let commit = commit_id!(repository);
+                    log::debug!("commit id after the checkout: {commit}");
+                    plugin.commit = Some(commit);
+                }


IMHO to much code for just sh!("git checkout {branch}")?

I can see some potential issues with simply using git checkout {branch}:

If the branch is new (not fetched yet), we need to git fetch origin first.

To make sure we are returning to the expected state after switching back, we need the repository's Git HEAD, not just the current branch. This is because the branch might have been updated remotely (when we ran git fetch origin), and we want to go back to the exact commit the user was on.

We need to verify that the repository's Git HEAD information exists (introduced in a previous PR as an Option to be backward compatible)

The plugin's commit field is updated during installation and then reverted afterward. Otherwise, we have false information.

Do you think that any of these steps is redundant?

my point is that we can do all with just a couple of git command, with your solution I do not see any fetch anywhere too and we need to make sure that your code is correct, otherwise we will have the same problem that we had with the upgrade logic

git_upgrade_with_branch() does git fetch origin

vincenzopalazzo · 2024-02-26T09:16:30Z

coffee_core/src/coffee.rs

+
+                        if branch.is_some() {
+                            // we can safely unwrap here because we have checked it before
+                            let current_git_head = current_git_head.unwrap();


the safety comment start with // SAFETY: ...

vincenzopalazzo · 2024-02-26T09:17:06Z

coffee_core/src/coffee.rs

+                        if branch.is_some() {
+                            // we can safely unwrap here because we have checked it before
+                            let current_git_head = current_git_head.unwrap();
+                            log::debug!("checking out to git head: {current_git_head}");
+                            // rollback to the previous git head
+                            let updated_status = git_upgrade_with_git_head(
+                                repo.url().path_string.as_str(),
+                                &current_git_head,
+                                verbose,
+                            )
+                            .await?;
+                            log::debug!("updated status: {:?}", updated_status);
+
+                            plugin.commit = current_plugin_commit;
+                        }


wow this install method is starting to be 2 pages long?

I agree!
We can keep coffee install for just installing and configuring plugins and introduce coffee upgrade --plugin <plugin_name> --branch <branch_name> for upgrades and branch switching

No, my point is that the code that we are writing the code of the install is not good at all. Please see some of my PR that I am refactoring it.

In addition, both install and upgrade command will run the same logic and with your code we should duplicate the checkout logic. This is error prone

No, my point is that the code that we are writing the code of the install is not good at all.

Ok. how do you think I can make it better? I explained why we need the extra code in install() in this conversation #242 (comment).
Do you think that any of these steps is redundant or should be implemented in another way?

In addition, both install and upgrade command will run the same logic and with your code we should duplicate the checkout logic. This is error prone

They will have the same logic because they will do very similar tasks. The function git_upgrade_with_commit() will be reused in coffee upgrade --plugin <plugin_name>.
We can start with implementing coffee upgrade --plugin <plugin_name> and then use it directly from coffee install --branch <branch_name>. This way we don't duplicate code

vincenzopalazzo · 2024-02-26T09:17:53Z

coffee_github/src/utils.rs

@@ -25,7 +25,7 @@ pub async fn clone_recursive_fix(repo: git2::Repository, url: &URL) -> Result<()
    Ok(())
 }

-pub async fn git_upgrade(
+pub async fn git_upgrade_with_branch(


why changing the name here?

because I added another function that uses the git commit to upgrade

I do not see the point of changing the name, due that there is no git upgrade command in git

Moreover, git uses always a branch there is never a without_branch cases

vincenzopalazzo · 2024-02-26T09:18:50Z

coffee_github/src/utils.rs

+
+    let (local_commit, _) = get_repo_info!(repo);
+
+    let mut cmd = format!("git fetch origin\n");


let mut cmd = "git fetch origin\n";

format! is not needed

vincenzopalazzo · 2024-02-26T09:19:41Z

coffee_github/src/utils.rs

+pub async fn git_upgrade_with_git_head(
+    path: &str,
+    git_head: &str,
+    verbose: bool,
+) -> Result<UpgradeStatus, CoffeeError> {
+    use tokio::process::Command;


idk if this is a good idea, the checkout is just git checkout <branch-name>

Why we need to use all this code?

vincenzopalazzo · 2024-02-26T09:20:30Z

coffee_cmd/src/coffee_term/command_show.rs

@@ -22,6 +22,7 @@ pub fn show_list(coffee_list: Result<CoffeeList, CoffeeError>) -> Result<(), Cof
        term::format::bold(String::from("Language")),
        term::format::bold(String::from("Name")),
        term::format::bold(String::from("Enabled")),
+        term::format::bold(String::from("Git HEAD")),


a user need also a git PHD to understand what is a git HEAD? Idk if this is necessary.

We already print the git HEAD to the user with coffee remote list
If we add the possibility to manage the branch for the plugin in separation from the repository, I think we have to print it in coffee list

Please call it commit there is no meaning to complicate a simple things with big words.

Sure. Do you want me to rename "Git HEAD" to "commit" in coffee remote list output too?

vincenzopalazzo · 2024-02-26T09:21:22Z

docs/docs-book/src/using-coffee.md

+#### Specifying a branch for installation
+
+To install a plugin from a specific branch, you simply need to run.
+
+```bash
+coffee install <plugin_name> --branch <branch_name>
+```
+


we should not document every single command possibility otherwise our documentation will be always uncompleted

Right. I can remove this section

but we should came up with a way to document optional parameters

vincenzopalazzo · 2024-03-01T10:47:48Z

Closing in favor of #248

tareknaser added 4 commits February 23, 2024 22:43

feat(lib): define a method for Repository to return git head

e91a987

This function will come in handy when we want to restore the state of a repository on disk after upgrading a plugin Signed-off-by: Tarek <[email protected]>

feat(cmd): print git HEAD of the plugin with coffee list

7e301a2

Since we now may have separate git heads for plugin and repository, we print the git HEAD of the plugin to the user Signed-off-by: Tarek <[email protected]>

docs: add docs for coffee install --branch

d77053b

Signed-off-by: Tarek <[email protected]>

tareknaser requested a review from vincenzopalazzo February 23, 2024 21:01

vincenzopalazzo requested changes Feb 26, 2024

View reviewed changes

vincenzopalazzo mentioned this pull request Feb 26, 2024

git: adding basic function to checkout a branch #248

Draft

vincenzopalazzo closed this Mar 1, 2024

tareknaser deleted the install-branch branch March 7, 2024 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Branch Selection for `coffee_install()` #242

Add Branch Selection for `coffee_install()` #242

tareknaser commented Feb 23, 2024

netlify bot commented Feb 23, 2024 •

edited

Loading

vincenzopalazzo left a comment

vincenzopalazzo Feb 26, 2024

tareknaser Feb 26, 2024

vincenzopalazzo Feb 26, 2024

vincenzopalazzo Feb 26, 2024

tareknaser Feb 26, 2024

vincenzopalazzo Feb 26, 2024

tareknaser Feb 26, 2024

vincenzopalazzo Feb 26, 2024

vincenzopalazzo Feb 26, 2024

tareknaser Feb 26, 2024

vincenzopalazzo Feb 26, 2024

tareknaser Feb 26, 2024

vincenzopalazzo Feb 26, 2024

tareknaser Feb 26, 2024

vincenzopalazzo Feb 26, 2024

vincenzopalazzo Feb 26, 2024

vincenzopalazzo Feb 26, 2024

vincenzopalazzo Feb 26, 2024

vincenzopalazzo Feb 26, 2024

tareknaser Feb 26, 2024

vincenzopalazzo Feb 26, 2024

tareknaser Feb 26, 2024

vincenzopalazzo Feb 26, 2024

tareknaser Feb 26, 2024

vincenzopalazzo Feb 26, 2024

vincenzopalazzo commented Mar 1, 2024

		/// return the git head of the repository.
		fn git_head(&self) -> Option<String>;


		let (local_commit, _) = get_repo_info!(repo);

		let mut cmd = format!("git fetch origin\n");

Add Branch Selection for coffee_install() #242

Add Branch Selection for coffee_install() #242

Conversation

tareknaser commented Feb 23, 2024

Description

Changes

Testing

netlify bot commented Feb 23, 2024 • edited Loading

✅ Deploy Preview for coffee-docs ready!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincenzopalazzo commented Mar 1, 2024

Add Branch Selection for `coffee_install()` #242

Add Branch Selection for `coffee_install()` #242

netlify bot commented Feb 23, 2024 •

edited

Loading