Lars Schneider <larsxschneider@xxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html