Hi Brian,
On Mon, Aug 29, 2016 at 7:27 AM, brian m. carlson
<[email protected]> wrote:
> Signed-off-by: brian m. carlson <[email protected]>
> ---
> 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.
Thanks,
Paul
