Re: [PATCH 18/20] builtin/am: convert to struct object_id

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 2446 bytes --]

On Mon, Aug 29, 2016 at 03:02:56PM +0800, Paul Tan wrote:
> Hi Brian,
> 
> On Mon, Aug 29, 2016 at 7:27 AM, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> >  builtin/am.c | 138 +++++++++++++++++++++++++++++------------------------------
> >  1 file changed, 69 insertions(+), 69 deletions(-)
> 
> I looked through this patch, and the conversion looks faithful and
> straightforward to me. Just two minor comments:
> 
> > diff --git a/builtin/am.c b/builtin/am.c
> > index 739b34dc..632d4288 100644
> > --- a/builtin/am.c
> > +++ b/builtin/am.c
> > @@ -1053,10 +1053,10 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
> >         else
> >                 write_state_text(state, "applying", "");
> >
> > -       if (!get_sha1("HEAD", curr_head)) {
> > -               write_state_text(state, "abort-safety", sha1_to_hex(curr_head));
> > +       if (!get_oid("HEAD", &curr_head)) {
> > +               write_state_text(state, "abort-safety", oid_to_hex(&curr_head));
> >                 if (!state->rebasing)
> > -                       update_ref("am", "ORIG_HEAD", curr_head, NULL, 0,
> > +                       update_ref("am", "ORIG_HEAD", curr_head.hash, NULL, 0,
> >                                         UPDATE_REFS_DIE_ON_ERR);
> 
> I noticed that you used update_ref_oid() in other places of this
> patch. Perhaps this should use update_ref_oid() as well for
> consistency?

I'll do that in a reroll.

> > @@ -1665,9 +1665,8 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa
> >   */
> >  static void do_commit(const struct am_state *state)
> >  {
> > -       unsigned char tree[GIT_SHA1_RAWSZ], parent[GIT_SHA1_RAWSZ],
> > -                     commit[GIT_SHA1_RAWSZ];
> > -       unsigned char *ptr;
> > +       struct object_id tree, parent, commit;
> > +       struct object_id *ptr;
> 
> Ah, I just noticed that this is a very poorly named variable. Whoops.
> Since we are here, should we rename this to something like "old_oid"?
> Also, this should probably be a "const struct object_id *" as well, I
> think.

I'll fix that.  Thanks for the review.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]