Repository: xonsh/xonsh
Pull request: ignore vc aliases in command_cache
I don't *think* this will have any side effects.

When we build the ``$PROMPT``, we lazily check for ``git`` and ``hg``
binaries to see if we should generate a ``<branch>`` field.

At the same time ``lazy_locate_binary`` doesn't return the path to a
command if it detects that that command is an alias (since aliases won't
have a path).

In the case that ``git`` or ``hg`` is aliased (the specific case here
was replacing ``git`` with ``hub``), ``lazy_locate_binary`` will return
``None`` because the command is an alias.

This breaks the logic in the branch detection. We don't want to change
the logic in the branch detection since there are many potential impacts
on prompt generation speed, which makes for a bad user experience.

So, this is a mildly ugly hack, but it's the best option I could come up
with in order to preserve prompt speed and still keep the branch
information available for this slightly weird edge case.

I'm all ears to alternative solutions.

Should resolve #2135 

Problem statement:
Branch not shown if 'git' is aliased to 'hub'
I you do `brew install hub` and add `aliases['git'] = ['hub']` to ~/.xonshrc, git branches will no longer be shown in directories.

This is very strange since Hub is just a wrapper around git, and `hub branch` yields exactly the same output as `git branch`. In fact the former just invokes the latter.

Hints from issue discussion:
Hey @moigagoo -- thanks for reporting. 

Quick question: does the same behavior occur if you set `aliases['git'] = 'hub'`?
hmmm, interesting. I can duplicate this by setting `aliases['git'] = 'git'`
Yes, the same behavior occurs with `aliases['git'] = 'hub'`.
the `curr_branch` function in `$PROMPT_FIELDS` that fetches the branch for the current git repo calls the git binary through python's subprocess module, same for the `gistatus` prompt field.
@laerus invoking `hub branch` with subprocess works just fine:

```python
>>> subprocess.run("git branch", shell=True)
* develop
  master
CompletedProcess(args='git branch', returncode=0)
>>> subprocess.run("hub branch", shell=True)
* develop
  master
CompletedProcess(args='hub branch', returncode=0)
```
Ok, I think I have an idea about what's going on.
Aliasing `git` to something, even itself, is throwing off something in `locate_binary`. It's not finding a binary for `git` since it's an alias and then the branch info isn't returned.
yeah, ok, in `commands_cache.py` line 165

`return path if not is_alias else None`

similar issue if you try, for instance, to do a `locate_binary('ls')`

Commit message: remove special case ugliness

Replace with something better.
No more special casing, now `allcmds` is a tuple of path and also alias
if it exists (otherwise `None`). `locate_binary` and
`lazy_locate_binary` also not take an additional option kwarg to force
returning the path to a binary even if an alias exists

Patch to review:
diff --git a/news/vc_alias_fix.rst b/news/vc_alias_fix.rst
new file mode 100644
index 0000000000..9469b4d785
--- /dev/null
+++ b/news/vc_alias_fix.rst
@@ -0,0 +1,14 @@
+**Added:** None
+
+**Changed:** None
+
+**Deprecated:** None
+
+**Removed:** None
+
+**Fixed:**
+
+* Fixed issue wherein if ``git`` or (presumably) ``hg`` are aliased, then branch
+  information no longer appears in the ``$PROMPT``
+
+**Security:** None
diff --git a/xonsh/commands_cache.py b/xonsh/commands_cache.py
index aa08b59ed6..9af7b8f223 100644
--- a/xonsh/commands_cache.py
+++ b/xonsh/commands_cache.py
@@ -79,7 +79,7 @@ def all_commands(self):
         cache_valid = path_hash == self._path_checksum
         self._path_checksum = path_hash
         # did aliases change?
-        alss = getattr(builtins, 'aliases', set())
+        alss = getattr(builtins, 'aliases', dict())
         al_hash = hash(frozenset(alss))
         cache_valid = cache_valid and al_hash == self._alias_checksum
         self._alias_checksum = al_hash
@@ -99,11 +99,11 @@ def all_commands(self):
             # entries at the back.
             for cmd in executables_in(path):
                 key = cmd.upper() if ON_WINDOWS else cmd
-                allcmds[key] = (os.path.join(path, cmd), cmd in alss)
+                allcmds[key] = (os.path.join(path, cmd), alss.get(key, None))
         for cmd in alss:
             if cmd not in allcmds:
                 key = cmd.upper() if ON_WINDOWS else cmd
