Re: [PATCH v4] git-p4: fix faulty paths for case insensitive systems
On 24 August 2015 at 13:43, Lars Schneider wrote: >> >> https://github.com/luked99/quick-git-p4-case-folding-test > As mentioned I realized that the problem occurs only if you use client specs. > Can you take a look at this test case / run it? > https://github.com/larsxschneider/git/blob/d3a191cb5fb4d8f5f48ca9314c772169d5dbf65b/t/t9821-git-p4-path-variations.sh#L112-L127 > > Does this make sense to you? If you want to I could also modify > “quick-git-p4-case-folding-test” to show the problem. If you're able to fix my hacky test to show the problem that would be very kind. If the problem only shows up when using client specs, is it possible that the core.ignorecase logic is just missing a code path somewhere? Glancing through the code, stripRepoPath() could perhaps be the culprit? If self.useClientSpec is FALSE, it will do the core.ignorecase trick by calling p4PathStartsWith, but if it is TRUE, it won't. Thanks! Luke -- 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 v4] git-p4: fix faulty paths for case insensitive systems
> > > >> >> While I was working on the examples for this email reply I realized that the >> problem is only present for paths given in a client-spec. I added a test >> case to prove that. That means I don’t need to request *all* paths. I >> *think* it is sufficient to request the paths mentioned in the client spec >> which would usually be really fast. Stay tuned for PATCH v5. > > I've just tried a small experiment with stock unaltered git-p4. > > - Started p4d with -C1 option to case-fold. > - Add some files with different cases of directory (Path/File1, > PATH/File2, pATH/File3). > - git-p4 clone. > > The result of the clone is separate directories if I do nothing > special (PATH/File1, Path/File2, etc). But if I set core.ignorecase, I > get a single case-folded directory, Path/File1, Path/File2, etc. I'm > still failing to get how that isn't what you need (other than being a > bit obscure to get to the right invocation). > > I've put a script that shows this here: > > https://github.com/luked99/quick-git-p4-case-folding-test As mentioned I realized that the problem occurs only if you use client specs. Can you take a look at this test case / run it? https://github.com/larsxschneider/git/blob/d3a191cb5fb4d8f5f48ca9314c772169d5dbf65b/t/t9821-git-p4-path-variations.sh#L112-L127 Does this make sense to you? If you want to I could also modify “quick-git-p4-case-folding-test” to show the problem. Cheers, Lars -- 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 v4] git-p4: fix faulty paths for case insensitive systems
On 24 August 2015 at 10:51, Lars Schneider wrote: >> >> I'm still trying to fully understand what's going on here - can you >> point out where I've got it wrong below please! > Your welcome + sure :) > > > While I was working on the examples for this email reply I realized that the > problem is only present for paths given in a client-spec. I added a test case > to prove that. That means I don’t need to request *all* paths. I *think* it > is sufficient to request the paths mentioned in the client spec which would > usually be really fast. Stay tuned for PATCH v5. I've just tried a small experiment with stock unaltered git-p4. - Started p4d with -C1 option to case-fold. - Add some files with different cases of directory (Path/File1, PATH/File2, pATH/File3). - git-p4 clone. The result of the clone is separate directories if I do nothing special (PATH/File1, Path/File2, etc). But if I set core.ignorecase, I get a single case-folded directory, Path/File1, Path/File2, etc. I'm still failing to get how that isn't what you need (other than being a bit obscure to get to the right invocation). I've put a script that shows this here: https://github.com/luked99/quick-git-p4-case-folding-test Thanks! Luke -- 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 v4] git-p4: fix faulty paths for case insensitive systems
> Lars - thanks for persisting with this! > > I'm still trying to fully understand what's going on here - can you > point out where I've got it wrong below please! Your welcome + sure :) > The server is on Linux, and is case-sensitive. For whatever reason > (probably people committing changes on Windows in the first place) We have many P4 servers. Some run on Linux and some on Windows. The Linux ones are executed with “-C1” flag to "Force the server to operate in case-insensitive mode on a normally case-sensitive platform.”. I did that in the test case, too. > we've ended up with a P4 repo that has differences in path case that > need to be ignored, with all upper-case paths being mapped to > lower-case. You are correct with “P4 repo that has differences in path case”. But it can be any path case variation. Not only all upper-case. I just realized that all my examples and tests show all upper-case. I will fix that. Consider these files in P4: //depot/Path/File1 //depot/PATH/File2 //depot/pATH/File3 P4 would sync them on a case insensitive filesystem to: $CLIENT_DIR/Path/File1 $CLIENT_DIR/Path/File2 $CLIENT_DIR/Path/File3 … and this is how they should end up in Git. > e.g. the *real* depot might be: > > //depot/path/File1 > //depot/PATH/File2 > > git-p4 clone should return this case-folded on Windows (and Linux?) > > $GIT_DIR/path/File1 > $GIT_DIR/path/File2 Correct. Although the correct path might be the following too: $GIT_DIR/PATH/File1 $GIT_DIR/PATH/File2 > (Am I right in thinking that this change folds the directory, but not > the filename?) Correct. > I don't really understand why a dictionary is required to do this > though - is there a reason we can't just lowercase all incoming paths? The result is not necessarily all lowercase. Even though the case doesn’t matter as we are talking about case-insensitve filesystems I want to use the case that P4 would pick (see example above). > Which is what the existing code in p4StartWith() is trying to do. That > code lowercases the *entire* path including the filename; this change > seems to leave the filename portion unchanged. I guess though that the > answer may be to do with your finding that P4 makes up the case of > directories based on lexicographic ordering - is that the basic > problem? Correct. > For a large repo, this approach is surely going to be really slow. True. But we use git-p4 as a one time operation to migrate our repos from P4 to Git. Therefore correctness is more important than speed. Plus it is not enabled by default. You need to pass a parameter switch to git-p4. > Additionally, could you update your patch to add some words to > Documentation/git-p4.txt please? I will do that! > On 21 August 2015 at 18:19, wrote: >> From: Lars Schneider >> >> 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. > > I'd like to think that the existing code that checks core.ignorecase > should handle this. I haven't tried it myself though. core.ignorecase doesn’t help. I added a test case to prove that. > > >> >> 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 >> --- >> git-p4.py | 82 ++-- >> t/t9821-git-p4-path-variations.sh | 109 >> ++ >> 2 files changed, 187 insertions(+), 4 deletions(-) >> create mode 100755 t/t9821-git-p4-path-variations.sh >> >> 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): >> """ Caching file paths by "p4 where" batch query """ >> >> # List depot file paths exclude that already cached >> @@ -1950,6 +1950,8 @@ class View(object): >> if "unmap" in res: >> # it will list all of them, but only one not unmap
Re: [PATCH v4] git-p4: fix faulty paths for case insensitive systems
On 24 Aug 2015, at 08:33, Junio C Hamano wrote: > Lars Schneider writes: > >>> - 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 cannot tell if you are joking or not, but just in case you are > serious, please check "git log" for recent history again. We do not > mark our paragraphs with noisy labels like "PROBLEM" and "SOLUTION", > regardless of case. Typically, our description outlines the current > status (which prepares the reader's mind to understand what you are > going to talk about), highlight what is problematic in that current > status, and then explains what change the patch does and justifies > why it is the right change, in this order. So those who read your > description can tell PROBLEM and SOLUTION apart without being told > with labels. I wasn’t joking. I got your point and I am going to change it. Sorry for the confusion. > >>> - 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? > > Often people do this with either > > (1) a cover letter for v4, that shows the "git diff" output to go > from the result of applying v3 to the result of applying v4 to > the same initial state; or > > (2) a textual description after three-dash line of v4 that explains > what has changed relative to v3. > > The latter is often done when the change between v3 and v4 is small > enough. Ok. Thanks! > >> 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 = ""): > > OK, just as I suspected. Then do not worry too much about it for > now, as fixes to existing style violations should be done outside of > this change, perhaps after the dust settles (or if you prefer, you > can do so as a preliminary clean-up patch, that does not change > anything but style, and then build your fix on top of it). > >> 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? > > If you are doing something other than style fixes (call that > "meaningful work"), it is strongly discouraged to fix existing style > violations in the same commit. If you are going to do meaningful > work on an otherwise dormant part of the system (you can judge it by > checking the recent history of the files you are going to touch, > e.g. "git log --no-merges pu -- git-p4.py"), you are encouraged to > first do the style fixes in separate patches as preliminary clean-ups > without changing anything else and then build your meaningful work > on top of it. > > What is discouraged is a change that tries to only do style fixes > etc. to parts of the system that are actively being modified by > other people for their meaningful work. Ok. Thanks for the explanation. > >>> 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. > > Thanks. I wanted to double-check that the problem you saw was not > about what is left in the filesystem but more about what is recorded > in the Git history. "ls-files" check is absolutely the right approach > in that case. Cool! -- 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 v4] git-p4: fix faulty paths for case insensitive systems
Lars Schneider writes: >> - 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 cannot tell if you are joking or not, but just in case you are serious, please check "git log" for recent history again. We do not mark our paragraphs with noisy labels like "PROBLEM" and "SOLUTION", regardless of case. Typically, our description outlines the current status (which prepares the reader's mind to understand what you are going to talk about), highlight what is problematic in that current status, and then explains what change the patch does and justifies why it is the right change, in this order. So those who read your description can tell PROBLEM and SOLUTION apart without being told with labels. >> - 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? Often people do this with either (1) a cover letter for v4, that shows the "git diff" output to go from the result of applying v3 to the result of applying v4 to the same initial state; or (2) a textual description after three-dash line of v4 that explains what has changed relative to v3. The latter is often done when the change between v3 and v4 is small enough. > 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 = ""): OK, just as I suspected. Then do not worry too much about it for now, as fixes to existing style violations should be done outside of this change, perhaps after the dust settles (or if you prefer, you can do so as a preliminary clean-up patch, that does not change anything but style, and then build your fix on top of it). > 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? If you are doing something other than style fixes (call that "meaningful work"), it is strongly discouraged to fix existing style violations in the same commit. If you are going to do meaningful work on an otherwise dormant part of the system (you can judge it by checking the recent history of the files you are going to touch, e.g. "git log --no-merges pu -- git-p4.py"), you are encouraged to first do the style fixes in separate patches as preliminary clean-ups without changing anything else and then build your meaningful work on top of it. What is discouraged is a change that tries to only do style fixes etc. to parts of the system that are actively being modified by other people for their meaningful work. >> 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. Thanks. I wanted to double-check that the problem you saw was not about what is left in the filesystem but more about what is recorded in the Git history. "ls-files" check is absolutely the right approach in that case. -- 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 v4] git-p4: fix faulty paths for case insensitive systems
On 21 Aug 2015, at 20:01, Junio C Hamano wrote: > larsxschnei...@gmail.com writes: > >> From: Lars Schneider >> >> 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 >> --- > > 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_e
Re: [PATCH v4] git-p4: fix faulty paths for case insensitive systems
Lars - thanks for persisting with this!
I'm still trying to fully understand what's going on here - can you
point out where I've got it wrong below please!
The server is on Linux, and is case-sensitive. For whatever reason
(probably people committing changes on Windows in the first place)
we've ended up with a P4 repo that has differences in path case that
need to be ignored, with all upper-case paths being mapped to
lower-case.
e.g. the *real* depot might be:
//depot/path/File1
//depot/PATH/File2
git-p4 clone should return this case-folded on Windows (and Linux?)
$GIT_DIR/path/File1
$GIT_DIR/path/File2
(Am I right in thinking that this change folds the directory, but not
the filename?)
I don't really understand why a dictionary is required to do this
though - is there a reason we can't just lowercase all incoming paths?
Which is what the existing code in p4StartWith() is trying to do. That
code lowercases the *entire* path including the filename; this change
seems to leave the filename portion unchanged. I guess though that the
answer may be to do with your finding that P4 makes up the case of
directories based on lexicographic ordering - is that the basic
problem?
For a large repo, this approach is surely going to be really slow.
Additionally, could you update your patch to add some words to
Documentation/git-p4.txt please?
A few other comments inline.
Thanks!
Luke
On 21 August 2015 at 18:19, wrote:
> From: Lars Schneider
>
> 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.
I'd like to think that the existing code that checks core.ignorecase
should handle this. I haven't tried it myself though.
>
> 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
> ---
> git-p4.py | 82 ++--
> t/t9821-git-p4-path-variations.sh | 109
> ++
> 2 files changed, 187 insertions(+), 4 deletions(-)
> create mode 100755 t/t9821-git-p4-path-variations.sh
>
> 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):
> """ Caching file paths by "p4 where" batch query """
>
> # List depot file paths exclude that already cached
> @@ -1950,6 +1950,8 @@ class View(object):
> if "unmap" in res:
> # it will list all of them, but only one not unmap-ped
> continue
> +if fixPathCase:
> +res['depotFile'] = fixPathCase(res['depotFile'])
> self.client_spec_path_cache[res['depotFile']] =
> self.convert_client_path(res["clientFile"])
>
> # not found files or unmap files set to ""
> @@ -1987,6 +1989,7 @@ class P4Sync(Command, P4UserMap):
> help="Maximum number of changes to
> import"),
> optparse.make_option("--changes-block-size",
> dest="changes_block_size", type="int",
> help="Internal block size to use when
> iteratively calling p4 changes"),
> +optparse.make_option("--ignore-path-case",
> dest="ignorePathCase", action="store_true"),
> optparse.make_option("--keep-path", dest="keepRepoPath",
> action='store_true',
> help="Keep entire BRANCH/DIR/SUBDIR
> prefix during import"),
> optparse.make_option("--use-client-spec",
> dest="useClientSpec", action='store_true',
> @@ -2017,6 +2020,7 @@ class P4Sync(Command, P4UserMap):
> self.maxChanges = ""
> self.changes_block_size = None
> self.keepRepoPath = False
> +self.ignorePathCase = False
> self.depotPaths = None
> self.p4BranchesInGit = []
> self.cloneExclude = []
> @@ -2049,7 +2053,8 @@ class P4Sync(Command, P4UserMap):
>
Re: [PATCH v4] git-p4: fix faulty paths for case insensitive systems
larsxschnei...@gmail.com writes: > From: Lars Schneider > > 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 > --- 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. > 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? > 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. > +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. -- 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
