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

> 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?

> > @@ -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.