Re: Is fetch.writeCommitGraph (and thus features.experimental) meant to work in the presence of shallow clones?

From: Elijah Newren <newren@gmail.com>
Date: Apr 15, 2020, 22:54 UTC
Message-ID: <CABPp-BE+bC8aECT4Ak_kSObXV-euq7vCnYeUYBEjYmW4Pko=rw@mail.gmail.com>
In-Reply-To: <20200415205442.GC216285@google.com>

On Wed, Apr 15, 2020 at 1:54 PM Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 34 quoted lines

>
> Hi,
>
> Elijah Newren wrote:
>
> > I was building a version of git for internal use, and thought I'd try
> > turning on features.experimental to get more testing of it.  The
> > following test error in the testsuite scared me, though:
> >
> > t5537.9 (fetch --update-shallow):
> >
> > ...
> > + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/*
> > remote: Enumerating objects: 18, done.
> > remote: Counting objects: 100% (18/18), done.
> > remote: Compressing objects: 100% (6/6), done.
> > remote: Total 16 (delta 0), reused 6 (delta 0), pack-reused 0
> > Unpacking objects: 100% (16/16), 1.16 KiB | 1.17 MiB/s, done.
> > From ../shallow/
> >  * [new branch]      master     -> shallow/master
> >  * [new tag]         heavy-tag  -> heavy-tag
> >  * [new tag]         light-tag  -> light-tag
> > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5
> > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5
> > error: Could not read ac67d3021b4319951fb176469d7732e6914530c5
> > fatal: unable to parse commit ac67d3021b4319951fb176469d7732e6914530c5
> >
> > Passing -c fetch.writeCommitGraph=false to the fetch command in that
> > test makes it pass.
>
> Oh!  Thanks for checking this.  At $DAYJOB this was the week we were
> going to roll out features.experimental.  Time to roll that back...
>
> How did you go about the experiment?  Does Taylor's patch make it pass?

Yes, Taylor's patch makes the experiment pass. My experiment was pretty simple; modify the code so that features.experimental defaults to true:

diff --git a/repo-settings.c b/repo-settings.c
index a703e407a3..b39bc21b99 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -8,6 +8,7 @@ void prepare_repo_settings(struct repository *r)
 {
        int value;
        char *strval;
+       int feature_experimental;

        if (r->settings.initialized)
                return;
@@ -51,7 +54,10 @@ void prepare_repo_settings(struct repository *r)
        }
        if (!repo_config_get_bool(r, "fetch.writecommitgraph", &value))
                r->settings.fetch_write_commit_graph = value;
-       if (!repo_config_get_bool(r, "feature.experimental", &value) && value) {
+       feature_experimental = 1;
+       if (!repo_config_get_bool(r, "feature.experimental", &value))
+               feature_experimental = value;
+       if (feature_experimental) {
                UPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);

UPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm,
FETCH_NEGOTIATION_SKIPPING);
                UPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 1);

(based on 2.26.0; Stolee has since made pack_use_sparse default to
true anyway) and then run the test suite and see what breaks.  For the
tests that fail, check if the failure is expected based on different
defaults and the code still functions correctly with the new defaults
(and update the test manually if so), or whether the test failure
represents some potentially scary problem.  This was the only bug
exposed by this little experiment.

> (I'm thinking it would be nice to have a
> GIT_TEST_EXPERIMENTAL_FEATURES setting that uses test_config_global at
> test init time.)