[arvados-dev] Brett's opinions on git workflow

Thu Apr 17 10:24:03 EDT 2014

I feel like git workflow is a subject that keeps coming up piecemeal.
Our current workflow just gave me a paper cut (I had to spend a few
minutes puzzling over the output of different git tools to reconcile
them), so if you'll indulge me, I'd like to take this one opportunity to
explain where I'm coming from. I promise I'll only do this once. If our
workflow stays as it is, I'm cool with that, but I feel like there's
room for improvement.

I was just reviewing a branch for Tom. Since this was my second pass on
it, I went to the git log to see what changed since my last pass. I've
attached the log graph output magit showed me. The complicated history
tree made it challenging to find all the relevant revisions. Tom
described making several changes that I didn't see at first, until I
found 1881da32 and friends near the bottom of the screen.

Our git history frequently looks like this, especially near the end of a
sprint, because when we're working on a feature branch, we merge its
parent (usually master) back into it so regularly. Isolating feature
work into a branch is great, but we lose some of the utility of having
that isolation, of having a single stream of commits about the feature,
when we merge the parent back in so often.

I like referring back to version control logs, probably more than most.
To me, it's documentation. It's that special kind of documentation that
you /must/ write /while/ you're doing the work. Understanding what
motivated a previous change often helps me do better when I'm working on
the next one. For example, if the log explains how a commit fixes a
subtle bug, I can avoid reintroducing that bug later on.

Because I think of it like documentation, I feel like it's worth putting
a little extra effort into making the history nice. Yes, improving our
workflow here might require more effort than our current merge-heavy
system. But like any effort put into documentation, I trust that it will
pay dividends in the future. I don't think it's merely putting effort
into git for the effort's sake.

There are at least two workflow changes we could make that would improve
our history-documentation:

 1. Just stop merging the parent branch back into its parent so
    regularly. |git diff master...| would be the canonical way to
    review. When the feature branch gets merged into master, the merge
    would be larger, so we'd have to put in additional effort to make
    sure that goes smoothly and doesn't introduce bugs.
 2. If we don't like big merges (very understandable!), where we
    currently merge the parent back into feature branches, instead we
    could rebase the feature branch on top of its parent. To do this,
    we'll have to let ourselves rewrite history on git.curoverse.com.
    This would require more discipline from us, but I feel like we
    already have that, since our rule is that a developer owns the
    branch they make. When you pull someone else's feature branch for
    review, you'd use |git pull -f| to get the rewritten history. Our
    process for reviewing and merging feature branches would remain the
    same, and those merges should be as straightforward as they are today.

There might be other options too, but I think these would require the
least disruption to our current workflow. Which is my goal, because I
generally like our current workflow. I just wish I could understand the
history better. :)

Thanks for reading,

-- 
Brett Smith
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.arvados.org/pipermail/arvados-dev/attachments/20140417/5616b154/attachment.html>
-------------- next part --------------
* c0deafe Fix errant use of symbol vs. string
* 86b7874 Fix unwarranted use of instance variable.
* d0fce65 Merge back-to-back condition blocks with the same condition.
*   2340ebe Merge branch 'master' into 2596-refactor-pipeline-create
|\  
| *   2f91702 Merge branch '2449-keep-index-status-handlers'
| |\  
| | * 51c1390 Check that specified Keep volumes actually exist.
| | *   b346f87 Merge branch 'master' into 2449-keep-index-status-handlers
| | |\  
| | * | e681576 Add a device_num field to status.json output.
| | * | d9f6071 Added IsValidLocator to filter /index output.
| | * |   ad2984e Merge branch 'master' into 2449-keep-index-status-handlers
| | |\ \  
| | * | | 9067fd1 Added rudimentary GetNodeStatus test. (refs #2561)
| | * | |   8e69317 Merge branch '2449-keep-write-blocks' into 2449-keep-index-status-handlers
| | |\ \ \  
| | * | | | dcea512 Simplify GetNodeStatus to return only disk usage stats for volumes (refs #2561)
| | * | | |   ea6dd70 Merge branch '2449-keep-write-blocks' into 2449-keep-index-status-handlers
| | |\ \ \ \  
| | * | | | | b7d408d Fix nil map bug and status.json route. (refs #2561)
| | * | | | |   df05c26 Merge branch '2449-keep-write-blocks' into 2449-keep-index-status-handlers
| | |\ \ \ \ \  
| | * \ \ \ \ \   75df7de Merge branch 'master' into 2449-keep-index-status-handlers
| | |\ \ \ \ \ \  
| | * \ \ \ \ \ \   17c335f Merge branch '2449-keep-write-blocks' into 2449-keep-index-status-handlers
| | |\ \ \ \ \ \ \  
| | * | | | | | | | c4b5183 Write status output in JSON. (refs #2561)
| | * | | | | | | |   3c554a9 Merge branch '2449-keep-write-blocks' into 2449-keep-index-status-handlers
| | |\ \ \ \ \ \ \ \  
| | * | | | | | | | | 0a09da6 Added status.json handler. (refs #2561)
| | * | | | | | | | | c7b155c Added /index handlers. (refs #2561)
| * | | | | | | | | | 89fcf2b Increased wait time on capybara test to reduce chance of failure.
| * | | | | | | | | |   8048da3 Merge branch '2376-show-collection-tags-everywhere'
| |\ \ \ \ \ \ \ \ \ \  
| | * | | | | | | | | | 340bd4f Fixed typo in application_helper.rb Removed commented out line in application.html.erb
| | * | | | | | | | | | d66cd99 Javascript to replace <span> tags used for labels to square brackets for selection lists that can't handle HTML. Some output sanitization in case a tag has HTML in it.
| | * | | | | | | | | | 2a73947 Removes spurious whitespace.
| | * | | | | | | | | | 6c76e5c Adds tags to link text for anything using #link_to_if_arvados_object for a collection Changed breadcrumb at top of page to use #link_to_if_arvados_object Adding a collection to selection list includes tags in item text
| * | | | | | | | | | | dc51a3a Fixed error reporting when head/tail_uuid don't match head/tail_kind for links. Added a couple more tests.
| * | | | | | | | | | |   ea7c6ef Merge branch '2488-jobs-pipeline-doc'
| |\ \ \ \ \ \ \ \ \ \ \  
| | * | | | | | | | | | | 4f4f0f5 Improved language about git revisions a bit based on review feeback.
| | * | | | | | | | | | | a27dd6c Reorganized job/pipeline docs into relevant API reference sections
| | |/ / / / / / / / / /  
| * | | | | | | | | | | 52b3716 Parameterize the 'arv' command so that it can be specified with an environment variable to make it easier to run with the right environment for development.
| | |_|_|_|_|_|_|_|_|/  
| |/| | | | | | | | |   
* | | | | | | | | | | 1881da3 Remove bootstrap_components logic from Workbench, now that it happens automatically on the API server side.
* | | | | | | | | | | fe046f1 Fix, and tests for, PipelineInstance#bootstrap_components
* | | | | | | | | | |   eb72276 Merge branch 'master' into 2596-refactor-pipeline-create
|\ \ \ \ \ \ \ \ \ \ \  
| |/ / / / / / / / / /  
| * | | | | | | | | | c2aeaa7 Job reuse bugfix: do not reuse completed jobs that have NULL output.
| * | | | | | | | | |   bb45025 Merge branch '2388-bogus-token-error-page'
| |\ \ \ \ \ \ \ \ \ \  
| | * | | | | | | | | | f9578ab Fix exception when valid token points to missing user
| | * | | | | | | | | | 9f3211f Add tests to expose some token handling bugs.