On Thu, Dec 15 2022, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@xxxxxxxxxx>
> + end = (unsigned char *)hdr + size;
Commentary: The "unsigned char *" cast has moved up here from the below,
that's needed (we're casting from the struct), and we should get rid of
it from below, good.
> + start = end - the_hash_algo->rawsz;
Okey, so here we mark the start, which is the end minus the rawsz,
but...
> + oidread(&oid, start);
> + if (oideq(&oid, null_oid()))
> + return 0;
> +
> the_hash_algo->init_fn(&c);
> the_hash_algo->update_fn(&c, hdr, size - the_hash_algo->rawsz);
> the_hash_algo->final_fn(hash, &c);
> - if (!hasheq(hash, (unsigned char *)hdr + size - the_hash_algo->rawsz))
> + if (!hasheq(hash, end - the_hash_algo->rawsz))
...here we got rid of the cast, which is good, but let's not use "end -
the_hash_algo->rawsz" here, let's use "start", which you already
computed as "end - the_hash_algo->rawsz". This is just repeating it.
I wondered if I just missed it being modified in the interim before
carefully re-reading this, but we pass your tests with:
- if (!hasheq(hash, end - the_hash_algo->rawsz))
+ assert((end - the_hash_algo->rawsz) == start);
+ if (!hasheq(hash, start))
So, we can indeed juse the simpler "start" here, and it makes it easier
to read, as we're assured that it didn't move in the interim.
> + git_config_get_maybe_bool("index.skiphash", (int *)&f->skip_hash);
Aside from the question of whether we use the repo_*() variant here,
which I noted in my reply to the CL. The cast is suspicious.
So, in the 1/4 we added this as *unsigned*:
+ * If set to 1, skip_hash indicates that we should
+ * not actually compute the hash for this hashfile and
+ * instead only use it as a buffered write.
+ */
+ unsigned int skip_hash;
But you need the cast here since the config API can and will set the
"dest" to -1. See the "*dest == -1" test in git_configset_get_value().
So, here we're relying on a "unsigned int" cast'd to "int" correctly
doing the right thing on a "-1" assignment.
I'm not sure if that's portable or leads to undefined behavior, but in
any case, won't such a -1 value be read back as ~0 from that "unsigned
int" variable on most modern platforms?
Just bypassing that entirely and making it "int" seems better here, or
having an intermediate variable.
I also wondered if this was all my fault, in your original version you
were doing:
int skip_hash;
[...]
if (!git_config_get_maybe_bool("index.skiphash", &skip_hash))
f->skip_hash = skip_hash;
And I suggested that this was redundant, and that you could just write
to "f->skip_hash" directly.
But I didn't notice it was "unsigned", and in any case your original
version had the same issue of assigning a -1 to the unsigned variable...
