On Tue, Dec 7, 2021 at 3:54 AM Patrick Steinhardt <ps@xxxxxx> wrote:
>
> On Tue, Dec 07, 2021 at 02:46:50AM +0000, Neeraj Singh via GitGitGadget wrote:
> > From: Neeraj Singh <neerajsi@xxxxxxxxxxxxx>
> [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
