Discussion:
Code review (reloaded)
(too old to reply)
Laurens Van Houtven
2013-05-28 10:10:05 UTC
Permalink
Hi,


I'd like to know what sort of code review practices Fossil users employ. I
believe this has come up at least twice: I've asked about it myself back in
2010, and Russ Paielli from the Scala team in 2011.

All the projects I currently work on have some explicit form of code
review, be it:

- Github pull requests
- explicit code review processes on top of an existing tool (such as
twisted + trac)
- Launchpad merge proposals

All of these tend to operate on the merging of a branch: the changes get
reviewed before being merged into master/trunk/...

It is my understanding that Fossil doesn't come with such a tool for code
reviews. Additionally, the entire point of autosync by default is to
prevent having to branch and merge all the time. So, I'm wondering, do you:

1. not do code review at all
2. only do code review on major things that get their own branch, not
reviewing small changes to trunk
3. have a code review system not based on merging into trunk
4. something else?

thanks in advance
lvh
Stephan Beal
2013-05-28 10:21:15 UTC
Permalink
Post by Laurens Van Houtven
It is my understanding that Fossil doesn't come with such a tool for code
reviews.
Correct.
Post by Laurens Van Houtven
Additionally, the entire point of autosync by default is to prevent having
to branch and merge all the time.
Not entirely. It's mainly (or largely) to avoid accidental forks. Early on
in the project we had accidental forks relatively often (fossil used to
fork automatically, but no longer does). Turning on autosync removes 90%+
of those cases.
Post by Laurens Van Houtven
1. not do code review at all
In some cases we look look over the diffs as they come in or before an
official release, but we have no standard practice/process for it.
Post by Laurens Van Houtven
2. only do code review on major things that get their own branch, not
reviewing small changes to trunk
Richard's Golden Rule to the developers is, "don't break the trunk," and so
far we've been relatively good about keeping the trunk working :). Small
changes tend to happen directly in trunk, but people sometimes stuff them
into a branch and ask for another set of eyes before they merge (kind of a
"code review honor system").

