On Thu, Jan 10, 2019 at 11:36 AM Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote:
>
> When fetching using protocol v2, the remote may send a "shallow-info"
> section if the client is shallow. If so, Git as the client currently
> takes the shallow file lock, even if the "shallow-info" section is
> empty.
>
> This is not a problem except that Git does not support taking the
> shallow file lock after modifying the shallow file, because
> is_repository_shallow() stores information that is never cleared. And
> this take-after-modify occurs when Git does a tag-following fetch from a
> shallow repository on a transport that does not support tag following
> (since in this case, 2 fetches are performed).
>
> To solve this issue, take the shallow file lock (and perform all other
> shallow processing) only if the "shallow-info" section is non-empty;
> otherwise, behave as if it were empty.
In other parts of the code we often have an early exit instead of
setting a variable and reacting to that in the end, i.e. what
do you think about:
static void receive_shallow_info(struct fetch_pack_args *args,
struct packet_reader *reader)
{
process_section_header(reader, "shallow-info", 0);
+ if (reader->status == PACKET_READ_FLUSH ||
+ reader->status == PACKET_READ_DELIM)
+ /* useful comment why empty sections appear */
+ return;
while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
...
instead? This would allow us to keep the rest of the function
relatively simple as well as we'd have a dedicated space where
we can explain why empty sections need to be treated specially.
> A full solution (probably, ensuring that any action of committing
> shallow file locks also includes clearing the information stored by
> is_repository_shallow()) would solve the issue without need for this
> patch, but this patch is independently useful (as an optimization to
> prevent writing a file in an unnecessary case), hence why I wrote it. I
> have included a NEEDSWORK outlining the full solution.
I like this patch..
> +test_expect_success 'ensure that multiple fetches in same process from a shallow repo works' '
> + rm -rf server client trace &&
> +
> + test_create_repo server &&
> + test_commit -C server one &&
> + test_commit -C server two &&
> + test_commit -C server three &&
> + git clone --shallow-exclude two "file://$(pwd)/server" client &&
> +
> + git -C server tag -a -m "an annotated tag" twotag two &&
> +
> + # Triggers tag following (thus, 2 fetches in one process)
> + GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
> + fetch --shallow-exclude one origin &&
> + # Ensure that protocol v2 is used
> + grep "fetch< version 2" trace
> +'
Would we also need to ensure tags 'one' and 'three',
but not 'two' are present?
(What error condition do we see without this patch?)
