magit-diff-visit-avoid-head-blob breaks visit a file from a diff

[aspiers]

With the latest magit, I customized magit-diff-visit-avoid-head-blob to t because I prefer the old behaviour. However, I think this revealed a bug in magit-diff-visit--range-to:

magit/lisp/magit-diff.el

Lines 1695 to 1702 in d24f749

	(defun magit-diff-visit--range-to (spec)
	(if (symbolp spec)
	spec
	(let ((rev (if (consp spec)
	(cdr spec)
	(cdr (magit-split-range spec)))))
	(if (and magit-diff-visit-avoid-head-blob
	(magit-rev-head-p spec))

It calls magit-rev-head-p with a spec, but it's expecting a string:

magit/lisp/magit-git.el

Lines 1094 to 1097 in d24f749

	(defun magit-rev-head-p (rev)
	(or (equal rev "HEAD")
	(and rev
	(not (string-match-p "\\.\\." rev))

Here is the stack trace:

Debugger entered--Lisp error: (wrong-type-argument stringp (commit . "6e6913c0"))
  string-match("\\.\\." (commit . "6e6913c0") nil)
  string-match-p("\\.\\." (commit . "6e6913c0"))
  (not (string-match-p "\\.\\." rev))
  (and rev (not (string-match-p "\\.\\." rev)) (equal (magit-rev-parse rev) (magit-rev-parse "HEAD")))
  (or (equal rev "HEAD") (and rev (not (string-match-p "\\.\\." rev)) (equal (magit-rev-parse rev) (magit-rev-parse "HEAD"))))
  magit-rev-head-p((commit . "6e6913c0"))
  (and magit-diff-visit-avoid-head-blob (magit-rev-head-p spec))
  (if (and magit-diff-visit-avoid-head-blob (magit-rev-head-p spec)) (quote unstaged) rev)
  (let ((rev (if (consp spec) (cdr spec) (cdr (magit-split-range spec))))) (if (and magit-diff-visit-avoid-head-blob (magit-rev-head-p spec)) (quote unstaged) rev))
  (if (symbolp spec) spec (let ((rev (if (consp spec) (cdr spec) (cdr (magit-split-range spec))))) (if (and magit-diff-visit-avoid-head-blob (magit-rev-head-p spec)) (quote unstaged) rev)))
  magit-diff-visit--range-to((commit . "6e6913c0"))
  magit-diff-visit-file--noselect("/home/adam/.GIT/adamspiers.org/emacs/.emacs.d/init.d/as-ui.el" nil)
  magit-diff-visit-file--internal("/home/adam/.GIT/adamspiers.org/emacs/.emacs.d/init.d/as-ui.el" nil pop-to-buffer-same-window)
  magit-diff-visit-file("/home/adam/.GIT/adamspiers.org/emacs/.emacs.d/init.d/as-ui.el" nil)
  funcall-interactively(magit-diff-visit-file "/home/adam/.GIT/adamspiers.org/emacs/.emacs.d/init.d/as-ui.el" nil)
  #<subr call-interactively>(magit-diff-visit-file nil nil)
  apply(#<subr call-interactively> magit-diff-visit-file (nil nil))
  call-interactively@ido-cr+-record-current-command(#<subr call-interactively> magit-diff-visit-file nil nil)
  apply(call-interactively@ido-cr+-record-current-command #<subr call-interactively> (magit-diff-visit-file nil nil))
  call-interactively(magit-diff-visit-file nil nil)
  command-execute(magit-diff-visit-file)

Version used to reproduce: Magit 20200614.1018, Git 2.27.0, Emacs 26.3, gnu/linux

I've said this many times before, but it's worth repeating: magit is unbelievably awesome - thanks for your continued work! I've donated in the past, but just set up a sponsorship subscription.

[aspiers]

BTW, changing the call to (magit-rev-head-p rev) fixes the error, although I'm not 100% sure that's the correct fix.

[tarsius]

Thanks for the report!