Re: [PATCH v4 03/19] builtin/diff-tree: convert to struct object_id
On Mon, Feb 20, 2017 at 08:08:36PM -0500, Jeff King wrote: > On Tue, Feb 21, 2017 at 12:25:19AM +, brian m. carlson wrote: > > > On Mon, Feb 20, 2017 at 03:09:02AM -0500, Jeff King wrote: > > > It's a little disturbing that we do not seem to have even a basic test > > > of: > > > > > > git rev-list --parents HEAD | git diff-tree --stdin > > > > > > which would exercise this code. > > > > I'm unsure, so I'll add a test. The only way to know if it works is to > > test it. > > Not to spoil the ending, but I did test and it does not work. :) Well, then I suppose I'll also end up sending out a new patch series. :) -- 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 v4 03/19] builtin/diff-tree: convert to struct object_id
On Tue, Feb 21, 2017 at 12:25:19AM +, brian m. carlson wrote: > On Mon, Feb 20, 2017 at 03:09:02AM -0500, Jeff King wrote: > > It's a little disturbing that we do not seem to have even a basic test > > of: > > > > git rev-list --parents HEAD | git diff-tree --stdin > > > > which would exercise this code. > > I'm unsure, so I'll add a test. The only way to know if it works is to > test it. Not to spoil the ending, but I did test and it does not work. :) -Peff
Re: [PATCH v4 03/19] builtin/diff-tree: convert to struct object_id
On Mon, Feb 20, 2017 at 03:09:02AM -0500, Jeff King wrote: > It's a little disturbing that we do not seem to have even a basic test > of: > > git rev-list --parents HEAD | git diff-tree --stdin > > which would exercise this code. I'm unsure, so I'll add a test. The only way to know if it works is to test it. -- 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 v4 03/19] builtin/diff-tree: convert to struct object_id
On Mon, Feb 20, 2017 at 12:10:15AM +, brian m. carlson wrote:
> /* 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)) {
> + struct object_id oid;
> + if (isspace(*p++) && !parse_oid_hex(p, &oid, &p)) {
> /* Graft the fake parents locally to the commit */
> - int pos = 41;
> 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 (isspace(*p++) && !parse_oid_hex(p, &oid, &p)) {
> + struct commit *parent = lookup_commit(oid.hash);
> if (parent) {
> pptr = &commit_list_insert(parent, pptr)->next;
> }
> - pos += 41;
> }
> }
Are you sure this is right? The first "if" will advance the "p" pointer,
and we'll miss it in the inner loop.
IOW, the original looked something like:
1. see if we have any parents after the initial commit sha1
2. if so, then free the original parent list, so we can parse the new
ones
3. starting at pos 41 (the same one we parsed in the conditional!),
loop and parse each parent sha1
The conditional in step 1 can't advance our pointer, or we miss the
first parent in step 3.
It's silly to parse the same sha1 twice, though. You could solve it by
adding the first "oid" from the conditional to the new parent list. In
my "something like this" patch, I solved it by dropping the conditional,
and just having the inner loop. It lazily drops the old parent list on
the first iteration.
It's a little disturbing that we do not seem to have even a basic test
of:
git rev-list --parents HEAD | git diff-tree --stdin
which would exercise this code.
-Peff
[PATCH v4 03/19] builtin/diff-tree: convert to struct object_id
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 | 43 +--
1 file changed, 21 insertions(+), 22 deletions(-)
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 8ce00480cd..1656e092bd 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(&log_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)) {
+ struct object_id oid;
+ if (isspace(*p++) && !parse_oid_hex(p, &oid, &p)) {
/* Graft the fake parents locally to the commit */
- int pos = 41;
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 (isspace(*p++) && !parse_oid_hex(p, &oid, &p)) {
+ struct commit *parent = lookup_commit(oid.hash);
if (parent) {
pptr = &commit_list_insert(parent, pptr)->next;
}
- pos += 41;
}
}
return log_tree_commit(&log_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, &oid, &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(&tree1->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, &oid, &p))
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(&oid), 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(&tree1->oid);
break;
case 2:
tree1 = opt->pending.objects[0].item;
@@ -164,9 +163,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)) {
- unsi
