Re: [PATCH v4] git-p4: fix faulty paths for case insensitive systems

From: Lars Schneider <larsxschneider@gmail.com>
Date: Aug 23, 2015, 17:10 UTC
Message-ID: <D3B14EF1-C005-43CE-A3FF-2ABEF879CA01@gmail.com>
In-Reply-To: <xmqqmvxk5xm9.fsf@gitster.dls.corp.google.com>

On 21 Aug 2015, at 20:01, Junio C Hamano <gitster@pobox.com> wrote:

Show 40 quoted lines

> larsxschneider@gmail.com writes:
> 
>> From: Lars Schneider <larsxschneider@gmail.com>
>> 
>> PROBLEM:
>> We run P4 servers on Linux and P4 clients on Windows. For an unknown
>> reason the file path for a number of files in P4 does not match the
>> directory path with respect to case sensitivity.
>> 
>> E.g. `p4 files` might return
>> //depot/path/to/file1
>> //depot/PATH/to/file2
>> 
>> If you use P4/P4V then these files end up in the same directory, e.g.
>> //depot/path/to/file1
>> //depot/path/to/file2
>> 
>> If you use git-p4 then all files not matching the correct file path
>> (e.g. `file2`) will be ignored.
>> 
>> SOLUTION:
>> Identify path names that are different with respect to case sensitivity.
>> If there are any then run `p4 dirs` to build up a dictionary
>> containing the "correct" cases for each path. It looks like P4
>> interprets "correct" here as the existing path of the first file in a
>> directory. The path dictionary is used later on to fix all paths.
>> 
>> This is only applied if the parameter "--ignore-path-case" is passed to
>> the git-p4 clone command.
>> 
>> 
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
> 
> Thanks.  A few comments.
> 
> - Have you checked "git log" on our history and notice how nobody
>   says "PROBLEM:" and "SOLUTION:" in capital letters?  Don't try to
>   be original in the form; your contributions are already original
>   and valuable in the substance ;-)

haha ok. I will make them lower case :-)

> 
> - I think I saw v3 yesterday.  It would be nice to see a brief
>   description of what has been updated in this version.

I discovered an optimization. In v3 I fixed the paths of *all* files that are underneath of a given P4 clone path. In v4 I fix only the paths that are visible on the client via client-spec (P4 can perform partial checkouts via “client-views”). I was wondering how to convey this change. Would have been a cover letter for v4 the correct way or should I have made another commit on top of my v3 change?

Show 19 quoted lines

> 
> I do not do Perforce and am not familiar enough with this code to
> judge myself.  Will wait for area experts (you have them CC'ed, which
> is good) to comment.
> 
>> diff --git a/git-p4.py b/git-p4.py
>> index 073f87b..61a587b 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -1931,7 +1931,7 @@ class View(object):
>>                 (self.client_prefix, clientFile))
>>         return clientFile[len(self.client_prefix):]
>> 
>> -    def update_client_spec_path_cache(self, files):
>> +    def update_client_spec_path_cache(self, files, fixPathCase = None):
> 
> I didn't check the remainder of the file, but I thought it is
> customery *not* to have spaces around '=' when the parameter is
> written with its default value in Python?

Yes, that is PEP-8 style and I will change it accordingly. Unfortunately git-p4.py does not follow a style guide. e.g. line 2369: def commit(self, details, files, branch, parent = ""):

More annoyingly (to me at least) is that git-p4 mixes CamelCase with snake_case even within classes/functions. I think I read somewhere that these kind of refactorings are discouraged. I assume that applies here, too?

Show 24 quoted lines

> 
>> diff --git a/t/t9821-git-p4-path-variations.sh b/t/t9821-git-p4-path-variations.sh
>> ...
>> +test_expect_success 'Clone the repo and WITHOUT path fixing' '
>> +	client_view "//depot/One/... //client/..." &&
>> +	git p4 clone --use-client-spec --destination="$git" //depot &&
>> +	test_when_finished cleanup_git &&
>> +	(
>> +		cd "$git" &&
>> +		# This method is used instead of "test -f" to ensure the case is
>> +		# checked even if the test is executed on case-insensitive file systems.
>> +		cat >expect <<-\EOF &&
>> +			two/File2.txt
>> +		EOF
> 
> I think we usually write the body of the indented here text
> (i.e. "<<-") indented to the same level as the command and
> delimiter, i.e.
> 
> 	cat >expect <<-\EOF &&
>        body of the here document line 1
>        body of the here document line 2
>        ...
>        EOF

ok

Show 17 quoted lines

> 
>> +		git ls-files >actual &&
>> +		test_cmp expect actual
>> +	)
>> +'
> 
> I think you used to check the result with "find .", i.e. what the
> filesystem actually recorded.  "ls-files" gives what the index
> records (i.e. to be committed if you were to make a new commit from
> that index).  They can be different in case on case-insensitive
> filesystems, which I think are the ones that are most problematic
> with the issue your patch is trying to address.
> 
> You are verifying what the set of "canonical" paths should be by
> checking ls-files output.  I think that is what you intended to do
> (i.e. I am saying "I think the code is more correct than the earlier
> round that used find"), but I just am double checking.

I agree that “ls-files” is better because it reflects what ends up in the Git repository and how it ends up there.

Show 12 quoted lines

> 
>> +test_expect_success 'Add a new file and clone the repo WITH path fixing' '
>> +	client_view "//depot/... //client/..." &&
>> +	(
>> +		cd "$cli" &&
>> +
>> +		mkdir -p One/two &&
> 
> A blank after 'cd' only in this one but not others.  A blank line is
> a good vehicle to convey that blocks of text above and below it are
> logically separate, but use it consistently.  I _think_ this blank
> line can go.

Agreed.

Show 5 quoted lines

> 
>> +		>One/two/File0.txt &&
>> +		p4 add One/two/File0.txt &&
> 
> Thanks.

Thanks again for the feedback, Lars