to combine commits, or not to combine commits. That is the question.

[Tom Pittenger]

I have a question about cleaning up commit history before a pull request. The dev wiki has contradicting statements. I'll break the main ones I'm asking about into two rules.

Rule "1" is designed to make reviewing easier by making "obviously correct" incremental commits. Also helps in playing musical commits by the push gods:

"Commits should be small, and do just one thing. If a change touches multiple libraries then there should be a separate commit per library, and a separate commit per vehicle directory. This is true even if it means that intermediate commits break the build."

Rule "2" is designed to make reviewing easier and to not clutter the repo history for everyone else:

"Before submitting code to the official repository, clean up your local commit history. When submitting patches the convention is to use an interactive rebase (eg. “git rebase -i HEAD~10”) to re-arrange patches and fold things together, so the patch set it “cleaned up” for submission"

Those are often contradicting rules. When I see a pull-request like this one with 15 commits to AP_HAL_Linux that obviously breaks rule 2 but makes rule 1 happy. And in that same PPR there there are "Global" commits that roll many commits into one that have a single similar line in multiple folders that breaks rule 1 but makes rule 2 happy.

I've always focused on rule 2 and rolled my commits together regardless of how easy it makes the review. Perhaps that is a bad rule to follow and I should break rule 2 occasionally to satisfy rule 1 for the greater good. I could easily just commit tiny things and get ranked here super fast. If that helps with reviewing then that is in everyone's best interest, right?

Am I being overly critical or is there a better set of rules we can come up with?

-TomP

[Craig Elder]

Good point Tom.  Added to the dev call agenda for discussion tomorrow

--
You received this message because you are subscribed to the Google Groups "drones-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to drones-discus...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Lucas De Marchi]

Tom, I think your interpretation of the second rule is the problem.

We can start by thinking the "end goal of these rules" is for easy of review an sane commit history. Rule 2 states that you should clean up your intermediary history while you were developing. In other words, there's no point in submit lots of changes in the first commits that later is reverted by subsequent commits. This is not the case in this PR: changes are ordered logically, much in agreement with the first rule.

Additionally the second rule does *not* mean you should squash  every thing into one commit per directory. Alas one commit per directory is the absolute minimum you are required to (unless the commit falls in the simple search and replace or very trivial changes, in which we have the convention to use the "Global:" prefix in the commit). if you go beyond that and split in logical pieces for easy of review, git history, "revertability", etc , it's even better. Btw if you were only aiming to split into one commit per directory you wouldn't even need to split as the script in gittools directory could do that automatically.

So, defending myself,  I don't think it really breaks any rule.

Lucas De Marchi

[Tom Pittenger]

I did not mean to use your PR as an attack, just an example because it crossed the lines and speaker nicely to both rules. No offense was meant.

[Lucas De Marchi]

On Sat, Feb 6, 2016 at 9:08 PM, Tom Pittenger <t...@airphrame.com> wrote:
> I did not mean to use your PR as an attack, just an example because it
> crossed the lines and speaker nicely to both rules. No offense was meant.

No offense taken. Just wanted to clarify why the PR is the way it is
and contribute with answering your question.

Lucas De Marchi

[Tom Pittenger]

>>crossed the lines and speaker nicely to both rules

I need to watch out for typing emails on my phone, auto-correct keeps killing me!