Re: Log-addressing branch ready for review

Stefan Fuhrmann wrote:
>>>>> As of r1516665, work on this branch has been completed.
>>>>> Please review. See the BRANCH-README for the list of
>>>>> major changes.

I have not reviewed, but looked at how to review it.

First I am looking for an overall description of the changes. The BRANCH-README says:

[[[
This is an integration / development branch to copy / implement and
review the logical addressing feature originally developed on the
fsfs-format7 branch.  After review and after the fsfs-improvements
branch got merged into /trunk, this whole branch will get merged
into /trunk as well.

Features to implement here:

* support for logical addressing
* switch to format 7; use log addressing after for new shards created
  after svnadmin upgrade
* block read / data prefetch for f7 rev / pack file data
* reorder data upon pack
* add checksums to index data
* improve verification to cover all f7 rev / pack file data

More on this here:
http://svn.haxx.se/dev/archive-2013-06/0755.shtml
http://svn.haxx.se/dev/archive-2013-07/0059.shtml
]]]

I don't know how up to date that is, but anyhow it doesn't precisely describe the changes. If you could perhaps, starting from this text, turn it into the standard log message format that you would eventually include in the merge log message, that would be much better. I am in favour of merges having a log message that really describes what's being merged, since trying to understand it from scanning the original series of branch commits can be very inefficient.

Then, to see the actual changes, we can run a diff if we first discover the trunk revision to diff against:

$ svn mergeinfo ^/subversion/trunk \
                ^/subversion/branches/fsfs-improvements
    youngest common ancestor
    |         last full merge
    |         |        tip of branch
    |         |        |         repository path

    1499980   1545953  1546120
    |         |        |      
  -------| |------------         subversion/trunk
     \         \              
      \         \             
       --| |------------         subversion/branches/fsfs-improvements
                       |      
                       1546120

$ svn mergeinfo ^/subversion/branches/fsfs-improvements \
                ^/subversion/branches/log-addressing
    youngest common ancestor
    |         last full merge
    |         |        tip of branch
    |         |        |         repository path

    1509278   1545955  1546118
    |         |        |      
  -------| |------------         subversion/branches/fsfs-improvements
     \         \              
      \         \             
       --| |------------         subversion/branches/log-addressing
                       |      
                       1546118

$ svn diff --old=^/subversion/trunk_at_1545953 \
           --new=^/subversion/branches/log-addressing > d

I find it easier to have the changes in a trunk WC and use diff-aware editors than to browse the patch file directly. I can apply this patch:

$ svn patch d
D         subversion/libsvn_subr/md5.h
D         subversion/libsvn_subr/sha1.c
D         subversion/libsvn_subr/sha1.h
U         build.conf
A         BRANCH-README
U         subversion/libsvn_fs_base/fs.c
[...]

OK, fine. (I got a minor text conflict in a test, presumably due to a recent trunk change that's not yet sync'd to the branch.)

Alternatively, I usually just run the proposed merge. At first I tried to merge it directly to a trunk WC, using automatic merge: "svn merge ^/subversion/branches/log-addressing". No go. Major conflicts.

The branching structure is like this (showing only the most recent merges):

------------------------ trunk
   \     ...   \
    +-----------+------- branches/fsfs-improvements
       \    ...    \
        +-----------+--- branches/log-addressing

Subversion's automatic merge doesn't support tracking merges going round a cycle (trunk -> fsfs-imp. -> log-addr. -> trunk). That's the basic reason it produces conflicts when we try. The "proper" way to merge this back to trunk, in terms of what Subversion's automatic merge supports, is first to merge it to fsfs-improvements. That works fine in my trial. (I only ran the merge, didn't try to build the resulting code.) Then I suppose a merge from fsfs-improvements to trunk would work fine.

It looks like fsfs-improvements itself currently has no changes on it that have not been merged to trunk (apart from BRANCH-README and a mergeinfo property).

Stefan, does all this match your understanding? Would it make sense for you to merge this to fsfs-improvements right away, to make it easier for other people to run a trial merge to trunk? And then of course the proposal would be to merge fsfs-improvements to trunk, rather than log-addressing to trunk. (I'm assuming this is the only active sub-branch off the fsfs-improvements branch at the moment.)

- Julian
Received on 2013-11-27 18:56:05 CET