Clarify remote name <-> remote branch

[dritter]

This fixes the original issue from #1209 . The vcs segment now shows <remote_branch>@<remote_name>, if there is a remote. The delimiter is configurable by setting P9K_VCS_REMOTE_DELIMITER.

In the screenshot I have a local branch next2 pointing at next from origin.

I hope that clarifies what the remote name is and what the remote branch is. Performance might got a bit worse, if a remote was detected (one subshell more)..

Additionally, this PR fixes some documentation issues and moves truncating the branch name in a better named hook (git-branch, instead of git-remote-branch).

// cc @Syphdias @romkatv

[Syphdias]

A few observations:

• The first time I tried it, I ended up with master→origin/master@ since I had set some git hooks. I then added git-gather-data, that led to loosing the branch entirely. This will definatly confuse people if the modify (or have modified) their hooks

• The \@-syntax is understandable but is not really git-syntax that can be copied. Just something to consider. Also setting the delimiter will not make that true since it's <branch><remote>.

• I would consider making the test a little bit more solid (one if these asserts actually fails):

  
  diff --git a/segments/vcs/vcs_git.spec b/segments/vcs/vcs_git.spec
  index b963109b..859b5aac 100755
  --- a/segments/vcs/vcs_git.spec
  +++ b/segments/vcs/vcs_git.spec
  @@ -425,11 +425,15 @@ function testAlwaysShowRemoteBranch() {
  
  git clone . ../vcs-test2 1>/dev/null 2>&1
  cd ../vcs-test2

• git remote rename origin some/remote

• assertEquals "%K{002} %F{000} master→master@origin %k%F{002}%f " "$(__p9k_build_left_prompt)"

• assertEquals "%K{002} %F{000} master→master@some/remote %k%F{002}%f " "$(__p9k_build_left_prompt)"

  local P9K_VCS_GIT_ALWAYS_SHOW_REMOTE_BRANCH='false' assertEquals "%K{002} %F{000} master %k%F{002}%f " "$(__p9k_build_left_prompt)"

• git branch -m master new/ref/master

• assertEquals "%K{002} %F{000} new/ref/master→master@some/remote %k%F{002}%f " "$(__p9k_build_left_prompt)" }

  function testGitDirClobber() {

[dritter]

Thanks for the review @Syphdias . Yep, how we merge that configuration array is still unsolved. I'm open to suggestions ;) The obvious one - merge that automatically - won't work, as we cannot know if a user configured that hook away for any reason, or if that hook was added by us..

I deliberately went away from the git syntax, because the git syntax is ambiguous as well (<remote>/<branch>; at least if using git rev-parse), if your branch contains slashes. A whitespace could be another solution. Do you like that better?

Agreed with making the tests more robust. Will do.

[Syphdias]

Thanks for the review @Syphdias . Yep, how we merge that configuration array is still unsolved. I'm open to suggestions ;) The obvious one - merge that automatically - won't work, as we cannot know if a user configured that hook away for any reason, or if that hook was added by us..

Two things:

1. I don't entirely get why you split it into two hooks. I suppose you need git-gather-data because you do something irreversible in a later hook and need the data. Could you make it a normal function that is called in the hook(s) where you need it, so there are no hooks depending on another hook? I'm not sure if there are other "hook dependencies" but I would like to avoid them firstly from a UX perspective and second to avoid github issues (aka work for us :wink:) which will be caused from people using it "wrongly".
2. The @ at the end I described should be easily avoidable by something like this in +vi-git-remotebranch (ugly but functional):

   [[ -n ${remote_name} ]] \
   && hook_com[branch]+="${__P9K_ICONS[VCS_REMOTE_BRANCH]}${remote_branch}${P9K_VCS_REMOTE_DELIMITER}${remote_name}" \
   || hook_com[branch]+="${__P9K_ICONS[VCS_REMOTE_BRANCH]}${remote_branch}"

I deliberately went away from the git syntax, because the git syntax is ambiguous as well (<remote>/<branch>; at least if using git rev-parse), if your branch contains slashes. A whitespace could be another solution. Do you like that better?

Just to be clear: I'm not totally disagreeing with the @. It's just usually used differently in git references. Not sure if whitespace would work.
I also thought of // since you can neither start a branch with / nor end a remote on it; but also non-standard.
Github uses : in PRs which cannot be used in either branch or remote name but is used in pushing afaik.

I would opt for : but I'll leave it up to you.

[dritter]

[...] I suppose you need git-gather-data because you do something irreversible in a later hook and need the data.

Correct. It is possible to truncate long branch names. That is why I saved the original branch name as early as possible.

[...] Could you make it a normal function [...]?

Well. The problem with that approach would be that we either need to cache the value on the first call (and there is no guarantee that the first call already truncated the branch name), or that we have to query git all the time to get the branch name..

Github uses : in PRs which cannot be used in either branch or remote name but is used in pushing afaik.

Fair point. The main problem is that our approach is still not very flexible. We are bound to the initial structure (branch first, remote second information). Sure, we could change that, but not the user (I'd love to have some simple templating for such things). GitLab uses a slash, GitHub a colon, but both write first remote, then branch name. I'll change to colon as well and remote first.

[dritter]

Hmm. I even introduced a new hook git-branch-icon, that shows the icon. With this hook, users can decide, where the branch icon should show up. This could be done as function as well, but then we could not decide, if the icon was already show. This is necessary, to show the icon for remote (git-remote-branch) and local branches (git-branch). Of course, if we do not want to show an branch icon for the remote branch, we could just include the icon in the normal git-branch hook..

[Syphdias]

Friendly reminder, this is still an issue:

Additional info this even happens with P9K_VCS_GIT_ALWAYS_SHOW_REMOTE_BRANCH=false. Maybe this could work?

diff --git a/segments/vcs/vcs.p9k b/segments/vcs/vcs.p9k
index e3581168..3cc48e51 100644
--- a/segments/vcs/vcs.p9k
+++ b/segments/vcs/vcs.p9k
@@ -217,7 +217,7 @@ function +vi-git-remotebranch() {
   local remote=${$(command git rev-parse --verify HEAD@{upstream} --symbolic-full-name 2>/dev/null)/refs\/(remotes|heads)\/}

   # Only show the remote if it differs from the local
-  if [[ -n ${remote} \
+  if [[ -n ${remote} && -n ${__P9K_VCS_DATA[branch]} \
       && ( \
         "${P9K_VCS_GIT_ALWAYS_SHOW_REMOTE_BRANCH}" == 'true' \
         || "${remote#*/}" != "${__P9K_VCS_DATA[branch]#heads/}" \

Also, I think we discussed this before. Is this wanted behaviour, showing the remote if it's not names "origin"? My idea to checking this in the tests:

diff --git a/segments/vcs/vcs_git.spec b/segments/vcs/vcs_git.spec
index 662af683..efc165d6 100755
--- a/segments/vcs/vcs_git.spec
+++ b/segments/vcs/vcs_git.spec
@@ -429,6 +429,7 @@ function testAlwaysShowRemoteBranch() {
   assertEquals "%K{002} %F{000} master %k%F{002}%f " "$(__p9k_build_left_prompt)"

   git remote rename origin some/remote
+  assertEquals "%K{002} %F{000} master %k%F{002}%f " "$(__p9k_build_left_prompt)"
   local P9K_VCS_GIT_ALWAYS_SHOW_REMOTE_BRANCH='true'
   assertEquals "%K{002} %F{000} master→some/remote:master %k%F{002}%f " "$(__p9k_build_left_prompt)"

And my approach to fixing it (this includes the bug fix from above):

diff --git a/segments/vcs/vcs.p9k b/segments/vcs/vcs.p9k
index e3581168..dd544527 100644
--- a/segments/vcs/vcs.p9k
+++ b/segments/vcs/vcs.p9k
@@ -217,12 +217,12 @@ function +vi-git-remotebranch() {
   local remote=${$(command git rev-parse --verify HEAD@{upstream} --symbolic-full-name 2>/dev/null)/refs\/(remotes|heads)\/}

   # Only show the remote if it differs from the local
-  if [[ -n ${remote} \
+  local remote_name=$(command git config branch.${__P9K_VCS_DATA[branch]#heads/}.remote)
+  if [[ -n ${remote} && -n ${__P9K_VCS_DATA[branch]} \
       && ( \
         "${P9K_VCS_GIT_ALWAYS_SHOW_REMOTE_BRANCH}" == 'true' \
-        || "${remote#*/}" != "${__P9K_VCS_DATA[branch]#heads/}" \
+        || "${remote#${remote_name}/}" != "${__P9K_VCS_DATA[branch]#heads/}" \
       ) ]]; then
-    local remote_name=$(command git config branch.${__P9K_VCS_DATA[branch]#heads/}.remote)
     # Remote Branch is the remote branch name, without the remote name.
     # Additionally, remove trailing whitespace and escape percent signs.
     local remote_branch=${${${remote#${remote_name}/}// /}//\%/%%}

Sorry that I'm turning you into the reviewer now :sweat_smile:

I still think this might case some issues with people who do not use the default hooks. We might need to inform users above the change, but I'm not sure if that causes more or less confusion.

PS: I find this exceptionally hard to test with all the possibilities and chances of weird edge cases. I think it is very hard to get this segment perfect. Kudos to you for trying!