Re: [ANNOUNCE] Git v2.0.0-rc4
On Tue, May 20, 2014 at 05:24:50PM -0700, Junio C Hamano wrote: (...) * git reset learned the -N option, which does not reset the index fully for paths the index knows about but the tree-ish the command resets to does not (these paths are kept as intend-to-add entries). I find it quite hard to parse this sentance. Maybe something like: which keeps paths as intent-to-add entries if they are currently staged, but not present in the tree-ish being reset to. would be clearer (I hope I've actually managed to understand it..)? (...) * Commands that take pathspecs on the command line misbehaved when the pathspec is given as an absolute pathname (which is a practice not particularly encouraged) that points at a symbolic link in the working tree. (merge later 655ee9e mw/symlinks to maint.) In order to include the latest cleanup to this patchset: setup: fix windows path buffer over-stepping this should be 6127ff6 instead. Sorry if it's unneeded to note, but just wanted to make sure :) -- Martin Erik Werner martinerikwer...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] setup: Fix windows path buffer over-stepping
Fix a buffer over-stepping issue triggered by providing an absolute path
that is similar to the work tree path.
abspath_part_inside_repo() may currently increment the path pointer by
offset_1st_component() + wtlen, which is too much, since
offset_1st_component() is a subset of wtlen.
For the *nix-style prefix '/', this does (by luck) not cause any issues,
since offset_1st_component() is 1 and there will always be a '/' or '\0'
that can absorb this.
In the case of DOS-style prefixes though, the offset_1st_component() is
3 and this can potentially over-step the string buffer. For example if
work_tree = c:/r
path = c:/rl
Then wtlen is 4, and incrementing the path pointer by (3 + 4) would
end up 2 bytes outside a string buffer of length 6.
Similarly if
work_tree = c:/r
path = c:/rl/d/a
Then (since the loop starts by also incrementing the pointer one step),
this would mean that the function would miss checking if c:/rl/d could
be the work_tree, arguably this is unlikely though, since it would only
be possible with symlinks on windows.
Fix this by simply avoiding to increment by offset_1st_component() and
wtlen at the same time.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
---
This is a follow-up on 655ee9e mw/symlinks which is currently merged into
master, prospective for git v2.0.0, the issue only affects v2.0.0-rc0.
setup.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/setup.c b/setup.c
index 613e3b3..0a22f8b 100644
--- a/setup.c
+++ b/setup.c
@@ -29,7 +29,7 @@ static int abspath_part_inside_repo(char *path)
return -1;
wtlen = strlen(work_tree);
len = strlen(path);
- off = 0;
+ off = offset_1st_component(path);
/* check if work tree is already the prefix */
if (wtlen = len !strncmp(path, work_tree, wtlen)) {
@@ -45,7 +45,7 @@ static int abspath_part_inside_repo(char *path)
off = wtlen;
}
path0 = path;
- path += offset_1st_component(path) + off;
+ path += off;
/* check each '/'-terminated level */
while (*path) {
--
1.9.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[BUG] t9151: Unreliable test/test setup
Hi, It appears that the last test in t9151-svn-mergeinfo.sh: test_expect_failure 'everything got merged in the end' ' unmerged=$(git rev-list --all --not master) [ -z $unmerged ] ' reports known breakage or breakage vanished seemingly at random: $ while true; do (cd t sh t9151-svn-mergeinfo.sh | \ grep -q vanished printf f || printf b); done bbbbbffbffbbbffbfffbbbbffbbbfbbffbbfbfff I would guess that it might not be the test itself that is unreliable, but rather the svn setup done prior, looking at test logs: (cd t mkdir -p logs; i=0; \ while true; do sh t9151-svn-mergeinfo.sh --verbose 21 | tee logs/cur \ | grep -q vanished \ (printf f mv logs/cur logs/fixed-$i) || \ (printf b mv logs/cur logs/broken-$i); \ i=$((i+1)); done) bbffbff the only consistent difference between broken and fixed seems to be in the svn setup stage and more specifically the bit below, with r44 becoming different SHA1s in broken and fixed imports: --- broken-02014-02-05 23:40:21.412967698 +0100 +++ fixed-2 2014-02-05 23:40:44.441536583 +0100 (...) @@ -176,12 +176,12 @@ M subdir/palindromes r43 = a671eec900764a4ab85a6166def3e0d30f1a2664 (refs/remotes/bugfix) M subdir/palindromes -Couldn't find revmap for file:///home/arand/utv/git/git/t/trash%20directory.t9151-svn-mergeinfo/svnrepo/branches/bugfix/subdir -Couldn't find revmap for file:///home/arand/utv/git/git/t/trash%20directory.t9151-svn-mergeinfo/svnrepo/tags/v1.0/subdir -W: Cannot find common ancestor between 90411e1b2118e11664e368a24a1eaa5e8749d150 and fdb537791ee8ba532e49c3d5a34a30feeb87bd59. Ignoring merge info. Couldn't find revmap for file:///home/arand/utv/git/git/t/trash%20directory.t9151-svn-mergeinfo/svnrepo/tags/v1.0 Found merge parent (svn:mergeinfo prop): a671eec900764a4ab85a6166def3e0d30f1a2664 -r44 = a110dec28a4b152b394906b1303fbf19174f7d26 (refs/remotes/trunk) +Couldn't find revmap for file:///home/arand/utv/git/git/t/trash%20directory.t9151-svn-mergeinfo/svnrepo/branches/bugfix/subdir +Couldn't find revmap for file:///home/arand/utv/git/git/t/trash%20directory.t9151-svn-mergeinfo/svnrepo/tags/v1.0/subdir +Found merge parent (svn:mergeinfo prop): fdb537791ee8ba532e49c3d5a34a30feeb87bd59 +r44 = 8b619659a5126105c0a9765b655b6a1add9db4c1 (refs/remotes/trunk) Checked out HEAD: file:///home/arand/utv/git/git/t/trash%20directory.t9151-svn-mergeinfo/svnrepo/trunk r44 ok 1 - load svn dump Does anyone who is more familiar with the test know what's going on here? Is there any way to fix it, or should the test maybe be disabled completely for the time being? -- Martin Erik Werner martinerikwer...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 5/6] setup: Add 'abspath_part_inside_repo' function
In order to extract the part of an absolute path which lies inside the
repo, it is not possible to directly use real_path, since that would
dereference symlinks both outside and inside the work tree.
Add an 'abspath_part_inside_repo' function which first checks if the
work tree is already the prefix, then incrementally checks each path
level by temporarily NUL-terminating at each '/' and comparing against
the work tree path. If a match is found, it overwrites the input path
with the remainder past the work tree (which will be the part inside the
work tree).
This function is currently only intended for use in
'prefix_path_gently'.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
Reviewed-by: Duy Nguyen pclo...@gmail.com
---
setup.c | 64
1 file changed, 64 insertions(+)
diff --git a/setup.c b/setup.c
index 5432a31..906c8e2 100644
--- a/setup.c
+++ b/setup.c
@@ -6,6 +6,70 @@ static int inside_git_dir = -1;
static int inside_work_tree = -1;
/*
+ * The input parameter must contain an absolute path, and it must already be
+ * normalized.
+ *
+ * Find the part of an absolute path that lies inside the work tree by
+ * dereferencing symlinks outside the work tree, for example:
+ * /dir1/repo/dir2/file (work tree is /dir1/repo) - dir2/file
+ * /dir/file (work tree is /) - dir/file
+ * /dir/symlink1/symlink2 (symlink1 points to work tree) - symlink2
+ * /dir/repolink/file (repolink points to /dir/repo) - file
+ * /dir/repo (exactly equal to work tree) - (empty string)
+ */
+static int abspath_part_inside_repo(char *path)
+{
+ size_t len;
+ size_t wtlen;
+ char *path0;
+ int off;
+
+ const char* work_tree = get_git_work_tree();
+ if (!work_tree)
+ return -1;
+ wtlen = strlen(work_tree);
+ len = strlen(path);
+ off = 0;
+
+ /* check if work tree is already the prefix */
+ if (wtlen = len !strncmp(path, work_tree, wtlen)) {
+ if (path[wtlen] == '/') {
+ memmove(path, path + wtlen + 1, len - wtlen);
+ return 0;
+ } else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') {
+ /* work tree is the root, or the whole path */
+ memmove(path, path + wtlen, len - wtlen + 1);
+ return 0;
+ }
+ /* work tree might match beginning of a symlink to work tree */
+ off = wtlen;
+ }
+ path0 = path;
+ path += offset_1st_component(path) + off;
+
+ /* check each '/'-terminated level */
+ while (*path) {
+ path++;
+ if (*path == '/') {
+ *path = '\0';
+ if (strcmp(real_path(path0), work_tree) == 0) {
+ memmove(path0, path + 1, len - (path - path0));
+ return 0;
+ }
+ *path = '/';
+ }
+ }
+
+ /* check whole path */
+ if (strcmp(real_path(path0), work_tree) == 0) {
+ *path0 = '\0';
+ return 0;
+ }
+
+ return -1;
+}
+
+/*
* Normalize path, prepending the prefix for relative paths. If
* remaining_prefix is not NULL, return the actual prefix still
* remains in the path. For example, prefix = sub1/sub2/ and path is
--
1.8.5.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 6/6] setup: Don't dereference in-tree symlinks for absolute paths
The 'prefix_path_gently' function currently applies real_path to
everything if given an absolute path, dereferencing symlinks both
outside and inside the work tree.
This causes most high-level functions to misbehave when acting on
symlinks given via absolute paths. For example
$ git add /dir/repo/symlink
attempts to add the target of the symlink rather than the symlink
itself, which is usually not what the user intends to do.
In order to manipulate symlinks in the work tree using absolute paths,
symlinks should only be dereferenced outside the work tree.
Modify the 'prefix_path_gently' to first normalize the path in order to
make sure path levels are separated by '/', then pass the result to
'abspath_part_inside_repo' to find the part inside the work tree
(without dereferencing any symlinks inside the work tree).
For absolute paths, 'prefix_path_gently' did not, nor does now do, any
actual prefixing, hence the result from 'abspath_part_in_repo' is
returned as-is.
Fixes t0060-82 and t3004-5.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
Reviewed-by: Duy Nguyen pclo...@gmail.com
---
setup.c | 30 ++
t/t0060-path-utils.sh | 2 +-
t/t3004-ls-files-basic.sh | 2 +-
3 files changed, 12 insertions(+), 22 deletions(-)
diff --git a/setup.c b/setup.c
index 906c8e2..ba08885 100644
--- a/setup.c
+++ b/setup.c
@@ -86,11 +86,17 @@ char *prefix_path_gently(const char *prefix, int len,
const char *orig = path;
char *sanitized;
if (is_absolute_path(orig)) {
- const char *temp = real_path(path);
- sanitized = xmalloc(len + strlen(temp) + 1);
- strcpy(sanitized, temp);
+ sanitized = xmalloc(strlen(path) + 1);
if (remaining_prefix)
*remaining_prefix = 0;
+ if (normalize_path_copy_len(sanitized, path, remaining_prefix))
{
+ free(sanitized);
+ return NULL;
+ }
+ if (abspath_part_inside_repo(sanitized)) {
+ free(sanitized);
+ return NULL;
+ }
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
@@ -98,26 +104,10 @@ char *prefix_path_gently(const char *prefix, int len,
strcpy(sanitized + len, path);
if (remaining_prefix)
*remaining_prefix = len;
- }
- if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
- goto error_out;
- if (is_absolute_path(orig)) {
- size_t root_len, len, total;
- const char *work_tree = get_git_work_tree();
- if (!work_tree)
- goto error_out;
- len = strlen(work_tree);
- root_len = offset_1st_component(work_tree);
- total = strlen(sanitized) + 1;
- if (strncmp(sanitized, work_tree, len) ||
- (len root_len sanitized[len] != '\0' sanitized[len]
!= '/')) {
- error_out:
+ if (normalize_path_copy_len(sanitized, sanitized,
remaining_prefix)) {
free(sanitized);
return NULL;
}
- if (sanitized[len] == '/')
- len++;
- memmove(sanitized, sanitized + len, total - len);
}
return sanitized;
}
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index c0a14f6..f04b82d 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '
test $sym = $(test-path-utils real_path $dir2/syml)
'
-test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work
tree symlinks' '
+test_expect_success SYMLINKS 'prefix_path works with absolute paths to work
tree symlinks' '
ln -s target symlink
test $(test-path-utils prefix_path prefix $(pwd)/symlink) =
symlink
'
diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh
index e20c077..9c7adbd 100755
--- a/t/t3004-ls-files-basic.sh
+++ b/t/t3004-ls-files-basic.sh
@@ -36,7 +36,7 @@ test_expect_success 'ls-files -h in corrupt repository' '
test_i18ngrep [Uu]sage: git ls-files broken/usage
'
-test_expect_failure SYMLINKS 'ls-files with absolute paths to symlinks' '
+test_expect_success SYMLINKS 'ls-files with absolute paths to symlinks' '
mkdir subs
ln -s nosuch link
ln -s ../nosuch subs/link
--
1.8.5.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 0/6] Handling of in-tree symlinks for absolute paths
The amount of tweaks seemed deserving of a reroll, so here it is:
* Added your test Junio, with what I figured was an appropriate commit
message describing the user-visible effect (to git-add since I think it's the
simplest to explain), the commit message for the second commit has been
reduced somewhat, to not duplicate the message completely.
* Duplicated quite a bit of the explanation from this first commit into
the last commit, in order to be more obvious about the issue it fixes, I'm
not sure if it's a bit too much?
* Reworded the last commit to not mention the full-path special case, and
replaced all occurences of in-repo with inside work tree or similar.
* Content-wise compared to 'pu' I've tweaked a few comments, un-inlined and
added the wtlen = len check (and the ls-files test is new of course):
diff --git a/setup.c b/setup.c
index 32a6f6b..ba08885 100644
--- a/setup.c
+++ b/setup.c
@@ -6,8 +6,8 @@ static int inside_git_dir = -1;
static int inside_work_tree = -1;
/*
- * No checking if the path is in fact an absolute path is done, and it must
- * already be normalized.
+ * The input parameter must contain an absolute path, and it must already be
+ * normalized.
*
* Find the part of an absolute path that lies inside the work tree by
* dereferencing symlinks outside the work tree, for example:
@@ -17,7 +17,7 @@ static int inside_work_tree = -1;
* /dir/repolink/file (repolink points to /dir/repo) - file
* /dir/repo (exactly equal to work tree) - (empty string)
*/
-static inline int abspath_part_inside_repo(char *path)
+static int abspath_part_inside_repo(char *path)
{
size_t len;
size_t wtlen;
@@ -32,7 +32,7 @@ static inline int abspath_part_inside_repo(char *path)
off = 0;
/* check if work tree is already the prefix */
- if (strncmp(path, work_tree, wtlen) == 0) {
+ if (wtlen = len !strncmp(path, work_tree, wtlen)) {
if (path[wtlen] == '/') {
memmove(path, path + wtlen + 1, len - wtlen);
return 0;
@@ -47,7 +47,7 @@ static inline int abspath_part_inside_repo(char *path)
path0 = path;
path += offset_1st_component(path) + off;
- /* check each level */
+ /* check each '/'-terminated level */
while (*path) {
path++;
if (*path == '/') {
Junio C Hamano (1):
t3004: Add test for ls-files on symlinks via absolute paths
Martin Erik Werner (5):
t0060: Add test for prefix_path on symlinks via absolute paths
t0060: Add test for prefix_path when path == work tree
t0060: Add tests for prefix_path when path begins with work tree
setup: Add 'abspath_part_inside_repo' function
setup: Don't dereference in-tree symlinks for absolute paths
setup.c | 94 +--
t/t0060-path-utils.sh | 21 +++
t/t3004-ls-files-basic.sh | 17 +
3 files changed, 112 insertions(+), 20 deletions(-)
--
1.8.5.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 1/6] t3004: Add test for ls-files on symlinks via absolute paths
From: Junio C Hamano gits...@pobox.com When symlinks in the working tree are manipulated using the absolute path, git dereferences them, and tries to manipulate the link target instead. This causes most high-level functions to misbehave when acting on symlinks given via absolute paths. For example $ git add /dir/repo/symlink attempts to add the target of the symlink rather than the symlink itself, which is usually not what the user intends to do. This is a regression introduced by 18e051a: setup: translate symlinks in filename when using absolute paths (which did not take symlinks inside the work tree into consideration). Add a known-breakage test using the ls-files function, checking both if the symlink leads to a target in the same directory, and a target in the above directory. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com Tested-by: Martin Erik Werner martinerikwer...@gmail.com --- t/t3004-ls-files-basic.sh | 17 + 1 file changed, 17 insertions(+) diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh index 8d9bc3c..e20c077 100755 --- a/t/t3004-ls-files-basic.sh +++ b/t/t3004-ls-files-basic.sh @@ -36,4 +36,21 @@ test_expect_success 'ls-files -h in corrupt repository' ' test_i18ngrep [Uu]sage: git ls-files broken/usage ' +test_expect_failure SYMLINKS 'ls-files with absolute paths to symlinks' ' + mkdir subs + ln -s nosuch link + ln -s ../nosuch subs/link + git add link subs/link + git ls-files -s link subs/link expect + git ls-files -s $(pwd)/link $(pwd)/subs/link actual + test_cmp expect actual + + ( + cd subs + git ls-files -s link ../expect + git ls-files -s $(pwd)/link ../actual + ) + test_cmp expect actual +' + test_done -- 1.8.5.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 3/6] t0060: Add test for prefix_path when path == work tree
The current behaviour of prefix_path is to return an empty string if prefixing and absolute path that only contains exactly the work tree. This behaviour is a potential regression point. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com Reviewed-by: Duy Nguyen pclo...@gmail.com --- t/t0060-path-utils.sh | 6 ++ 1 file changed, 6 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 0bba988..b8e92e1 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -195,6 +195,12 @@ test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree test $(test-path-utils prefix_path prefix $(pwd)/symlink) = symlink ' +test_expect_success 'prefix_path works with only absolute path to work tree' ' + echo expected + test-path-utils prefix_path prefix $(pwd) actual + test_cmp expected actual +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 4/6] t0060: Add tests for prefix_path when path begins with work tree
One edge-case that isn't currently checked in the tests is the beginning of the path matching the work tree, despite the target not actually being the work tree, for example: path = /dir/repoa work_tree = /dir/repo should fail since the path is outside the repo. However, if /dir/repoa is in fact a symlink that points to /dir/repo, it should instead succeed. Add two tests covering these cases, since they might be potential regression points. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com Reviewed-by: Duy Nguyen pclo...@gmail.com --- t/t0060-path-utils.sh | 10 ++ 1 file changed, 10 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index b8e92e1..c0a14f6 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' ' test_cmp expected actual ' +test_expect_success 'prefix_path rejects absolute path to dir with same beginning as work tree' ' + test_must_fail test-path-utils prefix_path prefix $(pwd)a +' + +test_expect_success 'prefix_path works with absolute path to a symlink to work tree having same beginning as work tree' ' + git init repo + ln -s repo repolink + test a = $(cd repo test-path-utils prefix_path prefix $(pwd)/../repolink/a) +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 2/6] t0060: Add test for prefix_path on symlinks via absolute paths
When symlinks in the working tree are manipulated using the absolute path, git dereferences them, and tries to manipulate the link target instead. This applies to most high-level commands but prefix_path is the common denominator for all of them. Add a known-breakage tests using the prefix_path function, which currently uses real_path, causing the dereference. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com Reviewed-by: Duy Nguyen pclo...@gmail.com --- t/t0060-path-utils.sh | 5 + 1 file changed, 5 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 07c10c8..0bba988 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -190,6 +190,11 @@ test_expect_success SYMLINKS 'real path works on symlinks' ' test $sym = $(test-path-utils real_path $dir2/syml) ' +test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' ' + ln -s target symlink + test $(test-path-utils prefix_path prefix $(pwd)/symlink) = symlink +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function
On Tue, 2014-02-04 at 10:09 -0800, Junio C Hamano wrote: Martin Erik Werner martinerikwer...@gmail.com writes: (...) I was trying to convey that if path is simply /dir/repo, then the while loop method of replacing a '/' and checking from the beginning won't work for the last level, since it has no terminating '/' to replace, so hence it's a special case, mentioning the part inside the work tree is arguably confusing in that case, since there isn't really one, maybe it should be left out completely, since the check each level explanation covers it already? I dunno about the explanation, but it still looks strange to have the special case to deal with /dir/repo before you enter the while loop, and then also have code immediately after the loop that seems to handle the same case. Isn't the latter one redundant? The check before the loop doesn't use 'real_path', the one after does: /dir/repo vs /dir/repolink -- Martin Erik Werner martinerikwer...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 5/5] setup: Don't dereference in-tree symlinks for absolute paths
On Mon, Feb 03, 2014 at 11:15:57AM +0700, Duy Nguyen wrote:
On Sun, Feb 2, 2014 at 11:35 PM, Martin Erik Werner
martinerikwer...@gmail.com wrote:
diff --git a/setup.c b/setup.c
index a2e60ab..230505c 100644
--- a/setup.c
+++ b/setup.c
@@ -86,11 +86,23 @@ char *prefix_path_gently(const char *prefix, int len,
const char *orig = path;
char *sanitized;
if (is_absolute_path(orig)) {
- const char *temp = real_path(path);
- sanitized = xmalloc(len + strlen(temp) + 1);
- strcpy(sanitized, temp);
+ char *npath;
+
+ npath = xmalloc(strlen(path) + 1);
if (remaining_prefix)
*remaining_prefix = 0;
+ if (normalize_path_copy_len(npath, path, remaining_prefix))
{
+ free(npath);
+ return NULL;
+ }
+ if (abspath_part_inside_repo(npath)) {
+ free(npath);
+ return NULL;
+ }
+
+ sanitized = xmalloc(strlen(npath) + 1);
+ strcpy(sanitized, npath);
+ free(npath);
We could replace these three lines with sanitized = npath;. But it's
not a big deal imo. The rest of the series looks good.
Reviewed-by: Duy Nguyen pclo...@gmail.com
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
--
Duy
Thank you for reviewing! And thanks Torsten and Junio Likewise. (And
thanks Richard for initial triggering and brief discussion of the bug :)
Hmm, yeah I don't really know what to prefer out of a. Two mallocs with
only a minimal one returned or 2. Single, potentially non-minimal, malloc
returned, if it makes little difference, for simplicity the latter seems nicer.
Then it seems like one could get rid of npath completely:
diff --git a/setup.c b/setup.c
index 230505c..dd120cd 100644
--- a/setup.c
+++ b/setup.c
@@ -88,21 +88,17 @@ char *prefix_path_gently(const char *prefix, int len,
if (is_absolute_path(orig)) {
char *npath;
- npath = xmalloc(strlen(path) + 1);
+ sanitized = xmalloc(strlen(path) + 1);
if (remaining_prefix)
*remaining_prefix = 0;
- if (normalize_path_copy_len(npath, path, remaining_prefix)) {
- free(npath);
+ if (normalize_path_copy_len(sanitized, path, remaining_prefix))
{
+ free(sanitized);
return NULL;
}
- if (abspath_part_inside_repo(npath)) {
- free(npath);
+ if (abspath_part_inside_repo(sanitized)) {
+ free(sanitized);
return NULL;
}
-
- sanitized = xmalloc(strlen(npath) + 1);
- strcpy(sanitized, npath);
- free(npath);
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
at the cost of 'sanitized' always being the length of path, regardless
if it's shorter, or even a NUL string.
--
Martin Erik Werner martinerikwer...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/5] t0060: Add test for manipulating symlinks via absolute paths
On Mon, Feb 03, 2014 at 10:50:17AM -0800, Junio C Hamano wrote: Martin Erik Werner martinerikwer...@gmail.com writes: When symlinks in the working tree are manipulated using the absolute path, git dereferences them, and tries to manipulate the link target instead. The above may a very good description of the root cause, but can we have description of a symptom that is visible by end-users that is caused by the bug being fixed with the series here, by ending the above like so: ... link target instead. This causes git foo bar to misbehave in this and that way. Testing the low-level underlying machinery like this patch does is not wrong per-se, but I suspect that this series was triggered by somebody noticing breakage in a end-user command git foo $path with $path being a full pathname to an in-tree symbolic link. It wouldn't be like somebody who was bored and ran test-path-utils for fun noticed the root cause without realizing how the fix would affect the behaviour that would be visible to end-users, right? Can we have that git foo $path to the testsuite as well? That is the breakage we do not want to repeat in the future by regressing. I am guessing foo is add, but I wasn't closely following the progression of the series. Thanks. Indeed, it was first discovered via git-mv, (by Richard, using git-annex) and me reproducing and reporting it was the start of the thread: http://thread.gmane.org/gmane.comp.version-control.git/240467 In going further (PATCHv0): I've done a bit more digging into this: The issue applies to pretty much all commands which can be given paths to files which are present in the work tree, so add, cat-file, rev-list, etc. At this stage I kind of dropped the reference to any specific top-level command since it seemed to apply to all of them in some way, and I figured it made more sense with a generic explanation that would apply to all commands. But it might definitely be worth to mention it in order for the commit messages to be less technical, and add at least one test which would actually trigger it in a user-manner. So for the explanation, something like that?: This causes for example 'git add /dir/repo/symlink' to attempt to add the target of the symlink rather than the symlink itself, which is usually not what the user intends to do. Hmm, come to think of it, I even made some of those tests back before I found it could be narrowly tested via prefix_path... I guess I'll pick out the git-add one since it's the simplest, should that be added to t0060-path-utils.sh as well, or would it fit better in t3700-add.sh?: From 910d8c9f51c3b3f2c03dbf15ce3cf7ea94de8d27 Mon Sep 17 00:00:00 2001 From: Martin Erik Werner martinerikwer...@gmail.com Date: Thu, 16 Jan 2014 00:24:43 +0100 Subject: [PATCH] Add test for manipulating symlinks via absolute paths When symlinks in the working tree are manipulated using the absolute path, git derferences them, and tries to manipulate the link target instead. Add three known-breakage tests using add, mv, and rev-list which checks this behaviour. The failure of the git-add test is a regression introduced by 18e051a: setup: translate symlinks in filename when using absolute paths (which did not take symlinks in the work tree into consideration). --- t/t0054-symlinks-via-abs-path.sh | 38 ++ 1 file changed, 38 insertions(+) create mode 100755 t/t0054-symlinks-via-abs-path.sh diff --git a/t/t0054-symlinks-via-abs-path.sh b/t/t0054-symlinks-via-abs-path.sh new file mode 100755 index 000..0b3c91e --- /dev/null +++ b/t/t0054-symlinks-via-abs-path.sh @@ -0,0 +1,38 @@ +#!/bin/sh + +test_description='symlinks via sbsolute paths + +This test checks the behavior when symlinks in the working tree are manipulated +via absolute path arguments. +' +. ./test-lib.sh + +test_expect_failure SYMLINKS 'git add symlink with absolute path' ' + + ln -s target symlink + git add $(pwd)/symlink + +' + +rm -f symlink + +test_expect_failure SYMLINKS 'git mv symlink with absolute path' ' + + ln -s target symlink + git add symlink + git mv $(pwd)/symlink moved + +' + +rm -f symlink moved + +test_expect_failure 'git rev-list symlink with absolute path' ' + + ln -s target symlink + git add symlink + git commit -m show + test $(git rev-list HEAD -- symlink) = $(git rev-list HEAD -- $(pwd)/symlink) + +' + +test_done -- 1.8.5.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function
On Mon, Feb 03, 2014 at 11:52:33AM -0800, Junio C Hamano wrote:
Junio C Hamano gits...@pobox.com writes:
Can we have that git foo $path to the testsuite as well? That is
the breakage we do not want to repeat in the future by regressing.
Something like this, perhaps?
t/t3004-ls-files-basic.sh | 17 +
1 file changed, 17 insertions(+)
diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh
index 8d9bc3c..d93089d 100755
--- a/t/t3004-ls-files-basic.sh
+++ b/t/t3004-ls-files-basic.sh
@@ -36,4 +36,21 @@ test_expect_success 'ls-files -h in corrupt repository' '
test_i18ngrep [Uu]sage: git ls-files broken/usage
'
+test_expect_success SYMLINKS 'ls-files with symlinks' '
+ mkdir subs
+ ln -s nosuch link
+ ln -s ../nosuch subs/link
+ git add link subs/link
+ git ls-files -s link subs/link expect
+ git ls-files -s $(pwd)/link $(pwd)/subs/link actual
+ test_cmp expect actual
+
+ (
+ cd subs
+ git ls-files -s link ../expect
+ git ls-files -s $(pwd)/link ../actual
+ )
+ test_cmp expect actual
+'
+
test_done
Your test looks preferrable to the simple git-add one that I proposed,
since it covers some more ground than the simple:
test_expect_success SYMLINKS 'add with abs path to link...' '
ln -s target link
git add link
'
Will you add that test or should I place it in the series with you as
author?
On Mon, Feb 03, 2014 at 01:00:48PM -0800, Junio C Hamano wrote:
Martin Erik Werner martinerikwer...@gmail.com writes:
The path being exactly equal to the work tree is handled separately,
since then there is no directory separator between the work tree and
in-repo part.
What is an in-repo part? Whatever it is, I am not sure if I
follow that logic. After the while (*path) loop checks each level,
you check the whole path---wouldn't that code handle that special
case already?
Given /dir1/repo/dir2/file I've used 'in-repo' to refer to
dir2/file, sometimes interchangably with part inside the work tree
which might be better terminology, and should replace it?
I was trying to convey that if path is simply /dir/repo, then the while
loop method of replacing a '/' and checking from the beginning won't
work for the last level, since it has no terminating '/' to replace, so
hence it's a special case, mentioning the part inside the work tree
is arguably confusing in that case, since there isn't really one, maybe
it should be left out completely, since the check each level
explanation covers it already?
Because it is probably the normal case not to have any symbolic link
in the leading part (hence not having to dereference them), I think
checking is work_tree[] a prefix of path[] early is justified from
performance point of view, though.
/*
+ * No checking if the path is in fact an absolute path is done, and it must
+ * already be normalized.
This is not quite parsable to me.
Hmm, what about
The input parameter must contain an absolute path, and it must
already be normalized.
?
+ * Find the part of an absolute path that lies inside the work tree by
+ * dereferencing symlinks outside the work tree, for example:
+ * /dir1/repo/dir2/file (work tree is /dir1/repo) - dir2/file
+ * /dir/file (work tree is /) - dir/file
+ * /dir/symlink1/symlink2 (symlink1 points to work tree) - symlink2
+ * /dir/repolink/file (repolink points to /dir/repo) - file
+ * /dir/repo (exactly equal to work tree) - (empty string)
+ */
+static inline int abspath_part_inside_repo(char *path)
It looks a bit too big to be marked inline; better leave it to the
compiler to notice that there is only a single callsite and decide
to (or not to) inline it.
Ok, will do.
+{
+ size_t len;
+ size_t wtlen;
+ char *path0;
+ int off;
+
+ const char* work_tree = get_git_work_tree();
+ if (!work_tree)
+ return -1;
+ wtlen = strlen(work_tree);
+ len = strlen(path);
I expect that this is called from a callsite that _knows_ how long
path[] is. Shouldn't len be a parameter to this function instead?
Currently, it actually doesn't, since 'normalize_path_copy_len' is
called on it prior, which can mess with the string length. Do you think
the 'strlen' call should still be moved up a step into
'prefix_path_gently'?
+ off = 0;
+
+ /* check if work tree is already the prefix */
+ if (strncmp(path, work_tree, wtlen) == 0) {
I wonder if we want to be explicit and compare wtlen and len before
even attempting strncmp(). If work_tree is longer than path, it
cannot be a prefix of path, right?
In other words, also with a style-fix to prefer !XXcmp() over
XXcmp() == 0:
if (wtlen = len !strncmp(path, work_tree, wtlen)) {
perhaps? That would make it clear why referring to path[wtlen] on
the next line
Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function
On Sun, Feb 02, 2014 at 09:19:04AM +0700, Duy Nguyen wrote:
On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner
martinerikwer...@gmail.com wrote:
+ /* check if work tree is already the prefix */
+ if (strncmp(path, work_tree, wtlen) == 0) {
+ if (path[wtlen] == '/')
+ memmove(path, path + wtlen + 1, len - wtlen);
+ else
+ /* work tree is the root, or the whole path */
+ memmove(path, path + wtlen, len - wtlen + 1);
+ return 0;
+ }
No the 4th time is not the charm yet :) if path is /abc/defghi and
work_tree is /abc/def you don't want to return ghi as the prefix
here.
Ah indeed, this should catch that:
diff --git a/setup.c b/setup.c
index 2270bd4..5817875 100644
--- a/setup.c
+++ b/setup.c
@@ -32,9 +32,11 @@ static inline int abspath_part_inside_repo(char *path)
if (strncmp(path, work_tree, wtlen) == 0) {
if (path[wtlen] == '/')
memmove(path, path + wtlen + 1, len - wtlen);
- else
+ else if (path[wtlen - 1] == '/' || path[wtlen] == '\0')
/* work tree is the root, or the whole path */
memmove(path, path + wtlen, len - wtlen + 1);
+ else
+ return -1;
return 0;
}
path0 = path;
Is it worth adding a test for this as well?:
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index f6f378b..05d3366 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -201,6 +201,10 @@ test_expect_success 'prefix_path works with only absolute
path to work tree' '
test_cmp expected actual
'
+test_expect_success 'prefix_path rejects absolute path to dir with same
beginning as work tree' '
+ test_must_fail test-path-utils prefix_path prefix $(pwd)a
+'
+
relative_path /foo/a/b/c/ /foo/a/b/ c/
relative_path /foo/a/b/c/ /foo/a/bc/
relative_path /foo/a//b//c////foo/a/b//c/ POSIX
+ path0 = path;
+ path += offset_1st_component(path);
+
+ /* check each level */
+ while (*path != '\0') {
+ path++;
To me it looks like we could write
for (; *path; path++) {
or even
for (path += offset_1st_component(path); *path; path++) {
but it's personal taste..
Yeah, I think aesthetically I don't like cramming too much into the for loop:
for (path += offset_1st_component(path0) + 1; *path; path++) {
neither leaving the init expression unused. So as long as it's just personal
taste I think I'll stick with the current while loop format. But I'll exchange
(*path == '\0') for (*path) though.
--
Martin Erik Werner martinerikwer...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function
On Sun, Feb 02, 2014 at 12:37:16PM +0100, Torsten Bögershausen wrote:
On 2014-02-02 12.21, David Kastrup wrote:
Martin Erik Werner martinerikwer...@gmail.com writes:
On Sun, Feb 02, 2014 at 09:19:04AM +0700, Duy Nguyen wrote:
On Sun, Feb 2, 2014 at 8:59 AM, Martin Erik Werner
martinerikwer...@gmail.com wrote:
+ /* check if work tree is already the prefix */
+ if (strncmp(path, work_tree, wtlen) == 0) {
+ if (path[wtlen] == '/')
+ memmove(path, path + wtlen + 1, len - wtlen);
+ else
+ /* work tree is the root, or the whole path */
+ memmove(path, path + wtlen, len - wtlen + 1);
+ return 0;
+ }
No the 4th time is not the charm yet :) if path is /abc/defghi and
work_tree is /abc/def you don't want to return ghi as the prefix
here.
Ah indeed, this should catch that:
diff --git a/setup.c b/setup.c
index 2270bd4..5817875 100644
--- a/setup.c
+++ b/setup.c
@@ -32,9 +32,11 @@ static inline int abspath_part_inside_repo(char *path)
if (strncmp(path, work_tree, wtlen) == 0) {
if (path[wtlen] == '/')
memmove(path, path + wtlen + 1, len - wtlen);
- else
+ else if (path[wtlen - 1] == '/' || path[wtlen] == '\0')
Is wtlen guaranteed to be nonzero?
Hmm, am I incorrect in thinking
if (!work_tree)
takes care of that?
Another comment:
The raw comparison with '/' is probably working well on all
POSIX/Linux/Unix systems.
To be more portable, the macro
is_dir_sep()
can be used:
if (is_dir_sep(path[wtlen]))
Since the path is already normalized by 'normalize_path_copy_len' which
seems to guarantee '/'-separation, I have assumed that this was
unnecessary?
--
Martin Erik Werner martinerikwer...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 0/5] Handling of in-tree symlinks for absolute paths
Again! (It seems you missed to CC me in your first reply David, please do :) New reroll, fixing the /dir/repoa and /dir/repolink - /dir/repo issues noted by Duy, and adding corresponding tests. If work tree matches beginning of path but needs further checking, it starts from the end of the work tree length, so as minimize the 'real_path' calls. Martin Erik Werner (5): t0060: Add test for manipulating symlinks via absolute paths t0060: Add test for prefix_path when path == work tree t0060: Add tests for prefix_path when path begins with work tree setup: Add 'abspath_part_inside_repo' function setup: Don't dereference in-tree symlinks for absolute paths setup.c | 100 -- t/t0060-path-utils.sh | 21 +++ 2 files changed, 101 insertions(+), 20 deletions(-) -- 1.8.5.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 4/5] setup: Add 'abspath_part_inside_repo' function
In order to extract the part of an absolute path which lies inside the
repo, it is not possible to directly use real_path, since that would
dereference symlinks both outside and inside the work tree.
Add an 'abspath_part_inside_repo' function which first checks if the
work tree is already the prefix, then incrementally checks each path
level by temporarily NUL-terminating at each '/' and comparing against
the work tree path. If a match is found, it overwrites the input path
with the remainder past the work tree (which will be the in-repo part).
The path being exactly equal to the work tree is handled separately,
since then there is no directory separator between the work tree and
in-repo part.
This function is currently only intended for use in
'prefix_path_gently'.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
---
setup.c | 64
1 file changed, 64 insertions(+)
diff --git a/setup.c b/setup.c
index 5432a31..a2e60ab 100644
--- a/setup.c
+++ b/setup.c
@@ -6,6 +6,70 @@ static int inside_git_dir = -1;
static int inside_work_tree = -1;
/*
+ * No checking if the path is in fact an absolute path is done, and it must
+ * already be normalized.
+ *
+ * Find the part of an absolute path that lies inside the work tree by
+ * dereferencing symlinks outside the work tree, for example:
+ * /dir1/repo/dir2/file (work tree is /dir1/repo) - dir2/file
+ * /dir/file (work tree is /) - dir/file
+ * /dir/symlink1/symlink2 (symlink1 points to work tree) - symlink2
+ * /dir/repolink/file (repolink points to /dir/repo) - file
+ * /dir/repo (exactly equal to work tree) - (empty string)
+ */
+static inline int abspath_part_inside_repo(char *path)
+{
+ size_t len;
+ size_t wtlen;
+ char *path0;
+ int off;
+
+ const char* work_tree = get_git_work_tree();
+ if (!work_tree)
+ return -1;
+ wtlen = strlen(work_tree);
+ len = strlen(path);
+ off = 0;
+
+ /* check if work tree is already the prefix */
+ if (strncmp(path, work_tree, wtlen) == 0) {
+ if (path[wtlen] == '/') {
+ memmove(path, path + wtlen + 1, len - wtlen);
+ return 0;
+ } else if (path[wtlen - 1] == '/' || path[wtlen] == '\0') {
+ /* work tree is the root, or the whole path */
+ memmove(path, path + wtlen, len - wtlen + 1);
+ return 0;
+ }
+ /* work tree might match beginning of a symlink to work tree */
+ off = wtlen;
+ }
+ path0 = path;
+ path += offset_1st_component(path) + off;
+
+ /* check each level */
+ while (*path) {
+ path++;
+ if (*path == '/') {
+ *path = '\0';
+ if (strcmp(real_path(path0), work_tree) == 0) {
+ memmove(path0, path + 1, len - (path - path0));
+ return 0;
+ }
+ *path = '/';
+ }
+ }
+
+ /* check whole path */
+ if (strcmp(real_path(path0), work_tree) == 0) {
+ *path0 = '\0';
+ return 0;
+ }
+
+ return -1;
+}
+
+/*
* Normalize path, prepending the prefix for relative paths. If
* remaining_prefix is not NULL, return the actual prefix still
* remains in the path. For example, prefix = sub1/sub2/ and path is
--
1.8.5.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 2/5] t0060: Add test for prefix_path when path == work tree
The current behaviour of prefix_path is to return an empty string if prefixing and absolute path that only contains exactly the work tree. This behaviour is a potential regression point. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com --- t/t0060-path-utils.sh | 6 ++ 1 file changed, 6 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 0bba988..b8e92e1 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -195,6 +195,12 @@ test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree test $(test-path-utils prefix_path prefix $(pwd)/symlink) = symlink ' +test_expect_success 'prefix_path works with only absolute path to work tree' ' + echo expected + test-path-utils prefix_path prefix $(pwd) actual + test_cmp expected actual +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 1/5] t0060: Add test for manipulating symlinks via absolute paths
When symlinks in the working tree are manipulated using the absolute path, git dereferences them, and tries to manipulate the link target instead. This is a regression introduced by 18e051a: setup: translate symlinks in filename when using absolute paths (which did not take symlinks in the work tree into consideration). Add a known-breakage tests using the prefix_path function, which currently uses real_path, causing the dereference. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com --- t/t0060-path-utils.sh | 5 + 1 file changed, 5 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 07c10c8..0bba988 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -190,6 +190,11 @@ test_expect_success SYMLINKS 'real path works on symlinks' ' test $sym = $(test-path-utils real_path $dir2/syml) ' +test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' ' + ln -s target symlink + test $(test-path-utils prefix_path prefix $(pwd)/symlink) = symlink +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 5/5] setup: Don't dereference in-tree symlinks for absolute paths
The 'prefix_path_gently' function currently applies real_path to
everything if given an absolute path, dereferencing symlinks both
outside and inside the work tree. In order to manipulate symlinks in the
work tree using absolute paths, symlinks should only be dereferenced
outside the work tree.
Modify the 'prefix_path_gently' to first normalize the path in order to
make sure path levels are separated by '/', then pass the result to
'abspath_part_inside_repo' to find the in-repo part (without dereferencing
any symlinks inside the work tree).
For absolute paths, 'prefix_path_gently' did not, nor does now do, any
actual prefixing, hence the result from 'abspath_part_in_repo' is
returned as-is.
Fixes t0060-82.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
---
setup.c | 36
t/t0060-path-utils.sh | 2 +-
2 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/setup.c b/setup.c
index a2e60ab..230505c 100644
--- a/setup.c
+++ b/setup.c
@@ -86,11 +86,23 @@ char *prefix_path_gently(const char *prefix, int len,
const char *orig = path;
char *sanitized;
if (is_absolute_path(orig)) {
- const char *temp = real_path(path);
- sanitized = xmalloc(len + strlen(temp) + 1);
- strcpy(sanitized, temp);
+ char *npath;
+
+ npath = xmalloc(strlen(path) + 1);
if (remaining_prefix)
*remaining_prefix = 0;
+ if (normalize_path_copy_len(npath, path, remaining_prefix)) {
+ free(npath);
+ return NULL;
+ }
+ if (abspath_part_inside_repo(npath)) {
+ free(npath);
+ return NULL;
+ }
+
+ sanitized = xmalloc(strlen(npath) + 1);
+ strcpy(sanitized, npath);
+ free(npath);
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
@@ -98,26 +110,10 @@ char *prefix_path_gently(const char *prefix, int len,
strcpy(sanitized + len, path);
if (remaining_prefix)
*remaining_prefix = len;
- }
- if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
- goto error_out;
- if (is_absolute_path(orig)) {
- size_t root_len, len, total;
- const char *work_tree = get_git_work_tree();
- if (!work_tree)
- goto error_out;
- len = strlen(work_tree);
- root_len = offset_1st_component(work_tree);
- total = strlen(sanitized) + 1;
- if (strncmp(sanitized, work_tree, len) ||
- (len root_len sanitized[len] != '\0' sanitized[len]
!= '/')) {
- error_out:
+ if (normalize_path_copy_len(sanitized, sanitized,
remaining_prefix)) {
free(sanitized);
return NULL;
}
- if (sanitized[len] == '/')
- len++;
- memmove(sanitized, sanitized + len, total - len);
}
return sanitized;
}
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index c0a14f6..f04b82d 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '
test $sym = $(test-path-utils real_path $dir2/syml)
'
-test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work
tree symlinks' '
+test_expect_success SYMLINKS 'prefix_path works with absolute paths to work
tree symlinks' '
ln -s target symlink
test $(test-path-utils prefix_path prefix $(pwd)/symlink) =
symlink
'
--
1.8.5.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 3/5] t0060: Add tests for prefix_path when path begins with work tree
One edge-case that isn't currently checked in the tests is the beginning of the path matching the work tree, despite the target not actually being the work tree, for example: path = /dir/repoa work_tree = /dir/repo should fail since the path is outside the repo. However, if /dir/repoa is in fact a symlink that points to /dir/repo, it should instead succeed. Add two tests covering these cases, since they might be potential regression points. --- t/t0060-path-utils.sh | 10 ++ 1 file changed, 10 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index b8e92e1..c0a14f6 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -201,6 +201,16 @@ test_expect_success 'prefix_path works with only absolute path to work tree' ' test_cmp expected actual ' +test_expect_success 'prefix_path rejects absolute path to dir with same beginning as work tree' ' + test_must_fail test-path-utils prefix_path prefix $(pwd)a +' + +test_expect_success 'prefix_path works with absolute path to a symlink to work tree having same beginning as work tree' ' + git init repo + ln -s repo repolink + test a = $(cd repo test-path-utils prefix_path prefix $(pwd)/../repolink/a) +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 0/4] Handling of in-tree symlinks for absolute paths
Hmm, maybe fourth time's the ch...nevermind.
On Sat, Feb 01, 2014 at 02:31:21AM +0100, Martin Erik Werner wrote:
On Fri, Jan 31, 2014 at 11:37:29PM +0100, Torsten Bögershausen wrote:
On 2014-01-31 21.22, Martin Erik Werner wrote:
(...)
diff --git a/cache.h b/cache.h
index ce377e1..242f27d 100644
--- a/cache.h
+++ b/cache.h
@@ -426,6 +426,7 @@ extern void verify_filename(const char *prefix,
int diagnose_misspelt_rev);
extern void verify_non_filename(const char *prefix, const char *name);
extern int path_inside_repo(const char *prefix, const char *path);
+extern int abspath_part_inside_repo(char *dst, const char *path);
abspath_part_inside_repo() is only used in setup.c, isn't it?
In this case we don't need it in cache.h, it can be declared inside setup.c
as
static int abspath_part_inside_repo(char *dst, const char *path);
(or static inline )
-
(And not in this patch: see the final setup.c:)
if (g) {
free(npath);
return NULL;
}
If this is the only caller of abspath_part_inside_repo(),
then do we need npath 2 times as a parameter ?
Or can we re-write it to look like this:
static inline int abspath_part_inside_repo(char *path)
[
]
I guess I've over-generalised it a bit too much, that should rather be
done if-and-when, I presume?
It is indeed only used in setup.c and only by the prefix_path_gently
function so static inline then?
Hmm, for single-parameter it should suffice to simply move the parameter
down into the function, like so?:
const char* src;
src = dst;
and carry on as before (obviously also renaming the variables sensibly),
or did you have something else in mind?
(I added two parameters since I was glancing at 'normalize_path_copy_len'
for inspiration, and was thinking about (purely theoretical) re-use in
other cases rather than minimizing it for the time being.)
What do you mean with the (And not in this patch... bit; what final
setup.c?
As per Torsten's suggestions I've re-worked abspath_part_inside_repo function
to only take one parameter and also put it as 'static inline' before
'prefix_path_gently' since currently only used once. (The change turned
out larger/nicer than I first guessed, since the 'src' pointer and copying
could be dropped completely.)
On Sat, Feb 01, 2014 at 09:31:26AM +0700, Duy Nguyen wrote:
On Sat, Feb 1, 2014 at 3:22 AM, Martin Erik Werner
martinerikwer...@gmail.com wrote:
(...)
+ // check root level
Um.. no C++ style comments. And there should be a test that work_tree
is the prefix of src (common case). If so we can return early and do
not need to do real_path() on every path component.
(...)
Oops, comments fixed.
I've added the check for work tree as existing prefix, which also had the nice
side-effect of checking the case of the work tree being the root of the
filesystem as a bonus.
This new single-buffer version also uses 'offset_1st_component' to move past
the root (since not having to worry about copying).
Martin Erik Werner (4):
t0060: Add test for manipulating symlinks via absolute paths
t0060: Add test for prefix_path when path == work tree
setup: Add 'abspath_part_inside_repo' function
setup: Don't dereference in-tree symlinks for absolute paths
setup.c | 93 ---
t/t0060-path-utils.sh | 11 ++
2 files changed, 84 insertions(+), 20 deletions(-)
--
1.8.5.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 1/4] t0060: Add test for manipulating symlinks via absolute paths
When symlinks in the working tree are manipulated using the absolute path, git dereferences them, and tries to manipulate the link target instead. This is a regression introduced by 18e051a: setup: translate symlinks in filename when using absolute paths (which did not take symlinks in the work tree into consideration). Add a known-breakage tests using the prefix_path function, which currently uses real_path, causing the dereference. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com --- t/t0060-path-utils.sh | 5 + 1 file changed, 5 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 07c10c8..0bba988 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -190,6 +190,11 @@ test_expect_success SYMLINKS 'real path works on symlinks' ' test $sym = $(test-path-utils real_path $dir2/syml) ' +test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' ' + ln -s target symlink + test $(test-path-utils prefix_path prefix $(pwd)/symlink) = symlink +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 2/4] t0060: Add test for prefix_path when path == work tree
The current behaviour of prefix_path is to return an empty string if prefixing and absolute path that only contains exactly the work tree. This behaviour is a potential regression point. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com --- t/t0060-path-utils.sh | 6 ++ 1 file changed, 6 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 0bba988..b8e92e1 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -195,6 +195,12 @@ test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree test $(test-path-utils prefix_path prefix $(pwd)/symlink) = symlink ' +test_expect_success 'prefix_path works with only absolute path to work tree' ' + echo expected + test-path-utils prefix_path prefix $(pwd) actual + test_cmp expected actual +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 3/4] setup: Add 'abspath_part_inside_repo' function
In order to extract the part of an absolute path which lies inside the
repo, it is not possible to directly use real_path, since that would
dereference symlinks both outside and inside the work tree.
Add an 'abspath_part_inside_repo' function which first checks if the
work tree is already the prefix, then incrementally checks each path
level by temporarily NUL-terminating at each '/' and comparing against
the work tree path. If a match is found, it overwrites the input path
with the remainder past the work tree (which will be the in-repo part).
The path being exactly equal to the work tree is handled separately,
since then there is no directory separator between the work tree and
in-repo part.
This function is currently only intended for use in
'prefix_path_gently'.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
---
setup.c | 57 +
1 file changed, 57 insertions(+)
diff --git a/setup.c b/setup.c
index 5432a31..e938785 100644
--- a/setup.c
+++ b/setup.c
@@ -6,6 +6,63 @@ static int inside_git_dir = -1;
static int inside_work_tree = -1;
/*
+ * No checking if the path is in fact an absolute path is done, and it must
+ * already be normalized.
+ *
+ * Find the part of an absolute path that lies inside the work tree by
+ * dereferencing symlinks outside the work tree, for example:
+ * /dir1/repo/dir2/file (work tree is /dir1/repo) - dir2/file
+ * /dir/file (work tree is /) - dir/file
+ * /dir/symlink1/symlink2 (symlink1 points to work tree) - symlink2
+ * /dir/repo (exactly equal to work tree) - (empty string)
+ */
+static inline int abspath_part_inside_repo(char *path)
+{
+ size_t len;
+ size_t wtlen;
+ char *path0;
+
+ const char* work_tree = get_git_work_tree();
+ if (!work_tree)
+ return -1;
+ wtlen = strlen(work_tree);
+ len = strlen(path);
+
+ /* check if work tree is already the prefix */
+ if (strncmp(path, work_tree, wtlen) == 0) {
+ if (path[wtlen] == '/')
+ memmove(path, path + wtlen + 1, len - wtlen);
+ else
+ /* work tree is the root, or the whole path */
+ memmove(path, path + wtlen, len - wtlen + 1);
+ return 0;
+ }
+ path0 = path;
+ path += offset_1st_component(path);
+
+ /* check each level */
+ while (*path != '\0') {
+ path++;
+ if (*path == '/') {
+ *path = '\0';
+ if (strcmp(real_path(path0), work_tree) == 0) {
+ memmove(path0, path + 1, len - (path - path0));
+ return 0;
+ }
+ *path = '/';
+ }
+ }
+
+ /* check whole path */
+ if (strcmp(real_path(path0), work_tree) == 0) {
+ *path0 = '\0';
+ return 0;
+ }
+
+ return -1;
+}
+
+/*
* Normalize path, prepending the prefix for relative paths. If
* remaining_prefix is not NULL, return the actual prefix still
* remains in the path. For example, prefix = sub1/sub2/ and path is
--
1.8.5.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 4/4] setup: Don't dereference in-tree symlinks for absolute paths
The 'prefix_path_gently' function currently applies real_path to
everything if given an absolute path, dereferencing symlinks both
outside and inside the work tree. In order to manipulate symlinks in the
work tree using absolute paths, symlinks should only be dereferenced
outside the work tree.
Modify the 'prefix_path_gently' to first normalize the path in order to
make sure path levels are separated by '/', then pass the result to
'abspath_part_inside_repo' to find the in-repo part (without dereferencing
any symlinks inside the work tree).
For absolute paths, 'prefix_path_gently' did not, nor does now do, any
actual prefixing, hence the result from 'abspath_part_in_repo' is
returned as-is.
Fixes t0060-82.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
---
setup.c | 36
1 file changed, 16 insertions(+), 20 deletions(-)
diff --git a/setup.c b/setup.c
index e938785..2270bd4 100644
--- a/setup.c
+++ b/setup.c
@@ -79,11 +79,23 @@ char *prefix_path_gently(const char *prefix, int len,
const char *orig = path;
char *sanitized;
if (is_absolute_path(orig)) {
- const char *temp = real_path(path);
- sanitized = xmalloc(len + strlen(temp) + 1);
- strcpy(sanitized, temp);
+ char *npath;
+
+ npath = xmalloc(strlen(path) + 1);
if (remaining_prefix)
*remaining_prefix = 0;
+ if (normalize_path_copy_len(npath, path, remaining_prefix)) {
+ free(npath);
+ return NULL;
+ }
+ if (abspath_part_inside_repo(npath)) {
+ free(npath);
+ return NULL;
+ }
+
+ sanitized = xmalloc(strlen(npath) + 1);
+ strcpy(sanitized, npath);
+ free(npath);
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
@@ -91,26 +103,10 @@ char *prefix_path_gently(const char *prefix, int len,
strcpy(sanitized + len, path);
if (remaining_prefix)
*remaining_prefix = len;
- }
- if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
- goto error_out;
- if (is_absolute_path(orig)) {
- size_t root_len, len, total;
- const char *work_tree = get_git_work_tree();
- if (!work_tree)
- goto error_out;
- len = strlen(work_tree);
- root_len = offset_1st_component(work_tree);
- total = strlen(sanitized) + 1;
- if (strncmp(sanitized, work_tree, len) ||
- (len root_len sanitized[len] != '\0' sanitized[len]
!= '/')) {
- error_out:
+ if (normalize_path_copy_len(sanitized, sanitized,
remaining_prefix)) {
free(sanitized);
return NULL;
}
- if (sanitized[len] == '/')
- len++;
- memmove(sanitized, sanitized + len, total - len);
}
return sanitized;
}
--
1.8.5.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/4] setup: Add 'abspath_part_inside_repo' function
In order to extract the part of an absolute path which lies inside the
repo, it is not possible to directly use real_path, since that would
dereference symlinks both outside and inside the work tree.
Add an 'abspath_part_inside_repo' function which incrementally checks
each path level by temporarily NUL-terminating at each '/' and comparing
against the work tree path. When a match is found, it copies the
remainder (which will be the in-repo part) to a destination
buffer.
The path being the filesystem root or exactly equal to the work tree are
special cases handled separately, since then there is no directory
separator between the work tree and in-repo part.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
---
cache.h | 1 +
setup.c | 63 +++
2 files changed, 64 insertions(+)
diff --git a/cache.h b/cache.h
index ce377e1..242f27d 100644
--- a/cache.h
+++ b/cache.h
@@ -426,6 +426,7 @@ extern void verify_filename(const char *prefix,
int diagnose_misspelt_rev);
extern void verify_non_filename(const char *prefix, const char *name);
extern int path_inside_repo(const char *prefix, const char *path);
+extern int abspath_part_inside_repo(char *dst, const char *path);
#define INIT_DB_QUIET 0x0001
diff --git a/setup.c b/setup.c
index 5432a31..e606846 100644
--- a/setup.c
+++ b/setup.c
@@ -77,6 +77,69 @@ int path_inside_repo(const char *prefix, const char *path)
return 0;
}
+/*
+ * It is ok if dst == src, but they should not overlap otherwise.
+ * No checking if the path is in fact an absolute path is done, and it must
+ * already be normalized.
+ *
+ * Find the part of an absolute path that lies inside the work tree by
+ * dereferencing symlinks outside the work tree, for example:
+ * /dir1/repo/dir2/file (work tree is /dir1/repo) - dir2/file
+ * /dir/file (work tree is /) - dir/file
+ * /dir/symlink1/symlink2 (symlink1 points to work tree) - symlink2
+ * /dir/repo (exactly equal to work tree) - (empty string)
+ */
+int abspath_part_inside_repo(char *dst, const char* src)
+{
+ size_t len;
+ char *dst0;
+ char temp;
+
+ const char* work_tree = get_git_work_tree();
+ if (!work_tree)
+ return -1;
+ len = strlen(src);
+ dst0 = dst;
+
+ // check root level
+ if (has_dos_drive_prefix(src)) {
+ *dst++ = *src++;
+ *dst++ = *src++;
+ *dst++ = *src++;
+ } else {
+ *dst++ = *src++;
+ }
+ temp = *dst;
+ *dst = '\0';
+ if (strcmp(real_path(dst0), work_tree) == 0) {
+ *dst = temp;
+ memmove(dst0, src, len - (dst - dst0) + 1);
+ return 0;
+ }
+ *dst = temp;
+
+ // check each level
+ while (*dst != '\0') {
+ *dst++ = *src++;
+ if (*dst == '/') {
+ *dst = '\0';
+ if (strcmp(real_path(dst0), work_tree) == 0) {
+ memmove(dst0, src + 1, len - (dst - dst0));
+ return 0;
+ }
+ *dst = '/';
+ }
+ }
+
+ // check whole path
+ if (strcmp(real_path(dst0), work_tree) == 0) {
+ *dst0 = '\0';
+ return 0;
+ }
+
+ return -1;
+}
+
int check_filename(const char *prefix, const char *arg)
{
const char *name;
--
1.8.5.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/4] t0060: Add test for manipulating symlinks via absolute paths
When symlinks in the working tree are manipulated using the absolute path, git dereferences them, and tries to manipulate the link target instead. This is a regression introduced by 18e051a: setup: translate symlinks in filename when using absolute paths (which did not take symlinks in the work tree into consideration). Add a known-breakage tests using the prefix_path function, which currently uses real_path, causing the dereference. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com --- t/t0060-path-utils.sh | 5 + 1 file changed, 5 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 07c10c8..0bba988 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -190,6 +190,11 @@ test_expect_success SYMLINKS 'real path works on symlinks' ' test $sym = $(test-path-utils real_path $dir2/syml) ' +test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree symlinks' ' + ln -s target symlink + test $(test-path-utils prefix_path prefix $(pwd)/symlink) = symlink +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/4] setup: Don't dereference in-tree symlinks for absolute paths
The 'prefix_path_gently' function currently applies real_path to
everything if given an absolute path, dereferencing symlinks both
outside and inside the work tree. In order to manipulate symlinks in the
work tree using absolute paths, symlinks should only be dereferenced
outside the work tree.
Modify the 'prefix_path_gently' to first normalize the path in order to
make sure path levels are separated by '/', then pass the result to
'abspath_part_inside_repo' to find the in-repo part (without dereferencing
any symlinks inside the work tree).
For absolute paths, 'prefix_path_gently' did not, nor does now do, any
actual prefixing, hence the result from 'abspath_part_in_repo' is
returned as-is.
Fixes t0060-82.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
---
setup.c | 36
t/t0060-path-utils.sh | 2 +-
2 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/setup.c b/setup.c
index e606846..2e65b48 100644
--- a/setup.c
+++ b/setup.c
@@ -22,11 +22,23 @@ char *prefix_path_gently(const char *prefix, int len,
const char *orig = path;
char *sanitized;
if (is_absolute_path(orig)) {
- const char *temp = real_path(path);
- sanitized = xmalloc(len + strlen(temp) + 1);
- strcpy(sanitized, temp);
+ char *npath;
+
+ npath = xmalloc(strlen(path) + 1);
if (remaining_prefix)
*remaining_prefix = 0;
+ if (normalize_path_copy_len(npath, path, remaining_prefix)) {
+ free(npath);
+ return NULL;
+ }
+ if (abspath_part_inside_repo(npath, npath)) {
+ free(npath);
+ return NULL;
+ }
+
+ sanitized = xmalloc(strlen(npath) + 1);
+ strcpy(sanitized, npath);
+ free(npath);
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
@@ -34,26 +46,10 @@ char *prefix_path_gently(const char *prefix, int len,
strcpy(sanitized + len, path);
if (remaining_prefix)
*remaining_prefix = len;
- }
- if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
- goto error_out;
- if (is_absolute_path(orig)) {
- size_t root_len, len, total;
- const char *work_tree = get_git_work_tree();
- if (!work_tree)
- goto error_out;
- len = strlen(work_tree);
- root_len = offset_1st_component(work_tree);
- total = strlen(sanitized) + 1;
- if (strncmp(sanitized, work_tree, len) ||
- (len root_len sanitized[len] != '\0' sanitized[len]
!= '/')) {
- error_out:
+ if (normalize_path_copy_len(sanitized, sanitized,
remaining_prefix)) {
free(sanitized);
return NULL;
}
- if (sanitized[len] == '/')
- len++;
- memmove(sanitized, sanitized + len, total - len);
}
return sanitized;
}
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index b8e92e1..f6f378b 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '
test $sym = $(test-path-utils real_path $dir2/syml)
'
-test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work
tree symlinks' '
+test_expect_success SYMLINKS 'prefix_path works with absolute paths to work
tree symlinks' '
ln -s target symlink
test $(test-path-utils prefix_path prefix $(pwd)/symlink) =
symlink
'
--
1.8.5.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/4] t0060: Add test for prefix_path when path == work tree
The current behaviour of prefix_path is to return an empty string if prefixing and absolute path that only contains exactly the work tree. This behaviour is a potential regression point. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com --- t/t0060-path-utils.sh | 6 ++ 1 file changed, 6 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 0bba988..b8e92e1 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -195,6 +195,12 @@ test_expect_failure SYMLINKS 'prefix_path works with absolute paths to work tree test $(test-path-utils prefix_path prefix $(pwd)/symlink) = symlink ' +test_expect_success 'prefix_path works with only absolute path to work tree' ' + echo expected + test-path-utils prefix_path prefix $(pwd) actual + test_cmp expected actual +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/4] setup: Don't dereference in-tree symlinks for absolute paths
On Mon, Jan 27, 2014 at 08:31:37AM -0800, Junio C Hamano wrote:
Martin Erik Werner martinerikwer...@gmail.com writes:
In order to manipulate symliks in the
work tree using absolute paths, symlinks should only be dereferenced
outside the work tree.
I agree 100% with this reasoning (modulo s/symliks/symlinks/).
As to the implementation, it looks a bit overly complicated,
though. I haven't tried writing the same myself, but
* I suspect that strbuf would help simplifying the allocation and
deallocation;
* Also I suspect that use of string-list to split and then join is
making the code unnecessarily complex. In Python/Perl, that would
be a normal approach, but in C, it would be a lot simpler if you
prepare a writable temporary in wtpart[], walk from left to right
finding '/' and replacing it temporarily with NUL to terminate in
order to check with real_path(), restore the NUL back to '/' to
check deeper, and rinse and repeat.
Having said that, I am not absolutely sure if the repeated
calls to real_path() are doing the right thing, though ;-)
diff --git a/setup.c b/setup.c
index 5432a31..0789a96 100644
--- a/setup.c
+++ b/setup.c
@@ -22,11 +22,51 @@ char *prefix_path_gently(const char *prefix, int len,
const char *orig = path;
char *sanitized;
if (is_absolute_path(orig)) {
- const char *temp = real_path(path);
- sanitized = xmalloc(len + strlen(temp) + 1);
- strcpy(sanitized, temp);
+ int i, match;
+ size_t wtpartlen;
+ char *npath, *wtpart;
+ struct string_list list = STRING_LIST_INIT_DUP;
+ const char *work_tree = get_git_work_tree();
+ if (!work_tree)
+ return NULL;
+ npath = xmalloc(strlen(path) + 1);
if (remaining_prefix)
*remaining_prefix = 0;
+ if (normalize_path_copy_len(npath, path, remaining_prefix)) {
+ free(npath);
+ return NULL;
+ }
+
+ string_list_split(list, npath, '/', -1);
+ wtpart = xmalloc(strlen(npath) + 1);
+ i = 0;
+ match = 0;
+ strcpy(wtpart, list.items[i++].string);
+ strcat(wtpart, /);
+ if (strcmp(real_path(wtpart), work_tree) == 0) {
+ match = 1;
+ } else {
+ while (i list.nr) {
+ strcat(wtpart, list.items[i++].string);
+ if (strcmp(real_path(wtpart), work_tree) == 0) {
+ match = 1;
+ break;
+ }
+ strcat(wtpart, /);
+ }
+ }
+ string_list_clear(list, 0);
+ if (!match) {
+ free(npath);
+ free(wtpart);
+ return NULL;
+ }
+
+ wtpartlen = strlen(wtpart);
+ sanitized = xmalloc(strlen(npath) - wtpartlen);
+ strcpy(sanitized, npath + wtpartlen + 1);
+ free(npath);
+ free(wtpart);
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
@@ -34,26 +74,10 @@ char *prefix_path_gently(const char *prefix, int len,
strcpy(sanitized + len, path);
if (remaining_prefix)
*remaining_prefix = len;
- }
- if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
- goto error_out;
- if (is_absolute_path(orig)) {
- size_t root_len, len, total;
- const char *work_tree = get_git_work_tree();
- if (!work_tree)
- goto error_out;
- len = strlen(work_tree);
- root_len = offset_1st_component(work_tree);
- total = strlen(sanitized) + 1;
- if (strncmp(sanitized, work_tree, len) ||
- (len root_len sanitized[len] != '\0' sanitized[len]
!= '/')) {
- error_out:
+ if (normalize_path_copy_len(sanitized, sanitized,
remaining_prefix)) {
free(sanitized);
return NULL;
}
- if (sanitized[len] == '/')
- len++;
- memmove(sanitized, sanitized + len, total - len);
}
return sanitized;
}
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 3a0677a..03a12ac 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on
symlinks' '
test $sym = $(test-path-utils real_path $dir2/syml)
'
-test_expect_failure SYMLINKS 'prefix_path works with work tree symlinks' '
+test_expect_success SYMLINKS 'prefix_path works
Re: [PATCH v3 3/4] setup: Add 'abspath_part_inside_repo' function
On Fri, Jan 31, 2014 at 11:37:29PM +0100, Torsten Bögershausen wrote:
On 2014-01-31 21.22, Martin Erik Werner wrote:
In order to extract the part of an absolute path which lies inside the
repo, it is not possible to directly use real_path, since that would
dereference symlinks both outside and inside the work tree.
Add an 'abspath_part_inside_repo' function which incrementally checks
each path level by temporarily NUL-terminating at each '/' and comparing
against the work tree path. When a match is found, it copies the
remainder (which will be the in-repo part) to a destination
buffer.
The path being the filesystem root or exactly equal to the work tree are
special cases handled separately, since then there is no directory
separator between the work tree and in-repo part.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
---
cache.h | 1 +
setup.c | 63
+++
2 files changed, 64 insertions(+)
diff --git a/cache.h b/cache.h
index ce377e1..242f27d 100644
--- a/cache.h
+++ b/cache.h
@@ -426,6 +426,7 @@ extern void verify_filename(const char *prefix,
int diagnose_misspelt_rev);
extern void verify_non_filename(const char *prefix, const char *name);
extern int path_inside_repo(const char *prefix, const char *path);
+extern int abspath_part_inside_repo(char *dst, const char *path);
abspath_part_inside_repo() is only used in setup.c, isn't it?
In this case we don't need it in cache.h, it can be declared inside setup.c as
static int abspath_part_inside_repo(char *dst, const char *path);
(or static inline )
-
(And not in this patch: see the final setup.c:)
if (g) {
free(npath);
return NULL;
}
If this is the only caller of abspath_part_inside_repo(),
then do we need npath 2 times as a parameter ?
Or can we re-write it to look like this:
static inline int abspath_part_inside_repo(char *path)
[
]
I guess I've over-generalised it a bit too much, that should rather be
done if-and-when, I presume?
It is indeed only used in setup.c and only by the prefix_path_gently
function so static inline then?
Hmm, for single-parameter it should suffice to simply move the parameter
down into the function, like so?:
const char* src;
src = dst;
and carry on as before (obviously also renaming the variables sensibly),
or did you have something else in mind?
(I added two parameters since I was glancing at 'normalize_path_copy_len'
for inspiration, and was thinking about (purely theoretical) re-use in
other cases rather than minimizing it for the time being.)
What do you mean with the (And not in this patch... bit; what final
setup.c?
--
Martin Erik Werner martinerikwer...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] in-tree symlink handling with absolute paths
On Wed, 2014-01-15 at 13:48 +0100, Martin Erik Werner wrote: If git-mv is provided absolute paths when moving symlinks, it tries to dereference them and (attempts to) move the symlink target rather than the symlink itself, this seems like a quite odd behaviour since it's inconsistent with how git-mv works with symlinks if given relative paths, and I'm thinking it might be a bug, since it not documented in the git-mv manpage. I've done a bit more digging into this: The issue applies to pretty much all commands which can be given paths to files which are present in the work tree, so add, cat-file, rev-list, etc. I've written a test and a fix which seems to do the right thing. Martin Erik Werner (2): t0060: Add test for manipulating symlinks via absolute paths setup: Don't dereference in-tree symlinks for absolute paths setup.c | 54 --- t/t0060-path-utils.sh | 7 +++ 2 files changed, 41 insertions(+), 20 deletions(-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] setup: Don't dereference in-tree symlinks for absolute paths
The prefix_path_gently() function currently applies real_path to
everything if given an absolute path, dereferencing symlinks both
outside and inside the work tree. In order to manipulate symliks in the
work tree using absolute paths, symlinks should only be dereferenced
outside the work tree.
Modify prefix_path_gently() to first normalize the path in order to
make sure path levels are separated by '/', then use this separator to
check the real path of each level of the path until it has found the
length that corresponds to the work tree.
For absolute paths, the function did not, nor does now do, any actual
prefixing, hence we simply remove the path corresponding to the work
tree and return the remaining in-tree part of the path.
Fixes t0060-82.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
---
setup.c | 54 ---
t/t0060-path-utils.sh | 2 +-
2 files changed, 35 insertions(+), 21 deletions(-)
diff --git a/setup.c b/setup.c
index 6c3f85f..bec587e 100644
--- a/setup.c
+++ b/setup.c
@@ -22,11 +22,41 @@ char *prefix_path_gently(const char *prefix, int len,
const char *orig = path;
char *sanitized;
if (is_absolute_path(orig)) {
- const char *temp = real_path(path);
- sanitized = xmalloc(len + strlen(temp) + 1);
- strcpy(sanitized, temp);
+ char npath[strlen(path)];
if (remaining_prefix)
*remaining_prefix = 0;
+ if (normalize_path_copy_len(npath, path, remaining_prefix))
+ return NULL;
+ const char *work_tree = get_git_work_tree();
+ if (!work_tree)
+ return NULL;
+ struct string_list list = STRING_LIST_INIT_DUP;
+ string_list_split(list, npath, '/', -1);
+
+ char wtpart[strlen(npath) + 1];
+ int i = 0;
+ int match = 0;
+ strcpy(wtpart, list.items[i++].string);
+ strcpy(wtpart, /);
+ if (strcmp(real_path(wtpart), work_tree) == 0) {
+ match = 1;
+ } else {
+ while (i list.nr) {
+ strcat(wtpart, list.items[i++].string);
+ if (strcmp(real_path(wtpart), work_tree) == 0) {
+ match = 1;
+ break;
+ }
+ strcat(wtpart, /);
+ }
+ }
+ string_list_clear(list, 0);
+ if (!match)
+ return NULL;
+
+ size_t wtpartlen = strlen(wtpart);
+ sanitized = xmalloc(strlen(npath) - wtpartlen);
+ strcpy(sanitized, npath + wtpartlen + 1);
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
@@ -34,26 +64,10 @@ char *prefix_path_gently(const char *prefix, int len,
strcpy(sanitized + len, path);
if (remaining_prefix)
*remaining_prefix = len;
- }
- if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
- goto error_out;
- if (is_absolute_path(orig)) {
- size_t root_len, len, total;
- const char *work_tree = get_git_work_tree();
- if (!work_tree)
- goto error_out;
- len = strlen(work_tree);
- root_len = offset_1st_component(work_tree);
- total = strlen(sanitized) + 1;
- if (strncmp(sanitized, work_tree, len) ||
- (len root_len sanitized[len] != '\0' sanitized[len]
!= '/')) {
- error_out:
+ if (normalize_path_copy_len(sanitized, sanitized,
remaining_prefix)) {
free(sanitized);
return NULL;
}
- if (sanitized[len] == '/')
- len++;
- memmove(sanitized, sanitized + len, total - len);
}
return sanitized;
}
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 3a0677a..03a12ac 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '
test $sym = $(test-path-utils real_path $dir2/syml)
'
-test_expect_failure SYMLINKS 'prefix_path works with work tree symlinks' '
+test_expect_success SYMLINKS 'prefix_path works with work tree symlinks' '
ln -s target symlink
test $(test-path-utils prefix_path prefix $(pwd)/symlink) =
symlink
--
1.8.5.2
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo
[PATCH 1/2] t0060: Add test for manipulating symlinks via absolute paths
When symlinks in the working tree are manipulated using the absolute path, git dereferences them, and tries to manipulate the link target instead. This is a regression introduced by 18e051a: setup: translate symlinks in filename when using absolute paths (which did not take symlinks in the work tree into consideration). Add a known-breakage tests using the prefix_path function, which currently uses real_path, causing the dereference. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com --- t/t0060-path-utils.sh | 7 +++ 1 file changed, 7 insertions(+) diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh index 07c10c8..3a0677a 100755 --- a/t/t0060-path-utils.sh +++ b/t/t0060-path-utils.sh @@ -190,6 +190,13 @@ test_expect_success SYMLINKS 'real path works on symlinks' ' test $sym = $(test-path-utils real_path $dir2/syml) ' +test_expect_failure SYMLINKS 'prefix_path works with work tree symlinks' ' + + ln -s target symlink + test $(test-path-utils prefix_path prefix $(pwd)/symlink) = symlink + +' + relative_path /foo/a/b/c/ /foo/a/b/ c/ relative_path /foo/a/b/c/ /foo/a/bc/ relative_path /foo/a//b//c////foo/a/b//c/ POSIX -- 1.8.5.2 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] setup: Don't dereference in-tree symlinks for absolute paths
On Sun, Jan 26, 2014 at 06:19:25PM +0100, Torsten Bögershausen wrote:
On 2014-01-26 15.22, Martin Erik Werner wrote:
The prefix_path_gently() function currently applies real_path to
everything if given an absolute path, dereferencing symlinks both
outside and inside the work tree. In order to manipulate symliks in the
work tree using absolute paths, symlinks should only be dereferenced
outside the work tree.
Modify prefix_path_gently() to first normalize the path in order to
make sure path levels are separated by '/', then use this separator to
check the real path of each level of the path until it has found the
length that corresponds to the work tree.
For absolute paths, the function did not, nor does now do, any actual
prefixing, hence we simply remove the path corresponding to the work
tree and return the remaining in-tree part of the path.
Fixes t0060-82.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
---
setup.c | 54
---
t/t0060-path-utils.sh | 2 +-
2 files changed, 35 insertions(+), 21 deletions(-)
diff --git a/setup.c b/setup.c
index 6c3f85f..bec587e 100644
--- a/setup.c
+++ b/setup.c
@@ -22,11 +22,41 @@ char *prefix_path_gently(const char *prefix, int len,
const char *orig = path;
char *sanitized;
if (is_absolute_path(orig)) {
- const char *temp = real_path(path);
- sanitized = xmalloc(len + strlen(temp) + 1);
- strcpy(sanitized, temp);
+ char npath[strlen(path)];
Is this portable ?
This is variable-length array, isn't it ?
Using xmalloc() may be better
Ah, right, that looks bad now that you mention it.
if (remaining_prefix)
*remaining_prefix = 0;
+ if (normalize_path_copy_len(npath, path, remaining_prefix))
+ return NULL;
+ const char *work_tree = get_git_work_tree();
declaration after statements should be avoided (not only here)
Indeed, I somehow guessed that declaration-after-statement was ok and
tried to keep them close to usage, bad guess, evidently.
I've rerolled the last v1 patch accordingly:
* Use xmalloc() when initializing char* from strlen()
* Separate and move declarations to beginning of scope
* Fix a strcpy that should've been a strcat (which would've nuked DOS
prefixes, I think)
[PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths
setup.c | 64 +++
t/t0060-path-utils.sh | 2 +-
2 files changed, 45 insertions(+), 21 deletions(-)
--
Martin Erik Werner martinerikwer...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] setup: Don't dereference in-tree symlinks for absolute paths
The prefix_path_gently() function currently applies real_path to
everything if given an absolute path, dereferencing symlinks both
outside and inside the work tree. In order to manipulate symliks in the
work tree using absolute paths, symlinks should only be dereferenced
outside the work tree.
Modify prefix_path_gently() to first normalize the path in order to
make sure path levels are separated by '/', then use this separator to
check the real path of each level of the path until it has found the
length that corresponds to the work tree.
For absolute paths, the function did not, nor does now do, any actual
prefixing, hence we simply remove the path corresponding to the work
tree and return the remaining in-tree part of the path.
Fixes t0060-82.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
---
setup.c | 64 +++
t/t0060-path-utils.sh | 2 +-
2 files changed, 45 insertions(+), 21 deletions(-)
diff --git a/setup.c b/setup.c
index 5432a31..0789a96 100644
--- a/setup.c
+++ b/setup.c
@@ -22,11 +22,51 @@ char *prefix_path_gently(const char *prefix, int len,
const char *orig = path;
char *sanitized;
if (is_absolute_path(orig)) {
- const char *temp = real_path(path);
- sanitized = xmalloc(len + strlen(temp) + 1);
- strcpy(sanitized, temp);
+ int i, match;
+ size_t wtpartlen;
+ char *npath, *wtpart;
+ struct string_list list = STRING_LIST_INIT_DUP;
+ const char *work_tree = get_git_work_tree();
+ if (!work_tree)
+ return NULL;
+ npath = xmalloc(strlen(path) + 1);
if (remaining_prefix)
*remaining_prefix = 0;
+ if (normalize_path_copy_len(npath, path, remaining_prefix)) {
+ free(npath);
+ return NULL;
+ }
+
+ string_list_split(list, npath, '/', -1);
+ wtpart = xmalloc(strlen(npath) + 1);
+ i = 0;
+ match = 0;
+ strcpy(wtpart, list.items[i++].string);
+ strcat(wtpart, /);
+ if (strcmp(real_path(wtpart), work_tree) == 0) {
+ match = 1;
+ } else {
+ while (i list.nr) {
+ strcat(wtpart, list.items[i++].string);
+ if (strcmp(real_path(wtpart), work_tree) == 0) {
+ match = 1;
+ break;
+ }
+ strcat(wtpart, /);
+ }
+ }
+ string_list_clear(list, 0);
+ if (!match) {
+ free(npath);
+ free(wtpart);
+ return NULL;
+ }
+
+ wtpartlen = strlen(wtpart);
+ sanitized = xmalloc(strlen(npath) - wtpartlen);
+ strcpy(sanitized, npath + wtpartlen + 1);
+ free(npath);
+ free(wtpart);
} else {
sanitized = xmalloc(len + strlen(path) + 1);
if (len)
@@ -34,26 +74,10 @@ char *prefix_path_gently(const char *prefix, int len,
strcpy(sanitized + len, path);
if (remaining_prefix)
*remaining_prefix = len;
- }
- if (normalize_path_copy_len(sanitized, sanitized, remaining_prefix))
- goto error_out;
- if (is_absolute_path(orig)) {
- size_t root_len, len, total;
- const char *work_tree = get_git_work_tree();
- if (!work_tree)
- goto error_out;
- len = strlen(work_tree);
- root_len = offset_1st_component(work_tree);
- total = strlen(sanitized) + 1;
- if (strncmp(sanitized, work_tree, len) ||
- (len root_len sanitized[len] != '\0' sanitized[len]
!= '/')) {
- error_out:
+ if (normalize_path_copy_len(sanitized, sanitized,
remaining_prefix)) {
free(sanitized);
return NULL;
}
- if (sanitized[len] == '/')
- len++;
- memmove(sanitized, sanitized + len, total - len);
}
return sanitized;
}
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 3a0677a..03a12ac 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -190,7 +190,7 @@ test_expect_success SYMLINKS 'real path works on symlinks' '
test $sym = $(test-path-utils real_path $dir2/syml)
'
-test_expect_failure SYMLINKS 'prefix_path works with work tree symlinks' '
+test_expect_success SYMLINKS 'prefix_path works with work tree
git-mv with absolute path derefereces symlinks
Hi, If git-mv is provided absolute paths when moving symlinks, it tries to dereference them and (attempts to) move the symlink target rather than the symlink itself, this seems like a quite odd behaviour since it's inconsistent with how git-mv works with symlinks if given relative paths, and I'm thinking it might be a bug, since it not documented in the git-mv manpage. ### $ git init linktest Initialized empty Git repository in /home/arand/tmp/linktest/.git/ $ cd linktest/ $ touch target $ ln -s target link $ ln -s /tmp/target link2 $ git add . $ git commit -m1 [master (root-commit) 3cfea66] 1 3 files changed, 2 insertions(+) create mode 12 link create mode 12 link2 create mode 100644 target $ git mv $(pwd)/link $(pwd)/moved $ git status On branch master Changes to be committed: (use git reset HEAD file... to unstage) renamed:target - moved $ git mv $(pwd)/link2 $(pwd)/moved2 fatal: /home/arand/tmp/linktest/link2: '/home/arand/tmp/linktest/link2' is outside repository ### -- Martin Erik Werner martinerikwer...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bug in filter-branch -d option, new files are dumped into parent
Hi, I think I have stumbled on a bug in the -d option of git filter-branch. It seems like in the final stage of filter-branch, regardless of where -d is set, it will make updates to the working directory as being the parent of the -d directory, and the actual working directory is left as it were before the filtering. For example if using -d /tmp/git-rewrite, the new checkout files are dumped into /tmp. A simple test scenario: ### mkdir test cd test git init echo foo foo git add foo git commit -mfoo echo bar bar git add bar git commit -mbar git checkout -b rewrite git filter-branch -d /tmp/git-rewrite --tree-filter 'echo baz baz' -- rewrite # git status # shows 'baz' as unstaged-deleted in the working directory # # ls /tmp/ # shows the 'baz' file created in the root, above git-rewrite # # git log --stat --oneline # does show expected result though # # if you run it again you'll get an error because of the /tmp/baz file git reset --hard master git filter-branch -f -d /tmp/git-rewrite --tree-filter 'echo baz baz' -- rewrite # Rewrite afac18542ac3d432b647866f5ac6918b81b3bb78 (2/2) # Ref 'refs/heads/rewrite' was rewritten # error: Untracked working tree file 'baz' would be overwritten by merge. # # At this point, 'baz' is instead staged-deleted in the working directory ### -- Martin Erik Werner martinerikwer...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] shell-prompt: clean up nested if-then
Minor clean up of if-then nesting in checks for environment variables
and config options. No functional changes.
---
contrib/completion/git-prompt.sh | 27 +--
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 9b2eec2..e29694d 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -320,26 +320,25 @@ __git_ps1 ()
b=GIT_DIR!
fi
elif [ true = $(git rev-parse --is-inside-work-tree
2/dev/null) ]; then
- if [ -n ${GIT_PS1_SHOWDIRTYSTATE-} ]; then
- if [ $(git config --bool bash.showDirtyState)
!= false ]; then
- git diff --no-ext-diff --quiet
--exit-code || w=*
- if git rev-parse --quiet --verify HEAD
/dev/null; then
- git diff-index --cached --quiet
HEAD -- || i=+
- else
- i=#
- fi
+ if test -n ${GIT_PS1_SHOWDIRTYSTATE-}
+ test $(git config --bool bash.showDirtyState) !=
false
+ then
+ git diff --no-ext-diff --quiet --exit-code ||
w=*
+ if git rev-parse --quiet --verify HEAD
/dev/null; then
+ git diff-index --cached --quiet HEAD --
|| i=+
+ else
+ i=#
fi
fi
if [ -n ${GIT_PS1_SHOWSTASHSTATE-} ]; then
git rev-parse --verify refs/stash /dev/null
21 s=$
fi
- if [ -n ${GIT_PS1_SHOWUNTRACKEDFILES-} ]; then
- if [ $(git config --bool
bash.showUntrackedFiles) != false ]; then
- if [ -n $(git ls-files --others
--exclude-standard) ]; then
- u=%
- fi
- fi
+ if test -n ${GIT_PS1_SHOWUNTRACKEDFILES-}
+ test $(git config --bool bash.showUntrackedFiles)
!= false
+ test -n $(git ls-files --others --exclude-standard)
+ then
+ u=%
fi
if [ -n ${GIT_PS1_SHOWUPSTREAM-} ]; then
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] shell-prompt: clean up nested if-then
On Mon, 2013-02-18 at 21:31 +0100, Simon vanaf Telefoon wrote:
Hi all, sorry for top posting :-( blame the phone and k9
I have a small issue with the use of test instead of [
If that only applies to this section of the entire file.
Coding style has some value.
Combining nested ifs with seems harmless enough, though should be
well tested.
Cheers
Simon
Ah, indeed, I looked around a bit more, and as per
http://mywiki.wooledge.org/BashPitfalls it seems like 'test' is bad to use with
multiple 's anyways.
I've changed to using [] [] and rerolled the patch.
Jonathan Nieder jrnie...@gmail.com wrote:
Hi Martin,
Martin Erik Werner wrote:
Minor clean up of if-then nesting in checks for environment
variables
and config options. No functional changes.
Yeah, the nesting was getting a little deep. Thanks for the cleanup.
May we have your sign-off?
Once this is signed off,
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
Meh, I keep on missing that :/
Old (and new) patch is:
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
Patch left unsnipped for reference.
---
contrib/completion/git-prompt.sh | 27
+--
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git
a/contrib/completion/git-prompt.sh
b/contrib/completion/git-prompt.sh
index 9b2eec2..e29694d 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -320,26 +320,25 @@ __git_ps1 ()
b=GIT_DIR!
fi
elif [ true = $(git rev-parse --is-inside-work-tree
2/dev/null) ]; then
- if [ -n ${GIT_PS1_SHOWDIRTYSTATE-} ]; then
-if [ $(git config --bool bash.showDirtyState) !=
false ]; then
- git diff --no-ext-diff --quiet --exit-code || w=*
- if git rev-parse --quiet --verify HEAD /dev/null; then
- git diff-index --cached --quiet HEAD -- || i=+
- else
- i=#
- fi
+ if test -n ${GIT_PS1_SHOWDIRTYSTATE-}
+ test $(git config --bool bash.showDirtyState) !=
false
+ then
+git diff --no-ext-diff --quiet --exit-code || w=*
+if git rev-parse --quiet --verify HEAD /dev/null; then
+ git diff-index --cached --quiet HEAD -- || i=+
+else
+ i=#
fi
fi
if [ -n ${GIT_PS1_SHOWSTASHSTATE-} ]; then
git rev-parse --verify refs/stash /dev/null 21 s=$
fi
- if [ -n ${GIT_PS1_SHOWUNTRACKEDFILES-} ]; then
-if [ $(git config --bool bash.showUntrackedFiles) !=
false ]; then
- if [ -n $(git ls-files --others --exclude-standard)
]; then
- u=%
- fi
-fi
+ if test -n ${GIT_PS1_SHOWUNTRACKEDFILES-}
+ test $(git config --bool bash.showUntrackedFiles) !=
false
+ test -n $(git ls-files --others --exclude-standard)
+ then
+u=%
fi
if [ -n ${GIT_PS1_SHOWUPSTREAM-} ];!
then
--
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/3] Add bash.showUntrackedFiles config option
On Tue, 2013-02-12 at 14:29 -0800, Junio C Hamano wrote: Martin Erik Werner martinerikwer...@gmail.com writes: Add a test case for the bash.showUntrackedFiles config option, which checks that the config option can disable the global effect of the GIT_PS1_SHOWUNTRACKEDFILES environmant variable. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com --- t/t9903-bash-prompt.sh | 11 +++ 1 file changed, 11 insertions(+) diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh index f17c1f8..c9417b9 100755 --- a/t/t9903-bash-prompt.sh +++ b/t/t9903-bash-prompt.sh @@ -447,6 +447,17 @@ test_expect_success 'prompt - untracked files status indicator - not shown insid test_cmp expected $actual ' +test_expect_success 'prompt - untracked files status indicator - disabled by config' ' + printf (master) expected + echo untracked file_untracked + test_config bash.showUntrackedFiles false + ( + GIT_PS1_SHOWUNTRACKEDFILES=y + __git_ps1 $actual + ) + test_cmp expected $actual +' All six combinations need checking: * not having the configuration at all and not having the shell variable should not show the untracked indicator (already tested). * not having the configuration at all and having the shell variable should show the untracked indicator (already tested). * setting configuration to true without having the shell variable should not show the untracked indicator. * setting configuration to true and having the shell variable should show the unttracked indicator. * setting configuration to false and having the shell variable should not show the untracked indicator (the above test checks this). * setting configuration to false without having the shell variable should not show the untracked indicator. to prevent others from breaking the code you wrote for [PATCH 1/2], so you need three more tests, I guess? Ah, yes, I was mimicing what the test did for bash.showDirtyState, I've now added the three extra tests for bash.showUntrackedFiles, which should cover all of the above cases, hopefully? I've also added in the three extra tests for bash.showDirtyState, equivalently. These only cover the case of dirty files and not combinations with content in index, which I felt was a bit overkill, is that reasonable? Thanks for the review :) -- Martin Erik Werner martinerikwer...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] shell prompt: add bash.showUntrackedFiles option
Add a config option 'bash.showUntrackedFiles' which allows enabling
the prompt showing untracked files on a per-repository basis. This is
useful for some repositories where the 'git ls-files ...' command may
take a long time.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
---
contrib/completion/git-prompt.sh | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 9bef053..9b2eec2 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -43,7 +43,10 @@
#
# If you would like to see if there're untracked files, then you can set
# GIT_PS1_SHOWUNTRACKEDFILES to a nonempty value. If there're untracked
-# files, then a '%' will be shown next to the branch name.
+# files, then a '%' will be shown next to the branch name. You can
+# configure this per-repository with the bash.showUntrackedFiles
+# variable, which defaults to true once GIT_PS1_SHOWUNTRACKEDFILES is
+# enabled.
#
# If you would like to see the difference between HEAD and its upstream,
# set GIT_PS1_SHOWUPSTREAM=auto. A indicates you are behind,
@@ -332,8 +335,10 @@ __git_ps1 ()
fi
if [ -n ${GIT_PS1_SHOWUNTRACKEDFILES-} ]; then
- if [ -n $(git ls-files --others
--exclude-standard) ]; then
- u=%
+ if [ $(git config --bool
bash.showUntrackedFiles) != false ]; then
+ if [ -n $(git ls-files --others
--exclude-standard) ]; then
+ u=%
+ fi
fi
fi
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/3] t9903: add tests for bash.showUntrackedFiles
Add 4 test for the bash.showUntrackedFiles config option, covering all combinations of the shell var being set/unset and the config option being enabled/disabled. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com --- t/t9903-bash-prompt.sh | 40 1 file changed, 40 insertions(+) diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh index f17c1f8..cb008e2 100755 --- a/t/t9903-bash-prompt.sh +++ b/t/t9903-bash-prompt.sh @@ -437,6 +437,46 @@ test_expect_success 'prompt - untracked files status indicator - untracked files test_cmp expected $actual ' +test_expect_success 'prompt - untracked files status indicator - shell variable unset with config disabled' ' + printf (master) expected + test_config bash.showUntrackedFiles false + ( + unset -v GIT_PS1_SHOWUNTRACKEDFILES + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success 'prompt - untracked files status indicator - shell variable unset with config enabled' ' + printf (master) expected + test_config bash.showUntrackedFiles true + ( + unset -v GIT_PS1_SHOWUNTRACKEDFILES + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success 'prompt - untracked files status indicator - shell variable set with config disabled' ' + printf (master) expected + test_config bash.showUntrackedFiles false + ( + GIT_PS1_SHOWUNTRACKEDFILES=y + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success 'prompt - untracked files status indicator - shell variable set with config enabled' ' + printf (master %%) expected + test_config bash.showUntrackedFiles true + ( + GIT_PS1_SHOWUNTRACKEDFILES=y + __git_ps1 $actual + ) + test_cmp expected $actual +' + test_expect_success 'prompt - untracked files status indicator - not shown inside .git directory' ' printf (GIT_DIR!) expected ( -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/3] t9903: add extra tests for bash.showDirtyState
Added 3 extra tests for the bash.showDirtyState config option, tests should now cover all combinations of the shell var being set/unset and the config option being enabled/disabled, given a dirty file. * Renamed test 'disabled by config' to 'shell variable set with config disabled' for consistency --- t/t9903-bash-prompt.sh | 38 +- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh index cb008e2..fa81b09 100755 --- a/t/t9903-bash-prompt.sh +++ b/t/t9903-bash-prompt.sh @@ -360,12 +360,48 @@ test_expect_success 'prompt - dirty status indicator - before root commit' ' test_cmp expected $actual ' -test_expect_success 'prompt - dirty status indicator - disabled by config' ' +test_expect_success 'prompt - dirty status indicator - shell variable unset with config disabled' ' printf (master) expected echo dirty file test_when_finished git reset --hard test_config bash.showDirtyState false ( + unset -v GIT_PS1_SHOWDIRTYSTATE + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success 'prompt - dirty status indicator - shell variable unset with config enabled' ' + printf (master) expected + echo dirty file + test_when_finished git reset --hard + test_config bash.showDirtyState true + ( + unset -v GIT_PS1_SHOWDIRTYSTATE + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success 'prompt - dirty status indicator - shell variable set with config disabled' ' + printf (master) expected + echo dirty file + test_when_finished git reset --hard + test_config bash.showDirtyState false + ( + GIT_PS1_SHOWDIRTYSTATE=y + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success 'prompt - dirty status indicator - shell variable set with config enabled' ' + printf (master *) expected + echo dirty file + test_when_finished git reset --hard + test_config bash.showDirtyState true + ( GIT_PS1_SHOWDIRTYSTATE=y __git_ps1 $actual ) -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/3] t9903: add tests for bash.showUntrackedFiles
On Wed, 2013-02-13 at 08:23 -0800, Junio C Hamano wrote: Martin Erik Werner martinerikwer...@gmail.com writes: Add 4 test for the bash.showUntrackedFiles config option, covering all combinations of the shell var being set/unset and the config option being enabled/disabled. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com --- t/t9903-bash-prompt.sh | 40 1 file changed, 40 insertions(+) diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh index f17c1f8..cb008e2 100755 --- a/t/t9903-bash-prompt.sh +++ b/t/t9903-bash-prompt.sh @@ -437,6 +437,46 @@ test_expect_success 'prompt - untracked files status indicator - untracked files test_cmp expected $actual ' +test_expect_success 'prompt - untracked files status indicator - shell variable unset with config disabled' ' + printf (master) expected + test_config bash.showUntrackedFiles false + ( + unset -v GIT_PS1_SHOWUNTRACKEDFILES We do not use unset -v anywhere else in our system. Shells mimicking SysV may choke on it. A Portable POSIX script can omit -v when unsetting a variable. Also unset can return false when the variable is not set to begin with with some shells. Neither of these matters for this particular case because we know we are running this under bash in non-posix mode. I however wonder if we can do something to prevent careless coders to copy and paste this piece when updating other tests that are not limited to bash. Commenting each and every use of unset -v does not sound like a good solution and perhaps I am being unnecessarily worried too much. Yeah, my (ba)sh foo is a bit limited, I was just basing on http://wiki.bash-hackers.org/commands/builtin/unset#portability_considerations which seemed to recommend using -v. So would it make sense to do: GIT_PS1_SHOWUNTRACKEDFILES=dummy unset GIT_PS1_SHOWUNTRACKEDFILES (...) instead then? -- Martin Erik Werner martinerikwer...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] t9903: add extra tests for bash.showDirtyState
On Wed, 2013-02-13 at 08:28 -0800, Junio C Hamano wrote: Martin Erik Werner martinerikwer...@gmail.com writes: Added 3 extra tests for the bash.showDirtyState config option, tests should now cover all combinations of the shell var being set/unset and the config option being enabled/disabled, given a dirty file. Strictly speaking, you have 6 not 4 combinations (shell variable set/unset * config missing/set to false/set to true). I think these additional tests cover should all 6 because config missing case should already have had tests before bash.showDirtyState was added. Indeed, I only mentioned 4 since the other ones existed already, and I didn't change them, but maybe it should be mentioned as combined with previous tests (...) cover all 6 combinations (...) then? * Renamed test 'disabled by config' to 'shell variable set with config disabled' for consistency --- Sign-off? Ah, just forgot the -s flag on that commit, yes it should be Signed-off by me. t/t9903-bash-prompt.sh | 38 +- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh index cb008e2..fa81b09 100755 --- a/t/t9903-bash-prompt.sh +++ b/t/t9903-bash-prompt.sh @@ -360,12 +360,48 @@ test_expect_success 'prompt - dirty status indicator - before root commit' ' test_cmp expected $actual ' -test_expect_success 'prompt - dirty status indicator - disabled by config' ' +test_expect_success 'prompt - dirty status indicator - shell variable unset with config disabled' ' printf (master) expected echo dirty file test_when_finished git reset --hard test_config bash.showDirtyState false ( + unset -v GIT_PS1_SHOWDIRTYSTATE + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success 'prompt - dirty status indicator - shell variable unset with config enabled' ' + printf (master) expected + echo dirty file + test_when_finished git reset --hard + test_config bash.showDirtyState true + ( + unset -v GIT_PS1_SHOWDIRTYSTATE + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success 'prompt - dirty status indicator - shell variable set with config disabled' ' + printf (master) expected + echo dirty file + test_when_finished git reset --hard + test_config bash.showDirtyState false + ( + GIT_PS1_SHOWDIRTYSTATE=y + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success 'prompt - dirty status indicator - shell variable set with config enabled' ' + printf (master *) expected + echo dirty file + test_when_finished git reset --hard + test_config bash.showDirtyState true + ( GIT_PS1_SHOWDIRTYSTATE=y __git_ps1 $actual ) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] t9903: add extra tests for bash.showDirtyState
On Wed, 2013-02-13 at 11:53 -0800, Junio C Hamano wrote: Martin Erik Werner martinerikwer...@gmail.com writes: Strictly speaking, you have 6 not 4 combinations (shell variable set/unset * config missing/set to false/set to true). I think these additional tests cover should all 6 because config missing case should already have had tests before bash.showDirtyState was added. Indeed, I only mentioned 4 since the other ones existed already, and I didn't change them, but maybe it should be mentioned as combined with previous tests (...) cover all 6 combinations (...) then? It should be sufficient to change the third line of your original to say the config option being missing/enabled/disabled, given a dirty file. and nothing else, I think. Sign-off? Ah, just forgot the -s flag on that commit, yes it should be Signed-off by me. OK, I'll locally amend the patch. Thanks. Ok, so I shouldn't reroll them with s/unset -v/sane_unset/ and reworded commits + sign-off then, I can if you prefer that? -- Martin Erik Werner martinerikwer...@gmail.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/3] t9903: add tests for bash.showUntrackedFiles
Add 4 test for the bash.showUntrackedFiles config option, the tests now cover all combinations of the shell var being set/unset and the config option being missing/enabled/disabled. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com --- t/t9903-bash-prompt.sh | 40 1 file changed, 40 insertions(+) diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh index f17c1f8..dd9ac6a 100755 --- a/t/t9903-bash-prompt.sh +++ b/t/t9903-bash-prompt.sh @@ -437,6 +437,46 @@ test_expect_success 'prompt - untracked files status indicator - untracked files test_cmp expected $actual ' +test_expect_success 'prompt - untracked files status indicator - shell variable unset with config disabled' ' + printf (master) expected + test_config bash.showUntrackedFiles false + ( + sane_unset GIT_PS1_SHOWUNTRACKEDFILES + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success 'prompt - untracked files status indicator - shell variable unset with config enabled' ' + printf (master) expected + test_config bash.showUntrackedFiles true + ( + sane_unset GIT_PS1_SHOWUNTRACKEDFILES + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success 'prompt - untracked files status indicator - shell variable set with config disabled' ' + printf (master) expected + test_config bash.showUntrackedFiles false + ( + GIT_PS1_SHOWUNTRACKEDFILES=y + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success 'prompt - untracked files status indicator - shell variable set with config enabled' ' + printf (master %%) expected + test_config bash.showUntrackedFiles true + ( + GIT_PS1_SHOWUNTRACKEDFILES=y + __git_ps1 $actual + ) + test_cmp expected $actual +' + test_expect_success 'prompt - untracked files status indicator - not shown inside .git directory' ' printf (GIT_DIR!) expected ( -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/3] t9903: add extra tests for bash.showDirtyState
Add 3 extra tests for the bash.showDirtyState config option, the test now cover all combinations of the shell var being set/unset and the config option being missing/enabled/disabled, given a dirty file. * Renamed test 'disabled by config' to 'shell variable set with config disabled' for consistency Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com --- t/t9903-bash-prompt.sh | 38 +- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh index dd9ac6a..2101d91 100755 --- a/t/t9903-bash-prompt.sh +++ b/t/t9903-bash-prompt.sh @@ -360,12 +360,48 @@ test_expect_success 'prompt - dirty status indicator - before root commit' ' test_cmp expected $actual ' -test_expect_success 'prompt - dirty status indicator - disabled by config' ' +test_expect_success 'prompt - dirty status indicator - shell variable unset with config disabled' ' printf (master) expected echo dirty file test_when_finished git reset --hard test_config bash.showDirtyState false ( + sane_unset GIT_PS1_SHOWDIRTYSTATE + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success 'prompt - dirty status indicator - shell variable unset with config enabled' ' + printf (master) expected + echo dirty file + test_when_finished git reset --hard + test_config bash.showDirtyState true + ( + sane_unset GIT_PS1_SHOWDIRTYSTATE + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success 'prompt - dirty status indicator - shell variable set with config disabled' ' + printf (master) expected + echo dirty file + test_when_finished git reset --hard + test_config bash.showDirtyState false + ( + GIT_PS1_SHOWDIRTYSTATE=y + __git_ps1 $actual + ) + test_cmp expected $actual +' + +test_expect_success 'prompt - dirty status indicator - shell variable set with config enabled' ' + printf (master *) expected + echo dirty file + test_when_finished git reset --hard + test_config bash.showDirtyState true + ( GIT_PS1_SHOWDIRTYSTATE=y __git_ps1 $actual ) -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] Add bash.showUntrackedFiles config option
Hi, Here is a patch adding a config option for showing untracked files in the shell prompt, I've noticed having it enabled tends to make the prompt act very sluggish in some cases (large repos / unfriendly filesystems). So it would be nice to have a more fine-grained control over it, similar to what exists for bash.showDirtyState. Martin Erik Werner (2): bash completion: add bash.showUntrackedFiles option t9903: add test case for bash.showUntrackedFiles contrib/completion/git-prompt.sh | 11 --- t/t9903-bash-prompt.sh | 11 +++ 2 files changed, 19 insertions(+), 3 deletions(-) -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] t9903: add test case for bash.showUntrackedFiles
Add a test case for the bash.showUntrackedFiles config option, which checks that the config option can disable the global effect of the GIT_PS1_SHOWUNTRACKEDFILES environmant variable. Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com --- t/t9903-bash-prompt.sh | 11 +++ 1 file changed, 11 insertions(+) diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh index f17c1f8..c9417b9 100755 --- a/t/t9903-bash-prompt.sh +++ b/t/t9903-bash-prompt.sh @@ -447,6 +447,17 @@ test_expect_success 'prompt - untracked files status indicator - not shown insid test_cmp expected $actual ' +test_expect_success 'prompt - untracked files status indicator - disabled by config' ' + printf (master) expected + echo untracked file_untracked + test_config bash.showUntrackedFiles false + ( + GIT_PS1_SHOWUNTRACKEDFILES=y + __git_ps1 $actual + ) + test_cmp expected $actual +' + test_expect_success 'prompt - format string starting with dash' ' printf -- -master expected __git_ps1 -%s $actual -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] bash completion: add bash.showUntrackedFiles option
Add a config option 'bash.showUntrackedFiles' which allows enabling
the prompt showing untracked files on a per-repository basis. This is
useful for some repositories where the 'git ls-files ...' command may
take a long time.
Signed-off-by: Martin Erik Werner martinerikwer...@gmail.com
---
contrib/completion/git-prompt.sh | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 9bef053..9b2eec2 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -43,7 +43,10 @@
#
# If you would like to see if there're untracked files, then you can set
# GIT_PS1_SHOWUNTRACKEDFILES to a nonempty value. If there're untracked
-# files, then a '%' will be shown next to the branch name.
+# files, then a '%' will be shown next to the branch name. You can
+# configure this per-repository with the bash.showUntrackedFiles
+# variable, which defaults to true once GIT_PS1_SHOWUNTRACKEDFILES is
+# enabled.
#
# If you would like to see the difference between HEAD and its upstream,
# set GIT_PS1_SHOWUPSTREAM=auto. A indicates you are behind,
@@ -332,8 +335,10 @@ __git_ps1 ()
fi
if [ -n ${GIT_PS1_SHOWUNTRACKEDFILES-} ]; then
- if [ -n $(git ls-files --others
--exclude-standard) ]; then
- u=%
+ if [ $(git config --bool
bash.showUntrackedFiles) != false ]; then
+ if [ -n $(git ls-files --others
--exclude-standard) ]; then
+ u=%
+ fi
fi
fi
--
1.7.10.4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
