unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#68183: 28.3; vc-dir fails when I have a certain branch checked out
@ 2023-12-31 18:59 Tom Tromey
  2023-12-31 19:34 ` Eli Zaretskii
  0 siblings, 1 reply; 12+ messages in thread
From: Tom Tromey @ 2023-12-31 18:59 UTC (permalink / raw)
  To: 68183


I use vc-dir frequently.

When I check out one particular branch of my gdb repository, vc-dir
fails with this error:

vc-do-command: Failed (status 2): git --no-pager remote get-url . .

The only thing that might be peculiar about this branch is that it uses
another local branch as its tracking branch.

If I check out other branches in this repository -- ones that track
remote branches -- vc-dir works again.



In GNU Emacs 28.3 (build 1, x86_64-redhat-linux-gnu, GTK+ Version 3.24.38, cairo version 1.17.8)
 of 2023-09-23 built on fd97b702fbea4aa3b70f5e90b3f3f165
Windowing system distributor 'The X.Org Foundation', version 11.0.12201009
System Description: Fedora Linux 38 (Workstation Edition)

Configured using:
 'configure --build=x86_64-redhat-linux-gnu
 --host=x86_64-redhat-linux-gnu --program-prefix=
 --disable-dependency-tracking --prefix=/usr --exec-prefix=/usr
 --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc
 --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64
 --libexecdir=/usr/libexec --localstatedir=/var --runstatedir=/run
 --sharedstatedir=/var/lib --mandir=/usr/share/man
 --infodir=/usr/share/info --with-dbus --with-gif --with-jpeg --with-png
 --with-rsvg --with-tiff --with-xpm --with-x-toolkit=gtk3 --with-gpm=no
 --with-xwidgets --with-modules --with-harfbuzz --with-cairo --with-json
 --with-native-compilation build_alias=x86_64-redhat-linux-gnu
 host_alias=x86_64-redhat-linux-gnu CC=gcc 'CFLAGS=-DMAIL_USE_LOCKF -O2
 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches
 -pipe -Wall -Werror=format-security
 -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS
 -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong
 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic
 -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection
 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer '
 LDFLAGS=-Wl,-z,relro
 PKG_CONFIG_PATH=:/usr/lib64/pkgconfig:/usr/share/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NATIVE_COMP NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND THREADS TIFF TOOLKIT_SCROLL_BARS
X11 XDBE XIM XPM XWIDGETS GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Compilation

Minor modes in effect:
  shell-dirtrack-mode: t
  which-function-mode: t
  erc-services-mode: t
  erc-networks-mode: t
  savehist-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  buffer-read-only: t
  column-number-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t

Load-path shadows:
/home/tromey/.emacs.d/elpa/bubbles-0.5/bubbles hides /usr/share/emacs/28.3/lisp/play/bubbles
/home/tromey/.emacs.d/elpa/dictionary-1.10/dictionary hides /usr/share/emacs/28.3/lisp/net/dictionary

Features:
(shadow emacsbug markdown-mode tcl m4-mode gud novice pcmpl-unix
pcmpl-gnu two-column url-http url-gw url-auth sh-script smie executable
dired-aux rng-xsd xsd-regexp rng-cmpct rng-nxml rng-valid rng-loc
rng-uri rng-parse nxml-parse rng-match rng-dt rng-util rng-pttrn nxml-ns
nxml-mode nxml-outln nxml-rap sgml-mode facemenu nxml-util nxml-enc
xmltok autoconf autoconf-mode gnus-html help-fns radix-tree url-cache
org-bullets org-element avl-tree ol-eww eww xdg url-queue mm-url
ol-rmail ol-mhe ol-irc ol-info ol-gnus nnselect gnus-search eieio-opt
speedbar ezimage dframe ol-docview doc-view image-mode exif ol-bibtex
ol-bbdb ol-w3m ol-doi org-link-doi org ob ob-tangle ob-ref ob-lob
ob-table ob-exp org-macro org-footnote org-src ob-comint org-pcomplete
org-list org-faces org-entities noutline outline org-version
ob-emacs-lisp ob-core ob-eval org-table oc-basic bibtex ol org-keys oc
org-compat org-macs org-loaddefs find-func flow-fill python tramp-sh
term/xterm xterm face-remap goto-addr log-edit vc-annotate mule-util
jka-compr find-dired texinfo texinfo-loaddefs find-file make-mode
log-view pcvs-util vc-mtn vc-hg vc-bzr vc-src vc-sccs vc-svn vc-cvs
vc-rcs copyright pulse ffap scheme mailalias dabbrev supercite regi
mail-hist ggtags hippie-exp etags fileloop generator xref project
bug-reference vc-git cc-mode cc-fonts cc-guess cc-menus cc-cmds
smerge-mode diff diff-mode shr-color mm-archive sort smiley gnus-cite
mail-extr qp gnus-async gnus-bcklg gnus-ml disp-table misearch
multi-isearch gnus-topic nndraft nnmh nnfolder utf-7 gnutls
network-stream nsm gnus-agent gnus-srvr gnus-score score-mode nnvirtual
gnus-msg gnus-art mm-uu mml2015 mm-view mml-smime smime dig nntp
gnus-cache gnus-sum shr kinsoku svg dom gnus-group gnus-undo smtpmail
sendmail gnus-start gnus-dbus dbus gnus-cloud nnimap nnmail mail-source
utf7 netrc nnoo gnus-spec gnus-int gnus-range message rmc puny dired
dired-loaddefs rfc822 mml mml-sec epa derived epg rfc6068 epg-config
mm-decode mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils
mailheader gnus-win gnus nnheader gnus-util rmail rmail-loaddefs rfc2047
rfc2045 ietf-drums mail-utils mm-util mail-prsvr add-log flyspell ispell
diminish projectile ibuf-macs pcase edmacro kmacro grep compile
text-property-search ibuf-ext ibuffer ibuffer-loaddefs dash appt
diary-lib diary-loaddefs cal-menu calendar cal-loaddefs tramp
tramp-loaddefs trampver tramp-integration files-x tramp-compat shell
pcomplete parse-time ls-lisp which-func imenu minimap autorevert
filenotify cus-load erc-track erc-match erc-services erc-networks
erc-hl-nicks easy-mmode color erc-button erc-fill erc-stamp wid-edit
erc-goodies erc erc-backend iso8601 time-date thingatpt pp format-spec
erc-loaddefs comp comp-cstr rx cl-extra help-mode warnings advice vc-dir
ewoc vc vc-dispatcher cc-styles cc-align cc-engine cc-vars cc-defs
ange-ftp comint ansi-color ring server savehist clang-rename
clang-include-fixer let-alist clang-format xml finder-inf
gdb-shell-autoloads lisppaste-autoloads pydoc-info-autoloads info-look
info cl weblogger-autoloads package browse-url url url-proxy url-privacy
url-expand url-methods url-history url-cookie url-domsuf url-util
mailcap url-handlers url-parse auth-source cl-seq eieio eieio-core
cl-macs eieio-loaddefs password-cache json subr-x map url-vars seq
byte-opt gv bytecomp byte-compile cconv cl-loaddefs cl-lib iso-transl
tooltip eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel term/x-win x-win term/common-win x-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer cl-generic
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice button loaddefs faces cus-face macroexp files
window text-properties overlay sha1 md5 base64 format env code-pages
mule custom widget hashtable-print-readable backquote threads
xwidget-internal dbusbind inotify dynamic-setting system-font-setting
font-render-setting cairo move-toolbar gtk x-toolkit x multi-tty
make-network-process native-compile emacs)

Memory information:
((conses 16 2511390 280777)
 (symbols 48 68379 41)
 (strings 32 200925 47137)
 (string-bytes 1 9410331)
 (vectors 16 109364)
 (vector-slots 8 3054321 332806)
 (floats 8 532 632)
 (intervals 56 469509 4701)
 (buffers 992 1022))





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#68183: 28.3; vc-dir fails when I have a certain branch checked out
  2023-12-31 18:59 bug#68183: 28.3; vc-dir fails when I have a certain branch checked out Tom Tromey
@ 2023-12-31 19:34 ` Eli Zaretskii
  2023-12-31 20:14   ` Tom Tromey
  0 siblings, 1 reply; 12+ messages in thread
From: Eli Zaretskii @ 2023-12-31 19:34 UTC (permalink / raw)
  To: Tom Tromey; +Cc: 68183

> From: Tom Tromey <tom@tromey.com>
> Date: Sun, 31 Dec 2023 11:59:11 -0700
> 
> 
> I use vc-dir frequently.
> 
> When I check out one particular branch of my gdb repository, vc-dir
> fails with this error:
> 
> vc-do-command: Failed (status 2): git --no-pager remote get-url . .
> 
> The only thing that might be peculiar about this branch is that it uses
> another local branch as its tracking branch.
> 
> If I check out other branches in this repository -- ones that track
> remote branches -- vc-dir works again.

Thanks, but I think we'd appreciate a reproducible recipe for this:
how can one create a Git repository which can be used to reproduce
this issue?





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#68183: 28.3; vc-dir fails when I have a certain branch checked out
  2023-12-31 19:34 ` Eli Zaretskii
@ 2023-12-31 20:14   ` Tom Tromey
  2024-01-03  9:46     ` Kévin Le Gouguec
  0 siblings, 1 reply; 12+ messages in thread
From: Tom Tromey @ 2023-12-31 20:14 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Tom Tromey, 68183

Eli> Thanks, but I think we'd appreciate a reproducible recipe for this:
Eli> how can one create a Git repository which can be used to reproduce
Eli> this issue?

This worked for me:

$ cd ~/Emacs/trunk
# This is my Emacs git repository
$ git checkout --track -b vc-dir-bug master
branch 'vc-dir-bug' set up to track 'master'.
Switched to a new branch 'vc-dir-bug'


Now invoke vc-dir on that directory.

Tom





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#68183: 28.3; vc-dir fails when I have a certain branch checked out
  2023-12-31 20:14   ` Tom Tromey
@ 2024-01-03  9:46     ` Kévin Le Gouguec
  2024-02-12  8:08       ` Kévin Le Gouguec
  0 siblings, 1 reply; 12+ messages in thread
From: Kévin Le Gouguec @ 2024-01-03  9:46 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Eli Zaretskii, 68183

Tom Tromey <tom@tromey.com> writes:

> Eli> Thanks, but I think we'd appreciate a reproducible recipe for this:
> Eli> how can one create a Git repository which can be used to reproduce
> Eli> this issue?
>
> This worked for me:
>
> $ cd ~/Emacs/trunk
> # This is my Emacs git repository
> $ git checkout --track -b vc-dir-bug master
> branch 'vc-dir-bug' set up to track 'master'.
> Switched to a new branch 'vc-dir-bug'
>
>
> Now invoke vc-dir on that directory.

I can reproduce; IIUC the salient point is setting start-point to a
local revision when calling git checkout, by opposition to
e.g. origin/master.  Continuing off of your recipe:

$ git branch --set-upstream-to origin/master

Then M-x vc-dir works again.

IIUC, to display

  Remote     : https://git.savannah.gnu.org/git/emacs.git

vc-git-dir-extra-headers runs

1. git config branch.vc-dir-bug.remote ⇒ "."
2. (vc-git-repository-url "[… EMACS DIR …]" ".")
    1. git config remote...url ⇒ error

git-config(1) says that branch.<name>.remote is "." when <name> is
tracking a local branch, whereas branch.<name>.merge points to the local
branch 'git pull' will resync with.  Wonder what TRT would be for the
purposes of vc-dir?

(1) Drop the "Remote" header: the current branch is not sync'd with a
remote branch, after all.

(2) Print "Remote: https://git.savannah.gnu.org/git/emacs.git" by making
vc-git-repository-url fall back to remote.origin.url when remote-name is
".".

(3) Print "Remote: master" by making vc-git-dir-extra-headers fall back
to branch.<name>.merge when .remote is ".".

(4) Make vc-git-dir-extra-headers fall back to
branch.<branch.<name>.merge>.remote.  In our example, that would yield
"Remote: https://git.savannah.gnu.org/git/emacs.git", but in general
that seems unreliable, since that remote could be "." as well, and
nothing prevents cycles AFAIU.

IMO (3) would be the most robust, though maybe confusing (calling a
local branch "remote"); (2) makes sense as well since
vc-git-repository-url already falls back to remote.origin.url when
remote-name is nil.

(1) sounds trivially "robust" and "not too incorrect", but maybe not the
most helpful.  (4) is under-specified and I'm not convinced it is
possible to make it generally useful.


Hope I've not mis-diagnosed the problem; apologies for the noise if so.





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#68183: 28.3; vc-dir fails when I have a certain branch checked out
  2024-01-03  9:46     ` Kévin Le Gouguec
@ 2024-02-12  8:08       ` Kévin Le Gouguec
  2024-02-14 19:56         ` Kévin Le Gouguec
  0 siblings, 1 reply; 12+ messages in thread
From: Kévin Le Gouguec @ 2024-02-12  8:08 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Dmitry Gutov, Eli Zaretskii, 68183, Juri Linkov

