From: Junio C Hamano <gitster@pobox.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, Elijah Newren <newren@gmail.com>,
Jonathan Nieder <jrnieder@gmail.com>,
Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
Date: Tue, 21 Apr 2020 13:41:56 -0700 [thread overview]
Message-ID: <xmqqeesgmuzv.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <8d295389ea43c6b7e008514067b7af6eacba64a5.1587492422.git.me@ttaylorr.com> (Taylor Blau's message of "Tue, 21 Apr 2020 12:09:18 -0600")
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.
> 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?
> + 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.
next prev parent reply other threads:[~2020-04-21 20:42 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-21 18:09 [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate Taylor Blau
2020-04-21 20:41 ` Junio C Hamano [this message]
2020-04-21 20:45 ` Taylor Blau
2020-04-21 20:52 ` Junio C Hamano
2020-04-21 22:21 ` Taylor Blau
2020-04-21 23:06 ` Junio C Hamano
2020-04-22 18:05 ` Jonathan Tan
2020-04-22 18:02 ` Jonathan Tan
2020-04-22 18:15 ` Junio C Hamano
2020-04-23 0:14 ` Taylor Blau
2020-04-23 0:25 ` [PATCH v2 0/2] shallow.c: reset shallow-ness after updating Taylor Blau
2020-04-23 0:25 ` [PATCH v2 1/2] t5537: use test_write_lines, indented heredocs for readability Taylor Blau
2020-04-23 1:14 ` Jonathan Nieder
2020-04-24 17:11 ` Taylor Blau
2020-04-24 17:17 ` Jonathan Nieder
2020-04-24 20:45 ` Junio C Hamano
2020-04-23 0:25 ` [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file' Taylor Blau
2020-04-23 1:23 ` Jonathan Nieder
2020-04-23 18:09 ` Jonathan Tan
2020-04-23 20:40 ` Junio C Hamano
2020-04-24 17:13 ` Taylor Blau
2020-06-03 3:42 ` Jonathan Nieder
2020-06-03 4:52 ` Taylor Blau
2020-06-03 5:16 ` Taylor Blau
2020-06-03 13:08 ` Derrick Stolee
2020-06-03 19:26 ` Taylor Blau
2020-06-03 21:23 ` Jonathan Nieder
2020-06-03 20:51 ` Jonathan Nieder
2020-06-03 22:14 ` Taylor Blau
2020-06-03 23:06 ` Jonathan Nieder
2020-06-04 17:45 ` Taylor Blau
2020-04-23 19:05 ` [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqeesgmuzv.fsf@gitster.c.googlers.com \
--to=gitster@pobox.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=jrnieder@gmail.com \
--cc=me@ttaylorr.com \
--cc=newren@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).