Re: [PATCH v2 8/8] fetch-pack: implement ref-in-want
Hi,
Brandon Williams wrote:
> On 06/14, Stefan Beller wrote:
> > On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams wrote:
>>> + for (r = refs; r; r = r->next) {
>>> + if (!strcmp(end, r->name)) {
>>> + oidcpy(&r->old_oid, &oid);
>>> + break;
>>> + }
>>> + }
>>
>> The server is documented as MUST NOT send additional refs,
>> which is fine here, as we'd have no way of storing them anyway.
>> Do we want to issue a warning, though?
>>
>> if (!r) /* never break'd */
>> warning ("server send unexpected line '%s'", reader.line);
>
> Depends, does this warning help out the end user or do you think it
> would confuse users to see this and still have their fetch succeed?
I think we'd want to error out instead of warning. That keeps the
spec simple and that way, server implementors will notice early if
they are doing something that clients aren't going to understand
anyway, which would benefit users.
Thanks,
Jonathan
Re: [PATCH v2 8/8] fetch-pack: implement ref-in-want
On 06/14, Stefan Beller wrote:
> On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams wrote:
>
> > +static void receive_wanted_refs(struct packet_reader *reader, struct ref
> > *refs)
> > +{
> ...
> > +
> > + for (r = refs; r; r = r->next) {
> > + if (!strcmp(end, r->name)) {
> > + oidcpy(&r->old_oid, &oid);
> > + break;
> > + }
> > + }
>
> The server is documented as MUST NOT send additional refs,
> which is fine here, as we'd have no way of storing them anyway.
> Do we want to issue a warning, though?
>
> if (!r) /* never break'd */
> warning ("server send unexpected line '%s'", reader.line);
Depends, does this warning help out the end user or do you think it
would confuse users to see this and still have their fetch succeed?
>
>
>
> > diff --git a/remote.c b/remote.c
> > index abe80c139..c9d452ac0 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -1735,6 +1735,7 @@ int get_fetch_map(const struct ref *remote_refs,
> > if (refspec->exact_sha1) {
> > ref_map = alloc_ref(name);
> > get_oid_hex(name, &ref_map->old_oid);
> > + ref_map->exact_sha1 = 1;
> > } else {
> > ref_map = get_remote_ref(remote_refs, name);
> > }
> > diff --git a/remote.h b/remote.h
> > index 45ecc6cef..e5338e368 100644
> > --- a/remote.h
> > +++ b/remote.h
> > @@ -73,6 +73,7 @@ struct ref {
> > force:1,
> > forced_update:1,
> > expect_old_sha1:1,
> > + exact_sha1:1,
>
> Can we rename that to exact_oid ?
I'll fix this.
--
Brandon Williams
Re: [PATCH v2 8/8] fetch-pack: implement ref-in-want
On Wed, Jun 13, 2018 at 2:39 PM Brandon Williams wrote:
> +static void receive_wanted_refs(struct packet_reader *reader, struct ref
> *refs)
> +{
...
> +
> + for (r = refs; r; r = r->next) {
> + if (!strcmp(end, r->name)) {
> + oidcpy(&r->old_oid, &oid);
> + break;
> + }
> + }
The server is documented as MUST NOT send additional refs,
which is fine here, as we'd have no way of storing them anyway.
Do we want to issue a warning, though?
if (!r) /* never break'd */
warning ("server send unexpected line '%s'", reader.line);
> diff --git a/remote.c b/remote.c
> index abe80c139..c9d452ac0 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1735,6 +1735,7 @@ int get_fetch_map(const struct ref *remote_refs,
> if (refspec->exact_sha1) {
> ref_map = alloc_ref(name);
> get_oid_hex(name, &ref_map->old_oid);
> + ref_map->exact_sha1 = 1;
> } else {
> ref_map = get_remote_ref(remote_refs, name);
> }
> diff --git a/remote.h b/remote.h
> index 45ecc6cef..e5338e368 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -73,6 +73,7 @@ struct ref {
> force:1,
> forced_update:1,
> expect_old_sha1:1,
> + exact_sha1:1,
Can we rename that to exact_oid ?
(bonus points for also converting expect_old_sha1)
Thanks,
Stefan
[PATCH v2 8/8] fetch-pack: implement ref-in-want
Implement ref-in-want on the client side so that when a server supports
the "ref-in-want" feature, a client will send "want-ref" lines for each
reference the client wants to fetch.
Signed-off-by: Brandon Williams
---
fetch-pack.c | 35 +++---
remote.c | 1 +
remote.h | 1 +
t/t5703-upload-pack-ref-in-want.sh | 4 ++--
4 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/fetch-pack.c b/fetch-pack.c
index 7799ee2cd..51e8356ba 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1102,9 +1102,10 @@ static void add_shallow_requests(struct strbuf *req_buf,
static void add_wants(const struct ref *wants, struct strbuf *req_buf)
{
+ int use_ref_in_want = server_supports_feature("fetch", "ref-in-want",
0);
+
for ( ; wants ; wants = wants->next) {
const struct object_id *remote = &wants->old_oid;
- const char *remote_hex;
struct object *o;
/*
@@ -1122,8 +1123,10 @@ static void add_wants(const struct ref *wants, struct
strbuf *req_buf)
continue;
}
- remote_hex = oid_to_hex(remote);
- packet_buf_write(req_buf, "want %s\n", remote_hex);
+ if (!use_ref_in_want || wants->exact_sha1)
+ packet_buf_write(req_buf, "want %s\n",
oid_to_hex(remote));
+ else
+ packet_buf_write(req_buf, "want-ref %s\n", wants->name);
}
}
@@ -1334,6 +1337,29 @@ static void receive_shallow_info(struct fetch_pack_args
*args,
args->deepen = 1;
}
+static void receive_wanted_refs(struct packet_reader *reader, struct ref *refs)
+{
+ process_section_header(reader, "wanted-refs", 0);
+ while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
+ struct object_id oid;
+ const char *end;
+ struct ref *r = NULL;
+
+ if (parse_oid_hex(reader->line, &oid, &end) || *end++ != ' ')
+ die("expected wanted-ref, got '%s'", reader->line);
+
+ for (r = refs; r; r = r->next) {
+ if (!strcmp(end, r->name)) {
+ oidcpy(&r->old_oid, &oid);
+ break;
+ }
+ }
+ }
+
+ if (reader->status != PACKET_READ_DELIM)
+ die("error processing wanted refs: %d", reader->status);
+}
+
enum fetch_state {
FETCH_CHECK_LOCAL = 0,
FETCH_SEND_REQUEST,
@@ -1408,6 +1434,9 @@ static struct ref *do_fetch_pack_v2(struct
fetch_pack_args *args,
if (process_section_header(&reader, "shallow-info", 1))
receive_shallow_info(args, &reader);
+ if (process_section_header(&reader, "wanted-refs", 1))
+ receive_wanted_refs(&reader, ref);
+
/* get the pack */
process_section_header(&reader, "packfile", 0);
if (get_pack(args, fd, pack_lockfile))
diff --git a/remote.c b/remote.c
index abe80c139..c9d452ac0 100644
--- a/remote.c
+++ b/remote.c
@@ -1735,6 +1735,7 @@ int get_fetch_map(const struct ref *remote_refs,
if (refspec->exact_sha1) {
ref_map = alloc_ref(name);
get_oid_hex(name, &ref_map->old_oid);
+ ref_map->exact_sha1 = 1;
} else {
ref_map = get_remote_ref(remote_refs, name);
}
diff --git a/remote.h b/remote.h
index 45ecc6cef..e5338e368 100644
--- a/remote.h
+++ b/remote.h
@@ -73,6 +73,7 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
+ exact_sha1:1,
deletion:1;
enum {
diff --git a/t/t5703-upload-pack-ref-in-want.sh
b/t/t5703-upload-pack-ref-in-want.sh
index 979ab6d03..b94a51380 100755
--- a/t/t5703-upload-pack-ref-in-want.sh
+++ b/t/t5703-upload-pack-ref-in-want.sh
@@ -204,7 +204,7 @@ test_expect_success 'server is initially ahead - no ref in
want' '
grep "ERR upload-pack: not our ref" err
'
-test_expect_failure 'server is initially ahead - ref in want' '
+test_expect_success 'server is initially ahead - ref in want' '
git -C "$REPO" config uploadpack.allowRefInWant true &&
rm -rf local &&
cp -r "$LOCAL_PRISTINE" local &&
@@ -228,7 +228,7 @@ test_expect_success 'server is initially behind - no ref in
want' '
test_cmp expected actual
'
-test_expect_failure 'server is initially behind - ref in want' '
+test_expect_success 'server is initially behind - ref in want' '
git -C "$REPO" config uploadpack.allowRefInWant true &&
rm -rf local &&
cp -r "$LOCAL_PRISTINE" local &&
--
2.18.0.rc1.242.g61856ae69a-goog
