Git development
 help / color / mirror / Atom feed
* Re: [PATCH] Submodules always use a relative path to gitdir
From: Junio C Hamano @ 2012-01-03 18:27 UTC (permalink / raw)
  To: Jens Lehmann; +Cc: Antony Male, git, iveqy
In-Reply-To: <4F007492.8010909@web.de>

Jens Lehmann <Jens.Lehmann@web.de> writes:

> Am 29.12.2011 23:40, schrieb Junio C Hamano:
>> Antony Male <antony.male@gmail.com> writes:
>> I further wonder if we can get away without using separate-git-dir option
>> in this codepath, though. IOW using
>> 
>>         git clone $quiet -bare ${reference:+"$reference"} "$url" "$gitdir"
>> 
>> might be a better solution.
>
> A quick test shows that using a bare repo won't fly because without the
> core.worktree setting commands that operate on the work tree can't be
> run anymore inside submodules (starting with the initial checkout). 

Probably the right thing to do would be to restructure the flow as I
suggested, i.e.

	if we do not have it yet
        then
        	git clone --bare ...
	fi
	# now we have it, make sure they are correct
	git config core.bare false
	git config core.worktree $there
        echo "gitdir: $here" >$there/.git

> Yes, and the core.worktree setting also contains an absolute path. So
> we must either make that relative too and rewrite it on every "git
> submodule add" to record the possibly changed path there or make the
> bare clone work with a work tree (which sounds a bit strange ;-).

Update of core.worktree has to be done regardless of the absolute/relative
differences anyway, no?

The first version of the superproject you trigger module_clone for
submodule $name may happen to have it at $path, module_clone notices that
you do not have it, and the initial "clone --separate-git-dir" will set
the core.worktree to $superproject/$path.  Nobody will update it after
that, even when we check out different version of superproject that has
the same submodule $name at a different location in the superproject.

^ permalink raw reply

* Re: [PATCH 2/2] git-svn, perl/Git.pm: extend and use Git->prompt method for querying users
From: Junio C Hamano @ 2012-01-03 18:19 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: git, Jeff King, Jakub Narebski
In-Reply-To: <4F00B7F3.1060105@tu-clausthal.de>

Sven Strickroth <sven.strickroth@tu-clausthal.de> writes:

> I'm still for the second patch to be applied (maybe w/o the certificate
> filename prompt), too, because this makes git-svn behave the save way as
> git-core does (especially asking for username).

I do not have much issues against the patch if the filename thing is
excluded as a short-term workaround to avoid regression.

For people who have been used to interact with git-svn from the terminal
but has *_ASKPASS for reasons other than their use of git-svn in their
environment, the change to the username codepath is technically a
regression, as they used to be able to see and correct typo while giving
their username but with the patch *_ASKPASS will kick in and they have to
type in bline, but I am not particularly worried about it. It is something
you type very often and committed to your muscle memory anyway.

^ permalink raw reply

* Re: git-p4 under cygwin
From: Thomas Berg @ 2012-01-03 16:07 UTC (permalink / raw)
  To: Paul Chown; +Cc: git
In-Reply-To: <1325604726.49597.YahooMailClassic@web132101.mail.ird.yahoo.com>

On Tue, Jan 3, 2012 at 4:32 PM, Paul Chown <pmchown@yahoo.co.uk> wrote:
> Path '/cygdrive/c/work/perforce/config_test\...' is not under client's root 'c:\work\perforce\config_test'.

Ah, sorry, I did not see this part of the error message when I first
replied. This does seem like a cygwin related problem. Not sure how
easy it is to solve. But as mentioned, msysgit works fine with git-p4.

- Thomas

^ permalink raw reply

* Re: git-p4 under cygwin
From: Thomas Berg @ 2012-01-03 15:54 UTC (permalink / raw)
  To: Paul Chown; +Cc: git
In-Reply-To: <1325604726.49597.YahooMailClassic@web132101.mail.ird.yahoo.com>

On Tue, Jan 3, 2012 at 4:32 PM, Paul Chown <pmchown@yahoo.co.uk> wrote:
> Is there are recommended approach for using git-p4 on Windows? I also tried using the non-Cygwin installations of git, but these don't seem to have any Python support in them, so the git-p4 code won't run at all.

I'm not sure, but the error may not be a cygwin related problem at
all, it could be a pure git-p4 usage issue. Do you have a perforce
workspace set up, and do you have the affected files mapped into your
workspace so git-p4 can run p4 sync on them?

Other than that, git-p4 works excellently with the regular msysgit
too. Just install python the regular way on Windows (under C:\Python27
for example) and add it to your path. This python runs just fine in
the MinGW shell too (I think it picks up any PATH changes you make in
Windows). Then just download the git-p4 python script and make sure it
is in your path. This is how I use it.

- Thomas Berg

^ permalink raw reply

* git-p4 under cygwin
From: Paul Chown @ 2012-01-03 15:32 UTC (permalink / raw)
  To: git

I'm trying to use git-p4 to push changes from git into a Perforce repository under Cygwin, using the cygwin git installation. I followed the instructions at http://kb.perforce.com/article/1417/git-p4 and everything works fine until I do the final 'git p4 submit'.

During the submit command I get the following error:
 
Path '/cygdrive/c/work/perforce/config_test\...' is not under client's root 'c:\work\perforce\config_test'.
Traceback (most recent call last):
  File "/usr/local/bin/git-p4", line 2371, in <module>
    main()
  File "/usr/local/bin/git-p4", line 2366, in main
    if not cmd.run(args):
  File "/usr/local/bin/git-p4", line 1130, in run
    p4_sync("...")
  File "/usr/local/bin/git-p4", line 137, in p4_sync
    p4_system(["sync", path])
  File "/usr/local/bin/git-p4", line 131, in p4_system
    subprocess.check_call(real_cmd, shell=expand)
  File "/usr/lib/python2.6/subprocess.py", line 498, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['p4', 'sync', '...']' returned non-zero exit status 1

Is there are recommended approach for using git-p4 on Windows? I also tried using the non-Cygwin installations of git, but these don't seem to have any Python support in them, so the git-p4 code won't run at all.

Many thanks in advance

Paul Chown

^ permalink raw reply

* Re: rebase -i does not use a commit it should
From: Kevin @ 2012-01-03 14:18 UTC (permalink / raw)
  To: git
In-Reply-To: <CAO54GHAwgbdpdqZw5Pc+jvEyq12DAq4Bio_MbTnmXHsvm-5p8Q@mail.gmail.com>

Ok, problem solved.

The issue was already present in the history of master, and thus it
would not be applied again.

On Tue, Jan 3, 2012 at 3:04 PM, Kevin <ikke@ikke.info> wrote:
> Using git version 1.7.8

^ permalink raw reply

* Re: rebase -i does not use a commit it should
From: Kevin @ 2012-01-03 14:04 UTC (permalink / raw)
  To: git
In-Reply-To: <CAO54GHB1uwJdjj0=dCVp_M4sAFtqooM9hnBSNurje2n+-dGDEA@mail.gmail.com>

Using git version 1.7.8

^ permalink raw reply

* rebase -i does not use a commit it should
From: Kevin @ 2012-01-03 14:00 UTC (permalink / raw)
  To: git

I tried to rebase some commits, when I found out it didn't pick all
commits it should.

A distiled situation is this:

A (issues/links_footer)
B (issues/links-footer)
C
|  D (master)
|  |
|  |
|  |
|  |
| /
*

