Re: [PATCH v5 03/19] builtin/diff-tree: convert to struct object_id
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
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
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
"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...
