On Fri, Nov 29, 2013 at 10:21:04PM +0100, Thomas Rast wrote:

> I do think it's worth fixing the syntax pedantry at the end so that we
> can keep supporting arcane compilers, but otherwise, meh.

Agreed. I've picked up those changes in my tree.

> > +static int open_pack_bitmap_1(struct packed_git *packfile)
> This goes somewhat against the naming convention (if you can call it
> that) used elsewhere in git.  Usually foo_1() is an implementation
> detail of foo(), used because it is convenient to wrap the main part in
> another function, e.g. so that it can consistently free resources or
> some such.  But this one operates on one pack file, so in the terms of
> the rest of git, it should probably be called open_pack_bitmap_one().

Hmm. I see your point, but I think that my (and Vicent's) mental model
was that is _was_ a helper for open_pack_bitmap. It just happens to also
fill the role of open_pack_bitmap_one(), but you would not want the
latter. We only support a single bitmap at a time; by calling the
helper, you would miss out on the assert which would catch the error.

So I don't care much, but I have a slight preference to leave it, as it
signals "you should not be calling this directly" more clearly.

> A bit unfortunate that you inherit the strange show_* naming from
> builtin/pack-objects.c, which seems to have stolen some code from
> builtin/rev-list.c at some point without worrying about better naming...

Yes, I agree they're not very descriptive. Let's leave it for now to
stay consistent with pack-objects, and I'd be happy to see a patch
giving all of them better names come later.

> > +   while (i < objects->word_alloc && ewah_iterator_next(&filter, &it)) {
> > +           eword_t word = objects->words[i] & filter;
> > +
> > +           for (offset = 0; offset < BITS_IN_WORD; ++offset) {
> > +                   const unsigned char *sha1;
> > +                   struct revindex_entry *entry;
> > +                   uint32_t hash = 0;
> > +
> > +                   if ((word >> offset) == 0)
> > +                           break;
> > +
> > +                   offset += ewah_bit_ctz64(word >> offset);
> > +
> > +                   if (pos + offset < bitmap_git.reuse_objects)
> > +                           continue;
> > +
> > +                   entry = &bitmap_git.reverse_index->revindex[pos + 
> > offset];
> > +                   sha1 = nth_packed_object_sha1(bitmap_git.pack, 
> > entry->nr);
> > +
> > +                   show_reach(sha1, object_type, 0, hash, bitmap_git.pack, 
> > entry->offset);
> > +           }
> You have a very nice bitmap_each_bit() function in ewah/bitmap.c, why
> not use it here?

We are bitwise-ANDing against an on-disk ewah bitmap to filter out
objects which do not match the desired type. bitmap_each_bit would make
this more complicated, because we wouldn't be able to move the
ewah_iterator in single-word lockstep. And it would probably be slower
(if you did it naively), because we'd end up checking each bit in the
ewah, rather than AND-ing whole words.

The right, reusable way to do it would probably be to bitmap_and_ewah
the original and the filter together, and then bitmap_each_bit the
result. But you would have to write bitmap_and_ewah first. :)

> > +   /*
> > +    * Reuse the packfile content if we need more than
> > +    * 90% of its objects
> > +    */
> > +   static const double REUSE_PERCENT = 0.9;
> Curious: is this based on some measurements or just a guess?

I think it's mostly a guess.

> > +enum pack_bitmap_opts {
> > +   BITMAP_OPT_FULL_DAG = 1,
> And I think this trailing comma on the last enum item is also strictly
> speaking not allowed, even though it is very nice to have:
> pack-bitmap.h:28:27: warning: comma at end of enumerator list [-Wpedantic]

It's allowed in C99, but was not in C89.  I've fixed this site for
consistency with the rest of git. But I wonder how relevant it still is.
The only data points I know of are:




It sounds like an ancient IBM VisualAge is the only reported problem.
And according to IBM, they stopped supporting it 10 years ago (well,
technically we have a few more weeks to hit the 10-year mark):


I do wonder if at some point we should revisit our "do not use any
C99-isms" philosophy. It was very good advice in 2005. I don't know how
good it is over 8 years later (it seems like even ancient systems should
be able to get gcc compiled as a last resort, but maybe there really are
people for whom that is a burden).

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to