On Mon, Sep 17, 2018 at 4:55 PM Ben Peart <peartben@xxxxxxxxx> wrote:
> On 9/15/2018 6:02 AM, Duy Nguyen wrote:
>
> >> 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.
> >> @@ -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@xxxxxxxxxxxxxxxxxxxxxxxxxxx/
>
>
> >> +
> >> +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
