Re: [PATCH v5 03/19] builtin/diff-tree: convert to struct object_id

[brian m. carlson]

On Wed, Feb 22, 2017 at 02:16:42PM -0500, Jeff King wrote:
> On Wed, Feb 22, 2017 at 10:50:21AM -0800, Junio C Hamano wrote:
> 
> > "brian m. carlson"  writes:
> > 
> > > Convert most leaf functions to struct object_id.  Change several
> > > hardcoded numbers to uses of parse_oid_hex.  In doing so, verify that we
> > > when we want two trees, we have exactly two trees.
> > >
> > > Finally, in stdin_diff_commit, avoid accessing the byte after the NUL.
> > > This will be a NUL as well, since the first NUL was a newline we
> > > overwrote.  However, with parse_oid_hex, we no longer need to increment
> > > the pointer directly, and can simply increment it as part of our check
> > > for the space character.
> > 
> > After reading the pre- and post-image twice, I think I convinced
> > myself that this is a faithful conersion and they do the same thing.
> 
> I think this is correct, too (but then, I think it largely comes from
> the patch I wrote the other night. So I did look at it carefully, but
> it's not exactly an independent review).

I did take that part, as well as other parts, from your patch.

I have a test, which I'll send in a minute, that verifies that they do
the same thing.  At first I introduced a segfault, and the test caught
it, so I feel confident that the patch does indeed function as the old
code did.
-- 
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 v5 03/19] builtin/diff-tree: convert to struct object_id

[Junio C Hamano]

Jeff King  writes:

