Re: [PATCH v5 1/5] eoie: add End of Index Entry (EOIE) extension

On Mon, Sep 17, 2018 at 4:55 PM Ben Peart [off-list ref] wrote:

On 9/15/2018 6:02 AM, Duy Nguyen wrote:

quoted

quoted

         default:
                 if (*ext < 'A' || 'Z' < *ext)
                         return error("index uses %.4s extension, which we do not understand",

@@ -1889,6 +1893,11 @@ static size_t estimate_cache_size(size_t ondisk_size, unsigned int entries)
         return ondisk_size + entries * per_entry;
  }

+#ifndef NO_PTHREADS
+static unsigned long read_eoie_extension(void *mmap_, size_t mmap_size);
+#endif

Keep functions unconditionally built as much as possible. I don't see
why this read_eoie_extension() must be built only on multithread
platforms.

This is conditional to avoid generating a warning on single threaded
platforms where the function is currently unused.  That seemed like a
better choice than calling it and ignoring it on single threaded
platforms just to avoid a compiler warning.

The third option is ignore the compiler. I consider that warning a
helpful suggestion, not a strict rule.

Most devs don't run single thread builds (I think) so is this function
is updated in a way that breaks single thread mode, it can only be
found out when this function is used in single thread mode. At that
point the function may have changed a lot. If it's built
unconditionally, at least single thread users will yell up much sooner
and we could fix it much earlier.

quoted

quoted

@@ -2520,11 +2534,13 @@ static int do_write_index(struct index_state *istate, struct tempfile *tempfile,
                 return err;

         /* Write extension data here */
+       offset = lseek(newfd, 0, SEEK_CUR) + write_buffer_len;
+       the_hash_algo->init_fn(&eoie_c);

Don't write (or even calculate to write it) unless it's needed. Which
means only do this when parallel reading is enabled and the index size
large enough, or when a test variable is set so you can force writing
this extension.

I made the logic always write the extension based on the earlier
discussion [1] where it was suggested this should have been part of the
original index format for extensions from the beginning.  This helps
ensure it is available for current and future uses we haven't even
discovered yet.

But it _is_ available now. If you need it, you write the extension
out. If we make this part of index version 5 (and make it not an
extension anymore) then I buy that argument. As it is, it's an
optional extension.

[1] https://public-inbox.org/git/xmqqwp2s1h1x.fsf@gitster.mtv.corp.google.com/

quoted

quoted

+
+static void write_eoie_extension(struct strbuf *sb, git_hash_ctx *eoie_context, unsigned long offset)

We normally just put function implementations before it's used to
avoid static forward declaration. Any special reason why it's not done
here?

This was done to promote readability of the (already large) read-cache.c
file.  I first considered moving the EOIE read/write functions into a
separate file entirely but they need access to information only
available within read-cache.c so I compromised and moved them to the end
of the file instead.

I consider grouping extension related functions closer to
read_index_extension gives better readability, or at least better than
just putting new functions at the end in no particular order. But I
guess this is personal view.
-- 
Duy