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

2016-08-29 Thread brian m. carlson 
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
>  wrote:
> > Signed-off-by: brian m. carlson 
> > ---
> >  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", _head)) {
> > +   write_state_text(state, "abort-safety", 
> > oid_to_hex(_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


signature.asc
Description: PGP signature

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

2016-08-29 Thread Paul Tan 
Hi Brian,

On Mon, Aug 29, 2016 at 7:27 AM, brian m. carlson
 wrote:
> Signed-off-by: brian m. carlson 
> ---
>  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", _head)) {
> +   write_state_text(state, "abort-safety", 
> oid_to_hex(_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

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

2016-08-28 Thread brian m. carlson 
Signed-off-by: brian m. carlson 
---
 builtin/am.c | 138 +--
 1 file changed, 69 insertions(+), 69 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 739b34dc..632d4288 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -108,7 +108,7 @@ struct am_state {
size_t msg_len;
 
/* when --rebasing, records the original commit the patch came from */
-   unsigned char orig_commit[GIT_SHA1_RAWSZ];
+   struct object_id orig_commit;
 
/* number of digits in patch filename */
int prec;
@@ -428,8 +428,8 @@ static void am_load(struct am_state *state)
read_commit_msg(state);
 
if (read_state_file(, state, "original-commit", 1) < 0)
-   hashclr(state->orig_commit);
-   else if (get_sha1_hex(sb.buf, state->orig_commit) < 0)
+   oidclr(>orig_commit);
+   else if (get_oid_hex(sb.buf, >orig_commit) < 0)
die(_("could not parse %s"), am_path(state, "original-commit"));
 
read_state_file(, state, "threeway", 1);
@@ -555,14 +555,14 @@ static int copy_notes_for_rebase(const struct am_state 
*state)
fp = xfopen(am_path(state, "rewritten"), "r");
 
while (!strbuf_getline_lf(, fp)) {
-   unsigned char from_obj[GIT_SHA1_RAWSZ], to_obj[GIT_SHA1_RAWSZ];
+   struct object_id from_obj, to_obj;
 
if (sb.len != GIT_SHA1_HEXSZ * 2 + 1) {
ret = error(invalid_line, sb.buf);
goto finish;
}
 
-   if (get_sha1_hex(sb.buf, from_obj)) {
+   if (get_oid_hex(sb.buf, _obj)) {
ret = error(invalid_line, sb.buf);
goto finish;
}
@@ -572,14 +572,14 @@ static int copy_notes_for_rebase(const struct am_state 
*state)
goto finish;
}
 
-   if (get_sha1_hex(sb.buf + GIT_SHA1_HEXSZ + 1, to_obj)) {
+   if (get_oid_hex(sb.buf + GIT_SHA1_HEXSZ + 1, _obj)) {
ret = error(invalid_line, sb.buf);
goto finish;
}
 
-   if (copy_note_for_rewrite(c, from_obj, to_obj))
+   if (copy_note_for_rewrite(c, from_obj.hash, to_obj.hash))
ret = error(_("Failed to copy notes from '%s' to '%s'"),
-   sha1_to_hex(from_obj), 
sha1_to_hex(to_obj));
+   oid_to_hex(_obj), 
oid_to_hex(_obj));
}
 
 finish:
@@ -985,7 +985,7 @@ static int split_mail(struct am_state *state, enum 
patch_format patch_format,
 static void am_setup(struct am_state *state, enum patch_format patch_format,
const char **paths, int keep_cr)
 {
-   unsigned char curr_head[GIT_SHA1_RAWSZ];
+   struct object_id curr_head;
const char *str;
struct strbuf sb = STRBUF_INIT;
 
@@ -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", _head)) {
+   write_state_text(state, "abort-safety", oid_to_hex(_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);
} else {
write_state_text(state, "abort-safety", "");
@@ -1081,7 +1081,7 @@ static void am_setup(struct am_state *state, enum 
patch_format patch_format,
  */
 static void am_next(struct am_state *state)
 {
-   unsigned char head[GIT_SHA1_RAWSZ];
+   struct object_id head;
 
free(state->author_name);
state->author_name = NULL;
@@ -1099,11 +1099,11 @@ static void am_next(struct am_state *state)
unlink(am_path(state, "author-script"));
unlink(am_path(state, "final-commit"));
 
-   hashclr(state->orig_commit);
+   oidclr(>orig_commit);
unlink(am_path(state, "original-commit"));
 
-   if (!get_sha1("HEAD", head))
-   write_state_text(state, "abort-safety", sha1_to_hex(head));
+   if (!get_oid("HEAD", ))
+   write_state_text(state, "abort-safety", oid_to_hex());
else
write_state_text(state, "abort-safety", "");
 
@@ -1145,17 +1145,17 @@ static void refresh_and_write_cache(void)
  */
 static int index_has_changes(struct strbuf *sb)
 {
-   unsigned char head[GIT_SHA1_RAWSZ];
+   struct object_id head;
int i;
 
-   if (!get_sha1_tree("HEAD", head)) {
+   if (!get_sha1_tree("HEAD", head.hash)) {
struct diff_options opt;