Re: [PATCH 06/12] builtin/update_ref: convert to struct object_id

[brian m. carlson]

On Mon, Jul 03, 2017 at 10:49:39PM +0200, Ævar Arnfjörð Bjarmason wrote:
> On Mon, Jul 03 2017, brian m. carlson jotted:
> > [...]
> >  1 file changed, 34 insertions(+), 35 deletions(-)
> > [...]
> > struct strbuf err = STRBUF_INIT;
> > char *refname;
> > -   unsigned char new_sha1[20];
> > -   unsigned char old_sha1[20];
> > +   struct object_id new_oid, old_oid;
> > [...]
> 
> It's easier to skim these when you leave changes in the number of lines
> to separate commits which do more than just rename boilerplate code,
> e.g. as in 05/12 where `const char *p` is introduced.

I can do that in the future.  I've received feedback in the past that I
could coalesce them into one line instead of needing multiple lines, but
since I don't have a preference either way, I'm happy to do whatever
makes review easier.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH 06/12] builtin/update_ref: convert to struct object_id

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 03 2017, brian m. carlson jotted:

> Convert the uses of unsigned char * to struct object_id.

I read all of this over (but didn't apply/test it) and it looks good to
me, Just a small nit:

> [...]
>  1 file changed, 34 insertions(+), 35 deletions(-)
> [...]
>   struct strbuf err = STRBUF_INIT;
>   char *refname;
> - unsigned char new_sha1[20];
> - unsigned char old_sha1[20];
> + struct object_id new_oid, old_oid;
> [...]

It's easier to skim these when you leave changes in the number of lines
to separate commits which do more than just rename boilerplate code,
e.g. as in 05/12 where `const char *p` is introduced.

Thanks for working on this.

[PATCH 06/12] builtin/update_ref: convert to struct object_id

[brian m. carlson]

Convert the uses of unsigned char * to struct object_id.

Signed-off-by: brian m. carlson 
---
 builtin/update-ref.c | 69 ++--
 1 file changed, 34 insertions(+), 35 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 40ccfc193..6b90c5dea 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -94,10 +94,10 @@ static char *parse_refname(struct strbuf *input, const char 
**next)
  * provided but cannot be converted to a SHA-1, die.  flags can
  * include PARSE_SHA1_OLD and/or PARSE_SHA1_ALLOW_EMPTY.
  */
-static int parse_next_sha1(struct strbuf *input, const char **next,
-  unsigned char *sha1,
-  const char *command, const char *refname,
-  int flags)
+static int parse_next_oid(struct strbuf *input, const char **next,
+ struct object_id *oid,
+ const char *command, const char *refname,
+ int flags)
 {
struct strbuf arg = STRBUF_INIT;
int ret = 0;
@@ -115,11 +115,11 @@ static int parse_next_sha1(struct strbuf *input, const 
char **next,
(*next)++;
*next = parse_arg(*next, );
if (arg.len) {
-   if (get_sha1(arg.buf, sha1))
+   if (get_oid(arg.buf, oid))
goto invalid;
} else {
/* Without -z, an empty value means all zeros: */
-   hashclr(sha1);
+   oidclr(oid);
}
} else {
/* With -z, read the next NUL-terminated line */
@@ -133,13 +133,13 @@ static int parse_next_sha1(struct strbuf *input, const 
char **next,
*next += arg.len;
 
if (arg.len) {
-   if (get_sha1(arg.buf, sha1))
+   if (get_oid(arg.buf, oid))
goto invalid;
} else if (flags & PARSE_SHA1_ALLOW_EMPTY) {
/* With -z, treat an empty value as all zeros: */
warning("%s %s: missing , treating as zero",
command, refname);
-   hashclr(sha1);
+   oidclr(oid);
} else {
/*
 * With -z, an empty non-required value means
@@ -182,26 +182,25 @@ static const char *parse_cmd_update(struct 
ref_transaction *transaction,
 {
struct strbuf err = STRBUF_INIT;
char *refname;
-   unsigned char new_sha1[20];
-   unsigned char old_sha1[20];
+   struct object_id new_oid, old_oid;
int have_old;
 
refname = parse_refname(input, );
if (!refname)
die("update: missing ");
 
-   if (parse_next_sha1(input, , new_sha1, "update", refname,
-   PARSE_SHA1_ALLOW_EMPTY))
+   if (parse_next_oid(input, , _oid, "update", refname,
+  PARSE_SHA1_ALLOW_EMPTY))
die("update %s: missing ", refname);
 
-   have_old = !parse_next_sha1(input, , old_sha1, "update", refname,
-   PARSE_SHA1_OLD);
+   have_old = !parse_next_oid(input, , _oid, "update", refname,
+  PARSE_SHA1_OLD);
 
if (*next != line_termination)
die("update %s: extra input: %s", refname, next);
 
if (ref_transaction_update(transaction, refname,
-  new_sha1, have_old ? old_sha1 : NULL,
+  new_oid.hash, have_old ? old_oid.hash : NULL,
   update_flags | create_reflog_flag,
   msg, ))
die("%s", err.buf);
@@ -218,22 +217,22 @@ static const char *parse_cmd_create(struct 
ref_transaction *transaction,
 {
struct strbuf err = STRBUF_INIT;
char *refname;
-   unsigned char new_sha1[20];
+   struct object_id new_oid;
 
refname = parse_refname(input, );
if (!refname)
die("create: missing ");
 
-   if (parse_next_sha1(input, , new_sha1, "create", refname, 0))
+   if (parse_next_oid(input, , _oid, "create", refname, 0))
die("create %s: missing ", refname);
 
-   if (is_null_sha1(new_sha1))
+   if (is_null_oid(_oid))
die("create %s: zero ", refname);
 
if (*next != line_termination)
die("create %s: extra input: %s", refname, next);
 
-   if (ref_transaction_create(transaction, refname, new_sha1,
+   if (ref_transaction_create(transaction, refname, new_oid.hash,
   update_flags | create_reflog_flag,
   msg, ))
die("%s", err.buf);
@@ -250,18 +249,18 @@ static