On Sat, Feb 18, 2017 at 01:18:11AM +0000, Ramsay Jones wrote:
>
>
> 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 ...)
Well, I think it's strictly an improvement in that we have avoided
writing hardcoded constants[0]. I did intend it as a "hash plus one"
chunk, which is actually quite common throughout the code.
I'm wondering if parse_oid_hex could be useful here as well.
[0] If we change the hash size, all of the GIT_SHA1_HEXSZ constants can
be replaced with a variable that varies based on hash size, and the code
still works.
--
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
Attachment:
signature.asc
Description: PGP signature