[-- Attachment #1: Type: text/plain, Size: 2044 bytes --]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> Tom Tromey <tom@tromey.com> writes:
>
>> Eli> Thanks, but I think we'd appreciate a reproducible recipe for this:
>> Eli> how can one create a Git repository which can be used to reproduce
>> Eli> this issue?
>>
>> This worked for me:
>>
>> $ cd ~/Emacs/trunk
>> # This is my Emacs git repository
>> $ git checkout --track -b vc-dir-bug master
>> branch 'vc-dir-bug' set up to track 'master'.
>> Switched to a new branch 'vc-dir-bug'
>>
>>
>> Now invoke vc-dir on that directory.
>

[…]

> git-config(1) says that branch.<name>.remote is "." when <name> is
> tracking a local branch, whereas branch.<name>.merge points to the local
> branch 'git pull' will resync with.  Wonder what TRT would be for the
> purposes of vc-dir?

Here's a patch.  tl;dr

(1) When branch.<name>.remote is ".", display…

    > Remote    : none (tracking local branch)

    … instead of raising an error.

(2) When branch.<name>.merge is set to BRANCH, display…

    > Tracking  : BRANCH

(3) Add tests, because why not.

CC'ing Dmitry and Juri, whom my brain associates with vc-git maintenance
(rightly or wrongly; apologies for the noise if the latter).

I might be slow to respond for the next couple of weeks, so please feel
free to adapt or dismiss the patch as convenient.  FWIW, off the top of
my head,

* the test should probably have a (skip-unless (have-git-or-something)),
* maybe "none (tracking local branch)" is not informative and we should
  ditch it,
  * maybe we should fall back to "origin", like vc-git-repository-url
    does,
* rushed the ChangeLog entry; vc-git-test--run should also be declared
  as a "new helper" (and maybe I should spell out that I used it to not
  have to depend on vc-git-- internal functions),
* maybe the new header deserves a NEWS entry.

I can tackle any of the above (and any other feedback), though again, if
someone has a vision and I don't answer in a timely manner, don't wait
on me.


[-- Attachment #2: 0001-Fix-vc-dir-when-remote-Git-branch-is-local.patch --]
[-- Type: text/x-patch, Size: 6680 bytes --]

From ba9b1f6f340a551acab0014c69f05f38191bd345 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Mon, 12 Feb 2024 08:29:19 +0100
Subject: [PATCH] Fix vc-dir when "remote" Git branch is local
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

While in there, add that "tracking" branch to the vc-dir buffer.
For bug#68183.

* lisp/vc/vc-git.el (vc-git-dir-extra-headers): Reduce
boilerplate with new function 'vc-git--out-ok'; skip calling
vc-git-repository-url when REMOTE is "."; display tracking
branch; prefer "none (<details…>)" to "not (<details…>)" since
that reads more grammatically correct.
(vc-git--out-ok): Add documentation.
(vc-git--out-str): New function to easily get the output from a
Git command.
* test/lisp/vc/vc-git-tests.el (vc-git-test--with-repo): New
helper.
(vc-git-test--run, vc-git-test-dir-track-local-branch): Check
that vc-dir does not crash.
---
 lisp/vc/vc-git.el            | 44 ++++++++++++++++++++++++------------
 test/lisp/vc/vc-git-tests.el | 39 ++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 14 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 456417e566e..bf3032e5537 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -817,27 +817,29 @@ vc-git--cmds-in-progress
     cmds))
 
 (defun vc-git-dir-extra-headers (dir)
-  (let ((str (with-output-to-string
-               (with-current-buffer standard-output
-                 (vc-git--out-ok "symbolic-ref" "HEAD"))))
+  (let ((str (vc-git--out-str "symbolic-ref" "HEAD"))
 	(stash-list (vc-git-stash-list))
         (default-directory dir)
         (in-progress (vc-git--cmds-in-progress))
 
-	branch remote remote-url stash-button stash-string)
+	branch remote-url stash-button stash-string upstream-branch)
     (if (string-match "^\\(refs/heads/\\)?\\(.+\\)$" str)
 	(progn
 	  (setq branch (match-string 2 str))
-	  (setq remote
-		(with-output-to-string
-		  (with-current-buffer standard-output
-		    (vc-git--out-ok "config"
-                                    (concat "branch." branch ".remote")))))
-	  (when (string-match "\\([^\n]+\\)" remote)
-	    (setq remote (match-string 1 remote)))
-          (when (> (length remote) 0)
-	    (setq remote-url (vc-git-repository-url dir remote))))
-      (setq branch "not (detached HEAD)"))
+          (let ((remote (vc-git--out-str
+                         "config" (concat "branch." branch ".remote")))
+                (merge (vc-git--out-str
+                        "config" (concat "branch." branch ".merge"))))
+            (when (string-match "\\([^\n]+\\)" remote)
+	      (setq remote (match-string 1 remote)))
+            (pcase remote
+              ("."
+               (setq remote-url "none (tracking local branch)"))
+              ((pred (not string-empty-p))
+               (setq remote-url (vc-git-repository-url dir remote))))
+            (when (string-match "^\\(refs/heads/\\)?\\(.+\\)$" merge)
+              (setq upstream-branch (match-string 2 merge)))))
+      (setq branch "none (detached HEAD)"))
     (when stash-list
       (let* ((len (length stash-list))
              (limit
@@ -896,6 +898,11 @@ vc-git-dir-extra-headers
 	(propertize "Remote     : " 'face 'vc-dir-header)
 	(propertize remote-url
 		    'face 'vc-dir-header-value)))
+     (when upstream-branch
+       (concat
+        "\n"
+        (propertize "Tracking   : " 'face 'vc-dir-header)
+        (propertize upstream-branch 'face 'vc-dir-header-value)))
      ;; For now just a heading, key bindings can be added later for various bisect actions
      (when (memq 'bisect in-progress)
        (propertize  "\nBisect     : in progress" 'face 'vc-dir-status-warning))
@@ -2219,8 +2226,17 @@ vc-git--call
     (apply #'process-file vc-git-program nil buffer nil "--no-pager" command args)))
 
 (defun vc-git--out-ok (command &rest args)
+  "Run `git COMMAND ARGS...' and insert standard output in current buffer.
+Return whether the process exited with status zero."
   (zerop (apply #'vc-git--call '(t nil) command args)))
 
+(defun vc-git--out-str (command &rest args)
+  "Run `git COMMAND ARGS...' and return standard output.
+The exit status is ignored."
+  (with-output-to-string
+    (with-current-buffer standard-output
+      (apply #'vc-git--out-ok command args))))
+
 (defun vc-git--run-command-string (file &rest args)
   "Run a git command on FILE and return its output as string.
 FILE can be nil."
diff --git a/test/lisp/vc/vc-git-tests.el b/test/lisp/vc/vc-git-tests.el
index c52cd9c5875..1f8abe7b9b0 100644
--- a/test/lisp/vc/vc-git-tests.el
+++ b/test/lisp/vc/vc-git-tests.el
@@ -24,6 +24,8 @@
 
 ;;; Code:
 
+(require 'ert-x)
+(require 'vc)
 (require 'vc-git)
 
 (ert-deftest vc-git-test-program-version-general ()
@@ -81,4 +83,41 @@ vc-git-test-annotate-time
     (should-not (vc-git-annotate-time))
     (should-not (vc-git-annotate-time))))
 
+(defmacro vc-git-test--with-repo (name &rest body)
+  "Initialize a repository in a temporary directory and evaluate BODY.
+
+The current directory will be set to the top of that repository; NAME
+will be bound to that directory's file name.  Once BODY exits, the
+directory will be deleted."
+  (declare (indent 1))
+  `(ert-with-temp-directory ,name
+     (let ((default-directory ,name))
+       (vc-create-repo 'Git)
+       ,@body)))
+
+(defun vc-git-test--run (&rest args)
+  "Run git ARGS…, check for non-zero status, and return output."
+  (with-temp-buffer
+    (apply 'vc-git-command t 0 nil args)
+    (buffer-string)))
+
+(ert-deftest vc-git-test-dir-track-local-branch ()
+  "Test that `vc-dir' works when tracking local branches.  See bug#68183."
+  (vc-git-test--with-repo repo
+    ;; Create an initial commit to get a branch started.
+    (write-region "hello" nil "README")
+    (vc-git-test--run "add" "README")
+    (vc-git-test--run "commit" "-mFirst")
+    ;; Get current branch name lazily, to remain agnostic of
+    ;; init.defaultbranch.
+    (let ((upstream-branch
+           (string-trim (vc-git-test--run "branch" "--show-current"))))
+      (vc-git-test--run "checkout" "--track" "-b" "hack" upstream-branch)
+      (vc-dir default-directory)
+      (pcase-dolist (`(,header ,value)
+                     `(("Branch" "hack")
+                       ("Tracking" ,upstream-branch)))
+        (goto-char (point-min))
+        (re-search-forward (format "^%s *: %s$" header value))))))
+
 ;;; vc-git-tests.el ends here
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#68183: 28.3; vc-dir fails when I have a certain branch checked out
  2024-02-12  8:08       ` Kévin Le Gouguec
@ 2024-02-14 19:56         ` Kévin Le Gouguec
  2024-03-13 20:03           ` Kévin Le Gouguec
  2024-03-15  2:57           ` Dmitry Gutov
  0 siblings, 2 replies; 12+ messages in thread
From: Kévin Le Gouguec @ 2024-02-14 19:56 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Dmitry Gutov, Eli Zaretskii, 68183, Juri Linkov

[-- Attachment #1: Type: text/plain, Size: 2196 bytes --]

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:
>
>> Tom Tromey <tom@tromey.com> writes:
>>
>>> Eli> Thanks, but I think we'd appreciate a reproducible recipe for this:
>>> Eli> how can one create a Git repository which can be used to reproduce
>>> Eli> this issue?
>>>
>>> This worked for me:
>>>
>>> $ cd ~/Emacs/trunk
>>> # This is my Emacs git repository
>>> $ git checkout --track -b vc-dir-bug master
>>> branch 'vc-dir-bug' set up to track 'master'.
>>> Switched to a new branch 'vc-dir-bug'
>>>
>>>
>>> Now invoke vc-dir on that directory.
>>

[…]

> Here's a patch.  […]

And here's another revision, addressing most of the points below.
WDY'allT?

> * the test should probably have a (skip-unless (have-git-or-something)),

Done.

> * maybe "none (tracking local branch)" is not informative and we should
>   ditch it,
>   * maybe we should fall back to "origin", like vc-git-repository-url
>     does,

FWIW, the current patch will show

    Branch     : vc-dir-tracking-branch
    Tracking   : origin/master
    Remote     : https://git.savannah.gnu.org/git/emacs.git

for my checkout of this work-in-progress patch, and

    Branch     : vc-dir-bug
    Tracking   : master
    Remote     : none (tracking local branch)

for a checkout made following Tom's recipe, and

    Branch     : trunk

for a fresh 'git init' with just a default branch.

OT1H "none (tracking local branch)" is redundant with "Tracking" not
being prefixed with "origin"; OTOH

* stripping "Remote" altogether might confuse users - at least "tracking
  local branch" hints at what's going on,

* Falling back to origin's URL might also cause confusion: users might
  then expect 'vc-pull' to fetch changes from that URL, which is not the
  case.

So all in all I think the above is reasonably useful.

> * rushed the ChangeLog entry; vc-git-test--run should also be declared
>   as a "new helper" (and maybe I should spell out that I used it to not
>   have to depend on vc-git-- internal functions),

Done.

> * maybe the new header deserves a NEWS entry.

Maybe?


[-- Attachment #2: 0001-Fix-vc-dir-when-remote-Git-branch-is-local.patch --]
[-- Type: text/x-diff, Size: 6732 bytes --]

From ccca29fe2cce9b8c3477fcd31c8b37b1d8a57e94 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?K=C3=A9vin=20Le=20Gouguec?= <kevin.legouguec@gmail.com>
Date: Mon, 12 Feb 2024 08:29:19 +0100
Subject: [PATCH] Fix vc-dir when "remote" Git branch is local
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

While in there, add that "tracking" branch to the vc-dir buffer.
For bug#68183.

* lisp/vc/vc-git.el (vc-git-dir-extra-headers): Reduce
boilerplate with new function 'vc-git--out-ok'; stop calling
vc-git-repository-url when REMOTE is "." to avoid throwing an
error; display tracking branch; prefer "none (<details…>)" to
"not (<details…>)" since that reads more grammatically correct.
(vc-git--out-ok): Add documentation.
(vc-git--out-str): New function to easily get the output from a
Git command.
* test/lisp/vc/vc-git-tests.el (vc-git-test--with-repo)
(vc-git-test--run): New helpers, defined to steer clear of vc-git--
internal functions.
(vc-git-test-dir-track-local-branch): Check that vc-dir does not
crash.
---
 lisp/vc/vc-git.el            | 46 +++++++++++++++++++++++++-----------
 test/lisp/vc/vc-git-tests.el | 40 +++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 14 deletions(-)

diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 456417e566e..b1d3dd533c6 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -817,27 +817,31 @@ vc-git--cmds-in-progress
     cmds))
 
 (defun vc-git-dir-extra-headers (dir)
-  (let ((str (with-output-to-string
-               (with-current-buffer standard-output
-                 (vc-git--out-ok "symbolic-ref" "HEAD"))))
+  (let ((str (vc-git--out-str "symbolic-ref" "HEAD"))
 	(stash-list (vc-git-stash-list))
         (default-directory dir)
         (in-progress (vc-git--cmds-in-progress))
 
-	branch remote remote-url stash-button stash-string)
+	branch remote-url stash-button stash-string tracking-branch)
     (if (string-match "^\\(refs/heads/\\)?\\(.+\\)$" str)
 	(progn
 	  (setq branch (match-string 2 str))
-	  (setq remote
-		(with-output-to-string
-		  (with-current-buffer standard-output
-		    (vc-git--out-ok "config"
-                                    (concat "branch." branch ".remote")))))
-	  (when (string-match "\\([^\n]+\\)" remote)
-	    (setq remote (match-string 1 remote)))
-          (when (> (length remote) 0)
-	    (setq remote-url (vc-git-repository-url dir remote))))
-      (setq branch "not (detached HEAD)"))
+          (let ((remote (vc-git--out-str
+                         "config" (concat "branch." branch ".remote")))
+                (merge (vc-git--out-str
+                        "config" (concat "branch." branch ".merge"))))
+            (when (string-match "\\([^\n]+\\)" remote)
+	      (setq remote (match-string 1 remote)))
+            (when (string-match "^\\(refs/heads/\\)?\\(.+\\)$" merge)
+              (setq tracking-branch (match-string 2 merge)))
+            (pcase remote
+              ("."
+               (setq remote-url "none (tracking local branch)"))
+              ((pred (not string-empty-p))
+               (setq
+                remote-url (vc-git-repository-url dir remote)
+                tracking-branch (concat remote "/" tracking-branch))))))
+      (setq branch "none (detached HEAD)"))
     (when stash-list
       (let* ((len (length stash-list))
              (limit
@@ -890,6 +894,11 @@ vc-git-dir-extra-headers
      (propertize "Branch     : " 'face 'vc-dir-header)
      (propertize branch
 		 'face 'vc-dir-header-value)
+     (when tracking-branch
+       (concat
+        "\n"
+        (propertize "Tracking   : " 'face 'vc-dir-header)
+        (propertize tracking-branch 'face 'vc-dir-header-value)))
      (when remote-url
        (concat
 	"\n"
@@ -2219,8 +2228,17 @@ vc-git--call
     (apply #'process-file vc-git-program nil buffer nil "--no-pager" command args)))
 
 (defun vc-git--out-ok (command &rest args)
+  "Run `git COMMAND ARGS...' and insert standard output in current buffer.
+Return whether the process exited with status zero."
   (zerop (apply #'vc-git--call '(t nil) command args)))
 
+(defun vc-git--out-str (command &rest args)
+  "Run `git COMMAND ARGS...' and return standard output.
+The exit status is ignored."
+  (with-output-to-string
+    (with-current-buffer standard-output
+      (apply #'vc-git--out-ok command args))))
+
 (defun vc-git--run-command-string (file &rest args)
   "Run a git command on FILE and return its output as string.
 FILE can be nil."
diff --git a/test/lisp/vc/vc-git-tests.el b/test/lisp/vc/vc-git-tests.el
index c52cd9c5875..fd3e8ccd602 100644
--- a/test/lisp/vc/vc-git-tests.el
+++ b/test/lisp/vc/vc-git-tests.el
@@ -24,6 +24,8 @@
 
 ;;; Code:
 
+(require 'ert-x)
+(require 'vc)
 (require 'vc-git)
 
 (ert-deftest vc-git-test-program-version-general ()
@@ -81,4 +83,42 @@ vc-git-test-annotate-time
     (should-not (vc-git-annotate-time))
     (should-not (vc-git-annotate-time))))
 
+(defmacro vc-git-test--with-repo (name &rest body)
+  "Initialize a repository in a temporary directory and evaluate BODY.
+
+The current directory will be set to the top of that repository; NAME
+will be bound to that directory's file name.  Once BODY exits, the
+directory will be deleted."
+  (declare (indent 1))
+  `(ert-with-temp-directory ,name
+     (let ((default-directory ,name))
+       (vc-create-repo 'Git)
+       ,@body)))
+
+(defun vc-git-test--run (&rest args)
+  "Run git ARGS…, check for non-zero status, and return output."
+  (with-temp-buffer
+    (apply 'vc-git-command t 0 nil args)
+    (buffer-string)))
+
+(ert-deftest vc-git-test-dir-track-local-branch ()
+  "Test that `vc-dir' works when tracking local branches.  Bug#68183."
+  (skip-unless (executable-find vc-git-program))
+  (vc-git-test--with-repo repo
+    ;; Create an initial commit to get a branch started.
+    (write-region "hello" nil "README")
+    (vc-git-test--run "add" "README")
+    (vc-git-test--run "commit" "-mFirst")
+    ;; Get current branch name lazily, to remain agnostic of
+    ;; init.defaultbranch.
+    (let ((upstream-branch
+           (string-trim (vc-git-test--run "branch" "--show-current"))))
+      (vc-git-test--run "checkout" "--track" "-b" "hack" upstream-branch)
+      (vc-dir default-directory)
+      (pcase-dolist (`(,header ,value)
+                     `(("Branch" "hack")
+                       ("Tracking" ,upstream-branch)))
+        (goto-char (point-min))
+        (re-search-forward (format "^%s *: %s$" header value))))))
+
 ;;; vc-git-tests.el ends here
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* bug#68183: 28.3; vc-dir fails when I have a certain branch checked out
  2024-02-14 19:56         ` Kévin Le Gouguec
@ 2024-03-13 20:03           ` Kévin Le Gouguec
  2024-03-15  2:57           ` Dmitry Gutov
  1 sibling, 0 replies; 12+ messages in thread
From: Kévin Le Gouguec @ 2024-03-13 20:03 UTC (permalink / raw)
  To: Tom Tromey; +Cc: Dmitry Gutov, Eli Zaretskii, 68183, Juri Linkov

Hello folks,

Kévin Le Gouguec <kevin.legouguec@gmail.com> writes:

> And here's another revision, addressing most of the points below.
> WDY'allT?
>

[…]

>> * maybe "none (tracking local branch)" is not informative and we should
>>   ditch it,
>>   * maybe we should fall back to "origin", like vc-git-repository-url
>>     does,
>
> FWIW, the current patch will show
>
>     Branch     : vc-dir-tracking-branch
>     Tracking   : origin/master
>     Remote     : https://git.savannah.gnu.org/git/emacs.git
>
> for my checkout of this work-in-progress patch, and
>
>     Branch     : vc-dir-bug
>     Tracking   : master
>     Remote     : none (tracking local branch)
>
> for a checkout made following Tom's recipe, and
>
>     Branch     : trunk
>
> for a fresh 'git init' with just a default branch.

Wondering if there's anything I can do to help with review, or if I
should install this.  OT1H I don't want to be pushy, OTOH I should be in
the area to answer for any collateral damage.





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#68183: 28.3; vc-dir fails when I have a certain branch checked out
  2024-02-14 19:56         ` Kévin Le Gouguec
  2024-03-13 20:03           ` Kévin Le Gouguec
@ 2024-03-15  2:57           ` Dmitry Gutov
  2024-03-16 17:56             ` Kévin Le Gouguec
  1 sibling, 1 reply; 12+ messages in thread
From: Dmitry Gutov @ 2024-03-15  2:57 UTC (permalink / raw)
  To: Kévin Le Gouguec, Tom Tromey; +Cc: Eli Zaretskii, 68183, Juri Linkov

Hi!

Sorry about the late reply.

It seems like you've done a fair amount of testing, both manual and 
automated - thanks, more tests are welcome.

On 14/02/2024 21:56, Kévin Le Gouguec wrote:
> And here's another revision, addressing most of the points below.
> WDY'allT?
> 
>> * the test should probably have a (skip-unless (have-git-or-something)),
> Done.
> 
>> * maybe "none (tracking local branch)" is not informative and we should
>>    ditch it,
>>    * maybe we should fall back to "origin", like vc-git-repository-url
>>      does,
> FWIW, the current patch will show
> 
>      Branch     : vc-dir-tracking-branch
>      Tracking   : origin/master
>      Remote     :https://git.savannah.gnu.org/git/emacs.git
> 
> for my checkout of this work-in-progress patch, and
> 
>      Branch     : vc-dir-bug
>      Tracking   : master
>      Remote     : none (tracking local branch)
> 
> for a checkout made following Tom's recipe, and
> 
>      Branch     : trunk
> 
> for a fresh 'git init' with just a default branch.

IIUC you're adding the new "Tracking" header to the output? That seems 
like it should be helpful.

Is there a way that we could/should optimize the display? I.e., I guess 
the most common case will be something like:

       Branch     : foo-bar
       Tracking   : origin/foo-bar

which is not bad, but might be less useful than indicating that the 
current branch does not track anything (and so the next 'git push' 
should come with '-u'), or tracks a differently named branch. It might 
be more ergonomic to emphasize "irregular" scenarios and maybe even save 
on the extra line in the "common" one.

Just a thought. Not something that needs to be addressed right now. And 
I might as well be off the mark here.

> OT1H "none (tracking local branch)" is redundant with "Tracking" not
> being prefixed with "origin"; OTOH
> 
> * stripping "Remote" altogether might confuse users - at least "tracking
>    local branch" hints at what's going on,
> 
> * Falling back to origin's URL might also cause confusion: users might
>    then expect 'vc-pull' to fetch changes from that URL, which is not the
>    case.

That seems fine.

> So all in all I think the above is reasonably useful.
> 
>> * rushed the ChangeLog entry; vc-git-test--run should also be declared
>>    as a "new helper" (and maybe I should spell out that I used it to not
>>    have to depend on vc-git-- internal functions),
> Done.
> 
>> * maybe the new header deserves a NEWS entry.
> Maybe?

It wouldn't hurt. Up to you.

Anyway, I think the patch is good to go. Please feel free to install it; 
whatever cosmetic changes we might like to add could be done later.





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#68183: 28.3; vc-dir fails when I have a certain branch checked out
  2024-03-15  2:57           ` Dmitry Gutov
@ 2024-03-16 17:56             ` Kévin Le Gouguec
  2024-03-17  1:06               ` Dmitry Gutov
  0 siblings, 1 reply; 12+ messages in thread
From: Kévin Le Gouguec @ 2024-03-16 17:56 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Eli Zaretskii, Tom Tromey, 68183, Juri Linkov

Dmitry Gutov <dmitry@gutov.dev> writes:

> Sorry about the late reply.

(Not at all, thanks for taking a look 🙏)

>> FWIW, the current patch will show
>>      Branch     : vc-dir-tracking-branch
>>      Tracking   : origin/master
>>      Remote     :https://git.savannah.gnu.org/git/emacs.git
>> for my checkout of this work-in-progress patch, and
>>      Branch     : vc-dir-bug
>>      Tracking   : master
>>      Remote     : none (tracking local branch)
>> for a checkout made following Tom's recipe, and
>>      Branch     : trunk
>> for a fresh 'git init' with just a default branch.
>
> IIUC you're adding the new "Tracking" header to the output? That seems like it should be helpful.
>
> Is there a way that we could/should optimize the display? I.e., I guess the most common case will be something like:
>
>       Branch     : foo-bar
>       Tracking   : origin/foo-bar

Right, the current patch indeed shows this for a common-case clone of
the Emacs repo:

    VC backend : Git
    Working dir: ~/src/emacs/master/
    Branch     : master
    Tracking   : origin/master
    Remote     : https://git.savannah.gnu.org/git/emacs.git

> which is not bad, but might be less useful than indicating that the current branch does not track anything (and so the next 'git push' should come with '-u'), or tracks a differently named branch. It might be more ergonomic to emphasize "irregular" scenarios and maybe even save on the extra line in the "common" one.

Good food for thought.

Re. optimizing the display (which I interpret as reducing redundant
information): as someone who often works with multiple remotes, seeing
"Branch: FOO ; Tracking: origin/FOO" is actually useful, so I wouldn't
want to remove the "tracking" bit unconditionally.  OTOH it could surely
be condensed to a single line, say

    Branch     : master (tracking: origin/master)

Likewise, in the local-tracking-branch case, we could go from

    Branch:    : vc-dir-bug
    Tracking   : master
    Remote     : none (tracking local branch)

to just

    Branch:    : vc-dir-bug (tracking: local master)

Re. emphasizing irregular scenarios, specifically those where 'git push'
will require '-u': I like the idea, but I am wary of us getting lost in
the weeds second-guessing Git's intentions.  E.g. even when
branch.foo.merge and branch.foo.remote are unset, 'git push' can still
be called without '-u' if push.default is 'current' and
remote.pushDefault is set (whereas 'git pull' will fail).

> Just a thought. Not something that needs to be addressed right now. And I might as well be off the mark here.

I agree it's worth thinking about.  The Right Solution™ would probably
come with user options to let users fine-tune which details they care
about?  It would be interesting to survey Git UIs to see how they
approach this (FWIW I am partial to Magit's presentation, but I have not
studied how it handles all the corner cases we considered).

>>> * maybe the new header deserves a NEWS entry.
>> Maybe?
>
> It wouldn't hurt. Up to you.
>
> Anyway, I think the patch is good to go. Please feel free to install it; whatever cosmetic changes we might like to add could be done later.

ACK.  I might go a head and install then (after polishing the changelog,
i.e. re-filling and scrubbing Unicode ellipses) in the spirit of getting
the original issue fixed; perhaps worth holding off on the NEWS entry
until we decide how exactly things should look.





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#68183: 28.3; vc-dir fails when I have a certain branch checked out
  2024-03-16 17:56             ` Kévin Le Gouguec
@ 2024-03-17  1:06               ` Dmitry Gutov
  2024-03-17 18:09                 ` Kévin Le Gouguec
  0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Gutov @ 2024-03-17  1:06 UTC (permalink / raw)
  To: Kévin Le Gouguec; +Cc: Eli Zaretskii, Tom Tromey, 68183, Juri Linkov

On 16/03/2024 19:56, Kévin Le Gouguec wrote:

>> IIUC you're adding the new "Tracking" header to the output? That seems like it should be helpful.
>>
>> Is there a way that we could/should optimize the display? I.e., I guess the most common case will be something like:
>>
>>        Branch     : foo-bar
>>        Tracking   : origin/foo-bar
> 
> Right, the current patch indeed shows this for a common-case clone of
> the Emacs repo:
> 
>      VC backend : Git
>      Working dir: ~/src/emacs/master/
>      Branch     : master
>      Tracking   : origin/master
>      Remote     : https://git.savannah.gnu.org/git/emacs.git
> 
>> which is not bad, but might be less useful than indicating that the current branch does not track anything (and so the next 'git push' should come with '-u'), or tracks a differently named branch. It might be more ergonomic to emphasize "irregular" scenarios and maybe even save on the extra line in the "common" one.
> 
> Good food for thought.
> 
> Re. optimizing the display (which I interpret as reducing redundant
> information): as someone who often works with multiple remotes, seeing
> "Branch: FOO ; Tracking: origin/FOO" is actually useful, so I wouldn't
> want to remove the "tracking" bit unconditionally.  OTOH it could surely
> be condensed to a single line, say
> 
>      Branch     : master (tracking: origin/master)

Yeah, something like this could work. I was imagining pseudo-graphical

      Branch     : master -> origin/master

, but it's good to have words. Maybe drop the last colon...

> Likewise, in the local-tracking-branch case, we could go from
> 
>      Branch:    : vc-dir-bug
>      Tracking   : master
>      Remote     : none (tracking local branch)
> 
> to just
> 
>      Branch:    : vc-dir-bug (tracking: local master)

I would say that the local-tracking scenario is a minority, and so it's 
not as important to optimize, but the above makes sense. But the word 
"local" is very close to "master", while the latter is a special string 
which should probably somehow stand out. So the "unoptimized" version 
might still have its advantages:

       Branch:    : vc-dir-bug (tracking master)
       Remote     : none (local branch)

or

       Branch:    : vc-dir-bug (-> master)
       Remote     : none (local branch)

or

       Branch:    : vc-dir-bug (tracking -> master)
       Remote     : none (local branch)

> Re. emphasizing irregular scenarios, specifically those where 'git push'
> will require '-u': I like the idea, but I am wary of us getting lost in
> the weeds second-guessing Git's intentions.  E.g. even when
> branch.foo.merge and branch.foo.remote are unset, 'git push' can still
> be called without '-u' if push.default is 'current' and
> remote.pushDefault is set (whereas 'git pull' will fail).

Okay, if you're sure. A (no upstream) note might make sense in the above 
scenario too, but I don't have a strong opinion. We could also make a 
user option later, if the new behavior makes sense for most usage habits 
but not all.

>> Just a thought. Not something that needs to be addressed right now. And I might as well be off the mark here.
> 
> I agree it's worth thinking about.  The Right Solution™ would probably
> come with user options to let users fine-tune which details they care
> about?  It would be interesting to survey Git UIs to see how they
> approach this (FWIW I am partial to Magit's presentation, but I have not
> studied how it handles all the corner cases we considered).

Magit, meaning just one line for Head: and another for Merge:?

>>>> * maybe the new header deserves a NEWS entry.
>>> Maybe?
>>
>> It wouldn't hurt. Up to you.
>>
>> Anyway, I think the patch is good to go. Please feel free to install it; whatever cosmetic changes we might like to add could be done later.
> 
> ACK.  I might go a head and install then (after polishing the changelog,
> i.e. re-filling and scrubbing Unicode ellipses) in the spirit of getting
> the original issue fixed; perhaps worth holding off on the NEWS entry
> until we decide how exactly things should look.

I'm okay with that.





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#68183: 28.3; vc-dir fails when I have a certain branch checked out
  2024-03-17  1:06               ` Dmitry Gutov
@ 2024-03-17 18:09                 ` Kévin Le Gouguec
  2024-03-18 15:26                   ` Dmitry Gutov
  0 siblings, 1 reply; 12+ messages in thread
From: Kévin Le Gouguec @ 2024-03-17 18:09 UTC (permalink / raw)
  To: Dmitry Gutov; +Cc: Eli Zaretskii, Tom Tromey, 68183, Juri Linkov

Dmitry Gutov <dmitry@gutov.dev> writes:

>> Re. optimizing the display (which I interpret as reducing redundant
>> information): as someone who often works with multiple remotes, seeing
>> "Branch: FOO ; Tracking: origin/FOO" is actually useful, so I wouldn't
>> want to remove the "tracking" bit unconditionally.  OTOH it could surely
>> be condensed to a single line, say
>>      Branch     : master (tracking: origin/master)
>
> Yeah, something like this could work. I was imagining pseudo-graphical
>
>      Branch     : master -> origin/master
>
> , but it's good to have words. Maybe drop the last colon...

Right, feels like we have some horizontal space to work with, so we can
spend the extra word if it conveys more meaning than an arrow (and yeah,
OTOH that last colon does not bring much).

>> Likewise, in the local-tracking-branch case, we could go from
>>      Branch:    : vc-dir-bug
>>      Tracking   : master
>>      Remote     : none (tracking local branch)
>> to just
>>      Branch:    : vc-dir-bug (tracking: local master)
>
> I would say that the local-tracking scenario is a minority, and so it's not as important to optimize, but the above makes sense. But the word "local" is very close to "master", while the latter is a special string which should probably somehow stand out. So the "unoptimized" version might still have its advantages:
>
>       Branch:    : vc-dir-bug (tracking master)
>       Remote     : none (local branch)

That looks sensible.  If I were to argue in favor of keeping "local"
juxtaposed to the tracking branch, I'd say that in the _absence_ of an
explicit qualifier in "Branch: vc-dir-bug (tracking master)", my brain
might "default" to assuming we are tracking the remote "master"; a
solution to let branch names stand out might then be faces:
e.g. vc-git-log-view-mode paints them with change-log-list…

But I don't know if it's a very powerful argument.

>>> Just a thought. Not something that needs to be addressed right now. And I might as well be off the mark here.
>> I agree it's worth thinking about.  The Right Solution™ would probably
>> come with user options to let users fine-tune which details they care
>> about?  It would be interesting to survey Git UIs to see how they
>> approach this (FWIW I am partial to Magit's presentation, but I have not
>> studied how it handles all the corner cases we considered).
>
> Magit, meaning just one line for Head: and another for Merge:?

So, given

𝒷   ≔ current branch
𝓂   ≔ branch.𝒷.merge
𝓇   ≔ branch.𝒷.remote
𝓅   ≔ branch.𝒷.pushRemote or remote.pushDefault

By default, magit-status shows:

"Head:"                 𝒷, or the current commit when detached
"Merge:" (or "Rebase:") 𝓇/𝓂, or just 𝓂 if 𝓇 = "."
"Push:"                 𝓅/𝒷, appending "does not exist" if applicable

(And another header related to tags I'm going to ignore for now)

Of note, Magit offers an option (magit-status-headers-hook) to let users
control which of these (and more) to display.

One of the available headers that is disabled by default shows

"Remote: ℛ <remote.ℛ.url>"

where ℛ is 𝓇, or "origin" if this branch's remote is unset or ".", or
"the first remote in alphabetic order" if "origin" does not exist.

(
  Also of note, IMO, is the branch-variable-configuration menu (the
  magit-branch-configure transient), which looks like this:

      Configure vc-dir-bug
       d branch.vc-dir-bug.description unset
       u branch.vc-dir-bug.merge refs/heads/master
         branch.vc-dir-bug.remote .
       r branch.vc-dir-bug.rebase [true|false|default:false]
       p branch.vc-dir-bug.pushRemote [hirondell|origin|savannah-rw|vps|remote.pushDefault:vps]

      Configure repository defaults
       R pull.rebase [true|false|default:false]
       P remote.pushDefault [hirondell|origin|savannah-rw|vps]
       b Update default branch

      Configure branch creation
       a m branch.autoSetupMerge [always|true|false|default:true]
       a r branch.autoSetupRebase [always|local|remote|never|default:never]
      __
  (Not pictured: the currently active value in [foo|bar|baz] is
  highlighted with a face; the keybindings on the left cycle between
  alternatives)

  No idea how/where that would fit into VC, but golly is it a neat UI to
  work with, so I thought it deserved a shout-out.
)

>>>>> * maybe the new header deserves a NEWS entry.
>>>> Maybe?
>>>
>>> It wouldn't hurt. Up to you.
>>>
>>> Anyway, I think the patch is good to go. Please feel free to install it; whatever cosmetic changes we might like to add could be done later.
>> ACK.  I might go a head and install then (after polishing the changelog,
>> i.e. re-filling and scrubbing Unicode ellipses) in the spirit of getting
>> the original issue fixed; perhaps worth holding off on the NEWS entry
>> until we decide how exactly things should look.
>
> I'm okay with that.

(Pushed 📨)





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#68183: 28.3; vc-dir fails when I have a certain branch checked out
  2024-03-17 18:09                 ` Kévin Le Gouguec
@ 2024-03-18 15:26                   ` Dmitry Gutov
  0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Gutov @ 2024-03-18 15:26 UTC (permalink / raw)
  To: Kévin Le Gouguec; +Cc: Eli Zaretskii, Tom Tromey, 68183, Juri Linkov

On 17/03/2024 20:09, Kévin Le Gouguec wrote:

>> I would say that the local-tracking scenario is a minority, and so it's not as important to optimize, but the above makes sense. But the word "local" is very close to "master", while the latter is a special string which should probably somehow stand out. So the "unoptimized" version might still have its advantages:
>>
>>        Branch:    : vc-dir-bug (tracking master)
>>        Remote     : none (local branch)
> 
> That looks sensible.  If I were to argue in favor of keeping "local"
> juxtaposed to the tracking branch, I'd say that in the _absence_ of an
> explicit qualifier in "Branch: vc-dir-bug (tracking master)", my brain
> might "default" to assuming we are tracking the remote "master"; a
> solution to let branch names stand out might then be faces:
> e.g. vc-git-log-view-mode paints them with change-log-list…
> 
> But I don't know if it's a very powerful argument.

I figured that when "local" is on the next line already, it's easy 
enough to notice. Anyway, it's up to you, I think.

>>>> Just a thought. Not something that needs to be addressed right now. And I might as well be off the mark here.
>>> I agree it's worth thinking about.  The Right Solution™ would probably
>>> come with user options to let users fine-tune which details they care
>>> about?  It would be interesting to survey Git UIs to see how they
>>> approach this (FWIW I am partial to Magit's presentation, but I have not
>>> studied how it handles all the corner cases we considered).
>>
>> Magit, meaning just one line for Head: and another for Merge:?
> 
> So, given
> 
> 𝒷   ≔ current branch
> 𝓂   ≔ branch.𝒷.merge
> 𝓇   ≔ branch.𝒷.remote
> 𝓅   ≔ branch.𝒷.pushRemote or remote.pushDefault
> 
> By default, magit-status shows:
> 
> "Head:"                 𝒷, or the current commit when detached
> "Merge:" (or "Rebase:") 𝓇/𝓂, or just 𝓂 if 𝓇 = "."
> "Push:"                 𝓅/𝒷, appending "does not exist" if applicable

So Magit also prints just "master" is it's a local tracking branch and 
something like "origin/master" when it belongs to a remote? Going with 
that approach, we'd seem justified to print

       Branch:    : vc-dir-bug (tracking master)

in the former case and

       Branch:    : vc-dir-bug (tracking origin/master)

in the latter.

> (And another header related to tags I'm going to ignore for now)
> 
> Of note, Magit offers an option (magit-status-headers-hook) to let users
> control which of these (and more) to display.
> 
> One of the available headers that is disabled by default shows
> 
> "Remote: ℛ <remote.ℛ.url>"
> 
> where ℛ is 𝓇, or "origin" if this branch's remote is unset or ".", or
> "the first remote in alphabetic order" if "origin" does not exist.

Meaning it prepends the remote's url with remote's name, if that line is 
configured to be shown. We could do that too.

> (
>    Also of note, IMO, is the branch-variable-configuration menu (the
>    magit-branch-configure transient), which looks like this:
> 
>        Configure vc-dir-bug
>         d branch.vc-dir-bug.description unset
>         u branch.vc-dir-bug.merge refs/heads/master
>           branch.vc-dir-bug.remote .
>         r branch.vc-dir-bug.rebase [true|false|default:false]
>         p branch.vc-dir-bug.pushRemote [hirondell|origin|savannah-rw|vps|remote.pushDefault:vps]
> 
>        Configure repository defaults
>         R pull.rebase [true|false|default:false]
>         P remote.pushDefault [hirondell|origin|savannah-rw|vps]
>         b Update default branch
> 
>        Configure branch creation
>         a m branch.autoSetupMerge [always|true|false|default:true]
>         a r branch.autoSetupRebase [always|local|remote|never|default:never]
>        __
>    (Not pictured: the currently active value in [foo|bar|baz] is
>    highlighted with a face; the keybindings on the left cycle between
>    alternatives)
> 
>    No idea how/where that would fit into VC, but golly is it a neat UI to
>    work with, so I thought it deserved a shout-out.
> )

Magit is definitely a great UI to learn Git's capabilities.

>>>>>> * maybe the new header deserves a NEWS entry.
>>>>> Maybe?
>>>>
>>>> It wouldn't hurt. Up to you.
>>>>
>>>> Anyway, I think the patch is good to go. Please feel free to install it; whatever cosmetic changes we might like to add could be done later.
>>> ACK.  I might go a head and install then (after polishing the changelog,
>>> i.e. re-filling and scrubbing Unicode ellipses) in the spirit of getting
>>> the original issue fixed; perhaps worth holding off on the NEWS entry
>>> until we decide how exactly things should look.
>>
>> I'm okay with that.
> 
> (Pushed 📨)

Thanks!





^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2024-03-18 15:26 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-31 18:59 bug#68183: 28.3; vc-dir fails when I have a certain branch checked out Tom Tromey
2023-12-31 19:34 ` Eli Zaretskii
2023-12-31 20:14   ` Tom Tromey
2024-01-03  9:46     ` Kévin Le Gouguec
2024-02-12  8:08       ` Kévin Le Gouguec
2024-02-14 19:56         ` Kévin Le Gouguec
2024-03-13 20:03           ` Kévin Le Gouguec
2024-03-15  2:57           ` Dmitry Gutov
2024-03-16 17:56             ` Kévin Le Gouguec
2024-03-17  1:06               ` Dmitry Gutov
2024-03-17 18:09                 ` Kévin Le Gouguec
2024-03-18 15:26                   ` Dmitry Gutov

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).