git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
@ 2020-04-21 18:09 Taylor Blau
  2020-04-21 20:41 ` Junio C Hamano
  2020-04-22 18:02 ` Jonathan Tan
  0 siblings, 2 replies; 32+ messages in thread
From: Taylor Blau @ 2020-04-21 18:09 UTC (permalink / raw)
  To: git; +Cc: Elijah Newren, Jonathan Nieder, Derrick Stolee

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
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).

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;
 	}
diff --git a/shallow.c b/shallow.c
index 7fd04afed1..9d1304e786 100644
--- a/shallow.c
+++ b/shallow.c
@@ -40,13 +40,6 @@ int register_shallow(struct repository *r, const struct object_id *oid)

 int is_repository_shallow(struct repository *r)
 {
-	/*
-	 * NEEDSWORK: This function updates
-	 * r->parsed_objects->{is_shallow,shallow_stat} as a side effect but
-	 * there is no corresponding function to clear them when the shallow
-	 * file is updated.
-	 */
-
 	FILE *fp;
 	char buf[1024];
 	const char *path = r->parsed_objects->alternate_shallow_file;
@@ -79,6 +72,12 @@ int is_repository_shallow(struct repository *r)
 	return r->parsed_objects->is_shallow;
 }

+void reset_repository_shallow(struct repository *r)
+{
+	r->parsed_objects->is_shallow = -1;
+	stat_validity_clear(r->parsed_objects->shallow_stat);
+}
+
 /*
  * TODO: use "int" elemtype instead of "int *" when/if commit-slab
  * supports a "valid" flag.
@@ -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);
 }

@@ -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);
 	}
 	strbuf_release(&sb);
 }
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 4f681dbbe1..c9c731c7a9 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -177,6 +177,42 @@ EOF
 	)
 '

+test_expect_success 'fetch --update-shallow (with fetch.writeCommitGraph)' '
+	(
+	cd shallow &&
+	git checkout master &&
+	commit 8 &&
+	git tag -m foo heavy-tag-for-graph HEAD^ &&
+	git tag light-tag-for-graph HEAD^:tracked
+	) &&
+	(
+	cd notshallow &&
+	test_config fetch.writeCommitGraph true &&
+	git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&
+	git fsck &&
+	git for-each-ref --sort=refname --format="%(refname)" >actual.refs &&
+	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
+	test_cmp expect.refs actual.refs &&
+	git log --format=%s shallow/master >actual &&
+	cat <<EOF >expect &&
+8
+7
+6
+5
+4
+3
+EOF
+	test_cmp expect actual
+	)
+'
+
 test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '
 	cp -R .git read-only.git &&
 	test_when_finished "find read-only.git -type d -print | xargs chmod +w" &&
--
2.26.2.108.g048abe1751

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
  2020-04-21 18:09 [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate Taylor Blau
@ 2020-04-21 20:41 ` Junio C Hamano
  2020-04-21 20:45   ` Taylor Blau
  2020-04-22 18:02 ` Jonathan Tan
  1 sibling, 1 reply; 32+ messages in thread
From: Junio C Hamano @ 2020-04-21 20:41 UTC (permalink / raw)
  To: Taylor Blau; +Cc: git, Elijah Newren, Jonathan Nieder, Derrick Stolee

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.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
  2020-04-21 20:41 ` Junio C Hamano
@ 2020-04-21 20:45   ` Taylor Blau
  2020-04-21 20:52     ` Junio C Hamano
  0 siblings, 1 reply; 32+ messages in thread
From: Taylor Blau @ 2020-04-21 20:45 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Taylor Blau, git, Elijah Newren, Jonathan Nieder, Derrick Stolee

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
  2020-04-21 20:45   ` Taylor Blau
@ 2020-04-21 20:52     ` Junio C Hamano
  2020-04-21 22:21       ` Taylor Blau
  2020-04-22 18:05       ` Jonathan Tan
  0 siblings, 2 replies; 32+ messages in thread
From: Junio C Hamano @ 2020-04-21 20:52 UTC (permalink / raw)
  To: Taylor Blau; +Cc: git, Elijah Newren, Jonathan Nieder, Derrick Stolee

Taylor Blau <me@ttaylorr.com> writes:

>> > @@ -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/.

I did, and then I asked the question, because I couldn't quite get
if JTan was asking a question similar to the one he asked earlier in
the message ("do you need a reset in the "else" branch as well?"),
or if he was saying what he sees there, "the opposite case", was
good.

Also, I was sort-of reacting to """In any case, I think the commit
message should discuss why reset_repository_shallow() is added only
on the unlink+rollback side in one "if" statement, but only on the
opposite "commit" side in another "if" statement.""" in that
message.

Thanks.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
  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
  1 sibling, 1 reply; 32+ messages in thread
From: Taylor Blau @ 2020-04-21 22:21 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Taylor Blau, git, Elijah Newren, Jonathan Nieder, Derrick Stolee

On Tue, Apr 21, 2020 at 01:52:46PM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> writes:
>
> >> > @@ -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/.
>
> I did, and then I asked the question, because I couldn't quite get
> if JTan was asking a question similar to the one he asked earlier in
> the message ("do you need a reset in the "else" branch as well?"),
> or if he was saying what he sees there, "the opposite case", was
> good.

Sorry, I think that the linked message is confusing (at least, it
confused me the second time I read it because I wasn't sure which part
of the mail Jonathan was asking about: my patch, or his response to my
patch).

I think that he was referring to how I had it in the original patch I
sent in that thread, which was wrong. Based on my understanding of his
message, his recommendations match what I have in _this_ patch.

> Also, I was sort-of reacting to """In any case, I think the commit
> message should discuss why reset_repository_shallow() is added only
> on the unlink+rollback side in one "if" statement, but only on the
> opposite "commit" side in another "if" statement.""" in that
> message.

Is the description that I attached in the earlier patch sufficient? I
could certainly add more detail if it's not.

In either case, I'm sitting on another patch locally to improve the
style of the surrounding tests, which is done as a preparatory step
before this patch. I'll re-send after hearing back from you.

> Thanks.

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
  2020-04-21 22:21       ` Taylor Blau
@ 2020-04-21 23:06         ` Junio C Hamano
  0 siblings, 0 replies; 32+ messages in thread
From: Junio C Hamano @ 2020-04-21 23:06 UTC (permalink / raw)
  To: Taylor Blau; +Cc: git, Elijah Newren, Jonathan Nieder, Derrick Stolee

Taylor Blau <me@ttaylorr.com> writes:

> Sorry, I think that the linked message is confusing (at least, it
> confused me the second time I read it because I wasn't sure which part
> of the mail Jonathan was asking about: my patch, or his response to my
> patch).
>
> I think that he was referring to how I had it in the original patch I
> sent in that thread, which was wrong. Based on my understanding of his
> message, his recommendations match what I have in _this_ patch.

Thanks for a clarification.

> In either case, I'm sitting on another patch locally to improve the
> style of the surrounding tests, which is done as a preparatory step
> before this patch. I'll re-send after hearing back from you.

Alright.  Thanks.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
  2020-04-21 18:09 [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate Taylor Blau
  2020-04-21 20:41 ` Junio C Hamano
@ 2020-04-22 18:02 ` Jonathan Tan
  2020-04-22 18:15   ` Junio C Hamano
  1 sibling, 1 reply; 32+ messages in thread
From: Jonathan Tan @ 2020-04-22 18:02 UTC (permalink / raw)
  To: me; +Cc: git, newren, jrnieder, dstolee, Jonathan Tan

> 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.

From a cursory reading of the code, it seems that this happens in
fetch-pack and receive-pack. Looking at those files, I found some more
occasions when this happens. I have outlined them in the patch after the
scissors (I hope I used the scissors correctly).

Maybe instead of enumerating the cases (of which there are quite a few),
say that we do this when we unlink the shallow file, we modify or create
the shallow file, or when we change the value of alternate_shallow_file.

> @@ -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);
>  	}
>  	strbuf_release(&sb);
>  }

The "if" part (not quoted here) commits the shallow lock file, and thus
possibly modifies (or creates) the shallow file, so I think we need to
put reset_repository_shallow() outside the whole "if" block. I have done
that in the patch after the scissors.

> +test_expect_success 'fetch --update-shallow (with fetch.writeCommitGraph)' '
> +	(
> +	cd shallow &&
> +	git checkout master &&
> +	commit 8 &&
> +	git tag -m foo heavy-tag-for-graph HEAD^ &&
> +	git tag light-tag-for-graph HEAD^:tracked
> +	) &&
> +	(
> +	cd notshallow &&
> +	test_config fetch.writeCommitGraph true &&

When I patched onto master, this line causes the test to fail with a
warning that test_when_finished doesn't work in a subshell. I've
replaced it with a regular "git config" and it works.

Here is the patch containing what I tried. I think that most of the new
reset_repository_shallow() invocations don't change any functionality
(we don't usually read shallow files so many times in a process), so
they can't be tested, but I think that it's better to include them for
completeness, and to close the open question mentioned in bd0b42aed3
("fetch-pack: do not take shallow lock unnecessarily", 2019-01-10)
(about the full solution involving clearing shallow information whenever
we commit the shallow lock - we find here that the full solution
involves this and also clearing shallow information in other cases too).

-- 8< --
From 46a69a133db2e8e948d2bf296294656c9902e5ae Mon Sep 17 00:00:00 2001
From: Jonathan Tan <jonathantanmy@google.com>
Date: Wed, 22 Apr 2020 10:53:30 -0700
Subject: [PATCH] fixup

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/receive-pack.c   | 1 +
 fetch-pack.c             | 2 ++
 shallow.c                | 2 +-
 t/t5537-fetch-shallow.sh | 2 +-
 4 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 2cc18bbffd..d61cbf60e2 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -878,6 +878,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 	}
 
 	commit_lock_file(&shallow_lock);
+	reset_repository_shallow(the_repository);
 
 	/*
 	 * Make sure setup_alternate_shallow() for the next ref does
diff --git a/fetch-pack.c b/fetch-pack.c
index 684868bc17..9a1cec470c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1657,6 +1657,7 @@ static void update_shallow(struct fetch_pack_args *args,
 						&alternate_shallow_file,
 						&extra);
 			commit_lock_file(&shallow_lock);
+			reset_repository_shallow(the_repository);
 			alternate_shallow_file = NULL;
 		}
 		oid_array_clear(&extra);
@@ -1695,6 +1696,7 @@ static void update_shallow(struct fetch_pack_args *args,
 					&alternate_shallow_file,
 					&extra);
 		commit_lock_file(&shallow_lock);
+		reset_repository_shallow(the_repository);
 		oid_array_clear(&extra);
 		oid_array_clear(&ref);
 		alternate_shallow_file = NULL;
diff --git a/shallow.c b/shallow.c
index 9d1304e786..1a1ca71ffe 100644
--- a/shallow.c
+++ b/shallow.c
@@ -414,8 +414,8 @@ void prune_shallow(unsigned options)
 	} else {
 		unlink(git_path_shallow(the_repository));
 		rollback_lock_file(&shallow_lock);
-		reset_repository_shallow(the_repository);
 	}
+	reset_repository_shallow(the_repository);
 	strbuf_release(&sb);
 }
 
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index c9c731c7a9..c5c40fb8e7 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -187,7 +187,7 @@ test_expect_success 'fetch --update-shallow (with fetch.writeCommitGraph)' '
 	) &&
 	(
 	cd notshallow &&
-	test_config fetch.writeCommitGraph true &&
+	git config fetch.writeCommitGraph true &&
 	git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&
 	git fsck &&
 	git for-each-ref --sort=refname --format="%(refname)" >actual.refs &&
-- 
2.26.1.301.g55bc3eb7cb9-goog


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
  2020-04-21 20:52     ` Junio C Hamano
  2020-04-21 22:21       ` Taylor Blau
@ 2020-04-22 18:05       ` Jonathan Tan
  1 sibling, 0 replies; 32+ messages in thread
From: Jonathan Tan @ 2020-04-22 18:05 UTC (permalink / raw)
  To: gitster; +Cc: me, git, newren, jrnieder, dstolee, Jonathan Tan

> Taylor Blau <me@ttaylorr.com> writes:
> 
> >> > @@ -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/.
> 
> I did, and then I asked the question, because I couldn't quite get
> if JTan was asking a question similar to the one he asked earlier in
> the message ("do you need a reset in the "else" branch as well?"),
> or if he was saying what he sees there, "the opposite case", was
> good.

Sorry for not being clear. My intention was to ask a question similar to
the earlier one - in this case, and in the previous case, I think that
the reset should happen no matter whether we execute the "if" case or
the "else" case, so we should just put it right after the entire "if"
statement.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
  2020-04-22 18:02 ` Jonathan Tan
@ 2020-04-22 18:15   ` Junio C Hamano
  2020-04-23  0:14     ` Taylor Blau
  0 siblings, 1 reply; 32+ messages in thread
From: Junio C Hamano @ 2020-04-22 18:15 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: me, git, newren, jrnieder, dstolee

Jonathan Tan <jonathantanmy@google.com> writes:

>> @@ -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);
>>  	}
>>  	strbuf_release(&sb);
>>  }
>
> The "if" part (not quoted here) commits the shallow lock file, and thus
> possibly modifies (or creates) the shallow file, so I think we need to
> put reset_repository_shallow() outside the whole "if" block. I have done
> that in the patch after the scissors.

Is there any rollback_lock_file() or commit_lock_file() call on the
shallow lock file in the files involved in this patch that does not
need a call to reset_repository_shallow() left after your work?

What I am trying to get at is if it would be safer to have a pair of
thin wrapper for rolling back or committing a new version of new
shallow file, e.g. rollback_shallow_file() + commit_shallow_file(),
and replace calls to {rollback,commit}_lock_file() with calls to
them.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
  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 19:05       ` [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate Junio C Hamano
  0 siblings, 2 replies; 32+ messages in thread
From: Taylor Blau @ 2020-04-23  0:14 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jonathan Tan, me, git, newren, jrnieder, dstolee

On Wed, Apr 22, 2020 at 11:15:33AM -0700, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
> >> @@ -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);
> >>  	}
> >>  	strbuf_release(&sb);
> >>  }
> >
> > The "if" part (not quoted here) commits the shallow lock file, and thus
> > possibly modifies (or creates) the shallow file, so I think we need to
> > put reset_repository_shallow() outside the whole "if" block. I have done
> > that in the patch after the scissors.
>
> Is there any rollback_lock_file() or commit_lock_file() call on the
> shallow lock file in the files involved in this patch that does not
> need a call to reset_repository_shallow() left after your work?
>
> What I am trying to get at is if it would be safer to have a pair of
> thin wrapper for rolling back or committing a new version of new
> shallow file, e.g. rollback_shallow_file() + commit_shallow_file(),
> and replace calls to {rollback,commit}_lock_file() with calls to
> them.

Very elegant. Thanks for an excellent suggestion. v2 incoming just as
soon as 'make test' finishes...

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 0/2] shallow.c: reset shallow-ness after updating
  2020-04-23  0:14     ` Taylor Blau
@ 2020-04-23  0:25       ` 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  0:25         ` [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file' Taylor Blau
  2020-04-23 19:05       ` [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate Junio C Hamano
  1 sibling, 2 replies; 32+ messages in thread
From: Taylor Blau @ 2020-04-23  0:25 UTC (permalink / raw)
  To: git; +Cc: jonathantanmy, gitster, newren

Hi,

Here is an updated version of the patch that I sent above (which was
itself an updated version of a patch that I sent to some other
sub-thread...).

In this version, I added an extra patch at the beginning to clean up
some surrounding tests in t5537 so the new ones are added both in a good
style, and in a way that is consistent with the surrounding code.

I also adopted a suggestion from Junio that introduces
'{commit,rollback}_shallow_file()' as thin wrappers around
'{commit,rollback}_lock_file()', which makes this whole thing a lot
tidier.

Thanks in advance for another look...

Taylor Blau (2):
  t5537: use test_write_lines, indented heredocs for readability
  shallow.c: use '{commit,rollback}_shallow_file'

 builtin/receive-pack.c   |  4 +-
 commit.h                 |  2 +
 fetch-pack.c             | 10 ++--
 shallow.c                | 30 ++++++++----
 t/t5537-fetch-shallow.sh | 99 +++++++++++++++++++---------------------
 5 files changed, 77 insertions(+), 68 deletions(-)

--
2.26.0.113.ge9739cdccc

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 1/2] t5537: use test_write_lines, indented heredocs for readability
  2020-04-23  0:25       ` [PATCH v2 0/2] shallow.c: reset shallow-ness after updating Taylor Blau
@ 2020-04-23  0:25         ` Taylor Blau
  2020-04-23  1:14           ` Jonathan Nieder
  2020-04-23  0:25         ` [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file' Taylor Blau
  1 sibling, 1 reply; 32+ messages in thread
From: Taylor Blau @ 2020-04-23  0:25 UTC (permalink / raw)
  To: git; +Cc: jonathantanmy, gitster, newren

A number of spots in t5537 use the non-indented heredoc '<<EOF' when
they would benefit from instead using '<<-EOF' or simply
test_write_lines.

In preparation for adding new tests in a good style and being consistent
with the surrounding code, update the existing tests to improve their
readability.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5537-fetch-shallow.sh | 70 +++++++++++-----------------------------
 1 file changed, 18 insertions(+), 52 deletions(-)

diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 4f681dbbe1..d66b3656c0 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -25,10 +25,7 @@ test_expect_success 'setup' '
 test_expect_success 'setup shallow clone' '
 	git clone --no-local --depth=2 .git shallow &&
 	git --git-dir=shallow/.git log --format=%s >actual &&
-	cat <<EOF >expect &&
-4
-3
-EOF
+	test_write_lines 4 3 >expect &&
 	test_cmp expect actual
 '
 
@@ -38,10 +35,7 @@ test_expect_success 'clone from shallow clone' '
 	cd shallow2 &&
 	git fsck &&
 	git log --format=%s >actual &&
-	cat <<EOF >expect &&
-4
-3
-EOF
+	test_write_lines 4 3 >expect &&
 	test_cmp expect actual
 	)
 '
@@ -56,11 +50,7 @@ test_expect_success 'fetch from shallow clone' '
 	git fetch &&
 	git fsck &&
 	git log --format=%s origin/master >actual &&
-	cat <<EOF >expect &&
-5
-4
-3
-EOF
+	test_write_lines 5 4 3 >expect &&
 	test_cmp expect actual
 	)
 '
@@ -75,10 +65,7 @@ test_expect_success 'fetch --depth from shallow clone' '
 	git fetch --depth=2 &&
 	git fsck &&
 	git log --format=%s origin/master >actual &&
-	cat <<EOF >expect &&
-6
-5
-EOF
+	test_write_lines 6 5 >expect &&
 	test_cmp expect actual
 	)
 '
@@ -89,12 +76,7 @@ test_expect_success 'fetch --unshallow from shallow clone' '
 	git fetch --unshallow &&
 	git fsck &&
 	git log --format=%s origin/master >actual &&
-	cat <<EOF >expect &&
-6
-5
-4
-3
-EOF
+	test_write_lines 6 5 4 3 >expect &&
 	test_cmp expect actual
 	)
 '
@@ -111,15 +93,10 @@ test_expect_success 'fetch something upstream has but hidden by clients shallow
 	git fetch ../.git +refs/heads/master:refs/remotes/top/master &&
 	git fsck &&
 	git log --format=%s top/master >actual &&
-	cat <<EOF >expect &&
-add-1-back
-4
-3
-EOF
+	test_write_lines add-1-back 4 3 >expect &&
 	test_cmp expect actual
 	) &&
 	git --git-dir=shallow2/.git cat-file blob $(echo 1|git hash-object --stdin) >/dev/null
-
 '
 
 test_expect_success 'fetch that requires changes in .git/shallow is filtered' '
@@ -133,14 +110,12 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' '
 	cd notshallow &&
 	git fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/*&&
 	git for-each-ref --format="%(refname)" >actual.refs &&
-	cat <<EOF >expect.refs &&
-refs/remotes/shallow/no-shallow
-EOF
+	echo refs/remotes/shallow/no-shallow >expect.refs &&
 	test_cmp expect.refs actual.refs &&
 	git log --format=%s shallow/no-shallow >actual &&
-	cat <<EOF >expect &&
-no-shallow
-EOF
+	cat <<-EOF >expect &&
+	no-shallow
+	EOF
 	test_cmp expect actual
 	)
 '
@@ -158,21 +133,15 @@ test_expect_success 'fetch --update-shallow' '
 	git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&
 	git fsck &&
 	git for-each-ref --sort=refname --format="%(refname)" >actual.refs &&
-	cat <<EOF >expect.refs &&
-refs/remotes/shallow/master
-refs/remotes/shallow/no-shallow
-refs/tags/heavy-tag
-refs/tags/light-tag
-EOF
+	cat <<-EOF >expect.refs &&
+	refs/remotes/shallow/master
+	refs/remotes/shallow/no-shallow
+	refs/tags/heavy-tag
+	refs/tags/light-tag
+	EOF
 	test_cmp expect.refs actual.refs &&
 	git log --format=%s shallow/master >actual &&
-	cat <<EOF >expect &&
-7
-6
-5
-4
-3
-EOF
+	test_write_lines 7 6 5 4 3 >expect &&
 	test_cmp expect actual
 	)
 '
@@ -183,10 +152,7 @@ test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '
 	find read-only.git -print | xargs chmod -w &&
 	git clone --no-local --depth=2 read-only.git from-read-only &&
 	git --git-dir=from-read-only/.git log --format=%s >actual &&
-	cat >expect <<EOF &&
-add-1-back
-4
-EOF
+	test_write_lines add-1-back 4 >expect &&
 	test_cmp expect actual
 '
 
-- 
2.26.0.113.ge9739cdccc


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'
  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  0:25         ` Taylor Blau
  2020-04-23  1:23           ` Jonathan Nieder
                             ` (2 more replies)
  1 sibling, 3 replies; 32+ messages in thread
From: Taylor Blau @ 2020-04-23  0:25 UTC (permalink / raw)
  To: git; +Cc: jonathantanmy, gitster, newren

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 repository with 'fetch.writeCommitGraph' enabled, since the
update to '.git/shallow' will cause Git to think that the repository
isn't shallow when it is, thereby circumventing the commit-graph
compatibility 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).

Address this by introducing thin wrappers over 'commit_lock_file' and
'rollback_lock_file' for use specifically when the lock is held over
'.git/shallow'. These wrappers (appropriately called
'commit_shallow_file' and 'rollback_shallow_file') call into their
respective functions in 'lockfile.h', but additionally reset validity
checks used by the shallow machinery.

Replace each instance of 'commit_lock_file' and 'rollback_lock_file'
with 'commit_shallow_file' and 'rollback_shallow_file' when the lock
being held is over the '.git/shallow' file.

As a result, '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>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 builtin/receive-pack.c   |  4 ++--
 commit.h                 |  2 ++
 fetch-pack.c             | 10 +++++-----
 shallow.c                | 30 +++++++++++++++++++++---------
 t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++
 5 files changed, 59 insertions(+), 16 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 2cc18bbffd..652661fa99 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -872,12 +872,12 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
 	opt.env = tmp_objdir_env(tmp_objdir);
 	setup_alternate_shallow(&shallow_lock, &opt.shallow_file, &extra);
 	if (check_connected(command_singleton_iterator, cmd, &opt)) {
-		rollback_lock_file(&shallow_lock);
+		rollback_shallow_file(the_repository, &shallow_lock);
 		oid_array_clear(&extra);
 		return -1;
 	}
 
-	commit_lock_file(&shallow_lock);
+	commit_shallow_file(the_repository, &shallow_lock);
 
 	/*
 	 * Make sure setup_alternate_shallow() for the next ref does
diff --git a/commit.h b/commit.h
index 008a0fa4a0..ab91d21131 100644
--- a/commit.h
+++ b/commit.h
@@ -249,6 +249,8 @@ struct oid_array;
 struct ref;
 int register_shallow(struct repository *r, const struct object_id *oid);
 int unregister_shallow(const struct object_id *oid);
+int commit_shallow_file(struct repository *r, struct lock_file *lk);
+void rollback_shallow_file(struct repository *r, struct lock_file *lk);
 int for_each_commit_graft(each_commit_graft_fn, void *);
 int is_repository_shallow(struct repository *r);
 struct commit_list *get_shallow_commits(struct object_array *heads,
diff --git a/fetch-pack.c b/fetch-pack.c
index 1734a573b0..a618f3b029 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1629,9 +1629,9 @@ static void update_shallow(struct fetch_pack_args *args,
 	if (args->deepen && alternate_shallow_file) {
 		if (*alternate_shallow_file == '\0') { /* --unshallow */
 			unlink_or_warn(git_path_shallow(the_repository));
