| Message ID | 2f0bffb484beccf58f2440ed5e2c04a1ba26e6c3.1669126703.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | git-jump: support Emacs | expand |
Hi Yoichi On 22/11/2022 14:18, Yoichi Nakayama via GitGitGadget wrote: > From: Yoichi Nakayama <yoichi.nakayama@gmail.com> > > It works with GIT_EDITOR="emacs", "emacsclient" or "emacsclient -t" Thanks for working on this, I'm looking forward to being able to use "git jump" with emacs. > Signed-off-by: Yoichi Nakayama <yoichi.nakayama@gmail.com> > --- > contrib/git-jump/git-jump | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > 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)))\"" I just tried this out in a frame (window for non emacs users) showing two files and the (delete-other-windows) call replaced both of them with the grep buffer. It would be nicer if it created a new window in the current frame or showed the grep buffer in one of the existing windows. If I delete (delete-other-windows) then the first time I run "git jump" it shows the grep buffer in the frame I already have open, but then if I run it again without closing the grep buffer it opens a new frame. I wonder if it would be better just to close the buffer if it exists before creating the new one or pass NAME-FUNCTION argument to compilation-start that creates unique names. I'm using emacsclient as my editor and when I run "git jump" it prints #<buffer *grep*> in the terminal (presumably because that is the return value of select-frame-set-input-focus) Could we read the file and set the buffer's mode to grep-mode (or compilation-mode?) without forking cat? Best Wishes Phillip > +} > + > mode_diff() { > git diff --no-prefix --relative "$@" | > perl -ne ' > @@ -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"
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
On Wed, Nov 23, 2022 at 1:40 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > + # 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)))\"" > > I just tried this out in a frame (window for non emacs users) showing > two files and the (delete-other-windows) call replaced both of them with > the grep buffer. It would be nicer if it created a new window in the > current frame or showed the grep buffer in one of the existing windows. Thanks for your feedback. The first point is that you want to keep the same window configuration as before you do git jump, and reuse existing window (like M-x grep), right? I think "(delete-other-windows)" was superfluous, so I'll remove it. Will it do what you want? - In case of editor="emacsclient", it will try to keep window configuration. - In case of editor="emacsclient -t" and editor="emacs", it will create two window configuration (up and down). > If I delete (delete-other-windows) then the first time I run "git jump" > it shows the grep buffer in the frame I already have open, but then if I > run it again without closing the grep buffer it opens a new frame. I > wonder if it would be better just to close the buffer if it exists > before creating the new one or pass NAME-FUNCTION argument to > compilation-start that creates unique names. I've also seen a new frame being created unintentionally. It is caused by the wrong use of switch-to-buffer-other-frame. I'll try to fix.
On Wed, Nov 23, 2022 at 3:54 AM Jeff King <peff@peff.net> wrote: > 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. Right. But the problem is not specific to emacs (it happens in vim too). Let's fix it another time (as you noted, that's pretty unlikely, and we may not even need to fix it). > 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 > } Sure.
On Wed, Nov 23, 2022 at 02:33:50PM +0900, Yoichi Nakayama wrote: > On Wed, Nov 23, 2022 at 3:54 AM Jeff King <peff@peff.net> wrote: > > 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. > > Right. But the problem is not specific to emacs (it happens in vim too). > Let's fix it another time (as you noted, that's pretty unlikely, and we may > not even need to fix it). Good point. The vim version is easier to fix (we just need to double-quote \$1 inside the eval), but the fact that nobody has complained is an indication that it does not really matter. -Peff
On Thu, Nov 24, 2022 at 10:09 AM Jeff King <peff@peff.net> wrote: > > On Wed, Nov 23, 2022 at 02:33:50PM +0900, Yoichi Nakayama wrote: > > > On Wed, Nov 23, 2022 at 3:54 AM Jeff King <peff@peff.net> wrote: > > > 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. > > > > Right. But the problem is not specific to emacs (it happens in vim too). > > Let's fix it another time (as you noted, that's pretty unlikely, and we may > > not even need to fix it). > > Good point. The vim version is easier to fix (we just need to > double-quote \$1 inside the eval), but the fact that nobody has > complained is an indication that it does not really matter. I've confirmed the vim version is fixed by eval "$editor -q \"\$1\"" With your hint, I found the emacs version can be fixed by single-quoting the variable (I found a mistake in the emacs version. Since there is only one argument, I should use $1 instead of $@. I'll fix it.), and the vim version can be also in the similar form with single quote: eval "$editor -q '$1'" The original vim version used the notation \$1 instead of $1. I'm worried that the emacs version might need the backslash. What does the backslash mean? Is it necessary?
On Thu, Nov 24, 2022 at 9:32 PM Yoichi Nakayama <yoichi.nakayama@gmail.com> wrote: > > On Thu, Nov 24, 2022 at 10:09 AM Jeff King <peff@peff.net> wrote: > > > > On Wed, Nov 23, 2022 at 02:33:50PM +0900, Yoichi Nakayama wrote: > > > > > On Wed, Nov 23, 2022 at 3:54 AM Jeff King <peff@peff.net> wrote: > > > > 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. > > > > > > Right. But the problem is not specific to emacs (it happens in vim too). > > > Let's fix it another time (as you noted, that's pretty unlikely, and we may > > > not even need to fix it). > > > > Good point. The vim version is easier to fix (we just need to > > double-quote \$1 inside the eval), but the fact that nobody has > > complained is an indication that it does not really matter. > > I've confirmed the vim version is fixed by > eval "$editor -q \"\$1\"" > > With your hint, I found the emacs version can be fixed > by single-quoting the variable (I found a mistake in the > emacs version. Since there is only one argument, I > should use $1 instead of $@. I'll fix it.), and the vim > version can be also in the similar form with single quote: > eval "$editor -q '$1'" > > The original vim version used the notation \$1 instead of $1. > I'm worried that the emacs version might need the backslash. > What does the backslash mean? Is it necessary? I found the answer myself. The backslash is to leave the evaluation of the argument to the 'eval' execution. And another question arose. Why do we use eval? What is the difference from running it directly like below? $editor -q $1
On Thu, Nov 24, 2022 at 09:32:45PM +0900, Yoichi Nakayama wrote: > > Good point. The vim version is easier to fix (we just need to > > double-quote \$1 inside the eval), but the fact that nobody has > > complained is an indication that it does not really matter. > > I've confirmed the vim version is fixed by > eval "$editor -q \"\$1\"" > > With your hint, I found the emacs version can be fixed > by single-quoting the variable (I found a mistake in the > emacs version. Since there is only one argument, I > should use $1 instead of $@. I'll fix it.), and the vim > version can be also in the similar form with single quote: > eval "$editor -q '$1'" This isn't quite a full solution, though. The "$1" is expanded by the outer double-quoted string, which is then fed to eval. The inner single-quotes make most characters literally except for single-quotes themselves. So if $1 has single-quotes, the eval will barf due to bad syntax. > The original vim version used the notation \$1 instead of $1. > I'm worried that the emacs version might need the backslash. > What does the backslash mean? Is it necessary? As you figured out in the other email, it inhibits the outer layer of expansion, and lets the eval expand it. This is the easiest way to pass things through levels of shell evals (since otherwise you have to actually quote, which is a real pain). None of this is sufficient for your emacs example, though. There you have three levels of quoting: - getting the argument intact into the eval; this can use the "\$" trick - the argument then appears inside a double-quoted string which will be evaluated by emacs. You'd want to protect it against double-quotes and presumably backslashes. - emacs will then execute the final command, presumably you a shell. So you'd want to protect against expansion in that shell. The easiest way to do that is usually to wrap each argument in single-quotes, and quoting against interior single quotes (by ending single-quote, adding a single backslashed single-quote, and then re-opening the single-quote). It's horribly ugly, but is (AFAIK) the shortest way to quote shell arguments, and what we usually do in Git. Those are the three tricks I sent in the earlier email (though looking at it again, I think the single-quote bits need to come first, so their backslashes are then quoted to protect against emacs evaluation). It's all quite confusing, which is why I am OK with just skipping it for now. ;) The nice thing, though, is that doing the quoting right means it's safe to get rid of the "cat", which solves your race problems in a more direct and robust way. -Peff
On Thu, Nov 24, 2022 at 09:58:04PM +0900, Yoichi Nakayama wrote: > > The original vim version used the notation \$1 instead of $1. > > I'm worried that the emacs version might need the backslash. > > What does the backslash mean? Is it necessary? > > I found the answer myself. The backslash is to leave the > evaluation of the argument to the 'eval' execution. > And another question arose. Why do we use eval? > What is the difference from running it directly like below? > $editor -q $1 The value of $editor is not a single program name, but is itself a shell command. So you could imagine: git config core.editor "some_command --with --args" or even more complicated shell hackery. From within Git, we'd run it as: sh -c "some_command --with --args" but when you are in a shell already, "eval" is a more efficient way of doing the same. -Peff
On Fri, Nov 25, 2022 at 1:29 AM Jeff King <peff@peff.net> wrote: > Those are the three tricks I sent in the earlier email (though looking > at it again, I think the single-quote bits need to come first, so their > backslashes are then quoted to protect against emacs evaluation). > > It's all quite confusing, which is why I am OK with just skipping it for > now. ;) The nice thing, though, is that doing the quoting right means > it's safe to get rid of the "cat", which solves your race problems in a > more direct and robust way. Thank you for taking the time to explain the details. I don't fully understand it yet, but I know it's hard to deal with properly. In PATCH v7, I made it the same level as the vim version (although it is not strictly at the same level because there is a Emacs Lisp processing). I skip futher treatment of temporary file paths. Regards,
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)))\"" +} + mode_diff() { git diff --no-prefix --relative "$@" | perl -ne ' @@ -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"