Re: [PATCH 1/7] Softrefs: Add softrefs header file with API documentation

[Johan Herland <johan@herland.net>]

On Sunday 10 June 2007, Johannes Schindelin wrote:
> Hi,
> 
> On Sat, 9 Jun 2007, Johan Herland wrote:
> 
> > See patch for documentation.
> 
> This is preposterous. Either you substitute the patch for a documentation, 
> or you document it in the commit message. I consider commit messages like 
> "See patch for documentation" as reasonable as all those CVS "** no 
> message **" abominations.

Well, I could have copied documentation from the header file into the commit
message

> > + * The softrefs db consists of two files: .git/softrefs.unsorted and
> > + * .git/softrefs.sorted. Both files use the same format; one softref per line
> > + * of the form "<from-sha1> <to-sha1>\n". Each sha1 sum is 40 bytes long; this
> > + * makes each entry exactly 82 bytes long (including the space between the sha1 + * sums and the terminating linefeed).
> > + *
> > + * The entries in .git/softrefs.sorted are sorted on <from-sha1>, in order to
> > + * make lookup fast.
> > + *
> > + * The entries in .git/softrefs.unsorted are _not_ sorted. This is to make
> > + * insertion fast.
> 
> This sure sounds like you buy the disadvantages of both. Last time I 
> checked, it was recommended to look at your needs and pick _one_ 
> appropriate data structure fitting _all_ your needs.

First, the unsorted file is bounded in size to make sure it never gets
large enough to really impact performance
Second, I'd ask you to look at the performance numbers (in patch #0)
before commenting on the performance.

> Besides, your lines are way too long. Yes, it is not in 
> Documentation/SubmittingPatches, but even just a cursory look into the 
> existing source shows you that it is mostly 80-chars-per-line. I think it 
> goes without saying that you should try to imitate the existing practices 
> in any project, and since you have to read the source to get acquainted 
> with it _anyway_, it would only be a duplication to have it in 
> SubmittingPatches, too.

I have indeed tried to follow the 80-chars-per-line rule. In softrefs.h
I fail to see a _single_ line exceeding 80 chars per line. In the other
files I believe the number of long lines is comparable to other files
in the git repo.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net