Re: [PATCH v2 2/3] core.fsync: introduce granular fsync control

[Neeraj Singh <nksingh85@gmail.com>]

On Tue, Dec 7, 2021 at 3:54 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Dec 07, 2021 at 02:46:50AM +0000, Neeraj Singh via GitGitGadget wrote:
> > From: Neeraj Singh <neerajsi@microsoft.com>
> [snip]
> > diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> > index c23d01de7dc..c32534c13b4 100644
> > --- a/builtin/index-pack.c
> > +++ b/builtin/index-pack.c
> > @@ -1286,7 +1286,7 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha
> >                           nr_objects - nr_objects_initial);
> >               stop_progress_msg(&progress, msg.buf);
> >               strbuf_release(&msg);
> > -             finalize_hashfile(f, tail_hash, 0);
> > +             finalize_hashfile(f, tail_hash, FSYNC_COMPONENT_PACK, 0);
> >               hashcpy(read_hash, pack_hash);
> >               fixup_pack_header_footer(output_fd, pack_hash,
> >                                        curr_pack, nr_objects,
> > @@ -1508,7 +1508,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
> >       if (!from_stdin) {
> >               close(input_fd);
> >       } else {
> > -             fsync_or_die(output_fd, curr_pack_name);
> > +             fsync_component_or_die(FSYNC_COMPONENT_PACK, output_fd, curr_pack_name);
> >               err = close(output_fd);
> >               if (err)
> >                       die_errno(_("error while closing pack file"));
> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > index 857be7826f3..916c55d6ce9 100644
> > --- a/builtin/pack-objects.c
> > +++ b/builtin/pack-objects.c
> > @@ -1204,11 +1204,13 @@ static void write_pack_file(void)
> >                * If so, rewrite it like in fast-import
> >                */
> >               if (pack_to_stdout) {
> > -                     finalize_hashfile(f, hash, CSUM_HASH_IN_STREAM | CSUM_CLOSE);
> > +                     finalize_hashfile(f, hash, FSYNC_COMPONENT_NONE,
> > +                                       CSUM_HASH_IN_STREAM | CSUM_CLOSE);
>
> It doesn't have any effect here given that we don't sync at all when
> writing to stdout, but I wonder whether we should set up the component
> correctly regardless of that such that it makes for a less confusing
> read.
>

If it's not actually a file with some name known to git, is it really
a component of the repository? I'd like to leave this one as-is.

> [snip]
> > diff --git a/config.c b/config.c
> > index c3410b8a868..29c867aab03 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -1213,6 +1213,73 @@ static int git_parse_maybe_bool_text(const char *value)
> >       return -1;
> >  }
> >
> > +static const struct fsync_component_entry {
> > +     const char *name;
> > +     enum fsync_component component_bits;
> > +} fsync_component_table[] = {
> > +     { "loose-object", FSYNC_COMPONENT_LOOSE_OBJECT },
> > +     { "pack", FSYNC_COMPONENT_PACK },
> > +     { "pack-metadata", FSYNC_COMPONENT_PACK_METADATA },
> > +     { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH },
> > +     { "objects", FSYNC_COMPONENTS_OBJECTS },
> > +     { "default", FSYNC_COMPONENTS_DEFAULT },
> > +     { "all", FSYNC_COMPONENTS_ALL },
> > +};
> > +
> > +static enum fsync_component parse_fsync_components(const char *var, const char *string)
> > +{
> > +     enum fsync_component output = 0;
> > +
> > +     if (!strcmp(string, "none"))
> > +             return output;
> > +
> > +     while (string) {
> > +             int i;
> > +             size_t len;
> > +             const char *ep;
> > +             int negated = 0;
> > +             int found = 0;
> > +
> > +             string = string + strspn(string, ", \t\n\r");
> > +             ep = strchrnul(string, ',');
> > +             len = ep - string;
> > +
> > +             if (*string == '-') {
> > +                     negated = 1;
> > +                     string++;
> > +                     len--;
> > +                     if (!len)
> > +                             warning(_("invalid value for variable %s"), var);
> > +             }
> > +
> > +             if (!len)
> > +                     break;
> > +
> > +             for (i = 0; i < ARRAY_SIZE(fsync_component_table); ++i) {
> > +                     const struct fsync_component_entry *entry = &fsync_component_table[i];
> > +
> > +                     if (strncmp(entry->name, string, len))
> > +                             continue;
> > +
> > +                     found = 1;
> > +                     if (negated)
> > +                             output &= ~entry->component_bits;
> > +                     else
> > +                             output |= entry->component_bits;
> > +             }
> > +
> > +             if (!found) {
> > +                     char *component = xstrndup(string, len);
> > +                     warning(_("unknown %s value '%s'"), var, component);
> > +                     free(component);
> > +             }
> > +
> > +             string = ep;
> > +     }
> > +
> > +     return output;
> > +}
> > +
> >  int git_parse_maybe_bool(const char *value)
> >  {
> >       int v = git_parse_maybe_bool_text(value);
> > @@ -1490,6 +1557,13 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
> >               return 0;
> >       }
> >
> > +     if (!strcmp(var, "core.fsync")) {
> > +             if (!value)
> > +                     return config_error_nonbool(var);
> > +             fsync_components = parse_fsync_components(var, value);
> > +             return 0;
> > +     }
> > +
> >       if (!strcmp(var, "core.fsyncmethod")) {
> >               if (!value)
> >                       return config_error_nonbool(var);
> > @@ -1503,7 +1577,7 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
> >       }
> >
> >       if (!strcmp(var, "core.fsyncobjectfiles")) {
> > -             fsync_object_files = git_config_bool(var, value);
> > +             warning(_("core.fsyncobjectfiles is deprecated; use core.fsync instead"));
> >               return 0;
> >       }
>
> Shouldn't we continue to support this for now such that users can
> migrate from the old, deprecated value first before we start to ignore
> it?
>
> Patrick
>

That's a good question and one I was hoping to answer through this
discussion.  I'm guessing that most users do not have this setting
explicitly set, and it's largely a non-functional change. Git only
behaves differently in the extreme corner case of a system crash after
git exits.  That's why I believe it's okay to deprecate and remove in
one release.

If we choose to keep supporting the setting, it would introduce a
little complexity in the configuration code, but it should be doable.
I think the right semantics would be to ignore core.fsyncobjectfiles
if core.fsync is specified with any value. Otherwise,
core.fsyncobjectfiles would be equivalent to `default,-loose-object`
or `default,loose-object` for `false` and `true` respectively. I'd
prefer not to do this if I can get away with it :).

Thanks,
Neeraj