On 24 Aug 2015, at 08:33, Junio C Hamano <gits...@pobox.com> wrote: > Lars Schneider <larsxschnei...@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! -- 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
