Git: Who cares about branches? It’s all about collaboration and code reviews

9 Replies

TL;DR: Using Git has made our team much better by removing barriers to collaboration and code reviews. Those are the real Git benefits, not specific features like fast branches.

Introduction

I heard about Git several times before starting 8th color, but was never convinced by the arguments that were presented to me. We started using Git from day one at 8th color, mostly because it “came with the package” – in the Ruby & Rails world, it seems that nary a developer is using SVN or even CVS.

The Git icon, already branching (Photo credit: Wikipedia)

18 months later, I finally feel confident enough to recommend it (on appropriate use cases) to my peers, mainly because, in my humble opinion, Git is very loudly but often very badly advertised. Here is a quick summary of the arguments I heard and discounted, and those I think are actually valid but too often overlooked

It come down to one thing: Git is not about speed or local commits or branches, Git is all about collaboration and code reviews.

What I heard about Git

My problem with Git advertising is that what I mostly heard were two things:

“You can create branches very fast”
“Git is distributed, you can commit locally”

That makes it much clearer (Photo credit: Wikipedia)

Problem was, with some years of SVN experience under my belt, those points looked like what I call a solution looking for a problem. Fact is, I never had big problems creating branches using SVN/CVS – but I had my share of problems merging them. Even then: creating and merging branches were not part of my daily work.

The “commit locally” was equally flawed. I already had two states (uncommitted and committed); why did I need a third one (uncommitted, committed locally, committed remotely)? I did not work long without a connection anyway, and was used to short well defined commit already. Moreover, I was never comfortable with code sitting only on my machine, committed or not.

The problem here is that those two selling points are not benefits – they are mere features. You do not sell a product or tool on features – you sell the added value to your job. Those features are interesting, but not as such: they are interesting for what they bring to your development flow.

What I love

The big point about speed and easy branches (and, one should said, a sane branching model, where branches are not simply folders and conventions) is that it allows you to start using a significantly different collaboration model.

My typical SVN flow

We used the main branch (trunk) for the “long running development”. Once a release (V1.0) was scheduled, I created a tag once all features were in, and we started working on the next one (V2.0). Should a problem arise in production with the V1.0, we could then create a branch from the tag, fix any bug there, and release a V1.1 without impacting our V2.0 work (except to port any bug fix done, of course).

This worked pretty well, with almost no merges to do (the V1.1 branch was left to die, with work occurring on the main branch). Bugs had to be backported, of course, but we used to have reasonably short release cycles, which helped.

As for the various features that we were working on (in a team of 4-5 developers), I tried to find a way of splitting work that allowed most of the team relative freedom, with the rules that once the code was not making tests fail, it could be committed – even if not finished. We had our “conflict” moments, but it worked decently – and it still does – when I’m using SVN with other teams.

What Git allows: feature branches

The main benefit that Git unique feature brings is the possibility to use feature branches. By possibility, I do not mean the technical feasibility (you could theoretically make feature branches using SVN) but practical feasibility – if you are in a development team, your version control tool is, well, a tool, so it should work and allows you to create value.

Features branches are just that: they mean that each time that someone starts working on a feature or even a bug (depending of your level of orthodoxy), he/she creates a branch for that work from the main branch (typically named “master”). Several of them can be created at the same time, which will be frequent if you are making sprints – typically one per active developer, each on his own feature.

As branches are basically “free” (they are created instantly, which is logical once you understand how they are implemented), you can start working immediately, and commits can be pushed without problems – after all, you do not risk any other developers work, as you are “isolated”.

Once the feature is done and well tested, it can be merged into the master. You can then starts a new one – or use an additional step.

Removing barriers to code reviews and collaboration

The fact that each feature is on a separate branch make it much easier to do code review and generally to collaborate on code. Before merging back in the master, it is a good time and a good idea to ask for a review.

