Re: [PATCH v4 2/2] git-jump: invoke emacs/emacsclient

[Jeff King <peff@peff.net>]

On Tue, Nov 22, 2022 at 02:18:23PM +0000, Yoichi Nakayama via GitGitGadget wrote:

> diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
> index babb3b5c68d..bfd759aa4b2 100755
> --- a/contrib/git-jump/git-jump
> +++ b/contrib/git-jump/git-jump
> @@ -26,6 +26,17 @@ open_editor() {
>  	eval "$editor -q \$1"
>  }
>  
> +open_emacs() {
> +	# Supported editor values are:
> +	# - emacs
> +	# - emacsclient
> +	# - emacsclient -t
> +	editor=`git var GIT_EDITOR`
> +	# Wait for completion of the asynchronously executed process
> +	# to avoid race conditions in case of "emacsclient".
> +	eval "$editor --eval \"(prog1 (switch-to-buffer-other-frame (compilation-start \\\"cat $@\\\" 'grep-mode)) (delete-other-windows) (while (get-buffer-process (current-buffer)) (sleep-for 0.1)) (select-frame-set-input-focus (selected-frame)))\""
> +}

Hmm, I know I suggested using a temporary file since "cat $tmpfile"
should be pretty safe. But it does still have problems if your tmp
directory has spaces. Or even other metacharacters, which I think will
be interpreted by the eval, since $@ is expanded in the outermost level
of the shell.

Those are fairly unlikely, but we could handle it. I think you'd need
something like:

	open_emacs() {
		quoted_args=
		for i in "$@"; do
			quoted_args="$quoted_args '$(printf %s "$i" | sed "s/'/'\\\\''/g")'"
		done
		eval "$editor --eval \"...\\\"cat \$quoted_args\\\"...\""
	}

which you can test with:

	cat >fake-emacs <<-\EOF
	#!/bin/sh
	echo "fake-emacs got args: "
	for i in "$@"; do
		echo "arg: $i"
	done
	EOF
	chmod +x fake-emacs

	editor=./fake-emacs
	open_emacs 'multiple args' 'with spaces'
	open_emacs '$dollar is ok because we use single-quotes'
	open_emacs "but 'single quotes' themselves need quoted"

Though it's possible you also need to be adding an extra layer of
quoting due to emacs parsing the string. So you'd probably need to
additionally escape double-quotes and backslashes, perhaps by changing
the sed invocation to:

  sed -e 's/\\/\\\\/g' \
      -e "s/'/'\\\\''/g" \
      -e 's/"/\\"/g'

Which is kind of horrific, but I think is bullet-proof.

Like I said, it's not that likely that somebody's tempfile path would
need all that (though spaces aren't totally out of the question,
especially on Windows). But...

If we have bullet-proof quoting, then you could go back to skipping the
tempfile for emacs, which avoids the race and sleep that you have here.

> @@ -98,4 +109,8 @@ tmp=`mktemp -t git-jump.XXXXXX` || exit 1
>  type "mode_$mode" >/dev/null 2>&1 || { usage >&2; exit 1; }
>  "mode_$mode" "$@" >"$tmp"
>  test -s "$tmp" || exit 0
> +if git var GIT_EDITOR | grep emacs >/dev/null; then
> +	open_emacs "$tmp"
> +	exit 0
> +fi
>  open_editor "$tmp"

If we are going to use a tempfile, this logic should probably get
stuffed into open_editor itself, like:

  open_editor() {
          editor=`git var GIT_EDITOR`
          case "$editor" in
          *emacs*)
                  ...do-the-emacs-thing...
          *)
                  # assume anything else is vi-compatible
                  eval "$editor -q \$1"
          esac
  }

but if you take the quoting suggestion above, then open_emacs() would
continue to be a top-level thing, before we even create the tempfile.

-Peff