Re: [PATCH] git-svn: trim leading and trailing whitespaces in author name
On 2019-09-12 at 16:47:41 +0200, Eric Sunshine wrote:
> On Thu, Sep 12, 2019 at 7:59 AM Tobias Klauser wrote:
> > In some cases, the svn author names might contain leading or trailing
> > whitespaces, leading to messages such as:
> >
> > Author: user1
> >not defined in authors.txt
> >
> > (the trailing newline leads to the line break). The user "user1" is
> > defined in authors.txt though, e.g.
> >
> > user1 = User
> >
> > Fix this by trimming the author name retreived from svn before using it
> > in check_author.
> >
> > Signed-off-by: Tobias Klauser
> > ---
> > diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
> > @@ -1491,6 +1491,7 @@ sub call_authors_prog {
> > sub check_author {
> > my ($author) = @_;
> > + $author =~ s/^\s+|\s+$//g;
> > if (!defined $author || length $author == 0) {
> > $author = '(no author)';
> > }
>
> This fix seems incomplete. What happens if $author is undefined?
> (There is a check for $author defined'ness just below the new code you
> add.)
Right, thanks for noting. The whitespace trimming should be moved below
the defined'ness check. I'll send an updated patch.
Re: [PATCH] git-svn: trim leading and trailing whitespaces in author name
On Thu, Sep 12, 2019 at 7:59 AM Tobias Klauser wrote:
> In some cases, the svn author names might contain leading or trailing
> whitespaces, leading to messages such as:
>
> Author: user1
>not defined in authors.txt
>
> (the trailing newline leads to the line break). The user "user1" is
> defined in authors.txt though, e.g.
>
> user1 = User
>
> Fix this by trimming the author name retreived from svn before using it
> in check_author.
>
> Signed-off-by: Tobias Klauser
> ---
> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
> @@ -1491,6 +1491,7 @@ sub call_authors_prog {
> sub check_author {
> my ($author) = @_;
> + $author =~ s/^\s+|\s+$//g;
> if (!defined $author || length $author == 0) {
> $author = '(no author)';
> }
This fix seems incomplete. What happens if $author is undefined?
(There is a check for $author defined'ness just below the new code you
add.)
Re: [PATCH] git-svn: avoid warning on undef readline()
Eric Wong writes:
> Ævar Arnfjörð Bjarmason wrote:
>> See https://public-inbox.org/git/86h8oobl36@phe.ftfl.ca/ for the
>> original report.
>
> Thanks for taking a look at this. Also https://bugs.debian.org/894997
>
>> --- a/perl/Git.pm
>> +++ b/perl/Git.pm
>> @@ -554,7 +554,7 @@ sub get_record {
>> my ($fh, $rs) = @_;
>> local $/ = $rs;
>> my $rec = <$fh>;
>> -chomp $rec if defined $rs;
>> +chomp $rec if defined $rs and defined $rec;
>
> I'm struggling to understand the reason for the "defined $rs"
> check. I think it was a braino on my part and meant to use:
>
> chomp $rec if defined $rec;
That sounds quite plausible, so if there is no further discussion,
let me queue a version that does s/rs/rec/ on that line myself
(bypassing a pull from your repository at bogomips.org/).
Thanks.
Re: [PATCH] git-svn: avoid warning on undef readline()
On Fri, Apr 06, 2018 at 01:15:14PM +, Ævar Arnfjörð Bjarmason wrote:
> Change code in Git.pm that sometimes calls chomp() on undef to only do
> so the value is defined.
>
> This code has been chomping undef values ever since it was added in
> b26098fc2f ("git-svn: reduce scope of input record separator change",
> 2016-10-14), but started warning due to the introduction of "use
> warnings" to Git.pm in my f0e19cb7ce ("Git.pm: add the "use warnings"
> pragma", 2018-02-25) released with 2.17.0.
>
> Since this function will return undef in those cases it's still
> possible that the code using it will warn if it does a chomp of its
> own, as the code added in b26098fc2f ("git-svn: reduce scope of input
> record separator change", 2016-10-14) might do, but since git-svn has
> "use warnings" already that's clearly not a codepath that's going to
> warn.
Thanks for this patch. I ran into this earlier this week (with git svn
fetch) and had intended to send a patch, but you clearly got to it
before I did.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
signature.asc
Description: PGP signature
Re: [PATCH] git-svn: avoid warning on undef readline()
On Fri, Apr 06 2018, Eric Wong wrote:
> Ævar Arnfjörð Bjarmason wrote:
>> On Fri, Apr 06 2018, Eric Wong wrote:
>> > Ævar Arnfjörð Bjarmason wrote:
>> >
>> >> --- a/perl/Git.pm
>> >> +++ b/perl/Git.pm
>> >> @@ -554,7 +554,7 @@ sub get_record {
>> >> my ($fh, $rs) = @_;
>> >> local $/ = $rs;
>> >> my $rec = <$fh>;
>> >> - chomp $rec if defined $rs;
>> >> + chomp $rec if defined $rs and defined $rec;
>> >
>> > I'm struggling to understand the reason for the "defined $rs"
>> > check. I think it was a braino on my part and meant to use:
>> >
>> >chomp $rec if defined $rec;
>>
>> Whether this makes any sense is another question, but you seem to have
>> explicitly meant this at the time. The full function definition with
>> documentation:
>>
>> =item get_record ( FILEHANDLE, INPUT_RECORD_SEPARATOR )
>>
>> Read one record from FILEHANDLE delimited by INPUT_RECORD_SEPARATOR,
>> removing any trailing INPUT_RECORD_SEPARATOR.
>
> I've always known chomp to respect the value of $/; so chomp($rec)
> whould only cut out whatever $rs is, and be a no-op if $rs is undef.
Yup, you're right. I missed that.
Re: [PATCH] git-svn: avoid warning on undef readline()
Ævar Arnfjörð Bjarmason wrote:
> On Fri, Apr 06 2018, Eric Wong wrote:
> > Ævar Arnfjörð Bjarmason wrote:
> >
> >> --- a/perl/Git.pm
> >> +++ b/perl/Git.pm
> >> @@ -554,7 +554,7 @@ sub get_record {
> >>my ($fh, $rs) = @_;
> >>local $/ = $rs;
> >>my $rec = <$fh>;
> >> - chomp $rec if defined $rs;
> >> + chomp $rec if defined $rs and defined $rec;
> >
> > I'm struggling to understand the reason for the "defined $rs"
> > check. I think it was a braino on my part and meant to use:
> >
> > chomp $rec if defined $rec;
>
> Whether this makes any sense is another question, but you seem to have
> explicitly meant this at the time. The full function definition with
> documentation:
>
> =item get_record ( FILEHANDLE, INPUT_RECORD_SEPARATOR )
>
> Read one record from FILEHANDLE delimited by INPUT_RECORD_SEPARATOR,
> removing any trailing INPUT_RECORD_SEPARATOR.
I've always known chomp to respect the value of $/; so chomp($rec)
whould only cut out whatever $rs is, and be a no-op if $rs is undef.
> It doesn't make to remove the trailing record separator if it's not
> defined, otherwise we'd be coercing undef to "\n" while at the same time
> returning multiple records. But then of course the only user of this
> with an "undef" argument just does:
>
> chomp($log_entry{log} = get_record($log_fh, undef));
Subtle difference, that chomp() still sees $/ as "\n".
$/ is only undef inside get_record.
> So we could also remove that chomp(), adn not check defined $rs, but IMO
> it's cleaner & more consistent this way.
I think the chomp is necessary. In git-svn.perl /^sub get_commit_entry {:
#
open my $log_fh, '>', $commit_editmsg or croak $!;
#
$msgbuf =~ s/\s+$//s;
#
print $log_fh $msgbuf or croak $!;
#
close $log_fh or croak $!;
# Above, we ensured the contents of $commit_editmsg has no trailing newline
if ($_edit || ($type eq 'tree')) {
chomp(my $editor = command_oneline(qw(var GIT_EDITOR)));
system('sh', '-c', $editor.' "$@"', $editor, $commit_editmsg);
}
# However, $editor is likely to introduce a trailing newline
rename $commit_editmsg, $commit_msg or croak $!;
{
require Encode;
# SVN requires messages to be UTF-8 when entering the repo
open $log_fh, '<', $commit_msg or croak $!;
binmode $log_fh;
# chomp trailing newline introduced by $editor:
chomp($log_entry{log} = get_record($log_fh, undef));
Re: [PATCH] git-svn: avoid warning on undef readline()
On Fri, Apr 06 2018, Eric Wong wrote:
> Ævar Arnfjörð Bjarmason wrote:
>> See https://public-inbox.org/git/86h8oobl36@phe.ftfl.ca/ for the
>> original report.
>
> Thanks for taking a look at this. Also https://bugs.debian.org/894997
>
>> --- a/perl/Git.pm
>> +++ b/perl/Git.pm
>> @@ -554,7 +554,7 @@ sub get_record {
>> my ($fh, $rs) = @_;
>> local $/ = $rs;
>> my $rec = <$fh>;
>> -chomp $rec if defined $rs;
>> +chomp $rec if defined $rs and defined $rec;
>
> I'm struggling to understand the reason for the "defined $rs"
> check. I think it was a braino on my part and meant to use:
>
> chomp $rec if defined $rec;
Whether this makes any sense is another question, but you seem to have
explicitly meant this at the time. The full function definition with
documentation:
=item get_record ( FILEHANDLE, INPUT_RECORD_SEPARATOR )
Read one record from FILEHANDLE delimited by INPUT_RECORD_SEPARATOR,
removing any trailing INPUT_RECORD_SEPARATOR.
=cut
sub get_record {
my ($fh, $rs) = @_;
local $/ = $rs;
my $rec = <$fh>;
chomp $rec if defined $rs;
$rec;
}
It doesn't make to remove the trailing record separator if it's not
defined, otherwise we'd be coercing undef to "\n" while at the same time
returning multiple records. But then of course the only user of this
with an "undef" argument just does:
chomp($log_entry{log} = get_record($log_fh, undef));
So we could also remove that chomp(), adn not check defined $rs, but IMO
it's cleaner & more consistent this way.
Re: [PATCH] git-svn: avoid warning on undef readline()
Ævar Arnfjörð Bjarmason wrote:
> See https://public-inbox.org/git/86h8oobl36@phe.ftfl.ca/ for the
> original report.
Thanks for taking a look at this. Also https://bugs.debian.org/894997
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -554,7 +554,7 @@ sub get_record {
> my ($fh, $rs) = @_;
> local $/ = $rs;
> my $rec = <$fh>;
> - chomp $rec if defined $rs;
> + chomp $rec if defined $rs and defined $rec;
I'm struggling to understand the reason for the "defined $rs"
check. I think it was a braino on my part and meant to use:
chomp $rec if defined $rec;
Re: [PATCH] git-svn: avoid warning on undef readline()
Hi Ævar,
On Fri, 6 Apr 2018, Ævar Arnfjörð Bjarmason wrote:
> Change code in Git.pm that sometimes calls chomp() on undef to only do
> so the value is defined.
>
> This code has been chomping undef values ever since it was added in
> b26098fc2f ("git-svn: reduce scope of input record separator change",
> 2016-10-14), but started warning due to the introduction of "use
> warnings" to Git.pm in my f0e19cb7ce ("Git.pm: add the "use warnings"
> pragma", 2018-02-25) released with 2.17.0.
>
> Since this function will return undef in those cases it's still
> possible that the code using it will warn if it does a chomp of its
> own, as the code added in b26098fc2f ("git-svn: reduce scope of input
> record separator change", 2016-10-14) might do, but since git-svn has
> "use warnings" already that's clearly not a codepath that's going to
> warn.
>
> See https://public-inbox.org/git/86h8oobl36@phe.ftfl.ca/ for the
> original report.
>
> Reported-by: Joseph Mingrone
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
> perl/Git.pm | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 16ebcc612c..6b6079c456 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -554,7 +554,7 @@ sub get_record {
> my ($fh, $rs) = @_;
> local $/ = $rs;
> my $rec = <$fh>;
> - chomp $rec if defined $rs;
> + chomp $rec if defined $rs and defined $rec;
> $rec;
> }
The explanation as well as the patch make a total lot of sense to me.
Ciao,
Dscho
Re: [PATCH] git-svn: control destruction order to avoid segfault
Eric Wong writes:
> Todd Zullinger wrote:
>> I'm running the tests with and without your patch as well.
>> So far I've run t9128 300 times with the patch and no
>> failures. Without it, it's failed 3 times in only a few
>> dozen runs. That's promising.
>
> Thanks for confirming it works on other systems.
> Pull request and patch below:
>
> The following changes since commit 5be1f00a9a701532232f57958efab4be8c959a29:
>
> First batch after 2.16 (2018-01-23 13:21:10 -0800)
>
> are available in the Git repository at:
>
> git://bogomips.org/git-svn.git svn-branch-segfault
>
> for you to fetch changes up to 2784b8d68faca823489949cbc69ead2f296cfc07:
>
> git-svn: control destruction order to avoid segfault (2018-01-29 23:12:00
> +)
>
>
> Eric Wong (1):
> git-svn: control destruction order to avoid segfault
>
> git-svn.perl | 5 +
> 1 file changed, 5 insertions(+)
Thanks. I'd actually apply this as a patch instead of pullilng, as
I suspect you'd want it in 'maint' as well, though.
> -8<-
> Subject: [PATCH] git-svn: control destruction order to avoid segfault
>
> It seems necessary to control destruction ordering to avoid a
> segfault with SVN 1.9.5 when using "git svn branch".
> I've also reported the problem against libsvn-perl to Debian
> [Bug #888791], but releasing the SVN::Client instance can be
> beneficial anyways to save memory.
>
> ref: https://bugs.debian.org/888791
> Tested-by: Todd Zullinger
> Reported-by: brian m. carlson
> Signed-off-by: Eric Wong
> ---
> git-svn.perl | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index 76a75d0b3d..a6b6c3e40c 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1200,6 +1200,11 @@ sub cmd_branch {
> $ctx->copy($src, $rev, $dst)
> unless $_dry_run;
>
> + # Release resources held by ctx before creating another SVN::Ra
> + # so destruction is orderly. This seems necessary with SVN 1.9.5
> + # to avoid segfaults.
> + $ctx = undef;
> +
> $gs->fetch_all;
> }
Re: [PATCH] git-svn: convert CRLF to LF in commit message to SVN
"Bennett, Brian" writes: > Thank you all for your guidance, > > I have completed my test this morning with the patch and the 'git > svn dcommit' is now SUCCESSFUl! Thanks, all.
RE: [PATCH] git-svn: convert CRLF to LF in commit message to SVN
Thank you all for your guidance, I have completed my test this morning with the patch and the 'git svn dcommit' is now SUCCESSFUl! Thank you again for all of your help and I'll await to see when the patch is posted. Brian Bennett | Supv System Admin & Support, TA TECH Change Mgmt/Production Support o: 319-355-7602 | c: 319-533-1094 e: brian.benn...@transamerica.com | w: www.transamerica.com Transamerica 6400 C St. SW, Cedar Rapids, IA 52404 MS-2410 Facebook | LinkedIn -Original Message- From: Todd Zullinger [mailto:todd.zullin...@gmail.com] On Behalf Of Todd Zullinger Sent: Thursday, December 14, 2017 5:50 PM To: Bennett, Brian Cc: Eric Wong ; Junio C Hamano ; git@vger.kernel.org Subject: Re: [PATCH] git-svn: convert CRLF to LF in commit message to SVN Hi Brian, Bennett, Brian wrote: > Thank you for your fast response, > > I haven't done a build of this type before (so I could test the patch > first) so I'm trying to do that and get this far: ... > I don't want to drag out testing the patch, so if either of you are > able to quickly guide me on what I am doing incorrectly I am willing > to get the build done so I can test it. If not, could one of you build > with the patch and somehow get that to me so I could test? I don't know about building git for windows, but since the git-svn command is a perl script, it might be easier to just patch that file. I think you can find the path where git-svn is installed using: git --exec-path For this one-liner, I'd just manually apply it. (If you want to use 'git apply' or the patch command, you'll have to edit the patch to adjust the name of the file, as it's git-svn.perl in the git tree. The .perl suffix is dropped in the installed version.) Hopefully that makes it easier for you to test Eric's patch. -- Todd ~~ I am in shape. Round is a shape.
Re: [PATCH] git-svn: convert CRLF to LF in commit message to SVN
Hi Brian, Bennett, Brian wrote: > Thank you for your fast response, > > I haven't done a build of this type before (so I could > test the patch first) so I'm trying to do that and get > this far: ... > I don't want to drag out testing the patch, so if either > of you are able to quickly guide me on what I am doing > incorrectly I am willing to get the build done so I can > test it. If not, could one of you build with the patch and > somehow get that to me so I could test? I don't know about building git for windows, but since the git-svn command is a perl script, it might be easier to just patch that file. I think you can find the path where git-svn is installed using: git --exec-path For this one-liner, I'd just manually apply it. (If you want to use 'git apply' or the patch command, you'll have to edit the patch to adjust the name of the file, as it's git-svn.perl in the git tree. The .perl suffix is dropped in the installed version.) Hopefully that makes it easier for you to test Eric's patch. -- Todd ~~ I am in shape. Round is a shape.
RE: [PATCH] git-svn: convert CRLF to LF in commit message to SVN
Thank you for your fast response, I haven't done a build of this type before (so I could test the patch first) so I'm trying to do that and get this far: 1. git clone https://github.com/msysgit/msysgit.git "c:\temp\git\guitbuild" 2. git clone https://github.com/git-for-windows/git.git "c:\temp\git\guitbuild\git" 3. c:\temp\git\guitbuild\msys.bat --- Building and Installing Git --- CC archive-tar.o In file included from run-command.h:5, from archive-tar.c:9: compat/win32/pthread.h:38: error: expected ')' before 'ConditionVariable' compat/win32/pthread.h:40: error: expected ')' before 'ConditionVariable' compat/win32/pthread.h:42: error: expected ')' before 'ConditionVariable' compat/win32/pthread.h:44: error: expected ')' before 'ConditionVariable' make: *** [archive-tar.o] Error 1 The applicable lines from compat/win32/pthread.h: 37: WINBASEAPI VOID WINAPI 38: InitializeConditionVariable(PCONDITION_VARIABLE ConditionVariable); 39: WINBASEAPI VOID WINAPI 40: WakeConditionVariable(PCONDITION_VARIABLE ConditionVariable); 41: WINBASEAPI VOID WINAPI 42: WakeAllConditionVariable(PCONDITION_VARIABLE ConditionVariable); 43: WINBASEAPI BOOL WINAPI 44: SleepConditionVariableCS(PCONDITION_VARIABLE ConditionVariable, 45: PCRITICAL_SECTION CriticalSection, 46: DWORD dwMilliseconds); I don't want to drag out testing the patch, so if either of you are able to quickly guide me on what I am doing incorrectly I am willing to get the build done so I can test it. If not, could one of you build with the patch and somehow get that to me so I could test? Brian Bennett | Supv System Admin & Support, TA TECH Change Mgmt/Production Support o: 319-355-7602 | c: 319-533-1094 e: brian.benn...@transamerica.com | w: www.transamerica.com Transamerica 6400 C St. SW, Cedar Rapids, IA 52404 MS-2410 Facebook | LinkedIn -Original Message- From: Eric Wong [mailto:e...@80x24.org] Sent: Wednesday, December 13, 2017 6:21 PM To: Bennett, Brian ; Junio C Hamano Cc: git@vger.kernel.org Subject: [PATCH] git-svn: convert CRLF to LF in commit message to SVN "Bennett, Brian" wrote: > Environment: > > Desktop: Windows 7 Enterprise 64-bit > svn client (if applicable): 1.8.8 from Apache git > (https://urldefense.proofpoint.com/v2/url?u=https-3A__git-2Dfor-2Dwind > ows.github.io_&d=DwIBaQ&c=9g4MJkl2VjLjS6R4ei18BA&r=CorEYR_fG6hKwP1xRO7 > dkFFJM6UfxLGgypqJT0q3mO4&m=f1K2uzEyLbtIX-0te07VlclknjdUztTvbgDMA0thROs > &s=3AqxH_SEQG48PhnwuCD8udYta0mqXfgKKlmAWMfSlfE&e=): git version > 2.10.1.windows.1 GitTfs > (https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_git-2 > Dtfs_git-2Dtfs&d=DwIBaQ&c=9g4MJkl2VjLjS6R4ei18BA&r=CorEYR_fG6hKwP1xRO7 > dkFFJM6UfxLGgypqJT0q3mO4&m=f1K2uzEyLbtIX-0te07VlclknjdUztTvbgDMA0thROs > &s=C2gZ6zgihigH5eMpa5UVgj1mglbQbGN1HG0blMqjmsY&e=): git-tfs version > 0.27.0.0 (TFS client library 14.0.0.0 (MS)) (32-bit) Team Foundation > Server: 2010 Visual Studio installation: 2010 and 2015 Thanks for the report and research! > I've researched enough to believe that the commit message being used > by git svn contains a carriage return character > (x'0D') and that has not been allowed in Subversion since version 1.6 > (I can replicate this specific error message using an SVN dump file > that contains x'0D' characters in the log messages.). However, I > cannot find where I have any control over the log message that git svn > is trying to use nor can I observe it. Note that I've also used the > '-v' switch with the 'git svn dcommit', but do not receive anything > other than what I am showing above. Maybe git-for-windows isn't filtering CRLF into LF as "git commit" does on GNU/Linux when the original commit was made? I had to use "git commit-tree" to reproduce the error in testing (instead of "git commit)" Anyways, the one-line fix below should be enough for you. Care to give it a shot? Thanks again. Junio: please pull when Brian confirms, thanks. The following changes since commit 95ec6b1b3393eb6e26da40c565520a8db9796e9f: RelNotes: the eighth batch (2017-12-06 09:29:50 -0800) are available in the git repository at: git://bogomips.org/git-svn.git svn-crlf for you to fetch changes up to 95450bbbaaacaf2d603a4fbded25d55243dfb291: git-svn: convert CRLF to LF in commit message to SVN (2017-12-14 00:09:38 +) Eric Wong (1): git-svn: convert CRLF to LF in commit message to SVN git-svn.perl| 1 + t/t9169-git-svn-dcommit-crlf.sh | 27 +++ 2 files changed, 28 insertions(+) create mode 100755 t/t9169-git-svn-dcommit-crlf.sh --8< Subject: [PATCH] git-svn: convert CRLF to LF in commit message to SVN Subversion since 1.6 does not accept CR characters in the commit message, so filter it out on o
Re: [PATCH] git svn fetch: Create correct commit timestamp when using --localtime
Junio C Hamano writes: > > Yep, seems alright. Can you apply directly? > > Been a bit preoccupied as of late. Thanks. > > Surely, I'll just add your Reviewed-by: myself ;-) OK, thanks. This will fix the bug I've reported here a week or so ago (see the References header). urs
Re: [PATCH] git svn fetch: Create correct commit timestamp when using --localtime
Eric Wong writes: > Junio C Hamano wrote: >> Urs Thuermann writes: >> >> > In parse_svn_date() prepend the correct UTC offset to the timestamp >> > returned. This is the offset in effect at the commit time instead of >> > the offset in effect at calling time. >> > >> > Signed-off-by: Urs Thuermann >> > --- >> > perl/Git/SVN.pm | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> >> Thanks; sounds sensible. >> >> Eric? > > Yep, seems alright. Can you apply directly? > Been a bit preoccupied as of late. Thanks. Surely, I'll just add your Reviewed-by: myself ;-) Thanks.
Re: [PATCH] git svn fetch: Create correct commit timestamp when using --localtime
Junio C Hamano wrote: > Urs Thuermann writes: > > > In parse_svn_date() prepend the correct UTC offset to the timestamp > > returned. This is the offset in effect at the commit time instead of > > the offset in effect at calling time. > > > > Signed-off-by: Urs Thuermann > > --- > > perl/Git/SVN.pm | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Thanks; sounds sensible. > > Eric? Yep, seems alright. Can you apply directly? Been a bit preoccupied as of late. Thanks.
Re: [PATCH] git svn fetch: Create correct commit timestamp when using --localtime
Urs Thuermann writes:
> In parse_svn_date() prepend the correct UTC offset to the timestamp
> returned. This is the offset in effect at the commit time instead of
> the offset in effect at calling time.
>
> Signed-off-by: Urs Thuermann
> ---
> perl/Git/SVN.pm | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Thanks; sounds sensible.
Eric?
>
> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
> index 98518f4..bc4eed3 100644
> --- a/perl/Git/SVN.pm
> +++ b/perl/Git/SVN.pm
> @@ -1416,7 +1416,7 @@ sub parse_svn_date {
> delete $ENV{TZ};
> }
>
> - my $our_TZ = get_tz_offset();
> + my $our_TZ = get_tz_offset($epoch_in_UTC);
>
> # This converts $epoch_in_UTC into our local timezone.
> my ($sec, $min, $hour, $mday, $mon, $year,
Re: [PATCH] git-svn: document special options for commit-diff
Thanks.
Re: [PATCH] git-svn: document special options for commit-diff
Andreas Heiduk wrote: > Some options specific for `git svn commit-diff` where not documented > so far. > > Signed-off-by: Andreas Heiduk Thanks again. It's been a while since "commit-diff" :) Signed-off and pushed for Junio: The following changes since commit 02a2850ad58eff6de70eb2dc5f96345c463857ac: Sync with maint (2017-06-13 13:52:53 -0700) are available in the git repository at: git://bogomips.org/git-svn.git svn-doc for you to fetch changes up to da446109ff75120a9836d21653e12154e58f8c46: git-svn: document special options for commit-diff (2017-06-15 01:09:31 +) Andreas Heiduk (1): git-svn: document special options for commit-diff Documentation/git-svn.txt | 15 +++ 1 file changed, 15 insertions(+)
Re: [PATCH] git svn branch fails with authenticaton failures
Hiroshi Shirosaki wrote:
> I have the following authentication failure while svn rebase and
> svn dcommit works fine without authentication failures.
>
> $ git svn branch v7_3
> Copying https://xxx at r27519
> to https:///v7_3...
> Can't create session: Unable to connect to a repository at URL
> 'https://': No more
> credentials or we tried too many times.
> Authentication failed at
> C:\Program Files\Git\mingw64/libexec/git-core\git-svn line 1200.
>
> I can workaround the issue to add auth configuration to
> SVN::Client->new().
Missing sign-off (see Documentation/SubmittingPatches).
Not my rule, but it's unfortunately required for this project.
Also, the Subject: should be in the imperative mood,
Perhaps something like:
Subject: [PATCH] git svn: fix authentication with 'branch'
I am less picky about the message body.
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1175,10 +1175,10 @@ sub cmd_branch {
> ::_req_svn();
> require SVN::Client;
>
> + my ($config, $baton, $callbacks) = Git::SVN::Ra::prepare_config_once();
Since we're not using it, here, you can avoid setting a variable
for $callbacks more explicitly:
my ($config, $baton, undef) = Git::SVN::Ra::prepare_config_once();
> my $ctx = SVN::Client->new(
> - config => SVN::Core::config_get_config(
> - $Git::SVN::Ra::config_dir
> - ),
> + auth => $baton,
> + config => $config,
> log_msg => sub {
> ${ $_[0] } = defined $_message
> ? $_message
> --
Anyways, this looks like a good change. I will accept a v2
with your sign-off and changes noted above. Thank you.
Re: [PATCH] git-svn: escape backslashes in refnames
Eric Wong writes: > Junio: ping? > > https://public-inbox.org/git/20161223014202.GA8327@starla/raw > > Thanks. Thanks for reminding. This indeed fell thru cracks.
Re: [PATCH] git-svn: escape backslashes in refnames
Junio: ping? https://public-inbox.org/git/20161223014202.GA8327@starla/raw Thanks.
Re: [PATCH] git-svn: escape backslashes in refnames
On 2016-12-23 02:42, Eric Wong wrote: > Hi Michael, the patch below should fix things up. Thank you Eric, I was able to successfully fetch the SVN tag with your patch applied. Cheers, -- Michael Fladischer Fladi.at signature.asc Description: OpenPGP digital signature
Re: [PATCH] git-svn: allow "0" in SVN path components
Eric Wong writes:
> Blindly checking a path component for falsiness is unwise, as
> "0" is false to Perl, but a valid pathname component for SVN
> (or any filesystem).
>
> Found via random code reading.
>
> Signed-off-by: Eric Wong
> ---
> Junio: this bugfix should go to "maint".
> Will push along with a doc fix for Juergen.
>
> perl/Git/SVN/Ra.pm | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
> index e764696801..56ad9870bc 100644
> --- a/perl/Git/SVN/Ra.pm
> +++ b/perl/Git/SVN/Ra.pm
> @@ -606,7 +606,7 @@ sub minimize_url {
> my $latest = $ra->get_latest_revnum;
> $ra->get_log("", $latest, 0, 1, 0, 1, sub {});
> };
> - } while ($@ && ($c = shift @components));
> + } while ($@ && defined($c = shift @components));
>
> return canonicalize_url($url);
> }
Makes sense to me. Thanks.
Re: [PATCH] git-svn: do not reuse caches memoized for a different architecture
Junio C Hamano wrote: > Just peeking from the sideline, but the your squash looks like an > improvement to me. Thanks. > Hopefully the final version after your interaction with Dscho can > come to me via another "pull this now"? Not sure if I'll be online the next few days, but I've preeptively pushed the patch + squash to my repo: The following changes since commit 2cc2e70264e0fcba04f9ef791d144bbc8b501206: Eleventh batch for 2.11 (2016-10-26 13:28:47 -0700) are available in the git repository at: git://bogomips.org/git-svn.git svn-cache for you to fetch changes up to a2c761ce5b7a5fd8b505b036f3509a9e6617dee8: git-svn: do not reuse caches memoized for a different architecture (2016-10-27 20:17:36 +) Gavin Lambert (1): git-svn: do not reuse caches memoized for a different architecture perl/Git/SVN.pm | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-)
Re: [PATCH] git-svn: do not reuse caches memoized for a different architecture
Eric Wong writes:
> Johannes Schindelin wrote:
>> +++ b/perl/Git/SVN.pm
>> @@ -1658,6 +1658,11 @@ sub tie_for_persistent_memoization {
>> if ($memo_backend > 0) {
>> tie %$hash => 'Git::SVN::Memoize::YAML', "$path.yaml";
>> } else {
>> +# first verify that any existing file can actually be loaded
>> +# (it may have been saved by an incompatible version)
>> +if (-e "$path.db") {
>> +unlink "$path.db" unless eval { retrieve("$path.db"); 1
>> };
>> +}
>
> That retrieve() call is unlikely to work without "use Storable"
> to import it into the current package.
>
> I also favor setting "$path.db" once to detect typos and avoid
> going over 80 columns. Additionally, having error-checking for
> unlink might be useful.
>
> So perhaps squashing this on top:
>
> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
> index 025c894..b3c1460 100644
> --- a/perl/Git/SVN.pm
> +++ b/perl/Git/SVN.pm
> @@ -1660,10 +1660,15 @@ sub tie_for_persistent_memoization {
> } else {
> # first verify that any existing file can actually be loaded
> # (it may have been saved by an incompatible version)
> - if (-e "$path.db") {
> - unlink "$path.db" unless eval { retrieve("$path.db"); 1
> };
> + my $db = "$path.db";
> + if (-e $db) {
> + use Storable qw(retrieve);
> +
> + if (!eval { retrieve($db); 1 }) {
> + unlink $db or die "unlink $db failed: $!";
> + }
> }
> - tie %$hash => 'Memoize::Storable', "$path.db", 'nstore';
> + tie %$hash => 'Memoize::Storable', $db, 'nstore';
> }
> }
>
>
> Thoughts? Thanks.
Just peeking from the sideline, but the your squash looks like an
improvement to me.
Hopefully the final version after your interaction with Dscho can
come to me via another "pull this now"?
Thanks.
Re: [PATCH] git-svn: do not reuse caches memoized for a different architecture
Johannes Schindelin wrote:
> +++ b/perl/Git/SVN.pm
> @@ -1658,6 +1658,11 @@ sub tie_for_persistent_memoization {
> if ($memo_backend > 0) {
> tie %$hash => 'Git::SVN::Memoize::YAML', "$path.yaml";
> } else {
> + # first verify that any existing file can actually be loaded
> + # (it may have been saved by an incompatible version)
> + if (-e "$path.db") {
> + unlink "$path.db" unless eval { retrieve("$path.db"); 1
> };
> + }
That retrieve() call is unlikely to work without "use Storable"
to import it into the current package.
I also favor setting "$path.db" once to detect typos and avoid
going over 80 columns. Additionally, having error-checking for
unlink might be useful.
So perhaps squashing this on top:
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 025c894..b3c1460 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -1660,10 +1660,15 @@ sub tie_for_persistent_memoization {
} else {
# first verify that any existing file can actually be loaded
# (it may have been saved by an incompatible version)
- if (-e "$path.db") {
- unlink "$path.db" unless eval { retrieve("$path.db"); 1
};
+ my $db = "$path.db";
+ if (-e $db) {
+ use Storable qw(retrieve);
+
+ if (!eval { retrieve($db); 1 }) {
+ unlink $db or die "unlink $db failed: $!";
+ }
}
- tie %$hash => 'Memoize::Storable', "$path.db", 'nstore';
+ tie %$hash => 'Memoize::Storable', $db, 'nstore';
}
}
Thoughts? Thanks.
Re: [PATCH] git-svn: allow --version to work anywhere
Junio C Hamano wrote: > Subject: [PATCH] t9100: portability fix > > Do not say "export VAR=VAL"; "VAR=VAL && export VAR" is always more > portable. Oops, sorry I should've caught that :x -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: allow --version to work anywhere
Junio C Hamano writes: > Eric Wong writes: > >> Repushed my master in case it's a convenient time to pull. >> >> The following changes since commit 29493589e97a2de0c4c1c314f61ccafaee3b5caf: >> >> archive-tar: huge offset and future timestamps would not work on 32-bit >> (2016-07-15 10:51:55 -0700) >> >> are available in the git repository at: >> >> git://bogomips.org/git-svn.git master >> >> for you to fetch changes up to c0071ae5dc1c610ab3791ece7ccf7d4772fde151: >> >> git-svn: allow --version to work anywhere (2016-07-22 20:38:11 +) > > Thanks, pulled. I was too deep in today's integration cycle to waste half-day's work, so I added this hotfix after the merge, directly on 'master'. -- >8 -- Subject: [PATCH] t9100: portability fix Do not say "export VAR=VAL"; "VAR=VAL && export VAR" is always more portable. Signed-off-by: Junio C Hamano --- t/t9100-git-svn-basic.sh | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh index ab6593b..d29f601 100755 --- a/t/t9100-git-svn-basic.sh +++ b/t/t9100-git-svn-basic.sh @@ -24,7 +24,8 @@ ceiling=$PWD test_expect_success 'git svn --version works anywhere' ' mkdir -p "$deepdir" && ( - export GIT_CEILING_DIRECTORIES="$ceiling" && + GIT_CEILING_DIRECTORIES="$ceiling" && + export GIT_CEILING_DIRECTORIES && cd "$deepdir" && git svn --version ) @@ -32,7 +33,8 @@ test_expect_success 'git svn --version works anywhere' ' test_expect_success 'git svn help works anywhere' ' mkdir -p "$deepdir" && ( - export GIT_CEILING_DIRECTORIES="$ceiling" && + GIT_CEILING_DIRECTORIES="$ceiling" && + export GIT_CEILING_DIRECTORIES && cd "$deepdir" && git svn help ) -- 2.9.2-662-gbb82c05 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: allow --version to work anywhere
Eric Wong writes: > Repushed my master in case it's a convenient time to pull. > > The following changes since commit 29493589e97a2de0c4c1c314f61ccafaee3b5caf: > > archive-tar: huge offset and future timestamps would not work on 32-bit > (2016-07-15 10:51:55 -0700) > > are available in the git repository at: > > git://bogomips.org/git-svn.git master > > for you to fetch changes up to c0071ae5dc1c610ab3791ece7ccf7d4772fde151: > > git-svn: allow --version to work anywhere (2016-07-22 20:38:11 +) Thanks, pulled. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: clone: Fail on missing url argument
Eric Wong writes:
> Christopher Layne wrote:
>> * cmd_clone should detect a missing $url arg before using it otherwise
>> an uninitialized value error is emitted in even the simplest case of
>> 'git svn clone' without arguments.
>
> Thanks, this patch looks obviously correct.
>
> I've eliminated the '* ' and space prefix from the version I've
> applied since it's not the convention around here.
>
>> Signed-off-by: Christopher Layne
>
> Signed-off-by: Eric Wong
>
> And pushed to "master" of git://bogomips.org/git-svn
> (I'll request for Junio to pull within a few days while
> other changes pile up).
Thanks.
>> sub cmd_clone {
>> my ($url, $path) = @_;
>> -if (!defined $path &&
>> +if (!$url) {
>> +die "SVN repository location required ",
>> +"as a command-line argument\n";
>
> "as a command-line argument" seems like an unnecessary phrase,
> but I see we use it elsewhere; so it's fine here.
>
> I might be tempted to queue up a separate patch
> to eliminate this extra statement from the rest of git-svn,
> though. Not sure if others feel the same way.
If it _can_ come from somewhere else (perhaps a future enhancement
may allow you to configure where to clone from? Not likely for
cmd_clone but other places in git-svn may be talking about something
that could be configured in the future), then "as a command-line
argument" is not just unnecessary but actively waiting to harm the
users.
But otherwise I do not think anybody cares either way.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: clone: Fail on missing url argument
> On Jul 2, 2016, at 2315 PT, Eric Wong wrote:
>> sub cmd_clone {
>> my ($url, $path) = @_;
>> -if (!defined $path &&
>> +if (!$url) {
>> +die "SVN repository location required ",
>> +"as a command-line argument\n";
>
> "as a command-line argument" seems like an unnecessary phrase,
> but I see we use it elsewhere; so it's fine here.
>
> I might be tempted to queue up a separate patch
> to eliminate this extra statement from the rest of git-svn,
> though. Not sure if others feel the same way.
I basically went with the same logic/error message that cmd_init()
was using a couple of lines down in an attempt to stay consistent:
527 sub cmd_init {
528 if (defined $_stdlayout) {
529 $_trunk = 'trunk' if (!defined $_trunk);
530 @_tags = 'tags' if (! @_tags);
531 @_branches = 'branches' if (! @_branches);
532 }
533 if (defined $_trunk || @_branches || @_tags) {
534 return cmd_multi_init(@_);
535 }
536 my $url = shift or die "SVN repository location required ",
537"as a command-line argument\n";
538 $url = canonicalize_url($url);
539 init_subdir(@_);
540 do_git_init_db();
541
542 if ($Git::SVN::_minimize_url eq 'unset') {
543 $Git::SVN::_minimize_url = 0;
544 }
545
546 Git::SVN->init($url);
547 }
-cl
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: clone: Fail on missing url argument
Christopher Layne wrote:
> * cmd_clone should detect a missing $url arg before using it otherwise
> an uninitialized value error is emitted in even the simplest case of
> 'git svn clone' without arguments.
Thanks, this patch looks obviously correct.
I've eliminated the '* ' and space prefix from the version I've
applied since it's not the convention around here.
> Signed-off-by: Christopher Layne
Signed-off-by: Eric Wong
And pushed to "master" of git://bogomips.org/git-svn
(I'll request for Junio to pull within a few days while
other changes pile up).
> sub cmd_clone {
> my ($url, $path) = @_;
> - if (!defined $path &&
> + if (!$url) {
> + die "SVN repository location required ",
> + "as a command-line argument\n";
"as a command-line argument" seems like an unnecessary phrase,
but I see we use it elsewhere; so it's fine here.
I might be tempted to queue up a separate patch
to eliminate this extra statement from the rest of git-svn,
though. Not sure if others feel the same way.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Git/SVN: die when there is no commit metadata
Christian Couder wrote: > Signed-off-by: Christian Couder Thanks Christian, Signed-off-by: Eric Wong ...And pushed to my svn/bad-ref branch for Junio. (I don't think I'll have other git-svn-related changes immediately pending) The following changes since commit 63a35025b11bf0e7ef39693aeea3b639a066b7b8: Sync with maint (2016-05-06 14:53:45 -0700) are available in the git repository at: git://bogomips.org/git-svn.git svn/bad-ref for you to fetch changes up to 523a33ca17c76bee007d7394fb3930266c577c02: Git/SVN: die when there is no commit metadata (2016-05-08 00:50:19 +) Christian Couder (1): Git/SVN: die when there is no commit metadata perl/Git/SVN.pm | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: shorten glob error message
Eric Wong writes:
> Junio C Hamano wrote:
>> I am not sure if it is a good idea to show */*/* as an example in
>> the message (that is an anti-example of 'one set of wildcard' by
>> having three stars, isn't it?), but that is not a new issue this
>> change introduces.
>
> Actually, going back to commit 570d35c26dfbc40757da6032cdc96afb58cc0037
> ("git-svn: Allow deep branch names by supporting multi-globs"),
> having equal '*' on both sides is all that is required.
>
> Not sure how to improve the wording, though...
I dunno, either, and that is why "not a new issue", iow, the patch
is good as-is. The wording might be an area with possible future
improvement, but that does not have to block the improvement the
patch under discussion brings us.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: shorten glob error message
Junio C Hamano wrote:
> I am not sure if it is a good idea to show */*/* as an example in
> the message (that is an anti-example of 'one set of wildcard' by
> having three stars, isn't it?), but that is not a new issue this
> change introduces.
Actually, going back to commit 570d35c26dfbc40757da6032cdc96afb58cc0037
("git-svn: Allow deep branch names by supporting multi-globs"),
having equal '*' on both sides is all that is required.
Not sure how to improve the wording, though...
> > my $state = "left";
> > - my $die_msg = "Only one set of wildcard directories " .
> > - "(e.g. '*' or '*/*/*') is supported: '$glob'\n";
> > + my $die_msg = "Only one set of wildcards " .
> > + "(e.g. '*' or '*/*/*') is supported: $glob\n";
> > for my $part (split(m|/|, $glob)) {
> > if ($pattern_ok && $part =~ /[{}]/ &&
> > $part !~ /^\{[^{}]+\}/) {
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: shorten glob error message
Hello all, On 01/14/2016 09:15 PM, Junio C Hamano wrote: > Eric Wong writes: > >> Error messages should attempt to fit within the confines of >> an 80-column terminal to avoid compatibility and accessibility >> problems. Furthermore the word "directories" can be misleading >> when used in the context of git refnames. >> >> Signed-off-by: Eric Wong >> --- >>Eric Wong wrote: >>> I also noticed the "Only one set of wildcard directories" error >>> message is unnecessary long and "wildcard directories" should >>> probably be shortened to "wildcards" to avoid wrapping in a terminal. >>> That will probably be a separate patch for me. >> >>There's likely more instances of this in git-svn, but I figured >>we'll get this one fixed, first. >> >>Also pushed to bogomips.org/git-svn.git >>(commit dc6aa7e61e9d33856f54d63b7acb518383420373) >>along with Victor's patch. > Thanks. > > I am not sure if it is a good idea to show */*/* as an example in > the message (that is an anti-example of 'one set of wildcard' by > having three stars, isn't it?), but that is not a new issue this > change introduces. I agree, this should be changed, however I think this should be done in separate patch. Do we have any questions left open before this could be merged into main git repo? -- Victor -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: improve rebase/mkdirs performance
Dair Grant wrote:
> This takes 10 minutes to process when using a single hash, vs
> 3 seconds with a hash of hashes.
>
> Signed-off-by: Dair Grant
Thanks! I've made some minor edits, will push the following out
to git://bogomips.org/git-svn.git for Junio to pull in a day
or so.
Btw, feel free to Cc: me on any git-(svn|Perl)-related stuff. Likewise,
Cc: any regular contributor/maintainer on any subsystem you work on. We
might not always answer, but appreciate being Cc:-ed anyways just in
case.
-- 8< ---
From: Dair Grant
Subject: [PATCH] git-svn: improve rebase/mkdirs performance
Processing empty_dir directives becomes extremely slow for svn
repositories with a large enough history.
This is due to using a single hash to store the list of empty
directories, with the expensive step being purging items from
that hash using grep+delete.
Storing directories in a hash of hashes improves the performance
of this purge step and removes a potentially lengthy delay after
every rebase/mkdirs command.
The svn repository with this behaviour has 110K commits with
unhandled.log containing 170K empty_dir directives.
This takes 10 minutes to process when using a single hash, vs
3 seconds with a hash of hashes.
[ew: wrap comments, shorten initializations, avoid unnecessary spaces]
Signed-off-by: Dair Grant
Signed-off-by: Eric Wong
---
perl/Git/SVN.pm | 84 +++--
1 file changed, 76 insertions(+), 8 deletions(-)
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 152fb7e..b2c14e2 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -1211,20 +1211,87 @@ sub do_fetch {
sub mkemptydirs {
my ($self, $r) = @_;
+ # add/remove/collect a paths table
+ #
+ # Paths are split into a tree of nodes, stored as a hash of hashes.
+ #
+ # Each node contains a 'path' entry for the path (if any) associated
+ # with that node and a 'children' entry for any nodes under that
+ # location.
+ #
+ # Removing a path requires a hash lookup for each component then
+ # dropping that node (and anything under it), which is substantially
+ # faster than a grep slice into a single hash of paths for large
+ # numbers of paths.
+ #
+ # For a large (200K) number of empty_dir directives this reduces
+ # scanning time to 3 seconds vs 10 minutes for grep+delete on a single
+ # hash of paths.
+ sub add_path {
+ my ($paths_table, $path) = @_;
+ my $node_ref;
+
+ foreach my $x (split('/', $path)) {
+ if (!exists($paths_table->{$x})) {
+ $paths_table->{$x} = { children => {} };
+ }
+
+ $node_ref = $paths_table->{$x};
+ $paths_table = $paths_table->{$x}->{children};
+ }
+
+ $node_ref->{path} = $path;
+ }
+
+ sub remove_path {
+ my ($paths_table, $path) = @_;
+ my $nodes_ref;
+ my $node_name;
+
+ foreach my $x (split('/', $path)) {
+ if (!exists($paths_table->{$x})) {
+ return;
+ }
+
+ $nodes_ref = $paths_table;
+ $node_name = $x;
+
+ $paths_table = $paths_table->{$x}->{children};
+ }
+
+ delete($nodes_ref->{$node_name});
+ }
+
+ sub collect_paths {
+ my ($paths_table, $paths_ref) = @_;
+
+ foreach my $v (values %$paths_table) {
+ my $p = $v->{path};
+ my $c = $v->{children};
+
+ collect_paths($c, $paths_ref);
+
+ if (defined($p)) {
+ push(@$paths_ref, $p);
+ }
+ }
+ }
+
sub scan {
- my ($r, $empty_dirs, $line) = @_;
+ my ($r, $paths_table, $line) = @_;
if (defined $r && $line =~ /^r(\d+)$/) {
return 0 if $1 > $r;
} elsif ($line =~ /^ \+empty_dir: (.+)$/) {
- $empty_dirs->{$1} = 1;
+ add_path($paths_table, $1);
} elsif ($line =~ /^ \-empty_dir: (.+)$/) {
- my @d = grep {m[^\Q$1\E(/|$)]} (keys %$empty_dirs);
- delete @$empty_dirs{@d};
+ remove_path($paths_table, $1);
}
1; # continue
};
- my %empty_dirs = ();
+ my @empty_dirs;
+ my %paths_table;
+
my $gz_file = "$self->{dir}/unhandled.log.gz";
if (-f $gz_file) {
if (!can_compress()) {
@@ -1235,7 +1302,7 @@ sub mkemptydirs {
die "Unable to open
RE: [PATCH] git-svn: make batch mode optional for git-cat-file
Hello Eric,
Thanks for all the advices. I have played with several repositories (both on
32bit and 64bit machines). You were correct most of the memory if used by
mapped files and yes it doesn't cause any problems, even a 32bit machine with
500Mb of memory works normally with a heavy loaded git-cat-file.
Thanks also for the advice to use git gc config options, I tested gc.auto=0 and
it lead to the same behavior as my setting MALLOC_LIMIT, however it is more
correct way to get this effect =)
I agree that we shouldn't worry about mapped files.
--
Best Regards,
Victor
From: Eric Wong [normalper...@yhbt.net]
Sent: Wednesday, September 23, 2015 12:22 PM
To: Victor Leschuk
Cc: Junio C Hamano; git@vger.kernel.org
Subject: Re: [PATCH] git-svn: make batch mode optional for git-cat-file
Victor Leschuk wrote:
> Hello Eric, thanks for looking into it.
>
> >> git-cat-file has outgrown the parent perl process several times
> >> (git-cat-file - ~3-4Gb, perl - 400Mb).
>
> > Ugh, that sucks.
> > Even the 400Mb size of Perl annoys me greatly and I'd work
> > on fixing it if I had more time.
>
> I was going to look at this problem also, but first I'd like to improve the
> situation with cat-file as on large repos it is larger problem. By the way,
> what direction would you suggest to begin with?
See below :)
> > > git-cat-file has outgrown the parent perl process several times
> > > (git-cat-file - ~3-4Gb, perl - 400Mb).
>
> > How much of that is anonymous memory, though?
>
> Haven't measured on this particular repo: didn't redo the 2 week
> experiment =) However I checked on a smaller test repo and anon memory
> is about 12M out of 40M total. Most of memory is really taken by
> mmaped *.pack and *idx files.
If it's mmap-ed files, that physical memory is only used on-demand
and can be dropped at any time because it's backed by disk.
In other words, I would not worry about any file-backed mmap at all
(unless you're on 32-bit, but I think git has workarounds for that)
Do you still have that giant repo around?
Are the combined size of the pack + idx files are at least 3-4 GB?
This should cat all the blobs in history without re-running git-svn:
git log --all --raw -r --no-abbrev | \
awk '/^:/ {print $3; print $4}' | git cat-file --batch
git log actually keeps growing, but the cat-file process shouldn't
use anonymous memory much if you inspect it with pmap.
> Actually I accidentally found out that if I export GIT_MALLOC_LIMIT
> variable set to several megabytes it has the following effect:
> * git-svn.perl launches git-gc
> * git-gc can't allocate enough memory and thus doesn't create any pack files
> * git-cat-file works only with pure blob object, not packs, and it's
> memory usage doesn't grow larger than 4-5M
>
> It gave me a thought that maybe we could get rid of "git gc" calls
> after each commit in perl code and just perform one large gc operation
> at the end. It will cost disk space during clone but save us memory.
> What do you think?
You can set gc.auto to zero in your $GIT_CONFIG to disable gc.
The "git gc" calls were added because unpacked repos were growing
too large and caused problems for other people.
Perhaps play with some other pack* options documented in
Documentation/config to limit maximum pack size/depth.
Is this a 32-bit or 64-bit system?
> As for your suggestion regarding periodic restart of batch process
> inside git-cat-file, I think we could give it a try, I can prepare a
> patch and run some tests.
I am not sure if we need it for git-svn.
In another project, the only reason I've found to restart
"cat-file --batch" is in case the repo got repacked and old packs
got unlinked, cat-file would hold a reference onto the old file
and suck up space. It might be better if "cat-file --batch" learned
to detect unlinked files and then munmap + close them.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: make batch mode optional for git-cat-file
Victor Leschuk wrote:
> Hello Eric, thanks for looking into it.
>
> >> git-cat-file has outgrown the parent perl process several times
> >> (git-cat-file - ~3-4Gb, perl - 400Mb).
>
> > Ugh, that sucks.
> > Even the 400Mb size of Perl annoys me greatly and I'd work
> > on fixing it if I had more time.
>
> I was going to look at this problem also, but first I'd like to improve the
> situation with cat-file as on large repos it is larger problem. By the way,
> what direction would you suggest to begin with?
See below :)
> > > git-cat-file has outgrown the parent perl process several times
> > > (git-cat-file - ~3-4Gb, perl - 400Mb).
>
> > How much of that is anonymous memory, though?
>
> Haven't measured on this particular repo: didn't redo the 2 week
> experiment =) However I checked on a smaller test repo and anon memory
> is about 12M out of 40M total. Most of memory is really taken by
> mmaped *.pack and *idx files.
If it's mmap-ed files, that physical memory is only used on-demand
and can be dropped at any time because it's backed by disk.
In other words, I would not worry about any file-backed mmap at all
(unless you're on 32-bit, but I think git has workarounds for that)
Do you still have that giant repo around?
Are the combined size of the pack + idx files are at least 3-4 GB?
This should cat all the blobs in history without re-running git-svn:
git log --all --raw -r --no-abbrev | \
awk '/^:/ {print $3; print $4}' | git cat-file --batch
git log actually keeps growing, but the cat-file process shouldn't
use anonymous memory much if you inspect it with pmap.
> Actually I accidentally found out that if I export GIT_MALLOC_LIMIT
> variable set to several megabytes it has the following effect:
> * git-svn.perl launches git-gc
> * git-gc can't allocate enough memory and thus doesn't create any pack files
> * git-cat-file works only with pure blob object, not packs, and it's
> memory usage doesn't grow larger than 4-5M
>
> It gave me a thought that maybe we could get rid of "git gc" calls
> after each commit in perl code and just perform one large gc operation
> at the end. It will cost disk space during clone but save us memory.
> What do you think?
You can set gc.auto to zero in your $GIT_CONFIG to disable gc.
The "git gc" calls were added because unpacked repos were growing
too large and caused problems for other people.
Perhaps play with some other pack* options documented in
Documentation/config to limit maximum pack size/depth.
Is this a 32-bit or 64-bit system?
> As for your suggestion regarding periodic restart of batch process
> inside git-cat-file, I think we could give it a try, I can prepare a
> patch and run some tests.
I am not sure if we need it for git-svn.
In another project, the only reason I've found to restart
"cat-file --batch" is in case the repo got repacked and old packs
got unlinked, cat-file would hold a reference onto the old file
and suck up space. It might be better if "cat-file --batch" learned
to detect unlinked files and then munmap + close them.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] git-svn: make batch mode optional for git-cat-file
Hello Eric, thanks for looking into it. >> git-cat-file has outgrown the parent perl process several times >> (git-cat-file - ~3-4Gb, perl - 400Mb). > Ugh, that sucks. > Even the 400Mb size of Perl annoys me greatly and I'd work > on fixing it if I had more time. I was going to look at this problem also, but first I'd like to improve the situation with cat-file as on large repos it is larger problem. By the way, what direction would you suggest to begin with? > A few more questions: > * What is the largest file that existed in that repo? About 2.5M > * Did you try "MALLOC_MMAP_THRESHOLD_" with glibc? Have just tried it on a smaller repo (which takes about 1 hour to clone and RSS grows from 4M to 40M during the process. Unfortunately there is no much of an effect: max RSS is 41M with default settings and 38M with MALLOC_MMAP_THRESHOLD_=131072. > If alloc.c is the culprit, I would consider to transparently restart "cat-file --batch" once it grows to a certain size or after a certain number of requests are made to it. alloc.c interface is not used in cat-file at all, only direct calls to xmalloc and xrealloc from wrapper.c, and also xmmap() from sha1_file.c. > > git-cat-file has outgrown the parent perl process several times > > (git-cat-file - ~3-4Gb, perl - 400Mb). > How much of that is anonymous memory, though? Haven't measured on this particular repo: didn't redo the 2 week experiment =) However I checked on a smaller test repo and anon memory is about 12M out of 40M total. Most of memory is really taken by mmaped *.pack and *idx files. Actually I accidentally found out that if I export GIT_MALLOC_LIMIT variable set to several megabytes it has the following effect: * git-svn.perl launches git-gc * git-gc can't allocate enough memory and thus doesn't create any pack files * git-cat-file works only with pure blob object, not packs, and it's memory usage doesn't grow larger than 4-5M It gave me a thought that maybe we could get rid of "git gc" calls after each commit in perl code and just perform one large gc operation at the end. It will cost disk space during clone but save us memory. What do you think? As for your suggestion regarding periodic restart of batch process inside git-cat-file, I think we could give it a try, I can prepare a patch and run some tests. -- Best Regards, Victor From: Eric Wong [normalper...@yhbt.net] Sent: Tuesday, September 22, 2015 5:35 PM To: Victor Leschuk Cc: Junio C Hamano; git@vger.kernel.org Subject: Re: [PATCH] git-svn: make batch mode optional for git-cat-file Eric Wong wrote: > Victor Leschuk wrote: > > The thing is that git-cat-file keeps growing during work when running > > in "batch" mode. See the figure attached: it is for cloning a rather > > small repo (1 hour to clone about ~14000 revisions). However the clone > > of a large repo (~28 revisions) took about 2 weeks and > > git-cat-file has outgrown the parent perl process several times > > (git-cat-file - ~3-4Gb, perl - 400Mb). How much of that is anonymous memory, though? (pmap $PID_OF_GIT_CAT_FILE) Running the following on the Linux kernel tree I had lying around: (for i in $(seq 100 200); do git ls-files | sed -e "s/^/HEAD~$i:/"; done)|\ git cat-file --batch >/dev/null Reveals about 510M RSS in top, but pmap says less than 20M of that is anonymous. So the rest are mmap-ed packfiles; that RSS gets transparently released back to the kernel under memory pressure. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: make batch mode optional for git-cat-file
Eric Wong wrote: > Victor Leschuk wrote: > > The thing is that git-cat-file keeps growing during work when running > > in "batch" mode. See the figure attached: it is for cloning a rather > > small repo (1 hour to clone about ~14000 revisions). However the clone > > of a large repo (~28 revisions) took about 2 weeks and > > git-cat-file has outgrown the parent perl process several times > > (git-cat-file - ~3-4Gb, perl - 400Mb). How much of that is anonymous memory, though? (pmap $PID_OF_GIT_CAT_FILE) Running the following on the Linux kernel tree I had lying around: (for i in $(seq 100 200); do git ls-files | sed -e "s/^/HEAD~$i:/"; done)|\ git cat-file --batch >/dev/null Reveals about 510M RSS in top, but pmap says less than 20M of that is anonymous. So the rest are mmap-ed packfiles; that RSS gets transparently released back to the kernel under memory pressure. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: make batch mode optional for git-cat-file
Victor Leschuk wrote: > The thing is that git-cat-file keeps growing during work when running > in "batch" mode. See the figure attached: it is for cloning a rather > small repo (1 hour to clone about ~14000 revisions). However the clone > of a large repo (~28 revisions) took about 2 weeks and > git-cat-file has outgrown the parent perl process several times > (git-cat-file - ~3-4Gb, perl - 400Mb). Ugh, that sucks. Even the 400Mb size of Perl annoys me greatly and I'd work on fixing it if I had more time. But I'm completely against adding this parameter to git-svn. git-svn is not the only "cat-file --batch" user, so this option is only hiding problems. The best choice is to figure out why cat-file is wasting memory. Disclaimer: I'm no expert on parts of git written in C, but perhaps the alloc.c interface is why memory keeps growing. > What was done: > * I have run it under valgrind and mtrace and haven't found any memory leaks > * Found the source of most number of memory reallocations > (batch_object_write() function (strbuf_expand -> realloc)) - tried to make > the streambuf object static and avoid reallocs - didn't help > * Tried preloading other allocators than standard glibc - no significant > difference A few more questions: * What is the largest file that existed in that repo? * Did you try "MALLOC_MMAP_THRESHOLD_" with glibc? Perhaps setting that to 131072 will help, that'll force releasing larger chunks than that; but it might be moot if alloc.c is getting in the way. If alloc.c is the culprit, I would consider to transparently restart "cat-file --batch" once it grows to a certain size or after a certain number of requests are made to it. We can probably do this inside "git cat-file" itself without changing any callers by calling execve. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: make batch mode optional for git-cat-file
Victor Leschuk writes: > We already do have some of these: 'no-metadata', 'no-checkout', > no-auth-cache'. So I was just following the existing convention. Do > you think we need to change it and stick with > --catch-file-batch=1/--cat-file-batch=0 ? Inventing a new --cat-file-batch=[0|1] is not a good idea, and certainly not what I would suggest at all. My suggestion was to accept --cat-file-batch to allow the --batch processing, and to accept--no-cat-file-batch to trigger your new codepath (and leave --cat-file-batch the default when neither is given). As these option descriptions are eventually passed to Getopt::Long, I thought it should not be too hard to arrange. Mimicking the existing handling of no-whatever is less bad than accepting --cat-file-batch=[0|1], if you cannot tell the code to take --[no-]cat-file-batch for whatever reason. In the longer term it would need to be cleaned up together with existing ones. Your patch would be adding another instance that needs to be cleaned up to that existing pile, but as long as it follows the same pattern as existing ones, it is easier to spot what needs to be fixed later. Compared to that, accepting --cat-file-batch=[0|1] would be far worse, as such a future clean-up effort can miss it due to its not following the same pattern. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] git-svn: make batch mode optional for git-cat-file
As for your remark regarding the option naming:
> An option whose name begins with no- looks somewhat strange. You
can even say --no-no-cat-file-batch from the command line, I
suspect.
We already do have some of these: 'no-metadata', 'no-checkout',
'no-auth-cache'. So I was just following the existing convention. Do you think
we need to change it and stick with --catch-file-batch=1/--cat-file-batch=0 ?
--
Best Regards,
Victor
From: Victor Leschuk
Sent: Monday, September 21, 2015 3:03 PM
To: Junio C Hamano
Cc: git@vger.kernel.org
Subject: RE: [PATCH] git-svn: make batch mode optional for git-cat-file
Hello Junio,
thanks for your review. First of all I'd like to apologize for sending the
patch without description. Actually I was in a hurry and sent it by accident: I
planned to edit the mail before sending...
Here is the detailed description:
Last week we had a quick discussion in this mailing list:
http://thread.gmane.org/gmane.comp.version-control.git/278021 .
The thing is that git-cat-file keeps growing during work when running in
"batch" mode. See the figure attached: it is for cloning a rather small repo (1
hour to clone about ~14000 revisions). However the clone of a large repo
(~28 revisions) took about 2 weeks and git-cat-file has outgrown the parent
perl process several times (git-cat-file - ~3-4Gb, perl - 400Mb).
What was done:
* I have run it under valgrind and mtrace and haven't found any memory leaks
* Found the source of most number of memory reallocations
(batch_object_write() function (strbuf_expand -> realloc)) - tried to make the
streambuf object static and avoid reallocs - didn't help
* Tried preloading other allocators than standard glibc - no significant
difference
After that I replaced the batch mode with separate cat-file calls for each blob
and it didn't have any impact on clone performance on real code repositories.
However I created a fake test repo with large number of small files (~10 bytes
each): here is how I created it https://bitbucket.org/vleschuk/svngenrepo
And on this artificial test repo it really slowed down the process. So I
decided to suggest to make the batch mode optional to let the user "tune" the
process and created a patch for this.
As for your code-style notes, I agree with them, will adjust the code.
--
Best Regards,
Victor
From: Junio C Hamano [jch2...@gmail.com] On Behalf Of Junio C Hamano
[gits...@pobox.com]
Sent: Monday, September 21, 2015 11:25 AM
To: Victor Leschuk
Cc: git@vger.kernel.org; Victor Leschuk
Subject: Re: [PATCH] git-svn: make batch mode optional for git-cat-file
Victor Leschuk writes:
> Signed-off-by: Victor Leschuk
> ---
Before the S-o-b line is a good place to explain why this is a good
change to have. Please use it.
> git-svn.perl | 1 +
> perl/Git.pm | 41 -
> 2 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index 36f7240..b793c26 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -139,6 +139,7 @@ my %fc_opts = ( 'follow-parent|follow!' =>
> \$Git::SVN::_follow_parent,
> 'use-log-author' => \$Git::SVN::_use_log_author,
> 'add-author-from' => \$Git::SVN::_add_author_from,
> 'localtime' => \$Git::SVN::_localtime,
> + 'no-cat-file-batch' => sub { $Git::no_cat_file_batch = 1; },
An option whose name begins with no- looks somewhat strange. You
can even say --no-no-cat-file-batch from the command line, I
suspect.
Why not give an option 'cat-file-batch' that sets the variable
$Git::cat_file_batch to false, and initialize the variable to true
to keep existing users who do not pass the option happy?
> %remote_opts );
>
> my ($_trunk, @_tags, @_branches, $_stdlayout);
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 19ef081..69e5293 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -107,6 +107,7 @@ use Fcntl qw(SEEK_SET SEEK_CUR);
> use Time::Local qw(timegm);
> }
>
> +our $no_cat_file_batch = 0;
>
> =head1 CONSTRUCTORS
>
> @@ -1012,6 +1013,10 @@ returns the number of bytes printed.
> =cut
>
> sub cat_blob {
> + (1 == $no_cat_file_batch) ? _cat_blob_cmd(@_) : _cat_blob_batch(@_);
Discard "1 ==" here. You are clearly using the variable as a
boolean, so writing this as
$cat_file_batch ? _cat_blob_batch(@_) : _cat_blob_cmd(@_);
or better yet
if ($cat_file_batch) {
_cat_blob_batch(@_);
} else {
_cat_blob_cmd(@_);
}
would be more natural.
> +}
> +
> +sub _cat_blob_batch {
> my ($self, $sha1, $f
RE: [PATCH] git-svn: make batch mode optional for git-cat-file
Hello Junio,
thanks for your review. First of all I'd like to apologize for sending the
patch without description. Actually I was in a hurry and sent it by accident: I
planned to edit the mail before sending...
Here is the detailed description:
Last week we had a quick discussion in this mailing list:
http://thread.gmane.org/gmane.comp.version-control.git/278021 .
The thing is that git-cat-file keeps growing during work when running in
"batch" mode. See the figure attached: it is for cloning a rather small repo (1
hour to clone about ~14000 revisions). However the clone of a large repo
(~28 revisions) took about 2 weeks and git-cat-file has outgrown the parent
perl process several times (git-cat-file - ~3-4Gb, perl - 400Mb).
What was done:
* I have run it under valgrind and mtrace and haven't found any memory leaks
* Found the source of most number of memory reallocations
(batch_object_write() function (strbuf_expand -> realloc)) - tried to make the
streambuf object static and avoid reallocs - didn't help
* Tried preloading other allocators than standard glibc - no significant
difference
After that I replaced the batch mode with separate cat-file calls for each blob
and it didn't have any impact on clone performance on real code repositories.
However I created a fake test repo with large number of small files (~10 bytes
each): here is how I created it https://bitbucket.org/vleschuk/svngenrepo
And on this artificial test repo it really slowed down the process. So I
decided to suggest to make the batch mode optional to let the user "tune" the
process and created a patch for this.
As for your code-style notes, I agree with them, will adjust the code.
--
Best Regards,
Victor
From: Junio C Hamano [jch2...@gmail.com] On Behalf Of Junio C Hamano
[gits...@pobox.com]
Sent: Monday, September 21, 2015 11:25 AM
To: Victor Leschuk
Cc: git@vger.kernel.org; Victor Leschuk
Subject: Re: [PATCH] git-svn: make batch mode optional for git-cat-file
Victor Leschuk writes:
> Signed-off-by: Victor Leschuk
> ---
Before the S-o-b line is a good place to explain why this is a good
change to have. Please use it.
> git-svn.perl | 1 +
> perl/Git.pm | 41 -
> 2 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index 36f7240..b793c26 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -139,6 +139,7 @@ my %fc_opts = ( 'follow-parent|follow!' =>
> \$Git::SVN::_follow_parent,
> 'use-log-author' => \$Git::SVN::_use_log_author,
> 'add-author-from' => \$Git::SVN::_add_author_from,
> 'localtime' => \$Git::SVN::_localtime,
> + 'no-cat-file-batch' => sub { $Git::no_cat_file_batch = 1; },
An option whose name begins with no- looks somewhat strange. You
can even say --no-no-cat-file-batch from the command line, I
suspect.
Why not give an option 'cat-file-batch' that sets the variable
$Git::cat_file_batch to false, and initialize the variable to true
to keep existing users who do not pass the option happy?
> %remote_opts );
>
> my ($_trunk, @_tags, @_branches, $_stdlayout);
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 19ef081..69e5293 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -107,6 +107,7 @@ use Fcntl qw(SEEK_SET SEEK_CUR);
> use Time::Local qw(timegm);
> }
>
> +our $no_cat_file_batch = 0;
>
> =head1 CONSTRUCTORS
>
> @@ -1012,6 +1013,10 @@ returns the number of bytes printed.
> =cut
>
> sub cat_blob {
> + (1 == $no_cat_file_batch) ? _cat_blob_cmd(@_) : _cat_blob_batch(@_);
Discard "1 ==" here. You are clearly using the variable as a
boolean, so writing this as
$cat_file_batch ? _cat_blob_batch(@_) : _cat_blob_cmd(@_);
or better yet
if ($cat_file_batch) {
_cat_blob_batch(@_);
} else {
_cat_blob_cmd(@_);
}
would be more natural.
> +}
> +
> +sub _cat_blob_batch {
> my ($self, $sha1, $fh) = @_;
>
> $self->_open_cat_blob_if_needed();
> @@ -1072,7 +1077,7 @@ sub cat_blob {
> sub _open_cat_blob_if_needed {
> my ($self) = @_;
>
> - return if defined($self->{cat_blob_pid});
> + return if ( defined($self->{cat_blob_pid}) || 1 == $no_cat_file_batch );
Likewise.
return if (!$cat_file_batch);
return if defined($self->{cat_blob_pid});
> +sub _cat_blob_cmd {
> + my ($self, $sha1, $fh) = @_;
> +...
The biggest thing that is missing from this patch is the explanation
of why this is a good thing to do. The batch interface was invented
because people found that it was wasteful to spawn
Re: [PATCH] git-svn: make batch mode optional for git-cat-file
Victor Leschuk writes:
> Signed-off-by: Victor Leschuk
> ---
Before the S-o-b line is a good place to explain why this is a good
change to have. Please use it.
> git-svn.perl | 1 +
> perl/Git.pm | 41 -
> 2 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index 36f7240..b793c26 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -139,6 +139,7 @@ my %fc_opts = ( 'follow-parent|follow!' =>
> \$Git::SVN::_follow_parent,
> 'use-log-author' => \$Git::SVN::_use_log_author,
> 'add-author-from' => \$Git::SVN::_add_author_from,
> 'localtime' => \$Git::SVN::_localtime,
> + 'no-cat-file-batch' => sub { $Git::no_cat_file_batch = 1; },
An option whose name begins with no- looks somewhat strange. You
can even say --no-no-cat-file-batch from the command line, I
suspect.
Why not give an option 'cat-file-batch' that sets the variable
$Git::cat_file_batch to false, and initialize the variable to true
to keep existing users who do not pass the option happy?
> %remote_opts );
>
> my ($_trunk, @_tags, @_branches, $_stdlayout);
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 19ef081..69e5293 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -107,6 +107,7 @@ use Fcntl qw(SEEK_SET SEEK_CUR);
> use Time::Local qw(timegm);
> }
>
> +our $no_cat_file_batch = 0;
>
> =head1 CONSTRUCTORS
>
> @@ -1012,6 +1013,10 @@ returns the number of bytes printed.
> =cut
>
> sub cat_blob {
> + (1 == $no_cat_file_batch) ? _cat_blob_cmd(@_) : _cat_blob_batch(@_);
Discard "1 ==" here. You are clearly using the variable as a
boolean, so writing this as
$cat_file_batch ? _cat_blob_batch(@_) : _cat_blob_cmd(@_);
or better yet
if ($cat_file_batch) {
_cat_blob_batch(@_);
} else {
_cat_blob_cmd(@_);
}
would be more natural.
> +}
> +
> +sub _cat_blob_batch {
> my ($self, $sha1, $fh) = @_;
>
> $self->_open_cat_blob_if_needed();
> @@ -1072,7 +1077,7 @@ sub cat_blob {
> sub _open_cat_blob_if_needed {
> my ($self) = @_;
>
> - return if defined($self->{cat_blob_pid});
> + return if ( defined($self->{cat_blob_pid}) || 1 == $no_cat_file_batch );
Likewise.
return if (!$cat_file_batch);
return if defined($self->{cat_blob_pid});
> +sub _cat_blob_cmd {
> + my ($self, $sha1, $fh) = @_;
> +...
The biggest thing that is missing from this patch is the explanation
of why this is a good thing to do. The batch interface was invented
because people found that it was wasteful to spawn a new cat-file
process every time the contents of a blob is needed and wanted to
avoid it, and this new feature gives the user a way to tell Git to
do things in a "wasteful" way, so there must be a reason why the
user would want to use the "wasteful" way, perhaps work around some
other issue. Without explaining that in the documentation what that
issue is, i.e. telling users who reads "git svn --help" when and why
the option might help them, nobody would use the feature to benefit
from it.
I wonder if "cat-file --batch" is leaky and bloats after running for
a while. If that is the case, I have to wonder if "never do batch"
like this patch does is a sensible way forward. Instead, "recycle
and renew the process after running it for N requests" (and ideally
auto-adjust that N without being told by the user) might be a better
way to do what you are trying to achieve, but as I already said, I
cannot read the motivation behind this change that is not explained,
so...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: parse authors file more leniently
Eric Wong venit, vidit, dixit 10.09.2015 20:08: > Michael J Gruber wrote: >> Instead, make git svn uses the perl regex >> >> /^(.+?|\(no author\))\s*=\s*(.+?)\s*<(.*)>\s*$/ >> >> for parsing the authors file so that the same (slightly more lenient) >> regex is used in both cases. >> >> Reported-by: Till Schäfer >> Signed-off-by: Michael J Gruber > > Thanks. > Signed-off-by: Eric Wong > > And pushed to master of git://bogomips.org/git-svn > (commit f7c6de0ea1bd5722a1181c6279676c6831b38a34) > > By the way, I also had some other patches sitting around for you. > Did you ever have time to revisit them? (I haven't) > > t/lib-httpd: load mod_unixd > t/lib-git-svn: check same httpd module dirs as lib-httpd > Also "from me". Short answer: No If I remember correctly, they were correct bit not complete in the sense that on a standard Fedora install (with newer apache), svn tests still wouldn't run over http. But I/we learned that those tests were simply run over local file protocol instead when svn over http didn't work. On a standard debian install (which apparantly has non-standard, thus downwards compatible apache config) everything was fine with or without those patches. I still plan to look at them when I find time. (I'll be retiring sometime between 20 and 30 years from now, so there's hope.) Michael -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: parse authors file more leniently
Eric Wong writes: > Michael J Gruber wrote: >> Instead, make git svn uses the perl regex >> >> /^(.+?|\(no author\))\s*=\s*(.+?)\s*<(.*)>\s*$/ >> >> for parsing the authors file so that the same (slightly more lenient) >> regex is used in both cases. >> >> Reported-by: Till Schäfer >> Signed-off-by: Michael J Gruber > > Thanks. > Signed-off-by: Eric Wong > > And pushed to master of git://bogomips.org/git-svn > (commit f7c6de0ea1bd5722a1181c6279676c6831b38a34) Thanks, pulled. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: parse authors file more leniently
Michael J Gruber wrote: > Instead, make git svn uses the perl regex > > /^(.+?|\(no author\))\s*=\s*(.+?)\s*<(.*)>\s*$/ > > for parsing the authors file so that the same (slightly more lenient) > regex is used in both cases. > > Reported-by: Till Schäfer > Signed-off-by: Michael J Gruber Thanks. Signed-off-by: Eric Wong And pushed to master of git://bogomips.org/git-svn (commit f7c6de0ea1bd5722a1181c6279676c6831b38a34) By the way, I also had some other patches sitting around for you. Did you ever have time to revisit them? (I haven't) t/lib-httpd: load mod_unixd t/lib-git-svn: check same httpd module dirs as lib-httpd -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: fix localtime=true on non-glibc environments
Junio C Hamano wrote:
> Ryuichi Kokubo writes:
>
> > git svn uses POSIX::strftime('%s', $sec, $min, ...) to make unix epoch time.
> > But lowercase %s formatting character is a GNU extention. This causes
> > problem
> > in git svn fetch --localtime on non-glibc systems, such as msys or cygwin.
> > Using Time::Local::timelocal($sec, $min, ...) fixes it.
> >
> > Signed-off-by: Ryuichi Kokubo
>
> Sounds sensible.
>
> Because we already have "use Time::Local qw(...)" in perl/Git.pm
> that is used by git-svn, we know the platforms that are capable
> of running the current git-svn do have Time::Local available, so
> I do not have worries about new dependency, either.
>
> Eric?
Looks good, thanks. Signed-off and queued up.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: fix localtime=true on non-glibc environments
Ryuichi Kokubo writes:
> git svn uses POSIX::strftime('%s', $sec, $min, ...) to make unix epoch time.
> But lowercase %s formatting character is a GNU extention. This causes problem
> in git svn fetch --localtime on non-glibc systems, such as msys or cygwin.
> Using Time::Local::timelocal($sec, $min, ...) fixes it.
>
> Signed-off-by: Ryuichi Kokubo
Sounds sensible.
Because we already have "use Time::Local qw(...)" in perl/Git.pm
that is used by git-svn, we know the platforms that are capable
of running the current git-svn do have Time::Local available, so
I do not have worries about new dependency, either.
Eric?
>
> Notes:
> lowercase %s format character in strftime is a GNU extension and not
> widely supported.
> POSIX::strftime affected by underlying crt's strftime because
> POSIX::strftime just calls crt's one.
> Time::Local is good function to replace POSIX::strftime because it's a
> perl core module function.
>
> Document about Time::Local.
> http://perldoc.perl.org/Time/Local.html
>
> These are specifications of strftime.
>
> The GNU C Library Reference Manual.
>
> http://www.gnu.org/software/libc/manual/html_node/Formatting-Calendar-Time.html
>
> perl POSIX module's strftime document. It does not have '%s'.
> http://perldoc.perl.org/POSIX.html
>
> strftime document of Microsort Windows C Run-Time library.
> https://msdn.microsoft.com/en-us/library/fe06s4ak.aspx
>
> The Open Group's old specification does not have '%s' too.
> http://pubs.opengroup.org/onlinepubs/007908799/xsh/strftime.html
>
> On my environment, following problems happened.
> - msys : git svn fetch does not progress at all with perl.exe consuming
> CPU.
> - cygwin : git svn fetch progresses but time stamp information is dropped.
>Every commits have unix epoch timestamp.
>
> I would like to thank git developer and contibutors.
> git helps me so much everyday.
> Thank you.
> ---
> perl/Git/SVN.pm |3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
> index 8e4af71..f243726 100644
> --- a/perl/Git/SVN.pm
> +++ b/perl/Git/SVN.pm
> @@ -14,6 +14,7 @@ use IPC::Open3;
> use Memoize; # core since 5.8.0, Jul 2002
> use Memoize::Storable;
> use POSIX qw(:signal_h);
> +use Time::Local;
>
> use Git qw(
> command
> @@ -1332,7 +1333,7 @@ sub parse_svn_date {
> $ENV{TZ} = 'UTC';
>
> my $epoch_in_UTC =
> - POSIX::strftime('%s', $S, $M, $H, $d, $m - 1, $Y - 1900);
> + Time::Local::timelocal($S, $M, $H, $d, $m - 1, $Y - 1900);
>
> # Determine our local timezone (including DST) at the
> # time of $epoch_in_UTC. $Git::SVN::Log::TZ stored the
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: show progress in working_head_info()
Eric, I'm sorry, but this change isn't important enough for me to follow up. Please merge the other two patches, if you think they're good. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: show progress in working_head_info()
Ramkumar Ramachandra wrote:
> The working_head_info routine takes a very long time to execute on large
> repositories. The least we can do is to comfort users that some progress
> is being made.
>
> Signed-off-by: Ramkumar Ramachandra
> ---
> I was worried because of the long wait, so I wrote this to convince
> myself that git-svn wasn't stuck.
Unfortunately, this breaks show-ignore and t9101-git-svn-props.sh
show-ignore.expect show-ignore.got differ: char 1, line 1
not ok 23 - test show-ignore
Perhaps output should go to STDERR and also respect $_q?
> git-svn.perl | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index 60f8814..6aa156c 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -2022,6 +2022,9 @@ sub working_head_info {
> next;
> }
> next unless s{^\s*(git-svn-id:)}{$1};
> + my $chomped = $_;
> + chomp $chomped;
> + print "[Importing] $chomped\n";
> my ($url, $rev, $uuid) = extract_metadata($_);
> if (defined $url && defined $rev) {
> next if $max{$url} and $max{$url} < $rev;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Git::SVN: handle missing ref_id case correctly
Ramkumar Ramachandra wrote: >> It is functional, maybe someone will use GIT_SVN_ID=0 ? > > Right. Kindly drop the first hunk. Amendment: there are actually many other places where variables are checked without "defined", so I doubt we want to put up with the extra ugliness to allow "0". -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Git::SVN: handle missing ref_id case correctly
Eric Wong wrote: > It is functional, maybe someone will use GIT_SVN_ID=0 ? Right. Kindly drop the first hunk. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: make it play nicely with submodules
Eric Wong wrote: > How portable is open on a directory? Perhaps it'd be better to > check if it's a file, first: Sure, that works; feel free to fix it up locally before committing. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Git::SVN: handle missing ref_id case correctly
Ramkumar Ramachandra wrote:
> Ramkumar Ramachandra wrote:
> > -our $default_ref_id = $ENV{GIT_SVN_ID} || 'git-svn';
> > +our $default_ref_id = defined $ENV{GIT_SVN_ID} ? $ENV{GIT_SVN_ID} :
> > 'git-svn';
>
> This is probably not a functional change; please look at the second hunk.
It is functional, maybe someone will use GIT_SVN_ID=0 ?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: make it play nicely with submodules
Ramkumar Ramachandra wrote:
> +++ b/git-svn.perl
> @@ -337,6 +337,10 @@ for (my $i = 0; $i < @ARGV; $i++) {
> # make sure we're always running at the top-level working directory
> if ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {
> $ENV{GIT_DIR} ||= ".git";
> + # catch the submodule case
> + if (open(my $fh, '<', $ENV{GIT_DIR})) {
> + $ENV{GIT_DIR} = $1 if <$fh> =~ /^gitdir: (.+)$/;
> + }
How portable is open on a directory? Perhaps it'd be better to
check if it's a file, first:
if (-f $ENV{GIT_DIR}) {
open(my $fh, '<', $ENV{GIT_DIR}) or
die "failed to open $ENV{GIT_DIR}: $!\n";
$ENV{GIT_DIR} = $1 if <$fh> =~ /^gitdir: (.+)$/;
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Git::SVN: handle missing ref_id case correctly
Ramkumar Ramachandra wrote:
> -our $default_ref_id = $ENV{GIT_SVN_ID} || 'git-svn';
> +our $default_ref_id = defined $ENV{GIT_SVN_ID} ? $ENV{GIT_SVN_ID} :
> 'git-svn';
This is probably not a functional change; please look at the second hunk.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: Support for git-svn propset
> On Dec 8, 2014, at 1:36 PM, Eric Wong wrote:
>
> Alfred Perlstein wrote:
>> Appearing here:
>> http://marc.info/?l=git&m=125259772625008&w=2
>
> Probably better to use a mid URL here, too
>
> http://mid.gmane.org/1927112650.1281253084529659.javamail.r...@klofta.sjsoft.com
>
> such a long URL, though...
>
>> --- a/perl/Git/SVN/Editor.pm
>> +++ b/perl/Git/SVN/Editor.pm
>> @@ -288,6 +288,44 @@ sub apply_autoprops {
>>}
>> }
>>
>> +sub check_attr {
>> +my ($attr,$path) = @_;
>> +my $fh = command_output_pipe("check-attr", $attr, "--", $path);
>> +return undef if (!$fh);
>> +
>> +my $val = <$fh>;
>> +close $fh;
>> +if ($val) { $val =~ s/^[^:]*:\s*[^:]*:\s*(.*)\s*$/$1/; }
>> +return $val;
>> +}
>
> I just noticed command_output_pipe didn't use a corresponding
> command_close_pipe to check for errors, but command_oneline is even
> better. I'll squash the following:
>
> --- a/perl/Git/SVN/Editor.pm
> +++ b/perl/Git/SVN/Editor.pm
> @@ -290,11 +290,7 @@ sub apply_autoprops {
>
> sub check_attr {
>my ($attr,$path) = @_;
> -my $fh = command_output_pipe("check-attr", $attr, "--", $path);
> -return undef if (!$fh);
> -
> -my $val = <$fh>;
> -close $fh;
> +my $val = command_oneline("check-attr", $attr, "--", $path);
>if ($val) { $val =~ s/^[^:]*:\s*[^:]*:\s*(.*)\s*$/$1/; }
>return $val;
> }
>
> In your test, "local" isn't portable, unfortunately, but tests seem to
> work fine without local so I've removed them:
>
> --- a/t/t9148-git-svn-propset.sh
> +++ b/t/t9148-git-svn-propset.sh
> @@ -29,10 +29,9 @@ test_expect_success 'fetch revisions from svn' '
>git svn fetch
>'
>
> -set_props()
> -{
> -local subdir="$1"
> -local file="$2"
> +set_props () {
> +subdir="$1"
> +file="$2"
>shift;shift;
>(cd "$subdir" &&
>while [ $# -gt 0 ] ; do
> @@ -43,10 +42,9 @@ set_props()
>git commit -m "testing propset" "$file")
> }
>
> -confirm_props()
> -{
> -local subdir="$1"
> -local file="$2"
> +confirm_props () {
> +subdir="$1"
> +file="$2"
>shift;shift;
>(set -e ; cd "svn_project/$subdir" &&
>while [ $# -gt 0 ] ; do
>
> Unless there's other improvements we missed, I'll push out your v3 with
> my changes squashed in for Junio to pull in a day or two. Thank you
> again for working on this!
>
Eric,
All looks good to me.
Thank you all very much for the feedback and help. It's made this a very
rewarding endeavor.
-Alfred. --
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: Support for git-svn propset
Alfred Perlstein wrote:
> Appearing here:
> http://marc.info/?l=git&m=125259772625008&w=2
Probably better to use a mid URL here, too
http://mid.gmane.org/1927112650.1281253084529659.javamail.r...@klofta.sjsoft.com
such a long URL, though...
> --- a/perl/Git/SVN/Editor.pm
> +++ b/perl/Git/SVN/Editor.pm
> @@ -288,6 +288,44 @@ sub apply_autoprops {
> }
> }
>
> +sub check_attr {
> + my ($attr,$path) = @_;
> + my $fh = command_output_pipe("check-attr", $attr, "--", $path);
> + return undef if (!$fh);
> +
> + my $val = <$fh>;
> + close $fh;
> + if ($val) { $val =~ s/^[^:]*:\s*[^:]*:\s*(.*)\s*$/$1/; }
> + return $val;
> +}
I just noticed command_output_pipe didn't use a corresponding
command_close_pipe to check for errors, but command_oneline is even
better. I'll squash the following:
--- a/perl/Git/SVN/Editor.pm
+++ b/perl/Git/SVN/Editor.pm
@@ -290,11 +290,7 @@ sub apply_autoprops {
sub check_attr {
my ($attr,$path) = @_;
- my $fh = command_output_pipe("check-attr", $attr, "--", $path);
- return undef if (!$fh);
-
- my $val = <$fh>;
- close $fh;
+ my $val = command_oneline("check-attr", $attr, "--", $path);
if ($val) { $val =~ s/^[^:]*:\s*[^:]*:\s*(.*)\s*$/$1/; }
return $val;
}
In your test, "local" isn't portable, unfortunately, but tests seem to
work fine without local so I've removed them:
--- a/t/t9148-git-svn-propset.sh
+++ b/t/t9148-git-svn-propset.sh
@@ -29,10 +29,9 @@ test_expect_success 'fetch revisions from svn' '
git svn fetch
'
-set_props()
-{
- local subdir="$1"
- local file="$2"
+set_props () {
+ subdir="$1"
+ file="$2"
shift;shift;
(cd "$subdir" &&
while [ $# -gt 0 ] ; do
@@ -43,10 +42,9 @@ set_props()
git commit -m "testing propset" "$file")
}
-confirm_props()
-{
- local subdir="$1"
- local file="$2"
+confirm_props () {
+ subdir="$1"
+ file="$2"
shift;shift;
(set -e ; cd "svn_project/$subdir" &&
while [ $# -gt 0 ] ; do
Unless there's other improvements we missed, I'll push out your v3 with
my changes squashed in for Junio to pull in a day or two. Thank you
again for working on this!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: Support for git-svn propset
On 12/6/14, 9:42 PM, Eric Wong wrote:
Alfred Perlstein wrote:
This change allows git-svn to support setting subversion properties.
Very useful for manually setting properties when committing to a
subversion repo that *requires* properties to be set without requiring
moving your changeset to separate subversion checkout in order to
set props.
This change is initially from David Fraser
No point to obfuscate email addresses in commit messages (especially
it's also in the Signed-off-by :).
Appearing here:
http://marc.info/?l=git&m=125259772625008&w=2
They are now forward ported to most recent git along with fixes to
deal with files in subdirectories.
Style and functional changes from Eric Wong have been taken
in thier entirety from:
s/thier/their/
http://marc.info/?l=git&m=141742735608544&w=2
Fwiw, I prefer equivalent mid.gmane.org links since the message-ID
remains useful if the web server ever goes away. e.g.:
http://mid.gmane.org/20141201094911.ga13...@dcvr.yhbt.net
Reviewed-by: Eric Wong
Signed-off-by: Alfred Perlstein
Signed-off-by: David Fraser
I'd like to squash in the following changes (in order of importance):
- use && to chain commands throughout tests
- use svn_cmd wrapper throughout tests
- show $! in die messages
- favor $(...) over `...` in tests
- make new_props an array simplify building the final list
- wrap long comments (help output still needs fixing)
- remove unnecessary FIXME comment
No need to resend if you're OK with these things. Thanks again.
Hmm, I refactored tests because it bothered me that I had done all that
cut and pasting, in doing so I found and fixed a bug with uninitialized
variables in the check_attr() function.
Let me send my final diff, I will try to properly incorporate your
changes as well in that diff.
-Alfred
diff --git a/git-svn.perl b/git-svn.perl
index 5cdbf39..ec5cee4 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1392,9 +1392,9 @@ sub cmd_propset {
my $file = basename($path);
my $dn = dirname($path);
my $cur_props = Git::SVN::Editor::check_attr( "svn-properties", $path );
- my $new_props = "";
+ my @new_props;
if ($cur_props eq "unset" || $cur_props eq "" || $cur_props eq "set") {
- $new_props = "$propname=$propval";
+ push @new_props, "$propname=$propval";
} else {
# TODO: handle combining properties better
my @props = split(/;/, $cur_props);
@@ -1403,24 +1403,24 @@ sub cmd_propset {
# Parse 'name=value' syntax and set the property.
if ($prop =~ /([^=]+)=(.*)/) {
my ($n,$v) = ($1,$2);
- if ($n eq $propname)
- {
+ if ($n eq $propname) {
$v = $propval;
$replaced_prop = 1;
}
- if ($new_props eq "") { $new_props="$n=$v"; }
- else { $new_props="$new_props;$n=$v"; }
+ push @new_props, "$n=$v";
}
}
if (!$replaced_prop) {
- $new_props = "$new_props;$propname=$propval";
+ push @new_props, "$propname=$propval";
}
}
my $attrfile = "$dn/.gitattributes";
open my $attrfh, '>>', $attrfile or die "Can't open $attrfile: $!\n";
# TODO: don't simply append here if $file already has svn-properties
- print $attrfh "$file svn-properties=$new_props\n" or die "write to
$attrfile";
- close $attrfh or die "close $attrfile";
+ my $new_props = join(';', @new_props);
+ print $attrfh "$file svn-properties=$new_props\n" or
+ die "write to $attrfile: $!\n";
+ close $attrfh or die "close $attrfile: $!\n";
}
# cmd_proplist (PATH)
diff --git a/perl/Git/SVN/Editor.pm b/perl/Git/SVN/Editor.pm
index dd15318..8bed2d9 100644
--- a/perl/Git/SVN/Editor.pm
+++ b/perl/Git/SVN/Editor.pm
@@ -288,8 +288,7 @@ sub apply_autoprops {
}
}
-sub check_attr
-{
+sub check_attr {
my ($attr,$path) = @_;
my $fh = command_output_pipe("check-attr", $attr, "--", $path);
return undef if (!$fh);
@@ -306,10 +305,12 @@ sub apply_manualprops {
if ($pending_properties eq "") { return; }
# Parse the list of properties to set.
my @props = split(/;/, $pending_properties);
- # TODO: get existing properties to compare to - this fails for add so
currently not done
+ # TODO: get existing properties to compare to
+ # - this fails for add so currently not done
# my $existing_props = ::get_svnprops($file);
my $existing_props = {};
- # TODO: caching svn properties or storing them in .gitattributes would
make that fast
Re: [PATCH] git-svn: propset support v2
On Sat, Dec 6, 2014 at 5:29 PM, Alfred Perlstein wrote: > I have incorporated Eric Wong's feedback into the git-svn propset support > patch. > > There is a nit that I want to point out. The code does not support adding > props > unless there are also content changes to the files as well. You can see this > in > the testcase. This is an important nugget of information which would be worthwhile to mention in the commit message of the patch rather than here as mere commentary. > That is still sufficient for many people's workflows (FreeBSD at > least). So I am wondering if this is OK. > > I would gladly take any pointers to making it work with unchanged > files either for a later diff or to wrap this up. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: Support for git-svn propset
On Sun, Dec 7, 2014 at 3:00 AM, Torsten Bögershausen wrote:
> On 2014-12-07 06.45, Torsten Bögershausen wrote:
> []
>>> +
>>> +test_expect_success 'add multiple props' '
>>> +git svn propset svn:keywords "FreeBSD=%H" foo &&
>>> +git svn propset fbsd:nokeywords yes foo &&
>>> +echo hello >> foo &&
>>> +git commit -m "testing propset" foo &&
>>> +git svn dcommit
>>> +svn_cmd co "$svnrepo" svn_project &&
>>> +(cd svn_project && test "`svn propget svn:keywords foo`" =
>>> "FreeBSD=%H") &&
>>> +(cd svn_project && test "`svn propget fbsd:nokeywords foo`" = "yes") &&
>>> +(cd svn_project && test "`svn proplist -q foo | wc -l`" -eq 2) &&
>>> +rm -rf svn_project
>>> +'
>>> +
>> Ah, another small thing:
>> the "wc -l" will not work under Mac OS X.
>> Please see test_line_count() in t/test-lib-functions.sh
>>
> My excuse:
> I think I am wrong here and I need to correct myself after having looked at
> other TC's.
> The "wc -l" should work under Mac OS X.
More specifically: On Mac OS X, "wc -l" outputs "{whitespace}number"
which won't match "2" with the string '=' operator, however, this case
works because the '-eq' operator coerces the output of "wc -l" to a
number, which can match the 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] git-svn: Support for git-svn propset
On 2014-12-07 06.45, Torsten Bögershausen wrote: [] >> + >> +test_expect_success 'add multiple props' ' >> +git svn propset svn:keywords "FreeBSD=%H" foo && >> +git svn propset fbsd:nokeywords yes foo && >> +echo hello >> foo && >> +git commit -m "testing propset" foo && >> +git svn dcommit >> +svn_cmd co "$svnrepo" svn_project && >> +(cd svn_project && test "`svn propget svn:keywords foo`" = >> "FreeBSD=%H") && >> +(cd svn_project && test "`svn propget fbsd:nokeywords foo`" = "yes") && >> +(cd svn_project && test "`svn proplist -q foo | wc -l`" -eq 2) && >> +rm -rf svn_project >> +' >> + > Ah, another small thing: > the "wc -l" will not work under Mac OS X. > Please see test_line_count() in t/test-lib-functions.sh > My excuse: I think I am wrong here and I need to correct myself after having looked at other TC's. The "wc -l" should work under Mac OS X. Another small nit: This "`svn propget svn:keywords foo`" = "FreeBSD=%H") can be written as "$(svn propget svn:keywords foo)" = "FreeBSD=%H") (if you want to use the "Git style" for command substitution) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: Support for git-svn propset
> diff --git a/t/t9148-git-svn-propset.sh b/t/t9148-git-svn-propset.sh
> new file mode 100755
> index 000..b36a8a2
> --- /dev/null
> +++ b/t/t9148-git-svn-propset.sh
> @@ -0,0 +1,71 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2014 Alfred Perlstein
> +#
> +
> +test_description='git svn propset tests'
> +
> +. ./lib-git-svn.sh
> +
> +foo_subdir2="subdir/subdir2/foo_subdir2"
> +
In case something goes wrong (for whatever reason):
do we need a && chain here ?
> +mkdir import
> +(cd import
> + mkdir subdir
> + mkdir subdir/subdir2
> + touch foo # for 'add props top level'
"touch foo" can be written shorter:
>foo
> + touch subdir/foo_subdir # for 'add props relative'
> + touch "$foo_subdir2"# for 'add props subdir'
> + svn_cmd import -m 'import for git svn' . "$svnrepo" >/dev/null
> +)
> +rm -rf import
> +
> +test_expect_success 'initialize git svn' 'git svn init "$svnrepo"'
> +test_expect_success 'fetch revisions from svn' 'git svn fetch'
This may look a little bit strange, 2 times test_expect_success in a row,
is the indentention OK ?
> +
> +# There is a bogus feature about svn propset which means that it will only
> +# be taken as a delta for svn dcommit iff the file is also modified.
> +# That is fine for now.
"there is a bogus feature ?"
Small typo: s/iff/if/
How about this:
#The current implementation has a restriction:
#svn propset will be taken as a delta for svn dcommit only
#if the file content is also modified
> +test_expect_success 'add props top level' '
> + git svn propset svn:keywords "FreeBSD=%H" foo &&
> + echo hello >> foo &&
> + git commit -m "testing propset" foo &&
> + git svn dcommit
> + svn_cmd co "$svnrepo" svn_project &&
> + (cd svn_project && test "`svn propget svn:keywords foo`" =
> "FreeBSD=%H") &&
> + rm -rf svn_project
> + '
Is there a reason why there is no "&&" after "git svn dcommit" ?
If yes, it could be better to make this really clear to the readers and write
(This idea is stolen from Peff)
{ git svn dcommit || true } &&
> +
> +test_expect_success 'add multiple props' '
> + git svn propset svn:keywords "FreeBSD=%H" foo &&
> + git svn propset fbsd:nokeywords yes foo &&
> + echo hello >> foo &&
> + git commit -m "testing propset" foo &&
> + git svn dcommit
> + svn_cmd co "$svnrepo" svn_project &&
> + (cd svn_project && test "`svn propget svn:keywords foo`" =
> "FreeBSD=%H") &&
> + (cd svn_project && test "`svn propget fbsd:nokeywords foo`" = "yes") &&
> + (cd svn_project && test "`svn proplist -q foo | wc -l`" -eq 2) &&
> + rm -rf svn_project
> + '
> +
Ah, another small thing:
the "wc -l" will not work under Mac OS X.
Please see test_line_count() in t/test-lib-functions.sh
And thanks for improving Git
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: Support for git-svn propset
Alfred Perlstein wrote:
> This change allows git-svn to support setting subversion properties.
>
> Very useful for manually setting properties when committing to a
> subversion repo that *requires* properties to be set without requiring
> moving your changeset to separate subversion checkout in order to
> set props.
>
> This change is initially from David Fraser
No point to obfuscate email addresses in commit messages (especially
it's also in the Signed-off-by :).
> Appearing here:
> http://marc.info/?l=git&m=125259772625008&w=2
>
> They are now forward ported to most recent git along with fixes to
> deal with files in subdirectories.
>
> Style and functional changes from Eric Wong have been taken
> in thier entirety from:
s/thier/their/
> http://marc.info/?l=git&m=141742735608544&w=2
Fwiw, I prefer equivalent mid.gmane.org links since the message-ID
remains useful if the web server ever goes away. e.g.:
http://mid.gmane.org/20141201094911.ga13...@dcvr.yhbt.net
> Reviewed-by: Eric Wong
> Signed-off-by: Alfred Perlstein
> Signed-off-by: David Fraser
I'd like to squash in the following changes (in order of importance):
- use && to chain commands throughout tests
- use svn_cmd wrapper throughout tests
- show $! in die messages
- favor $(...) over `...` in tests
- make new_props an array simplify building the final list
- wrap long comments (help output still needs fixing)
- remove unnecessary FIXME comment
No need to resend if you're OK with these things. Thanks again.
diff --git a/git-svn.perl b/git-svn.perl
index 5cdbf39..ec5cee4 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1392,9 +1392,9 @@ sub cmd_propset {
my $file = basename($path);
my $dn = dirname($path);
my $cur_props = Git::SVN::Editor::check_attr( "svn-properties", $path );
- my $new_props = "";
+ my @new_props;
if ($cur_props eq "unset" || $cur_props eq "" || $cur_props eq "set") {
- $new_props = "$propname=$propval";
+ push @new_props, "$propname=$propval";
} else {
# TODO: handle combining properties better
my @props = split(/;/, $cur_props);
@@ -1403,24 +1403,24 @@ sub cmd_propset {
# Parse 'name=value' syntax and set the property.
if ($prop =~ /([^=]+)=(.*)/) {
my ($n,$v) = ($1,$2);
- if ($n eq $propname)
- {
+ if ($n eq $propname) {
$v = $propval;
$replaced_prop = 1;
}
- if ($new_props eq "") { $new_props="$n=$v"; }
- else { $new_props="$new_props;$n=$v"; }
+ push @new_props, "$n=$v";
}
}
if (!$replaced_prop) {
- $new_props = "$new_props;$propname=$propval";
+ push @new_props, "$propname=$propval";
}
}
my $attrfile = "$dn/.gitattributes";
open my $attrfh, '>>', $attrfile or die "Can't open $attrfile: $!\n";
# TODO: don't simply append here if $file already has svn-properties
- print $attrfh "$file svn-properties=$new_props\n" or die "write to
$attrfile";
- close $attrfh or die "close $attrfile";
+ my $new_props = join(';', @new_props);
+ print $attrfh "$file svn-properties=$new_props\n" or
+ die "write to $attrfile: $!\n";
+ close $attrfh or die "close $attrfile: $!\n";
}
# cmd_proplist (PATH)
diff --git a/perl/Git/SVN/Editor.pm b/perl/Git/SVN/Editor.pm
index dd15318..8bed2d9 100644
--- a/perl/Git/SVN/Editor.pm
+++ b/perl/Git/SVN/Editor.pm
@@ -288,8 +288,7 @@ sub apply_autoprops {
}
}
-sub check_attr
-{
+sub check_attr {
my ($attr,$path) = @_;
my $fh = command_output_pipe("check-attr", $attr, "--", $path);
return undef if (!$fh);
@@ -306,10 +305,12 @@ sub apply_manualprops {
if ($pending_properties eq "") { return; }
# Parse the list of properties to set.
my @props = split(/;/, $pending_properties);
- # TODO: get existing properties to compare to - this fails for add so
currently not done
+ # TODO: get existing properties to compare to
+ # - this fails for add so currently not done
# my $existing_props = ::get_svnprops($file);
my $existing_props = {};
- # TODO: caching svn properties or storing them in .gitattributes would
make that faster
+ # TODO: caching svn properties or storing them in .gitattributes
+ # would make that faster
foreach my $prop (@props) {
# Parse 'name=value' syntax and set the property.
if ($prop =~ /([^=]+)=(.*)/) {
@@ -317,8 +318,6 @@ sub apply_manualprops {
Re: [PATCH] git-svn: Support for git-svn propset
Alfred Perlstein wrote:
> This change allows git-svn to support setting subversion properties.
>
> Very useful for manually setting properties when committing to a
> subversion repo that *requires* properties to be set without requiring
> moving your changeset to separate subversion checkout in order to
> set props.
Thanks Alfred, that's a good reason for supporting this feature
(something I wasn't convinced of with the original).
> This change is initially from David Fraser
> Appearing here: http://marc.info/?l=git&m=125259772625008&w=2
I've added David to the Cc: (but I never heard back from him regarding
comments from his original patch).
Alfred: I had some comments on David's original patch here that
were never addressed:
http://mid.gmane.org/20090923085812.ga20...@dcvr.yhbt.net
I suspect my concerns about .gitattributes in the source tree from
2009 are less relevant now in 2014 as git-svn seems more socially
acceptable in SVN-using projects.
Some of my other concerns about mimicking existing Perl style still
apply, and we have a Perl section in Documenting/CodingGuidelines
nowadays.
> They are now forward ported to most recent git along with fixes to
> deal with files in subdirectories.
>
> Developer's Certificate of Origin 1.1
no need for the whole DCO in the commit message.
just the S-o-b:
> Signed-off-by: Alfred Perlstein
Since this was originally written by David, his sign-off from the
original email should be here (Cc: for bigger changes)
> ---
> git-svn.perl | 50
> +-
> perl/Git/SVN/Editor.pm | 47 +++
We need a test case written for this new feature.
Most folks (including myself) who were ever involved with git-svn
rarely use it anymore, and this will likely be rarely-used.
> 2 files changed, 96 insertions(+), 1 deletion(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index b6e2186..91423a8 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -115,7 +115,7 @@ my ($_stdin, $_help, $_edit,
> $_before, $_after,
> $_merge, $_strategy, $_preserve_merges, $_dry_run, $_parents, $_local,
> $_prefix, $_no_checkout, $_url, $_verbose,
> - $_commit_url, $_tag, $_merge_info, $_interactive);
> + $_commit_url, $_tag, $_merge_info, $_interactive, $_set_svn_props);
>
> # This is a refactoring artifact so Git::SVN can get at this git-svn switch.
> sub opt_prefix { return $_prefix || '' }
> @@ -193,6 +193,7 @@ my %cmd = (
> 'dry-run|n' => \$_dry_run,
> 'fetch-all|all' => \$_fetch_all,
> 'commit-url=s' => \$_commit_url,
> + 'set-svn-props=s' => \$_set_svn_props,
> 'revision|r=i' => \$_revision,
> 'no-rebase' => \$_no_rebase,
> 'mergeinfo=s' => \$_merge_info,
> @@ -228,6 +229,9 @@ my %cmd = (
> 'propget' => [ \&cmd_propget,
> 'Print the value of a property on a file or directory',
> { 'revision|r=i' => \$_revision } ],
> +'propset' => [ \&cmd_propset,
> +'Set the value of a property on a file or directory -
> will be set on commit',
> +{} ],
> 'proplist' => [ \&cmd_proplist,
> 'List all properties of a file or directory',
> { 'revision|r=i' => \$_revision } ],
> @@ -1376,6 +1380,50 @@ sub cmd_propget {
> print $props->{$prop} . "\n";
> }
>
> +# cmd_propset (PROPNAME, PROPVAL, PATH)
> +#
> +# Adjust the SVN property PROPNAME to PROPVAL for PATH.
> +sub cmd_propset {
> + my ($propname, $propval, $path) = @_;
> + $path = '.' if not defined $path;
> + $path = $cmd_dir_prefix . $path;
> + usage(1) if not defined $propname;
> + usage(1) if not defined $propval;
> + my $file = basename($path);
> + my $dn = dirname($path);
> + # diff has check_attr locally, so just call direct
> + #my $current_properties = check_attr( "svn-properties", $path );
I prefer leaving out dead code entirely instead of leaving it commented out.
> + my $current_properties = Git::SVN::Editor::check_attr(
> "svn-properties", $path );
> + my $new_properties = "";
Since some lines are too long, local variables can be shortened
to cur_props, new_props without being any less descriptive.
> + if ($current_properties eq "unset" || $current_properties eq "" ||
> $current_properties eq "set") {
> + $new_properties = "$propname=$propval";
> + } else {
> + # TODO: handle combining properties better
> + my @props = split(/;/, $current_properties);
> + my $replaced_prop = 0;
> + foreach my $prop (@props) {
> + # Parse 'name=value' syntax and set the property.
> + if ($prop =~ /([^=]+)=(.*)/) {
> +
Re: [PATCH] git-svn: use SVN::Ra::get_dir2 when possible
Hin-Tak Leung wrote: > Hmm, I see you are filing the problem against subversion. FWIW, > I am currently using subversion-perl-1.8.10-1.fc20.x86_64 package on fedora > 20. > I'll possibly think about filing one under redhat's bugzilla and > let them take it upward too. This is another problem with the vbox repository: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=767530#10 And forwarded upstream: http://mid.gmane.org/20141101182722.gb20...@freya.jamessan.com -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: use SVN::Ra::get_dir2 when possible
Hmm, I see you are filing the problem against subversion. FWIW,
I am currently using subversion-perl-1.8.10-1.fc20.x86_64 package on fedora 20.
I'll possibly think about filing one under redhat's bugzilla and
let them take it upward too.
On Fri, 31/10/14, Eric Wong wrote:
This avoids the following failure
with normal "get_dir" on newer
versions of SVN (tested with SVN 1.8.8-1ubuntu3.1):
Incorrect parameters given: Could not convert '%ld'
into a number
get_dir2 also has the potential to be more efficient by
requesting
less data.
ref: <1414636504.45506.yahoomailba...@web172304.mail.ir2.yahoo.com>
ref: <1414722617.89476.yahoomailba...@web172305.mail.ir2.yahoo.com>
Signed-off-by: Eric Wong
Cc: Hin-Tak Leung
---
This should fix the vbox clone problem. SVN
Perl binding
breakage (again :<). I shall revert the
int() changes.
> I added those two lines to my git and there is
no improvement. It
> still won't svn fetch the next revision. I think
it may be
> important/interesting to find out when or how it
becomes non-int, so
> I have tar'gz'ed my wont-fetch virtual box .git
and in the middle of
> uploading here:
>
> http://sourceforge.net/projects/outmodedbonsai/files/R/
> I am also uploading my old R clone also - maybe
you'd like to see
> why its .git/svn/.caches is so big compared to a
recent one,
Jakob's changes causes different access patterns, so
it's expected the
sizes vary. I also changed the cherry pick
cache and removed the
_rev_list caching entirely, so it should be much
smaller now.
> as well as how and why there were an extra merge
and two missing
> merges compared to a recent clone?
The different merges are fine, I think, as stated in
http://mid.gmane.org/20141030230831.ga14...@dcvr.yhbt.net
perl/Git/SVN/Ra.pm | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index 82d6108..1e52709 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -177,7 +177,17 @@ sub get_dir {
}
}
my $pool = SVN::Pool->new;
- my ($d, undef, $props) =
$self->SUPER::get_dir($dir, $r, $pool);
+ my ($d, undef, $props);
+
+ if (defined &SVN::Ra::get_dir2) { #
appeared in SVN 1.4
+ # n.b. in addition to
being potentially more efficient,
+ # this works around
what appears to be a bug in some
+ # SVN 1.8 versions
+ my $kind = 1; #
SVN_DIRENT_KIND
+ ($d, undef, $props) =
$self->get_dir2($dir, $r, $kind, $pool);
+ } else {
+ ($d, undef, $props) =
$self->get_dir($dir, $r, $pool);
+ }
my %dirents = map { $_ => { kind
=> $d->{$_}->kind } } keys %$d;
$pool->clear;
if ($r != $cache->{r}) {
--
EW
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: clear global SVN pool between get_log invocations
Eric Wong wrote: > + SVN::Core->gpool->clear; Unfortunately, SVN::Core->gpool is not available in older SVN, but I'm cooking a better patch which saves even more memory. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: merge: fix rooturl/branchurl match check
Tommaso Colombo wrote:
> When populating svn:mergeinfo, git-svn merge checks if the merge parent
> of the merged branch is under the same root as the git-svn repository.
> This was implemented comparing $gs->repos_root with the return value of
> of cmt_metadata for the merge parent. However, the first may contain a
> username, whereas the second does not. In this case the comparison
> fails.
>
> Remove the username from $gs->repos_root before performing the
> comparison.
Thanks. Commit makes sense, but one of the test cases fails for me,
can you check it out?
$ make && make t9161-git-svn-mergeinfo-push.sh -C t GIT_TEST_OPTS='-i -v'
ok 11 - reintegration merge
expecting success:
mergeinfo=$(svn_cmd propget svn:mergeinfo "$svnrepo"/branches/svnb4)
test "$mergeinfo" = "/branches/svnb1:2-4,7-9,13-18
/branches/svnb2:3,8,16-17
/branches/svnb3:4,9
/branches/svnb5:6,11"
not ok 12 - check reintegration mergeinfo
#
# mergeinfo=$(svn_cmd propget svn:mergeinfo
"$svnrepo"/branches/svnb4)
# test "$mergeinfo" = "/branches/svnb1:2-4,7-9,13-18
# /branches/svnb2:3,8,16-17
# /branches/svnb3:4,9
# /branches/svnb5:6,11"
#
make: *** [t9161-git-svn-mergeinfo-push.sh] Error 1
make: Leaving directory `/home/ew/git-core/t'
You'll also need to sign-off (see Documentation/SubmittingPatches)
It is required (project policy, not mine)
> @@ -729,7 +730,7 @@ sub populate_merge_info {
> }
> my $branchpath = $1;
>
> - my $ra = Git::SVN::Ra->new($branchurl);
> + my $ra =
> Git::SVN::Ra->new(add_path_to_url($gs->repos_root, $branchpath));
Also, please keep long lines wrapped to <= 80 columns.
Thanks again.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: delay term initialization
Eric Wong writes:
> On my Debian 7 system, this gives annoying warnings when the output
> of "git svn" commands are redirected:
>
> Unable to get Terminal Size. The TIOCGWINSZ ioctl didn't work.
> The COLUMNS and LINES environment variables didn't work. The
> resize program didn't work.
>
> Signed-off-by: Eric Wong
> ---
> Also, manually tested to ensure dcommit --interactive works.
Makes sense.
>
> git-svn.perl | 18 +++---
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index 40565cd..ce0d7e1 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -306,13 +306,16 @@ sub readline {
> }
> package main;
>
> -my $term = eval {
> - $ENV{"GIT_SVN_NOTTY"}
> - ? new Term::ReadLine 'git-svn', \*STDIN, \*STDOUT
> - : new Term::ReadLine 'git-svn';
> -};
> -if ($@) {
> - $term = new FakeTerm "$@: going non-interactive";
> +my $term;
> +sub term_init {
> + $term = eval {
> + $ENV{"GIT_SVN_NOTTY"}
> + ? new Term::ReadLine 'git-svn', \*STDIN, \*STDOUT
> + : new Term::ReadLine 'git-svn';
> + };
> + if ($@) {
> + $term = new FakeTerm "$@: going non-interactive";
> + }
> }
>
> my $cmd;
> @@ -424,6 +427,7 @@ sub ask {
> my $default = $arg{default};
> my $resp;
> my $i = 0;
> + term_init() unless $term;
>
> if ( !( defined($term->IN)
> && defined( fileno($term->IN) )
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: delay term initialization
Eric Wong wrote: > On my Debian 7 system, this gives annoying warnings when the output s/gives/fixes/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: doublecheck if really file or dir
Andrej Manduch wrote:
> On 08/03/2014 02:22 PM, Andrej Manduch wrote:
> > Nice touch, It works like charm. However unfortunatelly now I think you
> > introduced new bug :)
Good catch!
> > On 08/03/2014 04:45 AM, Eric Wong wrote:
> >> sub cmd_info {
> >> - my $path = canonicalize_path(defined($_[0]) ? $_[0] : ".");
> >> - my $fullpath = canonicalize_path($cmd_dir_prefix . $path);
> >> + my $path_arg = defined($_[0]) ? $_[0] : '.';
> >> + my $path = $path_arg;
> >> + if ($path =~ m!\A/!) {
> >> + my $toplevel = eval {
> >> + my @cmd = qw/rev-parse --show-toplevel/;
> >> + command_oneline(\@cmd, STDERR => 0);
> >> + };
> >> + $path =~ s!\A\Q$toplevel\E/?!!;
> > I have problem with this line ^^^
> >
> > Suppose your $toplevel is "/sometning" and you type in command line
> > something like that: "git svn info /somethingsrc" and as you see this
> > should end up with error. However "$path =~ s!\A\Q$toplevel\E/?!!;"
> > will just cut "/sometning" from "/somethingsrc" and and up with same
> > answer as for "svn git info src" which is not equivalent query.
> >
> > Second scenario is something which worries me more: If your query look
> > like this: "git svn info /something//src" it will just end up with error
> > because it will set $path to "/src" witch is outside of repository.
> >
> > Second scenario can be fixed with this:
> >
>
> Actualy this will be even better:
Thanks Andrej. I'll queue that on top of mine.
Can you turn that into a proper commit message with Subject?
Thanks.
(The English-generating part of my brain is too tired)
> Signed-off-by: Andrej Manduch
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1483,6 +1483,7 @@ sub cmd_info {
> my @cmd = qw/rev-parse --show-toplevel/;
> command_oneline(\@cmd, STDERR => 0);
> };
> + $path = canonicalize_path($path);
> $path =~ s!\A\Q$toplevel\E/?!!;
> $path = canonicalize_path($path);
> } else {
> Because this will have not problem with really weird query like: "git
> svn info /media/../media/something//src"
I've also started working on the following test cases,
will squash:
diff --git a/t/t9119-git-svn-info.sh b/t/t9119-git-svn-info.sh
index 4f6e669..f16f323 100755
--- a/t/t9119-git-svn-info.sh
+++ b/t/t9119-git-svn-info.sh
@@ -84,6 +84,26 @@ test_expect_success 'info $(pwd)' '
"$(sed -ne \"/^Path:/ s!/gitwc!!\" expected.info-pwd &&
+ (cd gitwc; git svn info "$(pwd)/../gitwc") >actual.info-pwd &&
+ grep -v ^Path: expected.info-np &&
+ grep -v ^Path: actual.info-np &&
+ test_cmp_info expected.info-np actual.info-np &&
+ test "$(sed -ne \"/^Path:/ s!/svnwc!!\" expected.info-pwd &&
+ (cd gitwc; git svn info "$(pwd)/../gitwc//file") >actual.info-pwd &&
+ grep -v ^Path: expected.info-np &&
+ grep -v ^Path: actual.info-np &&
+ test_cmp_info expected.info-np actual.info-np &&
+ test "$(sed -ne \"/^Path:/ s!/svnwc!!\" http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: doublecheck if really file or dir
On 08/03/2014 02:22 PM, Andrej Manduch wrote:
> Hi Eric,
>
> Nice touch, It works like charm. However unfortunatelly now I think you
> introduced new bug :)
>
> On 08/03/2014 04:45 AM, Eric Wong wrote:
>> Hi Andrej, I could not help thinking your patch was obscuring
>> another bug. I think I have an alternative to your patch which
>> fixes both our bugs. Can you give this a shot? Thanks.
>>
>> --- 8<
>> Subject: [PATCH] git svn: info: correctly handle absolute path args
>>
>> Calling "git svn info $(pwd)" would hit:
>> "Reading from filehandle failed at ..."
>> errors due to improper prefixing and canonicalization.
>>
>> Strip the toplevel path from absolute filesystem paths to ensure
>> downstream canonicalization routines are only exposed to paths
>> tracked in git (or SVN).
>>
>> Noticed-by: Andrej Manduch
>> Signed-off-by: Eric Wong
>> ---
>> git-svn.perl| 21 +++--
>> t/t9119-git-svn-info.sh | 10 ++
>> 2 files changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/git-svn.perl b/git-svn.perl
>> index 1f41ee1..1f9582b 100755
>> --- a/git-svn.perl
>> +++ b/git-svn.perl
>> @@ -1477,10 +1477,19 @@ sub cmd_commit_diff {
>> }
>> }
>>
>> -
>> sub cmd_info {
>> -my $path = canonicalize_path(defined($_[0]) ? $_[0] : ".");
>> -my $fullpath = canonicalize_path($cmd_dir_prefix . $path);
>> +my $path_arg = defined($_[0]) ? $_[0] : '.';
>> +my $path = $path_arg;
>> +if ($path =~ m!\A/!) {
>> +my $toplevel = eval {
>> +my @cmd = qw/rev-parse --show-toplevel/;
>> +command_oneline(\@cmd, STDERR => 0);
>> +};
>> +$path =~ s!\A\Q$toplevel\E/?!!;
> I have problem with this line ^^^
>
> Suppose your $toplevel is "/sometning" and you type in command line
> something like that: "git svn info /somethingsrc" and as you see this
> should end up with error. However "$path =~ s!\A\Q$toplevel\E/?!!;"
> will just cut "/sometning" from "/somethingsrc" and and up with same
> answer as for "svn git info src" which is not equivalent query.
>
> Second scenario is something which worries me more: If your query look
> like this: "git svn info /something//src" it will just end up with error
> because it will set $path to "/src" witch is outside of repository.
>
> Second scenario can be fixed with this:
>
> diff --git a/git-svn.perl b/git-svn.perl
> index a69f0fc..00f9d01 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1483,7 +1483,7 @@ sub cmd_info {
> my @cmd = qw/rev-parse --show-toplevel/;
> command_oneline(\@cmd, STDERR => 0);
> };
> - $path =~ s!\A\Q$toplevel\E/?!!;
> + $path =~ s!\A\Q$toplevel\E/*!!;
> $path = canonicalize_path($path);
> } else {
> $path = canonicalize_path($cmd_dir_prefix . $path);
>
Actualy this will be even better:
Signed-off-by: Andrej Manduch
---
git-svn.perl | 1 +
1 file changed, 1 insertion(+)
diff --git a/git-svn.perl b/git-svn.perl
index a69f0fc..58df866 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1483,6 +1483,7 @@ sub cmd_info {
my @cmd = qw/rev-parse --show-toplevel/;
command_oneline(\@cmd, STDERR => 0);
};
+ $path = canonicalize_path($path);
$path =~ s!\A\Q$toplevel\E/?!!;
$path = canonicalize_path($path);
} else {
--
2.0.0.GIT
Because this will have not problem with really weird query like: "git
svn info /media/../media/something//src"
>
> However I'm not sure if this will work on windows (where slashes are in
> different orientation).
>
>
> On 08/03/2014 04:45 AM, Eric Wong wrote:
>> +$path = canonicalize_path($path);
>> +} else {
>> +$path = canonicalize_path($cmd_dir_prefix . $path);
>> +}
>> if (exists $_[1]) {
>> die "Too many arguments specified\n";
>> }
>> @@ -1501,14 +1510,14 @@ sub cmd_info {
>> # canonicalize_path() will return "" to make libsvn 1.5.x happy,
>> $path = "." if $path eq "";
>>
>> -my $full_url = canonicalize_url( add_path_to_url( $url, $fullpath ) );
>> +my $full_url = canonicalize_url( add_path_to_url( $url, $path ) );
>>
>> if ($_url) {
>> print "$full_url\n";
>> return;
>> }
>>
>> -my $result = "Path: $path\n";
>> +my $result = "Path: $path_arg\n";
>> $result .= "Name: " . basename($path) . "\n" if $file_type ne "dir";
>> $result .= "URL: $full_url\n";
>>
>> @@ -1539,7 +1548,7 @@ sub cmd_info {
>> }
>>
>> my ($lc_author, $lc_rev, $lc_date_utc);
>> -my @args = Git::SVN::Log::git_svn_log_cmd($rev, $rev, "--", $fullpath);
>> +my @args = Git::SVN::Log::git_svn_log_cmd($rev, $rev, "--", $path);
>> my $log = command_output_pipe(@args);
>> my $esc_color =
Re: [PATCH] git-svn: doublecheck if really file or dir
Hi Eric,
Nice touch, It works like charm. However unfortunatelly now I think you
introduced new bug :)
On 08/03/2014 04:45 AM, Eric Wong wrote:
> Hi Andrej, I could not help thinking your patch was obscuring
> another bug. I think I have an alternative to your patch which
> fixes both our bugs. Can you give this a shot? Thanks.
>
> --- 8<
> Subject: [PATCH] git svn: info: correctly handle absolute path args
>
> Calling "git svn info $(pwd)" would hit:
> "Reading from filehandle failed at ..."
> errors due to improper prefixing and canonicalization.
>
> Strip the toplevel path from absolute filesystem paths to ensure
> downstream canonicalization routines are only exposed to paths
> tracked in git (or SVN).
>
> Noticed-by: Andrej Manduch
> Signed-off-by: Eric Wong
> ---
> git-svn.perl| 21 +++--
> t/t9119-git-svn-info.sh | 10 ++
> 2 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index 1f41ee1..1f9582b 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1477,10 +1477,19 @@ sub cmd_commit_diff {
> }
> }
>
> -
> sub cmd_info {
> - my $path = canonicalize_path(defined($_[0]) ? $_[0] : ".");
> - my $fullpath = canonicalize_path($cmd_dir_prefix . $path);
> + my $path_arg = defined($_[0]) ? $_[0] : '.';
> + my $path = $path_arg;
> + if ($path =~ m!\A/!) {
> + my $toplevel = eval {
> + my @cmd = qw/rev-parse --show-toplevel/;
> + command_oneline(\@cmd, STDERR => 0);
> + };
> + $path =~ s!\A\Q$toplevel\E/?!!;
I have problem with this line ^^^
Suppose your $toplevel is "/sometning" and you type in command line
something like that: "git svn info /somethingsrc" and as you see this
should end up with error. However "$path =~ s!\A\Q$toplevel\E/?!!;"
will just cut "/sometning" from "/somethingsrc" and and up with same
answer as for "svn git info src" which is not equivalent query.
Second scenario is something which worries me more: If your query look
like this: "git svn info /something//src" it will just end up with error
because it will set $path to "/src" witch is outside of repository.
Second scenario can be fixed with this:
diff --git a/git-svn.perl b/git-svn.perl
index a69f0fc..00f9d01 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1483,7 +1483,7 @@ sub cmd_info {
my @cmd = qw/rev-parse --show-toplevel/;
command_oneline(\@cmd, STDERR => 0);
};
- $path =~ s!\A\Q$toplevel\E/?!!;
+ $path =~ s!\A\Q$toplevel\E/*!!;
$path = canonicalize_path($path);
} else {
$path = canonicalize_path($cmd_dir_prefix . $path);
However I'm not sure if this will work on windows (where slashes are in
different orientation).
On 08/03/2014 04:45 AM, Eric Wong wrote:
> + $path = canonicalize_path($path);
> + } else {
> + $path = canonicalize_path($cmd_dir_prefix . $path);
> + }
> if (exists $_[1]) {
> die "Too many arguments specified\n";
> }
> @@ -1501,14 +1510,14 @@ sub cmd_info {
> # canonicalize_path() will return "" to make libsvn 1.5.x happy,
> $path = "." if $path eq "";
>
> - my $full_url = canonicalize_url( add_path_to_url( $url, $fullpath ) );
> + my $full_url = canonicalize_url( add_path_to_url( $url, $path ) );
>
> if ($_url) {
> print "$full_url\n";
> return;
> }
>
> - my $result = "Path: $path\n";
> + my $result = "Path: $path_arg\n";
> $result .= "Name: " . basename($path) . "\n" if $file_type ne "dir";
> $result .= "URL: $full_url\n";
>
> @@ -1539,7 +1548,7 @@ sub cmd_info {
> }
>
> my ($lc_author, $lc_rev, $lc_date_utc);
> - my @args = Git::SVN::Log::git_svn_log_cmd($rev, $rev, "--", $fullpath);
> + my @args = Git::SVN::Log::git_svn_log_cmd($rev, $rev, "--", $path);
> my $log = command_output_pipe(@args);
> my $esc_color = qr/(?:\033\[(?:(?:\d+;)*\d*)?m)*/;
> while (<$log>) {
> diff --git a/t/t9119-git-svn-info.sh b/t/t9119-git-svn-info.sh
> index ff19695..4f6e669 100755
> --- a/t/t9119-git-svn-info.sh
> +++ b/t/t9119-git-svn-info.sh
> @@ -74,6 +74,16 @@ test_expect_success 'info .' "
> test_cmp_info expected.info-dot actual.info-dot
> "
>
> +test_expect_success 'info $(pwd)' '
> + (cd svnwc; svn info "$(pwd)") >expected.info-pwd &&
> + (cd gitwc; git svn info "$(pwd)") >actual.info-pwd &&
> + grep -v ^Path: expected.info-np &&
> + grep -v ^Path: actual.info-np &&
> + test_cmp_info expected.info-np actual.info-np &&
> + test "$(sed -ne \"/^Path:/ s!/svnwc!!\" + "$(sed -ne \"/^Path:/ s!/gitwc!!\" + '
> +
> test_expect_success 'info --url .' '
> test "$(cd gitwc; git svn info --url .)" = "$quot
Re: [PATCH] git-svn: doublecheck if really file or dir
Andrej Manduch wrote:
> On 07/24/2014 12:04 AM, Eric Wong wrote:
> > Andrej Manduch wrote:
> >> * this fixes 'git svn info `pwd`' buggy behaviour
> >
> > While your patch avoids an error, but the output isn't right, either.
> > I tried it running in /home/ew/ruby, the URL field is bogus:
> Thx, I missed this. However this bug was not introduced with my patch,
> it was there before. If you try use `git svn info full_path` and
> directory is not a root dir this bug will occour even wihout my patch.
Hi Andrej, I could not help thinking your patch was obscuring
another bug. I think I have an alternative to your patch which
fixes both our bugs. Can you give this a shot? Thanks.
--- 8<
Subject: [PATCH] git svn: info: correctly handle absolute path args
Calling "git svn info $(pwd)" would hit:
"Reading from filehandle failed at ..."
errors due to improper prefixing and canonicalization.
Strip the toplevel path from absolute filesystem paths to ensure
downstream canonicalization routines are only exposed to paths
tracked in git (or SVN).
Noticed-by: Andrej Manduch
Signed-off-by: Eric Wong
---
git-svn.perl| 21 +++--
t/t9119-git-svn-info.sh | 10 ++
2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 1f41ee1..1f9582b 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1477,10 +1477,19 @@ sub cmd_commit_diff {
}
}
-
sub cmd_info {
- my $path = canonicalize_path(defined($_[0]) ? $_[0] : ".");
- my $fullpath = canonicalize_path($cmd_dir_prefix . $path);
+ my $path_arg = defined($_[0]) ? $_[0] : '.';
+ my $path = $path_arg;
+ if ($path =~ m!\A/!) {
+ my $toplevel = eval {
+ my @cmd = qw/rev-parse --show-toplevel/;
+ command_oneline(\@cmd, STDERR => 0);
+ };
+ $path =~ s!\A\Q$toplevel\E/?!!;
+ $path = canonicalize_path($path);
+ } else {
+ $path = canonicalize_path($cmd_dir_prefix . $path);
+ }
if (exists $_[1]) {
die "Too many arguments specified\n";
}
@@ -1501,14 +1510,14 @@ sub cmd_info {
# canonicalize_path() will return "" to make libsvn 1.5.x happy,
$path = "." if $path eq "";
- my $full_url = canonicalize_url( add_path_to_url( $url, $fullpath ) );
+ my $full_url = canonicalize_url( add_path_to_url( $url, $path ) );
if ($_url) {
print "$full_url\n";
return;
}
- my $result = "Path: $path\n";
+ my $result = "Path: $path_arg\n";
$result .= "Name: " . basename($path) . "\n" if $file_type ne "dir";
$result .= "URL: $full_url\n";
@@ -1539,7 +1548,7 @@ sub cmd_info {
}
my ($lc_author, $lc_rev, $lc_date_utc);
- my @args = Git::SVN::Log::git_svn_log_cmd($rev, $rev, "--", $fullpath);
+ my @args = Git::SVN::Log::git_svn_log_cmd($rev, $rev, "--", $path);
my $log = command_output_pipe(@args);
my $esc_color = qr/(?:\033\[(?:(?:\d+;)*\d*)?m)*/;
while (<$log>) {
diff --git a/t/t9119-git-svn-info.sh b/t/t9119-git-svn-info.sh
index ff19695..4f6e669 100755
--- a/t/t9119-git-svn-info.sh
+++ b/t/t9119-git-svn-info.sh
@@ -74,6 +74,16 @@ test_expect_success 'info .' "
test_cmp_info expected.info-dot actual.info-dot
"
+test_expect_success 'info $(pwd)' '
+ (cd svnwc; svn info "$(pwd)") >expected.info-pwd &&
+ (cd gitwc; git svn info "$(pwd)") >actual.info-pwd &&
+ grep -v ^Path: expected.info-np &&
+ grep -v ^Path: actual.info-np &&
+ test_cmp_info expected.info-np actual.info-np &&
+ test "$(sed -ne \"/^Path:/ s!/svnwc!!\" http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: Avoid systematic prompt for client certificate when using "git svn branch"
Le 02/08/2014 12:07, Eric Wong a écrit : Thanks. In the future, it's expected to Cc: anybody who showed interest in previous versions of your patch. Monard Vong wrote: When a client certificate is required to connect to a Subversion repository, the certificate location and password may be stored in Subversion config directory. Commands like "git svn init/fetch/dcommit" do not prompt for client certificate/password if the location is set in SVN config file, but "git svn branch" does not use the config directory, resulting in prompting for certificate location/password all the time. The commit message is probably too long, and needs to be line-wrapped. Perhaps something like: ---8<--- Subject: [PATCH] git-svn: branch: avoid systematic prompt for cert/pass Commands such as "git svn init/fetch/dcommit" do not prompt for client certificate/password if they are stored in SVN config file. Make "git svn branch" consistent with the other commands, as SVN::Client is capable of building its own authentication baton from information in the SVN config directory. Signed-off-by: Monard Vong ---8<--- I can push the above with my Signed-off-by if you are happy with it. Thanks again! Please do, all pleasure is mine, and thanks a lot for your feedback. Monard -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: Avoid systematic prompt for client certificate when using "git svn branch"
Thanks. In the future, it's expected to Cc: anybody who showed interest in previous versions of your patch. Monard Vong wrote: > When a client certificate is required to connect to a Subversion repository, > the certificate location and password may be stored in Subversion config > directory. > Commands like "git svn init/fetch/dcommit" do not prompt for client > certificate/password > if the location is set in SVN config file, but "git svn branch" does not use > the config > directory, resulting in prompting for certificate location/password all the > time. The commit message is probably too long, and needs to be line-wrapped. Perhaps something like: ---8<--- Subject: [PATCH] git-svn: branch: avoid systematic prompt for cert/pass Commands such as "git svn init/fetch/dcommit" do not prompt for client certificate/password if they are stored in SVN config file. Make "git svn branch" consistent with the other commands, as SVN::Client is capable of building its own authentication baton from information in the SVN config directory. Signed-off-by: Monard Vong ---8<--- I can push the above with my Signed-off-by if you are happy with it. Thanks again! -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: doublecheck if really file or dir
Hi,
On 07/24/2014 12:04 AM, Eric Wong wrote:
> Andrej Manduch wrote:
>> * this fixes 'git svn info `pwd`' buggy behaviour
>
> Good catch, the commit could use a better description, something like:
> --- 8<
> Subject: [PATCH] git-svn: "info" checks for dirs more carefully
>
> This avoids a "Reading from filehandle failed at ..." error when
> running "git svn info `pwd`".
>
> Signed-off-by: Andrej Manduch
> --- 8<
>
> While your patch avoids an error, but the output isn't right, either.
> I tried it running in /home/ew/ruby, the URL field is bogus:
>
> ~/ruby$ git svn info `pwd`
> Path: /home/ew/ruby
> URL: svn+ssh://svn.ruby-lang.org/ruby/trunk/home/ew/ruby
> Repository Root: svn+ssh://svn.ruby-lang.org/ruby
> Repository UUID: b2dd03c8-39d4-4d8f-98ff-823fe69b080e
> Revision: 46901
> Node Kind: directory
> Schedule: normal
> Last Changed Author: hsbt
> Last Changed Rev: 46901
> Last Changed Date: 2014-07-22 19:06:12 + (Tue, 22 Jul 2014)
>
> The URL should be:
>
> URL: svn+ssh://svn.ruby-lang.org/ruby/trunk
>
> It's better than an error, but it'd be nice if someone who uses
> this command can fix it (*hint* :).
Thx, I missed this. However this bug was not introduced with my patch,
it was there before. If you try use `git svn info full_path` and
directory is not a root dir this bug will occour even wihout my patch.
However I'll try to find some time to fix this too.
On 07/24/2014 12:04 AM, Eric Wong wrote:
>
>> --- a/git-svn.perl
>> +++ b/git-svn.perl
>> @@ -2029,7 +2029,7 @@ sub find_file_type_and_diff_status {
>> my $mode = (split(' ', $ls_tree))[0] || "";
>>
>> return ("link", $diff_status) if $mode eq "12";
>> -return ("dir", $diff_status) if $mode eq "04";
>> +return ("dir", $diff_status) if $mode eq "04" or -d $path;
>
> "or" has a lower precedence than "||", so I would do the following:
>
> return ("dir", $diff_status) if $mode eq "04" || -d $path;
>
> The general rule I've learned is to use "||" for conditionals and
> "or" for control flow (e.g. do_something() or die("...") ).
>
> I can take your patch with the above changes (no need to resend),
> but I'd be happier to see the URL field corrected if you want
> to reroll.
I'll try to fix whis url bug, but it will be different patch 'cause I
think, this is different kind of a problem.
On 07/24/2014 12:04 AM, Eric Wong wrote:
>
> Thanks.
>
I thanks to you for great review.
--
Best Regards,
b.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: Initialize SVN::Client with svn config instead of, auth for "git svn branch".
Thanks for your reply, I updated commit message and subject, hoping it would be clearer. However I messed up "message-id" so it appear as a new message and not in the current thread. Sorry, still learning. Le 24/07/2014 00:33, Eric Wong a écrit : Monard Vong wrote: If a client certificate is required to connect to svn, "git svn branch" always prompt the user for the certificate location and password, even though those parameters are stored in svn file "server" located in svn config dir (generally ~/.subversion). On the opposite, "git svn info/init/dcommit" read the config dir and do not prompt if the parameters are set. This commit initializes for "git svn branch", the SVN::Client with the 'config' option instead of 'auth'. As decribed in the SVN documentation, http://search.cpan.org/~mschwern/Alien-SVN-v1.7.17.1/src/subversion/subversion/bindings/swig/perl/native/Client.pm#METHODS the SVN::Client will then read cached authentication options. Signed-off-by: Monard Vong Thanks, I do not have a good way to test this, but the idea seems correct. Your patch is whitespace mangled, and the commit message and subject needs to be improved (see Documentation/SubmittingPatches on how to describe your changes and how to send them without whitespace mangling.) Thanks again. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: Initialize SVN::Client with svn config instead of, auth for "git svn branch".
Monard Vong wrote: > If a client certificate is required to connect to svn, "git svn branch" > always prompt the user for the certificate location and password, > even though those parameters are stored in svn file "server" > located in svn config dir (generally ~/.subversion). > On the opposite, "git svn info/init/dcommit" read the config dir > and do not prompt if the parameters are set. > > This commit initializes for "git svn branch", the SVN::Client with > the 'config' > option instead of 'auth'. As decribed in the SVN documentation, > http://search.cpan.org/~mschwern/Alien-SVN-v1.7.17.1/src/subversion/subversion/bindings/swig/perl/native/Client.pm#METHODS > the SVN::Client will then read cached authentication options. > > Signed-off-by: Monard Vong Thanks, I do not have a good way to test this, but the idea seems correct. Your patch is whitespace mangled, and the commit message and subject needs to be improved (see Documentation/SubmittingPatches on how to describe your changes and how to send them without whitespace mangling.) Thanks again. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: doublecheck if really file or dir
Andrej Manduch wrote:
> * this fixes 'git svn info `pwd`' buggy behaviour
Good catch, the commit could use a better description, something like:
--- 8<
Subject: [PATCH] git-svn: "info" checks for dirs more carefully
This avoids a "Reading from filehandle failed at ..." error when
running "git svn info `pwd`".
Signed-off-by: Andrej Manduch
--- 8<
While your patch avoids an error, but the output isn't right, either.
I tried it running in /home/ew/ruby, the URL field is bogus:
~/ruby$ git svn info `pwd`
Path: /home/ew/ruby
URL: svn+ssh://svn.ruby-lang.org/ruby/trunk/home/ew/ruby
Repository Root: svn+ssh://svn.ruby-lang.org/ruby
Repository UUID: b2dd03c8-39d4-4d8f-98ff-823fe69b080e
Revision: 46901
Node Kind: directory
Schedule: normal
Last Changed Author: hsbt
Last Changed Rev: 46901
Last Changed Date: 2014-07-22 19:06:12 + (Tue, 22 Jul 2014)
The URL should be:
URL: svn+ssh://svn.ruby-lang.org/ruby/trunk
It's better than an error, but it'd be nice if someone who uses
this command can fix it (*hint* :).
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -2029,7 +2029,7 @@ sub find_file_type_and_diff_status {
> my $mode = (split(' ', $ls_tree))[0] || "";
>
> return ("link", $diff_status) if $mode eq "12";
> - return ("dir", $diff_status) if $mode eq "04";
> + return ("dir", $diff_status) if $mode eq "04" or -d $path;
"or" has a lower precedence than "||", so I would do the following:
return ("dir", $diff_status) if $mode eq "04" || -d $path;
The general rule I've learned is to use "||" for conditionals and
"or" for control flow (e.g. do_something() or die("...") ).
I can take your patch with the above changes (no need to resend),
but I'd be happier to see the URL field corrected if you want
to reroll.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: doublecheck if really file or dir
I'm sorry, i've made mistake in this one. Please ingnore it. Correct
patch is on the way.
kind regards,
b.
On Fri, Jul 18, 2014 at 6:05 AM, Andrej Manduch wrote:
> * this fixes 'git svn info `pwd`' buggy behaviour
>
> Signed-off-by: Andrej Manduch
> ---
> git-svn.perl | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-svn.perl b/git-svn.perl
> index 0a32372..c3d893e 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -2029,7 +2029,7 @@ sub find_file_type_and_diff_status {
> my $mode = (split(' ', $ls_tree))[0] || "";
>
> return ("link", $diff_status) if $mode eq "12";
> - return ("dir", $diff_status) if $mode eq "04";
> + return ("dir", $diff_status) if $mode eq "04" od -d $path;
> return ("file", $diff_status);
> }
>
> --
> 2.0.0.GIT
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: workaround for a bug in svn serf backend
2013/12/30 Thomas Rast :
> Roman Kagan writes:
>
>> + # workaround for a bug in svn serf backend (v1.8.5 and below):
>> + # store 3d argument to ->add_file() in a local variable, to make it
>> + # have the same lifetime as $fbat
>> + my $upa = $self->url_path($m->{file_a});
>> my $fbat = $self->add_file($self->repo_path($m->{file_b}), $pbat,
>> - $self->url_path($m->{file_a}), $self->{r});
>> + $upa, $self->{r});
>
> Hmm, now that you put it that way, I wonder if the patch is correct.
>
> Let me first rephrase the problem to verify that I understand the issue:
>
> $fbat keeps a pointer to the $upa string, without maintaining a
> reference to it. When $fbat is destroyed, it needs this string, so we
> must ensure that the lifetime of $upa is at least as long as that of
> $fbat.
No. The string is needed in subversion's close_file(), so we want to
keep it alive until close_file() returns. Surviving till the end of
the current function scope is sufficient for that.
> However, does Perl make any guarantees as to the order in which local
> variables are unreferenced and then destroyed?
We don't care about the order they are destroyed WRT each other.
Roman.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: workaround for a bug in svn serf backend
Roman Kagan writes:
> + # workaround for a bug in svn serf backend (v1.8.5 and below):
> + # store 3d argument to ->add_file() in a local variable, to make it
> + # have the same lifetime as $fbat
> + my $upa = $self->url_path($m->{file_a});
> my $fbat = $self->add_file($self->repo_path($m->{file_b}), $pbat,
> - $self->url_path($m->{file_a}), $self->{r});
> + $upa, $self->{r});
Hmm, now that you put it that way, I wonder if the patch is correct.
Let me first rephrase the problem to verify that I understand the issue:
$fbat keeps a pointer to the $upa string, without maintaining a
reference to it. When $fbat is destroyed, it needs this string, so we
must ensure that the lifetime of $upa is at least as long as that of
$fbat.
However, does Perl make any guarantees as to the order in which local
variables are unreferenced and then destroyed? I can't find any such
guarantee.
In the absence of such, wouldn't we have to keep $upa in an outer,
separate scope to ensure that $fbat is destroyed first?
--
Thomas Rast
t...@thomasrast.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
Re: [PATCH] git-svn: workaround for a bug in svn serf backend
2013/12/27 Roman Kagan : > 2013/12/27 Jonathan Nieder : >> Could this be reproduced with a test script to make sure we don't >> reintroduce the bug again later? (It's okay if the test only fails on >> machines with the problematic svn version.) > > That would need a fairly fancy setup phase, as the bug triggers only > on http(s)-accessed svn repositories. I'll take a look if there's > something already available in the existing test scripts Turns out the stuff is all there, and the tests doing file renames and dcomit-ting them do exist (t9115-git-svn-dcommit-funky-renames.sh for one). However, the httpd setup is seriously broken; I haven't managed to get it to run on my Fedora 20 with apache 2.4.6. Apparently git-svn tests (almost) never get executed against an http-based repository; even those who don't set NO_SVN_TESTS get them run against file-based repository and thus don't trigger the error. Someone with better apache-foo needs to take a look into that. Once that is sorted out I believe the tests will start triggering the bug. Meanwhile I assume that the patch doesn't need to include an extra testcase. Roman. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: workaround for a bug in svn serf backend
2013/12/27 Jonathan Nieder : > Roman Kagan wrote: > >> Subversion serf backend in versions 1.8.5 and below has a bug that the >> function creating the descriptor of a file change -- add_file() -- >> doesn't make a copy of its 3d argument when storing it on the returned > > 3d makes me think of 3-dimensional. ;-) I think you mean third > (or the abbreviation 3rd). Indeed. >> descriptor. As a result, by the time this field is used (in >> transactions of file copying or renaming) it may well be released. > > Please describe the symptom so this patch is easy to find when other > people run into it. OK > Do I remember correctly that "... released and scribbled over with a > new value, causing such-and-such assertion to fire" was what happened? Exactly. >> This patch works around this bug, by storing the value to be passed as >> the 3d argument to add_file() in a local variable with the same scope as >> the file change descriptor, making sure their lifetime is the same. > > Could this be reproduced with a test script to make sure we don't > reintroduce the bug again later? (It's okay if the test only fails on > machines with the problematic svn version.) That would need a fairly fancy setup phase, as the bug triggers only on http(s)-accessed svn repositories. I'll take a look if there's something already available in the existing test scripts; writing one from scratch for this specific case is IMO beyond the reasonable effort. > Modulo the confusing 3-dimensional arguments in comments, the code > change looks good. Thanks, I'll adjust the wording and resubmit. Roman. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: workaround for a bug in svn serf backend
Roman Kagan wrote: > Subversion serf backend in versions 1.8.5 and below has a bug that the > function creating the descriptor of a file change -- add_file() -- > doesn't make a copy of its 3d argument when storing it on the returned 3d makes me think of 3-dimensional. ;-) I think you mean third (or the abbreviation 3rd). > descriptor. As a result, by the time this field is used (in > transactions of file copying or renaming) it may well be released. Please describe the symptom so this patch is easy to find when other people run into it. Do I remember correctly that "... released and scribbled over with a new value, causing such-and-such assertion to fire" was what happened? > This patch works around this bug, by storing the value to be passed as > the 3d argument to add_file() in a local variable with the same scope as > the file change descriptor, making sure their lifetime is the same. Could this be reproduced with a test script to make sure we don't reintroduce the bug again later? (It's okay if the test only fails on machines with the problematic svn version.) Modulo the confusing 3-dimensional arguments in comments, the code change looks good. Thanks and hope that helps, 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] git-svn: Support svn:global-ignores property
Aleksey Vasenev wrote:
> ---
What Thomas said about commit messages.
Note: I hardly use git-svn or SVN anymore and don't pay attention
to SVN changes.
Some style nitpicks:
> @@ -1304,16 +1318,20 @@ sub cmd_create_ignore {
> # which git won't track
> mkpath([$path]) unless -d $path;
> my $ignore = $path . '.gitignore';
> - my $s = $props->{'svn:ignore'} or return;
> + my $s = &get_svn_ignore($props, 'svn:ignore');
> + my $s_global = &get_svn_ignore($props, 'svn:global-ignores');
&sub(...) convention isn't consistent with the rest of our Perl code.
Do this instead:
my $s = get_svn_ignore($props, 'svn:ignore');
my $s_global = get_svn_ignore($props, 'svn:global-ignores');
> + $s or $s_global or return;
Precedence should be more explicit:
($s || $s_global) or return;
Likewise for cmd_show_ignore. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: Support svn:global-ignores property
Hi Aleksey Thanks for your patch. I added Eric Wong to the Cc list; all git-svn patches should go to him. Aleksey Vasenev writes: > --- Can you write a commit message? If you need a guideline for what to write there, consider this snippet from Documentation/SubmittingPatches: The body should provide a meaningful commit message, which: . explains the problem the change tries to solve, iow, what is wrong with the current code without the change. . justifies the way the change solves the problem, iow, why the result with the change is better. . alternate solutions considered but discarded, if any. In particular, I'm curious about how global-ignores are different from ordinary ignores. After reading http://svnbook.red-bean.com/en/1.7/svn.advanced.props.special.ignore.html I don't understand why the above document speaks of a "config area" that holds the global-ignores configuration, while your patch seems to treat them as "just another property" set in the same way as existing svn:ignore. How does this work? > Documentation/git-svn.txt | 12 ++-- > git-svn.perl | 46 -- > 2 files changed, 38 insertions(+), 20 deletions(-) Can you add a test or two? -- Thomas Rast t...@thomasrast.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
Re: [PATCH] git-svn documentation: Use tabs consistently within the ascii doc
Stefan Beller writes: > While I can understand 4 or 7 white spaces are fancy, we'd rather want > to use tabs throughout the whole document. You missed lines 278 and 833. There are also some spaces around line 488, but maybe those are layout-relevant and so shouldn't be converted to tabs. -Keshav -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: fix signed commit parsing
Jonathan Nieder wrote: > Nicolas Vigier wrote: > > > When parsing a commit object, git-svn wrongly think that a line > > containing spaces means the end of headers and the start of the commit > > message. In case of signed commit, the gpgsig entry contains a line with > > one space, so "git svn dcommit" will include part of the signature in > > the commit message. > > > > An example of such problem : > > http://svnweb.mageia.org/treasurer?view=revision&revision=86 > > > > This commit changes the regex to only match an empty line as separator > > between the headers and the commit message. > > > > Signed-off-by: Nicolas Vigier > > Good catch. For what it's worth, > Reviewed-by: Jonathan Nieder Thanks both. I've signed-off, added Jonathan's reviewed-by and queued this up in my master. Currently at: The following changes since commit 128a96c98442524c7f2eeef4757b1e48445f24ce: Update draft release notes to 1.8.5 for the fifth batch of topics (2013-09-20 12:42:02 -0700) are available in the git repository at: git://git.bogomips.org/git-svn.git master for you to fetch changes up to 945b9c14ffd3e11c916ee2b2428a0b2be9645829: git-svn.txt: elaborate on rev_map files (2013-10-10 06:56:12 +) Keshav Kini (4): git-svn.txt: fix AsciiDoc formatting error git-svn.txt: reword description of gc command git-svn.txt: replace .git with $GIT_DIR git-svn.txt: elaborate on rev_map files Nicolas Vigier (1): git-svn: fix signed commit parsing Documentation/git-svn.txt | 46 +- git-svn.perl | 2 +- 2 files changed, 34 insertions(+), 14 deletions(-) -- Eric Wong -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: fix signed commit parsing
Nicolas Vigier wrote: > When parsing a commit object, git-svn wrongly think that a line > containing spaces means the end of headers and the start of the commit > message. In case of signed commit, the gpgsig entry contains a line with > one space, so "git svn dcommit" will include part of the signature in > the commit message. > > An example of such problem : > http://svnweb.mageia.org/treasurer?view=revision&revision=86 > > This commit changes the regex to only match an empty line as separator > between the headers and the commit message. > > Signed-off-by: Nicolas Vigier Good catch. For what it's worth, Reviewed-by: Jonathan Nieder -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: Fix termination issues for remote svn connections
"Uli Heller" writes:
> On Fri, September 6, 2013 2:44 pm, Kyle J. McKay wrote:
> ...
>> In any case, I can now reproduce the problem (serf 1.3.1 still breaks
>> since it does not yet contain the fix and it is the most recent serf
>> release available).
>>
>> And the Git/SVN/Ra.pm fix does eliminate the problem for me (both with
>> bulk updates and with skelta updates -- the crash occurs with either).
>
> Great. So I don't have to run any more tests ;)
>
> What shall I do next? Add some inline comments to the patch?
Something like this?
-- >8 --
From: Uli Heller
Subject: [PATCH] git-svn: fix termination issues for remote svn connections
git-svn used in combination with serf to talk to svn repository
served over HTTPS dumps core on termination.
This is caused by a bug in serf, and the most recent serf release
1.3.1 still exhibits the problem; a fix for the bug exists (see
https://code.google.com/p/serf/source/detail?r=2146).
Until the bug is fixed, work around the issue within the git perl
module Ra.pm by freeing the private copy of the remote access object
on termination, which seems to be sufficient to prevent the error
from happening.
Note: Since subversion-1.8.0 and later do require serf-1.2.1 or
later, this issue typically shows up when upgrading to a recent
version of subversion.
Credits go to Jonathan Lambrechts for proposing a fix to Ra.pm,
Evgeny Kotkov and Ivan Zhakov for fixing the issue in serf and
pointing me to that fix.
Signed-off-by: Uli Heller
Tested-by: Kyle J. McKay
---
perl/Git/SVN/Ra.pm | 5 +
1 file changed, 5 insertions(+)
diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index 75ecc42..78dd346 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -32,6 +32,14 @@ BEGIN {
}
}
+# serf has a bug that leads to a coredump upon termination if the
+# remote access object is left around (not fixed yet in serf 1.3.1).
+# Explicitly free it to work around the issue.
+END {
+ $RA = undef;
+ $ra_invalid = 1;
+}
+
sub _auth_providers () {
my @rv = (
SVN::Client::get_simple_provider(),
--
1.8.4-431-g94f3a6b
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: Fix termination issues for remote svn connections
On Fri, September 6, 2013 2:44 pm, Kyle J. McKay wrote: > On Sep 6, 2013, at 05:06, Uli Heller wrote: >>> I'm using Git built from master (57e4c1783). I see the same behavior >>> both with and without the SVN/Ra.pm patch (and using both bulk >>> updates >>> and skelta mode). Does the problem not happen on a git svn clone? I >>> can force serf back to version 1.2.1 and try that version just to >>> see, >>> but I would like to have an example of a known failing git svn >>> command >>> for testing purposes. Thanks. >> >> I think this command should produce the error: >> >> git svn clone --stdlayout https://github.com/uli-heller/uli-javawrapper >> >> You can use any other svn repo as well, you only have to specify an >> HTTPS >> url. > > Yes, that does it. Interesting that cloning from > "https://github.com/uli-heller/uli-javawrapper > " core dumps while cloning from > "http://github.com/uli-heller/uli-javawrapper > " does not even though the latter redirects to > "https://github.com/uli-heller/uli-javawrapper > ". > > In any case, I can now reproduce the problem (serf 1.3.1 still breaks > since it does not yet contain the fix and it is the most recent serf > release available). > > And the Git/SVN/Ra.pm fix does eliminate the problem for me (both with > bulk updates and with skelta updates -- the crash occurs with either). Great. So I don't have to run any more tests ;) What shall I do next? Add some inline comments to the patch? --- Best regards, Uli. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] git-svn: Fix termination issues for remote svn connections
On Sep 5, 2013, at 11:48, Junio C Hamano wrote:
I am Cc'ing Kyle McKay who apparently had some experience working
with git-svn with newer svn that can only use serf, hoping that we
can get an independent opinion/test just to be sure.
On Sep 3, 2013, at 00:35, Uli Heller wrote:
When using git-svn in combination with serf-1.2.1 core dumps are
created on termination. This is caused by a bug in serf, a fix for
the bug exists (see https://code.google.com/p/serf/source/detail?r=2146)
.
Nevertheless, I think it makes sense to fix the issue within the
git perl module Ra.pm, too. The change frees the private copy of
the remote access object on termination which prevents the error
from happening.
Note: Since subversion-1.8.0 and later do require serf-1.2.1 or later,
the core dumps typically do show up when upgrading to a recent version
of subversion.
Credits: Jonathan Lambrechts for proposing a fix to Ra.pm.
Evgeny Kotkov and Ivan Zhakov for fixing the issue in serf and
pointing me to that fix.
---
In addition to the needed 'Signed-off-by:' a mention should be made
here of the need to use 'https:' to reproduce the problem.
perl/Git/SVN/Ra.pm | 5 +
1 file changed, 5 insertions(+)
diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index 75ecc42..78dd346 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -32,6 +32,11 @@ BEGIN {
}
}
+END {
+ $RA = undef;
+ $ra_invalid = 1;
+}
+
sub _auth_providers () {
my @rv = (
SVN::Client::get_simple_provider(),
--
1.8.4
Tested-by: Kyle J. McKay
I was able to reproduce the core dumps using subversion 1.8.3 (the
most recent subversion release) with serf 1.3.1 (the most recent serf
release) and verify that this patch eliminates the core dump at git-
svn termination 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
