Re: [PATCH v3 14/35] connect: request remote refs using v2

[Junio C Hamano]

Jeff King  writes:

>> struct strs {...};
>> 
>> void strs_init(struct strs *);
>> void strs_push(struct strs *, const char *);
>> void strs_pushf(struct strs *, const char *fmt, ...);
>> void strs_pushl(struct strs *, ...);
>> void strs_pushv(struct strs *, const char **);
>> void strs_pop(struct strs *);
>> void strs_clear(struct strs *);
>> const char **strs_detach(struct strs *);
>> 
>> ...is short, feels pretty natural, and doesn't require understanding
>> "v" for "vector".
>
> Not bad. The "v" carries the information that it _is_ a NULL-terminated
> vector and not some other list-like structure (and so is suitable for
> feeding to execv, etc). But that may just be obvious from looking at its
> uses and documentation.

And with "v", it probably is obvious without looking at its uses and
documentation, so... ;-)

Re: [PATCH v3 14/35] connect: request remote refs using v2

[Jeff King]

On Tue, Feb 27, 2018 at 05:10:09PM -0500, Eric Sunshine wrote:

> > That would be fine with me. Though I would love it if we could find a
> > shorter name for the associated functions. For example,
> > argv_array_pushf() can make lines quite long, and something like
> > argv_pushf() is easier to read (in my opinion). And that might work
> > because "argv" is pretty unique by itself, but "string" is not.
> >
> > Some one-word name like "strarray" might work, though I find that is not
> > quite catchy. I guess "strv" is short if you assume that people know the
> > "v" suffix means "vector".
> 
> struct strs {...};
> 
> void strs_init(struct strs *);
> void strs_push(struct strs *, const char *);
> void strs_pushf(struct strs *, const char *fmt, ...);
> void strs_pushl(struct strs *, ...);
> void strs_pushv(struct strs *, const char **);
> void strs_pop(struct strs *);
> void strs_clear(struct strs *);
> const char **strs_detach(struct strs *);
> 
> ...is short, feels pretty natural, and doesn't require understanding
> "v" for "vector".

Not bad. The "v" carries the information that it _is_ a NULL-terminated
vector and not some other list-like structure (and so is suitable for
feeding to execv, etc). But that may just be obvious from looking at its
uses and documentation.

-Peff

Re: [PATCH v3 14/35] connect: request remote refs using v2

[Eric Sunshine]

On Tue, Feb 27, 2018 at 5:04 PM, Jeff King  wrote:
> On Tue, Feb 27, 2018 at 01:58:00PM -0800, Junio C Hamano wrote:
>> So are we looking for a natural name to call an array of trings?  I
>> personally do not mind argv_array too much, but perhaps we can call
>> it a string_array and then everybody will be happy?
>
> That would be fine with me. Though I would love it if we could find a
> shorter name for the associated functions. For example,
> argv_array_pushf() can make lines quite long, and something like
> argv_pushf() is easier to read (in my opinion). And that might work
> because "argv" is pretty unique by itself, but "string" is not.
>
> Some one-word name like "strarray" might work, though I find that is not
> quite catchy. I guess "strv" is short if you assume that people know the
> "v" suffix means "vector".

struct strs {...};

void strs_init(struct strs *);
void strs_push(struct strs *, const char *);
void strs_pushf(struct strs *, const char *fmt, ...);
void strs_pushl(struct strs *, ...);
void strs_pushv(struct strs *, const char **);
void strs_pop(struct strs *);
void strs_clear(struct strs *);
const char **strs_detach(struct strs *);

...is short, feels pretty natural, and doesn't require understanding
"v" for "vector".

Re: [PATCH v3 14/35] connect: request remote refs using v2

[Jeff King]

On Tue, Feb 27, 2018 at 01:58:00PM -0800, Junio C Hamano wrote:

> Jonathan Nieder  writes:
> 
> > Jonathan Tan wrote:
> >> On Thu, 22 Feb 2018 13:26:58 -0500
> >> Jeff King  wrote:
> >
> >>> I agree that it shouldn't matter much here. But if the name argv_array
> >>> is standing in the way of using it, I think we should consider giving it
> >>> a more general name. I picked that not to evoke "this must be arguments"
> >>> but "this is terminated by a single NULL".
> > [...]
> >> This sounds reasonable - I withdraw my comment about using struct
> >> string_list.
> >
> > Marking with #leftoverbits as a reminder to think about what such a
> > more general name would be (or what kind of docs to put in
> > argv-array.h) and make it so the next time I do a search for that
> > keyword.
> 
> So are we looking for a natural name to call an array of trings?  I
> personally do not mind argv_array too much, but perhaps we can call
> it a string_array and then everybody will be happy?