Yes, it "would be cool" to have some sort of code review feature built in
(i like Atlassian Fisheye, myself), but no, we neither have one nor do we
have a process which could be used as the basis for one developing.
--
----- stephan beal
http://wanderinghorse.net/home/stephan/
http://gplus.to/sgbeal
Richard Hipp
2013-05-28 10:38:48 UTC
Permalink
Post by Laurens Van Houtven
Hi,
I'd like to know what sort of code review practices Fossil users employ. I
believe this has come up at least twice: I've asked about it myself back in
2010, and Russ Paielli from the Scala team in 2011.
All the projects I currently work on have some explicit form of code
- Github pull requests
- explicit code review processes on top of an existing tool (such as
twisted + trac)
- Launchpad merge proposals
All of these tend to operate on the merging of a branch: the changes get
reviewed before being merged into master/trunk/...
It is my understanding that Fossil doesn't come with such a tool for code
reviews. Additionally, the entire point of autosync by default is to
1. not do code review at all
2. only do code review on major things that get their own branch, not
reviewing small changes to trunk
3. have a code review system not based on merging into trunk
4. something else?
There is a "pending review" branch (
http://www.fossil-scm.org/fossil/timeline?c=pending-review&y=ci) on the
Fossil self-hosting repository now!

Fossil also has the ability to move a check-in to a different branch after
it has been committed. Sometimes people will check-in some code by
mistake, or which is rejected by reviewers, and in those cases we simply
move the check-in off to a dead-end branch, often called "mistake". See
http://www.fossil-scm.org/fossil/timeline?r=mistake for some examples of
this.
Post by Laurens Van Houtven
thanks in advance
lvh
_______________________________________________
fossil-users mailing list
http://lists.fossil-scm.org:8080/cgi-bin/mailman/listinfo/fossil-users
--
D. Richard Hipp
***@sqlite.org
Laurens Van Houtven
2013-05-28 10:50:03 UTC
Permalink
Post by Richard Hipp
There is a "pending review" branch (
http://www.fossil-scm.org/fossil/timeline?c=pending-review&y=ci) on the
Fossil self-hosting repository now!

I'm not sure I understand the workflow here. It seems the branch name
itself is "pending-review". Where do I start writing code? How do I submit
it for review? How does code get reviewed?
Post by Richard Hipp
Fossil also has the ability to move a check-in to a different branch
after it has been committed. Sometimes people will check-in some code by
mistake, or which is rejected by reviewers, and in those cases we simply
move the check-in off to a dead-end branch, often called "mistake". See
http://www.fossil-scm.org/fossil/timeline?r=mistake for some examples of
this.
Post by Richard Hipp
Post by Laurens Van Houtven
thanks in advance
lvh
_______________________________________________
fossil-users mailing list
http://lists.fossil-scm.org:8080/cgi-bin/mailman/listinfo/fossil-users
--
D. Richard Hipp
_______________________________________________
fossil-users mailing list
http://lists.fossil-scm.org:8080/cgi-bin/mailman/listinfo/fossil-users
Richard Hipp
2013-05-28 11:15:23 UTC
Permalink
Post by Richard Hipp
Post by Richard Hipp
There is a "pending review" branch (
http://www.fossil-scm.org/fossil/timeline?c=pending-review&y=ci) on the
Fossil self-hosting repository now!
I'm not sure I understand the workflow here. It seems the branch name
itself is "pending-review". Where do I start writing code? How do I submit
it for review? How does code get reviewed?
Fossil follows a BSD-style of code development, rather than a GPL-style.

Have you ever considered the difference between these two licenses? One
way to think of the difference is that BSD is easier on readers and that
GPL is easier on writers.

With the GPL, you must agree to contribute back in change you make as a
precondition for reading the code. This make GPL a great license for
software that is built up incrementally by anonymous passers-by on the
internet. There are no pesky and burdensome licenses to deal with.
Anybody can submit a patch, without unnecessary paperwork, because by the
act of reading the code they have implicitly licensed their changes to the
world.

The BSD-style licenses (and I use BSD in this discussion only as an
archetype - most open-sources licenses work like BSD - GPL is the only real
outliers) do not place any restrictions on readers. You can download and
use BSD-licensed code with no preconditions and no restrictions. You do
not have to contribute any changes you make back to the public. That makes
BSD easier for readers to deal with. But it means that before you can
contribute back changes to a BSD-licensed project, you need to execute some
paperwork to certify that you are releasing those changes under the same
BSD-style license. This makes BSD more of a hassle for writers. Note that
no paperwork is required to contribute to a GPL-licensed project because
the contributor has implicitly consented to the license by the act of
reading the code.

In the case of Fossil we require all potential contributors to print and
sign a Contributor License Agreement (CLA). The CLA is just an affidavit
from the contributor saying that the contributions are covered by the same
BSD license as the rest of the code. The CLA is needed to help ensure that
nobody contributes changes to the project, then later comes back and tries
to claim royalty rights on the code.

Fossil's CLA can be seen at
http://www.fossil-scm.org/fossil/doc/trunk/www/copyright-release.pdf

I have signed copies of CLAs from all Fossil contributor in a file folder
in the fire safe at my office.

As it happens, I also use the CLA process to vet developers. I only turn
on the ability to push changes to the canonical Fossil repository for
people who have proven their ability to write good code. Typically, this
"proof" consists of submitting some patches either directly to me, or by
posting on this mailing list. Another way of looking at it: I place more
emphasis on reviewing coders than on reviewing code.



D. Richard Hipp
***@sqlite.org
Laurens Van Houtven
2013-05-28 15:05:43 UTC
Permalink
Post by Richard Hipp
Fossil follows a BSD-style of code development, rather than a GPL-style.
I think my question may have been a bit ambiguous. I've pondered the
differences between those licenses a lot, and I very strongly prefer
permissive licenses.

(I've snipped most of the discussions about development models; I agree
with most of what you have to say. I do have my doubts to what extent that
ties to the license. but I think that's mostly irrelevant)

As it happens, I also use the CLA process to vet developers. I only turn
Post by Richard Hipp
on the ability to push changes to the canonical Fossil repository for
people who have proven their ability to write good code. Typically, this
"proof" consists of submitting some patches either directly to me, or by
posting on this mailing list. Another way of looking at it: I place more
emphasis on reviewing coders than on reviewing code.
Right, right. I think Twisted, the other project I mentioned in my original
mail, is in a similar situation. It's MIT/X11 licensed, and contributions
come from a small team of constant contributors and a large long tail of
occasional contributors. Once you've proven your worth, you're likely to
get a commit bit.

The difference, however, is that Twisted still has a mandatory review
process regardless of whether or not you have a commit bit or not. It
appears you're saying that once people get a commit bit in Fossil, code
review requirements are significantly more lax, because they've shown that
they know what they're doing?

My question about how this process with the "pending-review" branch works
was more about the mechanics of how you'd use such a branch to facilitate
code review. What goes in that branch? How does it get there? What is the
sequence of fossil commands?

cheers
lvh
Richard Hipp
2013-05-28 15:25:43 UTC
Permalink
Post by Laurens Van Houtven
My question about how this process with the "pending-review" branch works
was more about the mechanics of how you'd use such a branch to facilitate
code review. What goes in that branch? How does it get there? What is the
sequence of fossil commands?
You can create the branch as you do the comment. For example:

fossil commit --branch pending-review

Or

fossil commit --branch experimental

If you forget to do it then, you can always visit a check-in after it is
committed and click on the "Edit" link to do things like revise the
check-in comment, update the check-in time, or move the check-in to a
different branch (such as "experimental" or "pending-review" or "mistake").

Sometimes somebody will check-in a change to trunk that I don't agree
with. When that happens, I just move their check-in off into a branch.

A tangent: Note that when you "edit" a check-in, you are not really
changing the check-in. You are, instead, adding additional information.
Fossil does not erase or modify, it only augments. The original check-in
comment, and time, and branch are all still there for anybody to see. By
'editing' the commit, you are adding a new record to the repository that
says "for display purposes, modify checking XYZ as follows..."

Notes also that Fossil allows you to start a new branch named (for example)
"experimental" even if there already exists one or more other branches with
the same name. At
http://www.fossil-scm.org/fossil/timeline?n=200&r=experimental it looks
like there are a dozen or more "experimental" branches currently in the
Fossil tree.
--
D. Richard Hipp
***@sqlite.org
Isaac Jurado
2013-05-28 15:34:49 UTC
Permalink
Post by Richard Hipp
Post by Laurens Van Houtven
My question about how this process with the "pending-review" branch
works was more about the mechanics of how you'd use such a branch to
facilitate code review. What goes in that branch? How does it get
there? What is the sequence of fossil commands?
fossil commit --branch pending-review
Or
fossil commit --branch experimental
If you forget to do it then, you can always visit a check-in after it
is committed and click on the "Edit" link to do things like revise the
check-in comment, update the check-in time, or move the check-in to a
different branch (such as "experimental" or "pending-review" or "mistake").
Sometimes somebody will check-in a change to trunk that I don't agree
with. When that happens, I just move their check-in off into a branch.
A tangent: Note that when you "edit" a check-in, you are not really
changing the check-in. You are, instead, adding additional
information. Fossil does not erase or modify, it only augments. The
original check-in comment, and time, and branch are all still there
for anybody to see. By 'editing' the commit, you are adding a new
record to the repository that says "for display purposes, modify
checking XYZ as follows..."
Notes also that Fossil allows you to start a new branch named (for
example) "experimental" even if there already exists one or more other
branches with the same name. At
http://www.fossil-scm.org/fossil/timeline?n=200&r=experimental it
looks like there are a dozen or more "experimental" branches currently
in the Fossil tree.
That is very interesting. How do you achieve the same filtering from
the command line?
--
Isaac Jurado

"The noblest pleasure is the joy of understanding"
Leonardo da Vinci
Andy Bradford
2013-08-03 15:55:31 UTC
Permalink
At http://www.fossil-scm.org/fossil/timeline?n=200&r=experimental it
looks like there are a dozen or more "experimental" branches
currently in the Fossil tree.
That is very interesting. How do you achieve the same filtering from
the command line?
fossil tag find experimental

It doesn't appear to have a way to limit the number of results to
return, so it isn't exactly the same.

Andy
--
TAI64 timestamp: 4000000051fd2816
Stephan Beal
2013-08-03 16:23:46 UTC
Permalink
Post by Andy Bradford
fossil tag find experimental
It doesn't appear to have a way to limit the number of results to
return, so it isn't exactly the same.
Sure, it does!

http://fossil-scm.org/index.html/info/73135ec22a
--
----- stephan beal
http://wanderinghorse.net/home/stephan/
http://gplus.to/sgbeal
Stephan Beal
2013-08-03 16:27:02 UTC
Permalink
Post by Stephan Beal
Sure, it does!
http://fossil-scm.org/index.html/info/73135ec22a
Caveat: when using the --raw flag the ordering is undefined (those elements
have no time information, apparently), so the limit is not all that useful
there.
--
----- stephan beal
http://wanderinghorse.net/home/stephan/
http://gplus.to/sgbeal
Andy Bradford
2013-08-03 16:44:02 UTC
Permalink
Post by Stephan Beal
http://fossil-scm.org/index.html/info/73135ec22a
Wow that was quick!

I suppose an alternate approach could have been to modify ``fossil
timeline'' to accept a TAG to restrict its search to just those
artifacts with the TAG, since it already had the ability to restrict it
to a limit. It also has the ability to restrict the search to particular
timeframe.

Andy
--
TAI64 timestamp: 4000000051fd3375
Stephan Beal
2013-08-03 17:48:07 UTC
Permalink
On Sat, Aug 3, 2013 at 6:44 PM, Andy Bradford <
Post by Andy Bradford
Post by Stephan Beal
http://fossil-scm.org/index.html/info/73135ec22a
Wow that was quick!
Only because it was easy to do and i'm belly-deep in the fossil sources
working on the library API ;).

I suppose an alternate approach could have been to modify ``fossil
Post by Andy Bradford
timeline'' to accept a TAG to restrict its search to just those
artifacts with the TAG, since it already had the ability to restrict it
to a limit. It also has the ability to restrict the search to particular
timeframe.
It can already search by tags:

http://fossil-scm.org/index.html/help?cmd=/timeline

Ah, right, that's the www interface and you want the CLI... that one's a
bit more difficult, so i won't be patching that tonight. The timeline is
probably the most complicated single piece of functionality in the app, and
touching it requires more nerves than i have left in me for today ;).

But... using the new library API it took me about 3 minutes to tweak the
timeline test script to do that:

[***@host:~/cvs/fossil/f2/th1ish]$ ./th1ish t2.th1ish --
-R=../../fossil.fsl -t=experimental -n=3
The 3 most recent timeline event(s) for ../../fossil.fsl:
ci @ 2013-01-23 13:31:09 [970cc4f16f34] by [joerg] in branch [experimental]
Only check time, if it is set.
ci @ 2013-01-18 23:05:39 [ee6ae580eee0] by [joerg] in branch [experimental]
Add new option max-download-time to limit the processing time of pull/sync
/clone requests. This helps to significantly cut down the number of time
outs
clients receive on busy server with reverse proxy configuration. It
generally
provides better response times.
ci @ 2012-10-09 16:32:44 [b21df7ecafa5] by [drh] in branch [experimental]
An alternative way to get mime-types from attachments for the "raw" page.


So... just an idea of how users will be able to write ad-hoc solutions for
their Fossil repos once this code is up and running. :)
--
----- stephan beal
http://wanderinghorse.net/home/stephan/
http://gplus.to/sgbeal
Andy Bradford
2013-08-03 18:47:26 UTC
Permalink
Ah, right, that's the www interface and you want the CLI... that
one's a bit more difficult, so i won't be patching that tonight.
The timeline is probably the most complicated single piece of
functionality in the app, and touching it requires more nerves than i
have left in me for today ;).
Haha, I did have a look at the code but I found, as you have stated,
that the query/code for the CLI timeline was complex enought that it
would warrant more time.

