Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-1181

Adopt guidelines on git commit best practices

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: SAT
    • Labels:
      None
    • Team:
      Architecture

      Description

      The question of what makes a "good commit" arises occasionally. I feel that the following document:

      https://wiki.openstack.org/wiki/GitCommitMessages#Structural_split_of_changes

      presents a decent guideline. Pasting from it:

      ================================
      Structural split of changes
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~
      The cardinal rule for creating good commits is to ensure there is only
      one "logical change" per commit. There are many reasons why this is an
      important rule:

      • The smaller the amount of code being changed, the quicker & easier it
        is to review & identify potential flaws.
      • If a change is found to be flawed later, it may be necessary to revert
        the broken commit. This is much easier to do if there are not other
        unrelated code changes entangled with the original commit.
      • When troubleshooting problems using GIT's bisect capability, small
        well defined changes will aid in isolating exactly where the code
        problem was introduced.
      • When browsing history using GIT annotate/blame, small well defined
        changes also aid in isolating exactly where & why a piece of code came from.

      Things to avoid when creating commits
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      With that in mind, there are some commonly encountered examples of bad
      things to avoid

      • Mixing whitespace changes with functional code changes.
        The whitespace changes will obscure the important functional changes,
        making it harder for a reviewer to correctly determine whether the
        change is correct. Solution: Create 2 commits, one with the whitespace
        changes, one with the functional changes. Typically the whitespace
        change would be done first, but that need not be a hard rule.
      • Mixing two unrelated functional changes.
        Again the reviewer will find it harder to identify flaws if two
        unrelated changes are mixed together. If it becomes necessary to later
        revert a broken commit, the two unrelated changes will need to be
        untangled, with further risk of bug creation.
      • Sending large new features in a single giant commit.
        It may well be the case that the code for a new feature is only useful
        when all of it is present. This does not, however, imply that the entire
        feature should be provided in a single commit. New features often entail
        refactoring existing code. It is highly desirable that any refactoring
        is done in commits which are separate from those implementing the new
        feature. This helps reviewers and test suites validate that the
        refactoring has no unintentional functional changes. Even the newly
        written code can often be split up into multiple pieces that can be
        independently reviewed. For example, changes which add new internal
        APIs/classes, can be in self-contained commits. Again this leads to
        easier code review. It also allows other developers to cherry-pick small
        parts of the work, if the entire new feature is not immediately ready
        for merge. Addition of new public APIs or RPC interfaces should be done
        in commits separate from the actual internal implementation. This will
        encourage the author & reviewers to think about the generic API/RPC
        design, and not simply pick a design that is easier for their currently
        chosen internal implementation.

      The basic rule to follow is:

      If a code change can be split into a sequence of patches/commits,
      then it should be split. Less is not more. More is more.
      ================================

      In light of increasingly allowing/encouraging rebasing to happen, I
      think we should set an expectation of what the commits should look like.
      I'd motion the SAT to put this onto the agenda as an addition to our
      coding standards.

        Attachments

          Issue Links

            Activity

            Hide
            shaw Richard Shaw [X] (Inactive) added a comment -

            The referenced document contains a lot of arguably useful content/advice, a small portion of which was excerpted in the description of this ticket. So did the SAT recommend the adoption of what's in the description, or the whole enchilada?

            Also, the OpenStack document has more the flavor of "best practices" rather than rules of law. Seems like that ought to be reflected in the Developer Guide (as a new heading, rather than an augmentation of the "Standards").

            Show
            shaw Richard Shaw [X] (Inactive) added a comment - The referenced document contains a lot of arguably useful content/advice, a small portion of which was excerpted in the description of this ticket. So did the SAT recommend the adoption of what's in the description, or the whole enchilada? Also, the OpenStack document has more the flavor of "best practices" rather than rules of law. Seems like that ought to be reflected in the Developer Guide (as a new heading, rather than an augmentation of the "Standards").
            Hide
            ktl Kian-Tat Lim added a comment -

            Generally, we should adopt community best practices and follow them. The referenced document happens to be a fairly good exposition of those practices, but we should adjust as necessary. Since these are best practices, they would be "SHOULD"-type standards if added there.

            Show
            ktl Kian-Tat Lim added a comment - Generally, we should adopt community best practices and follow them. The referenced document happens to be a fairly good exposition of those practices, but we should adjust as necessary. Since these are best practices, they would be "SHOULD"-type standards if added there.
            Hide
            shaw Richard Shaw [X] (Inactive) added a comment -

            The excerpted content has been added to the DM Developer Guide: see Git Commit Best Practices.

            Show
            shaw Richard Shaw [X] (Inactive) added a comment - The excerpted content has been added to the DM Developer Guide : see Git Commit Best Practices .
            Hide
            mjuric Mario Juric added a comment -

            Dick, thanks & sorry for the delay – I didn't realize I was a reviewer on this one.

            I made some changes to the document, to streamline it and minimize verbatim quoting of the external document. I also added the info for DM-1182.

            In general, when writing these documents I'd make them as terse as possible to a) minimize future maintenance effort, but also b) to make it easy to quickly refresh one's memory with just the important bits.

            Assuming these changes are OK with everyone, I move to close this issue.

            Show
            mjuric Mario Juric added a comment - Dick, thanks & sorry for the delay – I didn't realize I was a reviewer on this one. I made some changes to the document, to streamline it and minimize verbatim quoting of the external document. I also added the info for DM-1182 . In general, when writing these documents I'd make them as terse as possible to a) minimize future maintenance effort, but also b) to make it easy to quickly refresh one's memory with just the important bits. Assuming these changes are OK with everyone, I move to close this issue.
            Hide
            shaw Richard Shaw [X] (Inactive) added a comment -

            Mario,

            Your changes are excellent, thanks. Closing the issue.

            -Dick

            Show
            shaw Richard Shaw [X] (Inactive) added a comment - Mario, Your changes are excellent, thanks. Closing the issue. -Dick

              People

              Assignee:
              ktl Kian-Tat Lim
              Reporter:
              mjuric Mario Juric
              Reviewers:
              Jeff Kantor, Mario Juric, Robyn Allsman [X] (Inactive)
              Watchers:
              Jeff Kantor, Kian-Tat Lim, Mario Juric, Paul Price, Richard Shaw [X] (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  CI Builds

                  No builds found.