Straight.el: Avoid parsing git's output

[rassie]

What's wrong

straight.el is parsing git's output in some cases which can break in unexpected ways if a non-English locale is used causing git's output to be translated. If I understand the process correctly, there is no guarantee that a particular string can't be changed in a translation so that relying on string must be avoided at all costs. A solution might be as simple as forcing C-locale on each git execution.

Current example of issues it's causing: https://github.com/hlissner/doom-emacs/issues/4378.

Directions to reproduce

I'm sorry, I'm only an indirect user of straight.el via emacs-doom and have no idea how to reproduce anything with pure straight.el. You could try running e.g. git remote show origin with a de_DE.UTF-8 locale to see the difference. An example of the code snippet which I think is dangerous:

https://github.com/raxod502/straight.el/blob/3277e1c9648b41dd5bfb239c067b8374ed2ec2bb/straight.el#L2468-L2472

(dangerous because HEAD branch: is different in every git translation, see e.g. https://github.com/git/git/blob/master/po/de.po#L20379)

Version information

• Emacs version: 28.0.50-native-comp-be907b0
• Operating system: Ubuntu 18.04

[progfolio]

Thank you for bringing this to our attention.
Limited on time at the moment, but I will look into this soon.

[DonHugo69]

I ran into the same issue (but not using doom-emacs) and used el-patch to implement a naive fix. By manipulating the process-environment temporarily and adding LC_ALL=C to it, the output is in the expected language.

(when branch-list
  (let ((remote-show-output
         (let ((process-environment
                (append (list "LC_ALL=C") process-environment)))
           (cdr (straight--call "git" "remote" "show" remote)))))
    (string-match "HEAD branch: \\(.*\\)$" remote-show-output)
    (match-string 1 remote-show-output)))

I could imagine that adding this to straight--call is more reasonable to ensure that all calls are executed with the expected locale.

[progfolio]

It's unfortunate that we have to rely on parsing git's output for this, but as far as I can tell that's the best option that doesn't involve making an API call.
Setting LC_ALL for the commands seems reasonable. We could also modify the regular expression to be language independent (either way, in the end we're relying on the structure of the output of the command).
Something like the following should work independent of the user's locale:

(cl-defun straight-vc-git--default-remote-branch (remote &optional local-repo)
  "Return the default remote branch of LOCAL-REPO, with remote name REMOTE.
If LOCAL-REPO is not specified, assume we are the correct
directory for the repository. If there is no remote repository,
return nil."
  (let* ((default-directory (if local-repo
                                (let ((d (straight--repos-dir local-repo)))
                                  ;; New repositories do not yet
                                  ;; exist, so we don't want to switch
                                  ;; to them.
                                  (if (file-directory-p d) d
                                    default-directory))
                              default-directory))
         (branch-list (cdr (straight--call "git" "branch" "-r"))))
    (if (string-match "^.*origin/HEAD -> origin/\\(.*$\\)" branch-list)
        (match-string 1 branch-list)
      ;; git doesn't always have the default remote branch name
      ;; available locally. For these cases, we have to look at the
      ;; remote. This is more reliable but also involves is slower, so
      ;; we do this later.
      (when branch-list
        (let ((remote-show-output (straight--call "git" "remote" "show" remote)))
          (when (car remote-show-output)
            (replace-regexp-in-string ".*: \\(.*\\)$" "\\1"
                                      (nth 3 (split-string (cdr remote-show-output) "\n")))))))))

@DonHugo69, @rassie: Would you mind testing the above to see if it works with your locales?
You should be able to evaluate that definition and then pass it any repository in straight/repos, e.g.:

(straight-vc-git--default-remote-branch "origin" "straight.el")

[DonHugo69]

@DonHugo69, @rassie: Would you mind testing the above to see if it works with your locales?
You should be able to evaluate that definition and then pass it any repository in straight/repos, e.g.:

(straight-vc-git--default-remote-branch "origin" "straight.el")

@progfolio The fix seems to work on my side, (straight-vc-git--default-remote-branch "origin" "straight.el") evaluates without problems, as well as pulling and freezing forks of some packages.

[rassie]

@progfolio will test next time doom upgrades, thanks!

[progfolio]

Fix is merged into the develop branch.
I'll close this, but feel free to comment/reopen if the problem persists after upgrading to latest develop.

[raxod502]

If we run into this kind of problem again, we could write a simple straight--git function that invokes straight--call with a first argument of "git" and also sets LC_ALL=C in the process-environment, and then use that everywhere we call Git.