Andy
--
TAI64 timestamp: 4000000051fd5061
Laurens Van Houtven
2013-05-28 15:46:39 UTC
Permalink
Post by Richard Hipp
Post by Laurens Van Houtven
My question about how this process with the "pending-review" branch works
was more about the mechanics of how you'd use such a branch to facilitate
code review. What goes in that branch? How does it get there? What is the
sequence of fossil commands?
fossil commit --branch pending-review
Or
fossil commit --branch experimental
Cool, thanks :)
Post by Richard Hipp
If you forget to do it then, you can always visit a check-in after it is
committed and click on the "Edit" link to do things like revise the
check-in comment, update the check-in time, or move the check-in to a
different branch (such as "experimental" or "pending-review" or "mistake").
Ah, I didn't know about that feature.
Post by Richard Hipp
Sometimes somebody will check-in a change to trunk that I don't agree
with. When that happens, I just move their check-in off into a branch.
Right -- so basically, the default is to trust the commit is good and to do
something when it goes wrong, as opposed to always reviewing all of the
time? :)
Post by Richard Hipp
A tangent: Note that when you "edit" a check-in, you are not really
changing the check-in. You are, instead, adding additional information.
Fossil does not erase or modify, it only augments. The original check-in
comment, and time, and branch are all still there for anybody to see. By
'editing' the commit, you are adding a new record to the repository that
says "for display purposes, modify checking XYZ as follows..."
Right, that philosophy is one of the main reasons I want to get rid of git
:) One thing I'm confused about though: "for display purposes"? I
understand the ledger-like behavior of recording the change instead of
changing the original, but I would expect that the checkin is for all
intents and purposes in the new branch, not just for display ones.

