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

From: Paul Tan <pyokagan@gmail.com>
Date: Aug 29, 2016, 07:02 UTC
Message-ID: <CACRoPnQvdq3xaRF9niU-b0qLxVCvmbpv2_roxUaEaDFftt7_wQ@mail.gmail.com>
In-Reply-To: <20160828232757.373278-19-sandals@crustytoothpaste.net>

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:

Show 16 quoted lines

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

Show 9 quoted lines

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

Thanks, Paul