| Message ID | d8233f9617563d7c7168afc6e1abfaba57e54038.1669347422.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | git-jump: support Emacs | expand |
On Fri, Nov 25 2022, Yoichi Nakayama via GitGitGadget wrote: > From: Yoichi Nakayama <yoichi.nakayama@gmail.com> > > It works with GIT_EDITOR="emacs", "emacsclient" or "emacsclient -t" > > Signed-off-by: Yoichi Nakayama <yoichi.nakayama@gmail.com> > --- > contrib/git-jump/git-jump | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump > index cc97b0dcf02..eef9cda832f 100755 > --- a/contrib/git-jump/git-jump > +++ b/contrib/git-jump/git-jump > @@ -23,7 +23,22 @@ EOF > > open_editor() { > editor=`git var GIT_EDITOR` > - eval "$editor -q \$1" > + case "$editor" in > + *emacs*) > + # Supported editor values are: > + # - emacs > + # - emacsclient > + # - emacsclient -t > + # > + # Wait for completion of the asynchronously executed process > + # to avoid race conditions in case of "emacsclient". > + eval "$editor --eval \"(let ((buf (compilation-start \\\"cat \$1\\\" 'grep-mode))) (pop-to-buffer buf) (select-frame-set-input-focus (selected-frame)) (while (get-buffer-process buf) (sleep-for 0.1)))\"" > + ;; > + *) > + # assume anything else is vi-compatible > + eval "$editor -q \$1" > + ;; > + esac > } > > mode_diff() { I'd really like to have some closer and smarter emacs integration like this. But I don't see why we need to run the grep ourselves, pipe it to a temporary file, and then discover that we're using emacs, and --eval code into it to switch to that buffer, and fake up a "M-x grep" command with a compilation buffer to make it look like we ran M-x grep in the first place. Let's just ... run M-x grep earlier? Then we can skip all the earlier steps. I experimented with this a bit locally, and I didn't get the "switch to buffer" semantics to work with this (but that's presumably easy, I'm just rusty on my elisp APIs), but something in this direction seems much better: diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump index eef9cda832f..c932d7acd0f 100755 --- a/contrib/git-jump/git-jump +++ b/contrib/git-jump/git-jump @@ -22,23 +22,7 @@ EOF } open_editor() { - editor=`git var GIT_EDITOR` - case "$editor" in - *emacs*) - # Supported editor values are: - # - emacs - # - emacsclient - # - emacsclient -t - # - # Wait for completion of the asynchronously executed process - # to avoid race conditions in case of "emacsclient". - eval "$editor --eval \"(let ((buf (compilation-start \\\"cat \$1\\\" 'grep-mode))) (pop-to-buffer buf) (select-frame-set-input-focus (selected-frame)) (while (get-buffer-process buf) (sleep-for 0.1)))\"" - ;; - *) - # assume anything else is vi-compatible - eval "$editor -q \$1" - ;; - esac + eval "$editor -q \$1" } mode_diff() { @@ -83,11 +67,14 @@ mode_ws() { } use_stdout= +use_magic=t while test $# -gt 0; do case "$1" in --stdout) use_stdout=t - shift + ;; + --no-magic) + use_magic= ;; --*) usage >&2 @@ -96,7 +83,8 @@ while test $# -gt 0; do *) break ;; - esac + esac && + shift done if test $# -lt 1; then usage >&2 @@ -105,6 +93,22 @@ fi mode=$1; shift type "mode_$mode" >/dev/null 2>&1 || { usage >&2; exit 1; } +editor=`git var GIT_EDITOR` +if test "$use_magic" && test "$mode" = "grep" +then + case "$editor" in + *emacs*) + set -x + eval "$editor --eval \" \ + (grep \\\"git grep -H "$@"\\\") \ + \"" + exit + ;; + *) + ;; + esac +fi + if test "$use_stdout" = "t"; then "mode_$mode" "$@" exit 0 I.e. if we're going to trust emacs to eval this code, and assume that grep.el etc. works, let's just run the equivalent of M-x grep with our 'git grep' command. This is already better in that "grep" understands that I searched for "foo.*bar", so that's highlighted in the resulting buffer, just as with normal "grep" commands. This is missing the bit where we'd need to jump.grepCmd etc, so it's incomplete. I think this is all the prior art we'd need to invoke "git grep" the right way from emacs's "grep": https://github.com/eglaysher/find-things-fast/blob/master/find-things-fast.el#L246 B.t.w. I'd think the "--no-magic" here is something you'd want too. I like this new behavior (sans the comments above), but presumably there's people using emacs as their EDITOR who don't want to have this magic behavior, having an opt-out would be neat. I.e. if you have an existing customization intercepting these then this will screw with that, but maybe that's OK...
On Fri, Nov 25, 2022 at 6:08 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > I'd really like to have some closer and smarter emacs integration like > this. > > But I don't see why we need to run the grep ourselves, pipe it to a > temporary file, and then discover that we're using emacs, and --eval > code into it to switch to that buffer, and fake up a "M-x grep" command > with a compilation buffer to make it look like we ran M-x grep in the > first place. > > Let's just ... run M-x grep earlier? Then we can skip all the earlier > steps. There are two reasons. First, I want to reuse the modes that git-jump already have. In addition to mode_grep, mode_{diff,merge,ws} exist, and if we re-implement each for editor support, I think it will be difficult to maintain. Second, there is a difficulty passing arbitrary arguments properly to Emacs Lisp properly. For example, your version will cause error with git jump grep "hello world" My early patch was doing something similar. But the second problem was hard to deal with, so I switched to using a temporary file.
On Sat, Nov 26 2022, Yoichi Nakayama wrote: > On Fri, Nov 25, 2022 at 6:08 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> I'd really like to have some closer and smarter emacs integration like >> this. >> >> But I don't see why we need to run the grep ourselves, pipe it to a >> temporary file, and then discover that we're using emacs, and --eval >> code into it to switch to that buffer, and fake up a "M-x grep" command >> with a compilation buffer to make it look like we ran M-x grep in the >> first place. >> >> Let's just ... run M-x grep earlier? Then we can skip all the earlier >> steps. > > There are two reasons. > > First, I want to reuse the modes that git-jump already have. In > addition to mode_grep, > mode_{diff,merge,ws} exist, and if we re-implement each for editor > support, I think it will be > difficult to maintain. Yeah, maybe that'll be painful. I haven't poked much at it... > Second, there is a difficulty passing arbitrary arguments properly to > Emacs Lisp properly. > For example, your version will cause error with > git jump grep "hello world" > My early patch was doing something similar. But the second problem was > hard to deal with, > so I switched to using a temporary file. To the extent that that's painful couldn't we write the grep expression / arguments to the tempfile, then feed the tempfile to the ad-hoc elisp code? It would then read it, get the argument to grep for, and we'd call (grep that-argument).
On Sat, Nov 26, 2022 at 1:01 AM Yoichi Nakayama <yoichi.nakayama@gmail.com> wrote: > Second, there is a difficulty passing arbitrary arguments properly to > Emacs Lisp properly. > For example, your version will cause error with > git jump grep "hello world" > My early patch was doing something similar. But the second problem was > hard to deal with, > so I switched to using a temporary file. But even in that case it was fine to use the original grep function defined in grep.el. I'll fix it in v8. -- Yoichi NAKAYAMA
On Sat, Nov 26, 2022 at 12:52:50AM +0100, Ævar Arnfjörð Bjarmason wrote: > > Second, there is a difficulty passing arbitrary arguments properly to > > Emacs Lisp properly. > > For example, your version will cause error with > > git jump grep "hello world" > > My early patch was doing something similar. But the second problem was > > hard to deal with, > > so I switched to using a temporary file. > > To the extent that that's painful couldn't we write the grep expression > / arguments to the tempfile, then feed the tempfile to the ad-hoc elisp > code? > > It would then read it, get the argument to grep for, and we'd call (grep > that-argument). You'd still need to quote the arguments, since you'll be reading potentially multiple arguments out of the bytestream of the file[1]. If you're not going to quote, the simplest thing is to generate the line-oriented output and read that. If you are going to quote, then you don't need the tempfile at all. You can shove the command into the eval, as if git-jump were run from emacs directly (but you want to use the --stdout mode introduced in this series, and not the git commands directly, because of course they're non-trivial). I showed how to do the quoting earlier in the thread. But it is ugly, and this tempfile hack should work (modulo the gross wait loop afterwards). -Peff [1] Of course you could have a stripped-down version that only greps and only takes one argument, but then why are you using git-jump in the first place?
On Mon, Nov 28 2022, Jeff King wrote: > On Sat, Nov 26, 2022 at 12:52:50AM +0100, Ævar Arnfjörð Bjarmason wrote: > >> > Second, there is a difficulty passing arbitrary arguments properly to >> > Emacs Lisp properly. >> > For example, your version will cause error with >> > git jump grep "hello world" >> > My early patch was doing something similar. But the second problem was >> > hard to deal with, >> > so I switched to using a temporary file. >> >> To the extent that that's painful couldn't we write the grep expression >> / arguments to the tempfile, then feed the tempfile to the ad-hoc elisp >> code? >> >> It would then read it, get the argument to grep for, and we'd call (grep >> that-argument). > > You'd still need to quote the arguments, since you'll be reading > potentially multiple arguments out of the bytestream of the file[1]. > > If you're not going to quote, the simplest thing is to generate the > line-oriented output and read that. > > If you are going to quote, then you don't need the tempfile at all. You > can shove the command into the eval, as if git-jump were run from emacs > directly (but you want to use the --stdout mode introduced in this > series, and not the git commands directly, because of course they're > non-trivial). > > I showed how to do the quoting earlier in the thread. But it is ugly, > and this tempfile hack should work (modulo the gross wait loop > afterwards). Thanks, I'd missed https://lore.kernel.org/git/Y30a0ulfxyE7dnYi@coredump.intra.peff.net/ I think the case where the temporary directory itself has spaces in it isn't worth worrying about. So, all we'd need to worry about is getting the arguments to be grep'd to emacs. That should be simpler & bug-free with some equivalent of echo "args" >$tmpfile then in Emacs, given some "<tmpfile>" variable: (with-temp-buffer (insert-file-contents <tempfile>) (buffer-string))) We'd then invoke M-x grep with that. I think getting rid of the tempfile isn't worth it, or worth worrying about, what I was pointing out is that the implementation as it stands works notably differently than if you invoked M-x grep itself. I.e. it doesn't do highlighting, and (I didn't note this before) if it takes a while we'll "hang", if we had emacs itself invoke the "git grep" we'd stream out grep results as they came in.
On Mon, Nov 28, 2022 at 8:03 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > I think getting rid of the tempfile isn't worth it, or worth worrying > about, what I was pointing out is that the implementation as it stands > works notably differently than if you invoked M-x grep itself. > > I.e. it doesn't do highlighting, and (I didn't note this before) if it > takes a while we'll "hang", if we had emacs itself invoke the "git grep" > we'd stream out grep results as they came in. In PATCH v8, we stopped imitating the grep function and changed it to call the grep function. About the first problem, I still don't understand what you mean by highlighting problem. On my environment, the "*grep*" buffer is colored (i.e. filnemame:line_number part on each line is colored). About the second problem, If the "hang" is until writing to tempfile is completed, it is unavoidable as long as tempfile is used. If the "hang" is no response from Emacs when "cat tempfile" takes a long time, we can reduce the duration by detecting a change in (point-max) and exiting the loop. -- Yoichi NAKAYAMA
diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump index cc97b0dcf02..eef9cda832f 100755 --- a/contrib/git-jump/git-jump +++ b/contrib/git-jump/git-jump @@ -23,7 +23,22 @@ EOF open_editor() { editor=`git var GIT_EDITOR` - eval "$editor -q \$1" + case "$editor" in + *emacs*) + # Supported editor values are: + # - emacs + # - emacsclient + # - emacsclient -t + # + # Wait for completion of the asynchronously executed process + # to avoid race conditions in case of "emacsclient". + eval "$editor --eval \"(let ((buf (compilation-start \\\"cat \$1\\\" 'grep-mode))) (pop-to-buffer buf) (select-frame-set-input-focus (selected-frame)) (while (get-buffer-process buf) (sleep-for 0.1)))\"" + ;; + *) + # assume anything else is vi-compatible + eval "$editor -q \$1" + ;; + esac } mode_diff() {