Notes also that Fossil allows you to start a new branch named (for example)
Post by Richard Hipp
"experimental" even if there already exists one or more other branches with
the same name. At
http://www.fossil-scm.org/fossil/timeline?n=200&r=experimental it looks
like there are a dozen or more "experimental" branches currently in the
Fossil tree.
I knew that in the back of my head, but that definitely helped my
understanding: I was still thinking in terms of one single review branch
:-) Can commits be in multiple branches?
Post by Richard Hipp
--
D. Richard Hipp
_______________________________________________
fossil-users mailing list
http://lists.fossil-scm.org:8080/cgi-bin/mailman/listinfo/fossil-users
Richard Hipp
2013-05-28 18:19:17 UTC
Permalink
Post by Laurens Van Houtven
Post by Richard Hipp
A tangent: Note that when you "edit" a check-in, you are not really
changing the check-in. You are, instead, adding additional information.
Fossil does not erase or modify, it only augments. The original check-in
comment, and time, and branch are all still there for anybody to see. By
'editing' the commit, you are adding a new record to the repository that
says "for display purposes, modify checking XYZ as follows..."
Right, that philosophy is one of the main reasons I want to get rid of git
:) One thing I'm confused about though: "for display purposes"? I
understand the ledger-like behavior of recording the change instead of
changing the original, but I would expect that the checkin is for all
intents and purposes in the new branch, not just for display ones.
The "branch" that a check-in belongs to is the value of its "branch"
property. (Hence, a check-in can only belong to one branch at a time). To
move a check-in from one branch to another, one merely adds a new record to
the repository that says "use XYZ instead of ABC as the value of the branch
property for check-in PQR".

