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@crustytoothpaste.net>
> > ---
> >  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