As the reviewer, you are probably busy in some code yourself. But with Git, if all your changes are committed (if they are not, there is still a way to save them temporarily), looking at a colleague code means just checking out his branch – again a very fast operation. You can then look at the code, run the tests or the code, and even commit changes if needed. Don’t forget: “If you give a patch to a man, his code will be better today. If you discuss it with him, his code (and yours!) will be better for all his life.”

GitHub has actually made this process a feature named “Pull Request”, allowing for a conversation around the feature to take place in an easy way with a nice GUI.

Looking back at those Git features, the benefit is clear: it removes barrier to collaboration and code reviews, which are key to producing good software – and good developers.

From the trenches

Git usage, from the trenches (Photo credit: Frank Van Hevel)

Here are some recent, real life examples directly from our own three person team at 8th color.

We produce new branches on a daily basis, and switch between them several times a day (for example to look at a colleague’s code, then go back into our own).
I did create a branch & the associated pull request for a modification of exactly two lines. This may look as overkill but even small patches deserve feedback and as it is free, why miss the opportunity?
It happens that the review does not arrive immediately. In most situations, you can still create a new branch for the next feature, the merge can come afterward.
The pull request & review process is “mandatory” (i.e. we decided that it was a good way),
meaning that we conduct peer review (once again: a review is not something a senior does on a junior code. It is something someone does on someone’s else code.) on a daily basis enhancing our code quality – and our team work.
It is the reviewer job to merge the branch into the master. This is a “one second” job (using GitHub, it is actually one click), so why emphasize it? Because it means that the reviewer become co-owner (and co-responsible) for this code. If code you reviewed and allowed into the master ends up badly, you are as much guilty as the one that wrote it.

For starters

If this has convinced you to take a look at Git, some starters advice :

Git works in a significantly different way than SVN or CVS. Not only that, you have to understand (at high level) the way it works, or it will come back and bite you, whether with a data loss (Git is rightly advertised as never losing any data, but possess commands or options that can actually remove some !) or just surprising situations (when switching branches with uncommitted changes, for example).
Take a look at a Git Flow. Git low level commands allow for a lot of different possible workflows, meaning you have to pick one and stick to it – when are you creating branches, what do they means, how are they merged, etc. The model described in the link works pretty well for hundreds of team, and is probably a good starting point for yours too.
Git UI is complex and inconsistent, but you need to use it. And I do not speak of any GUI here, just about the command line. A command line is a UI. Git command line UI sucks. Use it anyway.
Stay where you are comfortable. Git has a lot, I mean A LOT of options and commands. You do not need everything to start. Actually 7 commands can get you really far
- Committing: add/commit: add basically add files that you changed to the next commit batch, and commit bundle them with your log message.
- Remoting: push/pull: push send your changes to the remote server, pull get the changes from the server back to you.
- Branching: branch/checkout/merge: branch create a new branch, checkout switch from one branch to another, merge merge the content of a branch inside the current one.
Use and abuse pull requests (if you are using GitHub), or ask for review before each merge. It really makes a world of difference.
Read about “merge vs rebase” if you want. No, don’t. On some mornings you can see advocates of both taking the sweet spots in Hacker News’ home page for saying exactly the opposite. Be warned: Here Be Dragons. My own advice on this subject is a cup of tea and a good spot on the sofa. Technological church wars are so fun to watch.

I hope you’ll find the journey as frustrating and the arrival as beneficial as I did.

Français : Début de l’installation de Git (Photo credit: Wikipedia)

Use your custom Bootstrap icons in your Rails apps

The icon specification

The icon dimensions are 60×56 and stored as a SVG file myicon.svg in the directory app/assets/images/icons/. When drawing the icon, try to keep in mind that the icon dimensions will be reduced to 15×14: draw everything as a multiple of 4.

The stylesheet

The technique of CSS sprites allows you to gather all background images in a single file. You then display only the part you want with a clipping rectangle defined by the dimensions of your block element and the background position. Twitter Bootstrap provides all the icons as is. If you look into the sass-bootstrap stylesheet defining the icons class, you’ll find three important parts:

the common properties to all icon classes [class^="icon-"], [class*=" icon-"],
the properties for inverted icon icon-white, and
a sprite for each icon, for instance .icon-glass { background-position: 0 0; }.

For our own icons with a class name starting with myicon-, we’ll do the first and last part (no inverted version). The properties common to all icons are the following:

app/assets/stylesheets/_mysprites.scss

[class^="myicon-"], [class*=" myicon-"] {
  display: inline-block;
  width: 15px;
  height: 14px;
  line-height: 14px;
  vertical-align: text-top;
  background-image: url('icons.png');
  background-position: 15px 14px;
  background-repeat: no-repeat;
  margin-top: 0;
  vertical-align: text-bottom;
}

For the last part defining the CSS sprites themselves, we’ll use sprite-factory.

The generation

The gem sprite-factory will help you to pack all icons in one file and generate the corresponding CSS sprites. It can even be used in a Rake task.

First write the dedicated Rake task

lib/tasks/resprite.rake

require 'sprite_factory'
namespace :assets do
  desc 'recreate sprite images and css'
  task :resprite => :environment do
    # your task that we'll describe below
  end
end

As we have icons as SVG files, I add first a command using the magic convert command of ImageMagick to transform them into PNG files with dimensions of 15×14:

lib/tasks/resprite.rake