That would be fine with me. Though I would love it if we could find a
shorter name for the associated functions. For example,
argv_array_pushf() can make lines quite long, and something like
argv_pushf() is easier to read (in my opinion). And that might work
because "argv" is pretty unique by itself, but "string" is not.

Some one-word name like "strarray" might work, though I find that is not
quite catchy. I guess "strv" is short if you assume that people know the
"v" suffix means "vector".

It may not be worth worrying too much about, though. We already have
24-character monstrosities like string_list_append_nodup(). ;)

-Peff

Re: [PATCH v3 14/35] connect: request remote refs using v2

[Junio C Hamano]

Jonathan Nieder  writes:

> Jonathan Tan wrote:
>> On Thu, 22 Feb 2018 13:26:58 -0500
>> Jeff King  wrote:
>
>>> I agree that it shouldn't matter much here. But if the name argv_array
>>> is standing in the way of using it, I think we should consider giving it
>>> a more general name. I picked that not to evoke "this must be arguments"
>>> but "this is terminated by a single NULL".
> [...]
>> This sounds reasonable - I withdraw my comment about using struct
>> string_list.
>
> Marking with #leftoverbits as a reminder to think about what such a
> more general name would be (or what kind of docs to put in
> argv-array.h) and make it so the next time I do a search for that
> keyword.

So are we looking for a natural name to call an array of trings?  I
personally do not mind argv_array too much, but perhaps we can call
it a string_array and then everybody will be happy?

Re: [PATCH v3 14/35] connect: request remote refs using v2

[Brandon Williams]

On 02/26, Jonathan Nieder wrote:
> Brandon Williams wrote:
> >  static char *server_capabilities;
> > +static struct argv_array server_capabilities_v2 = ARGV_ARRAY_INIT;
> 
> Can a quick doc comment describe these and how they relate?
> 
> Is only one of them set, based on which protocol version is in use?
> Should server_capabilities be renamed to server_capabilities_v1?

yes that's correct.  I can rename it.

