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

[Lars Schneider <larsxschneider@gmail.com>]

On 24 Aug 2015, at 08:33, Junio C Hamano <gitster@pobox.com> wrote:

> Lars Schneider <larsxschneider@gmail.com> 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!