Re: [PATCH] shallow: clear shallow cache when committing lock

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Jonathan Tan wrote:

> When setup_alternate_shallow() is invoked a second time in the same
> process, it fails with the error "shallow file has changed since we read
> it". One way of reproducing this is to fetch using protocol v2 in such a
> way that backfill_tags() is required, and that the responses to both
> requests contain a "shallow-info" section.
>
> This is because when the shallow lock is committed, the stat information
> of the shallow file is not cleared. Ensure that this information is
> always cleared whenever the shallow lock is committed by introducing a
> new API that hides the shallow lock behind a custom struct.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> This was discovered with the help of Aevar's GIT_TEST_PROTOCOL_VERSION
> patches.

Good catch.  I think the above note should go in the commit message,
since it would be very useful to anyone looking back at this commit
message and trying to find a quick reproduction recipe.

[...]
> I couldn't figure out if the test case included in this patch can be
> reduced - if any one has any idea, help is appreciated.

It's short enough to be comprehensible, so I wouldn't worry too
much. :)

> I'm also not sure why this issue only occurs with protocol v2. It's true
> that, when using protocol v0, the server can communicate shallow
> information both before and after "want"s are received, and if shallow
> communication is only communicated before, the client will invoke
> setup_temporary_shallow() instead of setup_alternate_shallow(). (In
> protocol v2, shallow information is always communicated after "want"s,
> thus demonstrating the issue.) But even in protocol v0, writes to the
> shallow file go through setup_alternate_shallow() anyway (in
> update_shallow()), so I would think that the issue would occur, but it
> doesn't.

Good question.  I'll try to poke more tomorrow morning, too.

[...]
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1,7 +1,6 @@
>  #include "builtin.h"
>  #include "repository.h"
>  #include "config.h"
> -#include "lockfile.h"
>  #include "pack.h"
>  #include "refs.h"
>  #include "pkt-line.h"
> @@ -864,7 +863,7 @@ static void refuse_unconfigured_deny_delete_current(void)
>  static int command_singleton_iterator(void *cb_data, struct object_id *oid);
>  static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
>  {
> -	struct lock_file shallow_lock = LOCK_INIT;
> +	struct shallow_lock shallow_lock;

Interesting.  Is there another name that can convey what this object
does more clearly?  At first glance I would expect this to be a less
deep kind of lock.

I'm awful at naming, but a couple of ideas to get things started:

- 'struct shallow_update', since this represents a pending update to
  the .git/shallow file?

- struct lock_file_for_shallow?

- struct locked_shallow_file?

- "struct shallow_file" or "struct shallow_commits_file"?  This is a
  handle representing the state of what will become .git/shallow file
  once the caller commits the update.

[...]
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
[...]
> @@ -1660,7 +1659,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_lock(&shallow_lock);

For a moment, I was worried that this line could be reached without
setup_alternate_shallow having been called first.  Fortunately,
shallow_lock is static so it is zero-initialized automatically, making
that harmless.

[...]
> --- a/shallow.c
> +++ b/shallow.c
[...]
> @@ -358,6 +359,22 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
>  	strbuf_release(&sb);
>  }
>  
> +int commit_shallow_lock(struct shallow_lock *shallow_lock)
> +{
> +	int ret;
> +
> +	if ((ret = commit_lock_file(&shallow_lock->lock)))
> +		return ret;
> +	the_repository->parsed_objects->is_shallow = -1;
> +	stat_validity_clear(the_repository->parsed_objects->shallow_stat);

In 'next', some other functions in this file handle an arbitrary
repository argument 'r'.  Should this one as well?  (I.e. can this
patch come with a conflict-resolution patch to handle that?)

[...]
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -471,6 +471,22 @@ test_expect_success 'upload-pack respects client shallows' '

Yay, thanks for the test.  I'll study it more closely tomorrow.

Thanks and hope that helps,
Jonathan