> > +{
> > +   int ret = 1;
> > +   int i = 0;
> > +   struct object_id old_oid;
> > +   struct ref *ref;
> > +   struct string_list line_sections = STRING_LIST_INIT_DUP;
> > +
> > +   if (string_list_split(_sections, line, ' ', -1) < 2) {
> 
> Can there be a comment describing the expected format?

Yep I'll write a comment up.

> > +
> > +   oidcpy(>old_oid, _oid);
> > +   **list = ref;
> > +   *list = >next;
> > +
> > +   for (; i < line_sections.nr; i++) {
> > +   const char *arg = line_sections.items[i].string;
> > +   if (skip_prefix(arg, "symref-target:", ))
> > +   ref->symref = xstrdup(arg);
> 
> Using space-delimited fields in a single pkt-line means that
> - values cannot contains a space
> - total length is limited by the size of a pkt-line
> 
> Given the context, I think that's fine.  More generally it is tempting
> to use a pkt-line per field to avoid the trouble v1 had with
> capability lists crammed into a pkt-line, but I see why you used a
> pkt-line per ref to avoid having to have sections-within-a-section.
> 
> My only potential worry is the length part: do we have an explicit
> limit on the length of a ref name?  git-check-ref-format(1) doesn't
> mention one.  A 32k ref name would be a bit ridiculous, though.

Yeah I think we're fine for now, mostly because we're out of luck with
the current protocol as it is.

> 
> > +
> > +   if (skip_prefix(arg, "peeled:", )) {
> > +   struct object_id peeled_oid;
> > +   char *peeled_name;
> > +   struct ref *peeled;
> > +   if (get_oid_hex(arg, _oid)) {
> > +   ret = 0;
> > +   goto out;
> > +   }
> 
> Can this also check that there's no trailing garbage after the oid?

Yeah I do that.

> 
> > +
> > +   packet_delim(fd_out);
> > +   /* When pushing we don't want to request the peeled tags */
> 
> Can you say more about this?  In theory it would be nice to have the
> peeled tags since they name commits whose history can be excluded from
> the pack.

I don't believe peeled refs are sent now in v0 for push.

> 
> > +   if (!for_push)
> > +   packet_write_fmt(fd_out, "peel\n");
> > +   packet_write_fmt(fd_out, "symrefs\n");
> 
> Are symrefs useful during push?

They may be at a later point in time when you want to update a symref :)

> 
> > +   for (i = 0; ref_patterns && i < ref_patterns->argc; i++) {
> > +   packet_write_fmt(fd_out, "ref-pattern %s\n",
> > +ref_patterns->argv[i]);
> > +   }
> 
> The exciting part.
> 
> Why do these pkts end with \n?  I would have expected the pkt-line
> framing to work to delimit them.

All pkts end with \n, that's just hows its been since v0.  Though the
server isn't supposed to complain if they don't contain newlines.

> 
> > +   packet_flush(fd_out);
> > +
> > +   /* Process response from server */
> > +   while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
> > +   if (!process_ref_v2(reader->line, ))
> > +   die("invalid ls-refs response: %s", reader->line);
> > +   }
> > +
> > +   if (reader->status != PACKET_READ_FLUSH)
> > +   die("protocol error");
> 
> Can this protocol error give more detail?  When diagnosing an error in
> servers, proxies, or lower-level networking issues, informative protocol
> errors can be very helpful (similar to syntax errors from a compiler).

I'll update the  error msg.

> [...]
> > --- a/remote.h
> > +++ b/remote.h
> > @@ -151,10 +151,14 @@ void free_refs(struct ref *ref);
> >  
> >  struct oid_array;
> >  struct packet_reader;
> > +struct argv_array;
> >  extern struct ref **get_remote_heads(struct packet_reader *reader,
> >  struct ref **list, unsigned int flags,
> >  struct oid_array *extra_have,
> >  struct oid_array *shallow_points);
> > +extern struct ref **get_remote_refs(int fd_out, struct packet_reader 
> > *reader,
> > +   struct ref **list, int for_push,
> > +   const struct argv_array *ref_patterns);
> 
> What is the difference between get_remote_heads and get_remote_refs?
> A comment might help.  (BTW, thanks for making the new saner name to
> replace get_remote_heads!)

I'll add a comment saying its used in v2 to retrieve a list of refs from
the remote.

-- 
Brandon Williams

Re: [PATCH v3 14/35] connect: request remote refs using v2

[Jonathan Nieder]

Brandon Williams wrote:

> Teach the client to be able to request a remote's refs using protocol
> v2.  This is done by having a client issue a 'ls-refs' request to a v2
> server.

Yay, ls-remote support!

[...]
> --- a/builtin/upload-pack.c
> +++ b/builtin/upload-pack.c
> @@ -5,6 +5,7 @@
>  #include "parse-options.h"
>  #include "protocol.h"
>  #include "upload-pack.h"
> +#include "serve.h"

nit, no change needed in this patch: What is a good logical order for
the #includes here?  Bonus points if there's a tool to make it happen
automatically.

Asking since adding #includes like this at the end tends to result in
a harder-to-read list of #includes, sometimes with duplicates, and
often producing conflicts when multiple patches in flight add a
#include to the same file.

[...]
> @@ -48,11 +50,9 @@ int cmd_upload_pack(int argc, const char **argv, const 
> char *prefix)
>  
>   switch (determine_protocol_version_server()) {
>   case protocol_v2:
> - /*
> -  * fetch support for protocol v2 has not been implemented yet,
> -  * so ignore the request to use v2 and fallback to using v0.
> -  */
> - upload_pack();
> + serve_opts.advertise_capabilities = opts.advertise_refs;
> + serve_opts.stateless_rpc = opts.stateless_rpc;
> + serve(_opts);

Interesting!  daemon.c has its own (file-local) serve() function;
can one of the two be renamed?

I actually like both names: serve() is a traditional name for the
function a server calls when it's done setting up and is ready to
serve.  But the clash is confusing.

[...]
> +++ b/connect.c
> @@ -12,9 +12,11 @@
>  #include "sha1-array.h"
>  #include "transport.h"
>  #include "strbuf.h"
> +#include "version.h"
>  #include "protocol.h"
>  
>  static char *server_capabilities;
> +static struct argv_array server_capabilities_v2 = ARGV_ARRAY_INIT;

Can a quick doc comment describe these and how they relate?

Is only one of them set, based on which protocol version is in use?
Should server_capabilities be renamed to server_capabilities_v1?

>  static const char *parse_feature_value(const char *, const char *, int *);
>  
>  static int check_ref(const char *name, unsigned int flags)
> @@ -62,6 +64,33 @@ static void die_initial_contact(int unexpected)
> "and the repository exists."));
>  }
>  
> +/* Checks if the server supports the capability 'c' */
> +int server_supports_v2(const char *c, int die_on_error)
> +{
> + int i;
> +
> + for (i = 0; i < server_capabilities_v2.argc; i++) {
> + const char *out;
> + if (skip_prefix(server_capabilities_v2.argv[i], c, ) &&
> + (!*out || *out == '='))
> + return 1;
> + }
> +
> + if (die_on_error)
> + die("server doesn't support '%s'", c);
> +
> + return 0;
> +}