See http://www.fossil-scm.org/fossil/info/4e573871bcef9199 for an example
of how this works. The original value for "branch" was "trunk" (which it
inherited from its parent check-in). But then a new record was added to
the repository (the
http://www.fossil-scm.org/fossil/info/33366a18c0artifact) that changed
the value of the "branch" property to "cleanDashN".
Post by Laurens Van Houtven
Notes also that Fossil allows you to start a new branch named (for
Post by Richard Hipp
example) "experimental" even if there already exists one or more other
branches with the same name. At
http://www.fossil-scm.org/fossil/timeline?n=200&r=experimental it looks
like there are a dozen or more "experimental" branches currently in the
Fossil tree.
I knew that in the back of my head, but that definitely helped my
understanding: I was still thinking in terms of one single review branch
:-) Can commits be in multiple branches?
The "branch" is just a convenient tag, nothing more. There is nothing
magic about a branch. The important data structure is the underlying
directed acyclic graph (DAG) that defines the history of the project. You
can tag and retag various subgraphs in the DAG any way you want.
(Presumably you will choose a tagging scheme that helps you to track your
project, but Fossil does not enforce that.)

Technically, "branch" is a property of the check-in since it has exactly
one value. Because it only has one value, a check-in can only be on one
branch at a time.

Notice that changing the value of the branch property (which is to say,
moving a check-in from one branch to another) does not change the DAG at
all. It merely labels the nodes differently, which causes the nodes to be
drawn in different columns of the timeline graph. The ancestry and content
of the check-in are unaffected by this.
--
D. Richard Hipp
***@sqlite.org
Nico Williams
2013-05-29 00:07:27 UTC
Permalink
Post by Richard Hipp
If you forget to do it then, you can always visit a check-in after it is
committed and click on the "Edit" link to do things like revise the check-in
comment, update the check-in time, or move the check-in to a different
branch (such as "experimental" or "pending-review" or "mistake").
Sometimes somebody will check-in a change to trunk that I don't agree with.
When that happens, I just move their check-in off into a branch.
Hmm, wait, isn't that re-writing history? I'd expect that instead
you'd commit a backout commit (that undoes the changes from the
undesirable commit) and possibly cherry-pick the undesirable commit
onto a different (likely new) branch.

