From: Jonathan Nieder <jrnieder@gmail.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, avarab@gmail.com
Subject: Re: [PATCH] shallow: clear shallow cache when committing lock
Date: Mon, 17 Dec 2018 19:46:48 -0800 [thread overview]
Message-ID: <20181218034648.GA221428@google.com> (raw)
In-Reply-To: <20181218010811.143608-1-jonathantanmy@google.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
next prev parent reply other threads:[~2018-12-18 3:46 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-18 1:08 [PATCH] shallow: clear shallow cache when committing lock Jonathan Tan
2018-12-18 3:46 ` Jonathan Nieder [this message]
2018-12-20 19:53 ` [RFC PATCH] fetch-pack: respect --no-update-shallow in v2 Jonathan Tan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181218034648.GA221428@google.com \
--to=jrnieder@gmail.com \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=jonathantanmy@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).