Nice.
> +
> +static void process_capabilities_v2(struct packet_reader *reader)
> +{
> + while (packet_reader_read(reader) == PACKET_READ_NORMAL)
> + argv_array_push(_capabilities_v2, reader->line);
> +
> + if (reader->status != PACKET_READ_FLUSH)
> + die("protocol error");

Can this say more?  E.g. "expected flush after capabilities, got "?

[...]
> @@ -85,7 +114,7 @@ enum protocol_version discover_version(struct 
> packet_reader *reader)
>   /* Maybe process capabilities here, at least for v2 */

Is this comment out of date now?

>   switch (version) {
>   case protocol_v2:
> - die("support for protocol v2 not implemented yet");
> + process_capabilities_v2(reader);
>   break;
>   case protocol_v1:
>   /* Read the peeked version line */
> @@ -293,6 +322,98 @@ struct ref **get_remote_heads(struct packet_reader 
> *reader,
>   return list;
>  }
>  
> +static int process_ref_v2(const char *line, struct ref ***list)

What does the return value represent?

Could it return the more typical 0 on success, -1 on error?

> +{
> + int ret = 1;
> + int i = 0;
> + struct object_id old_oid;
> + struct ref *ref;
> + struct string_list line_sections = STRING_LIST_INIT_DUP;
> +
> + if (string_list_split(_sections, line, ' ', -1) < 2) {

Can there be a comment describing the expected format?

> + ret = 0;
> + goto out;
> + }
> +
> + if (get_oid_hex(line_sections.items[i++].string, _oid)) {
> + ret = 0;
> + goto out;
> + }
> +
> + ref = alloc_ref(line_sections.items[i++].string);

Ref names cannot contains a space, so this is safe.  Good.

> +
> + oidcpy(>old_oid, _oid);
> + **list = ref;
> + *list = >next;
> +
> + for (; i < line_sections.nr; i++) {
> + const char *arg = line_sections.items[i].string;
> + if (skip_prefix(arg, "symref-target:", ))
> + ref->symref = xstrdup(arg);

Using space-delimited fields in a single pkt-line means that
- values 

Re: [PATCH v3 14/35] connect: request remote refs using v2

[Jonathan Nieder]

Jonathan Tan wrote:
> On Thu, 22 Feb 2018 13:26:58 -0500
> Jeff King  wrote:

>> I agree that it shouldn't matter much here. But if the name argv_array
>> is standing in the way of using it, I think we should consider giving it
>> a more general name. I picked that not to evoke "this must be arguments"
>> but "this is terminated by a single NULL".
[...]
> This sounds reasonable - I withdraw my comment about using struct
> string_list.

Marking with #leftoverbits as a reminder to think about what such a
more general name would be (or what kind of docs to put in
argv-array.h) and make it so the next time I do a search for that
keyword.

Thanks,
Jonathan

Re: [PATCH v3 14/35] connect: request remote refs using v2

[Jonathan Tan]

On Thu, 22 Feb 2018 13:26:58 -0500
Jeff King  wrote:

> On Thu, Feb 22, 2018 at 10:19:22AM -0800, Brandon Williams wrote:
> 
> > On 02/21, Jonathan Tan wrote:
> > > On Tue,  6 Feb 2018 17:12:51 -0800
> > > Brandon Williams  wrote:
> > > 
> > > > +extern struct ref **get_remote_refs(int fd_out, struct packet_reader 
> > > > *reader,
> > > > +   struct ref **list, int for_push,
> > > > +   const struct argv_array 
> > > > *ref_patterns);
> > > 
> > > I haven't looked at the rest of this patch in detail, but the type of
> > > ref_patterns is probably better as struct string_list, since this is not
> > > a true argument array (e.g. with flags starting with --). Same comment
> > > for the next few patches that deal with ref patterns.
> > 
> > Its just a list of strings which don't require having a util pointer
> > hanging around so actually using an argv_array would be more memory
> > efficient than a string_list.  But either way I don't think it matters
> > much.
> 
> I agree that it shouldn't matter much here. But if the name argv_array
> is standing in the way of using it, I think we should consider giving it
> a more general name. I picked that not to evoke "this must be arguments"
> but "this is terminated by a single NULL".
> 
> In general I think it should be the preferred structure for string
> lists, just because it actually converts for free to the "other" common
> format (whereas you can never pass string_list.items to a function that
> doesn't know about string lists).

This sounds reasonable - I withdraw my comment about using struct
string_list.

Re: [PATCH v3 14/35] connect: request remote refs using v2

[Jeff King]

On Thu, Feb 22, 2018 at 10:19:22AM -0800, Brandon Williams wrote:

> On 02/21, Jonathan Tan wrote:
> > On Tue,  6 Feb 2018 17:12:51 -0800
> > Brandon Williams  wrote:
> > 
> > > +extern struct ref **get_remote_refs(int fd_out, struct packet_reader 
> > > *reader,
> > > + struct ref **list, int for_push,
> > > + const struct argv_array *ref_patterns);
> > 
> > I haven't looked at the rest of this patch in detail, but the type of
> > ref_patterns is probably better as struct string_list, since this is not
> > a true argument array (e.g. with flags starting with --). Same comment
> > for the next few patches that deal with ref patterns.
> 
> Its just a list of strings which don't require having a util pointer
> hanging around so actually using an argv_array would be more memory
> efficient than a string_list.  But either way I don't think it matters
> much.

I agree that it shouldn't matter much here. But if the name argv_array
is standing in the way of using it, I think we should consider giving it
a more general name. I picked that not to evoke "this must be arguments"
but "this is terminated by a single NULL".

In general I think it should be the preferred structure for string
lists, just because it actually converts for free to the "other" common
format (whereas you can never pass string_list.items to a function that
doesn't know about string lists).

-Peff

Re: [PATCH v3 14/35] connect: request remote refs using v2

[Brandon Williams]

On 02/21, Jonathan Tan wrote:
> On Tue,  6 Feb 2018 17:12:51 -0800
> Brandon Williams  wrote:
> 
> > +extern struct ref **get_remote_refs(int fd_out, struct packet_reader 
> > *reader,
> > +   struct ref **list, int for_push,
> > +   const struct argv_array *ref_patterns);
> 
> I haven't looked at the rest of this patch in detail, but the type of
> ref_patterns is probably better as struct string_list, since this is not
> a true argument array (e.g. with flags starting with --). Same comment
> for the next few patches that deal with ref patterns.

Its just a list of strings which don't require having a util pointer
hanging around so actually using an argv_array would be more memory
efficient than a string_list.  But either way I don't think it matters
much.

-- 
Brandon Williams

Re: [PATCH v3 14/35] connect: request remote refs using v2

[Jonathan Tan]

On Tue,  6 Feb 2018 17:12:51 -0800
Brandon Williams  wrote:

> +extern struct ref **get_remote_refs(int fd_out, struct packet_reader *reader,
> + struct ref **list, int for_push,
> + const struct argv_array *ref_patterns);

I haven't looked at the rest of this patch in detail, but the type of
ref_patterns is probably better as struct string_list, since this is not
a true argument array (e.g. with flags starting with --). Same comment
for the next few patches that deal with ref patterns.

[PATCH v3 14/35] connect: request remote refs using v2

[Brandon Williams]

Teach the client to be able to request a remote's refs using protocol
v2.  This is done by having a client issue a 'ls-refs' request to a v2
server.

Signed-off-by: Brandon Williams 
---
 builtin/upload-pack.c  |  10 ++--
 connect.c  | 123 -
 connect.h  |   2 +
 remote.h   |   4 ++
 t/t5702-protocol-v2.sh |  53 +
 transport.c|   2 +-
 6 files changed, 187 insertions(+), 7 deletions(-)
 create mode 100755 t/t5702-protocol-v2.sh

diff --git a/builtin/upload-pack.c b/builtin/upload-pack.c
index 8d53e9794..a757df8da 100644
--- a/builtin/upload-pack.c
+++ b/builtin/upload-pack.c
@@ -5,6 +5,7 @@
 #include "parse-options.h"
 #include "protocol.h"
 #include "upload-pack.h"
+#include "serve.h"
 
 static const char * const upload_pack_usage[] = {
N_("git upload-pack [] "),
@@ -16,6 +17,7 @@ int cmd_upload_pack(int argc, const char **argv, const char 
*prefix)
const char *dir;
int strict = 0;
struct upload_pack_options opts = { 0 };
+   struct serve_options serve_opts = SERVE_OPTIONS_INIT;
struct option options[] = {
OPT_BOOL(0, "stateless-rpc", _rpc,
 N_("quit after a single request/response exchange")),
@@ -48,11 +50,9 @@ int cmd_upload_pack(int argc, const char **argv, const char 
*prefix)
 
switch (determine_protocol_version_server()) {
case protocol_v2:
-   /*
-* fetch support for protocol v2 has not been implemented yet,
-* so ignore the request to use v2 and fallback to using v0.
-*/
-   upload_pack();
+   serve_opts.advertise_capabilities = opts.advertise_refs;
+   serve_opts.stateless_rpc = opts.stateless_rpc;
+   serve(_opts);
break;
case protocol_v1:
/*
diff --git a/connect.c b/connect.c
index f2157a821..7cb1f1df7 100644
--- a/connect.c
+++ b/connect.c
@@ -12,9 +12,11 @@
 #include "sha1-array.h"
 #include "transport.h"
 #include "strbuf.h"
+#include "version.h"
 #include "protocol.h"
 
 static char *server_capabilities;
+static struct argv_array server_capabilities_v2 = ARGV_ARRAY_INIT;
 static const char *parse_feature_value(const char *, const char *, int *);
 
 static int check_ref(const char *name, unsigned int flags)
@@ -62,6 +64,33 @@ static void die_initial_contact(int unexpected)
  "and the repository exists."));
 }
 
+/* Checks if the server supports the capability 'c' */
+int server_supports_v2(const char *c, int die_on_error)
+{
+   int i;
+
+   for (i = 0; i < server_capabilities_v2.argc; i++) {
+   const char *out;
+   if (skip_prefix(server_capabilities_v2.argv[i], c, ) &&
+   (!*out || *out == '='))
+   return 1;
+   }
+
+   if (die_on_error)
+   die("server doesn't support '%s'", c);
+
+   return 0;
+}
+
+static void process_capabilities_v2(struct packet_reader *reader)
+{
+   while (packet_reader_read(reader) == PACKET_READ_NORMAL)
+   argv_array_push(_capabilities_v2, reader->line);
+
+   if (reader->status != PACKET_READ_FLUSH)
+   die("protocol error");
+}
+
 enum protocol_version discover_version(struct packet_reader *reader)
 {
enum protocol_version version = protocol_unknown_version;
@@ -85,7 +114,7 @@ enum protocol_version discover_version(struct packet_reader 
*reader)
/* Maybe process capabilities here, at least for v2 */
switch (version) {
case protocol_v2:
-   die("support for protocol v2 not implemented yet");
+   process_capabilities_v2(reader);
break;
case protocol_v1:
/* Read the peeked version line */
@@ -293,6 +322,98 @@ struct ref **get_remote_heads(struct packet_reader *reader,
return list;
 }
 
+static int process_ref_v2(const char *line, struct ref ***list)
+{
+   int ret = 1;
+   int i = 0;
+   struct object_id old_oid;
+   struct ref *ref;
+   struct string_list line_sections = STRING_LIST_INIT_DUP;
+
+   if (string_list_split(_sections, line, ' ', -1) < 2) {
+   ret = 0;
+   goto out;
+   }
+
+   if (get_oid_hex(line_sections.items[i++].string, _oid)) {
+   ret = 0;
+   goto out;
+   }
+
+   ref = alloc_ref(line_sections.items[i++].string);
+
+   oidcpy(>old_oid, _oid);
+   **list = ref;
+   *list = >next;
+
+   for (; i < line_sections.nr; i++) {
+   const char *arg = line_sections.items[i].string;
+   if (skip_prefix(arg, "symref-target:", ))
+   ref->symref = xstrdup(arg);
+
+   if (skip_prefix(arg, "peeled:", )) {
+   struct object_id peeled_oid;
+