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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 21, 2015, 18:01 UTC
Message-ID: <xmqqmvxk5xm9.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1440177578-5317-1-git-send-email-larsxschneider@gmail.com>

larsxschneider@gmail.com writes:

Show 31 quoted lines

> 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 ;-)

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

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.

Show 10 quoted lines

> 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?

Show 13 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

> +		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.

Show 6 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.

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

Thanks.