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

From: Jeff King <peff@peff.net>
Date: Nov 22, 2022, 18:54 UTC
Message-ID: <Y30a0ulfxyE7dnYi@coredump.intra.peff.net>
In-Reply-To: <2f0bffb484beccf58f2440ed5e2c04a1ba26e6c3.1669126703.git.gitgitgadget@gmail.com>

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

Show 18 quoted lines

> 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.

Show 9 quoted lines

> @@ -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