In particular, isn't this a breaking change for downstream clones of the trunk?
Post by Richard Hipp
A tangent: Note that when you "edit" a check-in, you are not really
changing the check-in. You are, instead, adding additional information.
Fossil does not erase or modify, it only augments. The original check-in
comment, and time, and branch are all still there for anybody to see. By
'editing' the commit, you are adding a new record to the repository that
says "for display purposes, modify checking XYZ as follows..."
OK, so editing the check-in is not history re-writing, but what about
the "moving" part? Are you merely creating a new branch headed by
that check-in and then resetting trunk to point to a line of check-ins
that excludes the bad one? If so, isn't that still a breaking change
to trunk? Or did you mean something more like checking-in a backout
check-in?
Post by Richard Hipp
Notes also that Fossil allows you to start a new branch named (for example)
"experimental" even if there already exists one or more other branches with
the same name. At
http://www.fossil-scm.org/fossil/timeline?n=200&r=experimental it looks like
there are a dozen or more "experimental" branches currently in the Fossil
tree.
How does one tell one apart from another?

Nico
--
Ron Wilson
2013-08-05 19:25:10 UTC
Permalink
Post by Laurens Van Houtven
Hi,
I'd like to know what sort of code review practices Fossil users employ. I
believe this has come up at least twice: I've asked about it myself back in
2010, and Russ Paielli from the Scala team in 2011.
All the projects I currently work on have some explicit form of code
- Github pull requests
- explicit code review processes on top of an existing tool (such as
twisted + trac)
- Launchpad merge proposals
In my organization, code reviews are part of our process. All issues (or
new features) get resolved (or implemented) on branches. AS part of the
integration and test phase, the latest approved code is merge into an issue
branch, code is built and tested. If it passes, another check is made for
new updates to merge in. If branch is up to date, then the developer sends
out a review request via email. After the changes are approved, another
check for updates is made. If branch must be updated, again, the code must
be reviewed, again. Otherwise, the branch is merged back in to the trunk as
approved code.

Continue reading on narkive:
Loading...