> On Wed, Feb 22, 2017 at 10:50:21AM -0800, Junio C Hamano wrote:
>
>> What the function does appears somewhat iffy in the modern world.
>> We are relying on the fact that while Git is operating in this mode
>> of reading a tuple of commits per line and doing log-tree, that Git
>> itself will not do the history traversal (instead, another instance
>> of Git that feeds us via our standard input is walking the history)
>> for this piece of code to work correctly.  
>> 
>> Of course, the "diff-tree --stdin" command was meant to sit on the
>> downstream of "rev-list --parents", so the assumption the code makes
>> (i.e. the parents field of the in-core commit objects do not have to
>> be usable for history traversal) may be reasonable, but still...
>
> I'm not sure it's that weird. "diff-tree" is about diffing, not
> traversal. The only reason it touches commit->parents at all (and
> doesn't just kick off a diff between the arguments it gets) is that it's
> been stuck with pretty-printing the commits, which might ask to show the
> parents.

Yeah, I understand all that as 45392a648d ("git-diff-tree --stdin:
show all parents.", 2006-02-05) was mostly mine.  It's just I sense
that the recent trend is to take whatever existing code and see if
they are reusable in other contexts, and this is one of the things
that people might want to libify, but cannot be as-is.

Re: [PATCH v5 03/19] builtin/diff-tree: convert to struct object_id

[Jeff King]

On Wed, Feb 22, 2017 at 10:50:21AM -0800, Junio C Hamano wrote:

> "brian m. carlson"  writes:
> 
> > Convert most leaf functions to struct object_id.  Change several
> > hardcoded numbers to uses of parse_oid_hex.  In doing so, verify that we
> > when we want two trees, we have exactly two trees.
> >
> > Finally, in stdin_diff_commit, avoid accessing the byte after the NUL.
> > This will be a NUL as well, since the first NUL was a newline we
> > overwrote.  However, with parse_oid_hex, we no longer need to increment
> > the pointer directly, and can simply increment it as part of our check
> > for the space character.
> 
> After reading the pre- and post-image twice, I think I convinced
> myself that this is a faithful conersion and they do the same thing.

I think this is correct, too (but then, I think it largely comes from
the patch I wrote the other night. So I did look at it carefully, but
it's not exactly an independent review).

> What the function does appears somewhat iffy in the modern world.
> We are relying on the fact that while Git is operating in this mode
> of reading a tuple of commits per line and doing log-tree, that Git
> itself will not do the history traversal (instead, another instance
> of Git that feeds us via our standard input is walking the history)
> for this piece of code to work correctly.  
> 
> Of course, the "diff-tree --stdin" command was meant to sit on the
> downstream of "rev-list --parents", so the assumption the code makes
> (i.e. the parents field of the in-core commit objects do not have to
> be usable for history traversal) may be reasonable, but still...

I'm not sure it's that weird. "diff-tree" is about diffing, not
traversal. The only reason it touches commit->parents at all (and
doesn't just kick off a diff between the arguments it gets) is that it's
been stuck with pretty-printing the commits, which might ask to show the
parents.

-Peff

Re: [PATCH v5 03/19] builtin/diff-tree: convert to struct object_id

[Junio C Hamano]

"brian m. carlson"  writes:

> Convert most leaf functions to struct object_id.  Change several
> hardcoded numbers to uses of parse_oid_hex.  In doing so, verify that we
> when we want two trees, we have exactly two trees.
>
> Finally, in stdin_diff_commit, avoid accessing the byte after the NUL.
> This will be a NUL as well, since the first NUL was a newline we
> overwrote.  However, with parse_oid_hex, we no longer need to increment
> the pointer directly, and can simply increment it as part of our check
> for the space character.

After reading the pre- and post-image twice, I think I convinced
myself that this is a faithful conersion and they do the same thing.

What the function does appears somewhat iffy in the modern world.
We are relying on the fact that while Git is operating in this mode
of reading a tuple of commits per line and doing log-tree, that Git
itself will not do the history traversal (instead, another instance
of Git that feeds us via our standard input is walking the history)
for this piece of code to work correctly.  

Of course, the "diff-tree --stdin" command was meant to sit on the
downstream of "rev-list --parents", so the assumption the code makes
(i.e. the parents field of the in-core commit objects do not have to
be usable for history traversal) may be reasonable, but still...

[PATCH v5 03/19] builtin/diff-tree: convert to struct object_id

[brian m. carlson]

Convert most leaf functions to struct object_id.  Change several
hardcoded numbers to uses of parse_oid_hex.  In doing so, verify that we
when we want two trees, we have exactly two trees.

Finally, in stdin_diff_commit, avoid accessing the byte after the NUL.
This will be a NUL as well, since the first NUL was a newline we
overwrote.  However, with parse_oid_hex, we no longer need to increment
the pointer directly, and can simply increment it as part of our check
for the space character.

Signed-off-by: brian m. carlson 
Signed-off-by: Jeff King 
---
 builtin/diff-tree.c | 61 ++---
 1 file changed, 30 insertions(+), 31 deletions(-)

diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 8ce00480cd..326f88b657 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -7,46 +7,44 @@
 
 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(_tree_opt, commit);
 }
 
 /* Diff one or more commits. */
-static int stdin_diff_commit(struct commit *commit, char *line, int len)
+static int stdin_diff_commit(struct commit *commit, const char *p)
 {
-   unsigned char sha1[20];
-   if (isspace(line[40]) && !get_sha1_hex(line+41, sha1)) {
-   /* Graft the fake parents locally to the commit */
-   int pos = 41;
-   struct commit_list **pptr;
+   struct object_id oid;
+   struct commit_list **pptr = NULL;
 
-   /* 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);
-   if (parent) {
-   pptr = _list_insert(parent, pptr)->next;
-   }
-   pos += 41;
+   /* Graft the fake parents locally to the commit */
+   while (isspace(*p++) && !parse_oid_hex(p, , )) {
+   struct commit *parent = lookup_commit(oid.hash);
+   if (!pptr) {
+   /* Free the real parent list */
+   free_commit_list(commit->parents);
+   commit->parents = NULL;
+   pptr = &(commit->parents);
+   }
+   if (parent) {
+   pptr = _list_insert(parent, pptr)->next;
}
}
return log_tree_commit(_tree_opt, commit);
 }
 
 /* Diff two trees. */
-static int stdin_diff_trees(struct tree *tree1, char *line, int len)
+static int stdin_diff_trees(struct tree *tree1, const char *p)
 {
-   unsigned char sha1[20];
+   struct object_id oid;
struct tree *tree2;
-   if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))
+   if (!isspace(*p++) || parse_oid_hex(p, , ) || *p)
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(>object.oid),
@@ -60,23 +58,24 @@ 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;
+   const char *p;
 
if (!len || line[len-1] != '\n')
return -1;
line[len-1] = 0;
-   if (get_sha1_hex(line, sha1))
+   if (parse_oid_hex(line, , ))
return -1;
-   obj = parse_object(sha1);
+   obj = parse_object(oid.hash);
if (!obj)
return -1;
if (obj->type == OBJ_COMMIT)
-   return stdin_diff_commit((struct commit *)obj, line, len);
+   return stdin_diff_commit((struct commit *)obj, p);
if (obj->type == OBJ_TREE)
-   return stdin_diff_trees((struct tree *)obj, line, len);
+   return stdin_diff_trees((struct tree *)obj, p);
error("Object %s is a %s, not a commit or tree",
- sha1_to_hex(sha1), typename(obj->type));
+ oid_to_hex(), typename(obj->type));
return -1;
 }
 
@@ -141,7 +140,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(>oid);
break;