* [PATCHv2 10/21] git p4: scrub crlf for utf16 files on windows
From: Pete Wyckoff @ 2013-01-27 3:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Sixt
In-Reply-To: <1359256284-5660-1-git-send-email-pw@padd.com>
Files of type utf16 are handled with "p4 print" instead of the
normal "p4 -G print" interface due to how the latter does not
produce correct output. See 55aa571 (git-p4: handle utf16
filetype properly, 2011-09-17) for details.
On windows, though, "p4 print" can not be told which line
endings to use, as there is no underlying client, and always
chooses crlf, even for utf16 files. Convert the \r\n into \n
when importing utf16 files.
The fix for this is complex, in that the problem is a property
of the NT version of p4. There are old versions of p4 that
were compiled directly for cygwin that should not be subjected
to text replacement. The right check here, then, is to look
at the p4 version, not the OS version. Note also that on cygwin,
platform.system() is "CYGWIN_NT-5.1" or similar, not "Windows".
Add a function to memoize the p4 version string and use it to
check for "/NT", indicating the Windows build of p4.
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
git-p4.py | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/git-p4.py b/git-p4.py
index 445d704..c62b2ca 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -170,6 +170,22 @@ def p4_system(cmd):
expand = isinstance(real_cmd, basestring)
subprocess.check_call(real_cmd, shell=expand)
+_p4_version_string = None
+def p4_version_string():
+ """Read the version string, showing just the last line, which
+ hopefully is the interesting version bit.
+
+ $ p4 -V
+ Perforce - The Fast Software Configuration Management System.
+ Copyright 1995-2011 Perforce Software. All rights reserved.
+ Rev. P4/NTX86/2011.1/393975 (2011/12/16).
+ """
+ global _p4_version_string
+ if not _p4_version_string:
+ a = p4_read_pipe_lines(["-V"])
+ _p4_version_string = a[-1].rstrip()
+ return _p4_version_string
+
def p4_integrate(src, dest):
p4_system(["integrate", "-Dt", wildcard_encode(src), wildcard_encode(dest)])
@@ -1973,7 +1989,6 @@ class P4Sync(Command, P4UserMap):
self.syncWithOrigin = True
self.importIntoRemotes = True
self.maxChanges = ""
- self.isWindows = (platform.system() == "Windows")
self.keepRepoPath = False
self.depotPaths = None
self.p4BranchesInGit = []
@@ -2118,7 +2133,14 @@ class P4Sync(Command, P4UserMap):
# operations. utf16 is converted to ascii or utf8, perhaps.
# But ascii text saved as -t utf16 is completely mangled.
# Invoke print -o to get the real contents.
+ #
+ # On windows, the newlines will always be mangled by print, so put
+ # them back too. This is not needed to the cygwin windows version,
+ # just the native "NT" type.
+ #
text = p4_read_pipe(['print', '-q', '-o', '-', file['depotFile']])
+ if p4_version_string().find("/NT") >= 0:
+ text = text.replace("\r\n", "\n")
contents = [ text ]
if type_base == "apple":
--
1.8.1.1.460.g6fa8886
^ permalink raw reply related
* [PATCHv2 11/21] git p4 test: newline handling
From: Pete Wyckoff @ 2013-01-27 3:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Sixt
In-Reply-To: <1359256284-5660-1-git-send-email-pw@padd.com>
P4 stores newlines in the depos as \n. By default, git does this
too, both on unix and windows. Test to make sure that this stays
true.
Both git and p4 have mechanisms to use \r\n in the working
directory. Exercise these.
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
t/t9802-git-p4-filetype.sh | 117 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 117 insertions(+)
diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index 21924df..c5ab626 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -8,6 +8,123 @@ test_expect_success 'start p4d' '
start_p4d
'
+#
+# This series of tests checks newline handling Both p4 and
+# git store newlines as \n, and have options to choose how
+# newlines appear in checked-out files.
+#
+test_expect_success 'p4 client newlines, unix' '
+ (
+ cd "$cli" &&
+ p4 client -o | sed "/LineEnd/s/:.*/:unix/" | p4 client -i &&
+ printf "unix\ncrlf\n" >f-unix &&
+ printf "unix\r\ncrlf\r\n" >f-unix-as-crlf &&
+ p4 add -t text f-unix &&
+ p4 submit -d f-unix &&
+
+ # LineEnd: unix; should be no change after sync
+ cp f-unix f-unix-orig &&
+ p4 sync -f &&
+ test_cmp f-unix-orig f-unix &&
+
+ # make sure stored in repo as unix newlines
+ # use sed to eat python-appened newline
+ p4 -G print //depot/f-unix | marshal_dump data 2 |\
+ sed \$d >f-unix-p4-print &&
+ test_cmp f-unix-orig f-unix-p4-print &&
+
+ # switch to win, make sure lf -> crlf
+ p4 client -o | sed "/LineEnd/s/:.*/:win/" | p4 client -i &&
+ p4 sync -f &&
+ test_cmp f-unix-as-crlf f-unix
+ )
+'
+
+test_expect_success 'p4 client newlines, win' '
+ (
+ cd "$cli" &&
+ p4 client -o | sed "/LineEnd/s/:.*/:win/" | p4 client -i &&
+ printf "win\r\ncrlf\r\n" >f-win &&
+ printf "win\ncrlf\n" >f-win-as-lf &&
+ p4 add -t text f-win &&
+ p4 submit -d f-win &&
+
+ # LineEnd: win; should be no change after sync
+ cp f-win f-win-orig &&
+ p4 sync -f &&
+ test_cmp f-win-orig f-win &&
+
+ # make sure stored in repo as unix newlines
+ # use sed to eat python-appened newline
+ p4 -G print //depot/f-win | marshal_dump data 2 |\
+ sed \$d >f-win-p4-print &&
+ test_cmp f-win-as-lf f-win-p4-print &&
+
+ # switch to unix, make sure lf -> crlf
+ p4 client -o | sed "/LineEnd/s/:.*/:unix/" | p4 client -i &&
+ p4 sync -f &&
+ test_cmp f-win-as-lf f-win
+ )
+'
+
+test_expect_success 'ensure blobs store only lf newlines' '
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ git init &&
+ git p4 sync //depot@all &&
+
+ # verify the files in .git are stored only with newlines
+ o=$(git ls-tree p4/master -- f-unix | cut -f1 | cut -d\ -f3) &&
+ git cat-file blob $o >f-unix-blob &&
+ test_cmp "$cli"/f-unix-orig f-unix-blob &&
+
+ o=$(git ls-tree p4/master -- f-win | cut -f1 | cut -d\ -f3) &&
+ git cat-file blob $o >f-win-blob &&
+ test_cmp "$cli"/f-win-as-lf f-win-blob &&
+
+ rm f-unix-blob f-win-blob
+ )
+'
+
+test_expect_success 'gitattributes setting eol=lf produces lf newlines' '
+ test_when_finished cleanup_git &&
+ (
+ # checkout the files and make sure core.eol works as planned
+ cd "$git" &&
+ git init &&
+ echo "* eol=lf" >.gitattributes &&
+ git p4 sync //depot@all &&
+ git checkout master &&
+ test_cmp "$cli"/f-unix-orig f-unix &&
+ test_cmp "$cli"/f-win-as-lf f-win
+ )
+'
+
+test_expect_success 'gitattributes setting eol=crlf produces crlf newlines' '
+ test_when_finished cleanup_git &&
+ (
+ # checkout the files and make sure core.eol works as planned
+ cd "$git" &&
+ git init &&
+ echo "* eol=crlf" >.gitattributes &&
+ git p4 sync //depot@all &&
+ git checkout master &&
+ test_cmp "$cli"/f-unix-as-crlf f-unix &&
+ test_cmp "$cli"/f-win-orig f-win
+ )
+'
+
+test_expect_success 'crlf cleanup' '
+ (
+ cd "$cli" &&
+ rm f-unix-orig f-unix-as-crlf &&
+ rm f-win-orig f-win-as-lf &&
+ p4 client -o | sed "/LineEnd/s/:.*/:unix/" | p4 client -i &&
+ p4 sync -f
+ )
+'
+
test_expect_success 'utf-16 file create' '
(
cd "$cli" &&
--
1.8.1.1.460.g6fa8886
^ permalink raw reply related
* Re: [PATCH v3 2/2] mergetools: Simplify how we handle "vim" and "defaults"
From: Junio C Hamano @ 2013-01-27 3:15 UTC (permalink / raw)
To: David Aguilar; +Cc: git, John Keeping
In-Reply-To: <1359247573-75825-1-git-send-email-davvid@gmail.com>
David Aguilar <davvid@gmail.com> writes:
> @@ -44,19 +46,9 @@ valid_tool () {
> }
>
> setup_tool () {
> - case "$1" in
> - vim*|gvim*)
> - tool=vim
> - ;;
> - *)
> - tool="$1"
> - ;;
> - esac
This part was an eyesore every time I looked at mergetools scripts.
Good riddance.
Is there still other special case like this, or was this the last
one?
Thanks, both of you, for working on this.
^ permalink raw reply
* [PATCHv2 12/21] git p4 test: use LineEnd unix in windows tests too
From: Pete Wyckoff @ 2013-01-27 3:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Sixt
In-Reply-To: <1359256284-5660-1-git-send-email-pw@padd.com>
In all clients, even those created on windows, use unix line
endings. This makes it possible to verify file contents without
doing OS-specific comparisons in all the tests.
Tests in t9802-git-p4-filetype.sh are used to make sure that
the other LineEnd options continue to work.
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
t/lib-git-p4.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index d5596de..67101b1 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -142,6 +142,7 @@ client_view() {
Description: $P4CLIENT
Root: $cli
AltRoots: $(native_path "$cli")
+ LineEnd: unix
View:
EOF
printf "\t%s\n" "$@"
--
1.8.1.1.460.g6fa8886
^ permalink raw reply related
* [PATCHv2 13/21] git p4 test: avoid wildcard * in windows
From: Pete Wyckoff @ 2013-01-27 3:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Sixt
In-Reply-To: <1359256284-5660-1-git-send-email-pw@padd.com>
This character is not valid in windows filenames, even though
it can appear in p4 depot paths. Avoid using it in tests on
windows, both mingw and cygwin.
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
t/t9809-git-p4-client-view.sh | 10 ++++++++--
t/t9812-git-p4-wildcards.sh | 37 +++++++++++++++++++++++++++++--------
2 files changed, 37 insertions(+), 10 deletions(-)
diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
index 0b58fb9..a911988 100755
--- a/t/t9809-git-p4-client-view.sh
+++ b/t/t9809-git-p4-client-view.sh
@@ -365,7 +365,10 @@ test_expect_success 'wildcard files submit back to p4, client-spec case' '
(
cd "$git" &&
echo git-wild-hash >dir1/git-wild#hash &&
- echo git-wild-star >dir1/git-wild\*star &&
+ if test_have_prereq NOT_MINGW NOT_CYGWIN
+ then
+ echo git-wild-star >dir1/git-wild\*star
+ fi &&
echo git-wild-at >dir1/git-wild@at &&
echo git-wild-percent >dir1/git-wild%percent &&
git add dir1/git-wild* &&
@@ -376,7 +379,10 @@ test_expect_success 'wildcard files submit back to p4, client-spec case' '
(
cd "$cli" &&
test_path_is_file dir1/git-wild#hash &&
- test_path_is_file dir1/git-wild\*star &&
+ if test_have_prereq NOT_MINGW NOT_CYGWIN
+ then
+ test_path_is_file dir1/git-wild\*star
+ fi &&
test_path_is_file dir1/git-wild@at &&
test_path_is_file dir1/git-wild%percent
) &&
diff --git a/t/t9812-git-p4-wildcards.sh b/t/t9812-git-p4-wildcards.sh
index 143d413..6763325 100755
--- a/t/t9812-git-p4-wildcards.sh
+++ b/t/t9812-git-p4-wildcards.sh
@@ -14,7 +14,10 @@ test_expect_success 'add p4 files with wildcards in the names' '
printf "file2\nhas\nsome\nrandom\ntext\n" >file2 &&
p4 add file2 &&
echo file-wild-hash >file-wild#hash &&
- echo file-wild-star >file-wild\*star &&
+ if test_have_prereq NOT_MINGW NOT_CYGWIN
+ then
+ echo file-wild-star >file-wild\*star
+ fi &&
echo file-wild-at >file-wild@at &&
echo file-wild-percent >file-wild%percent &&
p4 add -f file-wild* &&
@@ -28,7 +31,10 @@ test_expect_success 'wildcard files git p4 clone' '
(
cd "$git" &&
test -f file-wild#hash &&
- test -f file-wild\*star &&
+ if test_have_prereq NOT_MINGW NOT_CYGWIN
+ then
+ test -f file-wild\*star
+ fi &&
test -f file-wild@at &&
test -f file-wild%percent
)
@@ -40,7 +46,10 @@ test_expect_success 'wildcard files submit back to p4, add' '
(
cd "$git" &&
echo git-wild-hash >git-wild#hash &&
- echo git-wild-star >git-wild\*star &&
+ if test_have_prereq NOT_MINGW NOT_CYGWIN
+ then
+ echo git-wild-star >git-wild\*star
+ fi &&
echo git-wild-at >git-wild@at &&
echo git-wild-percent >git-wild%percent &&
git add git-wild* &&
@@ -51,7 +60,10 @@ test_expect_success 'wildcard files submit back to p4, add' '
(
cd "$cli" &&
test_path_is_file git-wild#hash &&
- test_path_is_file git-wild\*star &&
+ if test_have_prereq NOT_MINGW NOT_CYGWIN
+ then
+ test_path_is_file git-wild\*star
+ fi &&
test_path_is_file git-wild@at &&
test_path_is_file git-wild%percent
)
@@ -63,7 +75,10 @@ test_expect_success 'wildcard files submit back to p4, modify' '
(
cd "$git" &&
echo new-line >>git-wild#hash &&
- echo new-line >>git-wild\*star &&
+ if test_have_prereq NOT_MINGW NOT_CYGWIN
+ then
+ echo new-line >>git-wild\*star
+ fi &&
echo new-line >>git-wild@at &&
echo new-line >>git-wild%percent &&
git add git-wild* &&
@@ -74,7 +89,10 @@ test_expect_success 'wildcard files submit back to p4, modify' '
(
cd "$cli" &&
test_line_count = 2 git-wild#hash &&
- test_line_count = 2 git-wild\*star &&
+ if test_have_prereq NOT_MINGW NOT_CYGWIN
+ then
+ test_line_count = 2 git-wild\*star
+ fi &&
test_line_count = 2 git-wild@at &&
test_line_count = 2 git-wild%percent
)
@@ -87,7 +105,7 @@ test_expect_success 'wildcard files submit back to p4, copy' '
cd "$git" &&
cp file2 git-wild-cp#hash &&
git add git-wild-cp#hash &&
- cp git-wild\*star file-wild-3 &&
+ cp git-wild#hash file-wild-3 &&
git add file-wild-3 &&
git commit -m "wildcard copies" &&
git config git-p4.detectCopies true &&
@@ -134,7 +152,10 @@ test_expect_success 'wildcard files submit back to p4, delete' '
(
cd "$cli" &&
test_path_is_missing git-wild#hash &&
- test_path_is_missing git-wild\*star &&
+ if test_have_prereq NOT_MINGW NOT_CYGWIN
+ then
+ test_path_is_missing git-wild\*star
+ fi &&
test_path_is_missing git-wild@at &&
test_path_is_missing git-wild%percent
)
--
1.8.1.1.460.g6fa8886
^ permalink raw reply related
* [PATCHv2 14/21] git p4: cygwin p4 client does not mark read-only
From: Pete Wyckoff @ 2013-01-27 3:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Sixt
In-Reply-To: <1359256284-5660-1-git-send-email-pw@padd.com>
There are some old versions of p4, compiled for cygwin, that
treat read-only files differently.
Normally, a file that is not open is read-only, meaning that
"test -w" on the file is false. This works on unix, and it works
on windows using the NT version of p4. The cygwin version
of p4, though, changes the permissions, but does not set the
windows read-only attribute, so "test -w" returns false.
Notice this oddity and make the tests work, even on cygiwn.
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
t/lib-git-p4.sh | 13 +++++++++++++
t/t9807-git-p4-submit.sh | 14 ++++++++++++--
t/t9809-git-p4-client-view.sh | 4 ++--
3 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index 67101b1..2098b9b 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -148,3 +148,16 @@ client_view() {
printf "\t%s\n" "$@"
) | p4 client -i
}
+
+is_cli_file_writeable() {
+ # cygwin version of p4 does not set read-only attr,
+ # will be marked 444 but -w is true
+ file="$1" &&
+ if test_have_prereq CYGWIN && p4 -V | grep -q CYGWIN
+ then
+ stat=$(stat --format=%a "$file") &&
+ test $stat = 644
+ else
+ test -w "$file"
+ fi
+}
diff --git a/t/t9807-git-p4-submit.sh b/t/t9807-git-p4-submit.sh
index 0ae048f..1fb7bc7 100755
--- a/t/t9807-git-p4-submit.sh
+++ b/t/t9807-git-p4-submit.sh
@@ -17,6 +17,16 @@ test_expect_success 'init depot' '
)
'
+test_expect_failure 'is_cli_file_writeable function' '
+ (
+ cd "$cli" &&
+ echo a >a &&
+ is_cli_file_writeable a &&
+ ! is_cli_file_writeable file1 &&
+ rm a
+ )
+'
+
test_expect_success 'submit with no client dir' '
test_when_finished cleanup_git &&
git p4 clone --dest="$git" //depot &&
@@ -200,7 +210,7 @@ test_expect_success 'submit copy' '
(
cd "$cli" &&
test_path_is_file file5.ta &&
- test ! -w file5.ta
+ ! is_cli_file_writeable file5.ta
)
'
@@ -219,7 +229,7 @@ test_expect_success 'submit rename' '
cd "$cli" &&
test_path_is_missing file6.t &&
test_path_is_file file6.ta &&
- test ! -w file6.ta
+ ! is_cli_file_writeable file6.ta
)
'
diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
index a911988..77f6349 100755
--- a/t/t9809-git-p4-client-view.sh
+++ b/t/t9809-git-p4-client-view.sh
@@ -333,7 +333,7 @@ test_expect_success 'subdir clone, submit copy' '
(
cd "$cli" &&
test_path_is_file dir1/file11a &&
- test ! -w dir1/file11a
+ ! is_cli_file_writeable dir1/file11a
)
'
@@ -353,7 +353,7 @@ test_expect_success 'subdir clone, submit rename' '
cd "$cli" &&
test_path_is_missing dir1/file13 &&
test_path_is_file dir1/file13a &&
- test ! -w dir1/file13a
+ ! is_cli_file_writeable dir1/file13a
)
'
--
1.8.1.1.460.g6fa8886
^ permalink raw reply related
* [PATCHv2 15/21] git p4 test: use test_chmod for cygwin
From: Pete Wyckoff @ 2013-01-27 3:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Sixt
In-Reply-To: <1359256284-5660-1-git-send-email-pw@padd.com>
This test does a commit that is a pure mode change, submits
it to p4 but causes the submit to fail. It verifies that
the state in p4 as well as the client directory are both
unmodified after the failed submit.
On cygwin, "chmod +x" does nothing, so use the test_chmod
function to modify the index directly too.
Also on cygwin, the executable bit cannot be seen in the
filesystem, so avoid that part of the test. The checks of
p4 state are still valid, though.
Thanks-to: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
t/t9815-git-p4-submit-fail.sh | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/t/t9815-git-p4-submit-fail.sh b/t/t9815-git-p4-submit-fail.sh
index d2b7b3d..1243d96 100755
--- a/t/t9815-git-p4-submit-fail.sh
+++ b/t/t9815-git-p4-submit-fail.sh
@@ -405,8 +405,8 @@ test_expect_success 'cleanup chmod after submit cancel' '
git p4 clone --dest="$git" //depot &&
(
cd "$git" &&
- chmod u+x text &&
- chmod u-x text+x &&
+ test_chmod +x text &&
+ test_chmod -x text+x &&
git add text text+x &&
git commit -m "chmod texts" &&
echo n | test_expect_code 1 git p4 submit
@@ -415,10 +415,13 @@ test_expect_success 'cleanup chmod after submit cancel' '
cd "$cli" &&
test_path_is_file text &&
! p4 fstat -T action text &&
- stat --format=%A text | egrep ^-r-- &&
test_path_is_file text+x &&
! p4 fstat -T action text+x &&
- stat --format=%A text+x | egrep ^-r-x
+ if test_have_prereq NOT_CYGWIN
+ then
+ stat --format=%A text | egrep ^-r-- &&
+ stat --format=%A text+x | egrep ^-r-x
+ fi
)
'
--
1.8.1.1.460.g6fa8886
^ permalink raw reply related
* [PATCHv2 16/21] git p4: disable read-only attribute before deleting
From: Pete Wyckoff @ 2013-01-27 3:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Sixt
In-Reply-To: <1359256284-5660-1-git-send-email-pw@padd.com>
On windows, p4 marks un-edited files as read-only. Not only are
they read-only, but also they cannot be deleted. Remove the
read-only attribute before deleting in both the copy and rename
cases.
This also happens in the RCS cleanup code, where a file is marked
to be deleted, but must first be edited to remove adjust the
keyword lines. Make sure it is editable before patching.
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
git-p4.py | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/git-p4.py b/git-p4.py
index c62b2ca..a989704 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -21,6 +21,7 @@ import time
import platform
import re
import shutil
+import stat
verbose = False
@@ -1231,6 +1232,9 @@ class P4Submit(Command, P4UserMap):
p4_edit(dest)
pureRenameCopy.discard(dest)
filesToChangeExecBit[dest] = diff['dst_mode']
+ if self.isWindows:
+ # turn off read-only attribute
+ os.chmod(dest, stat.S_IWRITE)
os.unlink(dest)
editedFiles.add(dest)
elif modifier == "R":
@@ -1249,6 +1253,8 @@ class P4Submit(Command, P4UserMap):
p4_edit(dest) # with move: already open, writable
filesToChangeExecBit[dest] = diff['dst_mode']
if not self.p4HasMoveCommand:
+ if self.isWindows:
+ os.chmod(dest, stat.S_IWRITE)
os.unlink(dest)
filesToDelete.add(src)
editedFiles.add(dest)
@@ -1289,6 +1295,10 @@ class P4Submit(Command, P4UserMap):
for file in kwfiles:
if verbose:
print "zapping %s with %s" % (line,pattern)
+ # File is being deleted, so not open in p4. Must
+ # disable the read-only bit on windows.
+ if self.isWindows and file not in editedFiles:
+ os.chmod(file, stat.S_IWRITE)
self.patchRCSKeywords(file, kwfiles[file])
fixed_rcs_keywords = True
--
1.8.1.1.460.g6fa8886
^ permalink raw reply related
* [PATCHv2 17/21] git p4: avoid shell when mapping users
From: Pete Wyckoff @ 2013-01-27 3:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Sixt
In-Reply-To: <1359256284-5660-1-git-send-email-pw@padd.com>
The extra quoting and double-% are unneeded, just to work
around the shell. Instead, avoid the shell indirection.
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
git-p4.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/git-p4.py b/git-p4.py
index a989704..c43d044 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1050,7 +1050,8 @@ class P4Submit(Command, P4UserMap):
def p4UserForCommit(self,id):
# Return the tuple (perforce user,git email) for a given git commit id
self.getUserMapFromPerforceServer()
- gitEmail = read_pipe("git log --max-count=1 --format='%%ae' %s" % id)
+ gitEmail = read_pipe(["git", "log", "--max-count=1",
+ "--format=%ae", id])
gitEmail = gitEmail.strip()
if not self.emails.has_key(gitEmail):
return (None,gitEmail)
--
1.8.1.1.460.g6fa8886
^ permalink raw reply related
* [PATCHv2 18/21] git p4: avoid shell when invoking git rev-list
From: Pete Wyckoff @ 2013-01-27 3:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Sixt
In-Reply-To: <1359256284-5660-1-git-send-email-pw@padd.com>
Invoke git rev-list directly, avoiding the shell, in
P4Submit and P4Sync. The overhead of starting extra
processes is significant in cygwin; this speeds things
up on that platform.
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
git-p4.py | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index c43d044..c8ae83d 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1606,7 +1606,7 @@ class P4Submit(Command, P4UserMap):
self.check()
commits = []
- for line in read_pipe_lines("git rev-list --no-merges %s..%s" % (self.origin, self.master)):
+ for line in read_pipe_lines(["git", "rev-list", "--no-merges", "%s..%s" % (self.origin, self.master)]):
commits.append(line.strip())
commits.reverse()
@@ -2644,7 +2644,8 @@ class P4Sync(Command, P4UserMap):
def searchParent(self, parent, branch, target):
parentFound = False
- for blob in read_pipe_lines(["git", "rev-list", "--reverse", "--no-merges", parent]):
+ for blob in read_pipe_lines(["git", "rev-list", "--reverse",
+ "--no-merges", parent]):
blob = blob.strip()
if len(read_pipe(["git", "diff-tree", blob, target])) == 0:
parentFound = True
--
1.8.1.1.460.g6fa8886
^ permalink raw reply related
* [PATCHv2 19/21] git p4: avoid shell when invoking git config --get-all
From: Pete Wyckoff @ 2013-01-27 3:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Sixt
In-Reply-To: <1359256284-5660-1-git-send-email-pw@padd.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
git-p4.py | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/git-p4.py b/git-p4.py
index c8ae83d..7efa9a8 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -571,7 +571,8 @@ def gitConfig(key, args = None): # set args to "--bool", for instance
def gitConfigList(key):
if not _gitConfig.has_key(key):
- _gitConfig[key] = read_pipe("git config --get-all %s" % key, ignore_error=True).strip().split(os.linesep)
+ s = read_pipe(["git", "config", "--get-all", key], ignore_error=True)
+ _gitConfig[key] = s.strip().split(os.linesep)
return _gitConfig[key]
def p4BranchesInGit(branchesAreInRemotes=True):
--
1.8.1.1.460.g6fa8886
^ permalink raw reply related
* [PATCHv2 20/21] git p4: avoid shell when calling git config
From: Pete Wyckoff @ 2013-01-27 3:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Sixt
In-Reply-To: <1359256284-5660-1-git-send-email-pw@padd.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
git-p4.py | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index 7efa9a8..ff3e8c9 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -560,13 +560,16 @@ def gitBranchExists(branch):
return proc.wait() == 0;
_gitConfig = {}
-def gitConfig(key, args = None): # set args to "--bool", for instance
+
+def gitConfig(key, args=None): # set args to "--bool", for instance
if not _gitConfig.has_key(key):
- argsFilter = ""
- if args != None:
- argsFilter = "%s " % args
- cmd = "git config %s%s" % (argsFilter, key)
- _gitConfig[key] = read_pipe(cmd, ignore_error=True).strip()
+ cmd = [ "git", "config" ]
+ if args:
+ assert(args == "--bool")
+ cmd.append(args)
+ cmd.append(key)
+ s = read_pipe(cmd, ignore_error=True)
+ _gitConfig[key] = s.strip()
return _gitConfig[key]
def gitConfigList(key):
--
1.8.1.1.460.g6fa8886
^ permalink raw reply related
* [PATCHv2 21/21] git p4: introduce gitConfigBool
From: Pete Wyckoff @ 2013-01-27 3:11 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Johannes Sixt
In-Reply-To: <1359256284-5660-1-git-send-email-pw@padd.com>
Make the intent of "--bool" more obvious by returning a direct True
or False value. Convert a couple non-bool users with obvious bool
intent.
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
git-p4.py | 45 ++++++++++++++++++++++++++-------------------
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index ff3e8c9..955a5dd 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -561,17 +561,25 @@ def gitBranchExists(branch):
_gitConfig = {}
-def gitConfig(key, args=None): # set args to "--bool", for instance
+def gitConfig(key):
if not _gitConfig.has_key(key):
- cmd = [ "git", "config" ]
- if args:
- assert(args == "--bool")
- cmd.append(args)
- cmd.append(key)
+ cmd = [ "git", "config", key ]
s = read_pipe(cmd, ignore_error=True)
_gitConfig[key] = s.strip()
return _gitConfig[key]
+def gitConfigBool(key):
+ """Return a bool, using git config --bool. It is True only if the
+ variable is set to true, and False if set to false or not present
+ in the config."""
+
+ if not _gitConfig.has_key(key):
+ cmd = [ "git", "config", "--bool", key ]
+ s = read_pipe(cmd, ignore_error=True)
+ v = s.strip()
+ _gitConfig[key] = v == "true"
+ return _gitConfig[key]
+
def gitConfigList(key):
if not _gitConfig.has_key(key):
s = read_pipe(["git", "config", "--get-all", key], ignore_error=True)
@@ -722,8 +730,7 @@ def p4PathStartsWith(path, prefix):
#
# we may or may not have a problem. If you have core.ignorecase=true,
# we treat DirA and dira as the same directory
- ignorecase = gitConfig("core.ignorecase", "--bool") == "true"
- if ignorecase:
+ if gitConfigBool("core.ignorecase"):
return path.lower().startswith(prefix.lower())
return path.startswith(prefix)
@@ -959,7 +966,7 @@ class P4Submit(Command, P4UserMap):
self.usage += " [name of git branch to submit into perforce depot]"
self.origin = ""
self.detectRenames = False
- self.preserveUser = gitConfig("git-p4.preserveUser").lower() == "true"
+ self.preserveUser = gitConfigBool("git-p4.preserveUser")
self.dry_run = False
self.prepare_p4_only = False
self.conflict_behavior = None
@@ -1068,7 +1075,7 @@ class P4Submit(Command, P4UserMap):
(user,email) = self.p4UserForCommit(id)
if not user:
msg = "Cannot find p4 user for email %s in commit %s." % (email, id)
- if gitConfig('git-p4.allowMissingP4Users').lower() == "true":
+ if gitConfigBool("git-p4.allowMissingP4Users"):
print "%s" % msg
else:
die("Error: %s\nSet git-p4.allowMissingP4Users to true to allow this." % msg)
@@ -1163,7 +1170,7 @@ class P4Submit(Command, P4UserMap):
message. Return true if okay to continue with the submit."""
# if configured to skip the editing part, just submit
- if gitConfig("git-p4.skipSubmitEdit") == "true":
+ if gitConfigBool("git-p4.skipSubmitEdit"):
return True
# look at the modification time, to check later if the user saved
@@ -1179,7 +1186,7 @@ class P4Submit(Command, P4UserMap):
# If the file was not saved, prompt to see if this patch should
# be skipped. But skip this verification step if configured so.
- if gitConfig("git-p4.skipSubmitEditCheck") == "true":
+ if gitConfigBool("git-p4.skipSubmitEditCheck"):
return True
# modification time updated means user saved the file
@@ -1279,7 +1286,7 @@ class P4Submit(Command, P4UserMap):
# Patch failed, maybe it's just RCS keyword woes. Look through
# the patch to see if that's possible.
- if gitConfig("git-p4.attemptRCSCleanup","--bool") == "true":
+ if gitConfigBool("git-p4.attemptRCSCleanup"):
file = None
pattern = None
kwfiles = {}
@@ -1574,7 +1581,7 @@ class P4Submit(Command, P4UserMap):
sys.exit(128)
self.useClientSpec = False
- if gitConfig("git-p4.useclientspec", "--bool") == "true":
+ if gitConfigBool("git-p4.useclientspec"):
self.useClientSpec = True
if self.useClientSpec:
self.clientSpecDirs = getClientSpec()
@@ -1614,7 +1621,7 @@ class P4Submit(Command, P4UserMap):
commits.append(line.strip())
commits.reverse()
- if self.preserveUser or (gitConfig("git-p4.skipUserNameCheck") == "true"):
+ if self.preserveUser or gitConfigBool("git-p4.skipUserNameCheck"):
self.checkAuthorship = False
else:
self.checkAuthorship = True
@@ -1650,7 +1657,7 @@ class P4Submit(Command, P4UserMap):
else:
self.diffOpts += " -C%s" % detectCopies
- if gitConfig("git-p4.detectCopiesHarder", "--bool") == "true":
+ if gitConfigBool("git-p4.detectCopiesHarder"):
self.diffOpts += " --find-copies-harder"
#
@@ -1734,7 +1741,7 @@ class P4Submit(Command, P4UserMap):
"--format=format:%h %s", c])
print "You will have to do 'git p4 sync' and rebase."
- if gitConfig("git-p4.exportLabels", "--bool") == "true":
+ if gitConfigBool("git-p4.exportLabels"):
self.exportLabels = True
if self.exportLabels:
@@ -2834,7 +2841,7 @@ class P4Sync(Command, P4UserMap):
# will use this after clone to set the variable
self.useClientSpec_from_options = True
else:
- if gitConfig("git-p4.useclientspec", "--bool") == "true":
+ if gitConfigBool("git-p4.useclientspec"):
self.useClientSpec = True
if self.useClientSpec:
self.clientSpecDirs = getClientSpec()
@@ -3074,7 +3081,7 @@ class P4Sync(Command, P4UserMap):
sys.stdout.write("%s " % b)
sys.stdout.write("\n")
- if gitConfig("git-p4.importLabels", "--bool") == "true":
+ if gitConfigBool("git-p4.importLabels"):
self.importLabels = True
if self.importLabels:
--
1.8.1.1.460.g6fa8886
^ permalink raw reply related
* Re: [PATCH v3 2/2] mergetools: Simplify how we handle "vim" and "defaults"
From: David Aguilar @ 2013-01-27 3:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, John Keeping
In-Reply-To: <7vobgbz58a.fsf@alter.siamese.dyndns.org>
On Sat, Jan 26, 2013 at 7:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> @@ -44,19 +46,9 @@ valid_tool () {
>> }
>>
>> setup_tool () {
>> - case "$1" in
>> - vim*|gvim*)
>> - tool=vim
>> - ;;
>> - *)
>> - tool="$1"
>> - ;;
>> - esac
>
> This part was an eyesore every time I looked at mergetools scripts.
> Good riddance.
>
> Is there still other special case like this, or was this the last
> one?
>
> Thanks, both of you, for working on this.
I believe that was the last special case. :-) It should be easier
to auto-generate a list of tools for use in the documentation now.
That'll be the the next topic I look into.
I have a question. John mentioned that we can replace the use of
"$(..)" with $(..) in shell.
I have a trivial style patches to replace "$(..)" with $(..)
sitting uncommitted in my tree for mergetool--lib.
Grepping the rest of the tree shows that there are many
occurrences of the "$(..)" idiom in the shell code.
Is this a general rule that should be in CodingStyle,
or is it better left as-is? Are there cases where DQ should
be used around $(..)? My understanding is "no", but I don't
know whether that is correct.
Doing the documentation stuff is more important IMO so I probably
shouldn't let myself get too distracted by it, though I am curious.
--
David
^ permalink raw reply
* Re: [PATCH v3 2/2] mergetools: Simplify how we handle "vim" and "defaults"
From: Junio C Hamano @ 2013-01-27 4:25 UTC (permalink / raw)
To: David Aguilar; +Cc: git, John Keeping
In-Reply-To: <CAJDDKr7E1NkMV0_G+6oY9O3iCS9OCEzR1HYssKpwh77swDUcig@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
> I have a question. John mentioned that we can replace the use of
> "$(..)" with $(..) in shell.
I think it isn't so much about $(...) but more about quoting the RHS
of assignment.
Consider these:
var="$another_var"
var="$(command)"
var="$one_var$two_var"
var="$another_var$(command)"
all of which _can_ be done without dq around the RHS, because the
result of variable substitution and command substitution will not be
subject to further word splitting.
I however find it easier to read with dq around the latter two, i.e.
substitution and then concatenation of the result of substitution.
The extra dq makes the intent of the author clearer, especially
while reviewing a script written by other people whose understanding
of the syntax I am not sure about ;-). Between var=$another and
var="$another", the latter is slightly preferrable for the same
reason.
One questionable case is:
var=$(command "with quoted parameter")
It makes it a bit harder to scan if there is an extra set of dq
around RHS, i.e.
var="$(command "with quoted parameter")"
That case is easier to read without dq around it, or you could cheat
by doing this:
var="$(command 'with quoted parameter')"
In any case, as long as the result is correct, I do not have very
strong preference either way.
^ permalink raw reply
* Re: [PATCH v3 6/8] git-remote-testpy: hash bytes explicitly
From: Michael Haggerty @ 2013-01-27 4:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Keeping, git, Sverre Rabbelier
In-Reply-To: <7vwquzzkiw.fsf@alter.siamese.dyndns.org>
On 01/26/2013 10:44 PM, Junio C Hamano wrote:
> John Keeping <john@keeping.me.uk> writes:
>
>> Junio, can you replace the queued 0846b0c (git-remote-testpy: hash bytes
>> explicitly) with this?
>>
>> I hadn't realised that the "hex" encoding we chose before is a "bytes to
>> bytes" encoding so it just fails with an error on Python 3 in the same
>> way as the original code.
>>
>> Since we want to convert a Unicode string to bytes I think UTF-8 really
>> is the best option here.
>
> Ahh. I think it is already in "next", so this needs to be turned
> into an incremental to flip 'hex' to 'utf-8', with the justification
> being these five lines above.
>
> Thanks for catching.
>
>>
>> git-remote-testpy.py | 8 ++++----
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/git-remote-testpy.py b/git-remote-testpy.py
>> index d94a66a..f8dc196 100644
>> --- a/git-remote-testpy.py
>> +++ b/git-remote-testpy.py
>> @@ -31,9 +31,9 @@ from git_remote_helpers.git.exporter import GitExporter
>> from git_remote_helpers.git.importer import GitImporter
>> from git_remote_helpers.git.non_local import NonLocalGit
>>
>> -if sys.hexversion < 0x01050200:
>> - # os.makedirs() is the limiter
>> - sys.stderr.write("git-remote-testgit: requires Python 1.5.2 or later.\n")
>> +if sys.hexversion < 0x02000000:
>> + # string.encode() is the limiter
>> + sys.stderr.write("git-remote-testgit: requires Python 2.0 or later.\n")
>> sys.exit(1)
>>
>> def get_repo(alias, url):
>> @@ -45,7 +45,7 @@ def get_repo(alias, url):
>> repo.get_head()
>>
>> hasher = _digest()
>> - hasher.update(repo.path)
>> + hasher.update(repo.path.encode('utf-8'))
>> repo.hash = hasher.hexdigest()
>>
>> repo.get_base_path = lambda base: os.path.join(
This will still fail under Python 2.x if repo.path is a byte string that
contains non-ASCII characters. And it will fail under Python 3.1 and
later if repo.path contains characters using the surrogateescape
encoding option [1], as it will if the original command-line argument
contained bytes that cannot be decoded into Unicode using the user's
default encoding:
$ python3 --version
Python 3.2.3
$ python3 -c "
import sys
print(repr(sys.argv[1]))
print(repr(sys.argv[1].encode('utf-8')))
" $(echo français|iconv -t latin1)
'fran\udce7ais'
Traceback (most recent call last):
File "<string>", line 4, in <module>
UnicodeEncodeError: 'utf-8' codec can't encode character '\udce7' in
position 4: surrogates not allowed
I'm not sure what happens in Python 3.0.
I think the "modern" way to handle this situation in Python 3.1+ is via
PEP 383's surrogateescape encoding option [1]:
repo.path.encode('utf-8', 'surrogateescape')
Basically, byte strings that come from the OS are automatically decoded
into Unicode strings using
s = b.decode(sys.getfilesystemencoding(), 'surrogateescape')
If the string needs to be passed back to the filesystem as a byte string
it is via
b = s.encode(sys.getfilesystemencoding(), 'surrogateescape')
My understanding is that the surrogateescape mechanism guarantees that
the round-trip bytestring -> string -> bytestring gives back the
original byte string, which is what you want for things like filenames.
But a Unicode string that contains surrogate escape characters *cannot*
be encoded without the 'surrogateescape' option.
'surrogateescape' is not supported in Python 3.0, but I think it would
be quite acceptable only to support Python 3.x for x >= 1.
But 'surrogateescape' doesn't seem to be supported at all in Python 2.x
(I tested 2.7.3 and it's not there).
Here you don't really need byte-for-byte correctness; it would be enough
to get *some* byte string that is unique for a given input (ideally,
consistent with ASCII or UTF-8 for backwards compatibility). So you
could use
b = s.encode('utf-8', 'backslashreplace')
Unfortunately, this doesn't work under Python 2.x:
$ python2 -c "
import sys
print(repr(sys.argv[1]))
print(repr(sys.argv[1].encode('utf-8', 'backslashreplace')))
" $(echo français|iconv -t latin1)
'fran\xe7ais'
Traceback (most recent call last):
File "<string>", line 4, in <module>
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe7 in position
4: ordinal not in range(128)
Apparently when you call bytestring.encode(), Python first tries to
decode the string to Unicode using the 'ascii' encoding.
So to handle all of the cases across Python versions as closely as
possible to the old 2.x code, it might be necessary to make the code
explicitly depend on the Python version number, like:
hasher = _digest()
if sys.hexversion < 0x03000000:
pathbytes = repo.path
elif sys.hexversion < 0x03010000:
# If support for Python 3.0.x is desired (note: result can
# be different in this case than under 2.x or 3.1+):
pathbytes = repo.path.encode(sys.getfilesystemencoding(),
'backslashreplace')
else
pathbytes = repo.path.encode(sys.getfilesystemencoding(),
'surrogateescape')
hasher.update(pathbytes)
repo.hash = hasher.hexdigest()
Michael
[1] http://www.python.org/dev/peps/pep-0383/
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
From: Junio C Hamano @ 2013-01-27 4:57 UTC (permalink / raw)
To: John Keeping; +Cc: David Aguilar, git
In-Reply-To: <20130126121202.GH7498@serenity.lan>
John Keeping <john@keeping.me.uk> writes:
> I'm not sure creating an 'include' directory actually buys us much over
> declaring that 'vimdiff' is the real script and the others just source
> it.
Is 'include' really used for such a purpose? It only houses defaults.sh
as far as I can see.
As that defaults.sh file is used only to define trivially empty
shell functions, I wonder if it is better to get rid of it, and
define these functions in line in git-mergetool--lib.sh. Such a
change would like the attached on top of the entire series.
Makefile | 6 +-----
git-mergetool--lib.sh | 24 ++++++++++++++++++++++--
mergetools/include/defaults.sh | 22 ----------------------
3 files changed, 23 insertions(+), 29 deletions(-)
diff --git a/Makefile b/Makefile
index 26f217f..f69979e 100644
--- a/Makefile
+++ b/Makefile
@@ -2724,11 +2724,7 @@ install: all
$(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
- $(INSTALL) -m 644 $(filter-out mergetools/include,$(wildcard mergetools/*)) \
- '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
- $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'
- $(INSTALL) -m 644 mergetools/include/* \
- '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'
+ $(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
ifndef NO_GETTEXT
$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'
(cd po/build/locale && $(TAR) cf - .) | \
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 7ea7510..1d0fb12 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -57,8 +57,28 @@ setup_tool () {
return 2
fi
- # Load the default functions
- . "$MERGE_TOOLS_DIR/include/defaults.sh"
+ # Fallback definitions, to be overriden by tools.
+ can_merge () {
+ return 0
+ }
+
+ can_diff () {
+ return 0
+ }
+
+ diff_cmd () {
+ status=1
+ return $status
+ }
+
+ merge_cmd () {
+ status=1
+ return $status
+ }
+
+ translate_merge_tool_path () {
+ echo "$1"
+ }
# Load the redefined functions
. "$MERGE_TOOLS_DIR/$tool"
diff --git a/mergetools/include/defaults.sh b/mergetools/include/defaults.sh
deleted file mode 100644
index 21e63ec..0000000
--- a/mergetools/include/defaults.sh
+++ /dev/null
@@ -1,22 +0,0 @@
-# Redefined by builtin tools
-can_merge () {
- return 0
-}
-
-can_diff () {
- return 0
-}
-
-diff_cmd () {
- status=1
- return $status
-}
-
-merge_cmd () {
- status=1
- return $status
-}
-
-translate_merge_tool_path () {
- echo "$1"
-}
^ permalink raw reply related
* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
From: David Aguilar @ 2013-01-27 5:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Keeping, git
In-Reply-To: <7v8v7fz0ii.fsf@alter.siamese.dyndns.org>
On Sat, Jan 26, 2013 at 8:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> John Keeping <john@keeping.me.uk> writes:
>
>> I'm not sure creating an 'include' directory actually buys us much over
>> declaring that 'vimdiff' is the real script and the others just source
>> it.
>
> Is 'include' really used for such a purpose? It only houses defaults.sh
> as far as I can see.
>
> As that defaults.sh file is used only to define trivially empty
> shell functions, I wonder if it is better to get rid of it, and
> define these functions in line in git-mergetool--lib.sh. Such a
> change would like the attached on top of the entire series.
I think that's much better.
> Makefile | 6 +-----
> git-mergetool--lib.sh | 24 ++++++++++++++++++++++--
> mergetools/include/defaults.sh | 22 ----------------------
> 3 files changed, 23 insertions(+), 29 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 26f217f..f69979e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2724,11 +2724,7 @@ install: all
> $(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
> $(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
> $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
> - $(INSTALL) -m 644 $(filter-out mergetools/include,$(wildcard mergetools/*)) \
> - '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
> - $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'
> - $(INSTALL) -m 644 mergetools/include/* \
> - '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'
> + $(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
> ifndef NO_GETTEXT
> $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'
> (cd po/build/locale && $(TAR) cf - .) | \
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 7ea7510..1d0fb12 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -57,8 +57,28 @@ setup_tool () {
> return 2
> fi
>
> - # Load the default functions
> - . "$MERGE_TOOLS_DIR/include/defaults.sh"
> + # Fallback definitions, to be overriden by tools.
> + can_merge () {
> + return 0
> + }
> +
> + can_diff () {
> + return 0
> + }
> +
> + diff_cmd () {
> + status=1
> + return $status
> + }
> +
> + merge_cmd () {
> + status=1
> + return $status
> + }
> +
> + translate_merge_tool_path () {
> + echo "$1"
> + }
>
> # Load the redefined functions
> . "$MERGE_TOOLS_DIR/$tool"
> diff --git a/mergetools/include/defaults.sh b/mergetools/include/defaults.sh
> deleted file mode 100644
> index 21e63ec..0000000
> --- a/mergetools/include/defaults.sh
> +++ /dev/null
> @@ -1,22 +0,0 @@
> -# Redefined by builtin tools
> -can_merge () {
> - return 0
> -}
> -
> -can_diff () {
> - return 0
> -}
> -
> -diff_cmd () {
> - status=1
> - return $status
> -}
> -
> -merge_cmd () {
> - status=1
> - return $status
> -}
> -
> -translate_merge_tool_path () {
> - echo "$1"
> -}
--
David
^ permalink raw reply
* Re: [PATCH v3 6/8] git-remote-testpy: hash bytes explicitly
From: Junio C Hamano @ 2013-01-27 5:30 UTC (permalink / raw)
To: Michael Haggerty; +Cc: John Keeping, git, Sverre Rabbelier
In-Reply-To: <5104B0B5.1030501@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> This will still fail under Python 2.x if repo.path is a byte string that
> contains non-ASCII characters. And it will fail under Python 3.1 and
> later if repo.path contains characters using the surrogateescape
> encoding option [1],...
> Here you don't really need byte-for-byte correctness; it would be enough
> to get *some* byte string that is unique for a given input ...
Yeek.
As we do not care about the actual value at all, how about doing
something like this instead?
git-remote-testgit.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index 5f3ebd2..705750d 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -40,7 +40,7 @@ def get_repo(alias, url):
repo.get_head()
hasher = _digest()
- hasher.update(repo.path)
+ hasher.update(".".join([str(ord(c)) for c in repo.path]))
repo.hash = hasher.hexdigest()
repo.get_base_path = lambda base: os.path.join(
^ permalink raw reply related
* Re: [PATCH v3 6/8] git-remote-testpy: hash bytes explicitly
From: Sverre Rabbelier @ 2013-01-27 5:30 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Junio C Hamano, John Keeping, Git List
In-Reply-To: <5104B0B5.1030501@alum.mit.edu>
On Sat, Jan 26, 2013 at 8:44 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> So to handle all of the cases across Python versions as closely as
> possible to the old 2.x code, it might be necessary to make the code
> explicitly depend on the Python version number, like:
Does this all go away if we restrict ourselves to python 2.6 and just
use the b prefix?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH 0/3] lazily load commit->buffer
From: Junio C Hamano @ 2013-01-27 5:32 UTC (permalink / raw)
To: Jeff King
Cc: Jonathon Mah, Jonathan Nieder, Duy Nguyen, Stefan Näwe,
Armin, git@vger.kernel.org
In-Reply-To: <20130126221400.GA13827@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> My HEAD has about 400/3000 non-unique commits, which matches your
> numbers percentage-wise. Dropping the lines above (and always freeing)
> takes my best-of-five for "git log -g" from 0.085s to 0.080s. Which is
> well within the noise. Doing "git log -g Makefile" ended up at 0.183s
> both before and after.
>
> ... I'd be in favor of
> dropping the lines just to decrease complexity of the code.
I think we are in agreement, then.
^ permalink raw reply
* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
From: David Aguilar @ 2013-01-27 5:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: John Keeping, git
In-Reply-To: <CAJDDKr5cCbNi5q5_Ds-yohXR56ZfVs7YBTgJP3THjRx1=EgG9w@mail.gmail.com>
On Sat, Jan 26, 2013 at 9:07 PM, David Aguilar <davvid@gmail.com> wrote:
> On Sat, Jan 26, 2013 at 8:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> John Keeping <john@keeping.me.uk> writes:
>>
>>> I'm not sure creating an 'include' directory actually buys us much over
>>> declaring that 'vimdiff' is the real script and the others just source
>>> it.
>>
>> Is 'include' really used for such a purpose? It only houses defaults.sh
>> as far as I can see.
>>
>> As that defaults.sh file is used only to define trivially empty
>> shell functions, I wonder if it is better to get rid of it, and
>> define these functions in line in git-mergetool--lib.sh. Such a
>> change would like the attached on top of the entire series.
>
> I think that's much better.
Would you like me to put this together into a proper patch?
You can also squash it in (along with a removal of the
last line of the commit message) if you prefer.
>> Makefile | 6 +-----
>> git-mergetool--lib.sh | 24 ++++++++++++++++++++++--
>> mergetools/include/defaults.sh | 22 ----------------------
>> 3 files changed, 23 insertions(+), 29 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 26f217f..f69979e 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2724,11 +2724,7 @@ install: all
>> $(INSTALL) $(install_bindir_programs) '$(DESTDIR_SQ)$(bindir_SQ)'
>> $(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
>> $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
>> - $(INSTALL) -m 644 $(filter-out mergetools/include,$(wildcard mergetools/*)) \
>> - '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
>> - $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'
>> - $(INSTALL) -m 644 mergetools/include/* \
>> - '$(DESTDIR_SQ)$(mergetools_instdir_SQ)/include'
>> + $(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)'
>> ifndef NO_GETTEXT
>> $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'
>> (cd po/build/locale && $(TAR) cf - .) | \
>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
>> index 7ea7510..1d0fb12 100644
>> --- a/git-mergetool--lib.sh
>> +++ b/git-mergetool--lib.sh
>> @@ -57,8 +57,28 @@ setup_tool () {
>> return 2
>> fi
>>
>> - # Load the default functions
>> - . "$MERGE_TOOLS_DIR/include/defaults.sh"
>> + # Fallback definitions, to be overriden by tools.
>> + can_merge () {
>> + return 0
>> + }
>> +
>> + can_diff () {
>> + return 0
>> + }
>> +
>> + diff_cmd () {
>> + status=1
>> + return $status
>> + }
>> +
>> + merge_cmd () {
>> + status=1
>> + return $status
>> + }
>> +
>> + translate_merge_tool_path () {
>> + echo "$1"
>> + }
>>
>> # Load the redefined functions
>> . "$MERGE_TOOLS_DIR/$tool"
>> diff --git a/mergetools/include/defaults.sh b/mergetools/include/defaults.sh
>> deleted file mode 100644
>> index 21e63ec..0000000
>> --- a/mergetools/include/defaults.sh
>> +++ /dev/null
>> @@ -1,22 +0,0 @@
>> -# Redefined by builtin tools
>> -can_merge () {
>> - return 0
>> -}
>> -
>> -can_diff () {
>> - return 0
>> -}
>> -
>> -diff_cmd () {
>> - status=1
>> - return $status
>> -}
>> -
>> -merge_cmd () {
>> - status=1
>> - return $status
>> -}
>> -
>> -translate_merge_tool_path () {
>> - echo "$1"
>> -}
--
David
^ permalink raw reply
* Re: [PATCH] mergetools: Simplify how we handle "vim" and "defaults"
From: Junio C Hamano @ 2013-01-27 6:05 UTC (permalink / raw)
To: David Aguilar; +Cc: John Keeping, git
In-Reply-To: <CAJDDKr692zbg+PiFWx1y81yn=s2e=C0pFhsup4z0uTRNOTMPwg@mail.gmail.com>
David Aguilar <davvid@gmail.com> writes:
>> I think that's much better.
>
> Would you like me to put this together into a proper patch?
>
> You can also squash it in (along with a removal of the
> last line of the commit message) if you prefer.
I was lazy and didn't want to think about rewriting your log
message, so I just queued it with this log message on top.
mergetools: remove mergetools/include/defaults.sh
This and its containing directory are used only to define trivial
fallback definition of five shell functions in a single script.
Defining them in-line at the location that wants to have them makes
the result much easier to read.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
But as you said, removing the last line of your log message and
squashing the change into it would be more preferrable. Let me do
that later.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/2] optimizing pack access on "read only" fetch repos
From: Junio C Hamano @ 2013-01-27 6:32 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20130126224011.GA20675@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This is a repost from here:
>
> http://thread.gmane.org/gmane.comp.version-control.git/211176
>
> which got no response initially. Basically the issue is that read-only
> repos (e.g., a CI server) whose workflow is something like:
>
> git fetch $some_branch &&
> git checkout -f $some_branch &&
> make test
>
> will never run git-gc, and will accumulate a bunch of small packs and
> loose objects, leading to poor performance.
>
> Patch 1 runs "gc --auto" on fetch, which I think is sane to do.
>
> Patch 2 optimizes our pack dir re-scanning for fetch-pack (which, unlike
> the rest of git, should expect to be missing lots of objects, since we
> are deciding what to fetch).
>
> I think 1 is a no-brainer. If your repo is packed, patch 2 matters less,
> but it still seems like a sensible optimization to me.
>
> [1/2]: fetch: run gc --auto after fetching
> [2/2]: fetch-pack: avoid repeatedly re-scanning pack directory
>
> -Peff
Both makes sense to me.
I also wonder if we would be helped by another "repack" mode that
coalesces small packs into a single one with minimum overhead, and
run that often from "gc --auto", so that we do not end up having to
have 50 packfiles.
When we have 2 or more small and young packs, we could:
- iterate over idx files for these packs to enumerate the objects
to be packed, replacing read_object_list_from_stdin() step;
- always choose to copy the data we have in these existing packs,
instead of doing a full prepare_pack(); and
- use the order the objects appear in the original packs, bypassing
compute_write_order().
The procedure cannot be a straight byte-for-byte copy, because some
objects may appear in multiple packs, and extra copies of the same
object have to be excised from the result. OFS_DELTA offsets need
to be adjusted for objects that appear later in the output and for
objects that were deltified against such an object that recorded its
base with OFS_DELTA format.
But other than such OFS_DELTA adjustments, it feels that such an
"only coalesce multiple packs into one" mode should be fairly quick.
^ permalink raw reply
* Re: [PATCH] tests: turn on test-lint-shell-syntax by default
From: Torsten Bögershausen @ 2013-01-27 7:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Torsten Bögershausen, Jonathan Nieder, git, kraai
In-Reply-To: <7v1ud71uys.fsf@alter.siamese.dyndns.org>
On 26.01.13 22:43, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> Do we really need "which" to detect if frotz is installed?
> I think we all know the answer to that question is no, but why is
> that a relevant question in the context of this discussion? One of
> us may be very confused.
>
> I thought the topic of this discussion was that, already knowing
> that "which" should never be used anywhere in our scripts, you are
> trying to devise a mechanical way to catch newcomers' attempts to
> use it in their changes, in order to prevent patches that add use of
> "which" to be sent for review to waste our time. I was illustrating
> that the approach to override "which" in a shell function for test
> scripts will not be a useful solution for that goal.
Yes, the diskussion went away.
I would rather see the check-non-portable-shell.pl enabled per default.
It looks as if the "which" command is hard to find, when we want a minimal
risk of false positves.
I can see different solutions:
1) We can make a much simpler expression which only catches the most
common usage of which, like
"if whitch foo".
This will not catch lines like
if test -x "$(which foo 2>/dev/null)"
But I think the -x is not a useful anyway, because which should only
list command which have the executable bit set.
2) We drop the which from check-non-portable-shell.pl
I'll send a patch for 1)
/Torsten
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox