On 21 Aug 2015, at 20:01, Junio C Hamano <gitster@xxxxxxxxx> wrote: > larsxschneider@xxxxxxxxx writes: > >> From: Lars Schneider <larsxschneider@xxxxxxxxx> >> >> 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@xxxxxxxxx> >> --- > > 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? > > 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? > >> 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 > >> + 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. > >> +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. > >> + >One/two/File0.txt && >> + p4 add One/two/File0.txt && > > Thanks. Thanks again for the feedback, Lars -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html