Throughout my career of 20 years as an open source software developer I’ve seen all kinds of projects with all kinds of standards, but none with higher standards as the Git project (although the Linux kernel comes pretty close).
Being able to land a commit on git.git is a badge of honor that not all developers would be able to get, since it entails a variety of rare skills, like for example the ability to receive criticism after criticism, and the persistence to try many times–sometimes reaching double-digit attempts, not to mention the skill necessary to implement the solution to a problem nobody else saw before–or managed to implement properly–in C, and sometimes the creativity to come up with alternative solutions that address all the criticism received previously.
The purpose of this post is not to say “this is why Git is better than your project” (the Git project has many issues), the purpose is to showcase parts of a fine-tuned development process so you as a developer might consider adopting some yourself.
Discussion
The first part of the process (if done correctly) is inevitably discussion. The git mailing list is open, anyone can comment on it, and you don’t even need to be subscribed, just send a mail to the address and you will be Cc’ed in all the replies.
This particular story starts with a mail by Mathias Kunter in which he asks why git push fails with certain branches, but only if you don’t specify them. So for example if you are in a branch called “topic”, this would fail:
git pushBut not this:
git push origin topicWhy? His question is a valid one.
The first reply comes from me, and I explain to him how the code views the two commands, for which a basic understanding of refspecs is needed. Briefly, a refspec has the form of src:dst, so “test:topic” would take the “test” branch and push it to the remote as “topic”. In the first command above, the refspec would be the equivalent of “topic:” (no destination), while the second command would be “topic:topic”. In other words: git does’t assume what name you want as destination.
Did I know this from the top of my head? No. I had to look at the code to understand what it’s doing before synthesizing my understanding in as succinct as a reply as I possibly could write. I often don’t look at the official documentation because I find it very hard to understand (even for experts), and it’s often inaccurate, or ambiguous.
Notice that I simply answered his question of why the first command fails, and in addition I offered him a solution (with push.default=current git would assume the name of the destination to be the same as the source), but at no point did I express any value judgement as of what was my opinion of what git ought to actually do.
Mathias thanked me for my reply, and pushed back on the solution to use the “current” mode because he thought the “simple” mode (which is the default), should behave the same way in this particular case. For his argument he used the documentation about the simple mode:
When pushing to a remote that is different from the remote you normally pull from, work as
current.
This is where the nuance starts to kick in. If the “topic” branch has an upstream branch configured (e.g. “origin/topic”), then git push would behave the same in both the “simple” and “current” modes, however, if no upstream branch is configured (which is usually the case), then it depends on the remote. According to Mathias, if he has no upstream configured, then there’s no “remote that is different from the remote you normally pull from” (the remote you normally pull from in this case is “origin”, because upstream branch is “origin/topic”), so “simple” should work like “current”.
In my opinion he is 100% right.
Additionally, I have to say we need more users like Mathias, who even though he knows how to fix the issue for himself, he is arguing that this should be fixed for everyone.
Elijah Newren suggested that perhaps the documentation could be changed to explain that this only happens when “you have a remote that you normally pull from”, in other words: when you have configured an upstream branch. But this doesn’t make sense for two reasons. If you don’t have configured an upstream branch, then the other mode that would be used is “upstream”, but “upstream” fails if you don’t have configured an upstream branch, so it would always fail. Secondly, the reason why it doesn’t always fail is that the former is not possible: when you don’t have configured an upstream branch, “origin” is used, therefore you always have a “remote that you normally pull from”.
I explained to Elijah that the remote is never null, since by default it’s “origin”, and suggested a trivially small modification to the documentation to mention that (since even experts like him miss that), but he suggested an even bigger one:
If you have a default remote configured for the current branch and are pushing to a remote other than that one (or if you have no default remote configured and are pushing to a remote other than ‘origin’), then work as ‘current’.
Oh boy! I cannot even being to explain why I find this explanation so wrong on so many levels, but let me start by saying I find this completely unparseable. And this is the biggest problem the official git documentation has: it’s simply translating what the code is doing, but if the code is convoluted, then the documentation is convoluted as well. I shred this paragraph piece by piece and showed why changing the logic would make it much more understandable.
But at this point I got tired of reading the same spaghetti code over and over again. It’s not just that I found the code hard to follow, it’s that I wasn’t actually sure of what it was doing. And so my reorganization patch series began.
The series
I am of the philosophy of code early, and code often. I don’t believe in design as separate from code, I believe the design should evolve as the code evolves. So I just went ahead and wrote the thing. The first version of my patch series reorganized the code so that the “simple” mode was not defined in terms of “current” and “upstream”, but as a separate mode, and then once it became clear what “simple” actually did, redefine “current” in terms of “simple”, rather than the other way around. It consisted of 11 patches:
- push: hedge code of default=simple
- push: move code to setup_push_simple()
- push: reorganize setup_push_simple()
- push: simplify setup_push_simple()
- push: remove unused code in setup_push_upstream()
- push: merge current and simple
- push: remove redundant check
- push: fix Yoda condition
- push: remove trivial function
- push: flip !triangular for centralized
- doc: push: explain default=simple correctly
I could describe each one of these patches in great detail, and in fact I did: in the commit messages of each of these patches, but just to show an example of the refactorization these patches do, let’s look at patch #7, #8, and #9.
push: remove redundant check
static int is_workflow_triangular(struct remote *remote)
{
- struct remote *fetch_remote = remote_get(NULL);
- return (fetch_remote && fetch_remote != remote);
+ return remote_get(NULL) != remote;
}There is no need to do two checks: A && A != B, because if A is NULL, then NULL != B (B is never NULL), so A != B suffices.
push: fix Yoda condition
static int is_workflow_triangular(struct remote *remote)
{
- return remote_get(NULL) != remote;
+ return remote != remote_get(NULL);
}There’s a lot of Yoda conditions in the git code, and it’s very hard for many people to parse what that code is supposed to do in those situations. Here we want to check that the remote we are pushing to is not the same as the remote of the branch, so the order is the opposite of what’s written.
push: remove trivial function
-static int is_workflow_triangular(struct remote *remote)
-{
- return remote != remote_get(NULL);
-}
-
static void setup_default_push_refspecs(struct remote *remote)
{
struct branch *branch = branch_get(NULL);
- int triangular = is_workflow_triangular(remote);
+ int triangular = remote != remote_get(NULL);Now that the code is very simple, there’s no need to have a separate function for it.
The rest
Notice that there’s absolutely no functional changes in the three patches above: the code after the patches ends up doing the exactly same as before. The same applies for all the other patches.
There’s three things that I think are important of the overall result: 1. the new function setup_push_simple is a standalone function, so to understand the behavior of the “simple” mode, all you have to do is read that function, 2. the existing function is_workflow_triangular is unnecessary, has the wrong focus, and is inaccurate, and 3. now that it’s easy to follow setup_push_simple, the documentation becomes extremely clear:
pushes the current branch with the same name on the remote.
If you are working on a centralized workflow (pushing to the same repository you pull from, which is typically
origin), then you need to configure an upstream branch with the same name.
Now we don’t need to argue about what the “simple” mode does, there’s no confusion, and the documentation while still describing what the code does is easy to understand by anyone, including the original reporter: Mathias.
Great. Our work is done…
Not so fast. This is just the first step. Even though this patch series is perfectly good enough, in the Git project the first version is very rarely accepted; there’s always somebody with a comment.
The first comment comes from Elijah, he mentioned that in patch #3 I mentioned that I merely moved code around, but that wasn’t strictly true, since I did remove some dead code too, and that made it harder to review the patch for him. That wasn’t his only comment, additionally he stated that in patch #4 I should probably add a qualification to explain why branch->refname cannot be different from branch->merge[0]->src, and in patch #10 he said the commit message seemed “slightly funny to [him]” but he agreed it made the code easier to read, and finally in the last patch #11 which updates the documentation:
Much clearer. Well done.
I replied to all of Elijah’s feedback mostly stating that I’m fine with implementing his suggestions, but additionally I explained the reasoning behind changing “!triangular” to “centralized”, and that’s because I heard Linus Torvalds state that the whole purpose of git was to have a decentralized version control system so it makes no sense to have a conditional if (triangular) when that conditional is supposed to be true most of the time.
Additionally Bagas Sanjaya stated that we was fine with the changes to the documentation, since the grammar was fine.
v2
OK, so the first version seemed to be a success in the sense that everyone who reviewed it didn’t find any fatal flaws in the approach, but that doesn’t mean the series is done. In fact, most of the work is yet ahead.
For the next version I decided to split the patch series into two parts. The first part includes the patches necessary to reach the documentation update which was the main goal, and the second part would be everything that makes the code more readable but which is not necessary for the documentation update. For example changing !triangular to centralized is a nice change, but not strictly necessary.
Now, v2 requires more work than v1 because not only do I have to integrate all the suggestions from Elijah, but I have to do it in a way that it’s still a standalone patch series, so anyone who chooses to review v2 but hasn’t seen v1, can understand it. So essentially v2 has to be recreated.
This is why git rebase is so essential, because it allows me to choose which commits to update, and the end result would look like all the suggestions from Elijah had always been there.
But not only that, I have to update the description of the series (the cover letter in git lingo), to explain what the patch series does for the people that haven’t reviewed it before, and in addition explain what changed from v1 to v2 for the people that have already reviewed the first version.
This is where one tool which I think is mostly unknown comes into play: git rangediff. This tool compares two versions of a patch series (e.g. v1 and v2), and then generates a format similar to a diff delineating what changed. It takes into consideration all kinds of changes, including changes to the commit message of every commit, and if there are no changes either in the code or the commit message, that’s shown too.
So essentially people who have already reviewed v1 can simply look at the rangediff for v2 and based on that figure out if they are happy with the new version. The reason why it’s called rangediff is because it receives two ranges as arguments (e.g. master..topic-v1 master..topic-v2).
This time the result was 6 patches. If you check the rangediff you can see that I made no changes in the code, whoever, I changed some of the commit messages, and 5 patches were dropped.
Let’s look at one change from the rangediff for patch #3.
3: de1b621b7e ! 3: d66a442fba push: reorganize setup_push_simple()
@@ Metadata
## Commit message ##
push: reorganize setup_push_simple()
- Simply move the code around.
+ Simply move the code around and remove dead code. In particular the
+ 'trivial' conditional is a no-op since that part of the code is the
+ !trivial leg of the conditional beforehand.
No functional changes.
+ Suggestions-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
## builtin/push.c ##It should be pretty straightforward to see what I changed from the commit message.
The second part is what is most interesting. Not only does it includes better versions of the 5 patches that were dropped from the previous series, but it includes 10 patches more. So this is something that has to be reviewed from scratch, since it’s completely new.
- push: create new get_upstream_ref() helper
- push: return immediately in trivial switch case
- push: reorder switch cases
- push: factor out null branch check
- push: only get the branch when needed
- push: make setup_push_* return the dst
- push: trivial simplifications
- push: get rid of all the setup_push_* functions
- push: factor out the typical case
- push: remove redundant check
- push: fix Yoda condition
- push: remove trivial function
- push: only get triangular when needed
- push: don’t get a full remote object
- push: rename !triangular to same_remote
I’m not going go through all the different attempts I made locally to arrive to this series, but it was way more than one. I used git rebase over and over to create a sequence of commits to arrive to a clean result, where each commit implemented a logically-independent change that I thought would be easy to review, would not break previous functionality, and would make the code easier to read from the previous state.
The highlights from this series is that I added a new function get_upstream_ref that would be used by both the “simple” and “upstream” modes, but additionally by reorganizing the code I was able to remove all the setup_push_* functions, including the one I introduced: setup_push_simple. By moving everything into the same parent function now it’s easy to see what all the modes do, not only the “simple” mode. Additionally, instead of changing !triangular to centralized, I decided to change it to same_remote. Upon further analysis I realized that you could be in a triangular workflow and yet be pushing into the same repository (that you pull from), and the latter is what the code cared about, not triangular versus centralized. More on that later.
This time much more people commented: Mathias Kunter, Philip Oakley, Ævar Arnfjörð Bjarmason, and Junio C Hamano himself (the maintainer).
Philip Oakley pointed out that the documentation currently doesn’t explain what a triangular workflow is, but that isn’t a problem of this patch series. Ævar Arnfjörð Bjarmason provided suggestions for the first part of the series, but those were not feasible, and overridden by the second part of the series. Mathias Kunter said he actually preferred the explanation of the “simple” mode provided in another part of the documentation, but he incorrectly thought my patches changed the behavior to what he suggested, but that wasn’t the case.
Junio provided comments like:
- Much cleaner. Nice.
- Simpler and nicer. Good.
- Nice.
- Nicely done.
- Well done.
If you want to take a look at the change from the most significant patch, check: push: get rid of all the setup_push_* functions.
His only complaints were my use of the word “hedge” (which he wasn’t familiar with), of “move” (when in fact it’s duplicated), “reorder” (when in his opinion “split” is better), and that he prefers adding an implicit break even if the last case of a switch is empty.
I explained to Junio that the word “hedge” has other meanings:
* to enclose or protect with or as if with a dense row of shrubs or low trees: ENCIRCLE
* to confine so as to prevent freedom of movement or action
Some synonyms: block, border, cage, confine, coop, corral, edge, fence, restrict, ring, surround.
So my use of it fits.
For most of his comments I saw no issue implementing them as most didn’t even involve changing the code, but additionally he suggested changing the order so the rename of triangular is done earlier in the series. While I have no problem doing this and in fact I already tried in an interim version of the series, I knew it would entail resolving a ton of conflicts, and worse than that, if later on somebody decides they don’t like the name same_remote I would have to resolve the same amount of conflicts yet again. I said I would would consider this.
Moreover, I recalled the reason why I chose same_remote instead of centralized, and I explained that in detail. Essentially it’s possible to have a triangular workflow that is centralized, if you pull and push to different branches but of the same repository. The opposite of triangular is actually two-way, not centralized.
centralized = ~decentralized
triangular = ~two-wayThis triggered a discussion about what actually is a triangular workflow, and that’s one of the benefits of reviewing patches by email: discussions turn into patches, and patches turn into discussions. In total there were 36 comments in this thread (not counting the patches).
v3
For the next version I decided not just to move the same_remote rename sooner in the series as Junio suggested, but actually in the very first patch. Although a little risky, I thought there was a good chance nobody would have a problem with the name same_remote, since my rationale of triangular versus two-way seemed to be solid.
Part 1:
- push: rename !triangular to same_remote
- push: hedge code of default=simple
- push: copy code to setup_push_simple()
- push: reorganize setup_push_simple()
- push: simplify setup_push_simple()
- push: remove unused code in setup_push_upstream()
- doc: push: explain default=simple correctly
Part 2:
- push: create new get_upstream_ref() helper
- push: return immediately in trivial switch case
- push: split switch cases
- push: factor out null branch check
- push: only get the branch when needed
- push: make setup_push_* return the dst
- push: trivial simplifications
- push: get rid of all the setup_push_* functions
- push: factor out the typical case
- push: remove redundant check
- push: remove trivial function
- push: only check same_remote when needed
- push: don’t get a full remote object
Moving the same_remote patch to the top changed basically the whole series, as can be seen from the rangediff, but the end result is exactly the same, except for one little break.
Junio mentioned that now the end result matches what he had prepared in his repository and soon included then in his “what’s cooking” mails.
* fc/push-simple-updates (2021-06-02) 7 commits
Some code and doc clarification around "git push".
Will merge to 'next'.
* fc/push-simple-updates-cleanup (2021-06-02) 13 commits
Some more code and doc clarification around "git push".
Will merge to 'next'.So there it is, unless somebody finds an issue, they will be merged.
To be honest this is not representative of a typical patch series. Usually it takes more than 3 tries to got a series merged, many more. And it’s also not common to find so many low-hanging fruit.
switch (push_default) {
default:
case PUSH_DEFAULT_UNSPECIFIED:
case PUSH_DEFAULT_SIMPLE:
if (!same_remote)
break;
if (strcmp(branch->refname, get_upstream_ref(branch, remote->name)))
die_push_simple(branch, remote);
break;
case PUSH_DEFAULT_UPSTREAM:
if (!same_remote)
die(_("You are pushing to remote '%s', which is not the upstream of\n"
"your current branch '%s', without telling me what to push\n"
"to update which remote branch."),
remote->name, branch->name);
dst = get_upstream_ref(branch, remote->name);
break;
case PUSH_DEFAULT_CURRENT:
break;
}
refspec_appendf(&rs, "%s:%s", branch->refname, dst);Documentation/config/push.txt | 13 +++----
builtin/push.c | 91 +++++++++++++++++++------------------------
2 files changed, 47 insertions(+), 57 deletions(-)The commit
These two patch series cleaned up the code and improved the documentation, but they didn’t actually change any functionality. Now that it’s clear what the code is actually doing, Mathias Kunter’s question is easily answered: why is git push failing with “simple”? Because get_upstream_ref always fails if there’s no configured upstream branch, but should it?
All we have to do is make it not fail. In other words: if there’s no upstream branch configured, just go ahead, and thus it would behave as “current”.
Then this:
If you are working on a centralized workflow (pushing to the same repository you pull from, which is typically
origin), then you need to configure an upstream branch with the same name.
Becomes this:
If you are working on a centralized workflow (pushing to the same repository you pull from, which is typically
origin), and you have configured an upstream branch, then the name must be the same as the current branch, otherwise this action will fail as a precaution.
Which makes much more sense.
Right now pushing to master works by default:
git clone $central .
…
git pushBut not pushing to a new topic branch:
git clone $central .
git checkout -b topic
…
git pushIt makes no sense to allow pushing “master” which is much more dangerous, but fail pushing “topic”.
And this only fails because git push uses “origin” by default, and so does git clone. If you clone with --origin foobar the above will work too, because same_remote will be false.
This is the patch that changes the behavior to make it sensible, where I explain all of the above, and the change in the code is simple, basically don’t die().
It took 47 days from the moment Mathias sent his question to the point where Junio merged my patches to master, but we are close the finish line, right?
No, we are not.
Convincing Junio of some code refactoring is relatively easy, because it’s simply a matter of showing that 2 + 2 = 4; it’s a technical matter. But convincing him of what git ought to do is much more difficult because it requires changing his opinion using arguments, this part is not technical, but rhetorical.
For reference, even though several years ago I managed to convince everyone that @ is a good shortcut for HEAD, Junio still complains that it “looks ugly both at the UI level and at the implementation level“, so for some reason my arguments failed to convince him.
So can Junio be convinced of this obvious fix? Well, everything is possible, but I wouldn’t be holding my breath, especially since he has decided to ignore me and all my patches.
Either way the patch is good. It’s simple thanks to the many hours I spent cleaning up the code, and benefited from all the input from many reviewers. And of course if anyone still has any comments on it they are free to state them on the mailing list and I’d be happy to address them.
