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@xxxxxxxxxx>
> ---
> 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
