On 18/02/17 00:06, brian m. carlson wrote:
> Convert most leaf functions to struct object_id. Rewrite several
> hardcoded numbers in terms of GIT_SHA1_HEXSZ, using an intermediate
> variable where that makes sense.
>
> Signed-off-by: brian m. carlson <sandals@xxxxxxxxxxxxxxxxxxxx>
> ---
> builtin/diff-tree.c | 38 ++++++++++++++++++++------------------
> 1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
> index 8ce00480cd..1f1573bb2a 100644
> --- a/builtin/diff-tree.c
> +++ b/builtin/diff-tree.c
> @@ -7,9 +7,9 @@
>
> static struct rev_info log_tree_opt;
>
> -static int diff_tree_commit_sha1(const unsigned char *sha1)
> +static int diff_tree_commit_sha1(const struct object_id *oid)
> {
> - struct commit *commit = lookup_commit_reference(sha1);
> + struct commit *commit = lookup_commit_reference(oid->hash);
> if (!commit)
> return -1;
> return log_tree_commit(&log_tree_opt, commit);
> @@ -18,22 +18,22 @@ static int diff_tree_commit_sha1(const unsigned char *sha1)
> /* Diff one or more commits. */
> static int stdin_diff_commit(struct commit *commit, char *line, int len)
> {
> - unsigned char sha1[20];
> - if (isspace(line[40]) && !get_sha1_hex(line+41, sha1)) {
> + struct object_id oid;
> + if (isspace(line[GIT_SHA1_HEXSZ]) && !get_oid_hex(line+GIT_SHA1_HEXSZ+1, &oid)) {
> /* Graft the fake parents locally to the commit */
> - int pos = 41;
> + int pos = GIT_SHA1_HEXSZ + 1;
> struct commit_list **pptr;
>
> /* Free the real parent list */
> free_commit_list(commit->parents);
> commit->parents = NULL;
> pptr = &(commit->parents);
> - while (line[pos] && !get_sha1_hex(line + pos, sha1)) {
> - struct commit *parent = lookup_commit(sha1);
> + while (line[pos] && !get_oid_hex(line + pos, &oid)) {
> + struct commit *parent = lookup_commit(oid.hash);
> if (parent) {
> pptr = &commit_list_insert(parent, pptr)->next;
> }
> - pos += 41;
> + pos += GIT_SHA1_HEXSZ + 1;
> }
> }
> return log_tree_commit(&log_tree_opt, commit);
> @@ -42,11 +42,13 @@ static int stdin_diff_commit(struct commit *commit, char *line, int len)
> /* Diff two trees. */
> static int stdin_diff_trees(struct tree *tree1, char *line, int len)
> {
> - unsigned char sha1[20];
> + struct object_id oid;
> struct tree *tree2;
> - if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))
> + const int chunksz = GIT_SHA1_HEXSZ + 1;
> + if (len != 2 * chunksz || !isspace(line[chunksz-1]) ||
> + get_sha1_hex(line + chunksz, oid.hash))
I'm not sure that this is an improvement. The input expected in 'line'
is supposed to look like: '<sha1> + <space> + <sha1> + <\n>'. So your
'chunk' would be a <sha1> plus one 'char' of some sort. Except that the
caller of this function has already replaced the newline character with
a '\0' char (so strlen(line) would return 81), but still passes the
original line length! Also, note that this (and other functions in this
file) actually test for 'isspace(char)' rather than for a ' ' char!
Hmm, maybe just:
if (len < (2 * GIT_SHA1_HEXSZ + 1) || line[GIT_SHA1_HEXSZ] != ' ' ||
get_sha1_hex(line + GIT_SHA1_HEXSZ + 1, oid.hash))
(or, perhaps, still call isspace() in this patch ...)
ATB,
Ramsay Jones
> return error("Need exactly two trees, separated by a space");
> - tree2 = lookup_tree(sha1);
> + tree2 = lookup_tree(oid.hash);
> if (!tree2 || parse_tree(tree2))
> return -1;
> printf("%s %s\n", oid_to_hex(&tree1->object.oid),
> @@ -60,15 +62,15 @@ static int stdin_diff_trees(struct tree *tree1, char *line, int len)
> static int diff_tree_stdin(char *line)
> {
> int len = strlen(line);
> - unsigned char sha1[20];
> + struct object_id oid;
> struct object *obj;
>
> if (!len || line[len-1] != '\n')
> return -1;
> line[len-1] = 0;
> - if (get_sha1_hex(line, sha1))
> + if (get_oid_hex(line, &oid))
> return -1;
> - obj = parse_object(sha1);
> + obj = parse_object(oid.hash);
> if (!obj)
> return -1;
> if (obj->type == OBJ_COMMIT)
> @@ -76,7 +78,7 @@ static int diff_tree_stdin(char *line)
> if (obj->type == OBJ_TREE)
> return stdin_diff_trees((struct tree *)obj, line, len);
> error("Object %s is a %s, not a commit or tree",
> - sha1_to_hex(sha1), typename(obj->type));
> + oid_to_hex(&oid), typename(obj->type));
> return -1;
> }
>
> @@ -141,7 +143,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
> break;
> case 1:
> tree1 = opt->pending.objects[0].item;
> - diff_tree_commit_sha1(tree1->oid.hash);
> + diff_tree_commit_sha1(&tree1->oid);
> break;
> case 2:
> tree1 = opt->pending.objects[0].item;
> @@ -164,9 +166,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
> opt->diffopt.setup |= (DIFF_SETUP_USE_SIZE_CACHE |
> DIFF_SETUP_USE_CACHE);
> while (fgets(line, sizeof(line), stdin)) {
> - unsigned char sha1[20];
> + struct object_id oid;
>
> - if (get_sha1_hex(line, sha1)) {
> + if (get_oid_hex(line, &oid)) {
> fputs(line, stdout);
> fflush(stdout);
> }
>