-			rollback_lock_file(&shallow_lock);
+			rollback_shallow_file(the_repository, &shallow_lock);
 		} else
-			commit_lock_file(&shallow_lock);
+			commit_shallow_file(the_repository, &shallow_lock);
 		alternate_shallow_file = NULL;
 		return;
 	}
@@ -1655,7 +1655,7 @@ static void update_shallow(struct fetch_pack_args *args,
 			setup_alternate_shallow(&shallow_lock,
 						&alternate_shallow_file,
 						&extra);
-			commit_lock_file(&shallow_lock);
+			commit_shallow_file(the_repository, &shallow_lock);
 			alternate_shallow_file = NULL;
 		}
 		oid_array_clear(&extra);
@@ -1693,7 +1693,7 @@ static void update_shallow(struct fetch_pack_args *args,
 		setup_alternate_shallow(&shallow_lock,
 					&alternate_shallow_file,
 					&extra);
-		commit_lock_file(&shallow_lock);
+		commit_shallow_file(the_repository, &shallow_lock);
 		oid_array_clear(&extra);
 		oid_array_clear(&ref);
 		alternate_shallow_file = NULL;
@@ -1785,7 +1785,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
 			error(_("remote did not send all necessary objects"));
 			free_refs(ref_cpy);
 			ref_cpy = NULL;
-			rollback_lock_file(&shallow_lock);
+			rollback_shallow_file(the_repository, &shallow_lock);
 			goto cleanup;
 		}
 		args->connectivity_checked = 1;
