New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WITH-EDITOR... line doesn't work from a program that changes directory #55

Closed
raeburn opened this Issue Sep 29, 2018 · 6 comments

Comments

2 participants
@raeburn

raeburn commented Sep 29, 2018

I'm using a remote shell to explore a git checkout tree, run diffs, and do commits. (Yes, I know, should learn magit one of these days. :-) I used with-editor-export-editor to set $EDITOR in the shell process.

If I'm in a subdirectory of the git tree (not the top level), then "git commit" appears to invoke the editor from the top of the tree, with a path like ".git/COMMIT_EDITMSG"; running "lsof -p" on the "sleep" process run in the with-editor shell command confirms that its working directory is the top of the tree, not the directory where I invoked git from my shell, and therefore not the directory specified in default-directory (which is set correctly for my interactive shell's directory). So with-editor tries to edit a file that doesn't exist ($git_top/some/subdir/.git/COMMIT_EDITMSG), tells me how to create the non-existent .git directory, etc.

I'm experimenting with a change to the shell command to use something like:
case "$0" in
/*) echo ... $0 ... ;;
*) echo ... $(pwd -L)/$0 ... ;;
esac; ...

and at least at first it seems to work.

@tarsius tarsius self-assigned this Oct 3, 2018

tarsius added a commit that referenced this issue Oct 3, 2018

with-editor-sleeping-editor: Also print the working directory
When `git-commit' is invoked from a sub-directory, then it changes the
working directory to the root of the working tree before invoking the
editor.  Additionally it ask the editor to edit a relative path in
most cases.  That path is relative to the top-level, so we need the
sleeping editor to also print the name of that directory; or we would
end up trying to edit e.g. "/path/to/repo/subdir/.git/COMMIT_EDITMSG"
instead of "/path/to/repo/.git/COMMIT_EDITMSG".  Other programs might
do the same.

Fixes #55.
@tarsius

This comment has been minimized.

Member

tarsius commented Oct 3, 2018

My attempt in #56 does more of the work in elisp. Also I use pwd instead of pwd -L, which posix says shall be equivalent. Please try it out.

@tarsius

This comment has been minimized.

Member

tarsius commented Oct 8, 2018

What Git version are you using? 2.19.0 does not have this issue because it passes the absolute file-name to the $EDITOR.

Since this has been addressed for Git, I think there is nothing left to be done here. If a program changes the directory and does not use an absolute file-name, then I would consider that a bug. If we find other, up-to-date, programs that do this, then we can still reconsider, I will keep the branch with the proposed workaround alive for a while.

@tarsius tarsius closed this Oct 8, 2018

tarsius added a commit that referenced this issue Oct 8, 2018

with-editor-sleeping-editor: Also print the working directory
When `git-commit' is invoked from a sub-directory, then it changes the
working directory to the root of the working tree before invoking the
editor.  Additionally it ask the editor to edit a relative path in
most cases.  That path is relative to the top-level, so we need the
sleeping editor to also print the name of that directory; or we would
end up trying to edit e.g. "/path/to/repo/subdir/.git/COMMIT_EDITMSG"
instead of "/path/to/repo/.git/COMMIT_EDITMSG".

Actually, as of 2.19.0, `git-commit' no longer does this, it uses an
absolute file-name now.  But programs might still do it and that is
legitimate.  `$EDITOR' inherits the working directory from its parent
process, so it normally isn't a problem if the file-name is relative.
It only a problem here because `$EDITOR' in hour case forwards the
edit request to a running `emacs' instance, which is not a child
process and therefore does not inherit the working directory.

Fixes #55.

@tarsius tarsius reopened this Oct 8, 2018

tarsius added a commit that referenced this issue Oct 8, 2018

with-editor-sleeping-editor: Also print the working directory
When `git-commit' is invoked from a sub-directory, then it changes the
working directory to the root of the working tree before invoking the
editor.  Additionally it ask the editor to edit a relative path in
most cases.  That path is relative to the top-level, so we need the
sleeping editor to also print the name of that directory; or we would
end up trying to edit e.g. "/path/to/repo/subdir/.git/COMMIT_EDITMSG"
instead of "/path/to/repo/.git/COMMIT_EDITMSG".

Actually, as of 2.19.0, `git-commit' no longer does this, it uses an
absolute file-name now.  But programs might still do it and that is
legitimate.  `$EDITOR' inherits the working directory from its parent
process, so it normally isn't a problem if the file-name is relative.
It only a problem here because `$EDITOR' in hour case forwards the
edit request to a running `emacs' instance, which is not a child
process and therefore does not inherit the working directory.

Fixes #55.
@tarsius

This comment has been minimized.

Member

tarsius commented Oct 8, 2018

Changed my mind for the reason given in the commit message. Also I was able to do this in a backward compatible fashion.

@tarsius tarsius closed this in #56 Oct 8, 2018

@raeburn

This comment has been minimized.

raeburn commented Oct 8, 2018

I believe I'm using Fedora 28 and RHEL 7 supplied versions on my test systems -- versions range from 1.8.3 to 2.17.1. I'd prefer not to have to install an updated git manually on all the remote machines I use, especially since the machines themselves frequently get reinstalled in our dev/test processes.

The change in #56 looks good to me, and in a quick test it seems to work fine. (Minor nits: The doc string for the sleeping editor echos lowercase "in" in the alternate implementation where the default setting and the regexp use uppercase "IN". And it might be good to call out the fact that the string printed out includes a control character, in case the user tries typing the alternate version into a shell instead of copying the block via emacs.)

@raeburn

This comment has been minimized.

raeburn commented Oct 8, 2018

BTW, thanks for fixing it! :-)

@tarsius

This comment has been minimized.

Member

tarsius commented Oct 8, 2018

You're welcome.

I have updated the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment