Scp path separator should not be OS-dependent

[Tisza Gergő]

The path used in set_hooks_commit_msg to get the commit hook through SCP contains a local path fragment; that means that the path separators will depend on the local OS. Specifically, when git-review --setup is run on a Window machine, the path passed to the SCP command will contain backslashes, and the command will fail.

This is the offending line (set_hooks_commit_msg function in git-review file):

    scp_cmd += ":hooks/commit-msg %s" % target_file

a possible fix:

    rel_target_path = str.replace(target_file, os.path.sep, "/")
    scp_cmd += ":hooks/commit-msg %s" % rel_target_path

[Andrew Hutchings (linuxjedi)]

is this actually a reproducible problem in Windows? I don't know about Python but in most other languages I have used Windows actually figures out what you meant.

[Tisza Gergő (gtisza)]

It is not really a problem with Windows. The script uses os.path.join to create the path (so when it is run on a Windows machine, Python uses backslash as the path separator), but then it gets passed to an SCP command (which means it is actually used on the remote Linux machine). And while Windows is lax about which path separator you use, Linux is not.

As for reproducibility, other people had this problem. See the last bullet point here:
https://www.mediawiki.org/w/index.php?title=Git/Workflow&oldid=561410#Windows

[James E. Blair (corvus)]

I must be missing something; I don't see how the local path portion of an SCP command has a bearing on the remote SSH server.

[Launchpad Janitor (janitor)]

[Expired for git-review because there has been no activity for 60 days.]

[Marcin Cieślak (saperski)]

Proposed inclusion of the https://github.com/saper/simple-scp-client to relieve us of all SCP problems:

http://lists.openstack.org/pipermail/openstack-infra/2013-January/000023.html

[Per Olausson (per-olauzzon-h)]

This is quite a troublesome issue. It is not 100% reproducible because this issue depends on where the user is standing when he is invoking git-review.

(1) If the user is standing at the root of a Git repository, then git-review will work fine.

(2) If the user is standing anywhere else, then git-review will fail.

The reason for this is:

(1)

The path of git_dir will resolve to ".git" and when the path for the commit-msg hook is computed using os.path.join, the resulting path will be: ".git\\hooks\\commit-msg"

If python or scp is given this path, it will work.

Eg. "scp mylocalfile '.git\\hooks\\bla'
works.

(2)

If the user is in any sub directory of the Git repository, then rev-parse --show-toplevel --git-dir will generate a full path, eg.

c:/bla/bla/myrepo/.git

If this is then later used in an os.path.join() then '\\hook\\commit-msg' will be added to the full path.

Neither python nor scp will recognise any such path as being valid. And the hook will fail to be installed.

The solution for this is fairly trivial.

At the outset in main(), chdir to "git rev-parse --show-toplevel" and from there on I imagine that every and all operations will function properly, since everything coming out of git will be with relative paths.

Do you guys agree or have I missed any other functionality in git-review that will break as a result of altering the working directory at the outset?

[Per Olausson (per-olauzzon-h)]

If you agree, then the only patch required is.

diff --git a/git_review/cmd.py b/git_review/cmd.py
index 7f6318b..47e9158 100755
--- a/git_review/cmd.py
+++ b/git_review/cmd.py
@@ -1101,6 +1101,9 @@ def main():

     try:
         (top_dir, git_dir) = git_directories()
+ # address windows path separators related to using windows python with git-bash:
+ os.chdir(top_dir)
+ (top_dir, git_dir) = git_directories()
     except GitDirectoriesException:
         if sys.argv[1:] in ([], ['-h'], ['--help']):
             parser.print_help()

[Per Olausson (per-olauzzon)]

I just noticed a related issue which appear to affect all platforms (yes maybe it should be a new defect, but the solution is as above):

If a user adds new content, adds, commits and then issues git-review, then git-review will fail. That is because it isn't possible to issue a git reset from a new directory?

Example:

~/tmp/reorgs$ git clone ssh://.... x
Cloning into 'x'...
remote: Counting objects: 316, done
remote: Finding sources: 100% (316/316)
remote: Total 316 (delta 35), reused 287 (delta 35)
Receiving objects: 100% (316/316), 50.98 KiB, done.
Resolving deltas: 100% (35/35), done.
:~/tmp/reorgs$ cd x
:~/tmp/reorgs/x$ mkdir newdirectory
:~/tmp/reorgs/x$ cd newdirectory/
:~/tmp/reorgs/x/newdirectory$ touch x
:~/tmp/reorgs/x/newdirectory$ git add x
:~/tmp/reorgs/x/newdirectory$ git commit -am "addingnewstuff"
[master 67da244] addingnewstuff
 0 files changed
 create mode 100644 newdirectory/x
:~/tmp/reorgs/x/newdirectory$ git-review
Creating a git remote called "gerrit" that maps to:
 ssh://....
Errors running git reset --hard 67da244eb3195acae1dfd4b27bacd917f09fb8ff
fatal: Unable to read current working directory: No such file or directory
:~/tmp/reorgs/x/newdirectory$ cd ..
:~/tmp/reorgs/x$ git-review
remote: Resolving deltas: 100% (1/1)
remote: Processing changes: refs: 1, done
remote: ERROR: missing Change-Id in commit message footer
remote: Suggestion for commit message:
remote: addingnewstuff
remote:
remote: Change-Id: I67da244eb3195acae1dfd4b27bacd917f09fb8ff
remote:
remote: Hint: To automatically insert Change-Id, install the hook:
remote: scp -p -P 29418 ..../commit-msg `git rev-parse --git-dir`/hooks/commit-msg
remote:
remote:
To ssh://....
 ! [remote rejected] HEAD -> refs/publish/master (missing Change-Id in commit message footer)
error: failed to push some refs to 'ssh://....'

Again, all of this works if git-review is changed to chdir() to the root of the repository before doing any operations...

[Cedric Brandily (cbrandily)]

Fix committed: https://review.openstack.org/110331

[Shinobu Kinjo (shinobu-kj)]

Just question.

I've faced same kind of error message:

To ssh://....
 ! [remote rejected] HEAD -> refs/publish/master (missing Change-Id in commit message footer)
error: failed to push some refs to 'ssh://....'

Which I've never since I did some commitments.
Is this same error of this bug?

Any suggestion would be appreciated.
Thank you in advance.

[Cedric Brandily (cbrandily)]

It's unrelated, your commit message is incorrect:

 remote rejected] HEAD -> refs/publish/master (missing Change-Id in commit message footer)

means that no Change-Id: is in the commit message. You should do:

 git review -s
 git commit --amend
 git review