Re: Enhancements to git-protocoll
Fredrik Gustafsson writes: > Sometimes the server wants to communicate directly to the git user. > ... > For example: > gitolite has something called wild repos[1]. The management is > cumbersome and if you misspell when you clone a repo you might instead > create a new repo. > > This could have been avoided with a simply: > "Do you want to create a new repo[Yn]" I do not think the automatic repository creation done by gitolite is a good use case or example for whatever you seem to be advocating. IIUC, the auto-creation in gitolite-shell::main() is done way before gitolite-shell (which is used as a login shell for incoming ssh sessions) creates a new git repository, goes into it and spawns the git-receive-pack command. It all happens outside Git. # auto-create? if ( repo_missing($repo) and access( $repo, $user, '^C', 'any' ) !~ /DENIED/ ) { require Gitolite::Conf::Store; Gitolite::Conf::Store->import; new_wild_repo( $repo, $user, $aa ); gl_log( 'create', $repo, $user, $aa ); } The "access()" we see here is not the Perl builtin access(), but is a function defined in src/lib/Gitolite/Conf/Load.pm; that would be the place to allow the incoming ssh session to talk back to the end user, but at that point there is no Git processing on the server end. While I am not fundamentally opposed to adding yet another sideband channel to the git protocol, I do not think adding user interaction at random places in the protocol exchange is a viable or useful approach to implement an enhanced server that works with both enhanced and vanilla clients (and the same is true for enhanced client that works with both enhanced and vanilla server). -- 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] buitin_config: return postitive status in get_value
Nguyen Thai Ngoc Duy writes: > On Sat, Jul 28, 2012 at 04:18:49PM +0300, Nikolay Vladimirov wrote: >> But the behavior now seems kind of strange, or maybe I'm missing something: >> # git config foobar; echo $? >> error: key does not contain a section: foobar >> 255 >> >> # git config foobar.info; echo $? >> 1 >> >> git version 1.7.11.2 >> >> I would generally expect the both to behave the same way. > > Then the following patch may be better because it leaves other cases > untouched (I'm not saying that we should or should not do it though) I personally think that the documentation unnecessarily exposes the useless value assignment of the exit codes (the use of different exit codes was done merely to aid debugging the git-config command itself) by describing the then-current set of conditions, and should be reduced to say "0 for success, non-zero for any error". But if we really want to follow that "documented" subset of possible conditions, I agree that your version to return "1" in this case, together with a change to initialize "ret" to "7" and document it as "all other errors (ret=7), would make more sense. Other conditions that have been added since that partial enumeration of the exit code was done are regexp errors, which I think will get -1 from the same function. > > -- 8< -- > diff --git a/builtin/config.c b/builtin/config.c > index 8cd08da..d048ebf 100644 > --- a/builtin/config.c > +++ b/builtin/config.c > @@ -199,8 +199,10 @@ static int get_value(const char *key_, const char > *regex_) > goto free_strings; > } > } else { > - if (git_config_parse_key(key_, &key, NULL)) > + if (git_config_parse_key(key_, &key, NULL)) { > + ret = 1; > goto free_strings; > + } > } > > if (regex_) { > -- 8< -- -- 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] log: remove redundant check for merge commit
Martin von Zweigbergk writes: >> I incorrectly assumed that ignore_merges was about revision >> traversal, but now I think it's only diff output from 'git log' (and >> possibly others). Yeah, I realized the same after I wrote the response last night and went to bed. I am glad you figured all out yourself. -- 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] cleanup argument passing in submodule status command
Heiko Voigt writes: > Note: This is a code cleanup and does not fix any bugs. As a side effect > the variables containing the parsed flags to "git submodule status" are > passed down recursively. So everything was already behaving as expected. If that is the case, shouldn't we stop passing anything down, if we want it to be a "clean-up only, no behaviour changes" patch? While at it, we may want to kill that code to accumulate the original options in orig_flags because we haven't been using the variable. We _know_ $orig_args has been empty, i.e. the code has been working fine with only cmd_status there. Nobody has tried what happens when we pass the original arguments to cmd_status on that line. The patch changes the behaviour of the code; it makes the command line parsing "while" loop to run again, and if the code that accumulates original options in orig_flags have been buggy, now that bug will be exposed. > Signed-off-by: Heiko Voigt > --- > git-submodule.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/git-submodule.sh b/git-submodule.sh > index dba4d39..3a3f0a4 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -961,7 +961,7 @@ cmd_status() > prefix="$displaypath/" > clear_local_git_env > cd "$sm_path" && > - eval cmd_status "$orig_args" > + eval cmd_status "$orig_flags" > ) || > die "$(eval_gettext "Failed to recurse into submodule > path '\$sm_path'")" > fi -- 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] log: remove redundant check for merge commit
Sorry, I meant to CC the list. See below. On Sat, Jul 28, 2012 at 9:56 PM, Martin von Zweigbergk wrote: > On Fri, Jul 27, 2012 at 11:52 PM, Junio C Hamano wrote: >> Junio C Hamano writes: >> >> It seems to have some interaction with your other topic, though. >> These two patches alone will pass the existing tests, but merging it >> with mz/rebase-range breaks t3412. I didn't look into it, but >> perhaps this breaks "git cherry" in some way? > > Yes, it breaks "git cherry" quite badly, by not ignoring merges at > all. I incorrectly assumed that ignore_merges was about revision > traversal, but now I think it's only diff output from 'git log' (and > possibly others). What I think tricked me was seeing that > "ignore_merges = 1" closely followed by a comment saying "ignore > merges". But now I think the explicit code to "ignore merges" is > necessary (as show by the failing test case), but can be replaced by > "rev_info.max_parents = 1". Setting "ignore_merges = 1", OTOH, now > seems doubly redundant: not only does it set the same value as was set > in init_revisions, but it's also irrelevant. Since cherry doesn't > generate any diff output, I think ignore_merges is never used. > Flipping the values of all of "ignore_merges", "combine_merges" and > "diff" does not have any effect on test cases at least. I hope my > explanation makes some sense at least... > > I'll send a reroll when I get time. -- 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
Enhancements to git-protocoll
Hi, sometimes git communicates with something that's not git on the other side (gitolite and github for example). Sometimes the server wants to communicate directly to the git user. git isn't really designed for this. gitolite solves this by do user interaction on STDERR instead. The bad thing about this is that it can only be one-direction communication, for example error messages. If git would allow for the user to interact direct with the server, a lot of cool and and userfriendly features could be developed. For example: gitolite has something called wild repos[1]. The management is cumbersome and if you misspell when you clone a repo you might instead create a new repo. This could have been avoided with a simply: "Do you want to create a new repo[Yn]" To fix this, git protocol should have a command for printing input to STDOUT and accepting input on STDIN which should be sent to the server. And a command to switch back to orginal of course. The server could then switch to user interaction, do that and then switch back to normal operation. Before eventually starting to implement this, I would know your opinions. This feature would be wortless if it's not in the official git. [1] http://sitaramc.github.com/gitolite/wild.html -- Med vänliga hälsningar Fredrik Gustafsson tel: 0733-608274 e-post: iv...@iveqy.com -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
bug (?) in send email
Hello! send-email (tested versions 1.7.9.2 and 1.7.10.4) breaks email addresses. Steps to reproduce: Modify file. git commit --author="Michał Tz " modified.file -m "Test" git format-patch -o patches origin Now, the patch seems to have the address right, see [1] git send-email --to myown.addr...@mail.com --suppress-cc=author patches/0001-Test.patch But checking my inbox now shows an email starting with: From: Michał Tz So the address is splitted at the underscore. Furthermore, if I don't use --suppress-cc=author, the CC field shows the right address. Regards Christoph [1] less patches/0001-Test.patch From: =?UTF-8?q?Micha=C5=82=20Tz?= git show Author: Michał Tz -- 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/7] Change canonicalize_url() to use the SVN 1.7 API when available.
On 2012.7.28 1:02 PM, Jonathan Nieder wrote: > Jonathan Nieder wrote: >> Michael G Schwern wrote: > >>> I would suggest that worrying whether a few lines of code are introduced now >>> or 10 patches later in the same branch which is all going to be merged in >>> one >>> go (and retesting the patches after it) is not the most important thing. > [...] >> In that case they should be one patch, I'd think. >> >> The advantage of introducing changes gradually is that (1) the changes >> can be examined and tested one at a time, and (2) if later a change >> proves to be problematic, it can be isolated, understood, and fixed >> more easily. The strategy you are suggesting would have neither of >> those advantages. > > (To avoid confusion: by "The strategy you are suggesting" I mean > introducing dead code first and activating it later, not the path and > url object idea. The path and url object approach would be very > nice. :)) If this is all a topic branch then it doesn't matter much whether a couple lines of code is introduced at patch 8 of a branch or patch 13. Sure, it matters a little, but... https://secure.wikimedia.org/wikipedia/en/wiki/Opportunity_cost If it *isn't* going in a topic branch, if its not visible as a collected work in history, if its going to be rebased on top of master, then yeah I can see why you're so concerned. -- Alligator sandwich, and make it snappy! -- 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/7] Change canonicalize_url() to use the SVN 1.7 API when available.
Jonathan Nieder wrote: > Michael G Schwern wrote: >> I would suggest that worrying whether a few lines of code are introduced now >> or 10 patches later in the same branch which is all going to be merged in one >> go (and retesting the patches after it) is not the most important thing. [...] > In that case they should be one patch, I'd think. > > The advantage of introducing changes gradually is that (1) the changes > can be examined and tested one at a time, and (2) if later a change > proves to be problematic, it can be isolated, understood, and fixed > more easily. The strategy you are suggesting would have neither of > those advantages. (To avoid confusion: by "The strategy you are suggesting" I mean introducing dead code first and activating it later, not the path and url object idea. The path and url object approach would be very nice. :)) Sorry for the lack of clarity. Jonathan -- 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/7] Change canonicalize_url() to use the SVN 1.7 API when available.
Michael G Schwern wrote: > On 2012.7.28 12:30 PM, Jonathan Nieder wrote: >> Since this part of the series is not tested with SVN 1.7, this is >> basically adding dead code, right? That could be avoided by >> reordering the changes to keep "canonicalize_url" as-is until later in >> the series when the switchover is safe. > > I would suggest that worrying whether a few lines of code are introduced now > or 10 patches later in the same branch which is all going to be merged in one > go (and retesting the patches after it) is not the most important thing. The > code needs humans looking over it and deciding if canonicalizations were > missed or applied inappropriately. Or hey, work on that path and url object > idea that makes a lot of real code mess go away. In that case they should be one patch, I'd think. The advantage of introducing changes gradually is that (1) the changes can be examined and tested one at a time, and (2) if later a change proves to be problematic, it can be isolated, understood, and fixed more easily. The strategy you are suggesting would have neither of those advantages. Jonathan -- 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/7] Change canonicalize_url() to use the SVN 1.7 API when available.
On 2012.7.28 12:30 PM, Jonathan Nieder wrote: >> I didn't know about that. I don't know what your SVN backwards compat >> requirements are, but if that behavior goes back far enough in SVN to satisfy >> you folks, then canonicalize_url() should fall back to >> SVN::_Core::svn_path_canonicalize(). > > svn_path_canonicalize() has been usable for this kind of thing since > SVN 1.1, possibly earlier. Great! Then _canonicalize_url_ourselves() can probably be replaced with that. Just take my advice and do it after 1.7 is working and the code is ready for canonicalization. >> But try it at the end of the patch >> series. The code has to be prepared for canonicalization first. Then how it >> actually does it can be improved. > > Since this part of the series is not tested with SVN 1.7, this is > basically adding dead code, right? That could be avoided by > reordering the changes to keep "canonicalize_url" as-is until later in > the series when the switchover is safe. I would suggest that worrying whether a few lines of code are introduced now or 10 patches later in the same branch which is all going to be merged in one go (and retesting the patches after it) is not the most important thing. The code needs humans looking over it and deciding if canonicalizations were missed or applied inappropriately. Or hey, work on that path and url object idea that makes a lot of real code mess go away. -- ROCKS FALL! EVERYONE DIES! http://www.somethingpositive.net/sp05032002.shtml -- 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/8] SVN 1.7 will truncate "not-a%40{0}" to just "not-a".
On 2012.7.28 7:16 AM, Jonathan Nieder wrote: > Michael G. Schwern wrote: > >> Rather than guess what SVN is going to do for each version, make the test use >> the branch name that was actually created. > [...] >> -git rev-parse "refs/remotes/not-a%40{0}reflog" >> +git rev-parse "refs/remotes/$non_reflog" > > Doesn't this defeat the point of the testcase (checking that git-svn > is able to avoid creating git refs containing @{, following the rules > from git-check-ref-format(1))? Unless I messed up, entirely possible as I'm not a shell programmer, the test is still useful for testing SVN 1.6. Under SVN 1.6 $non_reflog should be 'not-a%40{0}reflog' as before. > Do you know when SVN truncates the directory name? IIRC its silently does it during the "svn cp". > Would historical > SVN repositories or historical SVN servers be able to have a directory > named with a %40 in it, or has this been disallowed completely, > leaving problematic historical repositories to be dumped with old SVN, > tweaked, and reloaded with new SVN? Dunno, lemme check... $ source ~/bin/svn16 $ svnadmin --version svnadmin, version 1.6.18 (r1303927) ... $ svnadmin create svnrepo $ mkdir project project/trunk project/branches project/tags $ echo foo > project/trunk/foo $ svn import -m 'test import' project file:///Users/schwern/tmp/test/svnrepo/project Adding project/tags Adding project/trunk Adding project/trunk/foo Adding project/branches Committed revision 1. $ rm -rf project/ $ svn cp -m 'reflog' file:///Users/schwern/tmp/test/svnrepo/project/trunk 'file:///Users/schwern/tmp/test/svnrepo/project/branches/not-a%40{0}reflog' Committed revision 2. $ svn ls file:///Users/schwern/tmp/test/svnrepo/project/branches not-a@{0}reflog/ $ source ~/bin/svn17 $ svn --version svn, version 1.7.5 (r1336830) ... $ svn ls file:///Users/schwern/tmp/test/svnrepo/project/branches not-a@{0}reflog/ If you make it with SVN 1.6 its still there with SVN 1.7. That's good, it means you can ship a prebuilt repository and check it against SVN 1.7. The bad news is the new code segfaults on it. I don't know if that's the SVN 1.7 API choking on its own stuff or because of my changes or both. If you set up the test I can try and fix it. Otherwise I'll just flounder in shell. -- "I went to college, which is a lot like being in the Army, except when stupid people yell at me for stupid things, I can hit them." -- Jonathan Schwarz -- 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/7] Change canonicalize_url() to use the SVN 1.7 API when available.
Michael G Schwern wrote: > On 2012.7.28 6:50 AM, Jonathan Nieder wrote: >> If I am reading Subversion r873487 correctly, in ancient times, >> svn_path_canonicalize() did the appropriate tweaking for URIs. Today >> its implementation is comforting: >> >> const char * >> svn_path_canonicalize(const char *path, apr_pool_t *pool) >> { >>if (svn_path_is_url(path)) >> return svn_uri_canonicalize(path, pool); >>else >> return svn_dirent_canonicalize(path, pool); >> } [...] > I didn't know about that. I don't know what your SVN backwards compat > requirements are, but if that behavior goes back far enough in SVN to satisfy > you folks, then canonicalize_url() should fall back to > SVN::_Core::svn_path_canonicalize(). svn_path_canonicalize() has been usable for this kind of thing since SVN 1.1, possibly earlier. > But try it at the end of the patch > series. The code has to be prepared for canonicalization first. Then how it > actually does it can be improved. Since this part of the series is not tested with SVN 1.7, this is basically adding dead code, right? That could be avoided by reordering the changes to keep "canonicalize_url" as-is until later in the series when the switchover is safe. Thanks. Will play around with this code more. Jonathan -- 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 7/7] Make Git::SVN and Git::SVN::Ra canonicalize paths and urls.
On 2012.7.28 7:11 AM, Jonathan Nieder wrote: > Yay! Am I correct in imagining this makes the following sequence of > commands[1] no longer trip an assertion failure in svn_path_join[2] > with SVN 1.6? > > git svn init -Thttp://trac-hacks.org/svn/tagsplugin/trunk \ > -thttp://trac-hacks.org/svn/tagsplugin/tags \ > -bhttp://trac-hacks.org/svn/tagsplugin/branches > git svn fetch Works For Me™! ... Checked out HEAD: http://trac-hacks.org/svn/tagsplugin/trunk r11776 -- Insulting our readers is part of our business model. http://somethingpositive.net/sp07122005.shtml -- 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 6/7] Switch path canonicalization to use the SVN API.
On 2012.7.28 6:55 AM, Jonathan Nieder wrote: > Michael G. Schwern wrote: >> --- a/perl/Git/SVN/Utils.pm >> +++ b/perl/Git/SVN/Utils.pm >> @@ -86,6 +86,27 @@ sub _collapse_dotdot { >> >> >> sub canonicalize_path { >> +my $path = shift; >> + >> +# The 1.7 way to do it >> +if ( defined &SVN::_Core::svn_dirent_canonicalize ) { >> +$path = _collapse_dotdot($path); >> +return SVN::_Core::svn_dirent_canonicalize($path); >> +} >> +# The 1.6 way to do it >> +elsif ( defined &SVN::_Core::svn_path_canonicalize ) { >> +$path = _collapse_dotdot($path); >> +return SVN::_Core::svn_path_canonicalize($path); >> +} >> +# No SVN API canonicalization is available, do it ourselves >> +else { > > When would this "else" case trip? When svn_path_canonicalize() does not exist in the SVN API, presumably because their SVN is too old. > Would it be safe to make it > return an error message, or even to do something like the following? I don't know what your SVN backwards compat requirements are, or when svn_path_canonicalize() appears in the API, so I left it as is. git-svn's home rolled path canonicalization worked and its no work to leave it working. No reason to break it IMO. > sub canonicalize_path { > my $path = shift; > $path = _collapse_dotdot($path); > > # Subversion 1.7 split svn_path_canonicalize() into > # svn_dirent_canonicalize() and svn_uri_canonicalize(). > if (!defined &SVN::_Core::svn_dirent_canonicalize) { > return SVN::_Core::svn_path_canonicalize($path); > } > > return SVN::_Core::svn_dirent_canonicalize($path); > } As a side note... "If they don't have Mars bar, get me a Twix. Else get me a Mars bar." "If they have a Mars bar, get me one. Else get me a Twix." -- Look at me talking when there's science to do. When I look out there it makes me glad I'm not you. I've experiments to be run. There is research to be done On the people who are still alive. -- Jonathan Coulton, "Still Alive" -- 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/7] Change canonicalize_url() to use the SVN 1.7 API when available.
On 2012.7.28 6:50 AM, Jonathan Nieder wrote: >> --- a/perl/Git/SVN/Utils.pm >> +++ b/perl/Git/SVN/Utils.pm > [...] >> @@ -100,6 +102,20 @@ API as a URL. >> =cut >> >> sub canonicalize_url { >> +my $url = shift; >> + >> +# The 1.7 way to do it >> +if ( defined &SVN::_Core::svn_uri_canonicalize ) { >> +return SVN::_Core::svn_uri_canonicalize($url); >> +} >> +# There wasn't a 1.6 way to do it, so we do it ourself. >> +else { >> +return _canonicalize_url_ourselves($url); >> +} >> +} >> + >> + >> +sub _canonicalize_url_ourselves { >> my ($url) = @_; >> $url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e; > > Leaves me a bit nervous. As it should, SVN dumped a mess on us. > What effect should we expect this change to have? Is our emulation > of svn_uri_canonicalize already perfect and this change just a little > futureproofing in case svn_uri_canonicalize gets even better, or is > this a trap waiting to happen when new callers of canonicalize_url > start relying on, e.g., %-encoding of special characters? This change is *just* about sliding in the SVN API call and seeing if git-svn still works with SVN 1.6. It should have no effect on SVN 1.6. These patches are a very slow and careful refactoring doing just one thing at a time. Every time I tried to do too many things at once, tests broke and I had to tease the patch apart. At this point in the patch series the code is not ready for canonicalization. Until 3/8 in the next patch series, canonicalize_url() basically does nothing on SVN 1.6 so the code has never had to deal with the problem. 3/8 deals with improving _canonicalize_url_ourselves() to work more like svn_uri_canonicalize() and thus "turns on" canonicalization for SVN 1.6 and deals with the breakage. > If I am reading Subversion r873487 correctly, in ancient times, > svn_path_canonicalize() did the appropriate tweaking for URIs. Today > its implementation is comforting: > > const char * > svn_path_canonicalize(const char *path, apr_pool_t *pool) > { > if (svn_path_is_url(path)) > return svn_uri_canonicalize(path, pool); > else > return svn_dirent_canonicalize(path, pool); > } > > It might be easier to rely on that on pre-1.7 systems. I didn't know about that. I don't know what your SVN backwards compat requirements are, but if that behavior goes back far enough in SVN to satisfy you folks, then canonicalize_url() should fall back to SVN::_Core::svn_path_canonicalize(). But try it at the end of the patch series. The code has to be prepared for canonicalization first. Then how it actually does it can be improved. -- Defender of Lexical Encapsulation -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] t7810-*.sh: Remove redundant test
Since commit bbc09c22 ("grep: rip out support for external grep", 12-01-2010), test number 60 ("grep -C1 hunk mark between files") is essentially the same as test number 59. Test 59 was intended to verify the behaviour of git-grep resulting from multiple invocations of an external grep. As part of the test, it creates and adds 1024 files to the index, which is now wasted effort. Remove test 59, since it is now redundant. Signed-off-by: Ramsay Jones --- t/t7810-grep.sh | 11 --- 1 file changed, 11 deletions(-) diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh index 24e9b19..523d041 100755 --- a/t/t7810-grep.sh +++ b/t/t7810-grep.sh @@ -399,17 +399,6 @@ test_expect_success 'grep -q, silently report matches' ' test_cmp empty actual ' -# Create 1024 file names that sort between "y" and "z" to make sure -# the two files are handled by different calls to an external grep. -# This depends on MAXARGS in builtin-grep.c being 1024 or less. -c32="0 1 2 3 4 5 6 7 8 9 a b c d e f g h i j k l m n o p q r s t u v" -test_expect_success 'grep -C1, hunk mark between files' ' - for a in $c32; do for b in $c32; do : >y-$a$b; done; done && - git add y-?? && - git grep -C1 "^[yz]" >actual && - test_cmp expected actual -' - test_expect_success 'grep -C1 hunk mark between files' ' git grep -C1 "^[yz]" >actual && test_cmp expected actual -- 1.7.11.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 1/2] t1100-*.sh: Fix an intermittent test failure
In particular, the final test ('flags and then non flags') fails intermittently, depending on how much time elapsed between the invocations of "git commit-tree" when creating the commits which later have their commit id's compared. For example, if the commits for childid-3 and childid-4 are created 1 or more seconds apart, then the commits, which would otherwise be identical, will have different commit id's. In order to make the test reproducible, we remove the variability by setting the author and committer times to a well defined state. We accomplish this with a single call to 'test_tick' at the start of the test. Signed-off-by: Ramsay Jones --- t/t1100-commit-tree-options.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/t/t1100-commit-tree-options.sh b/t/t1100-commit-tree-options.sh index a3b7723..f8457f9 100755 --- a/t/t1100-commit-tree-options.sh +++ b/t/t1100-commit-tree-options.sh @@ -47,6 +47,7 @@ test_expect_success \ test_expect_success 'flags and then non flags' ' + test_tick && echo comment text | git commit-tree $(cat treeid) >commitid && echo comment text | -- 1.7.11.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 0/2] test results for v1.7.12-rc0 on cygwin
Hi Junio, I actually tested v1.7.12-rc0-22-gcdd159b, thus: $ time $(GIT_SKIP_TESTS='t0061.3 t0070.3 t9010 t9300' make test > test-outp1 2>&1) real137m11.901s user118m55.071s sys 59m50.695s $ the result, ignoring the explicity skipped tests (nothing new there), was three additional failures: t1100.5, t7502.21 and t7810.59 I will be sending a patch to fix t1100.5 (patch #1). The failure in t7502-commit.sh seems to be the result of commit 8c5b1ae1 ("ident: reject bogus email addresses with IDENT_STRICT", 24-05-2012) being more strict with non fully qualified hostnames. I get the "tell me who you are" message and the error: fatal: unable to auto-detect email address (got 'ramsay@toshiba.(none)') However, I *think* I saw that Jeff has submitted a fix for this already. Unfortunately, I was unable to reproduce the final failure in t7810-grep.sh. I tried, among other things, to provoke a failure thus: $ for i in $(seq 100); do > if ! ./t7810-grep.sh -i -v; then > break; > fi > done $ but, apart from chewing on the cpu for about 50 minutes, it didn't result in a failure. :( However, after looking at test 59, it seems to me to be a stale (redundant) test. So, patch #2 removes that test! :-D [I wish I could reproduce the failure because I don't like not knowing why it failed, but ...] ATB, Ramsay Jones -- 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: [RFC/PATCH] t3300-*.sh: Fix a TAP parse error
Junio C Hamano wrote: > Jonathan Nieder writes: > >> FWIW I find Junio's test_setup name more self-explanatory. What >> mnemonic should I be using to remember the _fixture name? > > Previous exposure to things like Rails? I did once have a brief look at ruby, but my "new language to learn" list was over-subscribed. (I think I went with D) So, I'm not at all familiar with Rails. A test-fixture may be used in various xUnit unit-test libraries. (eg JUnit, cppUnit, etc) ATB, Ramsay Jones -- 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: [RFC/PATCH] t3300-*.sh: Fix a TAP parse error
Jonathan Nieder wrote: > [...] >> [1] For example, what should/will happen if someone uses test_must_fail, >> test_might_fail, etc., within the test_fixture script? Should they simply >> be banned within a text_fixture? > > Why wouldn't they act just like they do in test_expect_success blocks? Heh, well they do indeed act just like they do in text_expect_success blocks! I spent only about 20 minutes writing test_fixture, playing with it, and then deciding to shelve it for now. Again, I wanted a *quick* fix for the TAP parse error, so that it would make it into v1.7.12. :( Having now spent a further 30 minutes, I can see that I did a better job than I thought! :-P Actually, scratch that; rather I should say that Junio and the other authors of the test infrastructure did such a good job (particularly with separation of concerns), that I lucked into a good implementation. I still haven't done any serious testing, so if I subsequently find any problems, then the lousy implementation is my fault! ;-) > FWIW I find Junio's test_setup name more self-explanatory. What > mnemonic should I be using to remember the _fixture name? I don't have a problem with 'test_setup' either; test-fixture comes from the various xUnit unit-test libraries. (I think Kent Beck et.al. wrote JUnit first and then it was ported to various other languages. eg cppUnit for C++). Briefly, a test-fixture provides a context or common environment, via code for test setup and teardown, in which to run one or more tests. HTH ATB, Ramsay Jones -- 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: Recursive submodules fail when the repo path contains spaces
On Saturday, 28. July 2012 at 09:21, Heiko Voigt wrote: > On Tue, Jul 24, 2012 at 01:33:44PM -0700, Justin Spahr-Summers wrote: > > Here's some real output, with a couple specific names removed, starting > > from the root of the top-level repository (where External/twui is a > > submodule): > > > > $ cd External/twui > > $ git submodule add git://github.com/petejkim/expecta.git > > (http://github.com/petejkim/expecta.git) TwUITests/expecta > > Cloning into 'TwUITests/expecta'... > > remote: Counting objects: 988, done. > > remote: Compressing objects: 100% (404/404), done. > > remote: Total 988 (delta 680), reused 842 (delta 535) > > Receiving objects: 100% (988/988), 156.30 KiB, done. > > Resolving deltas: 100% (680/680), done. > > fatal: Not a git repository: ../../../../../../../../Volumes/drive name > > with spaces/Users/justin/Documents/Programming/project name with > > spaces/.git/modules/External/twui/modules/TwUITests/expecta > > > > Is this a copy&paste artefact or is the path in the error truncated? There's apparently some weird wrapping from my email client (the git error is on a single line in the terminal), but there is no truncation. That's the full path. Thanks for clarifying, though. -- 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: Recursive submodules fail when the repo path contains spaces
On Tue, Jul 24, 2012 at 01:33:44PM -0700, Justin Spahr-Summers wrote: > Here's some real output, with a couple specific names removed, starting from > the root of the top-level repository (where External/twui is a submodule): > > $ cd External/twui > $ git submodule add git://github.com/petejkim/expecta.git TwUITests/expecta > Cloning into 'TwUITests/expecta'... > remote: Counting objects: 988, done. > remote: Compressing objects: 100% (404/404), done. > remote: Total 988 (delta 680), reused 842 (delta 535) > Receiving objects: 100% (988/988), 156.30 KiB, done. > Resolving deltas: 100% (680/680), done. > fatal: Not a git repository: ../../../../../../../../Volumes/drive name with > spaces/Users/justin/Documents/Programming/project name with > spaces/.git/modules/External/twui/modules/TwUITests/expecta Is this a copy&paste artefact or is the path in the error truncated? Cheers Heiko -- 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: t1450-fsck (sometimes/often) failes on Mac OS X
Heiko Voigt writes: > if (!git_index_file) { > - git_index_file = xmalloc(strlen(git_dir) + 7); > + git_index_file = xmalloc(strlen(git_dir) + 7 + 8); > sprintf(git_index_file, "%s/index", git_dir); > } [...] > - if (!memcmp(ent->base, objdir, pfxlen)) { > + objdirlen = strlen(objdir); > + if (!memcmp(ent->base, objdir, pfxlen > objdirlen ? objdirlen : > pfxlen)) { [...] > Initialized empty Git repository in /Users/hvoigt/Repository/git/t/trash > directory.t1450-fsck/another/.git/ > ==42686== Invalid read of size 8 > ==42686==at 0x100625064: bcmp (in /usr/lib/libSystem.B.dylib) > ==42686==by 0x100112846: link_alt_odb_entries (in > /Users/hvoigt/Repository/git/t/valgrind/../../git) > ==42686==by 0x1001129C0: read_info_alternates (in > /Users/hvoigt/Repository/git/t/valgrind/../../git) [...] > ==42686== Address 0x100faca78 is 8 bytes inside a block of size 13 alloc'd > ==42686==at 0x10029C679: malloc (vg_replace_malloc.c:266) > ==42686==by 0x1001349CD: xmalloc (in > /Users/hvoigt/Repository/git/t/valgrind/../../git) > ==42686==by 0x1000C23F5: setup_git_env (in > /Users/hvoigt/Repository/git/t/valgrind/../../git) To me that looks just like a false positive. memcmp (which seems to be the same as bcmp) can load 8 bytes from an aligned address even if these are only partially within the block being compared, since an aligned load can never partially fault (it must all be within the same page). Valgrind normally substitutes its own routines for memcmp etc. to correctly handle this, but this does not seem to happen in your case for some reason. Then again I am not entirely sure how you could verify that this theory is correct :-) -- Thomas Rast trast@{inf,student}.ethz.ch -- 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] link_alt_odb_entry: fix read over array bounds reported by valgrind
pfxlen can be longer than the path in objdir when relative_base contains the path to gits object directory. Signed-off-by: Heiko Voigt --- sha1_file.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 4ccaf7a..631d0dd 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -251,7 +251,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative const char *objdir = get_object_directory(); struct alternate_object_database *ent; struct alternate_object_database *alt; - int pfxlen, entlen; + int pfxlen, entlen, objdirlen; struct strbuf pathbuf = STRBUF_INIT; if (!is_absolute_path(entry) && relative_base) { @@ -298,7 +298,8 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative return -1; } } - if (!memcmp(ent->base, objdir, pfxlen)) { + objdirlen = strlen(objdir); + if (!memcmp(ent->base, objdir, pfxlen > objdirlen ? objdirlen : pfxlen)) { free(ent); return -1; } -- 1.7.12.rc0.23.gc9a5ac4 -- 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: t1450-fsck (sometimes/often) failes on Mac OS X
Hi, just to verify that this is unlikely just a hardware issue on one machine. I today experienced this failure on master as well. On Mon, Jul 16, 2012 at 06:06:26PM +0200, Torsten B?gershausen wrote: > > Am 16.07.2012 um 09:57 schrieb Thomas Rast: > > > Torsten Bögershausen writes: > > What OS X are you running? I started a loop > > > > while : ; do ./t1450-fsck.sh || break; done > > > > and it hasn't failed yet. It is > > > > $ uname -a > > Darwin mackeller.inf.ethz.ch 11.4.0 Darwin Kernel Version 11.4.0: Mon Apr > > 9 19:32:15 PDT 2012; root:xnu-1699.26.8~1/RELEASE_X86_64 x86_64 > > > uname -a > Darwin birne.lan 10.8.0 Darwin Kernel Version 10.8.0: Tue Jun 7 16:33:36 PDT > 2011; root:xnu-1504.15.3~1/RELEASE_I386 i386 $ uname -a Darwin book.hvoigt.net 10.8.0 Darwin Kernel Version 10.8.0: Tue Jun 7 16:33:36 PDT 2011; root:xnu-1504.15.3~1/RELEASE_I386 i386 My bisect run did also not reveal anything useful. I tried to use valgrind. Interestingly just by prepending my valgrind directory to the PATH the test passes. When changing PATH further it sometimes passes and sometimes not. Reopening a new shell it reliably fails. The commit I am experiencing this with is cdd159b. This one reliably fails for me with the correct path setting :-) To me this smells a little bit like using a dangling pointer or uninitialized memory since threading seems to be out of the game. A dangling pointer only available on a certain OS X version? If I modify the PATH after adding the valgrind bin folder so that it matches the same amount of characters as before I get the failure again. It seems the error only occurs if my PATH is exactly 280 characters long. E.g.: export PATH=/usr/local/valgrind/bin:/Users/hvoigt/.local/bin:/usr/bin:/bin:/usr/local/bin:/sw/bin/:/aa When running under valgrind test 13 (the original one) passes but 2 fails. Not sure if this is a false positive[1]. If I add those eight bytes here the tests pass for me without valgrind: diff --git a/environment.c b/environment.c index 85edd7f..988f836 100644 --- a/environment.c +++ b/environment.c @@ -131,7 +131,7 @@ static void setup_git_env(void) } git_index_file = getenv(INDEX_ENVIRONMENT); if (!git_index_file) { - git_index_file = xmalloc(strlen(git_dir) + 7); + git_index_file = xmalloc(strlen(git_dir) + 7 + 8); sprintf(git_index_file, "%s/index", git_dir); } git_graft_file = getenv(GRAFT_ENVIRONMENT); But maybe thats just a coincidence and totally unrelated. The valgrind error can be fixed by this change: diff --git a/sha1_file.c b/sha1_file.c index 4ccaf7a..631d0dd 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -251,7 +251,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative const char *objdir = get_object_directory(); struct alternate_object_database *ent; struct alternate_object_database *alt; - int pfxlen, entlen; + int pfxlen, entlen, objdirlen; struct strbuf pathbuf = STRBUF_INIT; if (!is_absolute_path(entry) && relative_base) { @@ -298,7 +298,8 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative return -1; } } - if (!memcmp(ent->base, objdir, pfxlen)) { + objdirlen = strlen(objdir); + if (!memcmp(ent->base, objdir, pfxlen > objdirlen ? objdirlen : pfxlen)) { free(ent); return -1; } I will format this into a proper patch independent from this mail. These are my observations. Cheers Heiko [1] $ ./t1450-fsck.sh --valgrind [...] expecting success: mkdir another && ( cd another && git init && echo ../../../.git/objects >.git/objects/info/alternates && test_commit C fileC one && git fsck --no-dangling >../actual 2>&1 ) && test_cmp empty actual Initialized empty Git repository in /Users/hvoigt/Repository/git/t/trash directory.t1450-fsck/another/.git/ ==42686== Invalid read of size 8 ==42686==at 0x100625064: bcmp (in /usr/lib/libSystem.B.dylib) ==42686==by 0x100112846: link_alt_odb_entries (in /Users/hvoigt/Repository/git/t/valgrind/../../git) ==42686==by 0x1001129C0: read_info_alternates (in /Users/hvoigt/Repository/git/t/valgrind/../../git) ==42686==by 0x100112B8C: prepare_alt_odb (in /Users/hvoigt/Repository/git/t/valgrind/../../git) ==42686==by 0x1001148B7: prepare_packed_git (in /Users/hvoigt/Repository/git/t/valgrind/../../git) ==42686==by 0x100116A8B: find_pack_entry (in /Users/hvoigt/Repository/git/t/valgrind/../../git) ==42686==by 0x100118008: has_sha1_file (in /Users/hvoigt/Repositor
[PATCH] t: add missing executable bit to t7409
Signed-off-by: Jeff King --- 0 files changed mode change 100644 => 100755 t/t7409-submodule-detached-worktree.sh diff --git a/t/t7409-submodule-detached-worktree.sh b/t/t7409-submodule-detached-worktree.sh old mode 100644 new mode 100755 -- 1.7.12.rc0.15.g2184f59 -- 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 3/3] fsck: detect null sha1 in tree entries
Short of somebody happening to beat the 1 in 2^160 odds of actually generating content that hashes to the null sha1, we should never see this value in a tree entry. So let's have fsck warn if it it seen. As in the previous commit, we test both blob and submodule entries to future-proof the test suite against the implementation depending on connectivity to notice the error. Signed-off-by: Jeff King --- I left this as a warning, because git can still mostly handle such trees, which may aid in debugging or salvaging data. fsck.c | 8 +++- t/t1450-fsck.sh | 26 ++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/fsck.c b/fsck.c index 4c63b2c..7395ef6 100644 --- a/fsck.c +++ b/fsck.c @@ -139,6 +139,7 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con static int fsck_tree(struct tree *item, int strict, fsck_error error_func) { int retval; + int has_null_sha1 = 0; int has_full_path = 0; int has_empty_name = 0; int has_zero_pad = 0; @@ -157,9 +158,12 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func) while (desc.size) { unsigned mode; const char *name; + const unsigned char *sha1; - tree_entry_extract(&desc, &name, &mode); + sha1 = tree_entry_extract(&desc, &name, &mode); + if (is_null_sha1(sha1)) + has_null_sha1 = 1; if (strchr(name, '/')) has_full_path = 1; if (!*name) @@ -207,6 +211,8 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func) } retval = 0; + if (has_null_sha1) + retval += error_func(&item->object, FSCK_WARN, "contains entries pointing to null sha1"); if (has_full_path) retval += error_func(&item->object, FSCK_WARN, "contains full pathnames"); if (has_empty_name) diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh index 5b79c51..bf7a2cd 100755 --- a/t/t1450-fsck.sh +++ b/t/t1450-fsck.sh @@ -213,4 +213,30 @@ test_expect_success 'rev-list --verify-objects with bad sha1' ' grep -q "error: sha1 mismatch 63ff" out ' +_bz='\0' +_bz5="$_bz$_bz$_bz$_bz$_bz" +_bz20="$_bz5$_bz5$_bz5$_bz5" + +test_expect_success 'fsck notices blob entry pointing to null sha1' ' + (git init null-blob && +cd null-blob && +sha=$(printf "100644 file$_bz$_bz20" | + git hash-object -w --stdin -t tree) && + git fsck 2>out && + cat out && + grep "warning.*null sha1" out + ) +' + +test_expect_success 'fsck notices submodule entry pointing to null sha1' ' + (git init null-commit && +cd null-commit && +sha=$(printf "16 submodule$_bz$_bz20" | + git hash-object -w --stdin -t tree) && + git fsck 2>out && + cat out && + grep "warning.*null sha1" out + ) +' + test_done -- 1.7.11.3.42.g90758bf -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] do not write null sha1s to on-disk index
We should never need to write the null sha1 into an index entry (short of the 1 in 2^160 chance that somebody actually has content that hashes to it). If we attempt to do so, it is much more likely that it is a bug, since we use the null sha1 as a sentinel value to mean "not valid". The presence of null sha1s in the index (which can come from, among other things, "update-index --cacheinfo", or by reading a corrupted tree) can cause problems for later readers, because they cannot distinguish the literal null sha1 from its use a sentinel value. For example, "git diff-files" on such an entry would make it appear as if it is stat-dirty, and until recently, the diff code assumed such an entry meant that we should be diffing a working tree file rather than a blob. Ideally, we would stop such entries from entering even our in-core index. However, we do sometimes legitimately add entries with null sha1s in order to represent these sentinel situations; simply forbidding them in add_index_entry breaks a lot of the existing code. However, we can at least make sure that our in-core sentinel representation never makes it to disk. To be thorough, we will test an attempt to add both a blob and a submodule entry. In the former case, we might run into problems anyway because we will be missing the blob object. But in the latter case, we do not enforce connectivity across gitlink entries, making this our only point of enforcement. The current implementation does not care which type of entry we are seeing, but testing both cases helps future-proof the test suite in case that changes. Signed-off-by: Jeff King --- I confess to not being clear on all of the instances in which a null sha1 might enter the in-core index. I did try modifying add_index_entry, but the breakage was pretty severe. I traced through a few instances, and it seemed to be mostly related to unmerged entries. read-cache.c | 2 ++ t/t2107-update-index-basic.sh | 19 +++ 2 files changed, 21 insertions(+) diff --git a/read-cache.c b/read-cache.c index 2f8159f..d2be78e 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1800,6 +1800,8 @@ int write_index(struct index_state *istate, int newfd) continue; if (!ce_uptodate(ce) && is_racy_timestamp(istate, ce)) ce_smudge_racily_clean_entry(ce); + if (is_null_sha1(ce->sha1)) + return error("cache entry has null sha1: %s", ce->name); if (ce_write_entry(&c, newfd, ce, previous_name) < 0) return -1; } diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh index 809fafe..0dbbb00 100755 --- a/t/t2107-update-index-basic.sh +++ b/t/t2107-update-index-basic.sh @@ -29,4 +29,23 @@ test_expect_success 'update-index -h with corrupt index' ' grep "[Uu]sage: git update-index" broken/usage ' +test_expect_success '--cacheinfo does not accept blob null sha1' ' + echo content >file && + git add file && + git rev-parse :file >expect && + test_must_fail git update-index --cacheinfo 100644 $_z40 file && + git rev-parse :file >actual && + test_cmp expect actual +' + +test_expect_success '--cacheinfo does not accept gitlink null sha1' ' + git init submodule && + (cd submodule && test_commit foo) && + git add submodule && + git rev-parse :submodule >expect && + test_must_fail git update-index --cacheinfo 16 $_z40 submodule && + git rev-parse :submodule >actual && + test_cmp expect actual +' + test_done -- 1.7.11.3.42.g90758bf -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] diff: do not use null sha1 as a sentinel value
The diff code represents paths using the diff_filespec struct. This struct has a sha1 to represent the sha1 of the content at that path, as well as a sha1_valid member which indicates whether its sha1 field is actually useful. If sha1_valid is not true, then the filespec represents a working tree file (e.g., for the no-index case, or for when the index is not up-to-date). The diff_filespec is only used internally, though. At the interfaces to the diff subsystem, callers feed the sha1 directly, and we create a diff_filespec from it. It's at that point that we look at the sha1 and decide whether it is valid or not; callers may pass the null sha1 as a sentinel value to indicate that it is not. We should not typically see the null sha1 coming from any other source (e.g., in the index itself, or from a tree). However, a corrupt tree might have a null sha1, which would cause "diff --patch" to accidentally diff the working tree version of a file instead of treating it as a blob. This patch extends the edges of the diff interface to accept a "sha1_valid" flag whenever we accept a sha1, and to use that flag when creating a filespec. In some cases, this means passing the flag through several layers, making the code change larger than would be desirable. One alternative would be to simply die() upon seeing corrupted trees with null sha1s. However, this fix more directly addresses the problem (while bogus sha1s in a tree are probably a bad thing, it is really the sentinel confusion sending us down the wrong code path that is what makes it devastating). And it means that git is more capable of examining and debugging these corrupted trees. For example, you can still "diff --raw" such a tree to find out when the bogus entry was introduced; you just cannot do a "--patch" diff (just as you could not with any other corrupted tree, as we do not have any content to diff). Signed-off-by: Jeff King --- builtin.h | 2 +- builtin/blame.c| 9 ++--- builtin/cat-file.c | 2 +- builtin/diff.c | 8 +++-- combine-diff.c | 4 +-- diff-lib.c | 20 ++- diff-no-index.c| 2 +- diff.c | 16 + diff.h | 5 +++ diffcore-rename.c | 2 +- diffcore.h | 2 +- revision.c | 2 ++ t/t4054-diff-bogus-tree.sh | 83 ++ tree-diff.c| 8 ++--- 14 files changed, 134 insertions(+), 31 deletions(-) create mode 100755 t/t4054-diff-bogus-tree.sh diff --git a/builtin.h b/builtin.h index ba6626b..8e37752 100644 --- a/builtin.h +++ b/builtin.h @@ -43,7 +43,7 @@ extern int check_pager_config(const char *cmd); struct diff_options; extern void setup_diff_pager(struct diff_options *); -extern int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, char **buf, unsigned long *buf_size); +extern int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, int sha1_valid, char **buf, unsigned long *buf_size); extern int cmd_add(int argc, const char **argv, const char *prefix); extern int cmd_annotate(int argc, const char **argv, const char *prefix); diff --git a/builtin/blame.c b/builtin/blame.c index 0d50273..f2c48ef 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -110,6 +110,7 @@ static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b, long ctxlen, int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, + int sha1_valid, char **buf, unsigned long *buf_size) { @@ -117,7 +118,7 @@ int textconv_object(const char *path, struct userdiff_driver *textconv; df = alloc_filespec(path); - fill_filespec(df, sha1, mode); + fill_filespec(df, sha1, sha1_valid, mode); textconv = get_textconv(df); if (!textconv) { free_filespec(df); @@ -142,7 +143,7 @@ static void fill_origin_blob(struct diff_options *opt, num_read_blob++; if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) && - textconv_object(o->path, o->mode, o->blob_sha1, &file->ptr, &file_size)) + textconv_object(o->path, o->mode, o->blob_sha1, 1, &file->ptr, &file_size)) ; else file->ptr = read_sha1_file(o->blob_sha1, &type, &file_size); @@ -2123,7 +2124,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, switch (st.st_mode & S_IFMT) { case S_IFREG: if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) && - textconv_object(read_from, mode, null_sha1, &buf_ptr, &buf_len)) + textconv_object(read_from, mode, null_sha1, 0, &buf_ptr, &buf_len)) strbuf_attach(&bu
[PATCH 0/3] null sha1 in trees
I recently came across a tree in the wild that had a submodule entry whose sha1 was the null sha1 (i.e., all-zeros). It triggered an interesting bug in the diff code, which is fixed by patch 1. Unfortunately, I have no clue how this tree came about. I'm assuming it was simply a bug somewhere in git, where the entry should have had another sha1, or possibly been removed from the tree entirely.. Patches 2 and 3 tighten up our checks for null sha1s in a few places, which might help detect such a bug earlier. I assume I have never seen this with a non-submodule entry because such a tree would fail the usual connectivity checks during fsck or during a transfer. However, since we don't enforce connectivity on submodule entries, nothing blocked the creation and propagation of such an entry. I'm not at liberty to share the repository in question, but if anybody has specific things to look for, I'd be happy to investigate further. The patches are: [1/3]: diff: do not use null sha1 as a sentinel value This is the actual bug-fix, and I hope is obviously a good thing to do. [2/3]: do not write null sha1s to on-disk index This one tries to tighten our writing a bit. There are unfortunately a lot of different code paths that create trees in git. I hope by catching the index write as a choke-point, we can prevent bugs from spreading. However, there are a lot of tree-writers that update an index in-core and then write a tree out directly. I would not be surprised if this does not catch the bug by itself, but I think it is a step in the right direction. [3/3]: fsck: detect null sha1 in tree entries And this one will at least let us notice the bug once it has happened. And if transfer.fsckObjects is set, it will prevent bogus trees from passing between repositories, containing any damage. -Peff -- 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/8] SVN 1.7 will truncate "not-a%40{0}" to just "not-a".
Michael G. Schwern wrote: > Rather than guess what SVN is going to do for each version, make the test use > the branch name that was actually created. [...] > - git rev-parse "refs/remotes/not-a%40{0}reflog" > + git rev-parse "refs/remotes/$non_reflog" Doesn't this defeat the point of the testcase (checking that git-svn is able to avoid creating git refs containing @{, following the rules from git-check-ref-format(1))? Do you know when SVN truncates the directory name? Would historical SVN repositories or historical SVN servers be able to have a directory named with a %40 in it, or has this been disallowed completely, leaving problematic historical repositories to be dumped with old SVN, tweaked, and reloaded with new SVN? Thanks, Jonathan -- 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 7/7] Make Git::SVN and Git::SVN::Ra canonicalize paths and urls.
Michael G. Schwern wrote: > This canonicalizes paths and urls as early as possible so we don't > have to remember to do it at the point of use. Yay! Am I correct in imagining this makes the following sequence of commands[1] no longer trip an assertion failure in svn_path_join[2] with SVN 1.6? git svn init -Thttp://trac-hacks.org/svn/tagsplugin/trunk \ -thttp://trac-hacks.org/svn/tagsplugin/tags \ -bhttp://trac-hacks.org/svn/tagsplugin/branches git svn fetch [1] http://bugs.debian.org/616168 [2] $ git svn fetch W: Ignoring error from SVN, path probably does not exist: (160013): Filesystem has no item: File not found: revision 100, path '/tagsplugin' W: Do not be alarmed at the above message git-svn is just searching aggressively for old history. This may take a while on large repositories perl: /build/buildd-subversion_1.6.17dfsg-4-i386-MgNPeW/subversion-1.6.17dfsg/subversion/libsvn_subr/path.c:115: svn_path_join: Assertion `svn_path_is_canonical(component, pool)' failed. error: git-svn died of signal 6 -- 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 6/7] Switch path canonicalization to use the SVN API.
Michael G. Schwern wrote: > --- a/perl/Git/SVN/Utils.pm > +++ b/perl/Git/SVN/Utils.pm > @@ -86,6 +86,27 @@ sub _collapse_dotdot { > > > sub canonicalize_path { > + my $path = shift; > + > + # The 1.7 way to do it > + if ( defined &SVN::_Core::svn_dirent_canonicalize ) { > + $path = _collapse_dotdot($path); > + return SVN::_Core::svn_dirent_canonicalize($path); > + } > + # The 1.6 way to do it > + elsif ( defined &SVN::_Core::svn_path_canonicalize ) { > + $path = _collapse_dotdot($path); > + return SVN::_Core::svn_path_canonicalize($path); > + } > + # No SVN API canonicalization is available, do it ourselves > + else { When would this "else" case trip? Would it be safe to make it return an error message, or even to do something like the following? sub canonicalize_path { my $path = shift; $path = _collapse_dotdot($path); # Subversion 1.7 split svn_path_canonicalize() into # svn_dirent_canonicalize() and svn_uri_canonicalize(). if (!defined &SVN::_Core::svn_dirent_canonicalize) { return SVN::_Core::svn_path_canonicalize($path); } return SVN::_Core::svn_dirent_canonicalize($path); } Thanks, Jonathan -- 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/7] Change canonicalize_url() to use the SVN 1.7 API when available.
Hi, Michael G. Schwern wrote: > --- a/perl/Git/SVN/Utils.pm > +++ b/perl/Git/SVN/Utils.pm [...] > @@ -100,6 +102,20 @@ API as a URL. > =cut > > sub canonicalize_url { > + my $url = shift; > + > + # The 1.7 way to do it > + if ( defined &SVN::_Core::svn_uri_canonicalize ) { > + return SVN::_Core::svn_uri_canonicalize($url); > + } > + # There wasn't a 1.6 way to do it, so we do it ourself. > + else { > + return _canonicalize_url_ourselves($url); > + } > +} > + > + > +sub _canonicalize_url_ourselves { > my ($url) = @_; > $url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e; Leaves me a bit nervous. What effect should we expect this change to have? Is our emulation of svn_uri_canonicalize already perfect and this change just a little futureproofing in case svn_uri_canonicalize gets even better, or is this a trap waiting to happen when new callers of canonicalize_url start relying on, e.g., %-encoding of special characters? If I am reading Subversion r873487 correctly, in ancient times, svn_path_canonicalize() did the appropriate tweaking for URIs. Today its implementation is comforting: const char * svn_path_canonicalize(const char *path, apr_pool_t *pool) { if (svn_path_is_url(path)) return svn_uri_canonicalize(path, pool); else return svn_dirent_canonicalize(path, pool); } It might be easier to rely on that on pre-1.7 systems. Thanks, Jonathan -- 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] buitin_config: return postitive status in get_value
On Sat, Jul 28, 2012 at 04:18:49PM +0300, Nikolay Vladimirov wrote: > But the behavior now seems kind of strange, or maybe I'm missing something: > # git config foobar; echo $? > error: key does not contain a section: foobar > 255 > > # git config foobar.info; echo $? > 1 > > git version 1.7.11.2 > > I would generally expect the both to behave the same way. Then the following patch may be better because it leaves other cases untouched (I'm not saying that we should or should not do it though) -- 8< -- diff --git a/builtin/config.c b/builtin/config.c index 8cd08da..d048ebf 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -199,8 +199,10 @@ static int get_value(const char *key_, const char *regex_) goto free_strings; } } else { - if (git_config_parse_key(key_, &key, NULL)) + if (git_config_parse_key(key_, &key, NULL)) { + ret = 1; goto free_strings; + } } if (regex_) { -- 8< -- -- Duy -- 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] buitin_config: return postitive status in get_value
On Sat, Jul 28, 2012 at 6:42 PM, Nikolai Vladimirov wrote: > Returning -1 instead of 1 results in wrong exit status(255) since > the output of get_value is passed to exit(). > > 'git config missing_section' should now return proper exit status = 1, > as specified by the git config documentation. I'm curious. Why is -1 (or 255) wrong? It was introduced in the first version of get_value in 4ddba79 (git-config-set: add more options - 2005-11-20). Back then it returned -1 when there's regex compile error to distinguish with 0 and 1 (but git-config-set.txt in the same commit did not get update about exit code). Maybe we should update document instead of the code. -- Duy -- 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] cleanup argument passing in submodule status command
In commit 98dbe63 the variable $orig_args was renamed to $orig_flags. One location in cmd_status() was missed. Note: This is a code cleanup and does not fix any bugs. As a side effect the variables containing the parsed flags to "git submodule status" are passed down recursively. So everything was already behaving as expected. Signed-off-by: Heiko Voigt --- git-submodule.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-submodule.sh b/git-submodule.sh index dba4d39..3a3f0a4 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -961,7 +961,7 @@ cmd_status() prefix="$displaypath/" clear_local_git_env cd "$sm_path" && - eval cmd_status "$orig_args" + eval cmd_status "$orig_flags" ) || die "$(eval_gettext "Failed to recurse into submodule path '\$sm_path'")" fi -- 1.7.12.rc0.23.g3c7cae0 -- 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] buitin_config: return postitive status in get_value
Returning -1 instead of 1 results in wrong exit status(255) since the output of get_value is passed to exit(). 'git config missing_section' should now return proper exit status = 1, as specified by the git config documentation. Signed-off-by: Nikolai Vladimirov --- builtin/config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/config.c b/builtin/config.c index 8cd08da..c262cbb 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -160,7 +160,7 @@ static int show_config(const char *key_, const char *value_, void *cb) static int get_value(const char *key_, const char *regex_) { - int ret = -1; + int ret = 1; char *global = NULL, *xdg = NULL, *repo_config = NULL; const char *system_wide = NULL, *local; struct config_include_data inc = CONFIG_INCLUDE_INIT; -- 1.7.11.2 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Enable parallelism in git submodule update.
Hi, On Fri, Jul 27, 2012 at 04:25:58PM -0700, Junio C Hamano wrote: > Stefan Zager writes: > > > On Fri, Jul 27, 2012 at 2:38 PM, Junio C Hamano wrote: > > > >> Stefan Zager writes: > >> > >> > + module_list "$@" | awk '{print $4}' | xargs -L 1 -P > >> "$jobs" git submodule update $orig_args > >> > >> Capital-P option to xargs is not even in POSIX, no? > > > > I wasn't aware of that, but you appear to be correct. Don't know if you > > have a policy about that, but anecdotally, -P is supported on my linux, > > mac, and win/msys systems. > > About "policy", we use POSIX as a rough yardstick to warn us that we > might be breaking people on minority platforms. We do _not_ say "It > is in POSIX, so it is safe to use it", but we say "It is not even in > POSIX, so we need to think twice." We do not usually say "Linux, > Mac and Windows are the only things that matter, and they all > support it." > > Of course, any set of rules have exceptions ;-) There are a few > things to which we say "Even though it is not in POSIX, everybody > who matters supports it, and without taking advantage of it, what we > want to achieve will become too cumbersome to express". I was about to write that since this is limited to a given --jobs options the majority platforms should be enough as a start and others could add a parallelism mechanism later. Its only a matter of efficiency and not features. But if you look at my other post to this thread I described that we need some UI output extension so the user can still make sense of it. In short: The user should be able distinguish which job said what. I was already thinking about how an output caching could be implemented in core git. How about exposing it as a git command like this? git run [-j] ... It works like the xargs call above except that it caches each jobs output to stderr and stdout until its done and then replays the output to stderr/out in the correct order. We could design the code so that it can be reused later on to do the caching in parallel fetch/push/... . What do you think? If we decide to go this route I would have a look into whipping something up. Cheers Heiko -- 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] Enable parallelism in git submodule update.
Hi Stefan, neat patch. See below for a few notes. On Fri, Jul 27, 2012 at 11:37:34AM -0700, Stefan Zager wrote: > diff --git a/git-submodule.sh b/git-submodule.sh > index dba4d39..761420a 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -491,6 +492,20 @@ cmd_update() > -r|--rebase) > update="rebase" > ;; > + -j|--jobs) > + case "$2" in > + ''|-*) > + jobs="0" > + ;; > + *) > + jobs="$2" > + shift > + ;; > + esac > + # Don't preserve this arg. > + shift > + continue > + ;; > --reference) > case "$2" in '') usage ;; esac > reference="--reference=$2" > @@ -529,6 +544,12 @@ cmd_update() > cmd_init "--" "$@" || return > fi > > + if test "$jobs" != "1" > + then > + module_list "$@" | awk '{print $4}' | xargs -L 1 -P "$jobs" git > submodule update $orig_args I do not see orig_args set anywhere in submodule.sh. It seems the existing usage of it in cmd_status() is a leftover from commit 98dbe63 when this variable got renamed to orig_flags. I will follow up with a patch to that location. Another problem here is the passing of arguments. Have a look at a7eff1a8 to see how this was solved for other locations. The next thing I noticed is that the parallelism is not recursive. You drop the option and only execute the first depth in parallel. How about using the amount of modules defined by arguments left in $@ as an indicator whether you need to fork parallel execution or not. If there is exactly one you do the update if there are more you do the parallel thing. That way you can just keep passing the --jobs flag to the subprocesses. The next question to solve is UI: Since the output lines of the parallel update jobs will be mixed we need some way to distinguish them. Imagine one of the update fails somewhere how do we find out which it was? Two possible solutions come to my mind: 1. Prefix each line with a job number. This way you can distinguish which process outputted what and still have immediate feedback. 2. Cache the output (to stderr and stdout) of each job and output it once one job is done. I imagine this needs some infrastructure which we need to implement. We already have some ideas how to collect such output in C here[1]. I would prefer solution 2 since the output of 1 will be hard to read but I guess we could start with 1 and then move over to 2 later on. Cheers Heiko [1] http://article.gmane.org/gmane.comp.version-control.git/197747 -- 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 5/8] Canonicalize earlier in a couple spots.
From: "Michael G. Schwern" Just a few things I noticed. Its good to canonicalize as early as possible. --- git-svn.perl | 6 +++--- perl/Git/SVN/Ra.pm | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/git-svn.perl b/git-svn.perl index 6e97545..6b90765 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -1436,16 +1436,16 @@ sub cmd_info { # canonicalize_path() will return "" to make libsvn 1.5.x happy, $path = "." if $path eq ""; - my $full_url = $url . ($fullpath eq "" ? "" : "/$fullpath"); + my $full_url = canonicalize_url( $url . ($fullpath eq "" ? "" : "/$fullpath") ); if ($_url) { - print canonicalize_url($full_url), "\n"; + print "$full_url\n"; return; } my $result = "Path: $path\n"; $result .= "Name: " . basename($path) . "\n" if $file_type ne "dir"; - $result .= "URL: " . canonicalize_url($full_url) . "\n"; + $result .= "URL: $full_url\n"; eval { my $repos_root = $gs->repos_root; diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm index ed9dbe9..eee7c00 100644 --- a/perl/Git/SVN/Ra.pm +++ b/perl/Git/SVN/Ra.pm @@ -69,7 +69,7 @@ sub _auth_providers () { sub new { my ($class, $url) = @_; - $url =~ s!/+$!!; + $url = canonicalize_url($url); return $RA if ($RA && $RA->url eq $url); ::_req_svn(); @@ -101,7 +101,7 @@ sub new { $Git::SVN::Prompt::_no_auth_cache = 1; } } # no warnings 'once' - my $self = SVN::Ra->new(url => canonicalize_url($url), auth => $baton, + my $self = SVN::Ra->new(url => $url, auth => $baton, config => $config, pool => SVN::Pool->new, auth_provider_callbacks => $callbacks); -- 1.7.11.3 -- 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 3/8] Improve our URL canonicalization to be more like SVN 1.7's.
From: "Michael G. Schwern" Previously, our URL canonicalization didn't do much of anything. Now it actually escapes and collapses slashes. This is mostly a cut & paste of escape_url from git-svn. This is closer to how SVN 1.7's canonicalization behaves. Doing it with 1.6 lets us chase down some problems caused by more effective canonicalization without having to deal with all the other 1.7 issues on top of that. * Remote URLs have to be canonicalized otherwise Git::SVN->find_existing_remote will think they're different. * The SVN remote is now written to the git config canonicalized. That should be ok. Adjust a test to account for that. --- perl/Git/SVN.pm| 4 ++-- perl/Git/SVN/Utils.pm | 19 +-- t/Git-SVN/Utils/canonicalize_url.t | 26 ++ t/t9107-git-svn-migrate.sh | 4 +++- 4 files changed, 48 insertions(+), 5 deletions(-) create mode 100644 t/Git-SVN/Utils/canonicalize_url.t diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm index 798f6c4..cb6d83a 100644 --- a/perl/Git/SVN.pm +++ b/perl/Git/SVN.pm @@ -201,9 +201,9 @@ sub read_all_remotes { } elsif (m!^(.+)\.usesvmprops=\s*(.*)\s*$!) { $r->{$1}->{svm} = {}; } elsif (m!^(.+)\.url=\s*(.*)\s*$!) { - $r->{$1}->{url} = $2; + $r->{$1}->{url} = canonicalize_url($2); } elsif (m!^(.+)\.pushurl=\s*(.*)\s*$!) { - $r->{$1}->{pushurl} = $2; + $r->{$1}->{pushurl} = canonicalize_url($2); } elsif (m!^(.+)\.ignore-refs=\s*(.*)\s*$!) { $r->{$1}->{ignore_refs_regex} = $2; } elsif (m!^(.+)\.(branches|tags)=$svn_refspec$!) { diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm index 7ae6fac..dab6e4d 100644 --- a/perl/Git/SVN/Utils.pm +++ b/perl/Git/SVN/Utils.pm @@ -147,10 +147,25 @@ sub canonicalize_url { } +sub _canonicalize_url_path { + my ($uri_path) = @_; + + my @parts; + foreach my $part (split m{/+}, $uri_path) { + $part =~ s/([^~\w.%+-]|%(?![a-fA-F0-9]{2}))/sprintf("%%%02X",ord($1))/eg; + push @parts, $part; + } + + return join('/', @parts); +} + sub _canonicalize_url_ourselves { my ($url) = @_; - $url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e; - return $url; + if ($url =~ m#^([^:]+)://([^/]*)(.*)$#) { + my ($scheme, $domain, $uri) = ($1, $2, _canonicalize_url_path(canonicalize_path($3))); + $url = "$scheme://$domain$uri"; + } + $url; } diff --git a/t/Git-SVN/Utils/canonicalize_url.t b/t/Git-SVN/Utils/canonicalize_url.t new file mode 100644 index 000..05795ab --- /dev/null +++ b/t/Git-SVN/Utils/canonicalize_url.t @@ -0,0 +1,26 @@ +#!/usr/bin/env perl + +# Test our own home rolled URL canonicalizer. Test the private one +# directly because we can't predict what the SVN API is doing to do. + +use strict; +use warnings; + +use Test::More 'no_plan'; + +use Git::SVN::Utils; +my $canonicalize_url = \&Git::SVN::Utils::_canonicalize_url_ourselves; + +my %tests = ( + "http://x.com"; => "http://x.com";, + "http://x.com/"; => "http://x.com";, + "http://x.com/foo/bar"; => "http://x.com/foo/bar";, + "http://x.com//foo//bar//"; => "http://x.com/foo/bar";, + "http://x.com/ /%/"=> "http://x.com/%20%20/%25";, +); + +for my $arg (keys %tests) { + my $want = $tests{$arg}; + + is $canonicalize_url->($arg), $want, "canonicalize_url('$arg') => $want"; +} diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh index cfb4453..ee73013 100755 --- a/t/t9107-git-svn-migrate.sh +++ b/t/t9107-git-svn-migrate.sh @@ -27,6 +27,8 @@ test_expect_success 'setup old-looking metadata' ' head=`git rev-parse --verify refs/heads/git-svn-HEAD^0` test_expect_success 'git-svn-HEAD is a real HEAD' "test -n '$head'" +svnrepo_escaped=`echo $svnrepo | sed 's/ /%20/'` + test_expect_success 'initialize old-style (v0) git svn layout' ' mkdir -p "$GIT_DIR"/git-svn/info "$GIT_DIR"/svn/info && echo "$svnrepo" > "$GIT_DIR"/git-svn/info/url && @@ -35,7 +37,7 @@ test_expect_success 'initialize old-style (v0) git svn layout' ' ! test -d "$GIT_DIR"/git-svn && git rev-parse --verify refs/${remotes_git_svn}^0 && git rev-parse --verify refs/remotes/svn^0 && - test "$(git config --get svn-remote.svn.url)" = "$svnrepo" && + test "$(git config --get svn-remote.svn.url)" = "$svnrepo_escaped" && test `git config --get svn-remote.svn.fetch` = \ ":refs/${remotes_git_svn}" ' -- 1.7.11.3 -- 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.htm
[PATCH 7/8] Turn on canonicalization on newly minted URLs.
From: "Michael G. Schwern" Go through all the spots that use the new add_path_to_url() to make a new URL and canonicalize them. * copyfrom_path has to be canonicalized else find_parent_branch will get confused * due to the `canonicalize_url($full_url) ne $full_url)` line of logic in gs_do_switch(), $full_url is left alone until after. At this point SVN 1.7 passes except for 3 tests in t9100-git-svn-basic.sh that look like an SVN bug to do with symlinks. --- perl/Git/SVN.pm| 19 ++- perl/Git/SVN/Ra.pm | 9 - 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm index 22bf207..e5f7acc 100644 --- a/perl/Git/SVN.pm +++ b/perl/Git/SVN.pm @@ -362,6 +362,8 @@ sub init_remote_config { sub find_by_url { # repos_root and, path are optional my ($class, $full_url, $repos_root, $path) = @_; + $full_url = canonicalize_url($full_url); + return undef unless defined $full_url; remove_username($full_url); remove_username($repos_root) if defined $repos_root; @@ -400,6 +402,11 @@ sub find_by_url { # repos_root and, path are optional } $p =~ s#^\Q$z\E(?:/|$)#$prefix# or next; } + + # remote fetch paths are not URI escaped. Decode ours + # so they match + $p = uri_decode($p); + foreach my $f (keys %$fetch) { next if $f ne $p; return Git::SVN->new($fetch->{$f}, $repo_id, $f); @@ -934,18 +941,18 @@ sub rewrite_uuid { sub metadata_url { my ($self) = @_; my $url = $self->rewrite_root || $self->url; - return add_path_to_url( $url, $self->path ); + return canonicalize_url( add_path_to_url( $url, $self->path ) ); } sub full_url { my ($self) = @_; - return add_path_to_url( $self->url, $self->path ); + return canonicalize_url( add_path_to_url( $self->url, $self->path ) ); } sub full_pushurl { my ($self) = @_; if ($self->{pushurl}) { - return add_path_to_url( $self->{pushurl}, $self->path ); + return canonicalize_url( add_path_to_url( $self->{pushurl}, $self->path ) ); } else { return $self->full_url; } @@ -1113,7 +1120,7 @@ sub find_parent_branch { my $r = $i->{copyfrom_rev}; my $repos_root = $self->ra->{repos_root}; my $url = $self->ra->url; - my $new_url = add_path_to_url( $url, $branch_from ); + my $new_url = canonicalize_url( add_path_to_url( $url, $branch_from ) ); print STDERR "Found possible branch point: ", "$new_url => ", $self->full_url, ", $r\n" unless $::_q > 1; @@ -1869,7 +1876,9 @@ sub make_log_entry { $email ||= "$author\@$uuid"; $commit_email ||= "$author\@$uuid"; } elsif ($self->use_svnsync_props) { - my $full_url = add_path_to_url( $self->svnsync->{url}, $self->path ); + my $full_url = canonicalize_url( + add_path_to_url( $self->svnsync->{url}, $self->path ) + ); remove_username($full_url); my $uuid = $self->svnsync->{uuid}; $log_entry{metadata} = "$full_url\@$rev $uuid"; diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm index 788e642..29b46e8 100644 --- a/perl/Git/SVN/Ra.pm +++ b/perl/Git/SVN/Ra.pm @@ -5,6 +5,7 @@ use warnings; use SVN::Client; use Git::SVN::Utils qw( canonicalize_url + canonicalize_path add_path_to_url ); @@ -102,6 +103,7 @@ sub new { $Git::SVN::Prompt::_no_auth_cache = 1; } } # no warnings 'once' + my $self = SVN::Ra->new(url => $url, auth => $baton, config => $config, pool => SVN::Pool->new, @@ -200,6 +202,7 @@ sub get_log { qw/copyfrom_path copyfrom_rev action/; if ($s{'copyfrom_path'}) { $s{'copyfrom_path'} =~ s/$prefix_regex//; + $s{'copyfrom_path'} = canonicalize_path($s{'copyfrom_path'}); } $_[0]{$p} = \%s; } @@ -303,7 +306,11 @@ sub gs_do_switch { $ra = Git::SVN::Ra->new($full_url); $ra_invalid = 1; } elsif ($old_url ne $full_url) { - SVN::_Ra::svn_ra_reparent($self->{session}, $full_url, $pool); + SVN::_Ra::svn_ra_reparent( + $self->{session}, + canonicalize_url($full_url), + $pool + ); $self->url($full_url); $reparented = 1; } -- 1.7.11.3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a mes
[PATCH 6/8] Add function to append a path to a URL.
From: "Michael G. Schwern" Remove the ad-hoc versions. This is mostly to normalize the process and ensure the URLs produced don't have double slashes or anything. Also provides a place to fix the corner case where a file path contains a percent sign. --- git-svn.perl | 3 ++- perl/Git/SVN.pm | 33 +++-- perl/Git/SVN/Ra.pm| 8 perl/Git/SVN/Utils.pm | 27 +++ t/Git-SVN/Utils/add_path_to_url.t | 27 +++ 5 files changed, 75 insertions(+), 23 deletions(-) create mode 100644 t/Git-SVN/Utils/add_path_to_url.t diff --git a/git-svn.perl b/git-svn.perl index 6b90765..3d120d5 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -35,6 +35,7 @@ use Git::SVN::Utils qw( canonicalize_path canonicalize_url join_paths + add_path_to_url ); use Git qw( @@ -1436,7 +1437,7 @@ sub cmd_info { # canonicalize_path() will return "" to make libsvn 1.5.x happy, $path = "." if $path eq ""; - my $full_url = canonicalize_url( $url . ($fullpath eq "" ? "" : "/$fullpath") ); + my $full_url = canonicalize_url( add_path_to_url( $url, $fullpath ) ); if ($_url) { print "$full_url\n"; diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm index 4219e5b..22bf207 100644 --- a/perl/Git/SVN.pm +++ b/perl/Git/SVN.pm @@ -29,6 +29,7 @@ use Git::SVN::Utils qw( join_paths canonicalize_path canonicalize_url + add_path_to_url ); my $can_use_yaml; @@ -564,8 +565,7 @@ sub _set_svm_vars { # username is of no interest $src =~ s{(^[a-z\+]*://)[^/@]*@}{$1}; - my $replace = $ra->url; - $replace .= "/$path" if length $path; + my $replace = add_path_to_url($ra->url, $path); my $section = "svn-remote.$self->{repo_id}"; tmp_config("$section.svm-source", $src); @@ -582,7 +582,7 @@ sub _set_svm_vars { my $path = $self->path; my %tried; while (length $path) { - my $try = $self->url . "/$path"; + my $try = add_path_to_url($self->url, $path); unless ($tried{$try}) { return $ra if $self->read_svm_props($ra, $path, $r); $tried{$try} = 1; @@ -591,7 +591,7 @@ sub _set_svm_vars { } die "Path: '$path' should be ''\n" if $path ne ''; return $ra if $self->read_svm_props($ra, $path, $r); - $tried{$self->url."/$path"} = 1; + $tried{ add_path_to_url($self->url, $path) } = 1; if ($ra->{repos_root} eq $self->url) { die @err, (map { " $_\n" } keys %tried), "\n"; @@ -603,7 +603,7 @@ sub _set_svm_vars { $path = $ra->{svn_path}; $ra = Git::SVN::Ra->new($ra->{repos_root}); while (length $path) { - my $try = $ra->url ."/$path"; + my $try = add_path_to_url($ra->url, $path); unless ($tried{$try}) { $ok = $self->read_svm_props($ra, $path, $r); last if $ok; @@ -613,7 +613,7 @@ sub _set_svm_vars { } die "Path: '$path' should be ''\n" if $path ne ''; $ok ||= $self->read_svm_props($ra, $path, $r); - $tried{$ra->url ."/$path"} = 1; + $tried{ add_path_to_url($ra->url, $path) } = 1; if (!$ok) { die @err, (map { " $_\n" } keys %tried), "\n"; } @@ -933,20 +933,19 @@ sub rewrite_uuid { sub metadata_url { my ($self) = @_; - ($self->rewrite_root || $self->url) . - (length $self->path ? '/' . $self->path : ''); + my $url = $self->rewrite_root || $self->url; + return add_path_to_url( $url, $self->path ); } sub full_url { my ($self) = @_; - $self->url . (length $self->path ? '/' . $self->path : ''); + return add_path_to_url( $self->url, $self->path ); } sub full_pushurl { my ($self) = @_; if ($self->{pushurl}) { - return $self->{pushurl} . (length $self->path ? '/' . - $self->path : ''); + return add_path_to_url( $self->{pushurl}, $self->path ); } else { return $self->full_url; } @@ -1114,7 +1113,7 @@ sub find_parent_branch { my $r = $i->{copyfrom_rev}; my $repos_root = $self->ra->{repos_root}; my $url = $self->ra->url; - my $new_url = $url . $branch_from; + my $new_url = add_path_to_url( $url, $branch_from ); print STDERR "Found possible branch point: ", "$new_url => ", $self->full_url, ", $r\n" unless $::_q > 1; @@ -1443,12 +1442,11 @@ sub find_extra_svk_parents { for my $ticket ( @tickets ) { my ($uuid, $path, $rev) = split /:/, $ticket; if ( $uuid eq $self->ra_uuid ) { -
[PATCH 8/8] Remove some ad hoc canonicalizations.
From: "Michael G. Schwern" --- git-svn.perl| 8 perl/Git/SVN.pm | 1 - 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/git-svn.perl b/git-svn.perl index 3d120d5..56d1ba7 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -36,6 +36,7 @@ use Git::SVN::Utils qw( canonicalize_url join_paths add_path_to_url + join_paths ); use Git qw( @@ -1598,7 +1599,7 @@ sub post_fetch_checkout { sub complete_svn_url { my ($url, $path) = @_; - $path =~ s#/+$##; + $path = canonicalize_path($path); # If the path is not a URL... if ($path !~ m#^[a-z\+]+://#) { @@ -1617,7 +1618,7 @@ sub complete_url_ls_init { print STDERR "W: $switch not specified\n"; return; } - $repo_path =~ s#/+$##; + $repo_path = canonicalize_path($repo_path); if ($repo_path =~ m#^[a-z\+]+://#) { $ra = Git::SVN::Ra->new($repo_path); $repo_path = ''; @@ -1638,9 +1639,8 @@ sub complete_url_ls_init { } command_oneline('config', $k, $gs->url) unless $orig_url; - my $remote_path = $gs->path . "/$repo_path"; + my $remote_path = join_paths( $gs->path, $repo_path ); $remote_path =~ s{%([0-9A-F]{2})}{chr hex($1)}ieg; - $remote_path =~ s#/+#/#g; $remote_path =~ s#^/##g; $remote_path .= "/*" if $remote_path !~ /\*/; my ($n) = ($switch =~ /^--(\w+)/); diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm index e5f7acc..3c68c09 100644 --- a/perl/Git/SVN.pm +++ b/perl/Git/SVN.pm @@ -460,7 +460,6 @@ sub new { } { my $path = $self->path; - $path =~ s{/+}{/}g; $path =~ s{\A/}{}; $path =~ s{/\z}{}; $self->path($path); -- 1.7.11.3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/8] SVN 1.7 will truncate "not-a%40{0}" to just "not-a".
From: "Michael G. Schwern" Rather than guess what SVN is going to do for each version, make the test use the branch name that was actually created. --- t/t9118-git-svn-funky-branch-names.sh | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/t/t9118-git-svn-funky-branch-names.sh b/t/t9118-git-svn-funky-branch-names.sh index 63fc982..193d3ca 100755 --- a/t/t9118-git-svn-funky-branch-names.sh +++ b/t/t9118-git-svn-funky-branch-names.sh @@ -32,6 +32,11 @@ test_expect_success 'setup svnrepo' ' start_httpd ' +# SVN 1.7 will truncate "not-a%40{0]" to just "not-a". +# Look at what SVN wound up naming the branch and use that. +# Be sure to escape the @ if it shows up. +non_reflog=`svn_cmd ls "$svnrepo/pr ject/branches" | grep not-a | sed 's/\///' | sed 's/@/%40/'` + test_expect_success 'test clone with funky branch names' ' git svn clone -s "$svnrepo/pr ject" project && ( @@ -42,7 +47,7 @@ test_expect_success 'test clone with funky branch names' ' git rev-parse "refs/remotes/%2Eleading_dot" && git rev-parse "refs/remotes/trailing_dot%2E" && git rev-parse "refs/remotes/trailing_dotlock%2Elock" && - git rev-parse "refs/remotes/not-a%40{0}reflog" + git rev-parse "refs/remotes/$non_reflog" ) ' -- 1.7.11.3 -- 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 4/8] Replace hand rolled URL escapes with canonicalization
From: "Michael G. Schwern" Continuing to move towards getting everything canonicalizing the same way. * Git::SVN->init_remote_config and Git::SVN::Ra->minimize_url both have to canonicalize the same way else init_remote_config will incorrectly think they're different URLs causing t9107-git-svn-migrate.sh to fail. --- git-svn.perl | 24 +++- perl/Git/SVN.pm| 2 +- perl/Git/SVN/Ra.pm | 27 +-- 3 files changed, 9 insertions(+), 44 deletions(-) diff --git a/git-svn.perl b/git-svn.perl index 6e3e240..6e97545 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -1412,24 +1412,6 @@ sub cmd_commit_diff { } } -sub escape_uri_only { - my ($uri) = @_; - my @tmp; - foreach (split m{/}, $uri) { - s/([^~\w.%+-]|%(?![a-fA-F0-9]{2}))/sprintf("%%%02X",ord($1))/eg; - push @tmp, $_; - } - join('/', @tmp); -} - -sub escape_url { - my ($url) = @_; - if ($url =~ m#^([^:]+)://([^/]*)(.*)$#) { - my ($scheme, $domain, $uri) = ($1, $2, escape_uri_only($3)); - $url = "$scheme://$domain$uri"; - } - $url; -} sub cmd_info { my $path = canonicalize_path(defined($_[0]) ? $_[0] : "."); @@ -1457,18 +1439,18 @@ sub cmd_info { my $full_url = $url . ($fullpath eq "" ? "" : "/$fullpath"); if ($_url) { - print escape_url($full_url), "\n"; + print canonicalize_url($full_url), "\n"; return; } my $result = "Path: $path\n"; $result .= "Name: " . basename($path) . "\n" if $file_type ne "dir"; - $result .= "URL: " . escape_url($full_url) . "\n"; + $result .= "URL: " . canonicalize_url($full_url) . "\n"; eval { my $repos_root = $gs->repos_root; Git::SVN::remove_username($repos_root); - $result .= "Repository Root: " . escape_url($repos_root) . "\n"; + $result .= "Repository Root: " . canonicalize_url($repos_root) . "\n"; }; if ($@) { $result .= "Repository Root: (offline)\n"; diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm index cb6d83a..4219e5b 100644 --- a/perl/Git/SVN.pm +++ b/perl/Git/SVN.pm @@ -296,7 +296,7 @@ sub find_existing_remote { sub init_remote_config { my ($self, $url, $no_write) = @_; - $url =~ s!/+$!!; # strip trailing slash + $url = canonicalize_url($url); my $r = read_all_remotes(); my $existing = find_existing_remote($url, $r); if ($existing) { diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm index ef7b3dd..ed9dbe9 100644 --- a/perl/Git/SVN/Ra.pm +++ b/perl/Git/SVN/Ra.pm @@ -66,24 +66,6 @@ sub _auth_providers () { \@rv; } -sub escape_uri_only { - my ($uri) = @_; - my @tmp; - foreach (split m{/}, $uri) { - s/([^~\w.%+-]|%(?![a-fA-F0-9]{2}))/sprintf("%%%02X",ord($1))/eg; - push @tmp, $_; - } - join('/', @tmp); -} - -sub escape_url { - my ($url) = @_; - if ($url =~ m#^(https?)://([^/]+)(.*)$#) { - my ($scheme, $domain, $uri) = ($1, $2, escape_uri_only($3)); - $url = "$scheme://$domain$uri"; - } - $url; -} sub new { my ($class, $url) = @_; @@ -119,7 +101,7 @@ sub new { $Git::SVN::Prompt::_no_auth_cache = 1; } } # no warnings 'once' - my $self = SVN::Ra->new(url => escape_url($url), auth => $baton, + my $self = SVN::Ra->new(url => canonicalize_url($url), auth => $baton, config => $config, pool => SVN::Pool->new, auth_provider_callbacks => $callbacks); @@ -314,7 +296,7 @@ sub gs_do_switch { if ($old_url =~ m#^svn(\+ssh)?://# || ($full_url =~ m#^https?://# && -escape_url($full_url) ne $full_url)) { +canonicalize_url($full_url) ne $full_url)) { $_[0] = undef; $self = undef; $RA = undef; @@ -327,7 +309,7 @@ sub gs_do_switch { } $ra ||= $self; - $url_b = escape_url($url_b); + $url_b = canonicalize_url($url_b); my $reporter = $ra->do_switch($rev_b, '', 1, $url_b, $editor, $pool); my @lock = (::compare_svn_version('1.2.0') >= 0) ? (undef) : (); $reporter->set_path('', $rev_a, 0, @lock, $pool); @@ -582,7 +564,8 @@ sub minimize_url { $ra->get_log("", $latest, 0, 1, 0, 1, sub {}); }; } while ($@ && ($c = shift @components)); - $url; + + return canonicalize_url($url); } sub can_do_switch { -- 1.7.11.3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/8] Fix typo in test
From: "Michael G. Schwern" Test to check that the migration got rid of the old style git-svn directory. It wasn't failing, just throwing a message to STDERR. --- t/t9107-git-svn-migrate.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t9107-git-svn-migrate.sh b/t/t9107-git-svn-migrate.sh index 289fc31..cfb4453 100755 --- a/t/t9107-git-svn-migrate.sh +++ b/t/t9107-git-svn-migrate.sh @@ -32,7 +32,7 @@ test_expect_success 'initialize old-style (v0) git svn layout' ' echo "$svnrepo" > "$GIT_DIR"/git-svn/info/url && echo "$svnrepo" > "$GIT_DIR"/svn/info/url && git svn migrate && - ! test -d "$GIT_DIR"/git svn && + ! test -d "$GIT_DIR"/git-svn && git rev-parse --verify refs/${remotes_git_svn}^0 && git rev-parse --verify refs/remotes/svn^0 && test "$(git config --get svn-remote.svn.url)" = "$svnrepo" && -- 1.7.11.3 -- 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
Fix git-svn for SVN 1.7
This patch series fixes git-svn for SVN 1.7 tested against SVN 1.7.5 and 1.6.18. Patch 7/8 is where SVN 1.7 starts passing. There is one exception. t9100-git-svn-basic.sh fails 11-13. This appears to be due to a bug in SVN to do with symlinks. Leave that for somebody else, this is the final submission in the series. The work was difficult because the code relies on simple string equalty when comparing URLs and paths. Turning on canonicalization in one part of the code would cause another part to fail if it also did not canonicalize. There's likely still issues. A better solution would be to have path and URL objects which overload the eq operator and automatically stringify canonicalized and escaped. This patch series should be placed on top of the previous which made accessors canonicalize. I'm getting a bit ahead of things by submitting it now, but I'd like to get the review process rolling. -- 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 7/7] Make Git::SVN and Git::SVN::Ra canonicalize paths and urls.
From: "Michael G. Schwern" This canonicalizes paths and urls as early as possible so we don't have to remember to do it at the point of use. It will fix a swath of SVN 1.7 problems in one go. Its ok to double canonicalize things. SVN 1.7 still fails, still not worrying about that. --- perl/Git/SVN.pm| 6 -- perl/Git/SVN/Ra.pm | 6 +- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm index b0ed3ea..798f6c4 100644 --- a/perl/Git/SVN.pm +++ b/perl/Git/SVN.pm @@ -27,6 +27,8 @@ use Git::SVN::Utils qw( fatal can_compress join_paths + canonicalize_path + canonicalize_url ); my $can_use_yaml; @@ -2305,7 +2307,7 @@ sub path { if( @_ ) { my $path = shift; -$self->{path} = $path; +$self->{path} = canonicalize_path($path); return; } @@ -2318,7 +2320,7 @@ sub url { if( @_ ) { my $url = shift; -$self->{url} = $url; +$self->{url} = canonicalize_url($url); return; } diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm index 27dcdd5..ef7b3dd 100644 --- a/perl/Git/SVN/Ra.pm +++ b/perl/Git/SVN/Ra.pm @@ -3,6 +3,10 @@ use vars qw/@ISA $config_dir $_ignore_refs_regex $_log_window_size/; use strict; use warnings; use SVN::Client; +use Git::SVN::Utils qw( + canonicalize_url +); + use SVN::Ra; BEGIN { @ISA = qw(SVN::Ra); @@ -138,7 +142,7 @@ sub url { if( @_ ) { my $url = shift; -$self->{url} = $url; +$self->{url} = canonicalize_url($url); return; } -- 1.7.11.3 -- 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 6/7] Switch path canonicalization to use the SVN API.
From: "Michael G. Schwern" All tests pass with SVN 1.6. SVN 1.7 remains broken, not worrying about it yet. SVN changed its path canonicalization API between 1.6 and 1.7. http://svnbook.red-bean.com/en/1.6/svn.developer.usingapi.html#svn.developer.usingapi.urlpath http://svnbook.red-bean.com/en/1.7/svn.developer.usingapi.html#svn.developer.usingapi.urlpath The SVN API does not accept foo/.. but it also doesn't canonicalize it. We have to do it ourselves. --- perl/Git/SVN/Utils.pm | 21 + 1 file changed, 21 insertions(+) diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm index 6c8ae53..7ae6fac 100644 --- a/perl/Git/SVN/Utils.pm +++ b/perl/Git/SVN/Utils.pm @@ -86,6 +86,27 @@ sub _collapse_dotdot { sub canonicalize_path { + my $path = shift; + + # The 1.7 way to do it + if ( defined &SVN::_Core::svn_dirent_canonicalize ) { + $path = _collapse_dotdot($path); + return SVN::_Core::svn_dirent_canonicalize($path); + } + # The 1.6 way to do it + elsif ( defined &SVN::_Core::svn_path_canonicalize ) { + $path = _collapse_dotdot($path); + return SVN::_Core::svn_path_canonicalize($path); + } + # No SVN API canonicalization is available, do it ourselves + else { + $path = _canonicalize_path_ourselves($path); + return $path; + } +} + + +sub _canonicalize_path_ourselves { my ($path) = @_; my $dot_slash_added = 0; if (substr($path, 0, 1) ne "/") { -- 1.7.11.3 -- 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 5/7] Remove irrelevant comment.
From: "Michael G. Schwern" The code doesn't use File::Spec. --- perl/Git/SVN/Utils.pm | 2 -- 1 file changed, 2 deletions(-) diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm index deade07..6c8ae53 100644 --- a/perl/Git/SVN/Utils.pm +++ b/perl/Git/SVN/Utils.pm @@ -92,8 +92,6 @@ sub canonicalize_path { $path = "./" . $path; $dot_slash_added = 1; } - # File::Spec->canonpath doesn't collapse x/../y into y (for a - # good reason), so let's do this manually. $path =~ s#/+#/#g; $path =~ s#/\.(?:/|$)#/#g; $path = _collapse_dotdot($path); -- 1.7.11.3 -- 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 4/7] Add join_paths() to safely concatenate paths.
From: "Michael G. Schwern" Otherwise you might wind up with things like... my $path1 = undef; my $path2 = 'foo'; my $path = $path1 . '/' . $path2; creating '/foo'. Or this... my $path1 = 'foo/'; my $path2 = 'bar'; my $path = $path1 . '/' . $path2; creating 'foo//bar'. Could have used File::Spec, but I'm shying away from it due to SVN 1.7's pickiness about paths. Felt it would be better to have our own we can control completely. --- git-svn.perl | 3 ++- perl/Git/SVN.pm | 10 ++ perl/Git/SVN/Utils.pm| 32 t/Git-SVN/Utils/join_paths.t | 32 4 files changed, 72 insertions(+), 5 deletions(-) create mode 100644 t/Git-SVN/Utils/join_paths.t diff --git a/git-svn.perl b/git-svn.perl index a857484..6e3e240 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -34,6 +34,7 @@ use Git::SVN::Utils qw( can_compress canonicalize_path canonicalize_url + join_paths ); use Git qw( @@ -1275,7 +1276,7 @@ sub get_svnprops { $path = $cmd_dir_prefix . $path; fatal("No such file or directory: $path") unless -e $path; my $is_dir = -d $path ? 1 : 0; - $path = $gs->{path} . '/' . $path; + $path = join_paths($gs->{path}, $path); # canonicalize the path (otherwise libsvn will abort or fail to # find the file) diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm index 7913d8f..b0ed3ea 100644 --- a/perl/Git/SVN.pm +++ b/perl/Git/SVN.pm @@ -23,7 +23,11 @@ use Git qw( command_output_pipe command_close_pipe ); -use Git::SVN::Utils qw(fatal can_compress); +use Git::SVN::Utils qw( + fatal + can_compress + join_paths +); my $can_use_yaml; BEGIN { @@ -316,9 +320,7 @@ sub init_remote_config { } my $old_path = $self->path; $url =~ s!^\Q$min_url\E(/|$)!!; - if (length $old_path) { - $url .= "/$old_path"; - } + $url = join_paths($url, $old_path); $self->path($url); $url = $min_url; } diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm index 7314e52..deade07 100644 --- a/perl/Git/SVN/Utils.pm +++ b/perl/Git/SVN/Utils.pm @@ -12,6 +12,7 @@ our @EXPORT_OK = qw( can_compress canonicalize_path canonicalize_url + join_paths ); @@ -134,4 +135,35 @@ sub _canonicalize_url_ourselves { } +=head3 join_paths + +my $new_path = join_paths(@paths); + +Appends @paths together into a single path. Any empty paths are ignored. + +=cut + +sub join_paths { + my @paths = @_; + + @paths = grep { defined $_ && length $_ } @paths; + + return '' unless @paths; + return $paths[0] if @paths == 1; + + my $new_path = shift @paths; + $new_path =~ s{/+$}{}; + + my $last_path = pop @paths; + $last_path =~ s{^/+}{}; + + for my $path (@paths) { + $path =~ s{^/+}{}; + $path =~ s{/+$}{}; + $new_path .= "/$path"; + } + + return $new_path .= "/$last_path"; +} + 1; diff --git a/t/Git-SVN/Utils/join_paths.t b/t/Git-SVN/Utils/join_paths.t new file mode 100644 index 000..d4488e7 --- /dev/null +++ b/t/Git-SVN/Utils/join_paths.t @@ -0,0 +1,32 @@ +#!/usr/bin/env perl + +use strict; +use warnings; + +use Test::More 'no_plan'; + +use Git::SVN::Utils qw( + join_paths +); + +# A reference cannot be a hash key, so we use an array. +my @tests = ( + [] => '', + ["/x.com", "bar"] => '/x.com/bar', + ["x.com", ""] => 'x.com', + ["/x.com/foo/", undef, "bar"] => '/x.com/foo/bar', + ["x.com/foo/", "/bar/baz/"] => 'x.com/foo/bar/baz/', + ["foo", "bar"] => 'foo/bar', + ["/foo/bar", "baz", "/biff"]=> '/foo/bar/baz/biff', + ["", undef, "."]=> '.', + [] => '', + +); + +while(@tests) { + my($have, $want) = splice @tests, 0, 2; + + my $args = join ", ", map { qq['$_'] } map { defined($_) ? $_ : 'undef' } @$have; + my $name = "join_paths($args) eq '$want'"; + is join_paths(@$have), $want, $name; +} -- 1.7.11.3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/7] Change canonicalize_url() to use the SVN 1.7 API when available.
From: "Michael G. Schwern" No change on SVN 1.6. The tests all pass with SVN 1.6 if canonicalize_url() does nothing, so tests passing doesn't have much meaning. The tests are so messed up right now with SVN 1.7 it isn't really useful to check. They will be useful later. --- perl/Git/SVN/Utils.pm | 16 1 file changed, 16 insertions(+) diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm index c842d00..9d5d3c5 100644 --- a/perl/Git/SVN/Utils.pm +++ b/perl/Git/SVN/Utils.pm @@ -3,6 +3,8 @@ package Git::SVN::Utils; use strict; use warnings; +use SVN::Core; + use base qw(Exporter); our @EXPORT_OK = qw( @@ -100,6 +102,20 @@ API as a URL. =cut sub canonicalize_url { + my $url = shift; + + # The 1.7 way to do it + if ( defined &SVN::_Core::svn_uri_canonicalize ) { + return SVN::_Core::svn_uri_canonicalize($url); + } + # There wasn't a 1.6 way to do it, so we do it ourself. + else { + return _canonicalize_url_ourselves($url); + } +} + + +sub _canonicalize_url_ourselves { my ($url) = @_; $url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e; return $url; -- 1.7.11.3 -- 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 3/7] Extract, test and enhance the logic to collapse ../foo paths.
From: "Michael G. Schwern" The SVN API functions will not accept ../foo but their canonicalization functions will not collapse it. So we'll have to do it ourselves. _collapse_dotdot() works better than the existing regex did. This will be used shortly when canonicalize_path() starts using the SVN API. --- perl/Git/SVN/Utils.pm | 14 +- t/Git-SVN/Utils/collapse_dotdot.t | 23 +++ 2 files changed, 36 insertions(+), 1 deletion(-) create mode 100644 t/Git-SVN/Utils/collapse_dotdot.t diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm index 9d5d3c5..7314e52 100644 --- a/perl/Git/SVN/Utils.pm +++ b/perl/Git/SVN/Utils.pm @@ -72,6 +72,18 @@ API as a file path. =cut +# Turn foo/../bar into bar +sub _collapse_dotdot { + my $path = shift; + + 1 while $path =~ s{/[^/]+/+\.\.}{}; + 1 while $path =~ s{[^/]+/+\.\./}{}; + 1 while $path =~ s{[^/]+/+\.\.}{}; + + return $path; +} + + sub canonicalize_path { my ($path) = @_; my $dot_slash_added = 0; @@ -83,7 +95,7 @@ sub canonicalize_path { # good reason), so let's do this manually. $path =~ s#/+#/#g; $path =~ s#/\.(?:/|$)#/#g; - $path =~ s#/[^/]+/\.\.##g; + $path = _collapse_dotdot($path); $path =~ s#/$##g; $path =~ s#^\./## if $dot_slash_added; $path =~ s#^/##; diff --git a/t/Git-SVN/Utils/collapse_dotdot.t b/t/Git-SVN/Utils/collapse_dotdot.t new file mode 100644 index 000..1da1cce --- /dev/null +++ b/t/Git-SVN/Utils/collapse_dotdot.t @@ -0,0 +1,23 @@ +#!/usr/bin/env perl + +use strict; +use warnings; + +use Test::More 'no_plan'; + +use Git::SVN::Utils; +my $collapse_dotdot = \&Git::SVN::Utils::_collapse_dotdot; + +my %tests = ( + "foo/bar/baz" => "foo/bar/baz", + ".."=> "..", + "foo/.."=> "", + "/foo/bar/../../baz"=> "/baz", + "deeply/.././deeply/nested" => "./deeply/nested", +); + +for my $arg (keys %tests) { + my $want = $tests{$arg}; + + is $collapse_dotdot->($arg), $want, "_collapse_dotdot('$arg') => $want"; +} -- 1.7.11.3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/7] Move the canonicalization functions to Git::SVN::Utils
From: "Michael G. Schwern" So they can be used by others. I'd like to test them, but they're going to become SVN API wrappers shortly and those aren't predictable. No functional change. --- git-svn.perl | 33 +++- perl/Git/SVN/Utils.pm | 52 ++- 2 files changed, 58 insertions(+), 27 deletions(-) diff --git a/git-svn.perl b/git-svn.perl index de1ddd1..a857484 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -29,7 +29,13 @@ use Git::SVN::Prompt; use Git::SVN::Log; use Git::SVN::Migration; -use Git::SVN::Utils qw(fatal can_compress); +use Git::SVN::Utils qw( + fatal + can_compress + canonicalize_path + canonicalize_url +); + use Git qw( git_cmd_try command @@ -1256,31 +1262,6 @@ sub cmd_mkdirs { $gs->mkemptydirs($_revision); } -sub canonicalize_path { - my ($path) = @_; - my $dot_slash_added = 0; - if (substr($path, 0, 1) ne "/") { - $path = "./" . $path; - $dot_slash_added = 1; - } - # File::Spec->canonpath doesn't collapse x/../y into y (for a - # good reason), so let's do this manually. - $path =~ s#/+#/#g; - $path =~ s#/\.(?:/|$)#/#g; - $path =~ s#/[^/]+/\.\.##g; - $path =~ s#/$##g; - $path =~ s#^\./## if $dot_slash_added; - $path =~ s#^/##; - $path =~ s#^\.$##; - return $path; -} - -sub canonicalize_url { - my ($url) = @_; - $url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e; - return $url; -} - # get_svnprops(PATH) # -- # Helper for cmd_propget and cmd_proplist below. diff --git a/perl/Git/SVN/Utils.pm b/perl/Git/SVN/Utils.pm index 3d0bfa4..c842d00 100644 --- a/perl/Git/SVN/Utils.pm +++ b/perl/Git/SVN/Utils.pm @@ -5,7 +5,12 @@ use warnings; use base qw(Exporter); -our @EXPORT_OK = qw(fatal can_compress); +our @EXPORT_OK = qw( + fatal + can_compress + canonicalize_path + canonicalize_url +); =head1 NAME @@ -56,4 +61,49 @@ sub can_compress { } +=head3 canonicalize_path + +my $canoncalized_path = canonicalize_path($path); + +Converts $path into a canonical form which is safe to pass to the SVN +API as a file path. + +=cut + +sub canonicalize_path { + my ($path) = @_; + my $dot_slash_added = 0; + if (substr($path, 0, 1) ne "/") { + $path = "./" . $path; + $dot_slash_added = 1; + } + # File::Spec->canonpath doesn't collapse x/../y into y (for a + # good reason), so let's do this manually. + $path =~ s#/+#/#g; + $path =~ s#/\.(?:/|$)#/#g; + $path =~ s#/[^/]+/\.\.##g; + $path =~ s#/$##g; + $path =~ s#^\./## if $dot_slash_added; + $path =~ s#^/##; + $path =~ s#^\.$##; + return $path; +} + + +=head3 canonicalize_url + +my $canonicalized_url = canonicalize_url($url); + +Converts $url into a canonical form which is safe to pass to the SVN +API as a URL. + +=cut + +sub canonicalize_url { + my ($url) = @_; + $url =~ s#^([^:]+://[^/]*/)(.*)$#$1 . canonicalize_path($2)#e; + return $url; +} + + 1; -- 1.7.11.3 -- 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
Canonicalize the git-svn path & url accessors
This patch turns on canonicalization in the Git::SVN and Git::SVN::Ra path and url accessors. It also makes the canonicalizers use the SVN API when available. All patches pass with SVN 1.6. Next patch series will fix SVN 1.7. This should be placed on top of the previous patch series which added path and url accessors. -- 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: Make git-svn Use accessors for paths and urls
Michael G Schwern wrote: > On 2012.7.27 8:10 PM, Jonathan Nieder wrote: >> If you have a chance at some point to offer advice, I'd love to add >> the information to Documentation/SubmittingPatches that was missing. [...] > Remind me when I'm done with the 1.7 fix please? Sure, if I remember to. :) Thanks, Jonathan -- 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: Make git-svn Use accessors for paths and urls
On 2012.7.27 8:10 PM, Jonathan Nieder wrote: >> This is the last refactoring patch series. After this bugs, start >> getting fixed. > > I just wanted to say thanks for your thoughtful presentation of this > code. I was worried before, but these have been pleasantly submitted. You're welcome. I've gained at least three levels in rebasing in the process. > If you have a chance at some point to offer advice, I'd love to add > the information to Documentation/SubmittingPatches that was missing. > Proposed text is ideal, but outline form or a list of missing aspects > and confusing existing coverage would be fine, too. Remind me when I'm done with the 1.7 fix please? -- You are wicked and wrong to have broken inside and peeked at the implementation and then relied upon it. -- tchrist in <31832.969261130@chthon> -- 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: [RFC 14/16] transport-helper: add import|export-marks to fast-import command line.
Hi, Florian Achleitner wrote: > a515ebe9. [...] > Btw, these added capabilities are not mentioned in Docs. Sverre, any hints about how these (capabilities "import-marks " and "export-marks ") are meant to be used? Why would one use these instead of "feature import-marks" / "feature export-marks"? Should there be a corresponding capability for relative-marks, too? Curious, Jonathan -- 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: [RFC 1/4 v2] Implement a basic remote helper for svn in C.
Florian Achleitner wrote: > So I should kick printd out? I think so, yes. "git log -SGIT_TRANSPORT_HELPER_DEBUG transport-helper.c" tells me that that option was added to make the transport-helper machinery make noise to make it obvious at what stage a remote helper has deadlocked. GIT_TRANSPORT_HELPER_DEBUG already takes care of that, so there would not be need for an imitation of that in remote-svn, unless I am missing something (and even if I am missing something, it seems complicated enough to be worth moving to another patch where it can be explained more easily). [...] > + > + printf("import\n"); > + printf("\n"); > + fflush(stdout); > + return SUCCESS; > +} [...] >>Maybe the purpose of the flush would >> be more obvious if it were moved to the caller. > > Acutally this goes to the git parent process (not fast-import), waiting for a > reply to the command. I think I have to call flush on this side of the pipe. > Can you flush it from the reader? This wouldn't have the desired effect, it > drops buffered data. *slaps head* This is the "capabilities" command, and it needs to flush because the reader needs to know what commands it's allowed to use next before it starts using them. My brain turned off and I thought you were emitting an "import" command rather than advertising that you support it for some reason. And 'printf("\n")' was a separate printf because that way, patches like printf("import\n"); + printf("bidi-import\n"); printf("\n"); fflush(stdout); become simpler. I'm tempted to suggest a structure like const char * const capabilities[] = {"import"}; int i; for (i = 0; i < ARRAY_SIZE(capabilities); i++) puts(capabilities[i]); puts(""); /* blank line */ fflush(stdout); but your original code was fine, too. [...] > + /* opening a fifo for usually reading blocks until a writer has opened > it too. + * Therefore, we open with RDWR. > + */ > + report_fd = open(back_pipe_env, O_RDWR); > + if(report_fd < 0) { > + die("Unable to open fast-import back-pipe! %s", > strerror(errno)); [...] > I believe it can be solved using RDONLY and WRONLY too. Probably we solve it > by not using the fifo at all. > Currently the blocking comes from the fact, that fast-import doesn't parse > it's command line at startup. It rather reads an input line first and decides > whether to parse the argv after reading the first input line or at the end of > the input. (don't know why) > remote-svn opens the pipe before sending the first command to fast-import and > blocks on the open, while fast-import waits for input --> deadlock. Thanks for explaining. Now we've discussed a few different approproaches, none of which is perfect. a. use --cat-blob-fd, no FIFO Doing this unconditionally would break platforms that don't support --cat-blob-fd=(descriptor >2), like Windows, so we'd have to: * Make it conditional --- only do it (1) we are not on Windows and (2) the remote helper requests backflow by advertising the import-bidi capability. * Let the remote helper know what's going on by using "import-bidi" instead of "import" in the command stream to initiate the import. b. use envvars to pass around FIFO path This complicates the fast-import interface and makes debugging hard. It would be nice to avoid this if we can, but in case we can't, it's nice to have the option available. c. transport-helper.c uses FIFO behind the scenes. Like (a), except it would require a fast-import tweak (boo) and would work on Windows (yea) d. use --cat-blob-fd with FIFO Early scripted remote-svn prototypes did this to fulfill "fetch" requests. It has no advantage over "use --cat-blob-fd, no FIFO" except being easier to implement as a shell script. I'm listing this just for comparison; since (a) looks better in every way, I don't see any reason to pursue this one. Since avoiding deadlocks with bidirectional communication is always a little subtle, it would be nice for this to be implemented once in transport-helper.c rather than each remote helper author having to reimplement it again. As a result, my knee-jerk ranking is a > c > b > d. Sane? Jonathan -- 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