Re: [PATCH] repack.c: rename and unlink pack file if it exists
On 2014-02-05 02.16, Jeff King wrote: On Tue, Feb 04, 2014 at 03:40:15PM -0800, Junio C Hamano wrote: * Somehow this came to my private mailbox without Cc to list, so I am forwarding it. I think with 1190a1ac (pack-objects: name pack files after trailer hash, 2013-12-05), repacking the same set of objects may have less chance of producing colliding names, especially if you are on a box with more than one core, but it still would be a good idea to get this part right in the upcoming release. Actually, since 1190a1ac, if you have repacked and gotten the same pack name, then you do not have to do any rename dance at all; you can throw away what you just generated because you know that it is byte-for-byte identical. You could collide with a pack created by an older version of git that used the original scheme, but that is quite unlikely (on the order of 2^-160). -Peff OK, I messed up the email so it went only to Junios mailbox. Sorry for confusion. Jochen, it could be good if you could test this version of the patch (I couldn't reproduce the problem here) /Torsten -- 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] repack.c: rename and unlink pack file if it exists
On 2014-02-05 21.31, Junio C Hamano wrote: Jeff King p...@peff.net writes: The minimal fix you posted below does make sense to me as a stopgap, and we can look into dropping the code entirely during the next cycle. It would be nice to have a test to cover this case, though. Sounds sensible. Run repack -a -d once, and then another while forcing it to be single threaded, or something? I can put a test case on my todo list, and thanks for the minimal patch. -- 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] fast-import.c: always honor the filename case
On 2014-02-03 23.11, Reuben Hawkins wrote:
On Mon, Feb 3, 2014 at 2:21 PM, Torsten Bögershausen tbo...@web.de
mailto:tbo...@web.de wrote:
[]
So to summarize, when fast-import uses strncmp_icase (what fast-import
does now) import on a repository where ignorecase=true is wrong. My patch,
fast-import.c: always honor the filename case fixes this. Can you verify?
Thanks in advance,
Reuben
Yes, I can verify. My feeling is that
a) the fast-export should generate the rename the other way around.
Would that be feasable ?
I *think* this is feasible. I did try this, and it worked, but I didn't like
the idea of having to fix all the exporters. I know about hg-export and
svn-export, but I assume there are more that I don't know about, and maybe
even other tools out there none of us know about, which would also have to be
fixed in the same way. As such, I decided fixing fast-export isn't really
the right thing to do...I don't really think fast-export is broken to begin
with. I'm hoping there is a way to fix ignorecase such that it doesn't
create this type of problem with this...
M 100644 :1 Filename.txt
D FileName.txt
..maybe by very carefully identifying when ignorecase should apply and when
it shouldn't (I'm still trying to sort that out, the docs on ignorecase
aren't specific).
But for what it's worth, to fix fast-export, I added a check in
builtin/fast-export.c in the function depth_first before all the other checks
so it would always make diff_filepair-status == 'D' the lesser when not
equal...something like this (I'm not looking at the code now, so this may
*not* be what I did)...
if (a-status != b-status) {
if (a-status == 'D') return -1;
if (b-status == 'D') return 1;
}
/Reuben
Or generate a real rename ?
A rename may also work, but it may not. And that would also require fixing
all other exporters. If I understand the docs well enough, a rename would be
done like so...
R Filename.txt FileName.txt
I think in the ignorecase=true situation, case folding would happen and this
would be a nop like this...
R Filename.txt Filename.txt
...Right? I haven't tested this, but I *suspect* the result would be to not
rename the file when ignorecase=true...I definitely think it's worth a try
just to know the result, but this fixes the ignorecase problem in fast-import
by passing a requirement to all the fast-exporters...a semi-artificial
requirement created because ignorecase *could* be true, but may not be.
/Reuben
(I'm not using fast-export or import myself)
b) As a workaround, does it help if you manually set core.ignorecase
false ?
Yes, this works. It makes a single step clone, git clone hg::..., into a
multi step process like this...
$ mkdir test
$ git init
$ git config core.ignorecase false
$ git remote add origin hg::whatever
$ git fetch origin
$ git reset --hard origin/master
$ git branch --set-upstream-to=origin/master master
That isn't a too big deal for people fluent in GIT (if you only have to do it
once and wrote it down too maybe). It works, just not ideally and it's easy
to get burned on because git clone sort-of works, it just doesn't truly
clone. The resulting cloned repo can be mangled by case folding.
Typically, unless somebody explains the multi step process to everybody, some
people will have master with commit xx and others will have the exact
same master with commit yy. Some will have Filename.txt instead of
FileName.txt way back in history. Merging those branches is a mess.
So setting core.ignorecase=flase does work, it's just a bit cumbersome. My
fingers really want to just type git clone hg::whatever and I hope to get a
true 'clone' as in an exact, identical copy on all machines regardless of
filesystem. If GIT wants to do case folding after that I suppose it would be
fine. Maybe I'm expecting too much, but I've been under the impression for
years that a clone of any git repo will have all the exact same commit id's.
/Reuben
c) Does it help to use git-hg-remote ? (could be another workaround)
Yes, sorry, I guess I wasn't clear on that point. That is what I'm using.
/Reuben
And no, 50906e04e8f48215b0 does not include any test cases.
(try git show 50906e04e8)
This is only a short answer, I can prepare a longer answer about
ignorecase the next days.
/Torsten
Thank you! That would be very helpful. I'm still trying to wrap my head
around what ignorecase really needs to do, when and where it needs to do it
and what it shouldn't do. I suspect ignorecase is touching too many code
paths and needs to be reined in a bit.
I'm also wondering if it's possible to test a bunch of situations in 'make
tests' with ignorecase=true/falsebut I can't think of any way other than
mounting filesystems on loopbacks to setup the tests
Re: [PATCH v6 4/6] t0060: Add tests for prefix_path when path begins with work tree
On 2014-02-04 15.25, Martin Erik Werner wrote: 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) +' I think we need SYMLINKS here. -- 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] fast-import.c: always honor the filename case
[] So to summarize, when fast-import uses strncmp_icase (what fast-import does now) import on a repository where ignorecase=true is wrong. My patch, fast-import.c: always honor the filename case fixes this. Can you verify? Thanks in advance, Reuben Yes, I can verify. My feeling is that a) the fast-export should generate the rename the other way around. Would that be feasable ? Or generate a real rename ? (I'm not using fast-export or import myself) b) As a workaround, does it help if you manually set core.ignorecase false ? c) Does it help to use git-hg-remote ? (could be another workaround) And no, 50906e04e8f48215b0 does not include any test cases. (try git show 50906e04e8) This is only a short answer, I can prepare a longer answer about ignorecase the next days. /Torsten -- 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 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?
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]))
--
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
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? So true, sorry for the noise. -- 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: [msysGit] Re: [PATCH v2] repack.c: Use move_temp_to_file() instead of rename()
(It seems as if the mail went only to Junio, sorry)
On 2014-02-02 16.09, Torsten Bögershausen wrote:
On 2014-01-29 19.17, Junio C Hamano wrote:
But after a closer inspection, I no longer think that hunk is an
improvement. These new packfiles were created by pack-objects,
which finishes each packfile it produces by calling
finish_tmp_packfile(), and that is where adjust_shared_perm()
happens already. As far as pack-objects that was called from
repack is concerned, these new packfiles are not temporary; they
are finished product. It may be OK to remove them as part of
rewind back to the original state, as a later phase of repack
failed if we saw a failure (but note that the original
git-repack.sh didn't), but a plain vanilla rename(2) without any
frills is what we want to happen to them.
Thanks for deeper inspection, I now suspect the root cause to be here:
-- 8 --
Subject: [PATCH v3] repack.c: Rename and unlink pack file if it exists
This comment in builtin/repack.c:
* First see if there are packs of the same name and if so
* if we can move them out of the way (this can happen if we
* repacked immediately after packing fully.
indicates that when a repo was fully repacked, and is repacked again,
we may run into the situation that new packfiles have the name
(and content) as already existing ones.
The logic is to rename the existing ones into filename like old-XXX,
create the new ones and remove the old- ones.
When something went wrong, a manual roll-back could be done be renaming
the old- files.
The renaming into old- did not work as designed, because file_exists()
was done on the wrong file name.
This could give problems for a repo on a network share under Windows,
as reported by Jochen Haag zwanzi...@googlemail.com
Solution:
Create the right file name, like this:
fname = mkpathdup(%s/pack-%s%s, packdir,
and when removing the old-files:
fname_old = mkpath(%s/old-%s%s,
Rename the variables to match what they are used for:
fname, fname_old and fname_tmp
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
builtin/repack.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index 6284846..de69401 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -258,7 +258,7 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
for_each_string_list_item(item, names) {
for (ext = 0; ext 2; ext++) {
char *fname, *fname_old;
- fname = mkpathdup(%s/%s%s, packdir,
+ fname = mkpathdup(%s/pack-%s%s, packdir,
item-string, exts[ext]);
if (!file_exists(fname)) {
free(fname);
@@ -314,33 +314,33 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
/* Now the ones with the same name are out of the way... */
for_each_string_list_item(item, names) {
for (ext = 0; ext 2; ext++) {
- char *fname, *fname_old;
+ char *fname, *fname_tmp;
struct stat statbuffer;
fname = mkpathdup(%s/pack-%s%s,
packdir, item-string, exts[ext]);
- fname_old = mkpathdup(%s-%s%s,
+ fname_tmp = mkpathdup(%s-%s%s,
packtmp, item-string, exts[ext]);
- if (!stat(fname_old, statbuffer)) {
+ if (!stat(fname_tmp, statbuffer)) {
statbuffer.st_mode = ~(S_IWUSR | S_IWGRP |
S_IWOTH);
- chmod(fname_old, statbuffer.st_mode);
+ chmod(fname_tmp, statbuffer.st_mode);
}
- if (rename(fname_old, fname))
- die_errno(_(renaming '%s' failed), fname_old);
+ if (rename(fname_tmp, fname))
+ die_errno(_(renaming '%s' into '%s' failed),
fname_tmp, fname);
free(fname);
- free(fname_old);
+ free(fname_tmp);
}
}
/* Remove the old- files */
for_each_string_list_item(item, names) {
for (ext = 0; ext 2; ext++) {
- char *fname;
- fname = mkpath(%s/old-pack-%s%s,
+ char *fname_old;
+ fname_old = mkpath(%s/old-%s%s,
packdir,
item-string,
exts[ext]);
- if (remove_path(fname))
- warning(_(removing '%s' failed), fname);
+ if (remove_path(fname_old
Re: [PATCH] fast-import.c: always honor the filename case
On 2014-02-02 14.13, Reuben Hawkins wrote: fast-import should not use strncmp_icase. When it does, files with similar names, but different case can be lost in the import. For example... M 100644 :1 FileName.txt D Filename.txt That seems to be wrong, shouldn't it be D Filename.txt M 100644 :1 FileName.txt How do you generate the export/import stream ? Please see here: https://www.kernel.org/pub/software/scm/git/docs/git-fast-import.html Handling Renames When importing a renamed file or directory, simply delete the old name(s) and modify the new name(s) during the corresponding commit. Git performs rename detection after-the-fact, rather than explicitly during a commit. -- 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 1/2] init-db.c: honor case on case preserving fs
On 2014-02-01 10.14, Reuben Hawkins wrote: Most case-insensitive filesystems are case-preserving. In these filesystems (such as HFS+ on OS X) you can name a file Filename.txt, then rename the file to FileName.txt. That file will be accessible by both filenames, but the case is otherwise honored. We don't want to have git ignore case on these case-preserving filesystem implementations. Yes, we want. Because the file system will treat Filename.txt and FileName.txt the same. Whatever is on disc, the OS will not distinguish them. (On a case-insensitive HFS+ partition). And when core.ignorecase == true, Git does the same what the OS does, ignore the case. Could you describe the problems more in detail ? Could you supply a test case, (or a short script) which shows the problem and makes it reproducable for others? Which problems does your patch solve, which can not be solved by setting core.ignorecase==false manually? /Torsten -- 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 v3 3/4] setup: Add 'abspath_part_inside_repo' function
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)
[
]
--
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] add: don't complain when adding empty project root
[] filepattern is related to current directory too (e.g. *.sh from t won't cover git-rebase.sh, :/*.sh does). Yes a patch to update git-add.txt to use the term pathspec instead of filepattern would be nice. A pointer to pathspec glossary could help discover case-insensitive matching, negative matching.. To somewhat end this thread: I haven't been able to find a patch that really improves the documentation, because Documentation/git-add.txt has already been updated: commit 30784198b766b19a639c199e4365f2a805fc08c6 Author: Junio C Hamano gits...@pobox.com Date: Thu Feb 14 15:51:43 2013 -0800 Documentation/git-add: kill remaining filepattern And all changes are here: http://git-htmldocs.googlecode.com/git/git-add.html But, look at https://www.kernel.org/pub/software/scm/git/docs/git-add.html This page seems to need an update too, and I wonder why: a) The makefile did'nt re-generate html even if it should have b) That page is not owned or updated by the git.git maintainer c) Any other reason? Does anybody know how to trigger an update at kernel.org? /Torsten -- 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] repack.c: Use move_temp_to_file() instead of rename()
In a1bbc6c0 a shell command mv -f was replaced with the rename() function.
Use move_temp_to_file() from sha1_file.c instead of rename().
This is in line with the handling of other Git internal tmp files,
and calls adjust_shared_perm()
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
Thanks for all comments.
I haven't been able to reproduce the problem here.
Tips and information how to reproduce it are wellcome.
I think this patch makes sense to support core.sharedRepository(),
but I haven't made a test case for the pack/idx files.
Jochen, doess this patch fix your problem, or do we need
another patch on top of this?
builtin/repack.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index ba66c6e..4b6d663 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -271,7 +271,7 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
if (unlink(fname_old))
failed = 1;
- if (!failed rename(fname, fname_old)) {
+ if (!failed move_temp_to_file(fname, fname_old)) {
free(fname);
failed = 1;
break;
@@ -288,7 +288,7 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
char *fname, *fname_old;
fname = mkpathdup(%s/%s, packdir, item-string);
fname_old = mkpath(%s/old-%s, packdir, item-string);
- if (rename(fname_old, fname))
+ if (move_temp_to_file(fname_old, fname))
string_list_append(rollback_failure, fname);
free(fname);
}
@@ -324,7 +324,7 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
statbuffer.st_mode = ~(S_IWUSR | S_IWGRP |
S_IWOTH);
chmod(fname_old, statbuffer.st_mode);
}
- if (rename(fname_old, fname))
+ if (move_temp_to_file(fname_old, fname))
die_errno(_(renaming '%s' failed), fname_old);
free(fname);
free(fname_old);
--
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] repack.c: chmod +w before rename()
commit a1bbc6c0 repack: rewrite the shell script in C introduced
a possible regression, when a Git repo is located on a Windows network share.
When git gc is called on an already packed repository, it could fail like this:
fatal: renaming '.git/objects/pack/.tmp-xyz.pack' failed: Permission denied
Temporary *.pack and *.idx files remain in .git/objects/pack/
In a1bbc6c0 the rename() function replaced the mv -f shell command.
Obviously the -f option can also override a read-only file but
rename() can not on a network share.
Make the C-code to work similar to the shell script, by making the
files writable before calling rename().
Another solution could be to do the chmod +x in mingw_rename().
This may be done in another commit, because
a) It improves git gc only when Git for Windows is used
on the client machine
b) Windows refuses to delete a file when the file is read-only.
So setting a file to read-only under Windows is a way for a user
to protect it from being deleted.
Changing the behaviour of rename() globally may not be what we want.
Reported-by: Jochen Haag zwanzi...@googlemail.com
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
builtin/repack.c | 4
1 file changed, 4 insertions(+)
diff --git a/builtin/repack.c b/builtin/repack.c
index ba66c6e..033b4c2 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -324,6 +324,10 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
statbuffer.st_mode = ~(S_IWUSR | S_IWGRP |
S_IWOTH);
chmod(fname_old, statbuffer.st_mode);
}
+ if (!stat(fname, statbuffer)) {
+ statbuffer.st_mode |= (S_IWUSR | S_IWGRP |
S_IWOTH);
+ chmod(fname, statbuffer.st_mode);
+ }
if (rename(fname_old, fname))
die_errno(_(renaming '%s' failed), fname_old);
free(fname);
--
1.9.rc0.143.g6fd479e
--
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: libz and RHEL 5.9 compile of Git
On 2014-01-22 22.27, salmansheikh wrote: Got it working but then I had some issues with the perl portions of the install and I subsequently thought I could eliminate those portions and tried setting export NO_PERL=1 and that installed everything else...and got pass this error but when I tried to checkout a git repository as follows, I get some remote helper error. Is that related to the perl parts of the git? No git clone https://github.com/m-labs/migen.git Cloning into 'migen'... fatal: Unable to find remote helper for 'https' Please have a look at libcurl. It seems that libcurl on your system does not support https The Makefile of Git is your friend: # Define CURLDIR=/foo/bar if your curl header and library files are in # /foo/bar/include and /foo/bar/lib directories. -- 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: libz and RHEL 5.9 compile of Git
On 2014-01-22 16.59, salmansheikh wrote: Hello, I have a RHEL system that I am not the admin of. I needed to install git and got the source. Everything is okay until I got to this point below. I downloaded and installed the latest libz (1.2.8) but i installed it under a local directory under my user name (i.e. /home/ssheikh/local). The problem is that git only looks in the locations below. I even have that directory in my $LD_LIBRARY_PATH. So, how can I force make to use that version of libz and not the old one that came with this RHEL 5.9 distro? [ssheikh@gs-560g3080090e git-1.8.3.4]$ make LINK git-credential-store /usr/bin/ld: skipping incompatible /lib/libz.so when searching for -lz /usr/bin/ld: skipping incompatible /usr/lib/libz.so when searching for -lz /usr/bin/ld: skipping incompatible /usr/lib/libz.a when searching for -lz /usr/bin/ld: cannot find -lz collect2: ld returned 1 exit status make: *** [git-credential-store] Error 1 You need to tell the linker where to search for the library. Please have a look at the Makefile: ifdef ZLIB_PATH BASIC_CFLAGS += -I$(ZLIB_PATH)/include EXTLIBS += -L$(ZLIB_PATH)/$(lib) $(CC_LD_DYNPATH)$(ZLIB_PATH)/$(lib) endif -- 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: [msysGit] Consecutive git gc fails on Windows network share
--
Am Freitag, 17. Januar 2014 19:02:07 UTC+1 schrieb Torsten Bögershausen:
So, please, what happens if you revert that commit?
It could be good if you can test it and share the results with the list
/Torsten
Instead of reverting we did some more analysis.
In repack.c we found the following code:
/* Now the ones with the same name are out of the way... */
for_each_string_list_item(item, names) {
for (ext = 0; ext 2; ext++) {
char *fname, *fname_old;
struct stat statbuffer;
fname = mkpathdup(%s/pack-%s%s,
packdir, item-string, exts[ext]);
fname_old = mkpathdup(%s-%s%s,
packtmp, item-string, exts[ext]);
if (!stat(fname_old, statbuffer)) {
statbuffer.st_mode = ~(S_IWUSR | S_IWGRP |
S_IWOTH);
chmod(fname_old, statbuffer.st_mode);
}
if (rename(fname_old, fname))
die_errno(_(renaming '%s' failed),
fname_old);
free(fname);
free(fname_old);
}
}
The rename command replaces a mv -f command of the original shell script.
Obviously the -f option can also override a read-only file but rename can not
on a network share.
We changed the code as followed:
/* Now the ones with the same name are out of the way... */
for_each_string_list_item(item, names) {
for (ext = 0; ext 2; ext++) {
char *fname, *fname_old;
struct stat statbuffer;
fname = mkpathdup(%s/pack-%s%s,
packdir, item-string, exts[ext]);
fname_old = mkpathdup(%s-%s%s,
packtmp, item-string, exts[ext]);
if (!stat(fname_old, statbuffer)) {
statbuffer.st_mode = ~(S_IWUSR | S_IWGRP |
S_IWOTH);
chmod(fname_old, statbuffer.st_mode);
}
+++ if (!stat(fname, statbuffer)) {
+++ statbuffer.st_mode |= (S_IWUSR | S_IWGRP |
S_IWOTH);
+++ chmod(fname, statbuffer.st_mode);
+++ }
if (rename(fname_old, fname))
die_errno(_(renaming '%s' failed),
fname_old);
free(fname);
free(fname_old);
}
}
Before rename is called the destination file is made writeable. This worked
for us. We are not sure if this is a good implementation.
Jochen,
thanks for sharing the code changes with us.
I allowed me to
a) reconstruct the original mail,
b) add +++ at the places where you added the stat() and chmod(),
c) and to send the question is this a good implementation ? to upstream git.
I think your implementation makes sense.
/Torsten
--
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/WIP v2 03/14] read-cache: connect to file watcher
On 2014-01-17 10.47, Nguyễn Thái Ngọc Duy wrote: [snip[ diff --git a/file-watcher-lib.c b/file-watcher-lib.c +int connect_watcher(const char *path) Could it be worth to check if we can use some code from unix-socket.c ? Especially important could be that unix_sockaddr_init() wotks around a problem when long path names are used. -- 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: file permissions in Git repo
On 01/17/2014 03:26 AM, Jeff King wrote: On Thu, Jan 16, 2014 at 03:58:57PM -0800, SH wrote: We have a repository which holds lots of shell and perl scripts. We add the files to repository (from windows client) with executable permissions (using cygwin) but when we pull that repository on another machine (windows or linux), files dont have executable permission. Can you please provide a solutions for this? Git does not preserve file permissions _except_ for the executable bit. So this should be working. However, I suspect that `core.fileMode` is set to `false` in your repository, which causes git to ignore the executable bit. When a repository is initialized, we check whether the filesystem simply creates everything with the executable bit. If so, we turn off core.fileMode for the repository (since otherwise every file would have it set). -Peff Cygwin has been a little bit special (and mingw still is). Until this commit: Author: Junio C Hamano gits...@pobox.com Date: Wed Jul 24 19:22:49 2013 -0700 Merge branch 'ml/cygwin-updates' The tip one does _not_ revert c869753e (Force core.filemode to false on Cygwin., 2006-12-30) on purpose, so that people can still retain the old behaviour if they wanted to. * ml/cygwin-updates: cygwin: stop forcing core.filemode=false Cygwin 1.7 supports mmap Cygwin 1.7 has thread-safe pread Cygwin 1.7 needs compat/regex the repositories created by cygwin had always core.filemode=false. You can easily check your configuration by running git config -l on the cygwin machine, as Peff suggested. The next step is to check how the files had been recored in git, using git ls-files -s | less on any machine. If I do this on git.git, we find lines like this, where 100755 means an executable file, 100644 means non-executable file. 100755 9c3f4131b8586408acd81d1e60912b51688575ed 0 Documentation/technical/api-index.sh 100644 dd894043ae8b04269b3aa2108f96cb935217181d 0 Documentation/technical/api-lockfile.txt The 3rd step is to check how file are shown in cygwin, run ls -l (Do they have the executable bit set ?) Side note: And I think the way the auto-probing of the file system works is like this: When a git repo is initialized, the .git/config file is created. After that, we try to toggle the executable bit, and if lstat reports it as toggled, we set core.filemode=true. (See builtin/init-db.c, search for core.filemode) I tested to create a repo on a network share exported by SAMBA. The Server was configured so that all new created files had the executable bit set by default. Git detected that the executable bit could be switched off, and configured core.filemode=true Nice. /Torsten -- 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: file permissions in Git repo
(Please no top posting next time) On 2014-01-17 20.20, SH wrote: On Friday, January 17, 2014 10:08 AM, Torsten Bögershausen tbo...@web.de wrote: On 01/17/2014 03:26 AM, Jeff King wrote: On Thu, Jan 16, 2014 at 03:58:57PM -0800, SH wrote: We have a repository which holds lots of shell and perl scripts. We add the files to repository (from windows client) with executable permissions (using cygwin) but when we pull that repository on another machine (windows or linux), files dont have executable permission. Can you please provide a solutions for this? Git does not preserve file permissions _except_ for the executable bit. So this should be working. However, I suspect that `core.fileMode` is set to `false` in your repository, which causes git to ignore the executable bit. When a repository is initialized, we check whether the filesystem simply creates everything with the executable bit. If so, we turn off core.fileMode for the repository (since otherwise every file would have it set). -Peff Cygwin has been a little bit special (and mingw still is). Until this commit: Author: Junio C Hamano gits...@pobox.com Date: Wed Jul 24 19:22:49 2013 -0700 Merge branch 'ml/cygwin-updates' The tip one does _not_ revert c869753e (Force core.filemode to false on Cygwin., 2006-12-30) on purpose, so that people can still retain the old behaviour if they wanted to. * ml/cygwin-updates: cygwin: stop forcing core.filemode=false Cygwin 1.7 supports mmap Cygwin 1.7 has thread-safe pread Cygwin 1.7 needs compat/regex the repositories created by cygwin had always core.filemode=false. You can easily check your configuration by running git config -l on the cygwin machine, as Peff suggested. The next step is to check how the files had been recored in git, using git ls-files -s | less on any machine. If I do this on git.git, we find lines like this, where 100755 means an executable file, 100644 means non-executable file. 100755 9c3f4131b8586408acd81d1e60912b51688575ed 0 Documentation/technical/api-index.sh 100644 dd894043ae8b04269b3aa2108f96cb935217181d 0 Documentation/technical/api-lockfile.txt The 3rd step is to check how file are shown in cygwin, run ls -l (Do they have the executable bit set ?) Side note: And I think the way the auto-probing of the file system works is like this: When a git repo is initialized, the .git/config file is created. After that, we try to toggle the executable bit, and if lstat reports it as toggled, we set core.filemode=true. (See builtin/init-db.c, search for core.filemode) I tested to create a repo on a network share exported by SAMBA. The Server was configured so that all new created files had the executable bit set by default. Git detected that the executable bit could be switched off, and configured core.filemode=true Nice. /Torsten Thanks guys. Sorry but one more question, like I mentioned we have hosted repositories so how do I make some configuration changes are server based so whether the client have those changes or not, it wouldn't matter. Also I have one client on linux and another one on windows (for my testing purpose) and I see that .git/config on both machines are little different. Is that normal? Thanks Again. That a config file has small differences could be normal: the server has typically core.bare true. About other differences, please don't heasitate to consult http://git-htmldocs.googlecode.com/git/git-config.html And if there are still questions, they are there to be answered here. /Torsten -- 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: Re* manpage for git-pull mentions a non-valid option -m in a comment
On 2014-01-14 19.26, Junio C Hamano wrote: Ivan Zakharyaschev i...@altlinux.org writes: Hello! git-1.8.4.4 The manpage for git-pull mentions -m in a comment: --edit, -e, --no-edit Invoke an editor before committing successful mechanical merge to further edit the auto-generated merge message, so that the user can explain and justify the merge. The --no-edit option can be used to accept the auto-generated message (this is generally discouraged). The --edit (or -e) option is still useful if you are giving a draft message with the -m option from the command line and want to edit it in the editor. but it is not accepted actually: I do not think git pull ever had the -m message option; what you are seeing probably is a fallout from attempting to share the documentation pieces between git pull and git merge too agressively without proofreading. It seems that there are two issues; here is an untested attempt to fix. -- 8 -- Subject: Documentaiotn: exclude irrelevant options from git pull ^^ (Small nit ??) -- 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: Git: Question about specific subtree usage
On 2014-01-12 01.23, THILLOSEN Andreas wrote: Hi, I have a question about a specific use case for subtrees, but I'm not I feel a little bit confused: Are you talking about git submodules? And you may want to have a look at the repo tool: https://code.google.com/p/git-repo/ HTH /Torsten -- 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: A question about the error: svn_fspath__is_canonical
On 2014-01-10 20.28, Jonathan Nieder wrote: Dan Kaplan wrote: Do you think it'll still work? Yes, that's why I suggested it. ;-) You might need to install the gcc-core, libcurl-devel, openssl-devel, and subversion-perl packages first. Regards, Jonathan Out of my head: You probably need to install even: make, expat-devel (or similar) -- 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] remote-hg: do not fail on invalid bookmarks
On 2013-12-29 12.30, Antoine Pelisse wrote: Mercurial can have bookmarks pointing to nullid (the empty root revision), while Git can not have references to it. When cloning or fetching from a Mercurial repository that has such a bookmark, the import will fail because git-remote-hg will not be able to create the corresponding reference. Warn the user about the invalid reference, and continue the import, instead of stopping right away. Signed-off-by: Antoine Pelisse apeli...@gmail.com --- contrib/remote-helpers/git-remote-hg | 3 +++ 1 file changed, 3 insertions(+) diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg index eb89ef6..12d850e 100755 --- a/contrib/remote-helpers/git-remote-hg +++ b/contrib/remote-helpers/git-remote-hg @@ -625,6 +625,9 @@ def list_head(repo, cur): def do_list(parser): repo = parser.repo for bmark, node in bookmarks.listbookmarks(repo).iteritems(): +if node == '': +warn(Ignoring invalid bookmark '%s', bmark) +continue bmarks[bmark] = repo[node] cur = repo.dirstate.branch() (Side note: ap/remote-hg-skip-null-bookmarks) When I run the test-suite like this: ~/projects/git/git.pu/contrib/remote-helpers$ debug=t verbose=t make test-hg-hg-git.sh All 11 test cases fail on my systems (Debian Wheezy and Mac OS X): [snip] WARNING: Ignoring invalid bookmark 'master' To hg::../hgrepo-git ! [remote rejected] master - master error: failed to push some refs to 'hg::../hgrepo-git' not ok 1 - executable bit # [snip] -- 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: aborted 'git fetch' leaves workspace unusable
On 2013-12-30 18.07, stephen_le...@stephe-leake.org wrote: I forgot to do 'ssh-add', so a 'git fetch' running under Windows Emacs Windows native emacs or emacs under cygwin ? tried to prompt for the ssh passphrase, could not find an ssh passphrase prompt program, and aborted. That left the workspace unusable: - .git/FETCH_HEAD is empty that causes 'git rev-parse FETCH_HEAD' to fail with a confusing error message. Would you mind to post the confusion error message here? Because some people may find it useful. - 'git fetch' just hangs after outputting: remote: Counting objects: 15, done. remote: Compressing objects: 100% (8/8), done. remote: Total 9 (delta 5), reused 0 (delta 0) even with -v --progress A fresh clone allowed me to continue working, but this will happen again, so I'd like a better fix. The fetch is from stephen_le...@git.savannah.gnu.org/emacs/elpa.git I'm running git 1.7.9 from Cygwin. This feels old, we have v1.8.5.2 as the latest version. I have access to Debian, where I can compile git and run it under the debugger, if that helps. I have not yet tried to reproduce this bug on Debian. This could be helpful: a) compile git under cygwin (try 1.8.5.2), and see if the problem is still there. b) Which version of cygwin do you have? c) If the same problem exist under Debian, debugging it could be helpfull, yes. If the same problem exist here, in some version of git, it would be helpful to test the latest version of git. (Which means compile debug) HTH /Torsten -- 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] add: don't complain when adding empty project root
On 2013-12-24 00.46, Duy Nguyen wrote: [snip] We don't complain about adding an empty directory before or after this patch. Ok, thanks for the explanation. I think that https://www.kernel.org/pub/software/scm/git/docs/git-add.html could deserve an update. My understanding is that filepattern is related to $GIT_DIR, but . is the current directory. I will be happy to write a patch, (or to review one, whatever comes first) /Torsten -- 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] add: don't complain when adding empty project root
On 2013-12-23 10.02, Nguyễn Thái Ngọc Duy wrote:
This behavior was added in 07d7bed (add: don't complain when adding
empty project root - 2009-04-28) then broken by 84b8b5d (remove
match_pathspec() in favor of match_pathspec_depth() -
2013-07-14). Reinstate it.
Noticed-by: Thomas Ferris Nicolaisen tfn...@gmail.com
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/add.c | 2 +-
t/t3700-add.sh | 4
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/builtin/add.c b/builtin/add.c
index 226f758..fbd3f3a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -544,7 +544,7 @@ int cmd_add(int argc, const char **argv, const char
*prefix)
for (i = 0; i pathspec.nr; i++) {
const char *path = pathspec.items[i].match;
- if (!seen[i]
+ if (!seen[i] pathspec.items[i].match[0]
((pathspec.items[i].magic
(PATHSPEC_GLOB | PATHSPEC_ICASE)) ||
!file_exists(path))) {
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index aab86e8..1535d8f 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -307,4 +307,8 @@ test_expect_success 'git add --dry-run --ignore-missing
of non-existing file out
test_i18ncmp expect.err actual.err
'
+test_expect_success 'git add -A on empty repo does not error out' '
+ git init empty ( cd empty git add -A . )
+'
+
test_done
I am (a little bit) confused.
This is what git does:
rm -rf test mkdir test cd test git init touch A mkdir D cd D
touch B git add . git status
Initialized empty Git repository in /Users/tb/test/test/.git/
On branch master
Initial commit
Changes to be committed:
(use git rm --cached file... to unstage)
new file: B
Untracked files:
(use git add file... to include in what will be committed)
../A
And the behaviour is in line with
https://www.kernel.org/pub/software/scm/git/docs/git-add.html
. stands for the current directory somewhere in the worktree,
not only the project root.
-
Could it make sense to mention that replace
[PATCH] add: don't complain when adding empty project root
with
[PATCH] add: don't complain when adding empty directory.
(and similar in the commit message)
/Torsten
--
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: RLIMIT_NOFILE fallback
On 2013-12-20 10.12, Jeff King wrote: On Thu, Dec 19, 2013 at 09:39:55AM -0800, Junio C Hamano wrote: Torsten Bögershausen tbo...@web.de writes: Thanks for an interesting reading, please allow a side question: Could it be, that -1 == unlimited is Linux specific? And therefore not 100% portable ? And doesn't unlimited number of files call for trouble, having the risk to starve the machine ? BTW: cygwin returns 256. If you look at the caller, you will see that we do cap the value returned from this helper function down to a more reasonable and not so selfish maximum, exactly for the purpose of avoiding the risk of starving other processes. I am not sure you are reading the capping in the right direction. We do not cap at 25, but rather keep 25 open for other stuff. So at unlimited, we are consuming a mere UINT_MAX-25 descriptors. :) I think that 25 is not for the benefit of the rest of the system, but rather for _us_ to avoid running out of descriptors for normal operations. I do not think we need to be careful about starving other processes at all. That is the job of the ulimit in the first place, and we respect it. If the sysadmin turns off the limit, then we are just following their instructions. In practice, I'd be shocked if git behaved reasonably above about 500 packs anyway, so that puts a practical cap on our fd use. :) None of that impacts the patch under discussion, though. The only thing I was trying to bring up earlier is that on a system with: 1. No (or broken) getrlimit 2. No OPEN_MAX defined 3. sysconf that works, and returns -1 for unlimited 4. a sysadmin who has set the descriptor limit to unlimited We will end up at 1. Which is not great, but I am skeptical that a system matching the above 4 constraints actually exists. So I think the patch is fine in practice. -Peff My wrong: I was carefully reading the wrong version of the patch :-( Sorry for the noise. /torsten -- 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: RLIMIT_NOFILE fallback
On 2013-12-19 01.15, Jeff King wrote:
On Wed, Dec 18, 2013 at 02:59:12PM -0800, Junio C Hamano wrote:
Jeff King p...@peff.net writes:
Yes, that is locally OK, but depending on how the caller behaves, we
might need to have an extra saved_errno dance here, which I didn't
want to get into...
I think we are fine. The only caller is about to clobber errno by
closing packs anyway.
Also, I do not think we would be any worse off than the current code.
getrlimit almost certainly just clobbered errno anyway. Either it is
worth saving for the whole function, or not at all (and I think not at
all).
diff --git a/sha1_file.c b/sha1_file.c
index 760dd60..288badd 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -807,15 +807,38 @@ void free_pack_by_name(const char *pack_name)
static unsigned int get_max_fd_limit(void)
{
#ifdef RLIMIT_NOFILE
-struct rlimit lim;
+{
+struct rlimit lim;
-if (getrlimit(RLIMIT_NOFILE, lim))
-die_errno(cannot get RLIMIT_NOFILE);
+if (!getrlimit(RLIMIT_NOFILE, lim))
+return lim.rlim_cur;
+}
+#endif
Yeah, I think pulling the variable into its own block makes this more
readable.
+#ifdef _SC_OPEN_MAX
+{
+long open_max = sysconf(_SC_OPEN_MAX);
+if (0 open_max)
+return open_max;
+/*
+ * Otherwise, we got -1 for one of the two
+ * reasons:
+ *
+ * (1) sysconf() did not understand _SC_OPEN_MAX
+ * and signaled an error with -1; or
+ * (2) sysconf() said there is no limit.
+ *
+ * We _could_ clear errno before calling sysconf() to
+ * tell these two cases apart and return a huge number
+ * in the latter case to let the caller cap it to a
+ * value that is not so selfish, but letting the
+ * fallback OPEN_MAX codepath take care of these cases
+ * is a lot simpler.
+ */
+}
+#endif
This is probably OK. I assume sane systems actually provide OPEN_MAX,
and/or have a working getrlimit in the first place.
The fallback of 1 is actually quite low and can have an impact. Both
for performance, but also for concurrent use. We used to run into a
problem at GitHub where pack-objects serving a clone would have its
packfile removed from under it (by a concurrent repack), and then would
die. The normal code paths are able to just retry the object lookup and
find the new pack, but the pack-objects code is a bit more intimate with
the particular packfile and cannot (currently) do so. With a large
enough mmap window and descriptor limit, we just keep the packfiles
open. But if we have to close them for resource limits (like a too-low
descriptor limit), then we can end up in the die() situation above.
-Peff
Thanks for an interesting reading,
please allow a side question:
Could it be, that -1 == unlimited is Linux specific?
And therefore not 100% portable ?
And doesn't unlimited number of files call for trouble,
having the risk to starve the machine ?
BTW: cygwin returns 256.
http://pubs.opengroup.org/onlinepubs/007908799/xsh/sysconf.html
RETURN VALUE
If name is an invalid value, sysconf() returns -1 and sets errno to
indicate the error. If the variable corresponding to name is associated with
functionality that is not supported by the system, sysconf() returns -1 without
changing the value of errno.
-- Mac OS, based on BSD (?): --
RETURN VALUES
If the call to sysconf() is not successful, -1 is returned and errno is
set appropriately. Otherwise, if the variable is associated with func-
tionality that is not supported, -1 is returned and errno is not modi-
fied. Otherwise, the current variable value is returned.
ERRORS
The sysconf() function may fail and set errno for any of the errors spec-
ified for the library function sysctl(3). In addition, the following
error may be reported:
[EINVAL] The value of the name argument is invalid.
[snip]
The sysconf() function first appeared in 4.4BSD.
---
Linux, Debian:
OPEN_MAX - _SC_OPEN_MAX
The maximum number of files that a process can have open at any
time. Must not be less than _POSIX_OPEN_MAX (20).
[snip]
RETURN VALUE
If name is invalid, -1 is returned, and errno is set to EINVAL. Other‐
wise, the value returned is the value of the system resource and errno
is not changed. In the case of options, a positive value is returned
if a queried option is available, and -1 if it is not. In the case of
limits, -1 means that there is no definite limit.
--
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
Re: [PATCH v2 01/21] path.c: avoid PATH_MAX as buffer size from get_pathname()
On 2013-12-14 11.54, Nguyễn Thái Ngọc Duy wrote:
We've been avoiding PATH_MAX whenever possible. This patch avoids
PATH_MAX in get_pathname() and gives it enough room not to worry about
really long paths.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
path.c | 28
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/path.c b/path.c
index 24594c4..4c1c144 100644
--- a/path.c
+++ b/path.c
@@ -16,10 +16,11 @@ static int get_st_mode_bits(const char *path, int *mode)
static char bad_path[] = /bad-path/;
-static char *get_pathname(void)
+static char *get_pathname(size_t *len)
{
- static char pathname_array[4][PATH_MAX];
+ static char pathname_array[4][4096];
The PATH_MAX doesn't seem to be that bad:
http://pubs.opengroup.org/onlinepubs/009695399/basedefs/limits.h.html
Or do we have a an OS where PATH_MAX does not work ?
Windows uses MAX_PATH=260 PATH_MAX=259
http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247%28v=vs.85%29.aspx
Which means that the current implementation of git can not use path names
longer than
259 (260 including the trailing \0)
(Please correct me if this is wrong)
Which means that the code working with the buffers must make sure to stay
within range,
and not to write beyond the buffers.
If we really want to go away from PATH_MAX, is a hard-coded value of 4096 so
attractive ?
Because we can either
a) Re-define PATH_MAX in git-compat-util.h
b) Use an own #define in git-compat-util.h, like e.g. GIT_PATH_MAX
c) Change the code to use a strbuf which can grow on demand.
I would prefer c) over b) over a)
static int index;
+ *len = sizeof(pathname_array[0]);
return pathname_array[3 ++index];
}
@@ -108,24 +109,26 @@ char *mkpath(const char *fmt, ...)
{
va_list args;
unsigned len;
- char *pathname = get_pathname();
+ size_t n;
+ char *pathname = get_pathname(n);
va_start(args, fmt);
- len = vsnprintf(pathname, PATH_MAX, fmt, args);
+ len = vsnprintf(pathname, n, fmt, args);
Renaming n into something like max or max_len could
make this line 5% easier to read.
(And similar at some places below)
/Torsten
--
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: What's cooking in git.git (Dec 2013, #03; Thu, 12)
On 12/13/2013 01:57 AM, Junio C Hamano wrote: [Cooking] * fc/transport-helper-fixes (2013-12-09) 6 commits - remote-bzr: support the new 'force' option - test-hg.sh: tests are now expected to pass - transport-helper: check for 'forced update' message - transport-helper: add 'force' to 'export' helpers - transport-helper: don't update refs in dry-run - transport-helper: mismerge fix Updates transport-helper, fast-import and fast-export to allow the ref mapping and ref deletion in a way similar to the natively supported transports. Will merge to 'next'. This breaks t5541, (at least on my systems, both Linux and Mac OS) -- 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] t5541: Improve push test
On 2013-12-09 23.10, Junio C Hamano wrote: Torsten Bögershausen tbo...@web.de writes: The old log-line looked like this: + 9d498b0...8598732 master - master (forced update) And the new one like this: 9d498b0..8598732 master - master - Loosen the grep pattern by not demanding (forced update) Hmm, what is the reason for the change the output? The output this piece is testing is the result of this: git push origin master:retsam echo change changed path2 git commit -a -m path2 --amend # push master too; this ensures there is at least one ''push'' command to # the remote helper and triggers interaction with the helper. test_must_fail git push -v origin +master master:retsam output 21' This is run inside test_repo_clone, which has /smart/test_repo.git as its origin, which in turn has 'master' branch (and nothing else). It - pushes master to another branch retsam; - amends its 'master'; - attempts to push the updated master to force-update master, and also retsam without forcing. The latter needs to be forced to succeed, and that is why we expect it to fail. If the output from the push process says + 9d498b0...8598732 master - master (forced update) ! [rejected]master - retsam (non-fast-forward) error: failed to push some refs to '../test_repo_copy/' I think that is a good thing to do, no? After all, that is what we show with Git native transports. Is this patch merely matching a test to a broken behaviour of some sort? Puzzled... Thanks for the analysis: I thing the patch isn't the way to go. The regression in t5541 was introduced in f9e3c6bebb89de12. Which was a cleanup to previous commits: transport-helper: add 'force' to 'export' helpers So reverting f9e3c6bebb89de fixes t5541, but breaks contrib/remote-helpers. Felipe, could you have a look, please ? /Torsten force -- 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] t5541: Improve push test
The old log-line looked like this: + 9d498b0...8598732 master - master (forced update) And the new one like this: 9d498b0..8598732 master - master - Loosen the grep pattern by not demanding (forced update) - Improve the grep pattern and check the new SHA id Signed-off-by: Torsten Bögershausen tbo...@web.de --- The following revealed a weakness in t5541: commit f9e3c6bebb89de12f2dfdaa1899cb22e9ef32542 Author: Felipe Contreras felipe.contre...@gmail.com Date: Tue Nov 12 14:56:57 2013 -0600 transport-helper: check for 'forced update' message So don't look for forced update, but check for the SHA. (I want to fix a missing as well, that is for the next commit) diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh index 470ac54..1468a07 100755 --- a/t/t5541-http-push.sh +++ b/t/t5541-http-push.sh @@ -168,7 +168,8 @@ test_expect_success 'push fails for non-fast-forward refs unmatched by remote he test_must_fail git push -v origin +master master:retsam output 21' test_expect_success 'push fails for non-fast-forward refs unmatched by remote helper: remote output' ' - grep ^ + [a-f0-9]*\.\.\.[a-f0-9]* *master - master (forced update)$ output + newsha=$(git log --oneline -n1 | sed -e s/^\([0-9a-f]*\).*/\1/) + grep \.\.$newsha *master - master output grep ^ ! \[rejected\] *master - retsam (non-fast-forward)$ output ' -- 1.8.5 -- 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] diff: don't read index when --no-index is given
On 2013-12-09 21.40, Thomas Gummerer wrote: +test_expect_success 'git diff --no-index with broken index' ' + cd repo + echo broken .git/index + git diff --no-index a ../non/git/a ^^ I'm confused: Does this work with the trailing ? (and when we use cd repo it could be good to use a sub-shell (even when this is the last test case) -- 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: Build issue for git 1.8.5.1 under Mac OS X 10.8 (Mountain Lion)
On 12/04/2013 07:48 PM, Marius Schamschula wrote: Hi all, Over the years I have built many versions of git and released them on hmug.org. git 1.8.5.1 builds just fine under OS 10.7 (Lion) and 10.9 (Mavericks), but the build fails (also for 1.8.5) on 10.8 (Mountain Lion): snip GIT_VERSION = 1.8.5.1 * new build flags CC credential-store.o In file included from git-compat-util.h:330:0, from cache.h:4, from credential-store.c:1: compat/apple-common-crypto.h: In function 'git_CC_EVP_EncodeBlock': compat/apple-common-crypto.h:32:2: error: 'SecTransformRef' undeclared (first use in this function) compat/apple-common-crypto.h:32:2: note: each undeclared identifier is reported only once for each function it appears in compat/apple-common-crypto.h:32:18: error: expected ';' before 'encoder' compat/apple-common-crypto.h:36:2: error: 'encoder' undeclared (first use in this function) compat/apple-common-crypto.h:36:2: warning: implicit declaration of function 'SecEncodeTransformCreate' compat/apple-common-crypto.h:36:37: error: 'kSecBase64Encoding' undeclared (first use in this function) compat/apple-common-crypto.h:40:2: warning: implicit declaration of function 'SecTransformSetAttribute' compat/apple-common-crypto.h:40:36: error: 'kSecTransformInputAttributeName' undeclared (first use in this function) compat/apple-common-crypto.h:44:2: warning: implicit declaration of function 'SecTransformExecute' compat/apple-common-crypto.h: In function 'git_CC_EVP_DecodeBlock': compat/apple-common-crypto.h:62:2: error: 'SecTransformRef' undeclared (first use in this function) compat/apple-common-crypto.h:62:18: error: expected ';' before 'decoder' compat/apple-common-crypto.h:66:2: error: 'decoder' undeclared (first use in this function) compat/apple-common-crypto.h:66:2: warning: implicit declaration of function 'SecDecodeTransformCreate' compat/apple-common-crypto.h:66:37: error: 'kSecBase64Encoding' undeclared (first use in this function) compat/apple-common-crypto.h:70:36: error: 'kSecTransformInputAttributeName' undeclared (first use in this function) Makefile:1975: recipe for target 'credential-store.o' failed make: *** [credential-store.o] Error 1 /snip Apparently a header issue: I tried force feeding the Security/SecEncryptTransform.h file, and just got an other error… Any help would be welcome! Thanks in advance. Marius -- Marius Schamschula I can't reproduce it here (in other words, it compiles) What could help is to run the preprocessor, and see what the compiler see. a) Patch the Makefile of git like this: -- a/Makefile +++ b/Makefile @@ -349,7 +349,8 @@ GIT-VERSION-FILE: FORCE # CFLAGS and LDFLAGS are for the users to override from the command line. -CFLAGS = -g -O2 -Wall +#CFLAGS = -g -O2 -Wall +CFLAGS= -E b) Compile credential-store.o (Which is now an ASCII file) credential-store.o ; make credential-store.o c) Take an editor and have a look. On my system SecTransform.h is here: - /System/Library/Frameworks/Security.framework/Headers/SecTransform.h d) And the file contains something like this: --- typedef CFTypeRef SecTransformRef; typedef CFTypeRef SecGroupTransformRef; /Torsten -- 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] gettext.c: only work around the vsnprintf bug on glibc 2.17
On 2013-11-30 13.01, Nguyễn Thái Ngọc Duy wrote:
Bug 6530 [1] causes git show v0.99.6~1 to fail with error your
causes or caused (as we have a work around?)
vsnprintf is broken. The workaround avoids that, but it corrupts
system error messages in non-C locales.
[snip]
The bug in glibc has been fixed since 2.17. If git is built with glibc, it can
^^ (Should we name glibc ?)
[snip]
- setlocale(LC_MESSAGES, );
- init_gettext_charset(git);
+ setlocale(vsnprintf_broken ? LC_MESSAGES : LC_ALL, );
1) One thing I don't understand: Why do we need to set LC_ALL ?
The old patch didn't do it, or what do I miss ?
See https://wiki.debian.org/Locale :
Using LC_ALL is strongly discouraged as it overrides everything. Please use it
only when testing and never set it in a startup file.
2) I stole the code partly from here:
http://sourceware.org/bugzilla/show_bug.cgi?id=6530
--
#include stdio.h
#include locale.h
#include gnu/libc-version.h
#define STR ²éľÂíɱ²¡¶¾£¬ÖܺèµtÄúµÄ360²»×¨Òµ£¡
int main(void) {
char buf[200];
setlocale(LC_ALL, );
printf(gnu_glibc_version()=%s\n,
gnu_get_libc_version());
printf(ret(snprintf)=%d\n, snprintf(buf, 150, %.50s, STR));
return 0;
}
--
Then I run it on different machines:
gnu_glibc_version()=2.11.3 /* Ubuntu 10.4, no updates */
gnu_glibc_version()=2.11.3 /* Debian Squeze ?*/
gnu_glibc_version()=2.13 /* Debian Wheezy */
ret(snprintf)=50 /* All the 3 above */
-
So could it be that libc is patched in Debian/Ubuntu, and we
can do a runtime check (rather than looking at the version number),
similar to the code above ?
3) The patch didn't break anything here (Debian, Mac OS).
4) Could it be good to have a test case ? Is t0204 good for inspiration ?
5) I can do more testing if needed.
/Torsten
--
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] gettext.c: only work around the vsnprintf bug on glibc 2.17
gnu_glibc_version()=2.11.3 /* Ubuntu 10.4, no updates */ Sorry, Copy-paste error: 2.11.1 -- 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 v7 03/10] git_connect: remove artificial limit of a remote command
Since day one, function git_connect() had a limit on the command line of
the command that is invoked to make a connection. 7a33bcbe converted the
code that constructs the command to strbuf. This would have been the
right time to remove the limit, but it did not happen. Remove it now.
git_connect() uses start_command() to invoke the command; consequently,
the limits of the system still apply, but are diagnosed only at execve()
time. But these limits are more lenient than the 1K that git_connect()
imposed.
Signed-off-by: Johannes Sixt j...@kdbg.org
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
connect.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/connect.c b/connect.c
index 06e88b0..6cc1f8d 100644
--- a/connect.c
+++ b/connect.c
@@ -527,8 +527,6 @@ static struct child_process *git_proxy_connect(int fd[2],
char *host)
return proxy;
}
-#define MAX_CMD_LEN 1024
-
static char *get_port(char *host)
{
char *end;
@@ -570,7 +568,7 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
int free_path = 0;
char *port = NULL;
const char **arg;
- struct strbuf cmd;
+ struct strbuf cmd = STRBUF_INIT;
/* Without this we cannot rely on waitpid() to tell
* what happened to our children.
@@ -676,12 +674,9 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
conn = xcalloc(1, sizeof(*conn));
- strbuf_init(cmd, MAX_CMD_LEN);
strbuf_addstr(cmd, prog);
strbuf_addch(cmd, ' ');
sq_quote_buf(cmd, path);
- if (cmd.len = MAX_CMD_LEN)
- die(command line too long);
conn-in = conn-out = -1;
conn-argv = arg = xcalloc(7, sizeof(*arg));
--
1.8.5.rc0.23.gaa27064
--
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 v7 02/10] t5601: Add tests for ssh
Add more tests testing all the combinations:
-IPv4 or IPv6
-path starting with / or with /~
-with and without the ssh:// scheme
Some test fail, they need updates in connect.c
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
t/t5601-clone.sh | 100 ++-
1 file changed, 99 insertions(+), 1 deletion(-)
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index c634f77..ba99972 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -313,7 +313,7 @@ expect_ssh () {
setup_ssh_wrapper
-test_expect_success 'cloning myhost:src uses ssh' '
+test_expect_success 'clone myhost:src uses ssh' '
git clone myhost:src ssh-clone
expect_ssh myhost src
'
@@ -329,6 +329,104 @@ test_expect_success 'bracketed hostnames are still ssh' '
expect_ssh myhost:123 src
'
+counter=0
+# $1 url
+# $2 none|host
+# $3 path
+test_clone_url () {
+ counter=$(($counter + 1))
+ test_might_fail git clone $1 tmp$counter
+ expect_ssh $2 $3
+}
+
+test_expect_success NOT_MINGW 'clone c:temp is ssl' '
+ test_clone_url c:temp c temp
+'
+
+test_expect_success MINGW 'clone c:temp is dos drive' '
+ test_clone_url c:temp none
+'
+
+#ip v4
+for repo in rep rep/home/project /~proj 123
+do
+ test_expect_success clone host:$repo '
+ test_clone_url host:$repo host $repo
+ '
+done
+
+#ipv6
+# failing
+for repo in /~proj
+do
+ test_expect_failure clone [::1]:$repo '
+ test_clone_url [::1]:$repo ::1 $repo
+ '
+done
+
+for repo in rep rep/home/project 123
+do
+ test_expect_success clone [::1]:$repo '
+ test_clone_url [::1]:$repo ::1 $repo
+ '
+done
+
+# Corner cases
+# failing
+for repo in [foo]bar/baz:qux [foo/bar]:baz
+do
+ test_expect_failure clone $url is not ssh '
+ test_clone_url $url none
+ '
+done
+
+for url in foo/bar:baz
+do
+ test_expect_success clone $url is not ssh '
+ test_clone_url $url none
+ '
+done
+
+#with ssh:// scheme
+test_expect_success 'clone ssh://host.xz/home/user/repo' '
+ test_clone_url ssh://host.xz/home/user/repo host.xz /home/user/repo
+'
+
+# from home directory
+test_expect_success 'clone ssh://host.xz/~repo' '
+ test_clone_url ssh://host.xz/~repo host.xz ~repo
+'
+
+# with port number
+test_expect_success 'clone ssh://host.xz:22/home/user/repo' '
+ test_clone_url ssh://host.xz:22/home/user/repo -p 22 host.xz
/home/user/repo
+'
+
+# from home directory with port number
+test_expect_success 'clone ssh://host.xz:22/~repo' '
+ test_clone_url ssh://host.xz:22/~repo -p 22 host.xz ~repo
+'
+
+#IPv6
+test_expect_success 'clone ssh://[::1]/home/user/repo' '
+ test_clone_url ssh://[::1]/home/user/repo ::1 /home/user/repo
+'
+
+#IPv6 from home directory
+test_expect_success 'clone ssh://[::1]/~repo' '
+ test_clone_url ssh://[::1]/~repo ::1 ~repo
+'
+
+#IPv6 with port number
+test_expect_success 'clone ssh://[::1]:22/home/user/repo' '
+ test_clone_url ssh://[::1]:22/home/user/repo -p 22 ::1
/home/user/repo
+'
+
+#IPv6 from home directory with port number
+test_expect_success 'clone ssh://[::1]:22/~repo' '
+ test_clone_url ssh://[::1]:22/~repo -p 22 ::1 ~repo
+'
+
test_expect_success 'clone from a repository with two identical branches' '
(
--
1.8.5.rc0.23.gaa27064
--
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 v7 04/10] git_connect: factor out discovery of the protocol and its parts
git_connect has grown large due to the many different protocols syntaxes
that are supported. Move the part of the function that parses the URL to
connect to into a separate function for readability.
Signed-off-by: Johannes Sixt j...@kdbg.org
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
connect.c | 80 ++-
1 file changed, 53 insertions(+), 27 deletions(-)
diff --git a/connect.c b/connect.c
index 6cc1f8d..a6cf345 100644
--- a/connect.c
+++ b/connect.c
@@ -543,37 +543,20 @@ static char *get_port(char *host)
return NULL;
}
-static struct child_process no_fork;
-
/*
- * This returns a dummy child_process if the transport protocol does not
- * need fork(2), or a struct child_process object if it does. Once done,
- * finish the connection with finish_connect() with the value returned from
- * this function (it is safe to call finish_connect() with NULL to support
- * the former case).
- *
- * If it returns, the connect is successful; it just dies on errors (this
- * will hopefully be changed in a libification effort, to return NULL when
- * the connection failed).
+ * Extract protocol and relevant parts from the specified connection URL.
+ * The caller must free() the returned strings.
*/
-struct child_process *git_connect(int fd[2], const char *url_orig,
- const char *prog, int flags)
+static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
+ char **ret_port, char **ret_path)
{
char *url;
char *host, *path;
char *end;
int c;
- struct child_process *conn = no_fork;
enum protocol protocol = PROTO_LOCAL;
int free_path = 0;
char *port = NULL;
- const char **arg;
- struct strbuf cmd = STRBUF_INIT;
-
- /* Without this we cannot rely on waitpid() to tell
-* what happened to our children.
-*/
- signal(SIGCHLD, SIG_DFL);
if (is_url(url_orig))
url = url_decode(url_orig);
@@ -645,6 +628,49 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
if (protocol == PROTO_SSH host != url)
port = get_port(end);
+ *ret_host = xstrdup(host);
+ if (port)
+ *ret_port = xstrdup(port);
+ else
+ *ret_port = NULL;
+ if (free_path)
+ *ret_path = path;
+ else
+ *ret_path = xstrdup(path);
+ free(url);
+ return protocol;
+}
+
+static struct child_process no_fork;
+
+/*
+ * This returns a dummy child_process if the transport protocol does not
+ * need fork(2), or a struct child_process object if it does. Once done,
+ * finish the connection with finish_connect() with the value returned from
+ * this function (it is safe to call finish_connect() with NULL to support
+ * the former case).
+ *
+ * If it returns, the connect is successful; it just dies on errors (this
+ * will hopefully be changed in a libification effort, to return NULL when
+ * the connection failed).
+ */
+struct child_process *git_connect(int fd[2], const char *url,
+ const char *prog, int flags)
+{
+ char *host, *path;
+ struct child_process *conn = no_fork;
+ enum protocol protocol;
+ char *port;
+ const char **arg;
+ struct strbuf cmd = STRBUF_INIT;
+
+ /* Without this we cannot rely on waitpid() to tell
+* what happened to our children.
+*/
+ signal(SIGCHLD, SIG_DFL);
+
+ protocol = parse_connect_url(url, host, port, path);
+
if (protocol == PROTO_GIT) {
/* These underlying connection commands die() if they
* cannot connect.
@@ -666,9 +692,9 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
prog, path, 0,
target_host, 0);
free(target_host);
- free(url);
- if (free_path)
- free(path);
+ free(host);
+ free(port);
+ free(path);
return conn;
}
@@ -709,9 +735,9 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
fd[0] = conn-out; /* read from child's stdout */
fd[1] = conn-in; /* write to child's stdin */
strbuf_release(cmd);
- free(url);
- if (free_path)
- free(path);
+ free(host);
+ free(port);
+ free(path);
return conn;
}
--
1.8.5.rc0.23.gaa27064
--
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 v7 05/10] git fetch-pack: Add --diag-url
The main purpose is to trace the URL parser called by git_connect() in
connect.c
The main features of the parser can be listed as this:
- parse out host and path for URLs with a scheme (git:// file:// ssh://)
- parse host names embedded by [] correctly
- extract the port number, if present
- separate URLs like file (which are local)
from URLs like host:repo which should use ssh
Add the new parameter --diag-url to git fetch-pack,
which prints the value for protocol, host and path to stderr and exits.
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
builtin/fetch-pack.c | 14 +++---
connect.c| 28
connect.h| 1 +
fetch-pack.h | 1 +
4 files changed, 41 insertions(+), 3 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c8e8582..758b5ac 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -7,7 +7,7 @@
static const char fetch_pack_usage[] =
git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin]
[--include-tag] [--upload-pack=git-upload-pack] [--depth=n]
-[--no-progress] [-v] [host:]directory [refs...];
+[--no-progress] [--diag-url] [-v] [host:]directory [refs...];
static void add_sought_entry_mem(struct ref ***sought, int *nr, int *alloc,
const char *name, int namelen)
@@ -81,6 +81,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char
*prefix)
args.stdin_refs = 1;
continue;
}
+ if (!strcmp(--diag-url, arg)) {
+ args.diag_url = 1;
+ continue;
+ }
if (!strcmp(-v, arg)) {
args.verbose = 1;
continue;
@@ -146,10 +150,14 @@ int cmd_fetch_pack(int argc, const char **argv, const
char *prefix)
fd[0] = 0;
fd[1] = 1;
} else {
+ int flags = args.verbose ? CONNECT_VERBOSE : 0;
+ if (args.diag_url)
+ flags |= CONNECT_DIAG_URL;
conn = git_connect(fd, dest, args.uploadpack,
- args.verbose ? CONNECT_VERBOSE : 0);
+ flags);
+ if (!conn)
+ return args.diag_url ? 0 : 1;
}
-
get_remote_heads(fd[0], NULL, 0, ref, 0, NULL);
ref = fetch_pack(args, fd, conn, ref, dest,
diff --git a/connect.c b/connect.c
index a6cf345..a16bdaf 100644
--- a/connect.c
+++ b/connect.c
@@ -236,6 +236,20 @@ enum protocol {
PROTO_GIT
};
+static const char *prot_name(enum protocol protocol)
+{
+ switch (protocol) {
+ case PROTO_LOCAL:
+ return file;
+ case PROTO_SSH:
+ return ssh;
+ case PROTO_GIT:
+ return git;
+ default:
+ return unkown protocol;
+ }
+}
+
static enum protocol get_protocol(const char *name)
{
if (!strcmp(name, ssh))
@@ -670,6 +684,20 @@ struct child_process *git_connect(int fd[2], const char
*url,
signal(SIGCHLD, SIG_DFL);
protocol = parse_connect_url(url, host, port, path);
+ if (flags CONNECT_DIAG_URL) {
+ printf(Diag: url=%s\n, url ? url : NULL);
+ printf(Diag: protocol=%s\n, prot_name(protocol));
+ printf(Diag: hostandport=%s, host ? host : NULL);
+ if (port)
+ printf(:%s\n, port);
+ else
+ printf(\n);
+ printf(Diag: path=%s\n, path ? path : NULL);
+ free(host);
+ free(port);
+ free(path);
+ return NULL;
+ }
if (protocol == PROTO_GIT) {
/* These underlying connection commands die() if they
diff --git a/connect.h b/connect.h
index 64fb7db..527d58a 100644
--- a/connect.h
+++ b/connect.h
@@ -2,6 +2,7 @@
#define CONNECT_H
#define CONNECT_VERBOSE (1u 0)
+#define CONNECT_DIAG_URL (1u 1)
extern struct child_process *git_connect(int fd[2], const char *url, const
char *prog, int flags);
extern int finish_connect(struct child_process *conn);
extern int git_connection_is_socket(struct child_process *conn);
diff --git a/fetch-pack.h b/fetch-pack.h
index 461cbf3..20ccc12 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -14,6 +14,7 @@ struct fetch_pack_args {
use_thin_pack:1,
fetch_all:1,
stdin_refs:1,
+ diag_url:1,
verbose:1,
no_progress:1,
include_tag:1,
--
1.8.5.rc0.23.gaa27064
--
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 v7 07/10] git fetch: support host:/~repo
The documentation (in urls.txt) says that
ssh://host:/~repo,
host:/~repo or
host:~repo
specifiy the repository repo in the home directory at host.
This has not been working for host:/~repo.
Fix a minor regression:
Before commit 356bec Support [address] in URLs
the comparison url != hostname could be used to determine if
the URL had a scheme or not: ssh://host/host != host.
After 356bec [::1] was converted into ::1, yielding
url != hostname as well.
Solution:
Don't use if (url != hostname), but look at the separator instead.
Rename the variable c into separator.
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
connect.c | 14 +++---
t/t5500-fetch-pack.sh | 24
t/t5601-clone.sh | 12 ++--
3 files changed, 33 insertions(+), 17 deletions(-)
diff --git a/connect.c b/connect.c
index a16bdaf..7e5f608 100644
--- a/connect.c
+++ b/connect.c
@@ -567,7 +567,7 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
char *url;
char *host, *path;
char *end;
- int c;
+ int separator;
enum protocol protocol = PROTO_LOCAL;
int free_path = 0;
char *port = NULL;
@@ -582,10 +582,10 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
*host = '\0';
protocol = get_protocol(url);
host += 3;
- c = '/';
+ separator = '/';
} else {
host = url;
- c = ':';
+ separator = ':';
}
/*
@@ -605,9 +605,9 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
} else
end = host;
- path = strchr(end, c);
+ path = strchr(end, separator);
if (path !has_dos_drive_prefix(end)) {
- if (c == ':') {
+ if (separator == ':') {
if (host != url || path strchrnul(host, '/')) {
protocol = PROTO_SSH;
*path++ = '\0';
@@ -624,7 +624,7 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
* null-terminate hostname and point path to ~ for URL's like this:
*ssh://host.xz/~user/repo
*/
- if (protocol != PROTO_LOCAL host != url) {
+ if (protocol != PROTO_LOCAL) {
char *ptr = path;
if (path[1] == '~')
path++;
@@ -639,7 +639,7 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
/*
* Add support for ssh port: ssh://host.xy:port/...
*/
- if (protocol == PROTO_SSH host != url)
+ if (protocol == PROTO_SSH separator == '/')
port = get_port(end);
*ret_host = xstrdup(host);
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index a2b37af..2d3cdaa 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -589,6 +589,30 @@ do
check_prot_path $p://$h/~$r $p /~$r
'
done
+ # file without scheme
+ for h in nohost nohost:12 [::1] [::1]:23 [ [:aa
+ do
+ test_expect_success fetch-pack --diag-url ./$h:$r '
+ check_prot_path ./$h:$r $p ./$h:$r
+ '
+ # No /~ - ~ conversion for file
+ test_expect_success fetch-pack --diag-url ./$p:$h/~$r '
+ check_prot_path ./$p:$h/~$r $p ./$p:$h/~$r
+ '
+ done
+ #ssh without scheme
+ p=ssh
+ for h in host [::1]
+ do
+ hh=$(echo $h | tr -d [])
+ test_expect_success fetch-pack --diag-url $h:$r '
+ check_prot_path $h:$r $p $r
+ '
+ # Do /~ - ~ conversion
+ test_expect_success fetch-pack --diag-url $h:/~$r '
+ check_prot_host_path $h:/~$r $p $hh ~$r
+ '
+ done
done
test_done
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index ba99972..4db0c0b 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -348,7 +348,7 @@ test_expect_success MINGW 'clone c:temp is dos drive' '
'
#ip v4
-for repo in rep rep/home/project /~proj 123
+for repo in rep rep/home/project 123
do
test_expect_success clone host:$repo '
test_clone_url host:$repo host $repo
@@ -356,14 +356,6 @@ do
done
#ipv6
-# failing
-for repo in /~proj
-do
- test_expect_failure clone [::1]:$repo '
- test_clone_url [::1]:$repo ::1 $repo
- '
-done
-
for repo in rep rep/home/project 123
do
test_expect_success clone [::1]:$repo '
@@ -373,7 +365,7 @@ done
# Corner cases
# failing
-for repo in [foo]bar/baz:qux [foo/bar]:baz
+for url in [foo]bar/baz:qux [foo/bar]:baz
do
test_expect_failure clone $url is not ssh '
test_clone_url $url
[PATCH v7 06/10] t5500: Test case for diag-url
Add test cases using git fetch-pack --diag-url:
- parse out host and path for URLs with a scheme (git:// file:// ssh://)
- parse host names embedded by [] correctly
- extract the port number, if present
- separate URLs like file (which are local)
from URLs like host:repo which should use ssh
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
t/t5500-fetch-pack.sh | 59 +++
1 file changed, 59 insertions(+)
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index d87ddf7..a2b37af 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -531,5 +531,64 @@ test_expect_success 'shallow fetch with tags does not
break the repository' '
git fsck
)
'
+check_prot_path() {
+ cat expected -EOF
+ Diag: url=$1
+ Diag: protocol=$2
+ Diag: path=$3
+ EOF
+ git fetch-pack --diag-url $1 | grep -v hostandport= actual
+ test_cmp expected actual
+}
+
+check_prot_host_path() {
+ cat expected -EOF
+ Diag: url=$1
+ Diag: protocol=$2
+ Diag: hostandport=$3
+ Diag: path=$4
+ EOF
+ git fetch-pack --diag-url $1 actual
+ test_cmp expected actual
+}
+
+for r in repo re:po re/po
+do
+ # git or ssh with scheme
+ for p in ssh+git git+ssh git ssh
+ do
+ for h in host host:12 [::1] [::1]:23
+ do
+ case $p in
+ *ssh*)
+ hh=$(echo $h | tr -d [])
+ pp=ssh
+ ;;
+ *)
+ hh=$h
+ pp=$p
+ ;;
+ esac
+ test_expect_success fetch-pack --diag-url $p://$h/$r '
+ check_prot_host_path $p://$h/$r $pp $hh /$r
+ '
+ # /~ - ~ conversion
+ test_expect_success fetch-pack --diag-url $p://$h/~$r
'
+ check_prot_host_path $p://$h/~$r $pp $hh ~$r
+ '
+ done
+ done
+ # file with scheme
+ for p in file
+ do
+ test_expect_success fetch-pack --diag-url $p://$h/$r '
+ check_prot_path $p://$h/$r $p /$r
+ '
+ # No /~ - ~ conversion for file
+ test_expect_success fetch-pack --diag-url $p://$h/~$r '
+ check_prot_path $p://$h/~$r $p /~$r
+ '
+ done
+done
test_done
--
1.8.5.rc0.23.gaa27064
--
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 v7 09/10] connect.c: Refactor url parsing
Make the function is_local() from transport.c public, rename it into
url_is_local_not_ssh() and use it in both transport.c and connect.c
Use a protocol local for URLs for the local file system.
One note about using file:// under Windows:
The (absolute) path on Unix like system typically starts with /.
When the host is empty, it can be omitted, so that a shell scriptlet
url=file://$pwd
will give a URL like file:///home/user/repo.
Windows does not have the same concept of a root directory located in /.
When parsing the URL allow file://C:/user/repo
(even if RFC1738 indicates that file:///C:/user/repo should be used).
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
connect.c | 57 +++
connect.h | 1 +
t/t5500-fetch-pack.sh | 7 +++
t/t5601-clone.sh | 8
transport.c | 12 ++-
5 files changed, 48 insertions(+), 37 deletions(-)
diff --git a/connect.c b/connect.c
index 7874530..04093c4 100644
--- a/connect.c
+++ b/connect.c
@@ -232,14 +232,24 @@ int server_supports(const char *feature)
enum protocol {
PROTO_LOCAL = 1,
+ PROTO_FILE,
PROTO_SSH,
PROTO_GIT
};
+int url_is_local_not_ssh(const char *url)
+{
+ const char *colon = strchr(url, ':');
+ const char *slash = strchr(url, '/');
+ return !colon || (slash slash colon) ||
+ has_dos_drive_prefix(url);
+}
+
static const char *prot_name(enum protocol protocol)
{
switch (protocol) {
case PROTO_LOCAL:
+ case PROTO_FILE:
return file;
case PROTO_SSH:
return ssh;
@@ -261,7 +271,7 @@ static enum protocol get_protocol(const char *name)
if (!strcmp(name, ssh+git))
return PROTO_SSH;
if (!strcmp(name, file))
- return PROTO_LOCAL;
+ return PROTO_FILE;
die(I don't handle protocol '%s', name);
}
@@ -564,9 +574,8 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
char *url;
char *host, *path;
char *end;
- int separator;
+ int separator = '/';
enum protocol protocol = PROTO_LOCAL;
- int free_path = 0;
if (is_url(url_orig))
url = url_decode(url_orig);
@@ -578,10 +587,12 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
*host = '\0';
protocol = get_protocol(url);
host += 3;
- separator = '/';
} else {
host = url;
- separator = ':';
+ if (!url_is_local_not_ssh(url)) {
+ protocol = PROTO_SSH;
+ separator = ':';
+ }
}
/*
@@ -597,17 +608,12 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
} else
end = host;
- path = strchr(end, separator);
- if (path !has_dos_drive_prefix(end)) {
- if (separator == ':') {
- if (host != url || path strchrnul(host, '/')) {
- protocol = PROTO_SSH;
- *path++ = '\0';
- } else /* '/' in the host part, assume local path */
- path = end;
- }
- } else
+ if (protocol == PROTO_LOCAL)
path = end;
+ else if (protocol == PROTO_FILE has_dos_drive_prefix(end))
+ path = end; /* file://$(pwd) may be file://C:/projects/repo
*/
+ else
+ path = strchr(end, separator);
if (!path || !*path)
die(No path specified. See 'man git-pull' for valid url
syntax);
@@ -616,23 +622,20 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
* null-terminate hostname and point path to ~ for URL's like this:
*ssh://host.xz/~user/repo
*/
- if (protocol != PROTO_LOCAL) {
- char *ptr = path;
+
+ end = path; /* Need to \0 terminate host here */
+ if (separator == ':')
+ path++; /* path starts after ':' */
+ if (protocol == PROTO_GIT || protocol == PROTO_SSH) {
if (path[1] == '~')
path++;
- else {
- path = xstrdup(ptr);
- free_path = 1;
- }
-
- *ptr = '\0';
}
+ path = xstrdup(path);
+ *end = '\0';
+
*ret_host = xstrdup(host);
- if (free_path)
- *ret_path = path;
- else
- *ret_path = xstrdup(path);
+ *ret_path = path;
free(url);
return protocol;
}
diff --git a/connect.h b/connect.h
index 527d58a..c41a685 100644
--- a/connect.h
+++ b/connect.h
@@ -9,5 +9,6 @@ extern
[PATCH v7 01/10] t5601: remove clear_ssh, refactor setup_ssh_wrapper
Commit 8d3d28f5 added test cases for URLs which should be ssh.
Remove the function clear_ssh, use test_when_finished to clean up.
Introduce the function setup_ssh_wrapper, which could be factored
out together with expect_ssh.
Tighten one test and use foo:bar instead of ./foo:bar,
Helped-by: Jeff King p...@peff.net
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
Comments to V6:
Code from Johannes Sixt is part of the series, original found here:
http://permalink.gmane.org/gmane.comp.version-control.git/237339
http://permalink.gmane.org/gmane.comp.version-control.git/237338
Changes since V6:
- git fetch-pack --diag-url uses stdout instead of stderr
- cleanup in the test scripts
- Removed [PATCH v6 07/10] connect.c: Corner case for IPv6
- Added missing sign-off
- Try to explain better why windows supports file://C:/repo
(Actually we should support file:///C:/repo, but we don't
- Other remarks from code review, did I miss any ?
I'm not sure about 10/10, 2 cleanups which I didn't manage
to find a better place to be.
However, I want to concentrate on 1..9, so that 10/10 can be dropped.
And a question:
Can we replace tb/clone-ssh-with-colon-for-port with this stuff ?
If we are OK with part 1..4, I don't need to send them again.
t/t5601-clone.sh | 40
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 1d1c875..c634f77 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -280,25 +280,26 @@ test_expect_success 'clone checking out a tag' '
test_cmp fetch.expected fetch.actual
'
-test_expect_success 'setup ssh wrapper' '
- write_script $TRASH_DIRECTORY/ssh-wrapper -\EOF
- echo $TRASH_DIRECTORY/ssh-output ssh: $*
- # throw away all but the last argument, which should be the
- # command
- while test $# -gt 1; do shift; done
- eval $1
- EOF
-
- GIT_SSH=$TRASH_DIRECTORY/ssh-wrapper
- export GIT_SSH
- export TRASH_DIRECTORY
-'
-
-clear_ssh () {
- $TRASH_DIRECTORY/ssh-output
+setup_ssh_wrapper () {
+ test_expect_success 'setup ssh wrapper' '
+ write_script $TRASH_DIRECTORY/ssh-wrapper -\EOF
+ echo $TRASH_DIRECTORY/ssh-output ssh: $*
+ # throw away all but the last argument, which should be the
+ # command
+ while test $# -gt 1; do shift; done
+ eval $1
+ EOF
+ GIT_SSH=$TRASH_DIRECTORY/ssh-wrapper
+ export GIT_SSH
+ export TRASH_DIRECTORY
+ $TRASH_DIRECTORY/ssh-output
+ '
}
expect_ssh () {
+ test_when_finished '
+ (cd $TRASH_DIRECTORY rm -f ssh-expect ssh-output)
+ '
{
case $1 in
none)
@@ -310,21 +311,20 @@ expect_ssh () {
(cd $TRASH_DIRECTORY test_cmp ssh-expect ssh-output)
}
+setup_ssh_wrapper
+
test_expect_success 'cloning myhost:src uses ssh' '
- clear_ssh
git clone myhost:src ssh-clone
expect_ssh myhost src
'
test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '
- clear_ssh
cp -R src foo:bar
- git clone ./foo:bar foobar
+ git clone foo:bar foobar
expect_ssh none
'
test_expect_success 'bracketed hostnames are still ssh' '
- clear_ssh
git clone [myhost:123]:src ssh-bracket-clone
expect_ssh myhost:123 src
'
--
1.8.5.rc0.23.gaa27064
--
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 07/10] connect.c: Corner case for IPv6
Commit faea9ccbadf75128 introduced user-relative paths:
ssh://host.xz/~junio/repo is the same as host.xz:~junio/repo.
Commit 356bece0a27258181 Support [address] in URLs introduced a regression:
When an URL like [::1]:/~repo is used, the replacement of /~
with ~ was enabled for IPv6 host names, and [::1]:/~repo is
converted into [::1]:~repo.
Solution:
Don't use if (url != hostname), but look at the separator instead.
Rename the variable c into separator.
---
connect.c | 14 +++---
t/t5500-fetch-pack.sh | 16
t/t5601-clone.sh | 12 ++--
3 files changed, 13 insertions(+), 29 deletions(-)
diff --git a/connect.c b/connect.c
index 1b93b4d..0cb88b8 100644
--- a/connect.c
+++ b/connect.c
@@ -566,7 +566,7 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
char *url;
char *host, *path;
char *end;
- int c;
+ int separator;
enum protocol protocol = PROTO_LOCAL;
int free_path = 0;
char *port = NULL;
@@ -581,10 +581,10 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
*host = '\0';
protocol = get_protocol(url);
host += 3;
- c = '/';
+ separator = '/';
} else {
host = url;
- c = ':';
+ separator = ':';
}
/*
@@ -604,9 +604,9 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
} else
end = host;
- path = strchr(end, c);
+ path = strchr(end, separator);
if (path !has_dos_drive_prefix(end)) {
- if (c == ':') {
+ if (separator == ':') {
if (host != url || path strchrnul(host, '/')) {
protocol = PROTO_SSH;
*path++ = '\0';
@@ -623,7 +623,7 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
* null-terminate hostname and point path to ~ for URL's like this:
*ssh://host.xz/~user/repo
*/
- if (protocol != PROTO_LOCAL host != url) {
+ if (protocol != PROTO_LOCAL separator == '/') {
char *ptr = path;
if (path[1] == '~')
path++;
@@ -638,7 +638,7 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
/*
* Add support for ssh port: ssh://host.xy:port/...
*/
- if (protocol == PROTO_SSH host != url)
+ if (protocol == PROTO_SSH separator == '/')
port = get_port(end);
*ret_host = xstrdup(host);
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 7f55b95..ac5b08b 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -604,19 +604,11 @@ do
test_expect_success fetch-pack --diag-url $h:$r '
check_prot_path $h:$r $p $r
'
+ # No /~ - ~ conversion
+ test_expect_success fetch-pack --diag-url $h:/~$r '
+ check_prot_host_path $h:/~$r $p $hh /~$r
+ '
done
- h=host
- hh=host
- # /~ - ~ conversion
- test_expect_failure fetch-pack --diag-url $h:/~$r '
- check_prot_host_path $h:/~$r $p $hh ~$r
- '
- h=[::1]
- hh=$(echo $h | tr -d [])
- # /~ - ~ conversion
- test_expect_success fetch-pack --diag-url $h:/~$r '
- check_prot_host_path $h:/~$r $p $hh ~$r
- '
done
test_expect_success MINGW 'fetch-pack --diag-url file://c:/repo' '
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index ba99972..57234c0 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -356,15 +356,7 @@ do
done
#ipv6
-# failing
-for repo in /~proj
-do
- test_expect_failure clone [::1]:$repo '
- test_clone_url [::1]:$repo ::1 $repo
- '
-done
-
-for repo in rep rep/home/project 123
+for repo in rep rep/home/project 123 /~proj
do
test_expect_success clone [::1]:$repo '
test_clone_url [::1]:$repo ::1 $repo
@@ -373,7 +365,7 @@ done
# Corner cases
# failing
-for repo in [foo]bar/baz:qux [foo/bar]:baz
+for url in [foo]bar/baz:qux [foo/bar]:baz
do
test_expect_failure clone $url is not ssh '
test_clone_url $url none
--
1.8.4.457.g424cb08
--
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 02/10] t5601: Add tests for ssh
Add more tests testing all the combinations:
-IPv4 or IPv6
-path starting with / or with /~
-with and without the ssh:// scheme
Some test fail, they need updates in connect.c
---
t/t5601-clone.sh | 102 +--
1 file changed, 100 insertions(+), 2 deletions(-)
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 83b21f5..ba99972 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -298,7 +298,7 @@ setup_ssh_wrapper () {
expect_ssh () {
test_when_finished '
- (cd $TRASH_DIRECTORY rm -f ssh-expect ssh-output)
+ (cd $TRASH_DIRECTORY rm -f ssh-expect ssh-output)
'
{
case $1 in
@@ -313,7 +313,7 @@ expect_ssh () {
setup_ssh_wrapper
-test_expect_success 'cloning myhost:src uses ssh' '
+test_expect_success 'clone myhost:src uses ssh' '
git clone myhost:src ssh-clone
expect_ssh myhost src
'
@@ -329,6 +329,104 @@ test_expect_success 'bracketed hostnames are still ssh' '
expect_ssh myhost:123 src
'
+counter=0
+# $1 url
+# $2 none|host
+# $3 path
+test_clone_url () {
+ counter=$(($counter + 1))
+ test_might_fail git clone $1 tmp$counter
+ expect_ssh $2 $3
+}
+
+test_expect_success NOT_MINGW 'clone c:temp is ssl' '
+ test_clone_url c:temp c temp
+'
+
+test_expect_success MINGW 'clone c:temp is dos drive' '
+ test_clone_url c:temp none
+'
+
+#ip v4
+for repo in rep rep/home/project /~proj 123
+do
+ test_expect_success clone host:$repo '
+ test_clone_url host:$repo host $repo
+ '
+done
+
+#ipv6
+# failing
+for repo in /~proj
+do
+ test_expect_failure clone [::1]:$repo '
+ test_clone_url [::1]:$repo ::1 $repo
+ '
+done
+
+for repo in rep rep/home/project 123
+do
+ test_expect_success clone [::1]:$repo '
+ test_clone_url [::1]:$repo ::1 $repo
+ '
+done
+
+# Corner cases
+# failing
+for repo in [foo]bar/baz:qux [foo/bar]:baz
+do
+ test_expect_failure clone $url is not ssh '
+ test_clone_url $url none
+ '
+done
+
+for url in foo/bar:baz
+do
+ test_expect_success clone $url is not ssh '
+ test_clone_url $url none
+ '
+done
+
+#with ssh:// scheme
+test_expect_success 'clone ssh://host.xz/home/user/repo' '
+ test_clone_url ssh://host.xz/home/user/repo host.xz /home/user/repo
+'
+
+# from home directory
+test_expect_success 'clone ssh://host.xz/~repo' '
+ test_clone_url ssh://host.xz/~repo host.xz ~repo
+'
+
+# with port number
+test_expect_success 'clone ssh://host.xz:22/home/user/repo' '
+ test_clone_url ssh://host.xz:22/home/user/repo -p 22 host.xz
/home/user/repo
+'
+
+# from home directory with port number
+test_expect_success 'clone ssh://host.xz:22/~repo' '
+ test_clone_url ssh://host.xz:22/~repo -p 22 host.xz ~repo
+'
+
+#IPv6
+test_expect_success 'clone ssh://[::1]/home/user/repo' '
+ test_clone_url ssh://[::1]/home/user/repo ::1 /home/user/repo
+'
+
+#IPv6 from home directory
+test_expect_success 'clone ssh://[::1]/~repo' '
+ test_clone_url ssh://[::1]/~repo ::1 ~repo
+'
+
+#IPv6 with port number
+test_expect_success 'clone ssh://[::1]:22/home/user/repo' '
+ test_clone_url ssh://[::1]:22/home/user/repo -p 22 ::1
/home/user/repo
+'
+
+#IPv6 from home directory with port number
+test_expect_success 'clone ssh://[::1]:22/~repo' '
+ test_clone_url ssh://[::1]:22/~repo -p 22 ::1 ~repo
+'
+
test_expect_success 'clone from a repository with two identical branches' '
(
--
1.8.4.457.g424cb08
--
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 03/10] git_connect: remove artificial limit of a remote command
Since day one, function git_connect() had a limit on the command line of
the command that is invoked to make a connection. 7a33bcbe converted the
code that constructs the command to strbuf. This would have been the
right time to remove the limit, but it did not happen. Remove it now.
git_connect() uses start_command() to invoke the command; consequently,
the limits of the system still apply, but are diagnosed only at execve()
time. But these limits are more lenient than the 1K that git_connect()
imposed.
Signed-off-by: Johannes Sixt j...@kdbg.org
---
connect.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/connect.c b/connect.c
index 06e88b0..6cc1f8d 100644
--- a/connect.c
+++ b/connect.c
@@ -527,8 +527,6 @@ static struct child_process *git_proxy_connect(int fd[2],
char *host)
return proxy;
}
-#define MAX_CMD_LEN 1024
-
static char *get_port(char *host)
{
char *end;
@@ -570,7 +568,7 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
int free_path = 0;
char *port = NULL;
const char **arg;
- struct strbuf cmd;
+ struct strbuf cmd = STRBUF_INIT;
/* Without this we cannot rely on waitpid() to tell
* what happened to our children.
@@ -676,12 +674,9 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
conn = xcalloc(1, sizeof(*conn));
- strbuf_init(cmd, MAX_CMD_LEN);
strbuf_addstr(cmd, prog);
strbuf_addch(cmd, ' ');
sq_quote_buf(cmd, path);
- if (cmd.len = MAX_CMD_LEN)
- die(command line too long);
conn-in = conn-out = -1;
conn-argv = arg = xcalloc(7, sizeof(*arg));
--
1.8.4.457.g424cb08
--
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 06/10] t5500: Test case for diag-url
Add test cases using git fetch-pack --diag-url:
- parse out host and path for URLs with a scheme (git:// file:// ssh://)
- parse host names embedded by [] correctly
- extract the port number, if present
- seperate URLs like file (which are local)
from URLs like host:repo which should use ssh
---
t/t5500-fetch-pack.sh | 72 +++
1 file changed, 56 insertions(+), 16 deletions(-)
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 9136f2a..7f55b95 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -533,7 +533,7 @@ test_expect_success 'shallow fetch with tags does not break
the repository' '
'
check_prot_path() {
actual
- (git fetch-pack --diag-url $1 21 1stdout) | grep -v host= actual
+ (git fetch-pack --diag-url $1 21 1stdout) | grep -v host= actual
echo Diag: url=$1 expected
echo Diag: protocol=$2 expected
echo Diag: path=$3 expected
@@ -542,7 +542,7 @@ check_prot_path() {
check_prot_host_path() {
actual
- git fetch-pack --diag-url $1 2actual
+ git fetch-pack --diag-url $1 2actual
echo Diag: url=$1 expected
echo Diag: protocol=$2 expected
echo Diag: host=$3 expected
@@ -564,29 +564,69 @@ do
hh=$h
pp=$p
fi
- test_expect_success fetch-pack $p://$h/$r '
+ test_expect_success fetch-pack --diag-url $p://$h/$r '
check_prot_host_path $p://$h/$r $pp $hh /$r
'
- # /~ - ~ conversion for git
- test_expect_success fetch-pack $p://$h/~$r '
+ # /~ - ~ conversion
+ test_expect_success fetch-pack --diag-url $p://$h/~$r
'
check_prot_host_path $p://$h/~$r $pp $hh ~$r
'
done
done
+ p=file
# file with scheme
- for p in file
+ for h in host host:12 [::1] [::1]:23
do
- for h in host host:12 [::1] [::1]:23
- do
- test_expect_success fetch-pack $p://$h/$r '
- check_prot_path $p://$h/$r $p /$r
- '
- # No /~ - ~ conversion for file
- test_expect_success fetch-pack $p://$h/~$r '
- check_prot_path $p://$h/~$r $p /~$r
- '
- done
+ test_expect_success fetch-pack --diag-url $p://$h/$r '
+ check_prot_path $p://$h/$r $p /$r
+ '
+ # No /~ - ~ conversion for file
+ test_expect_success fetch-pack --diag-url $p://$h/~$r '
+ check_prot_path $p://$h/~$r $p /~$r
+ '
+ done
+ # file without scheme
+ for h in nohost nohost:12 [::1] [::1]:23 [ [:aa
+ do
+ test_expect_success fetch-pack --diag-url ./$h:$r '
+ check_prot_path ./$h:$r $p ./$h:$r
+ '
+ # No /~ - ~ conversion for file
+ test_expect_success fetch-pack --diag-url ./$p:$h/~$r '
+ check_prot_path ./$p:$h/~$r $p ./$p:$h/~$r
+ '
+ done
+ #ssh without scheme
+ p=ssh
+ for h in host [::1]
+ do
+ hh=$(echo $h | tr -d [])
+ test_expect_success fetch-pack --diag-url $h:$r '
+ check_prot_path $h:$r $p $r
+ '
done
+ h=host
+ hh=host
+ # /~ - ~ conversion
+ test_expect_failure fetch-pack --diag-url $h:/~$r '
+ check_prot_host_path $h:/~$r $p $hh ~$r
+ '
+ h=[::1]
+ hh=$(echo $h | tr -d [])
+ # /~ - ~ conversion
+ test_expect_success fetch-pack --diag-url $h:/~$r '
+ check_prot_host_path $h:/~$r $p $hh ~$r
+ '
done
+test_expect_success MINGW 'fetch-pack --diag-url file://c:/repo' '
+ check_prot_path file://c:/repo file c:/repo
+'
+test_expect_success MINGW 'fetch-pack --diag-url file:///c:/repo' '
+ check_prot_path file://c:/repo file c:/repo
+'
+test_expect_success MINGW 'fetch-pack --diag-url c:repo' '
+ check_prot_path c:repo file c:repo
+'
+
test_done
--
1.8.4.457.g424cb08
--
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 04/10] git_connect: factor out discovery of the protocol and its parts
git_connect has grown large due to the many different protocols syntaxes
that are supported. Move the part of the function that parses the URL to
connect to into a separate function for readability.
Signed-off-by: Johannes Sixt j...@kdbg.org
---
connect.c | 80 ++-
1 file changed, 53 insertions(+), 27 deletions(-)
diff --git a/connect.c b/connect.c
index 6cc1f8d..a6cf345 100644
--- a/connect.c
+++ b/connect.c
@@ -543,37 +543,20 @@ static char *get_port(char *host)
return NULL;
}
-static struct child_process no_fork;
-
/*
- * This returns a dummy child_process if the transport protocol does not
- * need fork(2), or a struct child_process object if it does. Once done,
- * finish the connection with finish_connect() with the value returned from
- * this function (it is safe to call finish_connect() with NULL to support
- * the former case).
- *
- * If it returns, the connect is successful; it just dies on errors (this
- * will hopefully be changed in a libification effort, to return NULL when
- * the connection failed).
+ * Extract protocol and relevant parts from the specified connection URL.
+ * The caller must free() the returned strings.
*/
-struct child_process *git_connect(int fd[2], const char *url_orig,
- const char *prog, int flags)
+static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
+ char **ret_port, char **ret_path)
{
char *url;
char *host, *path;
char *end;
int c;
- struct child_process *conn = no_fork;
enum protocol protocol = PROTO_LOCAL;
int free_path = 0;
char *port = NULL;
- const char **arg;
- struct strbuf cmd = STRBUF_INIT;
-
- /* Without this we cannot rely on waitpid() to tell
-* what happened to our children.
-*/
- signal(SIGCHLD, SIG_DFL);
if (is_url(url_orig))
url = url_decode(url_orig);
@@ -645,6 +628,49 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
if (protocol == PROTO_SSH host != url)
port = get_port(end);
+ *ret_host = xstrdup(host);
+ if (port)
+ *ret_port = xstrdup(port);
+ else
+ *ret_port = NULL;
+ if (free_path)
+ *ret_path = path;
+ else
+ *ret_path = xstrdup(path);
+ free(url);
+ return protocol;
+}
+
+static struct child_process no_fork;
+
+/*
+ * This returns a dummy child_process if the transport protocol does not
+ * need fork(2), or a struct child_process object if it does. Once done,
+ * finish the connection with finish_connect() with the value returned from
+ * this function (it is safe to call finish_connect() with NULL to support
+ * the former case).
+ *
+ * If it returns, the connect is successful; it just dies on errors (this
+ * will hopefully be changed in a libification effort, to return NULL when
+ * the connection failed).
+ */
+struct child_process *git_connect(int fd[2], const char *url,
+ const char *prog, int flags)
+{
+ char *host, *path;
+ struct child_process *conn = no_fork;
+ enum protocol protocol;
+ char *port;
+ const char **arg;
+ struct strbuf cmd = STRBUF_INIT;
+
+ /* Without this we cannot rely on waitpid() to tell
+* what happened to our children.
+*/
+ signal(SIGCHLD, SIG_DFL);
+
+ protocol = parse_connect_url(url, host, port, path);
+
if (protocol == PROTO_GIT) {
/* These underlying connection commands die() if they
* cannot connect.
@@ -666,9 +692,9 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
prog, path, 0,
target_host, 0);
free(target_host);
- free(url);
- if (free_path)
- free(path);
+ free(host);
+ free(port);
+ free(path);
return conn;
}
@@ -709,9 +735,9 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
fd[0] = conn-out; /* read from child's stdout */
fd[1] = conn-in; /* write to child's stdin */
strbuf_release(cmd);
- free(url);
- if (free_path)
- free(path);
+ free(host);
+ free(port);
+ free(path);
return conn;
}
--
1.8.4.457.g424cb08
--
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 01/10] t5601: remove clear_ssh, refactor setup_ssh_wrapper
Commit 8d3d28f5 added test cases for URLs which should be ssh.
Remove the function clear_ssh, use test_when_finished to clean up.
Introduce the function setup_ssh_wrapper, which could be factored
out together with expect_ssh.
Tighten one test and use foo:bar instead of ./foo:bar,
Helped-by: Jeff King p...@peff.net
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
Changes since last version:
- updated t5601, thanks Peff
- Split up the patch into 10 commits
- Hannes suggested 2 patches
- Add tests for git fetch-pack, which verifies the parsing
- Added lots of test cases in t5500 via git fetch-pack --diag-url
t/t5601-clone.sh | 40
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 1d1c875..83b21f5 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -280,25 +280,26 @@ test_expect_success 'clone checking out a tag' '
test_cmp fetch.expected fetch.actual
'
-test_expect_success 'setup ssh wrapper' '
- write_script $TRASH_DIRECTORY/ssh-wrapper -\EOF
- echo $TRASH_DIRECTORY/ssh-output ssh: $*
- # throw away all but the last argument, which should be the
- # command
- while test $# -gt 1; do shift; done
- eval $1
- EOF
-
- GIT_SSH=$TRASH_DIRECTORY/ssh-wrapper
- export GIT_SSH
- export TRASH_DIRECTORY
-'
-
-clear_ssh () {
- $TRASH_DIRECTORY/ssh-output
+setup_ssh_wrapper () {
+ test_expect_success 'setup ssh wrapper' '
+ write_script $TRASH_DIRECTORY/ssh-wrapper -\EOF
+ echo $TRASH_DIRECTORY/ssh-output ssh: $*
+ # throw away all but the last argument, which should be the
+ # command
+ while test $# -gt 1; do shift; done
+ eval $1
+ EOF
+ GIT_SSH=$TRASH_DIRECTORY/ssh-wrapper
+ export GIT_SSH
+ export TRASH_DIRECTORY
+ $TRASH_DIRECTORY/ssh-output
+ '
}
expect_ssh () {
+ test_when_finished '
+ (cd $TRASH_DIRECTORY rm -f ssh-expect ssh-output)
+ '
{
case $1 in
none)
@@ -310,21 +311,20 @@ expect_ssh () {
(cd $TRASH_DIRECTORY test_cmp ssh-expect ssh-output)
}
+setup_ssh_wrapper
+
test_expect_success 'cloning myhost:src uses ssh' '
- clear_ssh
git clone myhost:src ssh-clone
expect_ssh myhost src
'
test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '
- clear_ssh
cp -R src foo:bar
- git clone ./foo:bar foobar
+ git clone foo:bar foobar
expect_ssh none
'
test_expect_success 'bracketed hostnames are still ssh' '
- clear_ssh
git clone [myhost:123]:src ssh-bracket-clone
expect_ssh myhost:123 src
'
--
1.8.4.457.g424cb08
--
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 10/10] git fetch: support host:/~repo
Since commit faea9ccb URLs host:~repo or ssh://host:/~repo
reach the home directory.
In 356bece0 support for [] was introduced, and as a side
effect, [host]:/~repo was the same as [host]:~repo.
The side effect was removed in c01049ae, connect.c: Corner case for IPv6.
Re-reading the documentation (in urls.txt) we find that
ssh://host:/~repo,
host:/~repo or
host:~repo
specifiy the repository repo in the home directory at host.
So make the handling of host:/~repo (or [host]:/~repo) a feature,
and revert the possible regression introduced in c01049ae.
---
connect.c | 3 +--
t/t5500-fetch-pack.sh | 4 ++--
t/t5601-clone.sh | 12 ++--
3 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/connect.c b/connect.c
index 95568ac..2cad296 100644
--- a/connect.c
+++ b/connect.c
@@ -625,8 +625,7 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
end = path; /* Need to \0 terminate host here */
if (separator == ':')
path++; /* path starts after ':' */
- if ((protocol == PROTO_GIT) ||
- (protocol == PROTO_SSH separator == '/')) {
+ if (protocol == PROTO_GIT || protocol == PROTO_SSH) {
if (path[1] == '~')
path++;
}
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 69a2110..7d9f18c 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -601,9 +601,9 @@ do
test_expect_success fetch-pack --diag-url $h:$r '
check_prot_path $h:$r $p $r
'
- # No /~ - ~ conversion
+ # Do the /~ - ~ conversion
test_expect_success fetch-pack --diag-url $h:/~$r '
- check_prot_host_path $h:/~$r $p $h /~$r
+ check_prot_host_path $h:/~$r $p $h ~$r
'
done
done
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index bd1bfd3..62fbd7e 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -348,7 +348,7 @@ test_expect_success MINGW 'clone c:temp is dos drive' '
'
#ip v4
-for repo in rep rep/home/project /~proj 123
+for repo in rep rep/home/project 123
do
test_expect_success clone host:$repo '
test_clone_url host:$repo host $repo
@@ -356,12 +356,20 @@ do
done
#ipv6
-for repo in rep rep/home/project 123 /~proj
+for repo in rep rep/home/project 123
do
test_expect_success clone [::1]:$repo '
test_clone_url [::1]:$repo ::1 $repo
'
done
+#home directory
+test_expect_success clone host:/~repo '
+ test_clone_url host:/~repo host ~repo
+'
+
+test_expect_success clone [::1]:/~repo '
+ test_clone_url [::1]:/~repo ::1 ~repo
+'
# Corner cases
for url in foo/bar:baz [foo]bar/baz:qux [foo/bar]:baz
--
1.8.4.457.g424cb08
--
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 08/10] git_connect(): Refactor the port handling
Use get_host_and_port() even for ssh.
Remove the variable port git_connect(), and simplify parse_connect_url()
Use only one return point in git_connect(), doing the free() and return conn.
t5601 had 2 corner test cases which now pass.
---
connect.c | 47 +--
t/t5500-fetch-pack.sh | 9 +++--
2 files changed, 16 insertions(+), 40 deletions(-)
diff --git a/connect.c b/connect.c
index 0cb88b8..3d174c8 100644
--- a/connect.c
+++ b/connect.c
@@ -540,16 +540,13 @@ static struct child_process *git_proxy_connect(int fd[2],
char *host)
return proxy;
}
-static char *get_port(char *host)
+static const char *get_port_numeric(const char *p)
{
char *end;
- char *p = strchr(host, ':');
-
if (p) {
long port = strtol(p + 1, end, 10);
if (end != p + 1 *end == '\0' 0 = port port 65536) {
- *p = '\0';
- return p+1;
+ return p;
}
}
@@ -561,7 +558,7 @@ static char *get_port(char *host)
* The caller must free() the returned strings.
*/
static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
- char **ret_port, char **ret_path)
+ char **ret_path)
{
char *url;
char *host, *path;
@@ -569,7 +566,6 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
int separator;
enum protocol protocol = PROTO_LOCAL;
int free_path = 0;
- char *port = NULL;
if (is_url(url_orig))
url = url_decode(url_orig);
@@ -588,16 +584,12 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
}
/*
-* Don't do destructive transforms with git:// as that
-* protocol code does '[]' unwrapping of its own.
+* Don't do destructive transforms as protocol code does
+* '[]' unwrapping in get_host_and_port()
*/
if (host[0] == '[') {
end = strchr(host + 1, ']');
if (end) {
- if (protocol != PROTO_GIT) {
- *end = 0;
- host++;
- }
end++;
} else
end = host;
@@ -635,17 +627,7 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
*ptr = '\0';
}
- /*
-* Add support for ssh port: ssh://host.xy:port/...
-*/
- if (protocol == PROTO_SSH separator == '/')
- port = get_port(end);
-
*ret_host = xstrdup(host);
- if (port)
- *ret_port = xstrdup(port);
- else
- *ret_port = NULL;
if (free_path)
*ret_path = path;
else
@@ -673,7 +655,6 @@ struct child_process *git_connect(int fd[2], const char
*url,
char *host, *path;
struct child_process *conn = no_fork;
enum protocol protocol;
- char *port;
const char **arg;
struct strbuf cmd = STRBUF_INIT;
@@ -682,18 +663,13 @@ struct child_process *git_connect(int fd[2], const char
*url,
*/
signal(SIGCHLD, SIG_DFL);
- protocol = parse_connect_url(url, host, port, path);
+ protocol = parse_connect_url(url, host, path);
if (flags CONNECT_DIAG_URL) {
fprintf(stderr, Diag: url=%s\n, url ? url : NULL);
fprintf(stderr, Diag: protocol=%s\n, prot_name(protocol));
- fprintf(stderr, Diag: host=%s, host ? host : NULL);
- if (port)
- fprintf(stderr, :%s\n, port);
- else
- fprintf(stderr, \n);
+ fprintf(stderr, Diag: host=%s\n, host ? host : NULL);
fprintf(stderr, Diag: path=%s\n, path ? path : NULL);
free(host);
- free(port);
free(path);
return NULL;
}
@@ -720,7 +696,6 @@ struct child_process *git_connect(int fd[2], const char
*url,
target_host, 0);
free(target_host);
free(host);
- free(port);
free(path);
return conn;
}
@@ -736,6 +711,11 @@ struct child_process *git_connect(int fd[2], const char
*url,
if (protocol == PROTO_SSH) {
const char *ssh = getenv(GIT_SSH);
int putty = ssh strcasestr(ssh, plink);
+ char *ssh_host = host; /* keep host for the free() below */
+ const char *port = NULL;
+ get_host_and_port(ssh_host, port);
+ port = get_port_numeric(port);
+
if (!ssh) ssh = ssh;
*arg++ = ssh;
@@ -746,7 +726,7 @@ struct child_process
[PATCH v6 05/10] git fetch-pack: Add --diag-url
The main purpose is to trace the URL parser called by git_connect() in
connect.c
The main features of the parser can be listed as this:
- parse out host and path for URLs with a scheme (git:// file:// ssh://)
- parse host names embedded by [] correctly
- extract the port number, if present
- seperate URLs like file (which are local)
from URLs like host:repo which should use ssh
Add the new parameter --diag-url to git fetch-pack,
which prints the value for protocol, host and path to stderr and exits.
---
builtin/fetch-pack.c | 14 ++---
connect.c | 27
connect.h | 1 +
fetch-pack.h | 1 +
t/t5500-fetch-pack.sh | 57 +++
5 files changed, 97 insertions(+), 3 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c8e8582..758b5ac 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -7,7 +7,7 @@
static const char fetch_pack_usage[] =
git fetch-pack [--all] [--stdin] [--quiet|-q] [--keep|-k] [--thin]
[--include-tag] [--upload-pack=git-upload-pack] [--depth=n]
-[--no-progress] [-v] [host:]directory [refs...];
+[--no-progress] [--diag-url] [-v] [host:]directory [refs...];
static void add_sought_entry_mem(struct ref ***sought, int *nr, int *alloc,
const char *name, int namelen)
@@ -81,6 +81,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char
*prefix)
args.stdin_refs = 1;
continue;
}
+ if (!strcmp(--diag-url, arg)) {
+ args.diag_url = 1;
+ continue;
+ }
if (!strcmp(-v, arg)) {
args.verbose = 1;
continue;
@@ -146,10 +150,14 @@ int cmd_fetch_pack(int argc, const char **argv, const
char *prefix)
fd[0] = 0;
fd[1] = 1;
} else {
+ int flags = args.verbose ? CONNECT_VERBOSE : 0;
+ if (args.diag_url)
+ flags |= CONNECT_DIAG_URL;
conn = git_connect(fd, dest, args.uploadpack,
- args.verbose ? CONNECT_VERBOSE : 0);
+ flags);
+ if (!conn)
+ return args.diag_url ? 0 : 1;
}
-
get_remote_heads(fd[0], NULL, 0, ref, 0, NULL);
ref = fetch_pack(args, fd, conn, ref, dest,
diff --git a/connect.c b/connect.c
index a6cf345..1b93b4d 100644
--- a/connect.c
+++ b/connect.c
@@ -236,6 +236,19 @@ enum protocol {
PROTO_GIT
};
+static const char *prot_name(enum protocol protocol) {
+ switch (protocol) {
+ case PROTO_LOCAL:
+ return file;
+ case PROTO_SSH:
+ return ssh;
+ case PROTO_GIT:
+ return git;
+ default:
+ return unkown protocol;
+ }
+}
+
static enum protocol get_protocol(const char *name)
{
if (!strcmp(name, ssh))
@@ -670,6 +683,20 @@ struct child_process *git_connect(int fd[2], const char
*url,
signal(SIGCHLD, SIG_DFL);
protocol = parse_connect_url(url, host, port, path);
+ if (flags CONNECT_DIAG_URL) {
+ fprintf(stderr, Diag: url=%s\n, url ? url : NULL);
+ fprintf(stderr, Diag: protocol=%s\n, prot_name(protocol));
+ fprintf(stderr, Diag: host=%s, host ? host : NULL);
+ if (port)
+ fprintf(stderr, :%s\n, port);
+ else
+ fprintf(stderr, \n);
+ fprintf(stderr, Diag: path=%s\n, path ? path : NULL);
+ free(host);
+ free(port);
+ free(path);
+ return NULL;
+ }
if (protocol == PROTO_GIT) {
/* These underlying connection commands die() if they
diff --git a/connect.h b/connect.h
index 64fb7db..527d58a 100644
--- a/connect.h
+++ b/connect.h
@@ -2,6 +2,7 @@
#define CONNECT_H
#define CONNECT_VERBOSE (1u 0)
+#define CONNECT_DIAG_URL (1u 1)
extern struct child_process *git_connect(int fd[2], const char *url, const
char *prog, int flags);
extern int finish_connect(struct child_process *conn);
extern int git_connection_is_socket(struct child_process *conn);
diff --git a/fetch-pack.h b/fetch-pack.h
index 461cbf3..20ccc12 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -14,6 +14,7 @@ struct fetch_pack_args {
use_thin_pack:1,
fetch_all:1,
stdin_refs:1,
+ diag_url:1,
verbose:1,
no_progress:1,
include_tag:1,
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index d87ddf7..9136f2a 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -531,5 +531,62 @@
[PATCH v6 09/10] connect.c: Refactor url parsing
Make the function is_local() from tramsport.c public and use it
in both transport.c and connect.c
Use a protocol local for URLs for the local file system.
---
connect.c| 58 ++--
connect.h| 1 +
t/t5601-clone.sh | 10 +-
transport.c | 8
4 files changed, 33 insertions(+), 44 deletions(-)
diff --git a/connect.c b/connect.c
index 3d174c8..95568ac 100644
--- a/connect.c
+++ b/connect.c
@@ -232,13 +232,23 @@ int server_supports(const char *feature)
enum protocol {
PROTO_LOCAL = 1,
+ PROTO_FILE,
PROTO_SSH,
PROTO_GIT
};
+int is_local(const char *url)
+{
+ const char *colon = strchr(url, ':');
+ const char *slash = strchr(url, '/');
+ return !colon || (slash slash colon) ||
+ has_dos_drive_prefix(url);
+}
+
static const char *prot_name(enum protocol protocol) {
switch (protocol) {
case PROTO_LOCAL:
+ case PROTO_FILE:
return file;
case PROTO_SSH:
return ssh;
@@ -260,7 +270,7 @@ static enum protocol get_protocol(const char *name)
if (!strcmp(name, ssh+git))
return PROTO_SSH;
if (!strcmp(name, file))
- return PROTO_LOCAL;
+ return PROTO_FILE;
die(I don't handle protocol '%s', name);
}
@@ -563,9 +573,8 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
char *url;
char *host, *path;
char *end;
- int separator;
+ int separator = '/';
enum protocol protocol = PROTO_LOCAL;
- int free_path = 0;
if (is_url(url_orig))
url = url_decode(url_orig);
@@ -577,10 +586,12 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
*host = '\0';
protocol = get_protocol(url);
host += 3;
- separator = '/';
} else {
host = url;
- separator = ':';
+ if (!is_local(url)) {
+ protocol = PROTO_SSH;
+ separator = ':';
+ }
}
/*
@@ -596,17 +607,12 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
} else
end = host;
- path = strchr(end, separator);
- if (path !has_dos_drive_prefix(end)) {
- if (separator == ':') {
- if (host != url || path strchrnul(host, '/')) {
- protocol = PROTO_SSH;
- *path++ = '\0';
- } else /* '/' in the host part, assume local path */
- path = end;
- }
- } else
+ if (protocol == PROTO_LOCAL)
+ path = end;
+ else if (protocol == PROTO_FILE has_dos_drive_prefix(end))
path = end;
+ else
+ path = strchr(end, separator);
if (!path || !*path)
die(No path specified. See 'man git-pull' for valid url
syntax);
@@ -615,23 +621,21 @@ static enum protocol parse_connect_url(const char
*url_orig, char **ret_host,
* null-terminate hostname and point path to ~ for URL's like this:
*ssh://host.xz/~user/repo
*/
- if (protocol != PROTO_LOCAL separator == '/') {
- char *ptr = path;
+
+ end = path; /* Need to \0 terminate host here */
+ if (separator == ':')
+ path++; /* path starts after ':' */
+ if ((protocol == PROTO_GIT) ||
+ (protocol == PROTO_SSH separator == '/')) {
if (path[1] == '~')
path++;
- else {
- path = xstrdup(ptr);
- free_path = 1;
- }
-
- *ptr = '\0';
}
+ path = xstrdup(path);
+ *end = '\0';
+
*ret_host = xstrdup(host);
- if (free_path)
- *ret_path = path;
- else
- *ret_path = xstrdup(path);
+ *ret_path = path;
free(url);
return protocol;
}
diff --git a/connect.h b/connect.h
index 527d58a..ce657b3 100644
--- a/connect.h
+++ b/connect.h
@@ -9,5 +9,6 @@ extern int git_connection_is_socket(struct child_process *conn);
extern int server_supports(const char *feature);
extern int parse_feature_request(const char *features, const char *feature);
extern const char *server_feature_value(const char *feature, int *len_ret);
+int is_local(const char *url);
#endif
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 57234c0..bd1bfd3 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -364,15 +364,7 @@ do
done
# Corner cases
-# failing
-for url in [foo]bar/baz:qux [foo/bar]:baz
-do
- test_expect_failure clone $url is not ssh '
[PATCH v6 01/10] t5601: remove clear_ssh, refactor setup_ssh_wrapper
Commit 8d3d28f5 added test cases for URLs which should be ssh.
Remove the function clear_ssh, use test_when_finished to clean up.
Introduce the function setup_ssh_wrapper, which could be factored
out together with expect_ssh.
Tighten one test and use foo:bar instead of ./foo:bar,
Helped-by: Jeff King p...@peff.net
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
Changes since last version:
- updated t5601, thanks Peff
- Split up the patch into 10 commits
- Hannes suggested 2 patches
- Add tests for git fetch-pack, which verifies the parsing
- Added lots of test cases in t5500 via git fetch-pack --diag-url
t/t5601-clone.sh | 40
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 1d1c875..83b21f5 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -280,25 +280,26 @@ test_expect_success 'clone checking out a tag' '
test_cmp fetch.expected fetch.actual
'
-test_expect_success 'setup ssh wrapper' '
- write_script $TRASH_DIRECTORY/ssh-wrapper -\EOF
- echo $TRASH_DIRECTORY/ssh-output ssh: $*
- # throw away all but the last argument, which should be the
- # command
- while test $# -gt 1; do shift; done
- eval $1
- EOF
-
- GIT_SSH=$TRASH_DIRECTORY/ssh-wrapper
- export GIT_SSH
- export TRASH_DIRECTORY
-'
-
-clear_ssh () {
- $TRASH_DIRECTORY/ssh-output
+setup_ssh_wrapper () {
+ test_expect_success 'setup ssh wrapper' '
+ write_script $TRASH_DIRECTORY/ssh-wrapper -\EOF
+ echo $TRASH_DIRECTORY/ssh-output ssh: $*
+ # throw away all but the last argument, which should be the
+ # command
+ while test $# -gt 1; do shift; done
+ eval $1
+ EOF
+ GIT_SSH=$TRASH_DIRECTORY/ssh-wrapper
+ export GIT_SSH
+ export TRASH_DIRECTORY
+ $TRASH_DIRECTORY/ssh-output
+ '
}
expect_ssh () {
+ test_when_finished '
+ (cd $TRASH_DIRECTORY rm -f ssh-expect ssh-output)
+ '
{
case $1 in
none)
@@ -310,21 +311,20 @@ expect_ssh () {
(cd $TRASH_DIRECTORY test_cmp ssh-expect ssh-output)
}
+setup_ssh_wrapper
+
test_expect_success 'cloning myhost:src uses ssh' '
- clear_ssh
git clone myhost:src ssh-clone
expect_ssh myhost src
'
test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '
- clear_ssh
cp -R src foo:bar
- git clone ./foo:bar foobar
+ git clone foo:bar foobar
expect_ssh none
'
test_expect_success 'bracketed hostnames are still ssh' '
- clear_ssh
git clone [myhost:123]:src ssh-bracket-clone
expect_ssh myhost:123 src
'
--
1.8.4.457.g424cb08
--
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: write() _will_ fail on Mac OS X/XNU if nbytes INT_MAX
Hej,
I think the patch went in and out in git.git, please see below.
(I coudn't the following in msysgit,
but if it was there, the clipped_write() for Windows could go away.
/Torsten
commit 0b6806b9e45c659d25b87fb5713c920a3081eac8
Author: Steffen Prohaska proha...@zib.de
Date: Tue Aug 20 08:43:54 2013 +0200
xread, xwrite: limit size of IO to 8MB
Checking out 2GB or more through an external filter (see test) fails
on Mac OS X 10.8.4 (12E55) for a 64-bit executable with:
error: read from external filter cat failed
error: cannot feed the input to external filter cat
error: cat died of signal 13
error: external filter cat failed 141
error: external filter cat failed
The reason is that read() immediately returns with EINVAL when asked
to read more than 2GB. According to POSIX [1], if the value of
nbyte passed to read() is greater than SSIZE_MAX, the result is
implementation-defined. The write function has the same restriction
[2]. Since OS X still supports running 32-bit executables, the
32-bit limit (SSIZE_MAX = INT_MAX = 2GB - 1) seems to be also
imposed on 64-bit executables under certain conditions. For write,
the problem has been addressed earlier [6c642a].
Address the problem for read() and write() differently, by limiting
size of IO chunks unconditionally on all platforms in xread() and
xwrite(). Large chunks only cause problems, like causing latencies
when killing the process, even if OS X was not buggy. Doing IO in
reasonably sized smaller chunks should have no negative impact on
performance.
The compat wrapper clipped_write() introduced earlier [6c642a] is
not needed anymore. It will be reverted in a separate commit. The
new test catches read and write problems.
Note that 'git add' exits with 0 even if it prints filtering errors
to stderr. The test, therefore, checks stderr. 'git add' should
probably be changed (sometime in another commit) to exit with
nonzero if filtering fails. The test could then be changed to use
test_must_fail.
On 2013-11-20 11.15, Erik Faye-Lund wrote:
I know I'm extremely late to the party, and this patch has already
landed, but...
On Sat, May 11, 2013 at 1:05 AM, Junio C Hamano gits...@pobox.com wrote:
Filipe Cabecinhas fil...@gmail.com writes:
Due to a bug in the Darwin kernel, write() calls have a maximum size of
INT_MAX bytes.
This patch introduces a new compat function: clipped_write
This function behaves the same as write() but will write, at most, INT_MAX
characters.
It may be necessary to include this function on Windows, too.
We are already doing something similar for Windows in mingw_write (see
compat/mingw.c), but with a much smaller size.
It feels a bit pointless to duplicate this logic.
diff --git a/compat/clipped-write.c b/compat/clipped-write.c
new file mode 100644
index 000..9183698
--- /dev/null
+++ b/compat/clipped-write.c
@@ -0,0 +1,13 @@
+#include limits.h
+#include unistd.h
+
+/*
+ * Version of write that will write at most INT_MAX bytes.
+ * Workaround a xnu bug on Mac OS X
+ */
+ssize_t clipped_write(int fildes, const void *buf, size_t nbyte)
+{
+ if (nbyte INT_MAX)
+ nbyte = INT_MAX;
+ return write(fildes, buf, nbyte);
+}
If we were to reuse this logic with Windows, we'd need to have some
way of overriding the max-size of the write.
--
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
--
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] git-fetch-pack uses URLs like git-fetch
On 2013-11-11 18.44, Junio C Hamano wrote: Torsten Bögershausen tbo...@web.de writes: git fetch-pack allows [host:]directory to point out the source repository. Use the term repository, which is already used in git fetch or git pull to describe URLs supported by Git. Sign-off? Sorry, forgot -s. If the patch makes sense otherwise, are you willing to squeeze this in: Signed-off-by: Torsten Bögershausen tbo...@web.de --- Documentation/git-fetch-pack.txt | 15 +++ 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt index 444b805..93b5067 100644 --- a/Documentation/git-fetch-pack.txt +++ b/Documentation/git-fetch-pack.txt @@ -12,7 +12,7 @@ SYNOPSIS 'git fetch-pack' [--all] [--quiet|-q] [--keep|-k] [--thin] [--include-tag] [--upload-pack=git-upload-pack] [--depth=n] [--no-progress] -[-v] [host:]directory [refs...] +[-v] repository [refs...] DESCRIPTION --- @@ -97,19 +97,18 @@ be in a separate packet, and the list must end with a flush packet. -v:: Run verbosely. -host:: -A remote host that houses the repository. When this -part is specified, 'git-upload-pack' is invoked via -ssh. - -directory:: -The repository to sync from. +repository:: +The URL to the remote repository. refs...:: The remote heads to update from. This is relative to $GIT_DIR (e.g. HEAD, refs/heads/master). When unspecified, update from all heads the remote side has. +SEE ALSO + +linkgit:git-fetch[1] + GIT --- Part of the linkgit:git[1] suite -- 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 -- 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 0/5] fix up 'jk/pack-bitmap' branch
On 2013-11-08 23.29, Jeff King wrote: On Fri, Nov 08, 2013 at 06:10:30PM +0100, Torsten Bögershausen wrote: Side question: Do we have enough test coverage for htonll()/ntohll(), or do we want do the module test which I send a couple of days before ? The series adds tests for building and using the ewah bitmaps, which in turn rely on the htonll code. So they are being tested in the existing series. -Peff You are thinking about t5310-pack-bitmaps.sh ? If I do like this in compat/bswap.h # define ntohll(n) (n) # define htonll(n) (n) (on an Intel processor, little endian) then t5310 passes, even if the uint64_t words are written in little endian to disc instead of big endian. What do I miss? /torsten -- 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 0/5] fix up 'jk/pack-bitmap' branch
On 2013-11-07 23.19, Jeff King wrote: On Thu, Nov 07, 2013 at 09:58:02PM +, Ramsay Jones wrote: These patches fix various errors/warnings on the cygwin, MinGW and msvc builds, provoked by the jk/pack-bitmap branch. Thanks. Your timing is impeccable, as I was just sitting down to finalize the next re-roll. I'll add these in. -Peff Side question: Do we have enough test coverage for htonll()/ntohll(), or do we want do the module test which I send a couple of days before ? /Torsten -- 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] git-fetch-pack uses URLs like git-fetch
git fetch-pack allows [host:]directory to point out the source repository. Use the term repository, which is already used in git fetch or git pull to describe URLs supported by Git. --- Documentation/git-fetch-pack.txt | 15 +++ 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt index 444b805..93b5067 100644 --- a/Documentation/git-fetch-pack.txt +++ b/Documentation/git-fetch-pack.txt @@ -12,7 +12,7 @@ SYNOPSIS 'git fetch-pack' [--all] [--quiet|-q] [--keep|-k] [--thin] [--include-tag] [--upload-pack=git-upload-pack] [--depth=n] [--no-progress] - [-v] [host:]directory [refs...] + [-v] repository [refs...] DESCRIPTION --- @@ -97,19 +97,18 @@ be in a separate packet, and the list must end with a flush packet. -v:: Run verbosely. -host:: - A remote host that houses the repository. When this - part is specified, 'git-upload-pack' is invoked via - ssh. - -directory:: - The repository to sync from. +repository:: + The URL to the remote repository. refs...:: The remote heads to update from. This is relative to $GIT_DIR (e.g. HEAD, refs/heads/master). When unspecified, update from all heads the remote side has. +SEE ALSO + +linkgit:git-fetch[1] + GIT --- Part of the linkgit:git[1] suite -- 1.8.4.457.g424cb08 -- 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] git_connect: factor out discovery of the protocol and its parts
On 2013-11-05 22.22, Johannes Sixt wrote:
Am 05.11.2013 21:45, schrieb Torsten Bögershausen:
On 2013-11-05 20.39, Johannes Sixt wrote:
Thanks for picking this up, please see some minor nits inline,
and git_connect() is at the end
-struct child_process *git_connect(int fd[2], const char *url_orig,
- const char *prog, int flags)
+static enum protocol parse_connect_url(const char *url_orig, char
**ret_host,
+ char **ret_port, char **ret_path)
{
char *url;
char *host, *path;
char *end;
Can we put all the char * into one single line?
The idea here was to keep the diff minimal, and that further slight
cleanups should be combined with subsequent rewrites that should happen
to this function.
int c;
@@ -645,6 +628,49 @@ struct child_process *git_connect(int fd[2], const
char *url_orig,
if (protocol == PROTO_SSH host != url)
port = get_port(end);
+ *ret_host = xstrdup(host);
+ if (port)
+ *ret_port = xstrdup(port);
+ else
+ *ret_port = NULL;
+ if (free_path)
+ *ret_path = path;
+ else
+ *ret_path = xstrdup(path);
+ free(url);
+ return protocol;
+}
+
+static struct child_process no_fork;
+
+/*
+ * This returns a dummy child_process if the transport protocol does not
+ * need fork(2), or a struct child_process object if it does. Once done,
+ * finish the connection with finish_connect() with the value returned from
+ * this function (it is safe to call finish_connect() with NULL to support
+ * the former case).
+ *
+ * If it returns, the connect is successful; it just dies on errors (this
+ * will hopefully be changed in a libification effort, to return NULL when
+ * the connection failed).
+ */
+struct child_process *git_connect(int fd[2], const char *url,
+ const char *prog, int flags)
+{
+ char *host, *path;
+ struct child_process *conn = no_fork;
+ enum protocol protocol;
+ char *port;
+ const char **arg;
+ struct strbuf cmd = STRBUF_INIT;
+
+ /* Without this we cannot rely on waitpid() to tell
+* what happened to our children.
+*/
+ signal(SIGCHLD, SIG_DFL);
+
+ protocol = parse_connect_url(url, host, port, path);
+
if (protocol == PROTO_GIT) {
/* These underlying connection commands die() if they
* cannot connect.
@@ -666,9 +692,9 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
prog, path, 0,
target_host, 0);
free(target_host);
This is hard to see in the diff, I think we don't need target_host any more.
I though that as well first, but no, we still need it. Further rewrites
are needed that move the port discovery from git_proxy_connect() and
git_tcp_connect() to the new parse_connect_url() before target_host can
go away. And even then it is questionable because target_host is used in
an error message and is intended to reflect the original combined
host+port portion of the URL, if I read the code correctly.
- free(url);
- if (free_path)
- free(path);
+ free(host);
+ free(port);
+ free(path);
return conn;
}
@@ -709,9 +735,9 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
fd[0] = conn-out; /* read from child's stdout */
fd[1] = conn-in; /* write to child's stdin */
strbuf_release(cmd);
- free(url);
- if (free_path)
- free(path);
This end of function, free everything and return conn,
could we re-arange so that it is in the code only once ?
That would be quite simple now; just place the part after the first
return into the else branch. That opens opportunities to move variable
declarations from the top of the function into the else branch.
But all of these changes should go into a separate commit, IMO, so that
the function splitting that happens here can be verified more easily.
-- Hannes
Agreed on all points, (some re-reading was needed)
I will first focus on the test cases,
since having god test cases eases us the re-factoring later on.
Thanks
/Torsten
--
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: htonll, ntohll
On 2013-11-05 01.00, Ramsay Jones wrote:
On 31/10/13 13:24, Torsten Bögershausen wrote:
On 2013-10-30 22.07, Ramsay Jones wrote:
[ ... ]
Yep, this was the first thing I did as well! ;-) (*late* last night)
I haven't had time today to look into fixing up the msvc build
(or a complete re-write), so I look forward to seeing your solution.
(do you have msvc available? - or do you want me to look at fixing
it? maybe in a day or two?)
Ramsay,
I don't have msvc, so feel free to go ahead, as much as you can.
I'll send a patch for the test code I have made, and put bswap.h on hold for
a week
(to be able to continue with t5601/connect.c)
Unfortunately, I haven't had much time to look into this.
I do have a patch (given below) that works on Linux, cygwin,
MinGW and msvc. However, the msvc build is still broken (as a
result of _other_ commits in this 'jk/pack-bitmap' branch; as
well as the use of a VLA in another commit).
So, I still have work to do! :(
Anyway, I thought I would send what I have, so you can take a look.
Note, that I don't have an big-endian machine to test this on, so
YMMV. Indeed, the *only* testing I have done is to run the test added
by this branch (t5310-pack-bitmaps.sh), which works on Linux, cygwin
and MinGW.
[Note: I have never particularly liked htons, htonl et.al., so adding
these htonll/ntohll functions doesn't thrill me! :-D For example see
this post[1], which echo's my sentiments exactly.]
HTH
ATB,
Ramsay Jones
[1] http://commandcenter.blogspot.co.uk/2012/04/byte-order-fallacy.html
-- 8 --
Subject: [PATCH] compat/bswap.h: Fix build on cygwin, MinGW and msvc
---
compat/bswap.h | 97
--
1 file changed, 68 insertions(+), 29 deletions(-)
diff --git a/compat/bswap.h b/compat/bswap.h
index ea1a9ed..c18a78e 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -17,7 +17,20 @@ static inline uint32_t default_swab32(uint32_t val)
((val 0x00ff) 24));
}
+static inline uint64_t default_bswap64(uint64_t val)
+{
+ return (((val (uint64_t)0x00ffULL) 56) |
+ ((val (uint64_t)0xff00ULL) 40) |
+ ((val (uint64_t)0x00ffULL) 24) |
+ ((val (uint64_t)0xff00ULL) 8) |
+ ((val (uint64_t)0x00ffULL) 8) |
+ ((val (uint64_t)0xff00ULL) 24) |
+ ((val (uint64_t)0x00ffULL) 40) |
+ ((val (uint64_t)0xff00ULL) 56));
+}
+
#undef bswap32
+#undef bswap64
#if defined(__GNUC__) (defined(__i386__) || defined(__x86_64__))
@@ -32,54 +45,80 @@ static inline uint32_t git_bswap32(uint32_t x)
return result;
}
+#define bswap64 git_bswap64
+#if defined(__x86_64__)
+static inline uint64_t git_bswap64(uint64_t x)
+{
+ uint64_t result;
+ if (__builtin_constant_p(x))
+ result = default_bswap64(x);
+ else
+ __asm__(bswap %q0 : =r (result) : 0 (x));
+ return result;
+}
+#else
+static inline uint64_t git_bswap64(uint64_t x)
+{
+ union { uint64_t i64; uint32_t i32[2]; } tmp, result;
+ if (__builtin_constant_p(x))
+ result.i64 = default_bswap64(x);
+ else {
+ tmp.i64 = x;
+ result.i32[0] = git_bswap32(tmp.i32[1]);
+ result.i32[1] = git_bswap32(tmp.i32[0]);
+ }
+ return result.i64;
+}
+#endif
+
#elif defined(_MSC_VER) (defined(_M_IX86) || defined(_M_X64))
#include stdlib.h
#define bswap32(x) _byteswap_ulong(x)
+#define bswap64(x) _byteswap_uint64(x)
#endif
-#ifdef bswap32
+#if defined(bswap32)
#undef ntohl
#undef htonl
#define ntohl(x) bswap32(x)
#define htonl(x) bswap32(x)
-#ifndef __BYTE_ORDER
-#if defined(BYTE_ORDER) defined(LITTLE_ENDIAN) defined(BIG_ENDIAN)
-#define __BYTE_ORDER BYTE_ORDER
-#define __LITTLE_ENDIAN LITTLE_ENDIAN
-#define __BIG_ENDIAN BIG_ENDIAN
-#else
-#error Cannot determine endianness
-#endif
+#endif
+
+#if defined(bswap64)
+
+#undef ntohll
+#undef htonll
+#define ntohll(x) bswap64(x)
+#define htonll(x) bswap64(x)
+
+#else
+
+#undef ntohll
+#undef htonll
+
+#if !defined(__BYTE_ORDER)
+# if defined(BYTE_ORDER) defined(LITTLE_ENDIAN) defined(BIG_ENDIAN)
+# define __BYTE_ORDER BYTE_ORDER
+# define __LITTLE_ENDIAN LITTLE_ENDIAN
+# define __BIG_ENDIAN BIG_ENDIAN
+# endif
+#endif
+
+#if !defined(__BYTE_ORDER)
+# error Cannot determine endianness
#endif
#if __BYTE_ORDER == __BIG_ENDIAN
# define ntohll(n) (n)
# define htonll(n) (n)
-#elif __BYTE_ORDER == __LITTLE_ENDIAN
-#if defined(__GNUC__) defined(__GLIBC__)
-#include byteswap.h
-#else /* GNUC GLIBC */
-static inline uint64_t bswap_64(uint64_t val)
-{
- return ((val (uint64_t
Re: [PATCH 2/2] git_connect: factor out discovery of the protocol and its parts
On 2013-11-05 20.39, Johannes Sixt wrote:
Thanks for picking this up, please see some minor nits inline,
and git_connect() is at the end
-struct child_process *git_connect(int fd[2], const char *url_orig,
- const char *prog, int flags)
+static enum protocol parse_connect_url(const char *url_orig, char **ret_host,
+char **ret_port, char **ret_path)
{
char *url;
char *host, *path;
char *end;
Can we put all the char * into one single line?
int c;
@@ -645,6 +628,49 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
if (protocol == PROTO_SSH host != url)
port = get_port(end);
+ *ret_host = xstrdup(host);
+ if (port)
+ *ret_port = xstrdup(port);
+ else
+ *ret_port = NULL;
+ if (free_path)
+ *ret_path = path;
+ else
+ *ret_path = xstrdup(path);
+ free(url);
+ return protocol;
+}
+
+static struct child_process no_fork;
+
+/*
+ * This returns a dummy child_process if the transport protocol does not
+ * need fork(2), or a struct child_process object if it does. Once done,
+ * finish the connection with finish_connect() with the value returned from
+ * this function (it is safe to call finish_connect() with NULL to support
+ * the former case).
+ *
+ * If it returns, the connect is successful; it just dies on errors (this
+ * will hopefully be changed in a libification effort, to return NULL when
+ * the connection failed).
+ */
+struct child_process *git_connect(int fd[2], const char *url,
+ const char *prog, int flags)
+{
+ char *host, *path;
+ struct child_process *conn = no_fork;
+ enum protocol protocol;
+ char *port;
+ const char **arg;
+ struct strbuf cmd = STRBUF_INIT;
+
+ /* Without this we cannot rely on waitpid() to tell
+ * what happened to our children.
+ */
+ signal(SIGCHLD, SIG_DFL);
+
+ protocol = parse_connect_url(url, host, port, path);
+
if (protocol == PROTO_GIT) {
/* These underlying connection commands die() if they
* cannot connect.
@@ -666,9 +692,9 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
prog, path, 0,
target_host, 0);
free(target_host);
This is hard to see in the diff, I think we don't need target_host any more.
- free(url);
- if (free_path)
- free(path);
+ free(host);
+ free(port);
+ free(path);
return conn;
}
@@ -709,9 +735,9 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
fd[0] = conn-out; /* read from child's stdout */
fd[1] = conn-in; /* write to child's stdin */
strbuf_release(cmd);
- free(url);
- if (free_path)
- free(path);
This end of function, free everything and return conn,
could we re-arange so that it is in the code only once ?
+ free(host);
+ free(port);
+ free(path);
return conn;
}
struct child_process *git_connect(int fd[2], const char *url,
const char *prog, int flags)
{
char *host, *port, *path;
struct child_process *conn = no_fork;
enum protocol protocol;
const char **arg;
struct strbuf cmd = STRBUF_INIT;
/* Without this we cannot rely on waitpid() to tell
* what happened to our children.
*/
signal(SIGCHLD, SIG_DFL);
protocol = parse_connect_url(url, host, port, path);
if (protocol == PROTO_GIT) {
/* These underlying connection commands die() if they
* cannot connect.
*/
if (git_use_proxy(host))
conn = git_proxy_connect(fd, host);
else
git_tcp_connect(fd, host, flags);
/*
* Separate original protocol components prog and path
* from extended host header with a NUL byte.
*
* Note: Do not add any other headers here! Doing so
* will cause older git-daemon servers to crash.
*/
packet_write(fd[1],
%s %s%chost=%s%c,
prog, path, 0,
host, 0);
}
else
{
conn = xcalloc(1, sizeof(*conn));
strbuf_addstr(cmd, prog);
strbuf_addch(cmd, ' ');
sq_quote_buf(cmd, path);
conn-in = conn-out = -1;
conn-argv = arg = xcalloc(7, sizeof(*arg));
if (protocol == PROTO_SSH) {
const char *ssh = getenv(GIT_SSH);
int putty = ssh strcasestr(ssh, plink);
if (!ssh) ssh = ssh;
*arg++ = ssh;
if (putty !strcasestr(ssh, tortoiseplink))
*arg++ = -batch;
if (port) {
/* P is for PuTTY, p
[PATCH V4] git clone: is an URL local or ssh
Commit 8d3d28f5 added test cases for URLs which should be ssh.
Add more tests testing all the combinations:
-IPv4 or IPv6
-path starting with / or with /~
-with and without the ssh:// scheme
Add tests for ssh:// with port number.
When a git repository foo:bar exist, git clone will call
absolute_path() and git_connect() will be called with
something like /home/user/projects/foo:bar
Tighten the test and use foo:bar instead of ./foo:bar,
refactor clear_ssh() and expect_ssh() into test_clone_url().
git clone foo/bar:baz should not be ssh:
Make the rule
if there is a slash before the first colon, it is not ssh
more strict by using the same is_local() in both connect.c
and transport.c. Add a test case.
Bug fixes for corner cases:
- Handle clone of [::1]:/~repo correctly (2e7766655):
Git should call ssh ::1 ~repo, not ssh ::1 /~repo
This was caused by looking at (host != url), which never
worked for host names with literal IPv6 adresses, embedded by []
Support for the [] URLs was introduced in 356bece0a.
- Do not tamper local URLs starting with '[' which have a ']'
Bug fix for msygit in t5601 : use $PWD insted of $(pwd)
Helped-by: Jeff King p...@peff.net
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
Changes since V3:
- Integrated Peffs suggestions in t5601
(Remove clear_ssh)
- Decide early if it is ssl or local in connect.c
- Use git fetch instead of git clone in t5601:
clone use absolute_path() (adding a / before :), fetch does not
- Add a test for dos_drive C:temp for msys
- Make tests work for msys by using $PWD instead of $(pwd) (file://$PWD)
connect.c| 50
connect.h| 1 +
t/t5601-clone.sh | 117 ---
transport.c | 8
4 files changed, 130 insertions(+), 46 deletions(-)
diff --git a/connect.c b/connect.c
index 06e88b0..022d122 100644
--- a/connect.c
+++ b/connect.c
@@ -547,6 +547,25 @@ static char *get_port(char *host)
static struct child_process no_fork;
+int is_local(const char *url)
+{
+ const char *colon = strchr(url, ':');
+ const char *slash = strchr(url, '/');
+ if (has_dos_drive_prefix(url))
+ return 1;
+ if (!colon)
+ return 1;
+ if (slash slash colon)
+ return 1;
+ if (url[0] == '[') {
+ const char *end = strchr(url + 1, ']');
+ if (!end)
+ return 1;
+ return is_local(end);
+ }
+ return 0;
+}
+
/*
* This returns a dummy child_process if the transport protocol does not
* need fork(2), or a struct child_process object if it does. Once done,
@@ -564,7 +583,7 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
char *url;
char *host, *path;
char *end;
- int c;
+ int separator;
struct child_process *conn = no_fork;
enum protocol protocol = PROTO_LOCAL;
int free_path = 0;
@@ -587,17 +606,19 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
*host = '\0';
protocol = get_protocol(url);
host += 3;
- c = '/';
+ separator = '/';
} else {
host = url;
- c = ':';
+ protocol = is_local(url) ? PROTO_LOCAL : PROTO_SSH;
+ separator = ':';
}
/*
* Don't do destructive transforms with git:// as that
* protocol code does '[]' unwrapping of its own.
+* Don't change local URLs
*/
- if (host[0] == '[') {
+ if (protocol != PROTO_LOCAL host[0] == '[') {
end = strchr(host + 1, ']');
if (end) {
if (protocol != PROTO_GIT) {
@@ -610,17 +631,14 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
} else
end = host;
- path = strchr(end, c);
- if (path !has_dos_drive_prefix(end)) {
- if (c == ':') {
- if (host != url || path strchrnul(host, '/')) {
- protocol = PROTO_SSH;
- *path++ = '\0';
- } else /* '/' in the host part, assume local path */
- path = end;
+ path = strchr(end, separator);
+ if (separator == ':') {
+ if (path protocol == PROTO_SSH) {
+ *path++ = '\0';
+ } else {/* assume local path */
+ path = end;
}
- } else
- path = end;
+ }
if (!path || !*path)
die(No path specified. See 'man git-pull' for valid url
syntax);
@@ -629,7 +647,7 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
* null-terminate hostname and point path to ~ for URL's like this:
*ssh
[PATCH V4] git clone: is an URL local or ssh
Commit 8d3d28f5 added test cases for URLs which should be ssh.
Add more tests testing all the combinations:
-IPv4 or IPv6
-path starting with / or with /~
-with and without the ssh:// scheme
Add tests for ssh:// with port number.
When a git repository foo:bar exist, git clone will call
absolute_path() and git_connect() will be called with
something like /home/user/projects/foo:bar
Tighten the test and use foo:bar instead of ./foo:bar,
refactor clear_ssh() and expect_ssh() into test_clone_url().
git clone foo/bar:baz should not be ssh:
Make the rule
if there is a slash before the first colon, it is not ssh
more strict by using the same is_local() in both connect.c
and transport.c. Add a test case.
Bug fixes for corner cases:
- Handle clone of [::1]:/~repo correctly (2e7766655):
Git should call ssh ::1 ~repo, not ssh ::1 /~repo
This was caused by looking at (host != url), which never
worked for host names with literal IPv6 adresses, embedded by []
Support for the [] URLs was introduced in 356bece0a.
- Do not tamper local URLs starting with '[' which have a ']'
Bug fix for msygit in t5601 : use $PWD insted of $(pwd)
Helped-by: Jeff King p...@peff.net
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
Changes since V3:
- Integrated Peffs suggestions in t5601
(Remove clear_ssh)
- Decide early if it is ssl or local in connect.c
- Use git fetch instead of git clone in t5601:
clone use absolute_path() (adding a / before :), fetch does not
- Add a test for dos_drive C:temp for msys
- Make tests work for msys by using $PWD instead of $(pwd) (file://$PWD)
connect.c| 50
connect.h| 1 +
t/t5601-clone.sh | 117 ---
transport.c | 8
4 files changed, 130 insertions(+), 46 deletions(-)
diff --git a/connect.c b/connect.c
index 06e88b0..022d122 100644
--- a/connect.c
+++ b/connect.c
@@ -547,6 +547,25 @@ static char *get_port(char *host)
static struct child_process no_fork;
+int is_local(const char *url)
+{
+ const char *colon = strchr(url, ':');
+ const char *slash = strchr(url, '/');
+ if (has_dos_drive_prefix(url))
+ return 1;
+ if (!colon)
+ return 1;
+ if (slash slash colon)
+ return 1;
+ if (url[0] == '[') {
+ const char *end = strchr(url + 1, ']');
+ if (!end)
+ return 1;
+ return is_local(end);
+ }
+ return 0;
+}
+
/*
* This returns a dummy child_process if the transport protocol does not
* need fork(2), or a struct child_process object if it does. Once done,
@@ -564,7 +583,7 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
char *url;
char *host, *path;
char *end;
- int c;
+ int separator;
struct child_process *conn = no_fork;
enum protocol protocol = PROTO_LOCAL;
int free_path = 0;
@@ -587,17 +606,19 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
*host = '\0';
protocol = get_protocol(url);
host += 3;
- c = '/';
+ separator = '/';
} else {
host = url;
- c = ':';
+ protocol = is_local(url) ? PROTO_LOCAL : PROTO_SSH;
+ separator = ':';
}
/*
* Don't do destructive transforms with git:// as that
* protocol code does '[]' unwrapping of its own.
+* Don't change local URLs
*/
- if (host[0] == '[') {
+ if (protocol != PROTO_LOCAL host[0] == '[') {
end = strchr(host + 1, ']');
if (end) {
if (protocol != PROTO_GIT) {
@@ -610,17 +631,14 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
} else
end = host;
- path = strchr(end, c);
- if (path !has_dos_drive_prefix(end)) {
- if (c == ':') {
- if (host != url || path strchrnul(host, '/')) {
- protocol = PROTO_SSH;
- *path++ = '\0';
- } else /* '/' in the host part, assume local path */
- path = end;
+ path = strchr(end, separator);
+ if (separator == ':') {
+ if (path protocol == PROTO_SSH) {
+ *path++ = '\0';
+ } else {/* assume local path */
+ path = end;
}
- } else
- path = end;
+ }
if (!path || !*path)
die(No path specified. See 'man git-pull' for valid url
syntax);
@@ -629,7 +647,7 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
* null-terminate hostname and point path to ~ for URL's like this:
*ssh
[PATCH V4] git clone: is an URL local or ssh
Commit 8d3d28f5 added test cases for URLs which should be ssh.
Add more tests testing all the combinations:
-IPv4 or IPv6
-path starting with / or with /~
-with and without the ssh:// scheme
Add tests for ssh:// with port number.
When a git repository foo:bar exist, git clone will call
absolute_path() and git_connect() will be called with
something like /home/user/projects/foo:bar
Tighten the test and use foo:bar instead of ./foo:bar,
refactor clear_ssh() and expect_ssh() into test_clone_url().
git clone foo/bar:baz should not be ssh:
Make the rule
if there is a slash before the first colon, it is not ssh
more strict by using the same is_local() in both connect.c
and transport.c. Add a test case.
Bug fixes for corner cases:
- Handle clone of [::1]:/~repo correctly (2e7766655):
Git should call ssh ::1 ~repo, not ssh ::1 /~repo
This was caused by looking at (host != url), which never
worked for host names with literal IPv6 adresses, embedded by []
Support for the [] URLs was introduced in 356bece0a.
- Do not tamper local URLs starting with '[' which have a ']'
Bug fix for msygit in t5601 : use $PWD insted of $(pwd)
Helped-by: Jeff King p...@peff.net
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
Changes since V3:
- Integrated Peffs suggestions in t5601
(Remove clear_ssh)
- Decide early if it is ssl or local in connect.c
- Use git fetch instead of git clone in t5601:
clone use absolute_path() (adding a / before :), fetch does not
- Add a test for dos_drive C:temp for msys
- Make tests work for msys by using $PWD instead of $(pwd) (file://$PWD)
connect.c| 50
connect.h| 1 +
t/t5601-clone.sh | 117 ---
transport.c | 8
4 files changed, 130 insertions(+), 46 deletions(-)
diff --git a/connect.c b/connect.c
index 06e88b0..022d122 100644
--- a/connect.c
+++ b/connect.c
@@ -547,6 +547,25 @@ static char *get_port(char *host)
static struct child_process no_fork;
+int is_local(const char *url)
+{
+ const char *colon = strchr(url, ':');
+ const char *slash = strchr(url, '/');
+ if (has_dos_drive_prefix(url))
+ return 1;
+ if (!colon)
+ return 1;
+ if (slash slash colon)
+ return 1;
+ if (url[0] == '[') {
+ const char *end = strchr(url + 1, ']');
+ if (!end)
+ return 1;
+ return is_local(end);
+ }
+ return 0;
+}
+
/*
* This returns a dummy child_process if the transport protocol does not
* need fork(2), or a struct child_process object if it does. Once done,
@@ -564,7 +583,7 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
char *url;
char *host, *path;
char *end;
- int c;
+ int separator;
struct child_process *conn = no_fork;
enum protocol protocol = PROTO_LOCAL;
int free_path = 0;
@@ -587,17 +606,19 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
*host = '\0';
protocol = get_protocol(url);
host += 3;
- c = '/';
+ separator = '/';
} else {
host = url;
- c = ':';
+ protocol = is_local(url) ? PROTO_LOCAL : PROTO_SSH;
+ separator = ':';
}
/*
* Don't do destructive transforms with git:// as that
* protocol code does '[]' unwrapping of its own.
+* Don't change local URLs
*/
- if (host[0] == '[') {
+ if (protocol != PROTO_LOCAL host[0] == '[') {
end = strchr(host + 1, ']');
if (end) {
if (protocol != PROTO_GIT) {
@@ -610,17 +631,14 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
} else
end = host;
- path = strchr(end, c);
- if (path !has_dos_drive_prefix(end)) {
- if (c == ':') {
- if (host != url || path strchrnul(host, '/')) {
- protocol = PROTO_SSH;
- *path++ = '\0';
- } else /* '/' in the host part, assume local path */
- path = end;
+ path = strchr(end, separator);
+ if (separator == ':') {
+ if (path protocol == PROTO_SSH) {
+ *path++ = '\0';
+ } else {/* assume local path */
+ path = end;
}
- } else
- path = end;
+ }
if (!path || !*path)
die(No path specified. See 'man git-pull' for valid url
syntax);
@@ -629,7 +647,7 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
* null-terminate hostname and point path to ~ for URL's like this:
*ssh
Re: htonll, ntohll
On 2013-10-30 22.07, Ramsay Jones wrote: On 30/10/13 20:30, Torsten Bögershausen wrote: On 2013-10-30 20.06, Ramsay Jones wrote: On 30/10/13 17:14, Torsten Bögershausen wrote: On 2013-10-30 18.01, Vicent Martí wrote: On Wed, Oct 30, 2013 at 5:51 PM, Torsten Bögershausen tbo...@web.de wrote: There is a name clash under cygwin 1.7 (1.5 is OK) The following first aid hot fix works for me: /Torsten If Cygwin declares its own bswap_64, wouldn't it be better to use it instead of overwriting it with our own? Yes, this will be part of a longer patch. I found that some systems have something like this: #define htobe64(x) bswap_64(x) And bswap_64 is a function, so we can not detect it by asking #ifdef bswap_64 .. #endif But we can use #ifdef htobe64 ... #endif and this will be part of a bigger patch. And, in general, we should avoid to introduce functions which may have a name clash. Using the git_ prefix for function names is a good practice. So in order to unbrake the compilation error under cygwin 17, the hotfix can be used. heh, my patch (given below) took a different approach, but ATB, Ramsay Jones -- 8 -- Subject: [PATCH] compat/bswap.h: Fix redefinition of bswap_64 error on cygwin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since commit 452e0f20 (compat: add endianness helpers, 24-10-2013) the cygwin build has failed like so: GIT_VERSION = 1.8.4.1.804.g1f3748b * new build flags CC credential-store.o In file included from git-compat-util.h:305:0, from cache.h:4, from credential-store.c:1: compat/bswap.h:67:24: error: redefinition of 'bswap_64' In file included from /usr/include/endian.h:32:0, from /usr/include/cygwin/types.h:21, from /usr/include/sys/types.h:473, from /usr/include/sys/unistd.h:9, from /usr/include/unistd.h:4, from git-compat-util.h:98, from cache.h:4, from credential-store.c:1: /usr/include/byteswap.h:31:1: note: previous definition of \ ‘bswap_64’ was here Makefile:1985: recipe for target 'credential-store.o' failed make: *** [credential-store.o] Error 1 Note that cygwin has a defintion of 'bswap_64' in the byteswap.h header file (which had already been included by git-compat-util.h). In order to suppress the error, ensure that the byteswap.h header is included, just like the __GNUC__/__GLIBC__ case, rather than attempting to define a fallback implementation. Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk --- compat/bswap.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compat/bswap.h b/compat/bswap.h index ea1a9ed..b864abd 100644 --- a/compat/bswap.h +++ b/compat/bswap.h @@ -61,7 +61,7 @@ static inline uint32_t git_bswap32(uint32_t x) # define ntohll(n) (n) # define htonll(n) (n) #elif __BYTE_ORDER == __LITTLE_ENDIAN -# if defined(__GNUC__) defined(__GLIBC__) +# if defined(__GNUC__) (defined(__GLIBC__) || defined(__CYGWIN__)) # include byteswap.h # else /* GNUC GLIBC */ static inline uint64_t bswap_64(uint64_t val) Nice, much better. And here comes a patch for a big endian machine. I tryied to copy-paste a patch in my mail program, not sure if this applies. -- 8 -- Subject: [PATCH] compat/bswap.h: htonll and ntohll for big endian Since commit 452e0f20 (compat: add endianness helpers, 24-10-2013) the build on a Linux/ppc gave a warning like this: CC ewah/ewah_io.o ewah/ewah_io.c: In function ‘ewah_serialize_to’: ewah/ewah_io.c:81: warning: implicit declaration of function ‘htonll’ ewah/ewah_io.c: In function ‘ewah_read_mmap’: ewah/ewah_io.c:132: warning: implicit declaration of function ‘ntohll’ Fix it by placing the #endif for #ifdef bswap32 at the right place. Signed-off-by: Torsten Bögershausen tbo...@web.de --- compat/bswap.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compat/bswap.h b/compat/bswap.h index ea1a9ed..b4ddab0 --- a/compat/bswap.h +++ b/compat/bswap.h @@ -46,6 +46,7 @@ static inline uint32_t git_bswap32(uint32_t x) #undef htonl #define ntohl(x) bswap32(x) #define htonl(x) bswap32(x) +#endif #ifndef __BYTE_ORDER # if defined(BYTE_ORDER) defined(LITTLE_ENDIAN) defined(BIG_ENDIAN) @@ -82,4 +83,3 @@ static inline uint64_t bswap_64(uint64_t val) # error Can't define htonll or ntohll! #endif -#endif . Yep, this was the first thing I did as well! ;-) (*late* last night) I haven't had time today to look into fixing up the msvc build (or a complete re-write), so I look forward to seeing your solution. (do you have msvc available? - or do you want me to look at fixing it? maybe in a day or two?) ATB, Ramsay Jones Ramsay, I don't have msvc, so feel
[PATCH] Test code for htonll, ntohll
test-endianess.c adds test code for htonl(), ntohll()
and the recently introduced ntohll() and htonll()
The test is called in t0070
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
.gitignore |1 +
Makefile |3 +++
t/t0070-fundamental.sh |3 +++
test-endianess.c | 58
4 files changed, 65 insertions(+)
create mode 100644 test-endianess.c
diff --git a/.gitignore b/.gitignore
index 66199ed..750e7d8 100644
--- a/.gitignore
+++ b/.gitignore
@@ -183,6 +183,7 @@
/test-date
/test-delta
/test-dump-cache-tree
+/test-endianess
/test-scrap-cache-tree
/test-genrandom
/test-index-version
diff --git a/Makefile b/Makefile
index 07b0626..50957c7 100644
--- a/Makefile
+++ b/Makefile
@@ -555,6 +555,7 @@ TEST_PROGRAMS_NEED_X += test-ctype
TEST_PROGRAMS_NEED_X += test-date
TEST_PROGRAMS_NEED_X += test-delta
TEST_PROGRAMS_NEED_X += test-dump-cache-tree
+TEST_PROGRAMS_NEED_X += test-endianess
TEST_PROGRAMS_NEED_X += test-genrandom
TEST_PROGRAMS_NEED_X += test-hashmap
TEST_PROGRAMS_NEED_X += test-index-version
@@ -2275,6 +2276,8 @@ test-date$X: date.o ctype.o
test-delta$X: diff-delta.o patch-delta.o
+test-endianess$X: test-endianess.c
+
test-line-buffer$X: vcs-svn/lib.a
test-parse-options$X: parse-options.o parse-options-cb.o
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 5ed69a6..dc687da 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -33,5 +33,8 @@ test_expect_success 'check for a bug in the regex routines' '
# if this test fails, re-build git with NO_REGEX=1
test-regex
'
+test_expect_success 'test endianess, htonll(), ntohll()' '
+ test-endianess
+'
test_done
diff --git a/test-endianess.c b/test-endianess.c
new file mode 100644
index 000..5b49175
--- /dev/null
+++ b/test-endianess.c
@@ -0,0 +1,58 @@
+#include cache.h
+
+int main(int argc, char **argv)
+{
+ union {
+ uint8_t bytes[8];
+ uint32_t word32;
+ uint64_t word64;
+ } wordll;
+ volatile uint64_t word64;
+ volatile uint32_t word32;
+
+ wordll.bytes[0] = 0x80;
+ wordll.bytes[1] = 0x81;
+ wordll.bytes[2] = 0x82;
+ wordll.bytes[3] = 0x83;
+ wordll.bytes[4] = 0x00;
+ wordll.bytes[5] = 0x01;
+ wordll.bytes[6] = 0x02;
+ wordll.bytes[7] = 0x03;
+
+ word32 = htonl(wordll.word32);
+ if (word32 != 0x80818283)
+ die(htonl word32 != 0x80818283);
+
+ word64 = htonll(wordll.word64);
+ if (word64 != 0x8081828300010203ULL)
+ die(htonll word64 != 0x8081828300010203);
+
+ word32 = ntohl(wordll.word32);
+ if (word32 != 0x80818283)
+ die(ntohl word32 != 0x80818283);
+
+ word64 = ntohll(wordll.word64);
+ if (word64 != 0x8081828300010203ULL)
+ die(ntohll word64 != 0x8081828300010203);
+
+ wordll.bytes[0] = 0x04;
+ wordll.bytes[4] = 0x84;
+
+ word32 = htonl(wordll.word32);
+ if (word32 != 0x04818283)
+ die(htonl word32 != 0x04818283);
+
+ word64 = htonll(wordll.word64);
+ if (word64 != 0x0481828384010203ULL)
+ die(htonll word64 != 0x0481828384010203);
+
+ word32 = ntohl(wordll.word32);
+ if (word32 != 0x04818283)
+ die(ntohl word32 != 0x04818283);
+
+ word64 = ntohll(wordll.word64);
+ if (word64 != 0x0481828384010203ULL)
+ die(ntohll word64 != 0x0481828384010203);
+
+ return 0;
+}
--
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: What's cooking in git.git (Oct 2013, #07; Mon, 28)
On 2013-10-28 20.28, Junio C Hamano wrote:
* jk/pack-bitmap (2013-10-28) 20 commits
There is a name clash under cygwin 1.7 (1.5 is OK)
The following first aid hot fix works for me:
/Torsten
$ git diff
diff --git a/compat/bswap.h b/compat/bswap.h
index ea1a9ed..8dc39be 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -64,7 +64,7 @@ static inline uint32_t git_bswap32(uint32_t x)
# if defined(__GNUC__) defined(__GLIBC__)
# include byteswap.h
# else /* GNUC GLIBC */
-static inline uint64_t bswap_64(uint64_t val)
+static inline uint64_t git_bswap_64(uint64_t val)
{
return ((val (uint64_t)0x00ffULL) 56)
| ((val (uint64_t)0xff00ULL) 40)
@@ -76,8 +76,8 @@ static inline uint64_t bswap_64(uint64_t val)
| ((val (uint64_t)0xff00ULL) 56);
}
# endif /* GNUC GLIBC */
-# define ntohll(n) bswap_64(n)
-# define htonll(n) bswap_64(n)
+# define ntohll(n) git_bswap_64(n)
+# define htonll(n) git_bswap_64(n)
#else /* __BYTE_ORDER */
# error Can't define htonll or ntohll!
#endif
--
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: What's cooking in git.git (Oct 2013, #07; Mon, 28)
On 2013-10-30 18.01, Vicent Martí wrote: On Wed, Oct 30, 2013 at 5:51 PM, Torsten Bögershausen tbo...@web.de wrote: There is a name clash under cygwin 1.7 (1.5 is OK) The following first aid hot fix works for me: /Torsten If Cygwin declares its own bswap_64, wouldn't it be better to use it instead of overwriting it with our own? Yes, this will be part of a longer patch. I found that some systems have something like this: #define htobe64(x) bswap_64(x) And bswap_64 is a function, so we can not detect it by asking #ifdef bswap_64 .. #endif But we can use #ifdef htobe64 ... #endif and this will be part of a bigger patch. And, in general, we should avoid to introduce functions which may have a name clash. Using the git_ prefix for function names is a good practice. So in order to unbrake the compilation error under cygwin 17, the hotfix can be used. /Torsten -- 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: What's cooking in git.git (Oct 2013, #07; Mon, 28)
On 2013-10-30 18.14, Torsten Bögershausen wrote: On 2013-10-30 18.01, Vicent Martí wrote: On Wed, Oct 30, 2013 at 5:51 PM, Torsten Bögershausen tbo...@web.de wrote: There is a name clash under cygwin 1.7 (1.5 is OK) The following first aid hot fix works for me: /Torsten If Cygwin declares its own bswap_64, wouldn't it be better to use it instead of overwriting it with our own? Yes, this will be part of a longer patch. I found that some systems have something like this: #define htobe64(x) bswap_64(x) And bswap_64 is a function, so we can not detect it by asking #ifdef bswap_64 .. #endif But we can use #ifdef htobe64 ... #endif and this will be part of a bigger patch. And, in general, we should avoid to introduce functions which may have a name clash. Using the git_ prefix for function names is a good practice. So in order to unbrake the compilation error under cygwin 17, the hotfix can be used. /Torsten I just realized that there seem to problems to compile pu under msysgit. More investigation needed here. -- 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: What's cooking in git.git (Oct 2013, #07; Mon, 28)
On 2013-10-30 20.06, Ramsay Jones wrote: On 30/10/13 17:14, Torsten Bögershausen wrote: On 2013-10-30 18.01, Vicent Martí wrote: On Wed, Oct 30, 2013 at 5:51 PM, Torsten Bögershausen tbo...@web.de wrote: There is a name clash under cygwin 1.7 (1.5 is OK) The following first aid hot fix works for me: /Torsten If Cygwin declares its own bswap_64, wouldn't it be better to use it instead of overwriting it with our own? Yes, this will be part of a longer patch. I found that some systems have something like this: #define htobe64(x) bswap_64(x) And bswap_64 is a function, so we can not detect it by asking #ifdef bswap_64 .. #endif But we can use #ifdef htobe64 ... #endif and this will be part of a bigger patch. And, in general, we should avoid to introduce functions which may have a name clash. Using the git_ prefix for function names is a good practice. So in order to unbrake the compilation error under cygwin 17, the hotfix can be used. heh, my patch (given below) took a different approach, but ATB, Ramsay Jones -- 8 -- Subject: [PATCH] compat/bswap.h: Fix redefinition of bswap_64 error on cygwin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since commit 452e0f20 (compat: add endianness helpers, 24-10-2013) the cygwin build has failed like so: GIT_VERSION = 1.8.4.1.804.g1f3748b * new build flags CC credential-store.o In file included from git-compat-util.h:305:0, from cache.h:4, from credential-store.c:1: compat/bswap.h:67:24: error: redefinition of 'bswap_64' In file included from /usr/include/endian.h:32:0, from /usr/include/cygwin/types.h:21, from /usr/include/sys/types.h:473, from /usr/include/sys/unistd.h:9, from /usr/include/unistd.h:4, from git-compat-util.h:98, from cache.h:4, from credential-store.c:1: /usr/include/byteswap.h:31:1: note: previous definition of \ ‘bswap_64’ was here Makefile:1985: recipe for target 'credential-store.o' failed make: *** [credential-store.o] Error 1 Note that cygwin has a defintion of 'bswap_64' in the byteswap.h header file (which had already been included by git-compat-util.h). In order to suppress the error, ensure that the byteswap.h header is included, just like the __GNUC__/__GLIBC__ case, rather than attempting to define a fallback implementation. Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk --- compat/bswap.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compat/bswap.h b/compat/bswap.h index ea1a9ed..b864abd 100644 --- a/compat/bswap.h +++ b/compat/bswap.h @@ -61,7 +61,7 @@ static inline uint32_t git_bswap32(uint32_t x) # define ntohll(n) (n) # define htonll(n) (n) #elif __BYTE_ORDER == __LITTLE_ENDIAN -#if defined(__GNUC__) defined(__GLIBC__) +#if defined(__GNUC__) (defined(__GLIBC__) || defined(__CYGWIN__)) #include byteswap.h #else /* GNUC GLIBC */ static inline uint64_t bswap_64(uint64_t val) Nice, much better. And here comes a patch for a big endian machine. I tryied to copy-paste a patch in my mail program, not sure if this applies. -- 8 -- Subject: [PATCH] compat/bswap.h: htonll and ntohll for big endian Since commit 452e0f20 (compat: add endianness helpers, 24-10-2013) the build on a Linux/ppc gave a warning like this: CC ewah/ewah_io.o ewah/ewah_io.c: In function ‘ewah_serialize_to’: ewah/ewah_io.c:81: warning: implicit declaration of function ‘htonll’ ewah/ewah_io.c: In function ‘ewah_read_mmap’: ewah/ewah_io.c:132: warning: implicit declaration of function ‘ntohll’ Fix it by placing the #endif for #ifdef bswap32 at the right place. Signed-off-by: Torsten Bögershausen tbo...@web.de --- compat/bswap.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compat/bswap.h b/compat/bswap.h index ea1a9ed..b4ddab0 --- a/compat/bswap.h +++ b/compat/bswap.h @@ -46,6 +46,7 @@ static inline uint32_t git_bswap32(uint32_t x) #undef htonl #define ntohl(x) bswap32(x) #define htonl(x) bswap32(x) +#endif #ifndef __BYTE_ORDER # if defined(BYTE_ORDER) defined(LITTLE_ENDIAN) defined(BIG_ENDIAN) @@ -82,4 +83,3 @@ static inline uint64_t bswap_64(uint64_t val) # error Can't define htonll or ntohll! #endif -#endif -- 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] git clone: is an URL local or ssh
Commit 8d3d28f5 added test cases for URLs which should be ssh.
Add more tests testing all the combinations:
-IPv4 or IPv6
-path starting with / or with /~
-with and without the ssh:// scheme
Add tests for ssh:// with port number.
When a git repository foo:bar exist, git clone will call
absolute_path() and git_connect() will be called with
something like /home/user/projects/foo:bar
Tighten the test and use foo:bar instead of ./foo:bar,
refactor clear_ssh() and expect_ssh() into test_clone_url().
git clone foo/bar:baz should not be ssh:
Make the rule
if there is a slash before the first colon, it is not ssh
more strict by using the same is_local() in both connect.c
and transport.c. Add a test case.
Bug fixes for corner cases:
- Handle clone of [::1]:/~repo correctly (2e7766655):
Git should call ssh ::1 ~repo, not ssh ::1 /~repo
This was caused by looking at (host != url), which never
worked for host names with literal IPv6 adresses, embedded by []
Support for the [] URLs was introduced in 356bece0a.
- Do not tamper local URLs starting with '[' which have a ']'
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
(Thanks for the reviews)
Changes since V2:
clear_ssh and expect_ssh did come back
And I couldn't get it working without the
$TRASH_DIRECTORY/ssh-output
test_when_finished:
I could not get that to work. Probably because of the
battle between the quotings: '' ' ''
Other note about test_might_fail:
The first version did not need it, git clone did
always succeed.
After a while it always failed.
According to my understanding, git clone ssh://xxx.yy
should fail (as we can not clone) ??
But it seems to be a timing problem, anybody wants to comment
on this ?
connect.c| 47
connect.h| 1 +
t/t5601-clone.sh | 81 +---
transport.c | 8 --
4 files changed, 102 insertions(+), 35 deletions(-)
diff --git a/connect.c b/connect.c
index 06e88b0..d61adc9 100644
--- a/connect.c
+++ b/connect.c
@@ -231,6 +231,7 @@ int server_supports(const char *feature)
}
enum protocol {
+ PROTO_LOCAL_OR_SSH = 0,
PROTO_LOCAL = 1,
PROTO_SSH,
PROTO_GIT
@@ -547,6 +548,15 @@ static char *get_port(char *host)
static struct child_process no_fork;
+int is_local(const char *url)
+{
+ const char *colon = strchr(url, ':');
+ const char *slash = strchr(url, '/');
+ return !colon || (slash slash colon) ||
+ has_dos_drive_prefix(url);
+}
+
+
/*
* This returns a dummy child_process if the transport protocol does not
* need fork(2), or a struct child_process object if it does. Once done,
@@ -564,9 +574,9 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
char *url;
char *host, *path;
char *end;
- int c;
+ int separator;
struct child_process *conn = no_fork;
- enum protocol protocol = PROTO_LOCAL;
+ enum protocol protocol = PROTO_LOCAL_OR_SSH;
int free_path = 0;
char *port = NULL;
const char **arg;
@@ -587,20 +597,23 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
*host = '\0';
protocol = get_protocol(url);
host += 3;
- c = '/';
+ separator = '/';
} else {
host = url;
- c = ':';
+ separator = ':';
+ if (is_local(url))
+ protocol = PROTO_LOCAL;
}
/*
* Don't do destructive transforms with git:// as that
* protocol code does '[]' unwrapping of its own.
+* Don't change local URLs
*/
if (host[0] == '[') {
end = strchr(host + 1, ']');
if (end) {
- if (protocol != PROTO_GIT) {
+ if (protocol != PROTO_GIT protocol != PROTO_LOCAL) {
*end = 0;
host++;
}
@@ -610,17 +623,17 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
} else
end = host;
- path = strchr(end, c);
- if (path !has_dos_drive_prefix(end)) {
- if (c == ':') {
- if (host != url || path strchrnul(host, '/')) {
- protocol = PROTO_SSH;
- *path++ = '\0';
- } else /* '/' in the host part, assume local path */
- path = end;
+ path = strchr(end, separator);
+ if (separator == ':') {
+ if (path protocol == PROTO_LOCAL_OR_SSH) {
+ /* We have a ':' */
+ protocol = PROTO_SSH;
+ *path++ = '\0';
+ } else {/* assume local path */
+ protocol
[PATCH V2] git clone: is an URL local or ssh
Commit 8d3d28f5 added test cases for URLs which should be ssh.
Add more tests testing all the combinations:
-IPv4 or IPv6
-path starting with / or with /~
-with and without the ssh:// scheme
Add tests for ssh:// with port number.
When a git repository foo:bar exist, git clone will call
absolute_path() and git_connect() will be called with
something like /home/user/projects/foo:bar
Tighten the test and use foo:bar instead of ./foo:bar,
refactor clear_ssh() and expect_ssh() into test_clone_url().
git clone foo/bar:baz should not be ssh:
Make the rule
if there is a slash before the first colon, it is not ssh
more strict by using the same is_local() in both connect.c
and transport.c. Add a test case.
Bug fixes for corner cases:
- Handle clone of [::1]:/~repo correctly (2e7766655):
Git should call ssh ::1 ~repo, not ssh ::1 /~repo
This was caused by looking at (host != url), which never
worked for host names with literal IPv6 adresses, embedded by []
Support for the [] URLs was introduced in 356bece0a.
- Do not tamper local URLs starting with '[' which have a ']'
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
(This does apply on pu, not on master.
I'm almost sure there are more corner cases, but the
most important things should be covered)
Changes since V1: Comments from Eric Sunshine (thanks)
connect.c| 47 +--
connect.h| 1 +
t/t5601-clone.sh | 98
transport.c | 8 -
4 files changed, 108 insertions(+), 46 deletions(-)
diff --git a/connect.c b/connect.c
index 06e88b0..d61adc9 100644
--- a/connect.c
+++ b/connect.c
@@ -231,6 +231,7 @@ int server_supports(const char *feature)
}
enum protocol {
+ PROTO_LOCAL_OR_SSH = 0,
PROTO_LOCAL = 1,
PROTO_SSH,
PROTO_GIT
@@ -547,6 +548,15 @@ static char *get_port(char *host)
static struct child_process no_fork;
+int is_local(const char *url)
+{
+ const char *colon = strchr(url, ':');
+ const char *slash = strchr(url, '/');
+ return !colon || (slash slash colon) ||
+ has_dos_drive_prefix(url);
+}
+
+
/*
* This returns a dummy child_process if the transport protocol does not
* need fork(2), or a struct child_process object if it does. Once done,
@@ -564,9 +574,9 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
char *url;
char *host, *path;
char *end;
- int c;
+ int separator;
struct child_process *conn = no_fork;
- enum protocol protocol = PROTO_LOCAL;
+ enum protocol protocol = PROTO_LOCAL_OR_SSH;
int free_path = 0;
char *port = NULL;
const char **arg;
@@ -587,20 +597,23 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
*host = '\0';
protocol = get_protocol(url);
host += 3;
- c = '/';
+ separator = '/';
} else {
host = url;
- c = ':';
+ separator = ':';
+ if (is_local(url))
+ protocol = PROTO_LOCAL;
}
/*
* Don't do destructive transforms with git:// as that
* protocol code does '[]' unwrapping of its own.
+* Don't change local URLs
*/
if (host[0] == '[') {
end = strchr(host + 1, ']');
if (end) {
- if (protocol != PROTO_GIT) {
+ if (protocol != PROTO_GIT protocol != PROTO_LOCAL) {
*end = 0;
host++;
}
@@ -610,17 +623,17 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
} else
end = host;
- path = strchr(end, c);
- if (path !has_dos_drive_prefix(end)) {
- if (c == ':') {
- if (host != url || path strchrnul(host, '/')) {
- protocol = PROTO_SSH;
- *path++ = '\0';
- } else /* '/' in the host part, assume local path */
- path = end;
+ path = strchr(end, separator);
+ if (separator == ':') {
+ if (path protocol == PROTO_LOCAL_OR_SSH) {
+ /* We have a ':' */
+ protocol = PROTO_SSH;
+ *path++ = '\0';
+ } else {/* assume local path */
+ protocol = PROTO_LOCAL;
+ path = end;
}
- } else
- path = end;
+ }
if (!path || !*path)
die(No path specified. See 'man git-pull' for valid url
syntax);
@@ -629,7 +642,7 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
* null-terminate hostname and point path to ~ for URL's like
[PATCH/RFC] git clone: is an URL local or ssh
Commit 8d3d28f5 added test cases for URLs which should be ssh.
Add more tests testing all the combinations:
-IPv4 or IPv6
-path starting with / or with /~
-with and without the ssh:// scheme
Add tests for ssh:// with port number.
When a git repository foo:bar exist, git clone will call
absolute_path() and git_connect() will be called with
something like /home/user/projects/foo:bar
Tighten the test and use foo:bar instead of ./foo:bar,
refactor clear_ssh() and expect_ssh() into test_clone_url().
git clone foo/bar:baz should not be ssh:
Make the rule
if there is a slash before the first colon, it is not ssh
more strict by using the same is_local() in both connect.c
and transport.c. Add a test case.
Bug fixes for corner cases:
- Handle clone of [::1]:/~repo correctly (2e7766655):
Git should call ssh ::1 ~repo, not ssh ::1 /~repo
This was caused by looking at (host != url), which never
worked for host names with literal IPv6 adresses, embedded by []
Support for the [] URLs was introduced in 356bece0a.
- Do not tamper local URLs starting with '[' which have a ']'
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
(This does apply on pu, not on master.
I'm almost sure there are more corner cases, but the
most important things should be covered)
connect.c| 47 +--
connect.h| 1 +
t/t5601-clone.sh | 96 +++-
transport.c | 8 -
4 files changed, 106 insertions(+), 46 deletions(-)
diff --git a/connect.c b/connect.c
index 06e88b0..903063e 100644
--- a/connect.c
+++ b/connect.c
@@ -231,6 +231,7 @@ int server_supports(const char *feature)
}
enum protocol {
+ PROTO_LOCAL_OR_SSH = 0,
PROTO_LOCAL = 1,
PROTO_SSH,
PROTO_GIT
@@ -547,6 +548,15 @@ static char *get_port(char *host)
static struct child_process no_fork;
+int is_local(const char *url)
+{
+ const char *colon = strchr(url, ':');
+ const char *slash = strchr(url, '/');
+ return !colon || (slash slash colon) ||
+ has_dos_drive_prefix(url);
+}
+
+
/*
* This returns a dummy child_process if the transport protocol does not
* need fork(2), or a struct child_process object if it does. Once done,
@@ -564,9 +574,9 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
char *url;
char *host, *path;
char *end;
- int c;
+ int seperator;
struct child_process *conn = no_fork;
- enum protocol protocol = PROTO_LOCAL;
+ enum protocol protocol = PROTO_LOCAL_OR_SSH;
int free_path = 0;
char *port = NULL;
const char **arg;
@@ -587,20 +597,23 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
*host = '\0';
protocol = get_protocol(url);
host += 3;
- c = '/';
+ seperator = '/';
} else {
host = url;
- c = ':';
+ seperator = ':';
+ if (is_local(url))
+ protocol = PROTO_LOCAL;
}
/*
* Don't do destructive transforms with git:// as that
* protocol code does '[]' unwrapping of its own.
+* Don't change local URLs
*/
if (host[0] == '[') {
end = strchr(host + 1, ']');
if (end) {
- if (protocol != PROTO_GIT) {
+ if (protocol != PROTO_GIT protocol != PROTO_LOCAL) {
*end = 0;
host++;
}
@@ -610,17 +623,17 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
} else
end = host;
- path = strchr(end, c);
- if (path !has_dos_drive_prefix(end)) {
- if (c == ':') {
- if (host != url || path strchrnul(host, '/')) {
- protocol = PROTO_SSH;
- *path++ = '\0';
- } else /* '/' in the host part, assume local path */
- path = end;
+ path = strchr(end, seperator);
+ if (seperator == ':') {
+ if (path protocol == PROTO_LOCAL_OR_SSH) {
+ /* We have a ':' */
+ protocol = PROTO_SSH;
+ *path++ = '\0';
+ } else {/* assume local path */
+ protocol = PROTO_LOCAL;
+ path = end;
}
- } else
- path = end;
+ }
if (!path || !*path)
die(No path specified. See 'man git-pull' for valid url
syntax);
@@ -629,7 +642,7 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
* null-terminate hostname and point path to ~ for URL's like this:
*ssh://host.xz/~user/repo
Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big
On 20.10.13 08:05, Ondřej Bílka wrote:
On Sun, Oct 20, 2013 at 07:47:06AM +0200, Torsten Bögershausen wrote:
(may be s/path is too big/path is too long/ ?)
On 19.10.13 12:52, Antoine Pelisse wrote:
Currently, most buffers created with PATH_MAX length, are not checked
when being written, and can overflow if PATH_MAX is not big enough to
hold the path.
Fix that by using strlcpy() where strcpy() was used, and also run some
extra checks when copy is done with memcpy().
Reported-by: Wataru Noguchi wnoguchi.0...@gmail.com
Signed-off-by: Antoine Pelisse apeli...@gmail.com
---
diff --git a/abspath.c b/abspath.c
index 64adbe2..0e60ba4 100644
--- a/abspath.c
+++ b/abspath.c
@@ -216,11 +216,15 @@ const char *absolute_path(const char *path)
const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
{
static char path[PATH_MAX];
Why do you need static there?
Good point.
get_pathname() from path.c may be better.
+
+ if (pfx_len PATH_MAX)
I think this should be
if (pfx_len PATH_MAX-1) /* Keep 1 char for '\0'
+ die(Too long prefix path: %s, pfx);
+
#ifndef GIT_WINDOWS_NATIVE
if (!pfx_len || is_absolute_path(arg))
return arg;
memcpy(path, pfx, pfx_len);
- strcpy(path + pfx_len, arg);
+ strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len);
I'm not sure how to handle overlong path in general, there are several ways:
a) Silently overwrite memory (with help of memcpy() and/or strcpy()
b) Silently shorten the path using strlcpy() instead of strcpy()
c) Avoid the overwriting and call die().
d) Prepare a longer buffer using xmalloc()
There is also
e) modify allocation to place write protected page after buffer end.
Yes, I think this is what electric fence, DUMA or valgrind do:
http://sourceforge.jp/projects/freshmeat_efence/
http://duma.sourceforge.net/
http://valgrind.sourceforge.net/
Theses are very good tools for developers, finding memory corruption
(or other bugs like using uninitialized memory).
One of the motivation I asked for test cases is that a git developer can
run these test cases under valgrind and can verify that we are never out of
range.
For an end user a git crash caused by trying to write to a write protected
page
is better than silently corrupting memory.
And a range check, followed by die(), is even easier to debug.
For an end user.
/Torsten
--
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: [msysGit] [PATCH] Prevent buffer overflows when path is too big
(may be s/path is too big/path is too long/ ?)
On 19.10.13 12:52, Antoine Pelisse wrote:
Currently, most buffers created with PATH_MAX length, are not checked
when being written, and can overflow if PATH_MAX is not big enough to
hold the path.
Fix that by using strlcpy() where strcpy() was used, and also run some
extra checks when copy is done with memcpy().
Reported-by: Wataru Noguchi wnoguchi.0...@gmail.com
Signed-off-by: Antoine Pelisse apeli...@gmail.com
---
abspath.c| 10 +++---
diffcore-order.c | 2 +-
entry.c | 14 ++
unpack-trees.c | 2 ++
4 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/abspath.c b/abspath.c
index 64adbe2..0e60ba4 100644
--- a/abspath.c
+++ b/abspath.c
@@ -216,11 +216,15 @@ const char *absolute_path(const char *path)
const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
{
static char path[PATH_MAX];
+
+ if (pfx_len PATH_MAX)
I think this should be
if (pfx_len PATH_MAX-1) /* Keep 1 char for '\0'
+ die(Too long prefix path: %s, pfx);
+
#ifndef GIT_WINDOWS_NATIVE
if (!pfx_len || is_absolute_path(arg))
return arg;
memcpy(path, pfx, pfx_len);
- strcpy(path + pfx_len, arg);
+ strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len);
I'm not sure how to handle overlong path in general, there are several ways:
a) Silently overwrite memory (with help of memcpy() and/or strcpy()
b) Silently shorten the path using strlcpy() instead of strcpy()
c) Avoid the overwriting and call die().
d) Prepare a longer buffer using xmalloc()
Today we do a), this is not a good thing and the worst choice.
A little side note:
It would be good to have test cases for either b), c) or d).
As PATH_MAX is OS dependend, we need both a main program written in c
and a test case written in t/t.sh.
Some existing code can be used for inspiration, e.g.
test-wildmatch.c in combination with t/t3070-wildmatch.sh
This willl allow us to reproduce the error, and define how git should behave.
End of the side note, let's look closer at the suggested patch, implementing b)
Silently shortening an overlong path like
/foo/bar/baz could result something like
/foo/bar/ba /* That filename may be part of the repo too */
or
/foo/bar/ /* This is a directory, not a file name */
In either case the end user has no idea why git choose another file name.
And this could be hard to debug.
After a couple of hours she/he may send a message asking for help to the
mailing list,
and we end up in more people doing debugging.
c) Is much easier to debug:
Git can not handle this situation, and we print out the parameters in die()
I would prefer c) over b), make clear that git can't handle that situation.
d) Would mean some more re-factoring: Check all callers to prefix_filename().
Some of them call xstrdup() after prefix_filename(), which mean that we could
change prefix_filename() to always return new string which is long enough via
xmalloc(),
and not a static buffer.
So we come to the next point (and this is my personal experience,
so please don't get me wrong):
how much time can you spend on this?
If the answer is kind of very little, I would go for c)
Avoid the silent memory corruption, and say to the user we can not handle
this
If the answer is kind of little, I would go for c) and a test program,
covering all the different code path in abspath()
(WHich may deserve a refactoring as well, since the code for
GIT_WINDOWS_NATIVE
is very similar to the non-GIT_WINDOWS_NATIVE)
If the answer is kind of more than little, a different strategie may be
better:
Start sending a patch for c)
I think we have enough volunteers here for a review, so we can life without
the test code.
On top of that, some volunteer can develop d).
So far I have only looked at abspath(), and your patch touches more places.
I think more and more that calling die()
with all information included why we call die() is a good starting point.
It will allow the users to see what is going on.
May be the repo can be re-arranged to use shorter path names than what we can
handle.
[snip]
/Torsten
--
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: Git send-email fail on Mac OS X Lion
(Please, not top-posting) On Oct 12, 2013, at 8:47 AM, brian m. carlson sand...@crustytoothpaste.net wrote: On Fri, Oct 11, 2013 at 11:06:17PM -0500, Fernando Ortiz (e2k) wrote: I'm getting the following error when I do: git send-email --compose --from Fernando Ortiz eratos2...@gmail.com --to forti...@gmail.com --cc forti...@gmail.com 0001-Change-zcat-to-gzcat-to-fix-build-restore-steps.patch Net::SSLeay version 1.46 required--this is only version 1.36 at /Users/fortiz/perl5/perlbrew/perls/perl-5.14.4/lib/site_perl/5.14.4/IO/Socket/SSL.pm line 17. Here's your answer: Net::SSLeay is too old for IO::Socket::SSL. You either need to use cpan or cpanm to install a newer Net::SSLeay, and then it will work. On 2013-10-12 19.40, Gmail wrote: Brian, I already tried to reinstall with cpan/m using -f -i options. I even removed the PERL5LIB location and reinstalled the packages from scratch to no avail. Nando Sent from my iPhone This may be a stupid question: Which perl is in your $PATH ? What do you get entering type perl on the command line ? /Torsten -- 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] clone: local URLs are not for ssh
On 05.10.13 21:48, Torsten Bögershausen wrote:
On 2013-10-03 03.31, Jeff King wrote:
http://article.gmane.org/gmane.comp.version-control.git/235473
What do we think about extending the test a little bit:
git diff 771cf1dab9303bab3c8198b8b6 -- t5602-clone-remote-exec.sh
diff --git a/t/t5602-clone-remote-exec.sh b/t/t5602-clone-remote-exec.sh
index 3f353d9..790896f 100755
--- a/t/t5602-clone-remote-exec.sh
+++ b/t/t5602-clone-remote-exec.sh
@@ -30,5 +30,124 @@ test_expect_success 'clone calls specified git upload-pack
with -u option' '
echo localhost ./something/bin/git-upload-pack '\''/path/to/repo'\''
expected
test_cmp expected not_ssh_output
'
+test_expect_success 'setup ssh wrapper' '
+ write_script $TRASH_DIRECTORY/ssh-wrapper -\EOF
+ echo $TRASH_DIRECTORY/ssh-output ssh: $*
+ # throw away all but the last argument, which should be the
+ # command
+ while test $# -gt 1; do shift; done
+ eval $1
+ EOF
+
+ GIT_SSH=$TRASH_DIRECTORY/ssh-wrapper
+ export GIT_SSH
+ export TRASH_DIRECTORY
+'
+
+clear_ssh () {
+ $TRASH_DIRECTORY/ssh-output
+}
+
+expect_ssh () {
+ {
+ case $1 in
+ none)
+ ;;
+ *)
+ echo ssh: $1 git-upload-pack '$2'
+ esac
+ } $TRASH_DIRECTORY/ssh-expect
+ (cd $TRASH_DIRECTORY test_cmp ssh-expect ssh-output)
+}
+
+test_expect_success 'create src.git' '
+ mkdir src.git
+ (
+ cd src.git
+ git init
+ file
+ git add file
+ git commit -m add file
+ )
+'
+
+# git clone could fail, so break the chain and ignore the exit code
+# clone local
+test_expect_success './foo:bar is not ssh' '
+ clear_ssh
+ git clone ./foo:bar foobar
+ expect_ssh none
+'
+
+test_expect_success './[nohost:123]:src is not ssh' '
+ clear_ssh
+ git clone ./[nohost:123]:src 1_2_3
+ expect_ssh none
+'
+
+test_expect_success '[nohost:234] is not ssh' '
+ clear_ssh
+ git clone [nohost:234] 2_3_4
+ expect_ssh none
+'
+
+test_expect_success ':345 is not ssh' '
+ clear_ssh
+ git clone :345 3_4_5
+ expect_ssh none
+'
+
+test_expect_success '456: is not ssh' '
+ clear_ssh
+ git clone 456: 4_5_6
+ expect_ssh none
+'
+
+# clone via ssh
+# the expect_ssh checks that git clone tried to use ssh
+test_expect_success 'myhost:567 is ssh' '
+ clear_ssh
+ git clone myhost:567 myhost_567
+ expect_ssh myhost 567
+'
+
+test_expect_success '[myhost:678]:src is ssh' '
+ clear_ssh
+ git clone [myhost:678]:src myhost_678
+ expect_ssh myhost:678 src
+'
+
+#clone url looks like ssh, but is on disk
+test_expect_success SYMLINKS 'dir:123 on disk' '
+ clear_ssh
+ ln -s src.git dir:123
+ git clone dir:123 dir_123
+ expect_ssh none
+'
+
+test_expect_success SYMLINKS '[dir:234]:src on disk' '
+ clear_ssh
+ ln -s src.git [dir:234]:src
+ git clone [dir:234]:src dir_234_src
+ expect_ssh none
+'
+
+test_expect_success 'ssh://host.xz/~user/repo' '
+ clear_ssh
+ git clone ssh://host.xz/~user/repo user-repo
+ expect_ssh host.xz ~user/repo
+'
+
+test_expect_success 'ssh://host.xz:22/~user/repo' '
+ clear_ssh
+ git clone ssh://host.xz:22/~user/repo user-repo
+ expect_ssh -p 22 host.xz ~user/repo
+'
+
+test_expect_success 'ssh://[::1]:22/~user/repo' '
+ clear_ssh
+ git clone ssh://[::1]:22/~user/repo user-repo6
+ expect_ssh -p 22 ::1 ~user/repo
+'
test_done
==
And we need this on top of Duys patch:
diff --git a/connect.c b/connect.c
index e8473f3..09be2b3 100644
--- a/connect.c
+++ b/connect.c
@@ -611,7 +611,7 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
end = host;
path = strchr(end, c);
- if (path !has_dos_drive_prefix(end)) {
+ if (path host != path !has_dos_drive_prefix(end)) {
if (c == ':') {
if (host != url || path strchrnul(host, '/')) {
protocol = PROTO_SSH;
--
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] clone: local URLs are not for ssh
On 2013-09-29 02.33, Duy Nguyen wrote: On Sun, Sep 29, 2013 at 2:37 AM, Torsten Bögershausen tbo...@web.de wrote: git clone /foo/bar:baz or git clone ../foo/bar:baz are meant to clone from the local file system, and not to clone from a remote server over git-over-ssh. I don't think this is necessary. Commit 6000334 should detect both cases fine because both have a slash before the first colon. Sorry for the noise, I noticed it when I was trying to construct test cases. What do we think about adding this at the end of t5505: test_expect_success 'fetch fail [noexistinghost0:2223]:blink.git' ' ( ! git fetch [noexistinghost0:2223]:blink.git 2err grep ssh err rm err ) ' test_expect_success 'fetch fail noexistinghost1:2223:blink.git' ' ( ! git fetch noexistinghost1:2223:blink.git 2err grep ssh err rm err ) ' test_expect_success 'fetch fail noexistinghost2:2223' ' ( ! git fetch noexistinghost2:2223 2err grep ssh err rm err ) ' test_expect_success 'fetch fail ./noexistinghost4:2223' ' ( ! git fetch ./noexistinghost4:2223 2err grep does not appear to be a git repository err rm err ) ' -- 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] clone: tighten local paths with colons check a bit
On 2013-09-27 23.56, Jonathan Nieder wrote:
Nguyễn Thái Ngọc Duy wrote:
commit 6000334 (clone: allow cloning local paths with colons in them -
2013-05-04) is added to make it possible to specify a path that has
colons in it without file://, e.g. ../foo:bar/somewhere. But the check
is a bit loose.
[...]
Make sure we only check so when no protocol is specified and the url
is not started with '['.
More precisely, this disables the '/' before ':' check when the
url has been mangled by '[]' unwrapping (which only happens if the
URL starts with '[' and contains an ']' at some point later).
If I try to clone [foo]bar/baz:qux, after this change it will act as
though I specified the remote repository foo:qux instead of the local
repository ./foo:qux as before this change. Both are wrong ---
that's a bug for another day.
(Loud thinking)
Could it make sense to disable the SSH autodection logic
whenever the url starts with '.' (like in ../XX.git)
or with / like in /home/USER/projects/XX.git ?
diff --git a/connect.c b/connect.c
index a80ebd3..b382032 100644
--- a/connect.c
+++ b/connect.c
@@ -550,7 +550,8 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
end = host;
path = strchr(end, c);
- if (path !has_dos_drive_prefix(end)) {
+ if (path !has_dos_drive_prefix(end)
+ url[0] != '/' url[0] != '.' ) {
if (c == ':') {
if (path strchrnul(host, '/')) {
protocol = PROTO_SSH;
--
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] clone: local URLs are not for ssh
git clone /foo/bar:baz or git clone ../foo/bar:baz
are meant to clone from the local file system, and not to clone
from a remote server over git-over-ssh.
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
connect.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/connect.c b/connect.c
index a80ebd3..b382032 100644
--- a/connect.c
+++ b/connect.c
@@ -550,7 +550,8 @@ struct child_process *git_connect(int fd[2], const char
*url_orig,
end = host;
path = strchr(end, c);
- if (path !has_dos_drive_prefix(end)) {
+ if (path !has_dos_drive_prefix(end)
+ url[0] != '/' url[0] != '.' ) {
if (c == ':') {
if (path strchrnul(host, '/')) {
protocol = PROTO_SSH;
--
1.8.4.457.g424cb08
--
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: What's cooking in git.git (Sep 2013, #08; Wed, 25)
* jx/relative-path-regression-fix (2013-09-20) 3 commits - Use simpler relative_path when set_git_dir - relative_path should honor dos-driver-prefix - test: use unambigous leading path (/foo) for mingw Waiting for the review to settle. Is this V3, which is both fixing a regression and adding support for UNC path ? My understanding is that V2 commit 5a515ecc086dd8d0b74b0aff1248f4d1dc87f556 jx/relative-path-regression-fix git://github.com/gitster/git.git is only fixing the regression and could be merged into next, master and possibly maint. -- 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 1/2] relative_path should honor dos_drive_prefix
On 2013-09-17 21.32, Johannes Sixt wrote: Am 17.09.2013 10:24, schrieb Jiang Xin: I have checked the behavior of UNC path on Windows (msysGit): * I can cd to a UNC path: cd //server1/share1/path * can cd to other share: cd ../../share2/path * and can cd to other server's share: cd ../../../server2/share/path That means relative_path(path1, path2) support UNC paths out of the box. We only need to check both path1 and path2 are UNC paths, or both not. Your tests are flawed. You issued the commands in bash, which (or rather MSYS) does everything for you that you need to make it work. But in reality it does not, because the system cannot apply .. to //server/share: $ git ls-remote //srv/public/../repos/his/setups.git fatal: '//srv/public/../repos/his/setups.git' does not appear to be a git repository fatal: Could not read from remote repository. Please make sure you have the correct access rights and the repository exists. even though the repository (and //srv/public, let me assure) exists: $ git ls-remote //srv/repos/his/setups.git bea489b0611a72c41f133343fdccbd3e2b9f80b5HEAD ... The situation does not change with your latest round (v3). Please let me suggest not to scratch where there is no itch. ;) Your round v2 was good enough. If you really want to check UNC paths, then you must compare two path components after the the double-slash, not just one. Furthermore, you should audit all code that references is_absolute_path(), relative_path(), normalize_path_copy(), and possibly a few others whether the functions or call sites need improvement. That's worth a separate patch. -- Hannes I tend to agree here. The V2 patch fixed a regression. This should be one commit on its own: Documentation/SubmittingPatches: (1) Make separate commits for logically separate changes. Fixing a bug is a good thing, thanks for working on this, The support for UNC paths is a new feature, and this deserves a seperate commit. /Torsten -- 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 v3 2/3] relative_path should honor DOS and UNC paths
On 2013-09-18 16.37, Torsten Bögershausen wrote:
On 2013-09-17 18.12, Junio C Hamano wrote:
Jiang Xin worldhello@gmail.com writes:
diff --git a/compat/mingw.h b/compat/mingw.h
index bd0a88b..06e9f49 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -311,6 +311,15 @@ int winansi_fprintf(FILE *stream, const char *format,
...) __attribute__((format
#define has_dos_drive_prefix(path) (isalpha(*(path)) (path)[1] == ':')
#define is_dir_sep(c) ((c) == '/' || (c) == '\\')
+static inline int is_unc_path(const char *path)
+{
+ if (!is_dir_sep(*path) || !is_dir_sep(*(path+1)) ||
is_dir_sep(*(path+2)))
+ return 0;
If path[1] == '\0', it would be !is_dir_sep() and we end up
inspecting past the end of the string?
Yes
(If there was a previous mail, it was incomplete, sorry)
I think we want to catch 2 (back)slashes followed by a letter
http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247%28v=vs.85%29.aspx
#define is_unc_path(path) ((is_dir_sep(path)[0]) is_dir_sep((path)[1])
isalpha((path[2])))
Then we need
#define is_relative_path(path) (((path)[0]) !is_dir_sep((path)[1]))
And may be like this:
static int have_same_root(const char *path1, const char *path2)
{
int is_abs1, is_abs2;
is_abs1 = is_absolute_path(path1);
is_abs2 = is_absolute_path(path2);
if (is_abs1 is_abs2) {
if (is_unc_path(path1) !is_relative_path(path2))
return 0;
if (!is_relative_path(path1) is_unc_path(path2))
return 0;
return tolower(path1[0]) == tolower(path2[0]);
} else {
return !is_abs1 !is_abs2;
}
}
Could that work?
--
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] relative_path should honor dos_drive_prefix
On 13.09.13 07:08, Jiang Xin wrote:
Tvangeste found that the relative_path function could not work
properly on Windows if in and prefix have dos driver prefix.
($gmane/234434)
e.g., When execute: test-path-utils relative_path C:/a/b D:/x/y,
should return C:/a/b, but returns ../../C:/a/b, which is wrong.
So make relative_path honor dos_drive_prefix, and add test cases
for it in t0060.
Reported-by: Tvangeste i.4m.l...@yandex.ru
Helped-by: Johannes Sixt j...@kdbg.org
Signed-off-by: Jiang Xin worldhello@gmail.com
---
path.c| 20
t/t0060-path-utils.sh | 4
2 files changed, 24 insertions(+)
diff --git a/path.c b/path.c
index 9fd28bcd..65d376d 100644
--- a/path.c
+++ b/path.c
@@ -434,6 +434,16 @@ int adjust_shared_perm(const char *path)
return 0;
}
+static int have_same_root(const char *path1, const char *path2)
+{
+ int is_abs1, is_abs2;
+
+ is_abs1 = is_absolute_path(path1);
+ is_abs2 = is_absolute_path(path2);
+ return (is_abs1 is_abs2 tolower(path1[0]) == tolower(path2[0])) ||
+(!is_abs1 !is_abs2);
+}
+
I think the name of the fuction is somewhat misleading, as we are not sure if
they really have the same root.
And that is investigated further down.
may_have_same_root() could be a better name.
[snip]
while (i prefix_len j in_len prefix[i] == in[j]) {
if (is_dir_sep(prefix[i])) {
while (is_dir_sep(prefix[i]))
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 82a6f21..0187d11 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -210,6 +210,10 @@ relative_path foo/a/b/ foo/a/b ./
relative_path foo/a foo/a/b ../
relative_path foo/x/yfoo/a/b ../../x/y
relative_path foo/a/cfoo/a/b ../c
+relative_path foo/a/b/foo/x/yfoo/a/b
+relative_path /foo/a/b foo/x/y /foo/a/b
+relative_path d:/a/b D:/a/c ../bMINGW
+relative_path C:/a/b D:/a/c C:/a/b MINGW
Side question:
What happens if we feed in a relative path with a dos drive?
like c:foo which is different from c:/foo.
--
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 1/3] test: use unambigous leading path (/foo) for mingw
On 2013-09-13 21.51, Junio C Hamano wrote: Jiang Xin worldhello@gmail.com writes: In test cases for relative_path, path with one leading character (such as /a, /x) may be recogonized as a:/ or x:/ if there is such doc drive on MINGW platform. Use an umambigous leading path /foo instead. DOS drive, you mean? Are they really spelled as /a or /x (not e.g. //a or something)? Just double-checking. Yes, there is a directoctory structure in / like this: /usr /bin /lib Then we have the drive letters mapped to single letters: /c/Documents and Settings /c/temp As an alternative c:/temp can be used or the DOS style c:\temp And the // or \\ is used for the UNC names (Universal Name Convention) //Servername/ShareName/Directory /Torsten -- 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 1/2] relative_path should honor dos_drive_prefix
On 13.09.13 06:55, Jiang Xin wrote: 2013/9/13 Junio C Hamano gits...@pobox.com: For systems that need POSIX escape hatch for Apollo Domain ;-), we would need a bit more work. When both path1 and path2 begin with a double-dash, we would need to check if they match up to the next slash, so that - //host1/usr/src and //host1/usr/lib share the same root and the former can be made to ../src relative to the latter; - //host1/usr/src and //host2/usr/lib are of separate roots. or something. But how could we know which platform supports network pathnames and needs such implementation. Similar to the has_dos_drive_prefix: For Windows/Mingw we do like this mingw.h #define has_dos_drive_prefix(path) (isalpha(*(path)) (path)[1] == ':') And all other platforms defines has_dos_drive_prefix() to be 0 here git-compat-util.h #ifndef has_dos_drive_prefix #define has_dos_drive_prefix(path) 0 #endif mingw.h: #define has_unc_path_prefix(path) ((path)[0] == '/' (path)[1] == '/') (Or may be) #define has_unc_path_prefix(path) (is_dir_sep((path)[0]) is_dir_sep((path)[1])) -- 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 1/2] relative_path should honor dos_drive_prefix
On 2013-09-12 11.12, Jiang Xin wrote:
Tvangeste found that the relative_path function could not work
properly on Windows if in and prefix have dos driver prefix.
($gmane/234434)
e.g., When execute: test-path-utils relative_path C:/a/b D:/x/y,
should return C:/a/b, but returns ../../C:/a/b, which is wrong.
So make relative_path honor dos_drive_prefix, and add test cases
for it in t0060.
Reported-by: Tvangeste i.4m.l...@yandex.ru
Helped-by: Johannes Sixt j...@kdbg.org
Signed-off-by: Jiang Xin worldhello@gmail.com
---
path.c| 20
t/t0060-path-utils.sh | 4
2 files changed, 24 insertions(+)
diff --git a/path.c b/path.c
index 7f3324a..ffcdea1 100644
--- a/path.c
+++ b/path.c
@@ -441,6 +441,16 @@ int adjust_shared_perm(const char *path)
return 0;
}
+static int have_same_root(const char *path1, const char *path2)
+{
+ int is_abs1, is_abs2;
+
+ is_abs1 = is_absolute_path(path1);
+ is_abs2 = is_absolute_path(path2);
+ return (is_abs1 is_abs2 !strncasecmp(path1, path2, 1)) ||
^^^
I wonder: should strncasecmp() be replaced with strncmp_icase() ?
See dir.c:
int strncmp_icase(const char *a, const char *b, size_t count)
{
return ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);
}
/Torsten
+(!is_abs1 !is_abs2);
+}
+
/*
* Give path as relative to prefix.
*
@@ -461,6 +471,16 @@ const char *relative_path(const char *in, const char
*prefix,
else if (!prefix_len)
return in;
+ if (have_same_root(in, prefix)) {
+ /* bypass dos_drive, for c: is identical to C: */
+ if (has_dos_drive_prefix(in)) {
+ i = 2;
+ j = 2;
+ }
+ } else {
+ return in;
+ }
+
while (i prefix_len j in_len prefix[i] == in[j]) {
if (is_dir_sep(prefix[i])) {
while (is_dir_sep(prefix[i]))
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 76c7792..c3c3b33 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -208,6 +208,10 @@ relative_path a/b/ a/b ./
relative_path a a/b ../
relative_path x/ya/b ../../x/y
relative_path a/ca/b ../c
+relative_path a/b/x/ya/b
+relative_path /a/b x/y /a/bPOSIX
+relative_path d:/a/b D:/a/c ../bMINGW
+relative_path C:/a/b D:/a/c C:/a/b MINGW
relative_path a/bempty a/b
relative_path a/bnulla/b
relative_path empty /a/b./
--
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/RFC] core.precomposeunicode is true by default
(Sorry for the somewhat late reply, thanks for review)
Torsten Bögershausen tbo...@web.de writes:
When core.precomposeunicode was introduced, it was set to false
by default, to be compatible with older versions of Git.
Whenever UTF-8 file names are used in a mixed environment,
the Mac OS users need to find out that this configuration exist
and set it to true manually.
There is no measurable performance impact between false and true.
The real reason we default it to auto-sensing in the current code is
for correctness, I think. the new precompose code could be buggy,
and by auto-sensing, we hoped that we would enable it only on
filesystems that the codepath matters.
A smoother workflow can be achieved for new Git users,
so change the default to true:
- Remove the auto-sensing
Why?
- Rename the internal variable into precompose_unicode,
and set it to 1 meaning true.
Why the rename?
- Adjust and clean up test cases
The configuration core.precomposeunicode is still supported.
Sorry, but I do not quite understand the change. Is this because
the auto-sensing is not working, or after auto-sensing we do a wrong
thing? If that is the case, perhaps that is what we should fix?
diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
index 7980abd..5396b91 100644
--- a/compat/precompose_utf8.c
+++ b/compat/precompose_utf8.c
@@ -36,30 +36,6 @@ static size_t has_non_ascii(const char *s, size_t maxlen,
size_t *strlen_c)
}
-void probe_utf8_pathname_composition(char *path, int len)
-{
-static const char *auml_nfc = \xc3\xa4;
-static const char *auml_nfd = \x61\xcc\x88;
-int output_fd;
-if (precomposed_unicode != -1)
-return; /* We found it defined in the global config, respect it
*/
-strcpy(path + len, auml_nfc);
-output_fd = open(path, O_CREAT|O_EXCL|O_RDWR, 0600);
So we try to create a path under one name, and ...
-if (output_fd = 0) {
-close(output_fd);
-strcpy(path + len, auml_nfd);
-/* Indicate to the user, that we can configure it to true */
-if (!access(path, R_OK))
-git_config_set(core.precomposeunicode, false);
... see if that path can be seen under its alias. Why do we set it
to false? Isn't this the true culprit?
After all, this is not in the reinit codepath, so we know we are
dealing with a repository that was created afresh.
There is nothing wrong with the auto-sensing as such.
The problem for many users today is that we set core.precomposeunicode
to false, when it should be true.
A patch for that comes out in a minute. But first look back and
collect some experience with core.precomposeunicode.
Lets have a look at the variable precomposed_unicode,
(the one I wanted to rename to be more consistant).
It is controlled by the git config files and
depending on the config it is set like this:
core.precomposeuinicode false - precomposed_unicode = 0
core.precomposeuinicode true - precomposed_unicode = 1
core.precomposeuinicode not set - precomposed_unicode = -1.
Let's look what precomposed_unicode does and go through a couple
of git operations.
1)
When we create a repo under Mac OS using HFS+,
we want to have precomposed_unicode = 1
2)
When we access a repo from Windows/Linux using SAMBA,
readdir() will return decomposed.
When the repo is created by nonMacOS, core.precomposeunicode is undefined.
The precomposition is off, but should be on,
precomposed_unicode = -1, but should be = 1
3)
When we access a repo from another Mac OS system using
SAMBA, NFS or AFP readdir() will return decomposed.
As the repo is created under Mac OS, we have the same case as (1)
4)
When we access a repo from Linux using NFS we can have
precomposed_unicode = 0 (which is technically more correct).
If Linux users do not use decomposed unicode in their file names,
(according to my understanding this is the case), we can use 1
as well as 0:
precomposing an already precomposed string is a no-op, so it doesn't
harm.
5)
When we create a repo under Linux/Windows on a USB-drive,
and run git status under Mac OS, we want precomposed_unicode = 1.
There are few cases where we want to use precomposed_unicode=0:
a) To work around bugs. This may be a short term solution,
I would rather see bugs to be fixed.
I'm not aware of any bugs, so please remind me if I missed something.
b) Working with foreign vcs: E.g. bzr and hg use decomposed unicode,
so it may be better to use decomposed unicode in git as well.
The simplified V2 patch looks like this (I send it in a seperate mail):
diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
index 7980abd..95fe849 100644
--- a/compat/precompose_utf8.c
+++ b/compat/precompose_utf8.c
@@ -48,11 +48,8 @@ void probe_utf8_pathname_composition(char *path, int len)
if (output_fd = 0) {
close(output_fd);
strcpy(path + len, auml_nfd);
- /* Indicate to the user
[PATCH/RFC v2] Set core.precomposeunicode to true on e.g. HFS+
When core.precomposeunicode was introduced in 76759c7d,
it was set to false on a unicode decomposing file system like HFS+
to be compatible with older versions of Git.
The Mac OS users need to find out that this configuration exist
and change it manually from false to true.
A smoother workflow can be achieved,
so set core.precomposeunicode to true on a decomposing file system.
Signed-off-by: Torsten Bögershausen tbo...@web.de
---
compat/precompose_utf8.c | 7 ++-
t/t0050-filesystem.sh| 1 +
t/t3910-mac-os-precompose.sh | 2 +-
t/t7400-submodule-basic.sh | 1 -
4 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/compat/precompose_utf8.c b/compat/precompose_utf8.c
index 7980abd..95fe849 100644
--- a/compat/precompose_utf8.c
+++ b/compat/precompose_utf8.c
@@ -48,11 +48,8 @@ void probe_utf8_pathname_composition(char *path, int len)
if (output_fd = 0) {
close(output_fd);
strcpy(path + len, auml_nfd);
- /* Indicate to the user, that we can configure it to true */
- if (!access(path, R_OK))
- git_config_set(core.precomposeunicode, false);
- /* To be backward compatible, set precomposed_unicode to 0 */
- precomposed_unicode = 0;
+ precomposed_unicode = access(path, R_OK) ? 0 : 1;
+ git_config_set(core.precomposeunicode, precomposed_unicode ?
true : false);
strcpy(path + len, auml_nfc);
if (unlink(path))
die_errno(_(failed to unlink '%s'), path);
diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
index 05d78d2..6b3cedc 100755
--- a/t/t0050-filesystem.sh
+++ b/t/t0050-filesystem.sh
@@ -91,6 +91,7 @@ test_expect_failure CASE_INSENSITIVE_FS 'add (with different
case)' '
test_expect_success setup unicode normalization tests '
test_create_repo unicode
cd unicode
+ git config core.precomposeunicode false
touch $aumlcdiar
git add $aumlcdiar
git commit -m initial
diff --git a/t/t3910-mac-os-precompose.sh b/t/t3910-mac-os-precompose.sh
index 5fe57c5..e4ba601 100755
--- a/t/t3910-mac-os-precompose.sh
+++ b/t/t3910-mac-os-precompose.sh
@@ -36,7 +36,7 @@ Alongc=$Alongc$AEligatu$AEligatu #254 Byte
test_expect_success detect if nfd needed '
precomposeunicode=`git config core.precomposeunicode`
- test $precomposeunicode = false
+ test $precomposeunicode = true
git config core.precomposeunicode true
'
test_expect_success setup '
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 5ee97b0..f0f8cde 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -958,7 +958,6 @@ test_expect_success 'submodule with UTF-8 name' '
git add sub
git commit -m init sub
)
- test_config core.precomposeunicode true
git submodule add ./$svname
git submodule 2
test -n $(git submodule | grep $svname)
--
1.8.4.rc0.177.gceb3200
--
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/RFC] core.precomposeunicode is true by default
On 27.08.13 16:49, Junio C Hamano wrote: Torsten Bögershausen tbo...@web.de writes: ... see if that path can be seen under its alias. Why do we set it to false? Isn't this the true culprit? After all, this is not in the reinit codepath, so we know we are dealing with a repository that was created afresh. There is nothing wrong with the auto-sensing as such. The problem for many users today is that we set core.precomposeunicode to false, when it should be true. I think we are in agreement then. The code detects a broken filesystem just fine, but what it does when it finds the filesystem is broken is wrong---it sets the variable to false. That makes the whole auto-sensing wrong, and I think it makes sense to correct that behaviour. Let's look what precomposed_unicode does and go through a couple of git operations. 1) When we create a repo under Mac OS using HFS+, we want to have precomposed_unicode = 1 Yes. 2) When we access a repo from Windows/Linux using SAMBA, You mean s/repo/repository that resides on HFS+/? Sorry being unclear here, trying being clearer with an example: I have a /data/Docs on my linux box, which is handled by git I export /data/Docs via SAMBA, and use the Finder under Mac OS to have it mounted on my Mac OS X box: //tb@Linux/Docs on /Volumes/Docs (smbfs, nodev, nosuid, mounted by tb) readdir() will return decomposed. When the repo is created by nonMacOS, core.precomposeunicode is undefined. The precomposition is off, but should be on, precomposed_unicode = -1, but should be = 1 I do not think UTF-8-MAC is widely available; even if you flip the bit on, would it help much? In the above example /data/Docs/.git/config was created by Linux, so it does not have core.precomposeunicode set, neither false nor true. The Linux box does not have UTF-8-MAC under iconv, but will ignore core.precomposeunicode anyway (since the code is not compiled here) The Mac OS machine sees it under /Volumes/Docs/.git/config And here we want the precomposition, even if core.precomposeunicode is not present in the config. -- 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: Eclipse
On 27.08.13 17:10, Ron Tregaskis - NOAA Federal wrote: Does git work with Eclipse? No. If the question is does Eclipse work with git: yes. For more information please feel free to spend some seconds using a seach engine. -- 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/RFC] core.precomposeunicode is true by default
On 2013-08-27 18.27, Junio C Hamano wrote: Torsten Bögershausen tbo...@web.de writes: 2) When we access a repo from Windows/Linux using SAMBA, You mean s/repo/repository that resides on HFS+/? Sorry being unclear here, trying being clearer with an example: I have a /data/Docs on my linux box, which is handled by git I export /data/Docs via SAMBA, and use the Finder under Mac OS to have it mounted on my Mac OS X box: //tb@Linux/Docs on /Volumes/Docs (smbfs, nodev, nosuid, mounted by tb) readdir() will return decomposed. When the repo is created by nonMacOS, core.precomposeunicode is undefined. The precomposition is off, but should be on, precomposed_unicode = -1, but should be = 1 I do not think UTF-8-MAC is widely available; even if you flip the bit on, would it help much? In the above example /data/Docs/.git/config was created by Linux, so it does not have core.precomposeunicode set, neither false nor true. The Linux box does not have UTF-8-MAC under iconv, but will ignore core.precomposeunicode anyway (since the code is not compiled here) The Mac OS machine sees it under /Volumes/Docs/.git/config And here we want the precomposition, even if core.precomposeunicode is not present in the config. It almost makes me wonder if you want not a per-repository but a per-machine configuration, i.e. Whichever repository I am accessing, either on a local filesystem or shared over the network, readdir() on my box reports wrong paths, and I need correction. That, or if it hurts, don't do the remote mount then. No, we don't need to be that restrictive. We already have repository/user/system wide configuration files, allowing tweeks and this is a good thing. Re-Re-reading $gmane/188940: I am happy having the V2 patch from today being queued, thanks. As a next step I will have a look into the advice machine. -- 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/2] xread, xwrite: Limit size of IO, fixing IO of 2GB and more on Mac OS X
On 2013-08-20 08.43, Steffen Prohaska wrote: [] Thanks for V5. It was tested OK on my system here. (And apologies for recommending a wrapper on top of a wrapper). One question is left: As xread() is tolerant against EAGAIN and especially EINTR, could it make sense to replace read() with xread() everywhere? (The risk for getting EINTR is smaller when we only read a small amount of data, but it is more on the safe side) And s/write/xwrite/ /Torsten -- 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] compat: Fix read() of 2GB and more on Mac OS X
On 2013-08-19 08.38, Steffen Prohaska wrote:
[snip]
diff --git a/builtin/var.c b/builtin/var.c
index aedbb53..e59f5ba 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -38,6 +38,7 @@ static struct git_var git_vars[] = {
{ , NULL },
};
+#undef read
This is techically right for this very version of the code,
but not really future proof, if someone uses read() further down in the code
(in a later version)
I think the problem comes from further up:
--
struct git_var {
const char *name;
const char *(*read)(int);
};
-
could the read be replaced by readfn ?
===
diff --git a/streaming.c b/streaming.c
index debe904..c1fe34a 100644
--- a/streaming.c
+++ b/streaming.c
@@ -99,6 +99,7 @@ int close_istream(struct git_istream *st)
return r;
}
+#undef read
Same possible future problem as above.
When later someone uses read, the original (buggy) read() will be
used, and not the re-defined clipped_read() from git-compat-util.h
--
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] xread(): Fix read error when filtering = 2GB on Mac OS X
On 2013-08-17 14.40, Steffen Prohaska wrote:
Previously, filtering more than 2GB through an external filter (see
test) failed on Mac OS X 10.8.4 (12E55) with:
error: read from external filter cat failed
error: cannot feed the input to external filter cat
error: cat died of signal 13
error: external filter cat failed 141
error: external filter cat failed
The reason is that read() immediately returns with EINVAL if len = 2GB.
I haven't found any information under which specific conditions this
occurs. My suspicion is that it happens when reading from a pipe, while
reading from a standard file should always be fine. I haven't tested
any other version of Mac OS X, though I'd expect that other versions are
affected as well.
The problem is fixed by always reading less than 2GB in xread().
xread() doesn't guarantee to read all the requested data at once, and
callers are expected to gracefully handle partial reads. Slicing large
reads into 2GB pieces should not hurt practical performance.
Signed-off-by: Steffen Prohaska proha...@zib.de
---
t/t0021-conversion.sh | 9 +
wrapper.c | 8
2 files changed, 17 insertions(+)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index e50f0f7..aec1253 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -190,4 +190,13 @@ test_expect_success 'required filter clean failure' '
test_must_fail git add test.fc
'
+test_expect_success 'filter large file' '
+ git config filter.largefile.smudge cat
+ git config filter.largefile.clean cat
+ dd if=/dev/zero of=2GB count=2097152 bs=1024
+ echo /2GB filter=largefile .gitattributes
+ git add 2GB 2err
+ ! grep -q error err
+'
+
test_done
diff --git a/wrapper.c b/wrapper.c
index 6a015de..2a2f496 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -139,6 +139,14 @@ ssize_t xread(int fd, void *buf, size_t len)
{
ssize_t nr;
while (1) {
+#ifdef __APPLE__
+ const size_t twoGB = (1l 31);
+ /* len = 2GB immediately fails on Mac OS X with EINVAL when
+ * reading from pipe. */
+ if (len = twoGB) {
+ len = twoGB - 1;
+ }
+#endif
nr = read(fd, buf, len);
if ((nr 0) (errno == EAGAIN || errno == EINTR))
continue;
Thanks for the patch.
I think we can we can replace __APPLE__ define with a more generic one.
We had a similar patch for write() some time ago:
config.mak.uname
NEEDS_CLIPPED_WRITE = YesPlease
Makefile
ifdef NEEDS_CLIPPED_WRITE
BASIC_CFLAGS += -DNEEDS_CLIPPED_WRITE
COMPAT_OBJS += compat/clipped-write.o
endif
--
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: Bug in git on windows
On 2013-08-11 08.45, Илья Воронцов wrote: git under windows doesn't check case of letters in filename. So when one rename for example images from *.JPG to *.jpg, git doesn't files in a repository so when one deliver this repo on *nix -system, old filenames preserve and this matters. It can be very confusing when some of assets in your website on server can't be loaded after deploy, though on windows all was ok. Possibly git windows shall identify changed case of symbols and suggested to rename files in commit. It does (but this is disabled by default), you can try git config core.ignorecase false /Torsten -- 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: What's cooking in git.git (Jul 2013, #09; Mon, 29)
On 2013-08-01 22.51, Ramsay Jones wrote: Junio C Hamano wrote: Ramsay Jones ram...@ramsay1.demon.co.uk writes: I am personally in favor of this simpler solution. Comments? I had expected this to me marked for 'master'. Has this simply been overlooked, or do you have reservations about applying this patch? I am just being careful and do want to keep it cooking in 'next' during the feature freeze. The more users work with 'next' (not work *on* 'next'), the more confidence we would be with, and hopefully this can be one of the topis that graduate early after the 1.8.4 release. Hmm, this patch is a bug-fix for a bug that (currently) will be _introduced_ by v1.8.4. Do you want me to try and find a different bug-fix for v1.8.4? (Although that would most likely be more risky than simply taking this patch! ;-) ). ATB, Ramsay Jones I just managed to run v1.8.4-rc1 under cygwin 1.7, and it all passed. Good work, thanks. I realized that core.filemode is true by default, which by default switches of the stat()/lstat() code in cygwin.c Which bug fix are we missing for v1.8.4 ? /Torsten -- 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