-                allcmds[key] = (cmd, True)
+                allcmds[key] = (cmd, None)
         self._cmds_cache = allcmds
         return allcmds
 
@@ -142,14 +142,32 @@ def lazyget(self, key, default=None):
         """A lazy value getter."""
         return self._cmds_cache.get(self.cached_name(key), default)
 
-    def locate_binary(self, name):
-        """Locates an executable on the file system using the cache."""
+    def locate_binary(self, name, ignore_alias=False):
+        """Locates an executable on the file system using the cache.
+
+        Arguments
+        ---------
+        name : str
+                name of binary to search for
+        ignore_alias : bool, optional
+                Force return of binary path even if alias of ``name`` exists
+                (default ``False``)
+        """
         # make sure the cache is up to date by accessing the property
         _ = self.all_commands
-        return self.lazy_locate_binary(name)
-
-    def lazy_locate_binary(self, name):
-        """Locates an executable in the cache, without checking its validity."""
+        return self.lazy_locate_binary(name, ignore_alias)
+
+    def lazy_locate_binary(self, name, ignore_alias=False):
+        """Locates an executable in the cache, without checking its validity.
+
+        Arguments
+        ---------
+        name : str
+                name of binary to search for
+        ignore_alias : bool, optional
+                Force return of binary path even if alias of ``name`` exists
+                (default ``False``)
+        """
         possibilities = self.get_possible_names(name)
         if ON_WINDOWS:
             # Windows users expect to be able to execute files in the same
@@ -161,8 +179,11 @@ def lazy_locate_binary(self, name):
         cached = next((cmd for cmd in possibilities if cmd in self._cmds_cache),
                       None)
         if cached:
-            (path, is_alias) = self._cmds_cache[cached]
-            return path if not is_alias else None
+            (path, alias) = self._cmds_cache[cached]
+            if not alias or ignore_alias:
+                return path
+            else:
+                return None
         elif os.path.isfile(name) and name != pathbasename(name):
             return name
 
diff --git a/xonsh/prompt/vc.py b/xonsh/prompt/vc.py
index 54636547e2..0a62b5cc90 100644
--- a/xonsh/prompt/vc.py
+++ b/xonsh/prompt/vc.py
@@ -123,11 +123,11 @@ def current_branch():
     cmds = builtins.__xonsh_commands_cache__
     # check for binary only once
     if cmds.is_empty():
-        has_git = bool(cmds.locate_binary('git'))
-        has_hg = bool(cmds.locate_binary('hg'))
+        has_git = bool(cmds.locate_binary('git', ignore_alias=True))
+        has_hg = bool(cmds.locate_binary('hg', ignore_alias=True))
     else:
-        has_git = bool(cmds.lazy_locate_binary('git'))
-        has_hg = bool(cmds.lazy_locate_binary('hg'))
+        has_git = bool(cmds.lazy_locate_binary('git', ignore_alias=True))
+        has_hg = bool(cmds.lazy_locate_binary('hg', ignore_alias=True))
     if has_git:
         branch = get_git_branch()
     if not branch and has_hg:


Code review comments:
diff_hunk: @@ -99,11 +99,11 @@ def all_commands(self):
             # entries at the back.
             for cmd in executables_in(path):
                 key = cmd.upper() if ON_WINDOWS else cmd
-                allcmds[key] = (os.path.join(path, cmd), cmd in alss)
+                allcmds[key] = (os.path.join(path, cmd), alss.get(key, None))
         for cmd in alss:
             if cmd not in allcmds:
                 key = cmd.upper() if ON_WINDOWS else cmd
-                allcmds[key] = (cmd, True)
+                allcmds[key] = (cmd, None)
path: xonsh/commands_cache.py
text: @user1:
Why? Would this indicate that this is not an alias? 

@author:
Ahh, that's an interesting point. In this case, there's an alias that doesn't exist in `allcmds` for some reason -- the second value of the tuple is (now) the name of the alias if it exists and the first value is the full string path of the command, so in this case, the "alias" value is the same as the cmd value. I just set it to None, but it could be `(cmd, cmd)`

@user1:
But on line 183 you check: 
```
(path, alias) = self._cmds_cache[cached]
             if not alias or ignore_alias:
...
```
Which would evaluate to True since `alias` would be `None`. 

@author:
Ahh, gotcha. Yes, I agree. I'll change it.