# ...
Dir.glob('app/assets/images/icons/*.svg').each do |svg_file|
  %x(convert -antialias -background transparent #{svg_file} -resize 15x14 #{File.dirname svg_file}/#{File.basename(svg_file).gsub(/\.svg\z/, ".png")} )
end
# ...

Then, I set up sprite-factory to use sass-rails helper method image-url to be evaluated by the rails asset pipeline:

lib/tasks/resprite.rake

# ...
SpriteFactory.cssurl = "image-url('$IMAGE')"
# ...

I define an array rules of CSS directives that will be used by sprite-factory to generate the stylesheet, and already append to it the definition of common properties to all classes myicon-:

lib/tasks/resprite.rake

# ...
rules = []
rules << <<EOF
[class^="myicon-"], [class*=" myicon-"] {
  display: inline-block;
  width: 15px;
  height: 14px;
  line-height: 14px;
  vertical-align: text-top;
  background-image: url('icons.png');
  background-position: 15px 14px;
  background-repeat: no-repeat;
  margin-top: 0;
  vertical-align: text-bottom;
}
EOF
# ...

Finally, I run sprite-factory with some parameters

lib/tasks/resprite.rake

# ...
SpriteFactory.run!(
  'app/assets/images/icons',
  :style => :scss,
  :layout => :packed,
  :library => :chunkypng,
  :selector => '.myicon-',
  :output_style => 'app/assets/stylesheets/_mysprites.scss',
  :nocomments => true
) do |images|
  images.map do |name, data| # for each image, we add a CSS sprite
    rules << ".myicon-#{name} { background-position: -#{data[:x]}px -#{data[:y]}px; }"
  end
  rules.join("\n")
end
# ...

Get the final result at that Gist.

The usage

As everything is set up, I can use my Rake task as

rake assets:environment

It generates:

a PNG file into app/assets/images/icons for each icon SVG file,
a PNG file app/assets/images/icons.png containing all the icons packed into one file,
a stylesheet app/assets/stylesheet/_mysprites.scss

We can now enjoy it by importing it into the application stylesheet:

@import 'mysprites';

You have no more reason to not go create yours! If you do, let me know (and if we can see them on a page, even better!).

Dead code is rotting your codebase

3 Replies

TL;DR; : You are always coding for a fellow programmer, not for a machine. You should put the most energy to make it easy for he/she to understand your code. The machine will be fine anyway. Dead or commented code is like false trail signs that only befuddle people, so avoid it at all costs.

I spend a lot of time reading code – mine, my colleague’s, and others, when asked to “take a look” on a given codebase. On several recent occasions, I found myself staring at two versions of dead code: commented code and unused code. You may think that dead code is not good – and you’d be wrong. Dead code is far worse than that – it is actually rotting your code base. I wanted to take the opportunity to explain why.

Commented code – the memory devil

Commented code can appear quite easily: you are replacing some existing implementation by another, and willing to let everything work until you are sure of the “new way”, you just comment out the parts that will no more be used. Then, at some point, you forget about it, letting some puzzling piece of code prefixed by “//” or “#” or something like that.

Now, when I ask why it is “still” there, I get several kind of answers, that I could screenplay like this:

- Why is this code commented out?
- This one? Because it is no more needed, we are now doing Y which is much better.
- Great! Why it is still there then?
- Well, we changed it for the best, but the old approach did work, so it is there “in case of”.
- But it is commented, right? It does not do anything more?
- No, it is just to be able to use it again should it be needed.
- Don’t you use Git (SVN, CVS)?
- Yes, why?

Again, as outlined above, I think there is a perfectly valid case for commented out code: during a refactor. I do not wipe out everything when I’m refactoring, I’m moved pieces bits after bits, and the comment part is very useful. But, the commented code should be gone before any other person could see it (except my imaginary friend the pair programmer). By the time my pull request is ready, it should be long gone. Be careful, it is easy to forget.

Now, you can well think “it is not that big of a deal – it does not do anything anyway”.

Wrong.

There is one thing commented code is very efficient at: spreading FUD. Not for the machine, of course, it discards it immediately, but at your fellow programmers. Comments are supposed to be a trail, like little bread crumbs dropped by the previous programmers to help you find your way. Each of them is a sign, which means you should ponder them : how easy would it make for the person following you to follow this trail ? How straightforward is it ? Having no trail signs at all make the road difficult, but false or contradictory ones can be even worse.

The conversation above can seem funny, but it is the easy case, where there is someone to answer. There is not alway someone, and stumbling on commented code leaves me uneasy: why did the previous programmer remove it? What was the flaw in? Why did he let it there? Is it because we can still need it? In which case? I do not remember a lot of cases where the reason for commenting was itself commented.

Again, the best way is to remove it, and commit the reason in the log. This would leave the future programmer every chance to understand the code, and release him from the need to understand every step of the history of it. We need the code history, but we have a splendid tool for code archeology: the VCS. There is no need to double down its job.

Unused code – the insidious devil

Of course, commented out code is only the lesser evil, compared to his far more dangerous cousin: unused code. Code that is not commented out, but is “normally” no more used. Like in “should be no more used”.

This is far worse because it is not only sending incorrect message about the code base, it also costs in maintenance – I may well do some refactoring, and take tim

Not that glamorous (Photo credit: Wikipedia)

e to fix code that is actually just dead meat. Identifying dead code is no simple job, but facing the expression outlined above, I tend to urge to remove it as soon as possible, using tests to validate that it was really not used.

The “we let it there just in case” is even weaker here than in the previous case: again, the VCS is your friend, let it work. The smaller, leaner the code base is, the faster the new features will be done, and the fewer bugs will be introduced. I like to paraphrase the Agile Manifesto: the best code is the code you do not need to test, which exists: it is the code that you delete (or do not write at all).

Conclusion

This is one of the situations where it is very important to remember a primordial truth in programming: the primary target of any code should always be another human being, never a machine. That means that you should always write it to make it as easiest possible to grasp by that human being, not by the machine, for several reasons:

The machine will be able to understand it anyway, thank you. It does not need your help for it, nor does it really care about how impossible to read it is, as long as it is syntactically correct.
You can think that sometimes, it may make the code a little less efficient, but this is tricky to determine, and compilers are disgustingly good at running code, so chances are good that they can optimize it better than you can. Anyway, the day you code this kind of optimization, you are not forbidden to comment it extensively.
Most of the works on a codebase happen in maintenance, not in the initial glory days of greenfield development.

Finally, chances are good that that “other human being” is actually you in 3-6 months. So do you a favor, and remove this dead code today.