Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate

[Taylor Blau <me@ttaylorr.com>]

Hi Junio,

On Tue, Apr 21, 2020 at 01:41:56PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> > In bd0b42aed3 (fetch-pack: do not take shallow lock unnecessarily,
> > 2019-01-10), the author noted that 'is_repository_shallow' produces
> > visible side-effect(s) by setting 'is_shallow' and 'shallow_stat'.
> >
> > This is a problem for e.g., fetching with '--update-shallow' in a
> > shallow repsoitory with 'fetch.writeCommitGraph' enabled, since the
>
> repository.

Whoops, sorry about that.

> > update to '.git/shallow' will cause Git to think that the repository
> > isn't shallow when it is, thereby circumventing the commit-graph
> > compatability check.
> >
> > This causes problems in shallow repositories with at least shallow refs
> > that have at least one ancestor (since the client won't have those
> > objects, and therefore can't take the reachability closure over commits
> > when writing a commit-graph).
>
> OK.
>
> > Address this by introducing 'reset_repository_shallow()', and calling
> > it whenever the shallow files is updated. This happens in two cases:
> >
> >   * during 'update_shallow', when either the repository is
> >     un-shallowing, or after commit_lock_file, when the contents of
> >     .git/shallow is changing, and
> >
> >   * in 'prune_shallow', when the repository can go from shallow to
> >     un-shallow when the shallow file is updated, forcing
> >     'is_repository_shallow' to re-evaluate whether the repository is
> >     still shallow after fetching in the above scenario.
> >
> > As a result of the second change, 'prune_shallow' can now only be called
> > once (since 'check_shallow_file_for_update' will die after calling
> > 'reset_repository_shallow'). But, this is OK since we only call
> > 'prune_shallow' at most once per process.
> >
> > Helped-by: Jonathan Tan <jonathantanmy@google.com>
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> >
> > Here's a cleaned up version of the patch that we were originally
> > discussing in
> > https://lore.kernel.org/git/20200414235057.GA6863@syl.local/, which
> > addresses some of Jonathan's feedback and adds a test to make sure that
> > the new behavior is working correctly.
> >
> >  commit.h                 |  1 +
> >  fetch-pack.c             |  1 +
> >  shallow.c                | 15 ++++++++-------
> >  t/t5537-fetch-shallow.sh | 36 ++++++++++++++++++++++++++++++++++++
> >  4 files changed, 46 insertions(+), 7 deletions(-)
> >
> > diff --git a/commit.h b/commit.h
> > index 008a0fa4a0..ee1ba139d4 100644
> > --- a/commit.h
> > +++ b/commit.h
> > @@ -251,6 +251,7 @@ int register_shallow(struct repository *r, const struct object_id *oid);
> >  int unregister_shallow(const struct object_id *oid);
> >  int for_each_commit_graft(each_commit_graft_fn, void *);
> >  int is_repository_shallow(struct repository *r);
> > +void reset_repository_shallow(struct repository *r);
> >  struct commit_list *get_shallow_commits(struct object_array *heads,
> >  					int depth, int shallow_flag, int not_shallow_flag);
> >  struct commit_list *get_shallow_commits_by_rev_list(
> > diff --git a/fetch-pack.c b/fetch-pack.c
> > index 1734a573b0..684868bc17 100644
> > --- a/fetch-pack.c
> > +++ b/fetch-pack.c
> > @@ -1632,6 +1632,7 @@ static void update_shallow(struct fetch_pack_args *args,
> >  			rollback_lock_file(&shallow_lock);
> >  		} else
> >  			commit_lock_file(&shallow_lock);
> > +		reset_repository_shallow(the_repository);
> >  		alternate_shallow_file = NULL;
> >  		return;
> >  	}
>
> So, after updating shallow file with "fetch --update-shallow" (or
> failing to do so), we reset the in-core data.
>
> > +void reset_repository_shallow(struct repository *r)
> > +{
> > +	r->parsed_objects->is_shallow = -1;
> > +	stat_validity_clear(r->parsed_objects->shallow_stat);
> > +}
>
> OK.
>
> > @@ -362,6 +361,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
> >  		 * shallow file".
> >  		 */
> >  		*alternate_shallow_file = "";
> > +	reset_repository_shallow(the_repository);
> >  	strbuf_release(&sb);
> >  }
>
> And also after writing out the alternate shallow file (whether it is
> empty or polulated).
>
> > @@ -414,6 +414,7 @@ void prune_shallow(unsigned options)
> >  	} else {
> >  		unlink(git_path_shallow(the_repository));
> >  		rollback_lock_file(&shallow_lock);
> > +		reset_repository_shallow(the_repository);
> >  	}
>
> Here, we reset only after we realize we cannot write the updated
> shallow file.  Intended?

Yes, see this earlier discussion I had about it with Jonathan:
https://lore.kernel.org/git/20200416020509.225014-1-jonathantanmy@google.com/.

> > +	cat <<EOF >expect.refs &&
> > +refs/remotes/shallow/master
> > +refs/remotes/shallow/no-shallow
> > +refs/tags/heavy-tag
> > +refs/tags/heavy-tag-for-graph
> > +refs/tags/light-tag
> > +refs/tags/light-tag-for-graph
> > +EOF
>
> 	cat <<-EOF >expect.refs &&
> 	... body can be indented by any number of TAB
> 	... to make it easier to view
> 	EOF
>
> > +	test_cmp expect.refs actual.refs &&
> > +	git log --format=%s shallow/master >actual &&
> > +	cat <<EOF >expect &&
>
> Likewise.

I'd be happy to update these, but I am matching the (poor) style of the
surrounding tests. Are you OK with the inconsistency?  Would you like
another preparatory patch to clean up the surrounding?

Thanks,
Taylor