From: Christian Couder <christian.couder@gmail.com>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
Derrick Stolee <dstolee@microsoft.com>, Jeff King <peff@peff.net>,
Taylor Blau <me@ttaylorr.com>,
Jonathan Tan <jonathantanmy@google.com>,
Jonathan Nieder <jrnieder@gmail.com>,
Christian Couder <chriscool@tuxfamily.org>
Subject: [RFC PATCH] upload-pack: fix filter options scope
Date: Thu, 7 May 2020 11:58:29 +0200 [thread overview]
Message-ID: <20200507095829.16894-1-chriscool@tuxfamily.org> (raw)
The upload_pack_v2() function is sometimes called twice in the same
process while 'struct list_objects_filter_options filter_options' was
declared as static at the beginning of 'upload-pack.c'.
This made the check in list_objects_filter_die_if_populated(), which
is called by process_args(), fail the second time upload_pack_v2() is
called, as filter_options had already been populated the first time.
To fix that, filter_options is not static any more, but instead it's
now owned by upload_pack_v2() and upload_pack().
This fixes the first of the 2 bugs documented by d0badf8797
(partial-clone: demonstrate bugs in partial fetch, 2020-02-21).
Helped-by: Derrick Stolee <dstolee@microsoft.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
The issue was originally discussed in:
https://lore.kernel.org/git/dbc1bdcae16f8b9941add514264b0fe04cda48c0.1582129312.git.gitgitgadget@gmail.com/
I am not sure why upload_pack_v2() is called twice. Also it seems
that another similar issue might not be fixed by this. So this patch
is RFC for now.
t/t5616-partial-clone.sh | 2 +-
upload-pack.c | 40 ++++++++++++++++++++++++++--------------
2 files changed, 27 insertions(+), 15 deletions(-)
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 88002b24af..b73c38c29a 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -389,7 +389,7 @@ test_expect_success 'fetch lazy-fetches only to resolve deltas, protocol v2' '
# repository did not have tags during clone, but has tags
# in the fetch.
-test_expect_failure 'verify fetch succeeds when asking for new tags' '
+test_expect_success 'verify fetch succeeds when asking for new tags' '
git clone --filter=blob:none "file://$(pwd)/srv.bare" tag-test &&
for i in I J K
do
diff --git a/upload-pack.c b/upload-pack.c
index 902d0ad5e1..4e22e46294 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -68,7 +68,6 @@ static const char *pack_objects_hook;
static int filter_capability_requested;
static int allow_filter;
static int allow_ref_in_want;
-static struct list_objects_filter_options filter_options;
static int allow_sideband_all;
@@ -103,7 +102,8 @@ static int write_one_shallow(const struct commit_graft *graft, void *cb_data)
}
static void create_pack_file(const struct object_array *have_obj,
- const struct object_array *want_obj)
+ const struct object_array *want_obj,
+ struct list_objects_filter_options *filter_options)
{
struct child_process pack_objects = CHILD_PROCESS_INIT;
char data[8193], progress[128];
@@ -140,9 +140,9 @@ static void create_pack_file(const struct object_array *have_obj,
argv_array_push(&pack_objects.args, "--delta-base-offset");
if (use_include_tag)
argv_array_push(&pack_objects.args, "--include-tag");
- if (filter_options.choice) {
+ if (filter_options->choice) {
const char *spec =
- expand_list_objects_filter_spec(&filter_options);
+ expand_list_objects_filter_spec(filter_options);
if (pack_objects.use_shell) {
struct strbuf buf = STRBUF_INIT;
sq_quote_buf(&buf, spec);
@@ -848,7 +848,9 @@ static int process_deepen_not(const char *line, struct string_list *deepen_not,
return 0;
}
-static void receive_needs(struct packet_reader *reader, struct object_array *want_obj)
+static void receive_needs(struct packet_reader *reader,
+ struct object_array *want_obj,
+ struct list_objects_filter_options *filter_options)
{
struct object_array shallows = OBJECT_ARRAY_INIT;
struct string_list deepen_not = STRING_LIST_INIT_DUP;
@@ -883,8 +885,8 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wan
if (skip_prefix(reader->line, "filter ", &arg)) {
if (!filter_capability_requested)
die("git upload-pack: filtering capability not negotiated");
- list_objects_filter_die_if_populated(&filter_options);
- parse_list_objects_filter(&filter_options, arg);
+ list_objects_filter_die_if_populated(filter_options);
+ parse_list_objects_filter(filter_options, arg);
continue;
}
@@ -1087,11 +1089,14 @@ void upload_pack(struct upload_pack_options *options)
struct string_list symref = STRING_LIST_INIT_DUP;
struct object_array want_obj = OBJECT_ARRAY_INIT;
struct packet_reader reader;
+ struct list_objects_filter_options filter_options;
stateless_rpc = options->stateless_rpc;
timeout = options->timeout;
daemon_mode = options->daemon_mode;
+ memset(&filter_options, 0, sizeof(filter_options));
+
git_config(upload_pack_config, NULL);
head_ref_namespaced(find_symref, &symref);
@@ -1114,12 +1119,14 @@ void upload_pack(struct upload_pack_options *options)
PACKET_READ_CHOMP_NEWLINE |
PACKET_READ_DIE_ON_ERR_PACKET);
- receive_needs(&reader, &want_obj);
+ receive_needs(&reader, &want_obj, &filter_options);
if (want_obj.nr) {
struct object_array have_obj = OBJECT_ARRAY_INIT;
get_common_commits(&reader, &have_obj, &want_obj);
- create_pack_file(&have_obj, &want_obj);
+ create_pack_file(&have_obj, &want_obj, &filter_options);
}
+
+ list_objects_filter_release(&filter_options);
}
struct upload_pack_data {
@@ -1250,7 +1257,8 @@ static int parse_have(const char *line, struct oid_array *haves)
static void process_args(struct packet_reader *request,
struct upload_pack_data *data,
- struct object_array *want_obj)
+ struct object_array *want_obj,
+ struct list_objects_filter_options *filter_options)
{
while (packet_reader_read(request) == PACKET_READ_NORMAL) {
const char *arg = request->line;
@@ -1306,8 +1314,8 @@ static void process_args(struct packet_reader *request,
}
if (allow_filter && skip_prefix(arg, "filter ", &p)) {
- list_objects_filter_die_if_populated(&filter_options);
- parse_list_objects_filter(&filter_options, p);
+ list_objects_filter_die_if_populated(filter_options);
+ parse_list_objects_filter(filter_options, p);
continue;
}
@@ -1470,6 +1478,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
struct upload_pack_data data;
struct object_array have_obj = OBJECT_ARRAY_INIT;
struct object_array want_obj = OBJECT_ARRAY_INIT;
+ struct list_objects_filter_options filter_options;
clear_object_flags(ALL_FLAGS);
@@ -1478,10 +1487,12 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
upload_pack_data_init(&data);
use_sideband = LARGE_PACKET_MAX;
+ memset(&filter_options, 0, sizeof(filter_options));
+
while (state != FETCH_DONE) {
switch (state) {
case FETCH_PROCESS_ARGS:
- process_args(request, &data, &want_obj);
+ process_args(request, &data, &want_obj, &filter_options);
if (!want_obj.nr) {
/*
@@ -1514,7 +1525,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
send_shallow_info(&data, &want_obj);
packet_writer_write(&data.writer, "packfile\n");
- create_pack_file(&have_obj, &want_obj);
+ create_pack_file(&have_obj, &want_obj, &filter_options);
state = FETCH_DONE;
break;
case FETCH_DONE:
@@ -1525,6 +1536,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,
upload_pack_data_clear(&data);
object_array_clear(&have_obj);
object_array_clear(&want_obj);
+ list_objects_filter_release(&filter_options);
return 0;
}
--
2.26.2.562.ga2ab1ba363
next reply other threads:[~2020-05-07 9:58 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-07 9:58 Christian Couder [this message]
2020-05-07 11:36 ` [RFC PATCH] upload-pack: fix filter options scope Jeff King
2020-05-07 12:32 ` Christian Couder
2020-05-07 14:47 ` Jeff King
2020-05-07 16:42 ` Taylor Blau
2020-05-08 8:01 ` [PATCH v2] " Christian Couder
2020-05-08 13:06 ` Jeff King
2020-05-08 15:46 ` Junio C Hamano
2020-05-08 17:16 ` Jeff King
2020-05-08 18:00 ` Christian Couder
2020-05-08 18:11 ` Junio C Hamano
2020-05-08 18:09 ` [PATCH v3] upload-pack: clear filter_options for each v2 fetch command Christian Couder
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200507095829.16894-1-chriscool@tuxfamily.org \
--to=christian.couder@gmail.com \
--cc=chriscool@tuxfamily.org \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.com \
--cc=jrnieder@gmail.com \
--cc=me@ttaylorr.com \
--cc=peff@peff.net \
/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.
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).