Here is the output of git log --graph --oneline --format="%h %d" master HEAD
http://paste.pocoo.org/show/529305/

And this is the output of git log --oneline --format="%h" master..

7fb46fd
6efa164
50b5950

When I do git rebase -i master, it only shows the top 2 commits, but
if I do git rebase -i 50b5950^, it does show all three committs.

Anyone knows whats going wrong?

^ permalink raw reply

* [PATCH] fix hang in git fetch if pointed at a 0 length bundle
From: Brian Harring @ 2012-01-03 13:46 UTC (permalink / raw)
  To: git
In-Reply-To: <20120103011352.GA13825@localhost>

git-repo if interupted at the exact wrong time will generate zero
length bundles- literal empty files.  git-repo is wrong here, but
git fetch shouldn't effectively spin loop if pointed at a zero
length bundle.

Signed-off-by: Brian Harring <ferringb@chromium.org>
---
 bundle.c          |    4 ++--
 t/t5704-bundle.sh |    6 ++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/bundle.c b/bundle.c
index 4742f27..b8acf3c 100644
--- a/bundle.c
+++ b/bundle.c
@@ -31,8 +31,8 @@ static int strbuf_readline_fd(struct strbuf *sb, int fd)
 	while (1) {
 		char ch;
 		ssize_t len = xread(fd, &ch, 1);
-		if (len < 0)
-			return -1;
+		if (len <= 0)
+			return len;
 		strbuf_addch(sb, ch);
 		if (ch == '\n')
 			break;
diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index 728ccd8..4ae127d 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -53,4 +53,10 @@ test_expect_failure 'bundle --stdin <rev-list options>' '
 
 '
 
+test_expect_success 'empty bundle file is rejected' '
+
+    >empty-bundle && test_must_fail git fetch empty-bundle
+
+'
+
 test_done
-- 
1.7.8.2

^ permalink raw reply related

* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS
From: Sven Strickroth @ 2012-01-03 13:18 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Jakub Narebski, Jeff King
In-Reply-To: <CACBZZX6iMobuU90skpbNPaGQFxYNOAjmZ6ceO4PGqfZSMkgePQ@mail.gmail.com>

Am 03.01.2012 13:03 schrieb Ævar Arnfjörð Bjarmason:
>> I'm using msys perl (shipped with msysgit) and there just using chomp()
>> did not work.

But why not drop all \n and \r, since we only accept and wait for a
single line?

chomp($str = <STDIN>) works as expected, but for some reason the line I
got from the ASKPASS tool isn't. Btw. the askpass tool provides a string
followed with \n (c++).

> That's odd, what does this print:
> 
>     perl -MData::Dumper -MFile::Temp=tempfile -we 'my $str =
> "moo\015\012"; my ($fh, $name) = tempfile(); print $fh $str; close
> $fh; open my $in, "<", $name or die $!; my $in_str = <$in>; chomp(my
> $cin_str = $in_str); print "in_str:<$in_str> cin_str:<$cin_str>
> END\n"'
> 
> And how about this:
> 
>     perl -MData::Dumper -MFile::Temp=tempfile -we 'my $str =
> "moo\015\012"; my ($fh, $name) = tempfile(); print $fh $str; close
> $fh; open my $in, "<:crlf", $name or die $!; my $in_str = <$in>;
> chomp(my $cin_str = $in_str); print "in_str:<$in_str>
> cin_str:<$cin_str> END\n"'
> 
> It could be that there's some bug in either perl or mingw's build of
> perl where it won't turn on the :crlf IO layer by default.

I get an error in both cases "Das System kann die angegebene Datei nicht
finden".

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply

* Re: [PATCH] fix hang in git fetch if pointed at a 0 length bundle
From: Nguyen Thai Ngoc Duy @ 2012-01-03 12:07 UTC (permalink / raw)
  To: Brian Harring; +Cc: git, spearce
In-Reply-To: <20120103011352.GA13825@localhost>

On Tue, Jan 3, 2012 at 8:13 AM, Brian Harring <ferringb@gmail.com> wrote:
> @@ -31,7 +31,7 @@ static int strbuf_readline_fd(struct strbuf *sb, int fd)
>        while (1) {
>                char ch;
>                ssize_t len = xread(fd, &ch, 1);
> -               if (len < 0)
> +               if (len <= 0)
>                        return -1;
>                strbuf_addch(sb, ch);
>                if (ch == '\n')

I think it should return 0 when len == 0 because strictly speaking eof
is not a fault. It's not really important though because the two
callers in this file work fine either way.

FWIW I went through all xread call sites. All seem to handle return
value <= 0 correctly.
-- 
Duy

^ permalink raw reply

* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS
From: Ævar Arnfjörð Bjarmason @ 2012-01-03 12:06 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: git, Junio C Hamano, Jakub Narebski, Jeff King
In-Reply-To: <CACBZZX6iMobuU90skpbNPaGQFxYNOAjmZ6ceO4PGqfZSMkgePQ@mail.gmail.com>

On Tue, Jan 3, 2012 at 13:03, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Tue, Jan 3, 2012 at 11:25, Sven Strickroth
> <sven.strickroth@tu-clausthal.de> wrote:
>> Am 03.01.2012 11:17 schrieb Ævar Arnfjörð Bjarmason:
>>>> +       $ret = <$fh>;
>>>> +       $ret =~ s/[\012\015]//g; # strip \n\r, chomp does not work on all systems (i.e. windows) as expected
>>>
>>> Urm yes it does. \n in Perl is magical and doesn't mean \012 like it
>>> does in some languages. It means "The Platform's Native
>>> Newline".
>>>
>>> Which is \012 on Unix, \015\012 on Windows, and was \015 on Mac OS
>>> until support for it was removed. This is covered in the second
>>> section of "perldoc perlport".
>>>
>>> Can you show me a case where it fails, and under what environment
>>> exactly? Maybe it's e.g.s some Cygwin-specific peculiarity, in which
>>> case we could check for that platform specifically.
>>
>> I'm using msys perl (shipped with msysgit) and there just using chomp()
>> did not work.
>
> That's odd, what does this print:
>
>    perl -MData::Dumper -MFile::Temp=tempfile -we 'my $str =
> "moo\015\012"; my ($fh, $name) = tempfile(); print $fh $str; close
> $fh; open my $in, "<", $name or die $!; my $in_str = <$in>; chomp(my
> $cin_str = $in_str); print "in_str:<$in_str> cin_str:<$cin_str>
> END\n"'
>
> And how about this:
>
>    perl -MData::Dumper -MFile::Temp=tempfile -we 'my $str =
> "moo\015\012"; my ($fh, $name) = tempfile(); print $fh $str; close
> $fh; open my $in, "<:crlf", $name or die $!; my $in_str = <$in>;
> chomp(my $cin_str = $in_str); print "in_str:<$in_str>
> cin_str:<$cin_str> END\n"'
>
> It could be that there's some bug in either perl or mingw's build of
> perl where it won't turn on the :crlf IO layer by default.

Or actually you could do this before running your patch, except you
have to change the code to use chomp() instead of your regex hack:

  export PERLIO=crlf

If that makes it work we should just do that somewhere else (e.g. at
the top of Git.pm) if we detect Windows, which'll make chomp() work as
intended everywhere.

^ permalink raw reply

* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS
From: Ævar Arnfjörð Bjarmason @ 2012-01-03 12:03 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: git, Junio C Hamano, Jakub Narebski, Jeff King
In-Reply-To: <4F02D79B.1020002@tu-clausthal.de>

On Tue, Jan 3, 2012 at 11:25, Sven Strickroth
<sven.strickroth@tu-clausthal.de> wrote:
> Am 03.01.2012 11:17 schrieb Ævar Arnfjörð Bjarmason:
>>> +       $ret = <$fh>;
>>> +       $ret =~ s/[\012\015]//g; # strip \n\r, chomp does not work on all systems (i.e. windows) as expected
>>
>> Urm yes it does. \n in Perl is magical and doesn't mean \012 like it
>> does in some languages. It means "The Platform's Native
>> Newline".
>>
>> Which is \012 on Unix, \015\012 on Windows, and was \015 on Mac OS
>> until support for it was removed. This is covered in the second
>> section of "perldoc perlport".
>>
>> Can you show me a case where it fails, and under what environment
>> exactly? Maybe it's e.g.s some Cygwin-specific peculiarity, in which
>> case we could check for that platform specifically.
>
> I'm using msys perl (shipped with msysgit) and there just using chomp()
> did not work.

That's odd, what does this print:

    perl -MData::Dumper -MFile::Temp=tempfile -we 'my $str =
"moo\015\012"; my ($fh, $name) = tempfile(); print $fh $str; close
$fh; open my $in, "<", $name or die $!; my $in_str = <$in>; chomp(my
$cin_str = $in_str); print "in_str:<$in_str> cin_str:<$cin_str>
END\n"'

And how about this:

    perl -MData::Dumper -MFile::Temp=tempfile -we 'my $str =
"moo\015\012"; my ($fh, $name) = tempfile(); print $fh $str; close
$fh; open my $in, "<:crlf", $name or die $!; my $in_str = <$in>;
chomp(my $cin_str = $in_str); print "in_str:<$in_str>
cin_str:<$cin_str> END\n"'

It could be that there's some bug in either perl or mingw's build of
perl where it won't turn on the :crlf IO layer by default.

^ permalink raw reply

* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS
From: Sven Strickroth @ 2012-01-03 10:25 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Junio C Hamano, Jakub Narebski, Jeff King
In-Reply-To: <CACBZZX7P9PEq0wZp0d3dSwDjF6J6Z3cO4VtWc9_frBengtqPLw@mail.gmail.com>

Am 03.01.2012 11:17 schrieb Ævar Arnfjörð Bjarmason:
>> +       $ret = <$fh>;
>> +       $ret =~ s/[\012\015]//g; # strip \n\r, chomp does not work on all systems (i.e. windows) as expected
> 
> Urm yes it does. \n in Perl is magical and doesn't mean \012 like it
> does in some languages. It means "The Platform's Native
> Newline".
> 
> Which is \012 on Unix, \015\012 on Windows, and was \015 on Mac OS
> until support for it was removed. This is covered in the second
> section of "perldoc perlport".
> 
> Can you show me a case where it fails, and under what environment
> exactly? Maybe it's e.g.s some Cygwin-specific peculiarity, in which
> case we could check for that platform specifically.

I'm using msys perl (shipped with msysgit) and there just using chomp()
did not work.

-- 
Best regards,
 Sven Strickroth
 ClamAV, a GPL anti-virus toolkit   http://www.clamav.net
 PGP key id F5A9D4C4 @ any key-server

^ permalink raw reply

* Re: [PATCH 1/2] git-svn, perl/Git.pm: add central method for prompting passwords honoring GIT_ASKPASS and SSH_ASKPASS
From: Ævar Arnfjörð Bjarmason @ 2012-01-03 10:17 UTC (permalink / raw)
  To: Sven Strickroth; +Cc: git, Junio C Hamano, Jakub Narebski, Jeff King
In-Reply-To: <4EFA5EB3.4000802@tu-clausthal.de>

On Wed, Dec 28, 2011 at 01:11, Sven Strickroth
<sven.strickroth@tu-clausthal.de> wrote:

Nom nom, some Perl. Thanks for tackling this. Reviewing it as
requested by Junio.

> diff --git a/git-svn.perl b/git-svn.perl
> index eeb83d3..bcee8aa 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -4415,25 +4415,7 @@ sub username {
>
>  sub _read_password {
>        my ($prompt, $realm) = @_;
> -       my $password = '';
> -       if (exists $ENV{GIT_ASKPASS}) {
> -               open(PH, "-|", $ENV{GIT_ASKPASS}, $prompt);
> -               $password = <PH>;
> -               $password =~ s/[\012\015]//; # \n\r
> -               close(PH);
> -       } else {
> -               print STDERR $prompt;
> -               STDERR->flush;
> -               require Term::ReadKey;
> -               Term::ReadKey::ReadMode('noecho');
> -               while (defined(my $key = Term::ReadKey::ReadKey(0))) {
> -                       last if $key =~ /[\012\015]/; # \n\r
> -                       $password .= $key;
> -               }
> -               Term::ReadKey::ReadMode('restore');
> -               print STDERR "\n";
> -               STDERR->flush;
> -       }
> +       my $password = Git->prompt($prompt);
>        $password;
>  }
>
> diff --git a/perl/Git.pm b/perl/Git.pm
> index f7ce511..b1c7c50 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -58,7 +58,7 @@ require Exporter;
>                 command_output_pipe command_input_pipe command_close_pipe
>                 command_bidi_pipe command_close_bidi_pipe
>                 version exec_path html_path hash_object git_cmd_try
> -                remote_refs
> +                remote_refs prompt
>                 temp_acquire temp_release temp_reset temp_path);
>
>
> @@ -512,6 +512,55 @@ C<git --html-path>). Useful mostly only internally.
>  sub html_path { command_oneline('--html-path') }
>
>
> +=item prompt ( PROMPT )
> +
> +Query user C<PROMPT> and return answer from user.
> +
> +Check if GIT_ASKPASS or SSH_ASKPASS is set, use first matching for querying
> +user and return answer. If no *_ASKPASS variable is set, the variable is
> +empty or an error occoured, the terminal is tried as a fallback.
> +
> +=cut
> +
> +sub prompt {
> +       my ($self, $prompt) = _maybe_self(@_);
> +       my $ret;
> +       if (exists $ENV{'GIT_ASKPASS'}) {
> +               $ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
> +       }
> +       if (!defined $ret && exists $ENV{'SSH_ASKPASS'}) {
> +               $ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
> +       }
> +       if (!defined $ret) {
> +               print STDERR $prompt;
> +               STDERR->flush;
> +               require Term::ReadKey;
> +               Term::ReadKey::ReadMode('noecho');
> +               while (defined(my $key = Term::ReadKey::ReadKey(0))) {
> +                       last if $key =~ /[\012\015]/; # \n\r
> +                       $ret .= $key;
> +               }
> +               Term::ReadKey::ReadMode('restore');
> +               print STDERR "\n";
> +               STDERR->flush;
> +       }
> +       return $ret;
> +}

Personally I prefer not to write code I don't have to write. Here
you're doing:

    my ($self, $prompt) = _maybe_self(@_);

And then never using $self, so there's actually no reason for this to
be an object/class function. It could just be called as:

    my $password = Git::prompt($prompt);

Instead of:

    my $password = Git->prompt($prompt);

Which means you could change the first line to just:

    my ($prompt) = @_;

Which wouldn't leave the reader wondering why this needs to maybe
construct an object just to throw it away.

> +sub _prompt {
> +       my ($askpass, $prompt) = @_;
> +       unless ($askpass) {
> +               return undef;
> +       }

The empty list is false in Perl, so explicitly returning undef is
usually the wrong thing. It means that in list context you'll return a
true list (consisting of one undef element), instead of an empty false
one.

    $ perl -MData::Dump=dump -wle 'sub a { return } sub b { return
undef }; my @lists = ([a()], [b()]); for (@lists) { print @$_ ? "true"
: "false" }'
    false
    true

You're not running into that error here since you always use scalar
context. But it's better just to do:

    if (xyz) {
        return;
    }

Unless you want this behavior.

But aside from that I find this code a bit bizarre. The caller is
doing:

    if (exists $ENV{'GIT_ASKPASS'}) {
            $ret = _prompt($ENV{'GIT_ASKPASS'}, $prompt);
    }
    if (!defined $ret && exists $ENV{'SSH_ASKPASS'}) {
            $ret = _prompt($ENV{'SSH_ASKPASS'}, $prompt);
    }

Which means we check whether *_ASKPASS *exists* in the env hash, and
then the function checks whether that value is true or not.

Which means the code implicitly depends on Perl's idea of true/false
values for the names of those commands. E.g. you can't create a
command called "0" and put it in your $PATH.

I suppose the case you actually care about is someone being able to do:

    export GIT_ASKPASS=

Instead of the better:

    unset GIT_ASKPASS

> +       my $ret;
> +       open my $fh, "-|", $askpass, $prompt || return undef;

As pointed out already this is a logic error due to the ||.

But even if that worked I think this behavior is a bit
questionable. Why don't we just throw an error at this point? The way
I'd write this would be something like:

    my $pass = _prompt('GIT_ASKPASS', $prompt);

And then:

    sub _prompt {
        my ($env_var, $prompt) = @_;

        # or exists(), depending on whether we insist on "unset"
        return unless length $ENV{$env_var};

        my $command = $ENV{$env_var};
        open my $fh, "-|", $command or die "We can't get your password
from `$command' given in $env_var: $!";
        chomp(my $password = <$fh>);
        close $fh or die "We can't close() `$command' given in $env_var: $!";

        return $password;
    }

Which would give the user an error if his GIT_ASKPASS command
failed. Check the return value of close() too, and re-structure the
passing around of the env variable so we could give a sensible error
message.

> +       $ret = <$fh>;
> +       $ret =~ s/[\012\015]//g; # strip \n\r, chomp does not work on all systems (i.e. windows) as expected

Urm yes it does. \n in Perl is magical and doesn't mean \012 like it
does in some languages. It means "The Platform's Native
Newline".

Which is \012 on Unix, \015\012 on Windows, and was \015 on Mac OS
until support for it was removed. This is covered in the second
section of "perldoc perlport".

Can you show me a case where it fails, and under what environment
exactly? Maybe it's e.g.s some Cygwin-specific peculiarity, in which
case we could check for that platform specifically.

^ permalink raw reply

* Re: [PATCH] fix hang in git fetch if pointed at a 0 length bundle
From: Johannes Sixt @ 2012-01-03  8:35 UTC (permalink / raw)
  To: Brian Harring; +Cc: git, spearce
In-Reply-To: <20120103011352.GA13825@localhost>

Am 03.01.2012 02:13, schrieb Brian Harring:
> git-repo if interupted at the exact wrong time will generate zero
> length bundles- literal empty files.  git-repo is wrong here, but
> git fetch shouldn't effectively spin loop if pointed at a zero
> length bundle.

Adding a test case is very much appreciated.

> +test_expect_success 'die if bundle file is empty' '

How about 'empty bundle file is rejected'?

> +
> +   echo -n > empty-bundle

'echo -n' is not portable; use simply

	>empty-bundle &&

(note the style: no blank after >). Also chain commands using &&.

> +   timeout 5 git fetch empty-bundle

Yes, there was an infinite loop. But we do not specifically protect our
git invocations in the test suite against this sort of failure. Just write

	test_must_fail git fetch empty-bundle

and end the test case here.

> +   ret=$?
> +   [ $ret == 128 ] && return 0
> +   return $ret
> +
> +'

Furthermore, indentation should be one tabstop, not blanks.

-- Hannes

^ permalink raw reply

* Re: Git ghost references
From: Chris Leong @ 2012-01-03  8:32 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: git
In-Reply-To: <4F02BAF9.1000206@kdbg.org>

I did have a production file in my .git and also an empty folder left
in my remotes. Thanks heaps!

On Tue, Jan 3, 2012 at 7:23 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 03.01.2012 00:42, schrieb Chris Leong:
>> I seem to have a "ghost reference" - ie. I can check out a reference
>> that doesn't appear to exist. Does anyone know what might cause this?
>>
>> ~/gaf-cvs (project-membership)$ g show-ref | grep production
>> ~/gaf-cvs (project-membership)$ g co production
>> Note: checking out 'production'.
>> ...
>
> The most likely reason is that you have a ref 'production' directly in
> the .git directory. Perhaps you or one of your scripts created it
> accidentally using 'git update-ref production ae5b621', i.e., without
> giving the full ref path name.
>
> -- Hannes

^ permalink raw reply

* Re: Git ghost references
From: Johannes Sixt @ 2012-01-03  8:23 UTC (permalink / raw)
  To: Chris Leong; +Cc: git
In-Reply-To: <CAJ6vYjfpx-hfDsd+urp5_iS99p0RhmxohOc+TNv7SUWFnYe5wQ@mail.gmail.com>

Am 03.01.2012 00:42, schrieb Chris Leong:
> I seem to have a "ghost reference" - ie. I can check out a reference
> that doesn't appear to exist. Does anyone know what might cause this?
> 
> ~/gaf-cvs (project-membership)$ g show-ref | grep production
> ~/gaf-cvs (project-membership)$ g co production
> Note: checking out 'production'.
> ...

The most likely reason is that you have a ref 'production' directly in
the .git directory. Perhaps you or one of your scripts created it
accidentally using 'git update-ref production ae5b621', i.e., without
giving the full ref path name.

-- Hannes

^ permalink raw reply

* [PATCH] fix hang in git fetch if pointed at a 0 length bundle
From: Brian Harring @ 2012-01-03  1:13 UTC (permalink / raw)
  To: git; +Cc: spearce

git-repo if interupted at the exact wrong time will generate zero
length bundles- literal empty files.  git-repo is wrong here, but
git fetch shouldn't effectively spin loop if pointed at a zero
length bundle.

Signed-off-by: Brian Harring <ferringb@chromium.org>
---
 bundle.c          |    2 +-
 t/t5704-bundle.sh |   10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/bundle.c b/bundle.c
index 4742f27..ac9cc20 100644
--- a/bundle.c
+++ b/bundle.c
@@ -31,7 +31,7 @@ static int strbuf_readline_fd(struct strbuf *sb, int fd)
 	while (1) {
 		char ch;
 		ssize_t len = xread(fd, &ch, 1);
-		if (len < 0)
+		if (len <= 0)
 			return -1;
 		strbuf_addch(sb, ch);
 		if (ch == '\n')
diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index 728ccd8..75ce5b8 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -53,4 +53,14 @@ test_expect_failure 'bundle --stdin <rev-list options>' '
 
 '
 
+test_expect_success 'die if bundle file is empty' '
+
+   echo -n > empty-bundle
+   timeout 5 git fetch empty-bundle
+   ret=$?
+   [ $ret == 128 ] && return 0
+   return $ret
+
+'
+
 test_done
-- 
1.7.8.2

^ permalink raw reply related

* Git ghost references
From: Chris Leong @ 2012-01-02 23:42 UTC (permalink / raw)
  To: git

I seem to have a "ghost reference" - ie. I can check out a reference
that doesn't appear to exist. Does anyone know what might cause this?

~/gaf-cvs (project-membership)$ g show-ref | grep production
~/gaf-cvs (project-membership)$ g gc --prune=now
Counting objects: 117306, done.
Delta compression using up to 2 threads.
Compressing objects: 100% (25270/25270), done.
Writing objects: 100% (117306/117306), done.
Total 117306 (delta 83839), reused 117283 (delta 83820)
~/gaf-cvs (project-membership)$ g co production
Note: checking out 'production'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b new_branch_name

HEAD is now at ae5b621... Merge branch 'master' of git.freelancer.com:production
~/gaf-cvs ((no branch))$

^ permalink raw reply

* Stashing individual files
From: Chris Leong @ 2012-01-02 23:32 UTC (permalink / raw)
  To: git

Hi all,

Thanks for making such a wonderful product. I find the stash command
really useful, but it doesn't work very well when I just need to
temporarily revert one or two files. I know that there is the
interactive command, but if you have modified a large number of files,
then it takes quite a bit of effort. Is there any way I can define an
alias, stashfiles, so that I can just type git stashfiles file1 file2?
Also, please consider adding such a feature into a future version.

Thanks,

Chris

^ permalink raw reply

* [PATCH 6/6] git-p4: view spec documentation
From: Pete Wyckoff @ 2012-01-02 23:05 UTC (permalink / raw)
  To: git; +Cc: Gary Gibbons
In-Reply-To: <1325545554-16540-1-git-send-email-pw@padd.com>


Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 Documentation/git-p4.txt |   40 +++++++++++++++++++++++++++-------------
 1 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index f97b1c5..84f5b91 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -230,12 +230,7 @@ git repository:
 
 --use-client-spec::
 	Use a client spec to find the list of interesting files in p4.
-	The client spec is discovered using 'p4 client -o' which checks
-	the 'P4CLIENT' environment variable and returns a mapping of
-	depot files to workspace files.  Note that a depot path is
-	still required, but files found in the path that match in
-	the client spec view will be laid out according to the client
-	spec.
+	See the "CLIENT SPEC" section below.
 
 Clone options
 ~~~~~~~~~~~~~
@@ -304,6 +299,27 @@ p4 revision specifier on the end:
 See 'p4 help revisions' for the full syntax of p4 revision specifiers.
 
 
+CLIENT SPEC
+-----------
+The p4 client specification is maintained with the 'p4 client' command
+and contains among other fields, a View that specifies how the depot
+is mapped into the client repository.  Git-p4 can consult the client
+spec when given the '--use-client-spec' option or useClientSpec
+variable.
+
+The full syntax for a p4 view is documented in 'p4 help views'.  Git-p4
+knows only a subset of the view syntax.  It understands multi-line
+mappings, overlays with '+', exclusions with '-' and double-quotes
+around whitespace.  Of the possible wildcards, git-p4 only handles
+'...', and only when it is at the end of the path.  Git-p4 will complain
+if it encounters an unhandled wildcard.
+
+The name of the client can be given to git-p4 in multiple ways.  The
+variable 'git-p4.client' takes precedence if it exists.  Otherwise,
+normal p4 mechanisms of determining the client are used:  environment
+variable P4CLIENT, a file referenced by P4CONFIG, or the local host name.
+
+
 BRANCH DETECTION
 ----------------
 P4 does not have the same concept of a branch as git.  Instead,
@@ -387,9 +403,7 @@ git-p4.host::
 
 git-p4.client::
 	Client specified as an option to all p4 commands, with
-	'-c <client>'.  This can also be used as a way to find
-	the client spec for the 'useClientSpec' option.
-	The environment variable 'P4CLIENT' can be used instead.
+	'-c <client>', including the client spec.
 
 Clone and sync variables
 ~~~~~~~~~~~~~~~~~~~~~~~~
@@ -417,10 +431,10 @@ git config --add git-p4.branchList main:branchB
 -------------
 
 git-p4.useClientSpec::
-	Specify that the p4 client spec to be used to identify p4 depot
-	paths of interest.  This is equivalent to specifying the option
-	'--use-client-spec'.  The variable 'git-p4.client' can be used
-	to specify the name of the client.
+	Specify that the p4 client spec should be used to identify p4
+	depot paths of interest.  This is equivalent to specifying the
+	option '--use-client-spec'.  See the "CLIENT SPEC" section above.
+	This variable is a boolean, not the name of a p4 client.
 
 Submit variables
 ~~~~~~~~~~~~~~~~
-- 
1.7.8.1.407.gd70cb

^ permalink raw reply related

* [PATCH 5/6] git-p4: rewrite view handling
From: Pete Wyckoff @ 2012-01-02 23:05 UTC (permalink / raw)
  To: git; +Cc: Gary Gibbons
In-Reply-To: <1325545554-16540-1-git-send-email-pw@padd.com>

The old code was not very complete or robust.  Redo it.

This new code should be useful for a few possible additions
in the future:

    - support for * and %%n wildcards
    - allowing ... inside paths
    - representing branch specs (not just client specs)
    - tracking changes to views

Mark the remaining 12 tests in t9809 as fixed.

Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4    |  335 ++++++++++++++++++++++++++++++-----------
 t/t9809-git-p4-client-view.sh |   24 ++--
 2 files changed, 258 insertions(+), 101 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 8f28e0a..3e1aa27 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1169,6 +1169,218 @@ class P4Submit(Command, P4UserMap):
 
         return True
 
+class View(object):
+    """Represent a p4 view ("p4 help views"), and map files in a
+       repo according to the view."""
+
+    class Path(object):
+        """A depot or client path, possibly containing wildcards.
+           The only one supported is ... at the end, currently.
+           Initialize with the full path, with //depot or //client."""
+
+        def __init__(self, path, is_depot):
+            self.path = path
+            self.is_depot = is_depot
+            self.find_wildcards()
+            # remember the prefix bit, useful for relative mappings
+            m = re.match("(//[^/]+/)", self.path)
+            if not m:
+                die("Path %s does not start with //prefix/" % self.path)
+            prefix = m.group(1)
+            if not self.is_depot:
+                # strip //client/ on client paths
+                self.path = self.path[len(prefix):]
+
+        def find_wildcards(self):
+            """Make sure wildcards are valid, and set up internal
+               variables."""
+
+            self.ends_triple_dot = False
+            # There are three wildcards allowed in p4 views
+            # (see "p4 help views").  This code knows how to
+            # handle "..." (only at the end), but cannot deal with
+            # "%%n" or "*".  Only check the depot_side, as p4 should
+            # validate that the client_side matches too.
+            if re.search(r'%%[1-9]', self.path):
+                die("Can't handle %%n wildcards in view: %s" % self.path)
+            if self.path.find("*") >= 0:
+                die("Can't handle * wildcards in view: %s" % self.path)
+            triple_dot_index = self.path.find("...")
+            if triple_dot_index >= 0:
+                if not self.path.endswith("..."):
+                    die("Can handle ... wildcard only at end of path: %s" %
+                        self.path)
+                self.ends_triple_dot = True
+
+        def ensure_compatible(self, other_path):
+            """Make sure the wildcards agree."""
+            if self.ends_triple_dot != other_path.ends_triple_dot:
+                 die("Both paths must end with ... if either does;\n" +
+                     "paths: %s %s" % (self.path, other_path.path))
+
+        def match_wildcards(self, test_path):
+            """See if this test_path matches us, and fill in the value
+               of the wildcards if so.  Returns a tuple of
+               (True|False, wildcards[]).  For now, only the ... at end
+               is supported, so at most one wildcard."""
+            if self.ends_triple_dot:
+                dotless = self.path[:-3]
+                if test_path.startswith(dotless):
+                    wildcard = test_path[len(dotless):]
+                    return (True, [ wildcard ])
+            else:
+                if test_path == self.path:
+                    return (True, [])
+            return (False, [])
+
+        def match(self, test_path):
+            """Just return if it matches; don't bother with the wildcards."""
+            b, _ = self.match_wildcards(test_path)
+            return b
+
+        def fill_in_wildcards(self, wildcards):
+            """Return the relative path, with the wildcards filled in
+               if there are any."""
+            if self.ends_triple_dot:
+                return self.path[:-3] + wildcards[0]
+            else:
+                return self.path
+
+    class Mapping(object):
+        def __init__(self, depot_side, client_side, overlay, exclude):
+            # depot_side is without the trailing /... if it had one
+            self.depot_side = View.Path(depot_side, is_depot=True)
+            self.client_side = View.Path(client_side, is_depot=False)
+            self.overlay = overlay  # started with "+"
+            self.exclude = exclude  # started with "-"
+            assert not (self.overlay and self.exclude)
+            self.depot_side.ensure_compatible(self.client_side)
+
+        def __str__(self):
+            c = " "
+            if self.overlay:
+                c = "+"
+            if self.exclude:
+                c = "-"
+            return "View.Mapping: %s%s -> %s" % \
+                   (c, self.depot_side, self.client_side)
+
+        def map_depot_to_client(self, depot_path):
+            """Calculate the client path if using this mapping on the
+               given depot path; does not consider the effect of other
+               mappings in a view.  Even excluded mappings are returned."""
+            matches, wildcards = self.depot_side.match_wildcards(depot_path)
+            if not matches:
+                return ""
+            client_path = self.client_side.fill_in_wildcards(wildcards)
+            return client_path
+
+    #
+    # View methods
+    #
+    def __init__(self):
+        self.mappings = []
+
+    def append(self, view_line):
+        """Parse a view line, splitting it into depot and client
+           sides.  Append to self.mappings, preserving order."""
+
+        # Split the view line into exactly two words.  P4 enforces
+        # structure on these lines that simplifies this quite a bit.
+        #
+        # Either or both words may be double-quoted.
+        # Single quotes do not matter.
+        # Double-quote marks cannot occur inside the words.
+        # A + or - prefix is also inside the quotes.
+        # There are no quotes unless they contain a space.
+        # The line is already white-space stripped.
+        # The two words are separated by a single space.
+        #
+        if view_line[0] == '"':
+            # First word is double quoted.  Find its end.
+            close_quote_index = view_line.find('"', 1)
+            if close_quote_index <= 0:
+                die("No first-word closing quote found: %s" % view_line)
+            depot_side = view_line[1:close_quote_index]
+            # skip closing quote and space
+            rhs_index = close_quote_index + 1 + 1
+        else:
+            space_index = view_line.find(" ")
+            if space_index <= 0:
+                die("No word-splitting space found: %s" % view_line)
+            depot_side = view_line[0:space_index]
+            rhs_index = space_index + 1
+
+        if view_line[rhs_index] == '"':
+            # Second word is double quoted.  Make sure there is a
+            # double quote at the end too.
+            if not view_line.endswith('"'):
+                die("View line with rhs quote should end with one: %s" %
+                    view_line)
+            # skip the quotes
+            client_side = view_line[rhs_index+1:-1]
+        else:
+            client_side = view_line[rhs_index:]
+
+        # prefix + means overlay on previous mapping
+        overlay = False
+        if depot_side.startswith("+"):
+            overlay = True
+            depot_side = depot_side[1:]
+
+        # prefix - means exclude this path
+        exclude = False
+        if depot_side.startswith("-"):
+            exclude = True
+            depot_side = depot_side[1:]
+
+        m = View.Mapping(depot_side, client_side, overlay, exclude)
+        self.mappings.append(m)
+
+    def map_in_client(self, depot_path):
+        """Return the relative location in the client where this
+           depot file should live.  Returns "" if the file should
+           not be mapped in the client."""
+
+        paths_filled = []
+        client_path = ""
+
+        # look at later entries first
+        for m in self.mappings[::-1]:
+
+            # see where will this path end up in the client
+            p = m.map_depot_to_client(depot_path)
+
+            if p == "":
+                # Depot path does not belong in client.  Must remember
+                # this, as previous items should not cause files to
+                # exist in this path either.  Remember that the list is
+                # being walked from the end, which has higher precedence.
+                # Overlap mappings do not exclude previous mappings.
+                if not m.overlay:
+                    paths_filled.append(m.client_side)
+
+            else:
+                # This mapping matched; no need to search any further.
+                # But, the mapping could be rejected if the client path
+                # has already been claimed by an earlier mapping.
+                already_mapped_in_client = False
+                for f in paths_filled:
+                    # this is View.Path.match
+                    if f.match(p):
+                        already_mapped_in_client = True
+                        break
+                if not already_mapped_in_client:
+                    # Include this file, unless it is from a line that
+                    # explicitly said to exclude it.
+                    if not m.exclude:
+                        client_path = p
+
+                # a match, even if rejected, always stops the search
+                break
+
+        return client_path
+
 class P4Sync(Command, P4UserMap):
     delete_actions = ( "delete", "move/delete", "purge" )
 
@@ -1216,8 +1428,7 @@ class P4Sync(Command, P4UserMap):
         self.p4BranchesInGit = []
         self.cloneExclude = []
         self.useClientSpec = False
-        self.clientSpecDirs = []
-        self.haveSingleFileClientViews = False
+        self.clientSpecDirs = None
 
         if gitConfig("git-p4.syncFromOrigin") == "false":
             self.syncWithOrigin = False
@@ -1268,30 +1479,7 @@ class P4Sync(Command, P4UserMap):
 
     def stripRepoPath(self, path, prefixes):
         if self.useClientSpec:
-
-            # if using the client spec, we use the output directory
-            # specified in the client.  For example, a view
-            #   //depot/foo/branch/... //client/branch/foo/...
-            # will end up putting all foo/branch files into
-            #  branch/foo/
-            for val in self.clientSpecDirs:
-                if self.haveSingleFileClientViews and len(path) == abs(val[1][0]) and path == val[0]:
-                    # since 'path' is a depot file path, if it matches the LeftMap,
-                    # then the View is a single file mapping, so use the entire rightMap
-                    # first two tests above avoid the last == test for common cases
-                    path =  val[1][1]
-                    # now strip out the client (//client/...)
-                    path = re.sub("^(//[^/]+/)", '', path)
-                    # the rest is local client path
-                    return path
-
-                if path.startswith(val[0]):
-                    # replace the depot path with the client path
-                    path = path.replace(val[0], val[1][1])
-                    # now strip out the client (//client/...)
-                    path = re.sub("^(//[^/]+/)", '', path)
-                    # the rest is all path
-                    return path
+            return self.clientSpecDirs.map_in_client(path)
 
         if self.keepRepoPath:
             prefixes = [re.sub("^(//[^/]+/).*", r'\1', prefixes[0])]
@@ -1441,19 +1629,17 @@ class P4Sync(Command, P4UserMap):
         filesToDelete = []
 
         for f in files:
-            includeFile = True
-            for val in self.clientSpecDirs:
-                if f['path'].startswith(val[0]):
-                    if val[1][0] <= 0:
-                        includeFile = False
-                    break
+            # if using a client spec, only add the files that have
+            # a path in the client
+            if self.clientSpecDirs:
+                if self.clientSpecDirs.map_in_client(f['path']) == "":
+                    continue
 
-            if includeFile:
-                filesForCommit.append(f)
-                if f['action'] in self.delete_actions:
-                    filesToDelete.append(f)
-                else:
-                    filesToRead.append(f)
+            filesForCommit.append(f)
+            if f['action'] in self.delete_actions:
+                filesToDelete.append(f)
+            else:
+                filesToRead.append(f)
 
         # deleted files...
         for f in filesToDelete:
@@ -1892,60 +2078,31 @@ class P4Sync(Command, P4UserMap):
 
 
     def getClientSpec(self):
-        specList = p4CmdList( "client -o" )
-        temp = {}
-        for entry in specList:
-            for k,v in entry.iteritems():
-                if k.startswith("View"):
-
-                    # p4 has these %%1 to %%9 arguments in specs to
-                    # reorder paths; which we can't handle (yet :)
-                    if re.search('%%\d', v) != None:
-                        print "Sorry, can't handle %%n arguments in client specs"
-                        sys.exit(1)
-                    if re.search('\*', v) != None:
-                        print "Sorry, can't handle * mappings in client specs"
-                        sys.exit(1)
-
-                    if v.startswith('"'):
-                        start = 1
-                    else:
-                        start = 0
-                    index = v.find("...")
-
-                    # save the "client view"; i.e the RHS of the view
-                    # line that tells the client where to put the
-                    # files for this view.
-
-                    # check for individual file mappings - those which have no '...'
-                    if  index < 0 :
-                        v,cv = v.strip().split()
-                        v = v[start:]
-                        self.haveSingleFileClientViews = True
-                    else:
-                        cv = v[index+3:].strip() # +3 to remove previous '...'
-                        cv=cv[:-3]
-                        v = v[start:index]
-
-                    # now save the view; +index means included, -index
-                    # means it should be filtered out.
-                    if v.startswith("-"):
-                        v = v[1:]
-                        include = -len(v)
-                    else:
-                        include = len(v)
+        specList = p4CmdList("client -o")
+        if len(specList) != 1:
+            die('Output from "client -o" is %d lines, expecting 1' %
+                len(specList))
+
+        # dictionary of all client parameters
+        entry = specList[0]
+
+        # just the keys that start with "View"
+        view_keys = [ k for k in entry.keys() if k.startswith("View") ]
+
+        # hold this new View
+        view = View()
 
-                    # store the View #number for sorting
-                    # and the View string itself (this last for documentation)
-                    temp[v] = (include, cv, int(k[4:]),k)
+        # append the lines, in order, to the view
+        for view_num in range(len(view_keys)):
+            k = "View%d" % view_num
+            if k not in view_keys:
+                die("Expected view key %s missing" % k)
+            view.append(entry[k])
 
-        self.clientSpecDirs = temp.items()
-        # Perforce ViewNN with higher #numbers override those with lower
-        # reverse sort on the View #number
-        self.clientSpecDirs.sort( lambda x, y:  y[1][2] -  x[1][2]  )
+        self.clientSpecDirs = view
         if self.verbose:
-            for val in self.clientSpecDirs:
-                    print "clientSpecDirs: %s %s" % (val[0],val[1])
+            for i, m in enumerate(self.clientSpecDirs.mappings):
+                    print "clientSpecDirs %d: %s" % (i, str(m))
 
     def run(self, args):
         self.depotPaths = []
diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
index 1cc83c5..c9471d5 100755
--- a/t/t9809-git-p4-client-view.sh
+++ b/t/t9809-git-p4-client-view.sh
@@ -107,7 +107,7 @@ test_expect_success 'wildcard ... only supported at end of spec' '
 	test_must_fail "$GITP4" clone --use-client-spec --dest="$git" //depot
 '
 
-test_expect_failure 'basic map' '
+test_expect_success 'basic map' '
 	client_view "//depot/dir1/... //client/cli1/..." &&
 	files="cli1/file11 cli1/file12" &&
 	client_verify $files &&
@@ -116,7 +116,7 @@ test_expect_failure 'basic map' '
 	git_verify $files
 '
 
-test_expect_failure 'client view with no mappings' '
+test_expect_success 'client view with no mappings' '
 	client_view &&
 	client_verify &&
 	test_when_finished cleanup_git &&
@@ -124,7 +124,7 @@ test_expect_failure 'client view with no mappings' '
 	git_verify
 '
 
-test_expect_failure 'single file map' '
+test_expect_success 'single file map' '
 	client_view "//depot/dir1/file11 //client/file11" &&
 	files="file11" &&
 	client_verify $files &&
@@ -144,7 +144,7 @@ test_expect_success 'later mapping takes precedence (entire repo)' '
 	git_verify $files
 '
 
-test_expect_failure 'later mapping takes precedence (partial repo)' '
+test_expect_success 'later mapping takes precedence (partial repo)' '
 	client_view "//depot/dir1/... //client/..." \
 		    "//depot/dir2/... //client/..." &&
 	files="file21 file22" &&
@@ -159,7 +159,7 @@ test_expect_failure 'later mapping takes precedence (partial repo)' '
 #   dir1 cannot go to cli12 since it was filled by dir2
 #   dir1 also does not go to cli3, since the second rule
 #     noticed that it matched, but was already filled
-test_expect_failure 'depot path matching rejected client path' '
+test_expect_success 'depot path matching rejected client path' '
 	client_view "//depot/dir1/... //client/cli3/..." \
 		    "//depot/dir1/... //client/cli12/..." \
 		    "//depot/dir2/... //client/cli12/..." &&
@@ -172,7 +172,7 @@ test_expect_failure 'depot path matching rejected client path' '
 
 # since both have the same //client/..., the exclusion
 # rule keeps everything out
-test_expect_failure 'exclusion wildcard, client rhs same (odd)' '
+test_expect_success 'exclusion wildcard, client rhs same (odd)' '
 	client_view "//depot/... //client/..." \
 		    "-//depot/dir2/... //client/..." &&
 	client_verify &&
@@ -201,7 +201,7 @@ test_expect_success 'exclusion single file' '
 	git_verify $files
 '
 
-test_expect_failure 'overlay wildcard' '
+test_expect_success 'overlay wildcard' '
 	client_view "//depot/dir1/... //client/cli/..." \
 		    "+//depot/dir2/... //client/cli/...\n" &&
 	files="cli/file11 cli/file12 cli/file21 cli/file22" &&
@@ -211,7 +211,7 @@ test_expect_failure 'overlay wildcard' '
 	git_verify $files
 '
 
-test_expect_failure 'overlay single file' '
+test_expect_success 'overlay single file' '
 	client_view "//depot/dir1/... //client/cli/..." \
 		    "+//depot/dir2/file21 //client/cli/file21" &&
 	files="cli/file11 cli/file12 cli/file21" &&
@@ -221,7 +221,7 @@ test_expect_failure 'overlay single file' '
 	git_verify $files
 '
 
-test_expect_failure 'exclusion with later inclusion' '
+test_expect_success 'exclusion with later inclusion' '
 	client_view "//depot/... //client/..." \
 		    "-//depot/dir2/... //client/dir2/..." \
 		    "//depot/dir2/... //client/dir2incl/..." &&
@@ -232,7 +232,7 @@ test_expect_failure 'exclusion with later inclusion' '
 	git_verify $files
 '
 
-test_expect_failure 'quotes on rhs only' '
+test_expect_success 'quotes on rhs only' '
 	client_view "//depot/dir1/... \"//client/cdir 1/...\"" &&
 	client_verify "cdir 1/file11" "cdir 1/file12" &&
 	test_when_finished cleanup_git &&
@@ -266,7 +266,7 @@ test_expect_success 'rename files to introduce spaces' '
 		      "dir 2/file21" "dir 2/file22"
 '
 
-test_expect_failure 'quotes on lhs only' '
+test_expect_success 'quotes on lhs only' '
 	client_view "\"//depot/dir 1/...\" //client/cdir1/..." &&
 	files="cdir1/file11 cdir1/file12" &&
 	client_verify $files &&
@@ -275,7 +275,7 @@ test_expect_failure 'quotes on lhs only' '
 	client_verify $files
 '
 
-test_expect_failure 'quotes on both sides' '
+test_expect_success 'quotes on both sides' '
 	client_view "\"//depot/dir 1/...\" \"//client/cdir 1/...\"" &&
 	client_verify "cdir 1/file11" "cdir 1/file12" &&
 	test_when_finished cleanup_git &&
-- 
1.7.8.1.407.gd70cb

^ permalink raw reply related

* [PATCH 4/6] git-p4: support single file p4 client view maps
From: Pete Wyckoff @ 2012-01-02 23:05 UTC (permalink / raw)
  To: git; +Cc: Gary Gibbons
In-Reply-To: <1325545554-16540-1-git-send-email-pw@padd.com>

From: Gary Gibbons <ggibbons@perforce.com>

Perforce client views can map individual files,
mapping one //depot file path to one //client file path.
These mappings contain no meta/masking characters.
This patch add support for these file maps to
the currently supported '...' view mappings.

[pw: one test now suceeds]

Signed-off-by: Gary Gibbons <ggibbons@perforce.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4    |   29 ++++++++++++++++++++---------
 t/t9809-git-p4-client-view.sh |    2 +-
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 2fd4d3b..8f28e0a 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1217,6 +1217,7 @@ class P4Sync(Command, P4UserMap):
         self.cloneExclude = []
         self.useClientSpec = False
         self.clientSpecDirs = []
+        self.haveSingleFileClientViews = False
 
         if gitConfig("git-p4.syncFromOrigin") == "false":
             self.syncWithOrigin = False
@@ -1274,6 +1275,16 @@ class P4Sync(Command, P4UserMap):
             # will end up putting all foo/branch files into
             #  branch/foo/
             for val in self.clientSpecDirs:
+                if self.haveSingleFileClientViews and len(path) == abs(val[1][0]) and path == val[0]:
+                    # since 'path' is a depot file path, if it matches the LeftMap,
+                    # then the View is a single file mapping, so use the entire rightMap
+                    # first two tests above avoid the last == test for common cases
+                    path =  val[1][1]
+                    # now strip out the client (//client/...)
+                    path = re.sub("^(//[^/]+/)", '', path)
+                    # the rest is local client path
+                    return path
+
                 if path.startswith(val[0]):
                     # replace the depot path with the client path
                     path = path.replace(val[0], val[1][1])
@@ -1905,19 +1916,19 @@ class P4Sync(Command, P4UserMap):
                     # save the "client view"; i.e the RHS of the view
                     # line that tells the client where to put the
                     # files for this view.
-                    cv = v[index+3:].strip() # +3 to remove previous '...'
 
-                    # if the client view doesn't end with a
-                    # ... wildcard, then we're going to mess up the
-                    # output directory, so fail gracefully.
-                    if not cv.endswith('...'):
-                        print 'Sorry, client view in "%s" needs to end with wildcard' % (k)
-                        sys.exit(1)
-                    cv=cv[:-3]
+                    # check for individual file mappings - those which have no '...'
+                    if  index < 0 :
+                        v,cv = v.strip().split()
+                        v = v[start:]
+                        self.haveSingleFileClientViews = True
+                    else:
+                        cv = v[index+3:].strip() # +3 to remove previous '...'
+                        cv=cv[:-3]
+                        v = v[start:index]
 
                     # now save the view; +index means included, -index
                     # means it should be filtered out.
-                    v = v[start:index]
                     if v.startswith("-"):
                         v = v[1:]
                         include = -len(v)
diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
index 06652cb..1cc83c5 100755
--- a/t/t9809-git-p4-client-view.sh
+++ b/t/t9809-git-p4-client-view.sh
@@ -191,7 +191,7 @@ test_expect_success 'exclusion wildcard, client rhs different (normal)' '
 	git_verify $files
 '
 
-test_expect_failure 'exclusion single file' '
+test_expect_success 'exclusion single file' '
 	client_view "//depot/... //client/..." \
 		    "-//depot/dir2/file22 //client/file22" &&
 	files="dir1/file11 dir1/file12 dir2/file21" &&
-- 
1.7.8.1.407.gd70cb

^ permalink raw reply related

* [PATCH 3/6] git-p4: sort client views by reverse View number
From: Pete Wyckoff @ 2012-01-02 23:05 UTC (permalink / raw)
  To: git; +Cc: Gary Gibbons
In-Reply-To: <1325545554-16540-1-git-send-email-pw@padd.com>

From: Gary Gibbons <ggibbons@perforce.com>

Correct view sorting to support the Perforce order,
where client views are ordered and later views
override earlier view mappings.

[pw: one test now succeeds]

Signed-off-by: Gary Gibbons <ggibbons@perforce.com>
Signed-off-by: Pete Wyckoff <pw@padd.com>
---
 contrib/fast-import/git-p4    |   11 +++++++++--
 t/t9809-git-p4-client-view.sh |    2 +-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index c144c89..2fd4d3b 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1924,10 +1924,17 @@ class P4Sync(Command, P4UserMap):
                     else:
                         include = len(v)
 
-                    temp[v] = (include, cv)
+                    # store the View #number for sorting
+                    # and the View string itself (this last for documentation)
+                    temp[v] = (include, cv, int(k[4:]),k)
 
         self.clientSpecDirs = temp.items()
-        self.clientSpecDirs.sort( lambda x, y: abs( y[1][0] ) - abs( x[1][0] ) )
+        # Perforce ViewNN with higher #numbers override those with lower
+        # reverse sort on the View #number
+        self.clientSpecDirs.sort( lambda x, y:  y[1][2] -  x[1][2]  )
+        if self.verbose:
+            for val in self.clientSpecDirs:
+                    print "clientSpecDirs: %s %s" % (val[0],val[1])
 
     def run(self, args):
         self.depotPaths = []
diff --git a/t/t9809-git-p4-client-view.sh b/t/t9809-git-p4-client-view.sh
index cbf2213..06652cb 100755
--- a/t/t9809-git-p4-client-view.sh
+++ b/t/t9809-git-p4-client-view.sh
@@ -133,7 +133,7 @@ test_expect_failure 'single file map' '
 	git_verify $files
 '
 
-test_expect_failure 'later mapping takes precedence (entire repo)' '
+test_expect_success 'later mapping takes precedence (entire repo)' '
 	client_view "//depot/dir1/... //client/cli1/..." \
 		    "//depot/... //client/cli2/..." &&
 	files="cli2/dir1/file11 cli2/dir1/file12
-- 
1.7.8.1.407.gd70cb

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox