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(VCS): display the remote repository name #784

Merged
merged 9 commits into from
Oct 23, 2023
Merged

feat(VCS): display the remote repository name #784

merged 9 commits into from
Oct 23, 2023

Conversation

nojhan
Copy link
Collaborator

@nojhan nojhan commented Jul 26, 2023

  • Only if LP_ENABLE_VCS_REMOTE is enabled (disabled by default, as the function parses a file and the output takes some room).
  • Introduces LP_MARK_VCS_REMOTE.
  • Re-use LP_COLOR_COMMITS and LP_COLOR_COMMITS_BEHIND.

Refs: #782

- Only if LP_ENABLE_VCS_REMOTE is enabled (disabled by default, as the function parses a file and the output takes some room).
- Introduces LP_MARK_VCS_REMOTE.
- Re-use LP_COLOR_COMMITS and LP_COLOR_COMMITS_BEHIND.

Refs: #782
@nojhan nojhan added this to the v2.2 milestone Jul 26, 2023
@nojhan nojhan self-assigned this Jul 26, 2023
@nojhan nojhan requested a review from Rycieos July 26, 2023 13:37
Copy link
Collaborator

@Rycieos Rycieos left a comment

Choose a reason for hiding this comment

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

Basics look good.

docs/config.rst Outdated Show resolved Hide resolved
liquidprompt Show resolved Hide resolved
liquidprompt Outdated Show resolved Hide resolved
liquidprompt Outdated Show resolved Hide resolved
@nojhan
Copy link
Collaborator Author

nojhan commented Oct 7, 2023

There was a bug in the function, and tests had to use a local remote on an existing branch.

@nojhan nojhan requested a review from Rycieos October 7, 2023 17:13
docs/config.rst Outdated Show resolved Hide resolved
docs/config.rst Show resolved Hide resolved
tests/test_git.sh Outdated Show resolved Hide resolved
Co-authored-by: Mark Vander Stel <mvndrstl@gmail.com>
@nojhan
Copy link
Collaborator Author

nojhan commented Oct 10, 2023

test_git does pass on CI know.

@nojhan nojhan requested a review from Rycieos October 10, 2023 18:45
Copy link
Collaborator

@Rycieos Rycieos left a comment

Choose a reason for hiding this comment

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

I can't comment on lines that didn't change, so I'll say it here:

  • You are missing an empty definition for _lp_svn_remote().

This is pretty much done, other than the large issue of the lp_vcs_dir. But I can take care of that, and since the other stuff is pretty small, I can wrap it all up if you like.

docs/config.rst Outdated Show resolved Hide resolved
liquidprompt Outdated Show resolved Hide resolved
liquidprompt Outdated Show resolved Hide resolved
liquidprompt Show resolved Hide resolved
liquidprompt Show resolved Hide resolved
Git SVN detection and the new remote branch detection were broken when
in a worktree. Worktrees have two Git dirs: one is the main dir, which
holds the DB and config. The other is the worktree specific dir, which
holds directory specific information, like index and status like merge
or rebase in progress.
@Rycieos
Copy link
Collaborator

Rycieos commented Oct 14, 2023

@nojhan I pushed two commits to this PR to fix the issues I saw. If you could just take a quick look I can merge this.

@nojhan
Copy link
Collaborator Author

nojhan commented Oct 15, 2023

PR looks good (I'm trying hard to find something you forgot, like I always do, but I fail miserably).

On the design side, I am now thinking that the remote name display should only occur when there are commits to be pushed or pulled, and not displayed (even with LP_ENABLE_VCS_REMOTE=1) when there is no commit. This would be more consistent with LP's general design of displaying information only when needed.

This would probably only require the last control flow section of _lp_vcs_details_color to be edited by removing the else subsections.

What do you think of the idea?

@nojhan
Copy link
Collaborator Author

nojhan commented Oct 15, 2023

This would look like:

    if _lp_vcs_remote; then
        # Commits behind but not ahead.
        if   [[ "$lp_vcs_commit_behind" -ne "0" && "$lp_vcs_commit_ahead" -eq "0"  ]]; then
            lp_vcs_details_color+="${LP_COLOR_COMMITS_BEHIND}${LP_MARK_VCS_REMOTE}${lp_vcs_remote}${NO_COL}"

        # Commits ahead but not behind.
        elif [[ "$lp_vcs_commit_behind" -eq "0" && "$lp_vcs_commit_ahead" -ne "0" ]]; then
            lp_vcs_details_color+="${LP_COLOR_COMMITS}${LP_MARK_VCS_REMOTE}${lp_vcs_remote}${NO_COL}"

        # Commits both ahead and behind: the mark is colored differently.
        elif [[ "$lp_vcs_commit_behind" -ne "0" && "$lp_vcs_commit_ahead" -ne "0" ]]; then
            lp_vcs_details_color+="${LP_COLOR_COMMITS_BEHIND}${LP_MARK_VCS_REMOTE}${LP_COLOR_COMMITS}${lp_vcs_remote}${NO_COL}"

        # else: display nothing.
        fi
    fi

@Rycieos
Copy link
Collaborator

Rycieos commented Oct 15, 2023

Yeah I like that much more actually. I added bb32831 with that change.

@nojhan
Copy link
Collaborator Author

nojhan commented Oct 15, 2023

The related doc may be outdated after this change.

@Rycieos
Copy link
Collaborator

Rycieos commented Oct 16, 2023

Good call, I amended commit 98e861e with clarified docs.

@Rycieos Rycieos merged commit 043b205 into master Oct 23, 2023
10 checks passed
@Rycieos Rycieos deleted the feat/remote branch October 23, 2023 18:28
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.

None yet

2 participants