diff --git a/shallow.c b/shallow.c
index 7fd04afed1..5010a6c732 100644
--- a/shallow.c
+++ b/shallow.c
@@ -40,13 +40,6 @@ int register_shallow(struct repository *r, const struct object_id *oid)
 
 int is_repository_shallow(struct repository *r)
 {
-	/*
-	 * NEEDSWORK: This function updates
-	 * r->parsed_objects->{is_shallow,shallow_stat} as a side effect but
-	 * there is no corresponding function to clear them when the shallow
-	 * file is updated.
-	 */
-
 	FILE *fp;
 	char buf[1024];
 	const char *path = r->parsed_objects->alternate_shallow_file;
@@ -79,6 +72,25 @@ int is_repository_shallow(struct repository *r)
 	return r->parsed_objects->is_shallow;
 }
 
+static void reset_repository_shallow(struct repository *r)
+{
+	r->parsed_objects->is_shallow = -1;
+	stat_validity_clear(r->parsed_objects->shallow_stat);
+}
+
+int commit_shallow_file(struct repository *r, struct lock_file *lk)
+{
+	int res = commit_lock_file(lk);
+	reset_repository_shallow(r);
+	return res;
+}
+
+void rollback_shallow_file(struct repository *r, struct lock_file *lk)
+{
+	rollback_lock_file(lk);
+	reset_repository_shallow(r);
+}
+
 /*
  * TODO: use "int" elemtype instead of "int *" when/if commit-slab
  * supports a "valid" flag.
@@ -410,10 +422,10 @@ void prune_shallow(unsigned options)
 		if (write_in_full(fd, sb.buf, sb.len) < 0)
 			die_errno("failed to write to %s",
 				  get_lock_file_path(&shallow_lock));
-		commit_lock_file(&shallow_lock);
+		commit_shallow_file(the_repository, &shallow_lock);
 	} else {
 		unlink(git_path_shallow(the_repository));
-		rollback_lock_file(&shallow_lock);
+		rollback_shallow_file(the_repository, &shallow_lock);
 	}
 	strbuf_release(&sb);
 }
diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index d66b3656c0..a51c4b39d9 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -146,6 +146,35 @@ test_expect_success 'fetch --update-shallow' '
 	)
 '
 
+test_expect_success 'fetch --update-shallow (with fetch.writeCommitGraph)' '
+	(
+	cd shallow &&
+	git checkout master &&
+	commit 8 &&
+	git tag -m foo heavy-tag-for-graph HEAD^ &&
+	git tag light-tag-for-graph HEAD^:tracked
+	) &&
+	test_config -C notshallow fetch.writeCommitGraph true &&
+	(
+	cd notshallow &&
+	git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&
+	git fsck &&
+	git for-each-ref --sort=refname --format="%(refname)" >actual.refs &&
+	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
+	test_cmp expect.refs actual.refs &&
+	git log --format=%s shallow/master >actual &&
+	test_write_lines 8 7 6 5 4 3 >expect &&
+	test_cmp expect actual
+	)
+'
+
 test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '
 	cp -R .git read-only.git &&
 	test_when_finished "find read-only.git -type d -print | xargs chmod +w" &&
-- 
2.26.0.113.ge9739cdccc

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 1/2] t5537: use test_write_lines, indented heredocs for readability
  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
  0 siblings, 1 reply; 32+ messages in thread
From: Jonathan Nieder @ 2020-04-23  1:14 UTC (permalink / raw)
  To: Taylor Blau; +Cc: git, jonathantanmy, gitster, newren

Hi,

Taylor Blau wrote:

> A number of spots in t5537 use the non-indented heredoc '<<EOF' when
> they would benefit from instead using '<<-EOF' or simply
> test_write_lines.
>
> In preparation for adding new tests in a good style and being consistent
> with the surrounding code, update the existing tests to improve their
> readability.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  t/t5537-fetch-shallow.sh | 70 +++++++++++-----------------------------
>  1 file changed, 18 insertions(+), 52 deletions(-)

Sounds like a good idea.  Some nitpicks --- please don't act on them
all, but only the ones that seem appropriate to you:

[...]
> +++ b/t/t5537-fetch-shallow.sh
> @@ -25,10 +25,7 @@ test_expect_success 'setup' '
>  test_expect_success 'setup shallow clone' '
>  	git clone --no-local --depth=2 .git shallow &&
>  	git --git-dir=shallow/.git log --format=%s >actual &&
> -	cat <<EOF >expect &&
> -4
> -3
> -EOF
> +	test_write_lines 4 3 >expect &&
>  	test_cmp expect actual
>  '

Nice.

[...]
> @@ -133,14 +110,12 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' '
[...]
> -	cat <<EOF >expect &&
> -no-shallow
> -EOF
> +	cat <<-EOF >expect &&
> +	no-shallow
> +	EOF

Can this use "echo"?  Or if using cat, please quote the EOF in <<-EOF
so the reader doesn't have to check for $substitutions in the body:

		cat >expect <<-\EOF &&

[...]
> @@ -158,21 +133,15 @@ test_expect_success 'fetch --update-shallow' '
>  	git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&
>  	git fsck &&
>  	git for-each-ref --sort=refname --format="%(refname)" >actual.refs &&
> -	cat <<EOF >expect.refs &&
> -refs/remotes/shallow/master
> -refs/remotes/shallow/no-shallow
> -refs/tags/heavy-tag
> -refs/tags/light-tag
> -EOF
> +	cat <<-EOF >expect.refs &&

Likewise (missing \ before EOF).

A few more nits, that probably don't belong in the same patch:

- the code in subshells would be more readable if indented
- existing <<-EOF here blocks should \quote the EOF
- the resulting history would be more realistic if it uses test_tick
  before running "git commit".  Or perhaps this can use the
  test_commit helper to handle that
- should use test_must_fail in preference to ! git
- might be simpler if http tests go in a different file

Thanks,
Jonathan

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'
  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-06-03  3:42           ` Jonathan Nieder
  2 siblings, 0 replies; 32+ messages in thread
From: Jonathan Nieder @ 2020-04-23  1:23 UTC (permalink / raw)
  To: Taylor Blau; +Cc: git, jonathantanmy, gitster, newren

Taylor Blau wrote:

> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -872,12 +872,12 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
>  	opt.env = tmp_objdir_env(tmp_objdir);
>  	setup_alternate_shallow(&shallow_lock, &opt.shallow_file, &extra);
>  	if (check_connected(command_singleton_iterator, cmd, &opt)) {
> -		rollback_lock_file(&shallow_lock);
> +		rollback_shallow_file(the_repository, &shallow_lock);

I like it.

I wonder, is there a way we can make it more difficult to accidentally
use rollback_lock_file where rollback_shallow_file is needed?  For
example, what if shallow_lock has a different type "struct
shallow_lock" so one would have to reach in to its lock_file member to
bypass the shallow_file interface?

[...]
>  		oid_array_clear(&extra);
>  		return -1;
>  	}
>  
> -	commit_lock_file(&shallow_lock);
> +	commit_shallow_file(the_repository, &shallow_lock);
>  
>  	/*
>  	 * Make sure setup_alternate_shallow() for the next ref does
> diff --git a/commit.h b/commit.h
> index 008a0fa4a0..ab91d21131 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -249,6 +249,8 @@ struct oid_array;
>  struct ref;
>  int register_shallow(struct repository *r, const struct object_id *oid);
>  int unregister_shallow(const struct object_id *oid);
> +int commit_shallow_file(struct repository *r, struct lock_file *lk);
> +void rollback_shallow_file(struct repository *r, struct lock_file *lk);

optional: might make sense to put this near setup_alternate_shallow
for discoverability

Could this have an API doc comment?

[...]
> --- a/shallow.c
> +++ b/shallow.c
> @@ -40,13 +40,6 @@ int register_shallow(struct repository *r, const struct object_id *oid)
>  
>  int is_repository_shallow(struct repository *r)

Not about this patch: it might make sense to split out a shallow.h
header / API.

Thanks and hope that helps,
Jonathan

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'
  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-06-03  3:42           ` Jonathan Nieder
  2 siblings, 1 reply; 32+ messages in thread
From: Jonathan Tan @ 2020-04-23 18:09 UTC (permalink / raw)
  To: me; +Cc: git, jonathantanmy, gitster, newren

> Replace each instance of 'commit_lock_file' and 'rollback_lock_file'
> with 'commit_shallow_file' and 'rollback_shallow_file' when the lock
> being held is over the '.git/shallow' file.

I think Jonathan Nieder already covered 1/2 so I'll just close the loop
on this patch. There was one potential issue in that a previous version
of this patch also called reset_repository_shallow() in
setup_alternate_shallow(), but this version does not. But after looking
into it, this looks fine - setup_alternate_shallow() deals with a
passed-in alternate_shallow_file variable, which is different from the
r->parsed_objects->alternate_shallow_file that is_repository_shallow()
uses to set the global variables. (I might have confused the two during
earlier reviews.) Also, setup_alternate_shallow() is called either
before any shallow processing (empirically demonstrating that no
resetting is needed in this case, because it has been working), or right
before a commit or rollback of the lock file (so the global variables
are being reset anyway, so we do not need to call
reset_repository_shallow()).

So,
Reviewed-by: Jonathan Tan <jonathantanmy@google.com>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate
  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 19:05       ` Junio C Hamano
  1 sibling, 0 replies; 32+ messages in thread
From: Junio C Hamano @ 2020-04-23 19:05 UTC (permalink / raw)
  To: Taylor Blau; +Cc: Jonathan Tan, git, newren, jrnieder, dstolee

Taylor Blau <me@ttaylorr.com> writes:

>> What I am trying to get at is if it would be safer to have a pair of
>> thin wrapper for rolling back or committing a new version of new
>> shallow file, e.g. rollback_shallow_file() + commit_shallow_file(),
>> and replace calls to {rollback,commit}_lock_file() with calls to
>> them.
>
> Very elegant. Thanks for an excellent suggestion. v2 incoming just as
> soon as 'make test' finishes...

Note that I didn't verify there is a case where we want not to call
reset_repository_shallow() after committing or rolling back, either
for performance or correctness purposes.  As long as the experts on
the codepaths involved are happy with the idea, I'd be happy, too.

JNieder raised the idea of using a different type to avoid calling
the bare rollback/commit functions by mistake.  It appears that, in
addition to these two functions, setup_alternate_shallow() needs to
be updated if we wanted to go that route, and it sounds like a good
idea to gain safety with minimal cost.

Thanks.



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'
  2020-04-23 18:09           ` Jonathan Tan
@ 2020-04-23 20:40             ` Junio C Hamano
  2020-04-24 17:13               ` Taylor Blau
  0 siblings, 1 reply; 32+ messages in thread
From: Junio C Hamano @ 2020-04-23 20:40 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: me, git, newren

Jonathan Tan <jonathantanmy@google.com> writes:

>> Replace each instance of 'commit_lock_file' and 'rollback_lock_file'
>> with 'commit_shallow_file' and 'rollback_shallow_file' when the lock
>> being held is over the '.git/shallow' file.
>
> I think Jonathan Nieder already covered 1/2 so I'll just close the loop
> on this patch. There was one potential issue in that a previous version
> of this patch also called reset_repository_shallow() in
> setup_alternate_shallow(), but this version does not. But after looking
> into it, this looks fine - setup_alternate_shallow() deals with a
> passed-in alternate_shallow_file variable, which is different from the
> r->parsed_objects->alternate_shallow_file that is_repository_shallow()
> uses to set the global variables. (I might have confused the two during
> earlier reviews.) Also, setup_alternate_shallow() is called either
> before any shallow processing (empirically demonstrating that no
> resetting is needed in this case, because it has been working), or right
> before a commit or rollback of the lock file (so the global variables
> are being reset anyway, so we do not need to call
> reset_repository_shallow()).
>
> So,
> Reviewed-by: Jonathan Tan <jonathantanmy@google.com>

Thanks for a review.

And of course, thanks Taylor.  Will queue.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 1/2] t5537: use test_write_lines, indented heredocs for readability
  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
  0 siblings, 2 replies; 32+ messages in thread
From: Taylor Blau @ 2020-04-24 17:11 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Taylor Blau, git, jonathantanmy, gitster, newren

On Wed, Apr 22, 2020 at 06:14:44PM -0700, Jonathan Nieder wrote:
> Hi,
>
> Taylor Blau wrote:
>
> > A number of spots in t5537 use the non-indented heredoc '<<EOF' when
> > they would benefit from instead using '<<-EOF' or simply
> > test_write_lines.
> >
> > In preparation for adding new tests in a good style and being consistent
> > with the surrounding code, update the existing tests to improve their
> > readability.
> >
> > Suggested-by: Junio C Hamano <gitster@pobox.com>
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> >  t/t5537-fetch-shallow.sh | 70 +++++++++++-----------------------------
> >  1 file changed, 18 insertions(+), 52 deletions(-)
>
> Sounds like a good idea.  Some nitpicks --- please don't act on them
> all, but only the ones that seem appropriate to you:

Thanks -- here's an updated venison of this patch with some of the more
minor points addressed. I agree that there is room for cleaning up these
tests more substantially, but I think that those can be done in other
patches.

Junio -- would you please used the patch below as PATCH 1/2 instead of
the original here? Thanks.

-- 8< --

Subject: [PATCH] t5537: use test_write_lines, indented heredocs for readability

A number of spots in t5537 use the non-indented heredoc '<<EOF' when
they would benefit from instead using '<<-EOF' or simply
test_write_lines.

In preparation for adding new tests in a good style and being consistent
with the surrounding code, update the existing tests to improve their
readability.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5537-fetch-shallow.sh | 70 ++++++++++------------------------------
 1 file changed, 17 insertions(+), 53 deletions(-)

diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
index 4f681dbbe1..d02f9b5ae8 100755
--- a/t/t5537-fetch-shallow.sh
+++ b/t/t5537-fetch-shallow.sh
@@ -16,7 +16,7 @@ test_expect_success 'setup' '
 	commit 3 &&
 	commit 4 &&
 	git config --global transfer.fsckObjects true &&
-	test_oid_cache <<-EOF
+	test_oid_cache <<-\EOF
 	perl sha1:s/0034shallow %s/0036unshallow %s/
 	perl sha256:s/004cshallow %s/004eunshallow %s/
 	EOF
@@ -25,10 +25,7 @@ test_expect_success 'setup' '
 test_expect_success 'setup shallow clone' '
 	git clone --no-local --depth=2 .git shallow &&
 	git --git-dir=shallow/.git log --format=%s >actual &&
-	cat <<EOF >expect &&
-4
-3
-EOF
+	test_write_lines 4 3 >expect &&
 	test_cmp expect actual
 '

@@ -38,10 +35,7 @@ test_expect_success 'clone from shallow clone' '
 	cd shallow2 &&
 	git fsck &&
 	git log --format=%s >actual &&
-	cat <<EOF >expect &&
-4
-3
-EOF
+	test_write_lines 4 3 >expect &&
 	test_cmp expect actual
 	)
 '
@@ -56,11 +50,7 @@ test_expect_success 'fetch from shallow clone' '
 	git fetch &&
 	git fsck &&
 	git log --format=%s origin/master >actual &&
-	cat <<EOF >expect &&
-5
-4
-3
-EOF
+	test_write_lines 5 4 3 >expect &&
 	test_cmp expect actual
 	)
 '
@@ -75,10 +65,7 @@ test_expect_success 'fetch --depth from shallow clone' '
 	git fetch --depth=2 &&
 	git fsck &&
 	git log --format=%s origin/master >actual &&
-	cat <<EOF >expect &&
-6
-5
-EOF
+	test_write_lines 6 5 >expect &&
 	test_cmp expect actual
 	)
 '
@@ -89,12 +76,7 @@ test_expect_success 'fetch --unshallow from shallow clone' '
 	git fetch --unshallow &&
 	git fsck &&
 	git log --format=%s origin/master >actual &&
-	cat <<EOF >expect &&
-6
-5
-4
-3
-EOF
+	test_write_lines 6 5 4 3 >expect &&
 	test_cmp expect actual
 	)
 '
@@ -111,15 +93,10 @@ test_expect_success 'fetch something upstream has but hidden by clients shallow
 	git fetch ../.git +refs/heads/master:refs/remotes/top/master &&
 	git fsck &&
 	git log --format=%s top/master >actual &&
-	cat <<EOF >expect &&
-add-1-back
-4
-3
-EOF
+	test_write_lines add-1-back 4 3 >expect &&
 	test_cmp expect actual
 	) &&
 	git --git-dir=shallow2/.git cat-file blob $(echo 1|git hash-object --stdin) >/dev/null
-
 '

 test_expect_success 'fetch that requires changes in .git/shallow is filtered' '
@@ -133,14 +110,10 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' '
 	cd notshallow &&
 	git fetch ../shallow/.git refs/heads/*:refs/remotes/shallow/*&&
 	git for-each-ref --format="%(refname)" >actual.refs &&
-	cat <<EOF >expect.refs &&
-refs/remotes/shallow/no-shallow
-EOF
+	echo refs/remotes/shallow/no-shallow >expect.refs &&
 	test_cmp expect.refs actual.refs &&
 	git log --format=%s shallow/no-shallow >actual &&
-	cat <<EOF >expect &&
-no-shallow
-EOF
+	echo no-shallow >expect &&
 	test_cmp expect actual
 	)
 '
@@ -158,21 +131,15 @@ test_expect_success 'fetch --update-shallow' '
 	git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&
 	git fsck &&
 	git for-each-ref --sort=refname --format="%(refname)" >actual.refs &&
-	cat <<EOF >expect.refs &&
-refs/remotes/shallow/master
-refs/remotes/shallow/no-shallow
-refs/tags/heavy-tag
-refs/tags/light-tag
-EOF
+	cat <<-\EOF >expect.refs &&
+	refs/remotes/shallow/master
+	refs/remotes/shallow/no-shallow
+	refs/tags/heavy-tag
+	refs/tags/light-tag
+	EOF
 	test_cmp expect.refs actual.refs &&
 	git log --format=%s shallow/master >actual &&
-	cat <<EOF >expect &&
-7
-6
-5
-4
-3
-EOF
+	test_write_lines 7 6 5 4 3 >expect &&
 	test_cmp expect actual
 	)
 '
@@ -183,10 +150,7 @@ test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '
 	find read-only.git -print | xargs chmod -w &&
 	git clone --no-local --depth=2 read-only.git from-read-only &&
 	git --git-dir=from-read-only/.git log --format=%s >actual &&
-	cat >expect <<EOF &&
-add-1-back
-4
-EOF
+	test_write_lines add-1-back 4 >expect &&
 	test_cmp expect actual
 '

--
2.26.0.113.ge9739cdccc


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'
  2020-04-23 20:40             ` Junio C Hamano
@ 2020-04-24 17:13               ` Taylor Blau
  0 siblings, 0 replies; 32+ messages in thread
From: Taylor Blau @ 2020-04-24 17:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jonathan Tan, me, git, newren

On Thu, Apr 23, 2020 at 01:40:47PM -0700, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
> >> Replace each instance of 'commit_lock_file' and 'rollback_lock_file'
> >> with 'commit_shallow_file' and 'rollback_shallow_file' when the lock
> >> being held is over the '.git/shallow' file.
> >
> > I think Jonathan Nieder already covered 1/2 so I'll just close the loop
> > on this patch. There was one potential issue in that a previous version
> > of this patch also called reset_repository_shallow() in
> > setup_alternate_shallow(), but this version does not. But after looking
> > into it, this looks fine - setup_alternate_shallow() deals with a
> > passed-in alternate_shallow_file variable, which is different from the
> > r->parsed_objects->alternate_shallow_file that is_repository_shallow()
> > uses to set the global variables. (I might have confused the two during
> > earlier reviews.) Also, setup_alternate_shallow() is called either
> > before any shallow processing (empirically demonstrating that no
> > resetting is needed in this case, because it has been working), or right
> > before a commit or rollback of the lock file (so the global variables
> > are being reset anyway, so we do not need to call
> > reset_repository_shallow()).
> >
> > So,
> > Reviewed-by: Jonathan Tan <jonathantanmy@google.com>
>
> Thanks for a review.
>
> And of course, thanks Taylor.  Will queue.

Thanks, both. I'll send some more patches on top to introduce a
'shallow_lock' type.

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 1/2] t5537: use test_write_lines, indented heredocs for readability
  2020-04-24 17:11             ` Taylor Blau
@ 2020-04-24 17:17               ` Jonathan Nieder
  2020-04-24 20:45               ` Junio C Hamano
  1 sibling, 0 replies; 32+ messages in thread
From: Jonathan Nieder @ 2020-04-24 17:17 UTC (permalink / raw)
  To: Taylor Blau; +Cc: git, jonathantanmy, gitster, newren

Taylor Blau wrote:
> Subject: [PATCH] t5537: use test_write_lines, indented heredocs for readability
>
> A number of spots in t5537 use the non-indented heredoc '<<EOF' when
> they would benefit from instead using '<<-EOF' or simply
> test_write_lines.
>
> In preparation for adding new tests in a good style and being consistent
> with the surrounding code, update the existing tests to improve their
> readability.
>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  t/t5537-fetch-shallow.sh | 70 ++++++++++------------------------------
>  1 file changed, 17 insertions(+), 53 deletions(-)

The commit message is missing the mention of \EOF quoting.

See "git log --grep='modernize style'" for some examples about how to
write this kind of commit message without it getting tedious.

That said, I think it's fine --- just mentioning that for the future.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 1/2] t5537: use test_write_lines, indented heredocs for readability
  2020-04-24 17:11             ` Taylor Blau
  2020-04-24 17:17               ` Jonathan Nieder
@ 2020-04-24 20:45               ` Junio C Hamano
  1 sibling, 0 replies; 32+ messages in thread
From: Junio C Hamano @ 2020-04-24 20:45 UTC (permalink / raw)
  To: Taylor Blau; +Cc: Jonathan Nieder, git, jonathantanmy, newren

Taylor Blau <me@ttaylorr.com> writes:

> Subject: [PATCH] t5537: use test_write_lines, indented heredocs for readability

Assuming that "indented" is not a verb in the past tense, but
adjective on the noun "heredocs", this reads well, but "s/,/ and/"
would make it more clear in that case.  Or "for readability, use X
and indent Y" would work equally well.

But the original is probably OK as-is.

> -	cat <<EOF >expect &&
> -4
> -3
> -EOF
> +	test_write_lines 4 3 >expect &&

Yeah, becomes much nicer with test_write_lines.

Thanks.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'
  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-06-03  3:42           ` Jonathan Nieder
  2020-06-03  4:52             ` Taylor Blau
  2 siblings, 1 reply; 32+ messages in thread
From: Jonathan Nieder @ 2020-06-03  3:42 UTC (permalink / raw)
  To: Taylor Blau
  Cc: git, jonathantanmy, gitster, newren, Jay Conrod, Derrick Stolee

Hi,

Taylor Blau wrote:

> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  builtin/receive-pack.c   |  4 ++--
>  commit.h                 |  2 ++
>  fetch-pack.c             | 10 +++++-----
>  shallow.c                | 30 +++++++++++++++++++++---------
>  t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++
>  5 files changed, 59 insertions(+), 16 deletions(-)

I haven't investigated the cause yet, but I've run into an interesting
bug that bisects to this commit.  Jay Conrod (cc-ed) reports:

| I believe this is also the cause of Go toolchain test failures we've
| been seeing. Go uses git to fetch dependencies.
|
| The problem we're seeing can be reproduced with the script below. It
| should print "success". Instead, the git merge-base command fails
| because the commit 7303f77963648d5f1ec5e55eccfad8e14035866c
| (origin/master) has no history.

-- 8< --
#!/bin/bash

set -euxo pipefail
if [ -d legacytest ]; then
  echo "legacytest directory already exists" >&2
  exit 1
fi
mkdir legacytest
cd legacytest
git init --bare
git config protocol.version 2
git config fetch.writeCommitGraph true
git remote add origin -- https://github.com/rsc/legacytest
git fetch -f --depth=1 origin refs/heads/master:refs/heads/master
git fetch -f origin 'refs/heads/*:refs/heads/*' 'refs/tags/*:refs/tags/*'
git fetch --unshallow -f origin
git merge-base --is-ancestor -- v2.0.0 7303f77963648d5f1ec5e55eccfad8e14035866c
echo success
-- >8 --

The fetch.writeCommitGraph part is interesting.  When does a commit
graph file get written in this sequence of operations?  In an
unshallow operation, does the usual guard against writing a commit
graph in a shallow repo get missed?

"rm -fr objects/info/commit-graphs" recovers the full history in the
repo, so this is not a case of writing the wrong shallows --- it's
only a commit graph issue.

I'll take a closer look, but thought I'd give others a chance to look
to in case there's something obvious. :)

Thanks,
Jonathan

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'
  2020-06-03  3:42           ` Jonathan Nieder
@ 2020-06-03  4:52             ` Taylor Blau
  2020-06-03  5:16               ` Taylor Blau
  0 siblings, 1 reply; 32+ messages in thread
From: Taylor Blau @ 2020-06-03  4:52 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Taylor Blau, git, jonathantanmy, gitster, newren, Jay Conrod,
	Derrick Stolee

Hi Jonathan,

On Tue, Jun 02, 2020 at 08:42:13PM -0700, Jonathan Nieder wrote:
> Hi,
>
> Taylor Blau wrote:
>
> > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > ---
> >  builtin/receive-pack.c   |  4 ++--
> >  commit.h                 |  2 ++
> >  fetch-pack.c             | 10 +++++-----
> >  shallow.c                | 30 +++++++++++++++++++++---------
> >  t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++
> >  5 files changed, 59 insertions(+), 16 deletions(-)
>
> I haven't investigated the cause yet, but I've run into an interesting
> bug that bisects to this commit.  Jay Conrod (cc-ed) reports:
>
> | I believe this is also the cause of Go toolchain test failures we've
> | been seeing. Go uses git to fetch dependencies.
> |
> | The problem we're seeing can be reproduced with the script below. It
> | should print "success". Instead, the git merge-base command fails
> | because the commit 7303f77963648d5f1ec5e55eccfad8e14035866c
> | (origin/master) has no history.
>
> -- 8< --
> #!/bin/bash
>
> set -euxo pipefail
> if [ -d legacytest ]; then
>   echo "legacytest directory already exists" >&2
>   exit 1
> fi
> mkdir legacytest
> cd legacytest
> git init --bare
> git config protocol.version 2
> git config fetch.writeCommitGraph true
> git remote add origin -- https://github.com/rsc/legacytest
> git fetch -f --depth=1 origin refs/heads/master:refs/heads/master
> git fetch -f origin 'refs/heads/*:refs/heads/*' 'refs/tags/*:refs/tags/*'
> git fetch --unshallow -f origin
> git merge-base --is-ancestor -- v2.0.0 7303f77963648d5f1ec5e55eccfad8e14035866c
> echo success
> -- >8 --

Thanks to you and Jay for the report and reproduction script. Indeed, I
can reproduce this on the tip of master (which is equivalent to v2.27.0
at the time of writing).

> The fetch.writeCommitGraph part is interesting.  When does a commit
> graph file get written in this sequence of operations?  In an
> unshallow operation, does the usual guard against writing a commit
> graph in a shallow repo get missed?

The last 'git fetch' is the one that writes the commit-graph. You can
verify this by sticking a 'ls objects/info' after each 'git' invocation
in your script.

Here's where things get weird, though. Prior to this patch, Git would
pick up that the repository is shallow before unshallowing, but never
invalidate this fact after unshallowing. That means that once we got to
'write_commit_graph', we'd exit immediately since it appears as if the
repository is shallow.

In this patch, we don't do that anymore, since we rightly unset the fact
that we are (were) shallow.

In a debugger, I ran your script and a 'git commit-graph write --split
--reachable' side-by-side, and found an interesting discrepancy: some
commits (loaded from 'copy_oids_to_commits') *don't* have their parents
set when invoked from 'git fetch', but *do* when invoked as 'git
commit-graph write ...'.

I'm not an expert in the object cache, but my hunch is that when we
fetch these objects they're marked as parsed without having loaded their
parents. When we load them again via 'lookup_object', we get objects
that look parsed, but don't have parents where they otherwise should.

I'm going to CC Stolee to see if he has any thoughts on how to handle
this and/or if my idea is on the right track.

> "rm -fr objects/info/commit-graphs" recovers the full history in the
> repo, so this is not a case of writing the wrong shallows --- it's
> only a commit graph issue.
>
> I'll take a closer look, but thought I'd give others a chance to look
> to in case there's something obvious. :)
>
> Thanks,
> Jonathan

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'
  2020-06-03  4:52             ` Taylor Blau
@ 2020-06-03  5:16               ` Taylor Blau
  2020-06-03 13:08                 ` Derrick Stolee
  2020-06-03 20:51                 ` Jonathan Nieder
  0 siblings, 2 replies; 32+ messages in thread
From: Taylor Blau @ 2020-06-03  5:16 UTC (permalink / raw)
  To: Taylor Blau
  Cc: Jonathan Nieder, git, jonathantanmy, gitster, newren, Jay Conrod,
	Derrick Stolee

On Tue, Jun 02, 2020 at 10:52:48PM -0600, Taylor Blau wrote:
> Hi Jonathan,
>
> On Tue, Jun 02, 2020 at 08:42:13PM -0700, Jonathan Nieder wrote:
> > Hi,
> >
> > Taylor Blau wrote:
> >
> > > Signed-off-by: Taylor Blau <me@ttaylorr.com>
> > > ---
> > >  builtin/receive-pack.c   |  4 ++--
> > >  commit.h                 |  2 ++
> > >  fetch-pack.c             | 10 +++++-----
> > >  shallow.c                | 30 +++++++++++++++++++++---------
> > >  t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++
> > >  5 files changed, 59 insertions(+), 16 deletions(-)
> >
> > I haven't investigated the cause yet, but I've run into an interesting
> > bug that bisects to this commit.  Jay Conrod (cc-ed) reports:
> >
> > | I believe this is also the cause of Go toolchain test failures we've
> > | been seeing. Go uses git to fetch dependencies.
> > |
> > | The problem we're seeing can be reproduced with the script below. It
> > | should print "success". Instead, the git merge-base command fails
> > | because the commit 7303f77963648d5f1ec5e55eccfad8e14035866c
> > | (origin/master) has no history.
> >
> > -- 8< --
> > #!/bin/bash
> >
> > set -euxo pipefail
> > if [ -d legacytest ]; then
> >   echo "legacytest directory already exists" >&2
> >   exit 1
> > fi
> > mkdir legacytest
> > cd legacytest
> > git init --bare
> > git config protocol.version 2
> > git config fetch.writeCommitGraph true
> > git remote add origin -- https://github.com/rsc/legacytest
> > git fetch -f --depth=1 origin refs/heads/master:refs/heads/master
> > git fetch -f origin 'refs/heads/*:refs/heads/*' 'refs/tags/*:refs/tags/*'
> > git fetch --unshallow -f origin
> > git merge-base --is-ancestor -- v2.0.0 7303f77963648d5f1ec5e55eccfad8e14035866c
> > echo success
> > -- >8 --
>
> Thanks to you and Jay for the report and reproduction script. Indeed, I
> can reproduce this on the tip of master (which is equivalent to v2.27.0
> at the time of writing).
>
> > The fetch.writeCommitGraph part is interesting.  When does a commit
> > graph file get written in this sequence of operations?  In an
> > unshallow operation, does the usual guard against writing a commit
> > graph in a shallow repo get missed?
>
> The last 'git fetch' is the one that writes the commit-graph. You can
> verify this by sticking a 'ls objects/info' after each 'git' invocation
> in your script.
>
> Here's where things get weird, though. Prior to this patch, Git would
> pick up that the repository is shallow before unshallowing, but never
> invalidate this fact after unshallowing. That means that once we got to
> 'write_commit_graph', we'd exit immediately since it appears as if the
> repository is shallow.
>
> In this patch, we don't do that anymore, since we rightly unset the fact
> that we are (were) shallow.
>
> In a debugger, I ran your script and a 'git commit-graph write --split
> --reachable' side-by-side, and found an interesting discrepancy: some
> commits (loaded from 'copy_oids_to_commits') *don't* have their parents
> set when invoked from 'git fetch', but *do* when invoked as 'git
> commit-graph write ...'.
>
> I'm not an expert in the object cache, but my hunch is that when we
> fetch these objects they're marked as parsed without having loaded their
> parents. When we load them again via 'lookup_object', we get objects
> that look parsed, but don't have parents where they otherwise should.

Ah, this only sort of has to do with the object cache. In
'parse_commit_buffer()' we stop parsing parents in the case that the
repository is shallow (this goes back to 7f3140cd23 (git repack: keep
commits hidden by a graft, 2009-07-23)).

That makes me somewhat nervous. We're going to keep any objects opened
prior to unshallowing in the cache, along with their hidden parents. I
suspect that this is why Git has kept the shallow bit as sticky for so
long.

I'm not quite sure what to do here. I think that any of the following
would work:

  * Keep the shallow bit sticky, at least for fetch.writeCommitGraph
    (i.e., pretend as if fetch.writecommitgraph=0 in the case of
    '--unshallow').

  * Dump the object cache upon un-shallowing, forcing us to re-discover
    the parents when they are no longer hidden behind a graft.

The latter seems like the most complete feasible fix. The former should
work fine to address this case, but I wonder if there are other
call-sites that are affected by this behavior. My hunch is that this is
a unique case, since it requires going from shallow to unshallow in the
same process.

I have yet to create a smaller test case, but the following should be
sufficient to dump the cache of parsed objects upon shallowing or
un-shallowing:

diff --git a/shallow.c b/shallow.c
index b826de9b67..06db857f53 100644
--- a/shallow.c
+++ b/shallow.c
@@ -90,6 +90,9 @@ static void reset_repository_shallow(struct repository *r)
 {
 	r->parsed_objects->is_shallow = -1;
 	stat_validity_clear(r->parsed_objects->shallow_stat);
+
+	parsed_object_pool_clear(r->parsed_objects);
+	r->parsed_objects = parsed_object_pool_new();
 }

 int commit_shallow_file(struct repository *r, struct shallow_lock *lk)

Is this something we want to go forward with? Are there some
far-reaching implications that I'm missing?

> I'm going to CC Stolee to see if he has any thoughts on how to handle
> this and/or if my idea is on the right track.
>
> > "rm -fr objects/info/commit-graphs" recovers the full history in the
> > repo, so this is not a case of writing the wrong shallows --- it's
> > only a commit graph issue.
> >
> > I'll take a closer look, but thought I'd give others a chance to look
> > to in case there's something obvious. :)
> >
> > Thanks,
> > Jonathan
>
> Thanks,
> Taylor

Thanks,
Taylor

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'
  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
  1 sibling, 2 replies; 32+ messages in thread
From: Derrick Stolee @ 2020-06-03 13:08 UTC (permalink / raw)
  To: Taylor Blau
  Cc: Jonathan Nieder, git, jonathantanmy, gitster, newren, Jay Conrod

On 6/3/2020 1:16 AM, Taylor Blau wrote:
> On Tue, Jun 02, 2020 at 10:52:48PM -0600, Taylor Blau wrote:
>> Hi Jonathan,
>>
>> On Tue, Jun 02, 2020 at 08:42:13PM -0700, Jonathan Nieder wrote:
>>> Hi,
>>>
>>> Taylor Blau wrote:
>>>
>>>> Signed-off-by: Taylor Blau <me@ttaylorr.com>
>>>> ---
>>>>  builtin/receive-pack.c   |  4 ++--
>>>>  commit.h                 |  2 ++
>>>>  fetch-pack.c             | 10 +++++-----
>>>>  shallow.c                | 30 +++++++++++++++++++++---------
>>>>  t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++
>>>>  5 files changed, 59 insertions(+), 16 deletions(-)
>>>
>>> I haven't investigated the cause yet, but I've run into an interesting
>>> bug that bisects to this commit.  Jay Conrod (cc-ed) reports:
>>>
>>> | I believe this is also the cause of Go toolchain test failures we've
>>> | been seeing. Go uses git to fetch dependencies.
>>> |
>>> | The problem we're seeing can be reproduced with the script below. It
>>> | should print "success". Instead, the git merge-base command fails
>>> | because the commit 7303f77963648d5f1ec5e55eccfad8e14035866c
>>> | (origin/master) has no history.
>>>
>>> -- 8< --
>>> #!/bin/bash
>>>
>>> set -euxo pipefail
>>> if [ -d legacytest ]; then
>>>   echo "legacytest directory already exists" >&2
>>>   exit 1
>>> fi
>>> mkdir legacytest
>>> cd legacytest
>>> git init --bare
>>> git config protocol.version 2
>>> git config fetch.writeCommitGraph true
>>> git remote add origin -- https://github.com/rsc/legacytest
>>> git fetch -f --depth=1 origin refs/heads/master:refs/heads/master
>>> git fetch -f origin 'refs/heads/*:refs/heads/*' 'refs/tags/*:refs/tags/*'
>>> git fetch --unshallow -f origin
>>> git merge-base --is-ancestor -- v2.0.0 7303f77963648d5f1ec5e55eccfad8e14035866c
>>> echo success
>>> -- >8 --
>>
>> Thanks to you and Jay for the report and reproduction script. Indeed, I
>> can reproduce this on the tip of master (which is equivalent to v2.27.0
>> at the time of writing).
>>
>>> The fetch.writeCommitGraph part is interesting.  When does a commit
>>> graph file get written in this sequence of operations?  In an
>>> unshallow operation, does the usual guard against writing a commit
>>> graph in a shallow repo get missed?
>>
>> The last 'git fetch' is the one that writes the commit-graph. You can
>> verify this by sticking a 'ls objects/info' after each 'git' invocation
>> in your script.
>>
>> Here's where things get weird, though. Prior to this patch, Git would
>> pick up that the repository is shallow before unshallowing, but never
>> invalidate this fact after unshallowing. That means that once we got to
>> 'write_commit_graph', we'd exit immediately since it appears as if the
>> repository is shallow.
>>
>> In this patch, we don't do that anymore, since we rightly unset the fact
>> that we are (were) shallow.
>>
>> In a debugger, I ran your script and a 'git commit-graph write --split
>> --reachable' side-by-side, and found an interesting discrepancy: some
>> commits (loaded from 'copy_oids_to_commits') *don't* have their parents
>> set when invoked from 'git fetch', but *do* when invoked as 'git
>> commit-graph write ...'.
>>
>> I'm not an expert in the object cache, but my hunch is that when we
>> fetch these objects they're marked as parsed without having loaded their
>> parents. When we load them again via 'lookup_object', we get objects
>> that look parsed, but don't have parents where they otherwise should.
> 
> Ah, this only sort of has to do with the object cache. In
> 'parse_commit_buffer()' we stop parsing parents in the case that the
> repository is shallow (this goes back to 7f3140cd23 (git repack: keep
> commits hidden by a graft, 2009-07-23)).
> 
> That makes me somewhat nervous. We're going to keep any objects opened
> prior to unshallowing in the cache, along with their hidden parents. I
> suspect that this is why Git has kept the shallow bit as sticky for so
> long.
> 
> I'm not quite sure what to do here. I think that any of the following
> would work:
> 
>   * Keep the shallow bit sticky, at least for fetch.writeCommitGraph
>     (i.e., pretend as if fetch.writecommitgraph=0 in the case of
>     '--unshallow').

I'm in favor of this option, if possible. Anything that alters the
commit history in-memory at any point in the Git process is unsafe to
combine with a commit-graph read _or_ write. I'm sorry that the guards
in commit_graph_compatible() are not enough here.

>   * Dump the object cache upon un-shallowing, forcing us to re-discover
>     the parents when they are no longer hidden behind a graft.
> 
> The latter seems like the most complete feasible fix. The former should
> work fine to address this case, but I wonder if there are other
> call-sites that are affected by this behavior. My hunch is that this is
> a unique case, since it requires going from shallow to unshallow in the
> same process.

The latter would solve issues that could arise outside of the commit-graph
space. But it also presents an opportunity for another gap if someone edits
the shallow logic without putting in the proper guards.

To be extra safe, I'd be in favor of adding an "if (grafts_ever_existed)"
condition in commit_graph_compatible() based on a global that is assigned
a non-zero value whenever grafts are loaded at any point in the process,
mostly because it would be easy to guarantee that it is safe. It could
even be localized to the repository struct.
 
> I have yet to create a smaller test case, but the following should be
> sufficient to dump the cache of parsed objects upon shallowing or
> un-shallowing:
> 
> diff --git a/shallow.c b/shallow.c
> index b826de9b67..06db857f53 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -90,6 +90,9 @@ static void reset_repository_shallow(struct repository *r)
>  {
>  	r->parsed_objects->is_shallow = -1;
>  	stat_validity_clear(r->parsed_objects->shallow_stat);
> +
> +	parsed_object_pool_clear(r->parsed_objects);
> +	r->parsed_objects = parsed_object_pool_new();
>  }
> 
>  int commit_shallow_file(struct repository *r, struct shallow_lock *lk)
> 
> Is this something we want to go forward with? Are there some
> far-reaching implications that I'm missing?

I'd like to see the extra-careful check, in addition to this one. This
is such a rarely-used and narrowly-tested case that we need to be really
really careful to avoid regressions.

Thanks,
-Stolee


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'
  2020-06-03 13:08                 ` Derrick Stolee
@ 2020-06-03 19:26                   ` Taylor Blau
  2020-06-03 21:23                   ` Jonathan Nieder
  1 sibling, 0 replies; 32+ messages in thread
From: Taylor Blau @ 2020-06-03 19:26 UTC (permalink / raw)
  To: Derrick Stolee
  Cc: Taylor Blau, Jonathan Nieder, git, jonathantanmy, gitster,
	newren, Jay Conrod

Hi Stolee,

On Wed, Jun 03, 2020 at 09:08:26AM -0400, Derrick Stolee wrote:
> On 6/3/2020 1:16 AM, Taylor Blau wrote:
> > On Tue, Jun 02, 2020 at 10:52:48PM -0600, Taylor Blau wrote:
> >> Hi Jonathan,
> >>
> >> On Tue, Jun 02, 2020 at 08:42:13PM -0700, Jonathan Nieder wrote:
> >>> Hi,
> >>>
> >>> Taylor Blau wrote:
> >>>
> >>>> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> >>>> ---
> >>>>  builtin/receive-pack.c   |  4 ++--
> >>>>  commit.h                 |  2 ++
> >>>>  fetch-pack.c             | 10 +++++-----
> >>>>  shallow.c                | 30 +++++++++++++++++++++---------
> >>>>  t/t5537-fetch-shallow.sh | 29 +++++++++++++++++++++++++++++
> >>>>  5 files changed, 59 insertions(+), 16 deletions(-)
> >>>
> >>> I haven't investigated the cause yet, but I've run into an interesting
> >>> bug that bisects to this commit.  Jay Conrod (cc-ed) reports:
> >>>
> >>> | I believe this is also the cause of Go toolchain test failures we've
> >>> | been seeing. Go uses git to fetch dependencies.
> >>> |
> >>> | The problem we're seeing can be reproduced with the script below. It
> >>> | should print "success". Instead, the git merge-base command fails
> >>> | because the commit 7303f77963648d5f1ec5e55eccfad8e14035866c
> >>> | (origin/master) has no history.
> >>>
> >>> -- 8< --
> >>> #!/bin/bash
> >>>
> >>> set -euxo pipefail
> >>> if [ -d legacytest ]; then
> >>>   echo "legacytest directory already exists" >&2
> >>>   exit 1
> >>> fi
> >>> mkdir legacytest
> >>> cd legacytest
> >>> git init --bare
> >>> git config protocol.version 2
> >>> git config fetch.writeCommitGraph true
> >>> git remote add origin -- https://github.com/rsc/legacytest
> >>> git fetch -f --depth=1 origin refs/heads/master:refs/heads/master
> >>> git fetch -f origin 'refs/heads/*:refs/heads/*' 'refs/tags/*:refs/tags/*'
> >>> git fetch --unshallow -f origin
> >>> git merge-base --is-ancestor -- v2.0.0 7303f77963648d5f1ec5e55eccfad8e14035866c
> >>> echo success
> >>> -- >8 --
> >>
> >> Thanks to you and Jay for the report and reproduction script. Indeed, I
> >> can reproduce this on the tip of master (which is equivalent to v2.27.0
> >> at the time of writing).
> >>
> >>> The fetch.writeCommitGraph part is interesting.  When does a commit
> >>> graph file get written in this sequence of operations?  In an
> >>> unshallow operation, does the usual guard against writing a commit
> >>> graph in a shallow repo get missed?
> >>
> >> The last 'git fetch' is the one that writes the commit-graph. You can
> >> verify this by sticking a 'ls objects/info' after each 'git' invocation
> >> in your script.
> >>
> >> Here's where things get weird, though. Prior to this patch, Git would
> >> pick up that the repository is shallow before unshallowing, but never
> >> invalidate this fact after unshallowing. That means that once we got to
> >> 'write_commit_graph', we'd exit immediately since it appears as if the
> >> repository is shallow.
> >>
> >> In this patch, we don't do that anymore, since we rightly unset the fact
> >> that we are (were) shallow.
> >>
> >> In a debugger, I ran your script and a 'git commit-graph write --split
> >> --reachable' side-by-side, and found an interesting discrepancy: some
> >> commits (loaded from 'copy_oids_to_commits') *don't* have their parents
> >> set when invoked from 'git fetch', but *do* when invoked as 'git
> >> commit-graph write ...'.
> >>
> >> I'm not an expert in the object cache, but my hunch is that when we
> >> fetch these objects they're marked as parsed without having loaded their
> >> parents. When we load them again via 'lookup_object', we get objects
> >> that look parsed, but don't have parents where they otherwise should.
> >
> > Ah, this only sort of has to do with the object cache. In
> > 'parse_commit_buffer()' we stop parsing parents in the case that the
> > repository is shallow (this goes back to 7f3140cd23 (git repack: keep
> > commits hidden by a graft, 2009-07-23)).
> >
> > That makes me somewhat nervous. We're going to keep any objects opened
> > prior to unshallowing in the cache, along with their hidden parents. I
> > suspect that this is why Git has kept the shallow bit as sticky for so
> > long.
> >
> > I'm not quite sure what to do here. I think that any of the following
> > would work:
> >
> >   * Keep the shallow bit sticky, at least for fetch.writeCommitGraph
> >     (i.e., pretend as if fetch.writecommitgraph=0 in the case of
> >     '--unshallow').
>
> I'm in favor of this option, if possible. Anything that alters the
> commit history in-memory at any point in the Git process is unsafe to
> combine with a commit-graph read _or_ write. I'm sorry that the guards
> in commit_graph_compatible() are not enough here.
>
> >   * Dump the object cache upon un-shallowing, forcing us to re-discover
> >     the parents when they are no longer hidden behind a graft.
> >
> > The latter seems like the most complete feasible fix. The former should
> > work fine to address this case, but I wonder if there are other
> > call-sites that are affected by this behavior. My hunch is that this is
> > a unique case, since it requires going from shallow to unshallow in the
> > same process.
>
> The latter would solve issues that could arise outside of the commit-graph
> space. But it also presents an opportunity for another gap if someone edits
> the shallow logic without putting in the proper guards.
>
> To be extra safe, I'd be in favor of adding an "if (grafts_ever_existed)"
> condition in commit_graph_compatible() based on a global that is assigned
> a non-zero value whenever grafts are loaded at any point in the process,
> mostly because it would be easy to guarantee that it is safe. It could
> even be localized to the repository struct.
>
> > I have yet to create a smaller test case, but the following should be
> > sufficient to dump the cache of parsed objects upon shallowing or
> > un-shallowing:
> >
> > diff --git a/shallow.c b/shallow.c
> > index b826de9b67..06db857f53 100644
> > --- a/shallow.c
> > +++ b/shallow.c
> > @@ -90,6 +90,9 @@ static void reset_repository_shallow(struct repository *r)
> >  {
> >  	r->parsed_objects->is_shallow = -1;
> >  	stat_validity_clear(r->parsed_objects->shallow_stat);
> > +
> > +	parsed_object_pool_clear(r->parsed_objects);
> > +	r->parsed_objects = parsed_object_pool_new();
> >  }
> >
> >  int commit_shallow_file(struct repository *r, struct shallow_lock *lk)
> >
> > Is this something we want to go forward with? Are there some
> > far-reaching implications that I'm missing?
>
> I'd like to see the extra-careful check, in addition to this one. This
> is such a rarely-used and narrowly-tested case that we need to be really
> really careful to avoid regressions.

I'm a little confused at which suggestion you're in favor of ;-). For
clarity, are you suggesting that we add a new 'r->grafts_ever_existed'
bit in addition to doing a hard reset of the object pool?

> Thanks,
> -Stolee

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'
  2020-06-03  5:16               ` Taylor Blau
  2020-06-03 13:08                 ` Derrick Stolee
@ 2020-06-03 20:51                 ` Jonathan Nieder
  2020-06-03 22:14                   ` Taylor Blau
  1 sibling, 1 reply; 32+ messages in thread
From: Jonathan Nieder @ 2020-06-03 20:51 UTC (permalink / raw)
  To: Taylor Blau
  Cc: git, jonathantanmy, gitster, newren, Jay Conrod, Derrick Stolee

Taylor Blau wrote:

> Ah, this only sort of has to do with the object cache. In
> 'parse_commit_buffer()' we stop parsing parents in the case that the
> repository is shallow (this goes back to 7f3140cd23 (git repack: keep
> commits hidden by a graft, 2009-07-23)).

Ah, good analysis.  (In fact, the behavior is older: it's from
5da5c8f4cf4 (Teach parse_commit_buffer about grafting., 2005-07-30).)
So this is additional "cached" data that needs to be invalidated by
reset_repository_shallow.

So the question is, what other information falls into that category?

[...]
> --- a/shallow.c
> +++ b/shallow.c
> @@ -90,6 +90,9 @@ static void reset_repository_shallow(struct repository *r)
>  {
>  	r->parsed_objects->is_shallow = -1;
>  	stat_validity_clear(r->parsed_objects->shallow_stat);
> +

(nit: the above two lines wouldn't be needed if r->parsed_objects is
being thrown away.)

> +	parsed_object_pool_clear(r->parsed_objects);
> +	r->parsed_objects = parsed_object_pool_new();
>  }

Shallows don't affect the ref store.  They only affect object walks.
So r->parsed_objects does seem like the only place that could be
affected.

That said, with this change I'd worry about use-after-free from any
existing references to objects in the pool.

Stepping back, what I think I would like to see is to *not* have
grafts and shallow state affect the in-memory persisted parsed
objects.  Instead, act as an overlay in accessors that traverse over
them.

Lacking that, I like the idea of a "dirty bit" that gets written as
soon as we have started lying in the parsed object pool.  Something
like this.  What do you think?

diff --git i/commit-graph.c w/commit-graph.c
index 2ff042fbf4f..84b49ce903b 100644
--- i/commit-graph.c
+++ w/commit-graph.c
@@ -149,7 +149,8 @@ static int commit_graph_compatible(struct repository *r)
 	}
 
 	prepare_commit_graft(r);
-	if (r->parsed_objects && r->parsed_objects->grafts_nr)
+	if (r->parsed_objects &&
+	    (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent))
 		return 0;
 	if (is_repository_shallow(r))
 		return 0;
diff --git i/commit.c w/commit.c
index 87686a7055b..762f09e53ae 100644
--- i/commit.c
+++ w/commit.c
@@ -423,6 +423,8 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b
 	pptr = &item->parents;
 
 	graft = lookup_commit_graft(r, &item->object.oid);
+	if (graft)
+		r->parsed_objects->substituted_parent = 1;
 	while (bufptr + parent_entry_len < tail && !memcmp(bufptr, "parent ", 7)) {
 		struct commit *new_parent;
 
@@ -447,6 +449,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b
 	if (graft) {
 		int i;
 		struct commit *new_parent;
+
 		for (i = 0; i < graft->nr_parent; i++) {
 			new_parent = lookup_commit(r,
 						   &graft->parent[i]);
diff --git i/object.h w/object.h
index b22328b8383..db02fdcd6b2 100644
--- i/object.h
+++ w/object.h
@@ -26,6 +26,7 @@ struct parsed_object_pool {
 	char *alternate_shallow_file;
 
 	int commit_graft_prepared;
+	int substituted_parent;
 
 	struct buffer_slab *buffer_slab;
 };

Thanks,
Jonathan

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'
  2020-06-03 13:08                 ` Derrick Stolee
  2020-06-03 19:26                   ` Taylor Blau
@ 2020-06-03 21:23                   ` Jonathan Nieder
  1 sibling, 0 replies; 32+ messages in thread
From: Jonathan Nieder @ 2020-06-03 21:23 UTC (permalink / raw)
  To: Derrick Stolee
  Cc: Taylor Blau, git, jonathantanmy, gitster, newren, Jay Conrod

Hi,

Derrick Stolee wrote:
> On 6/3/2020 1:16 AM, Taylor Blau wrote:

>>   * Keep the shallow bit sticky, at least for fetch.writeCommitGraph
>>     (i.e., pretend as if fetch.writecommitgraph=0 in the case of
>>     '--unshallow').
>
> I'm in favor of this option, if possible. Anything that alters the
> commit history in-memory at any point in the Git process is unsafe to
> combine with a commit-graph read _or_ write. I'm sorry that the guards
> in commit_graph_compatible() are not enough here.

As described in [1], I agree --- this kind of "dirty bit" is a good
option and seems like the right thing to do here.

And I'm glad you said read _or_ write here.  I hadn't realized that
this check already applies for reads, and I'm very happy it does.

[...]
>>   * Dump the object cache upon un-shallowing, forcing us to re-discover
>>     the parents when they are no longer hidden behind a graft.
>>
>> The latter seems like the most complete feasible fix. The former should
>> work fine to address this case, but I wonder if there are other
>> call-sites that are affected by this behavior. My hunch is that this is
>> a unique case, since it requires going from shallow to unshallow in the
>> same process.
>
> The latter would solve issues that could arise outside of the commit-graph
> space. But it also presents an opportunity for another gap if someone edits
> the shallow logic without putting in the proper guards.

This, however, I don't agree with.

I'm a strong believer in having clear invariants --- without them,
code can only be understood empirically, and
https://ieeexplore.ieee.org/document/9068304 describes how painful of
a world that can be.

So because shallow is specifically about changing the set of parents
in objects used for traversal, I want to make sure that we as
reviewers will push back on any potential other new meaning of
shallow.  *If* we had a safe way to invalidate the object cache, it
would be sufficient and would be the right thing to do.  As described
in [1], we unfortunately don't have such a thing.

Okay, that's all an aside.  Now for the actual reason I'm replying:

I had been wondering why this wasn't caught at read time, but of
course --unshallow clears away the shallows so there was no way for
that to happen.  So what I wonder is, why isn't this caught by fsck?
Can "git fsck" run "git commit-graph verify" automatically and include
its result as part of what it reports?

Thanks,
Jonathan

[1] https://lore.kernel.org/git/20200603205151.GC253041@google.com/

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'
  2020-06-03 20:51                 ` Jonathan Nieder
@ 2020-06-03 22:14                   ` Taylor Blau
  2020-06-03 23:06                     ` Jonathan Nieder
  0 siblings, 1 reply; 32+ messages in thread
From: Taylor Blau @ 2020-06-03 22:14 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Taylor Blau, git, jonathantanmy, gitster, newren, Jay Conrod,
	Derrick Stolee

On Wed, Jun 03, 2020 at 01:51:51PM -0700, Jonathan Nieder wrote:
> Taylor Blau wrote:
>
> > Ah, this only sort of has to do with the object cache. In
> > 'parse_commit_buffer()' we stop parsing parents in the case that the
> > repository is shallow (this goes back to 7f3140cd23 (git repack: keep
> > commits hidden by a graft, 2009-07-23)).
>
> Ah, good analysis.  (In fact, the behavior is older: it's from
> 5da5c8f4cf4 (Teach parse_commit_buffer about grafting., 2005-07-30).)
> So this is additional "cached" data that needs to be invalidated by
> reset_repository_shallow.
>
> So the question is, what other information falls into that category?
>
> [...]
> > --- a/shallow.c
> > +++ b/shallow.c
> > @@ -90,6 +90,9 @@ static void reset_repository_shallow(struct repository *r)
> >  {
> >  	r->parsed_objects->is_shallow = -1;
> >  	stat_validity_clear(r->parsed_objects->shallow_stat);
> > +
>
> (nit: the above two lines wouldn't be needed if r->parsed_objects is
> being thrown away.)

Right, thanks. I don't think that it matters since you point out a
legitimate issue with dangling references, but serves me right for
working on this so late at night ;-).

> > +	parsed_object_pool_clear(r->parsed_objects);
> > +	r->parsed_objects = parsed_object_pool_new();
> >  }
>
> Shallows don't affect the ref store.  They only affect object walks.
> So r->parsed_objects does seem like the only place that could be
> affected.
>
> That said, with this change I'd worry about use-after-free from any
> existing references to objects in the pool.
>
> Stepping back, what I think I would like to see is to *not* have
> grafts and shallow state affect the in-memory persisted parsed
> objects.  Instead, act as an overlay in accessors that traverse over
> them.
>
> Lacking that, I like the idea of a "dirty bit" that gets written as
> soon as we have started lying in the parsed object pool.  Something
> like this.  What do you think?
>
> diff --git i/commit-graph.c w/commit-graph.c
> index 2ff042fbf4f..84b49ce903b 100644
> --- i/commit-graph.c
> +++ w/commit-graph.c
> @@ -149,7 +149,8 @@ static int commit_graph_compatible(struct repository *r)
>  	}
>
>  	prepare_commit_graft(r);
> -	if (r->parsed_objects && r->parsed_objects->grafts_nr)
> +	if (r->parsed_objects &&
> +	    (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent))

This is a little tricky. Why would we set substituted_parent without
also incrementing grafts_nr? That seems like the real bug here: if we
incremented grafts_nr, then we would return a non-zero value from
'commit_graph_compatible' and rightly stop even without this sticky-bit.

I don't quite understand this myself. If it's an oversight, it's a
remarkably long-lived one. Do you have a better sense of this?

>  		return 0;
>  	if (is_repository_shallow(r))
>  		return 0;
> diff --git i/commit.c w/commit.c
> index 87686a7055b..762f09e53ae 100644
> --- i/commit.c
> +++ w/commit.c
> @@ -423,6 +423,8 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b
>  	pptr = &item->parents;
>
>  	graft = lookup_commit_graft(r, &item->object.oid);
> +	if (graft)
> +		r->parsed_objects->substituted_parent = 1;
>  	while (bufptr + parent_entry_len < tail && !memcmp(bufptr, "parent ", 7)) {
>  		struct commit *new_parent;
>
> @@ -447,6 +449,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b
>  	if (graft) {
>  		int i;
>  		struct commit *new_parent;
> +

Nit: unnecessary whitespace change, but I doubt it really matters much.

>  		for (i = 0; i < graft->nr_parent; i++) {
>  			new_parent = lookup_commit(r,
>  						   &graft->parent[i]);
> diff --git i/object.h w/object.h
> index b22328b8383..db02fdcd6b2 100644
> --- i/object.h
> +++ w/object.h
> @@ -26,6 +26,7 @@ struct parsed_object_pool {
>  	char *alternate_shallow_file;
>
>  	int commit_graft_prepared;
> +	int substituted_parent;
>
>  	struct buffer_slab *buffer_slab;
>  };
>
> Thanks,
> Jonathan

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'
  2020-06-03 22:14                   ` Taylor Blau
@ 2020-06-03 23:06                     ` Jonathan Nieder
  2020-06-04 17:45                       ` Taylor Blau
  0 siblings, 1 reply; 32+ messages in thread
From: Jonathan Nieder @ 2020-06-03 23:06 UTC (permalink / raw)
  To: Taylor Blau
  Cc: git, jonathantanmy, gitster, newren, Jay Conrod, Derrick Stolee

Taylor Blau wrote:
> On Wed, Jun 03, 2020 at 01:51:51PM -0700, Jonathan Nieder wrote:

>> --- i/commit-graph.c
>> +++ w/commit-graph.c
>> @@ -149,7 +149,8 @@ static int commit_graph_compatible(struct repository *r)
>>  	}
>>
>>  	prepare_commit_graft(r);
>> -	if (r->parsed_objects && r->parsed_objects->grafts_nr)
>> +	if (r->parsed_objects &&
>> +	    (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent))
>
> This is a little tricky. Why would we set substituted_parent without
> also incrementing grafts_nr? That seems like the real bug here: if we
> incremented grafts_nr, then we would return a non-zero value from
> 'commit_graph_compatible' and rightly stop even without this sticky-bit.
> 
> I don't quite understand this myself. If it's an oversight, it's a
> remarkably long-lived one. Do you have a better sense of this?

The idea here is to check for two different things:

 1. Do we have grafts (either from a grafts file or from a shallow
    file)?  If so, this repository is not commit graph compatible
    because we could encounter one of them.

 2. Have cached parsed objects taken any history modifications into
    account?  If so, this in-memory state is not commit graph
    compatible because we could encounter one of them.

The check (1) might seem sufficient if the set of grafts is constant
for the lifetime of a git process.  But since 37b9dcabfc (shallow.c:
use '{commit,rollback}_shallow_file', 2020-04-22), it is not constant
for the process lifetime, so we need the check (2) as well.

We might want a similar check for replace refs as well some day, but
not today (there is not a way to remove entries from replace_map
during the life of a process).

I can try sending a proper patch with commit message and tests
tomorrow morning (or if someone else takes care of it, that's fine,
too).  Thanks, both, for your help --- it was nice seeing a clear
explanation of the cause already figured out and explained when I woke
up.

Regards,
Jonathan

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 2/2] shallow.c: use '{commit,rollback}_shallow_file'
  2020-06-03 23:06                     ` Jonathan Nieder
@ 2020-06-04 17:45                       ` Taylor Blau
  0 siblings, 0 replies; 32+ messages in thread
From: Taylor Blau @ 2020-06-04 17:45 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Taylor Blau, git, jonathantanmy, gitster, newren, Jay Conrod,
	Derrick Stolee

Hi Jonathan,

On Wed, Jun 03, 2020 at 04:06:11PM -0700, Jonathan Nieder wrote:
> Taylor Blau wrote:
> > On Wed, Jun 03, 2020 at 01:51:51PM -0700, Jonathan Nieder wrote:
>
> >> --- i/commit-graph.c
> >> +++ w/commit-graph.c
> >> @@ -149,7 +149,8 @@ static int commit_graph_compatible(struct repository *r)
> >>  	}
> >>
> >>  	prepare_commit_graft(r);
> >> -	if (r->parsed_objects && r->parsed_objects->grafts_nr)
> >> +	if (r->parsed_objects &&
> >> +	    (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent))
> >
> > This is a little tricky. Why would we set substituted_parent without
> > also incrementing grafts_nr? That seems like the real bug here: if we
> > incremented grafts_nr, then we would return a non-zero value from
> > 'commit_graph_compatible' and rightly stop even without this sticky-bit.
> >
> > I don't quite understand this myself. If it's an oversight, it's a
> > remarkably long-lived one. Do you have a better sense of this?
>
> The idea here is to check for two different things:
>
>  1. Do we have grafts (either from a grafts file or from a shallow
>     file)?  If so, this repository is not commit graph compatible
>     because we could encounter one of them.
>
>  2. Have cached parsed objects taken any history modifications into
>     account?  If so, this in-memory state is not commit graph
>     compatible because we could encounter one of them.

Ah, breaking it up like this makes it clearer. The gist is that it is
possible to generate a situation which has (1) no grafts explicitly, but
(2) *does* have history modifications.

> The check (1) might seem sufficient if the set of grafts is constant
> for the lifetime of a git process.  But since 37b9dcabfc (shallow.c:
> use '{commit,rollback}_shallow_file', 2020-04-22), it is not constant
> for the process lifetime, so we need the check (2) as well.
>
> We might want a similar check for replace refs as well some day, but
> not today (there is not a way to remove entries from replace_map
> during the life of a process).

Right.

> I can try sending a proper patch with commit message and tests
> tomorrow morning (or if someone else takes care of it, that's fine,
> too).  Thanks, both, for your help --- it was nice seeing a clear
> explanation of the cause already figured out and explained when I woke
> up.

If you are interested, by all means -- I'd certainly be appreciative.
I'm on vacation next week, and so it may be better if you are able to
shepherd the patches through. Otherwise, I'd be happy to do it the week
after when I get back.

> Regards,
> Jonathan

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2020-06-04 17:45 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-21 18:09 [PATCH] shallow.c: use 'reset_repository_shallow' when appropriate Taylor Blau
2020-04-21 20:41 ` Junio C Hamano
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

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).