* [PATCH 0/9] introducing oideq()
@ 2018-08-25 8:00 Jeff King
2018-08-25 8:03 ` [PATCH 1/9] coccinelle: use <...> for function exclusion Jeff King
` (9 more replies)
0 siblings, 10 replies; 30+ messages in thread
From: Jeff King @ 2018-08-25 8:00 UTC (permalink / raw)
To: git; +Cc: brian m. carlson, Derrick Stolee, Jacob Keller
This is a follow-up to the discussion in:
https://public-inbox.org/git/20180822030344.GA14684@sigill.intra.peff.net/
The general idea is that the majority of callers don't care about actual
plus/minus ordering from oidcmp() and hashcmp(); they just want to know
if two oids are equal. The stricter equality check can be optimized
better by the compiler.
Due to the simplicity of the current code and our inlining, the compiler
can usually figure this out for now. So I wouldn't expect this patch to
actually improve performance right away. But as that discussion shows,
we are likely to take a performance hit as we move to more runtime
determination of the_hash_algo parameters. Having these callers use the
more strict form will potentially help us recover that.
So in that sense we _could_ simply punt on this series until then (and
it's certainly post-v2.19 material). But I think it's worth doing now,
simply from a readability/annotation standpoint. IMHO the resulting code
is more clear (though I've long since taught myself to read !foocmp() as
equality).
If we do punt, patch 1 could still be picked up now, as it's a related
cleanup that stands on its own.
The diffstat is scary, but the vast majority of that is purely
mechanical via coccinelle. There's some discussion in the individual
patches.
We also don't _have_ to convert all sites now. I strongly suspect that
only a few calls are actual measurable bottlenecks (the one in
lookup_object() is the one I was primarily interested in). But it's
just as easy to do it all at once with coccinelle, rather than try to
measure each one (and once we add the coccinelle patches, we have to
convert everything, or "make coccicheck" will nag people to do so).
I didn't bother to hold back changes for any topics in flight. There
are a handful of conflicts with "pu", but they're mostly trivial. The
only big one is due to some code movement in ds/reachable. But though
the conflict is big, the resolution is still easy (you can even just
take the other side and "make coccicheck" to do it automatically).
[1/9]: coccinelle: use <...> for function exclusion
[2/9]: introduce hasheq() and oideq()
[3/9]: convert "oidcmp() == 0" to oideq()
[4/9]: convert "hashcmp() == 0" to hasheq()
[5/9]: convert "oidcmp() != 0" to "!oideq()"
[6/9]: convert "hashcmp() != 0" to "!hasheq()"
[7/9]: convert hashmap comparison functions to oideq()
[8/9]: read-cache: use oideq() in ce_compare functions
[9/9]: show_dirstat: simplify same-content check
bisect.c | 6 ++--
blame.c | 8 ++---
builtin/am.c | 2 +-
builtin/checkout.c | 2 +-
builtin/describe.c | 10 +++---
builtin/diff.c | 2 +-
builtin/difftool.c | 4 +--
builtin/fast-export.c | 2 +-
builtin/fetch.c | 6 ++--
builtin/fmt-merge-msg.c | 6 ++--
builtin/index-pack.c | 8 ++---
builtin/log.c | 6 ++--
builtin/merge-tree.c | 2 +-
builtin/merge.c | 6 ++--
builtin/pack-objects.c | 4 +--
builtin/pull.c | 4 +--
builtin/receive-pack.c | 4 +--
builtin/remote.c | 2 +-
builtin/replace.c | 6 ++--
builtin/rm.c | 2 +-
builtin/show-branch.c | 6 ++--
builtin/tag.c | 2 +-
builtin/unpack-objects.c | 4 +--
builtin/update-index.c | 4 +--
bulk-checkin.c | 2 +-
bundle.c | 2 +-
cache-tree.c | 2 +-
cache.h | 22 +++++++++----
combine-diff.c | 4 +--
commit-graph.c | 10 +++---
commit.c | 2 +-
connect.c | 2 +-
contrib/coccinelle/commit.cocci | 4 +--
contrib/coccinelle/object_id.cocci | 50 ++++++++++++++++++++++++------
diff-lib.c | 4 +--
diff.c | 23 ++++++--------
diffcore-break.c | 2 +-
diffcore-rename.c | 2 +-
dir.c | 6 ++--
fast-import.c | 10 +++---
fetch-pack.c | 2 +-
http-push.c | 2 +-
http-walker.c | 4 +--
http.c | 2 +-
log-tree.c | 6 ++--
match-trees.c | 2 +-
merge-recursive.c | 4 +--
notes-merge.c | 24 +++++++-------
notes.c | 8 ++---
object.c | 2 +-
oidmap.c | 12 +++----
pack-check.c | 6 ++--
pack-objects.c | 2 +-
pack-write.c | 4 +--
packfile.c | 12 +++----
patch-ids.c | 8 ++---
read-cache.c | 12 +++----
ref-filter.c | 2 +-
refs.c | 8 ++---
refs/files-backend.c | 6 ++--
refs/packed-backend.c | 2 +-
refs/ref-cache.c | 2 +-
remote.c | 8 ++---
revision.c | 2 +-
sequencer.c | 40 ++++++++++++------------
sha1-array.c | 2 +-
sha1-file.c | 12 +++----
sha1-name.c | 2 +-
submodule-config.c | 4 +--
submodule.c | 2 +-
t/helper/test-dump-cache-tree.c | 2 +-
transport.c | 2 +-
tree-diff.c | 2 +-
unpack-trees.c | 6 ++--
wt-status.c | 10 +++---
xdiff-interface.c | 2 +-
76 files changed, 259 insertions(+), 224 deletions(-)
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH 1/9] coccinelle: use <...> for function exclusion 2018-08-25 8:00 [PATCH 0/9] introducing oideq() Jeff King @ 2018-08-25 8:03 ` Jeff King 2018-08-25 8:05 ` [PATCH 2/9] introduce hasheq() and oideq() Jeff King ` (8 subsequent siblings) 9 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2018-08-25 8:03 UTC (permalink / raw) To: git; +Cc: brian m. carlson, Derrick Stolee, Jacob Keller Sometimes we want to suppress a coccinelle transformation inside a particular function. For example, in finding conversions of hashcmp() to oidcmp(), we should not convert the call in oidcmp() itself, since that would cause infinite recursion. We write that like this: @@ identifier f != oidcmp; expression E1, E2; @@ f(...) {... - hashcmp(E1->hash, E2->hash) + oidcmp(E1, E2) ...} to match the interior of any function _except_ oidcmp(). Unfortunately, this doesn't catch all cases (e.g., the one in sequencer.c that this patch fixes). The problem, as explained by one of the Coccinelle developers in [1], is: For transformation, A ... B requires that B occur on every execution path starting with A, unless that execution path ends up in error handling code. (eg, if (...) { ... return; }). Here your A is the start of the function. So you need a call to hashcmp on every path through the function, which fails when you add ifs. [...] Another issue with A ... B is that by default A and B should not appear in the matched region. So your original rule matches only the case where every execution path contains exactly one call to hashcmp, not more than one. One way to solve this is to put the pattern inside an angle-bracket pattern like "<... P ...>", which allows zero or more matches of P. That works (and is what this patch does), but it has one drawback: it matches more than we care about, and Coccinelle uses extra CPU. Here are timings for "make coccicheck" before and after this patch: [before] real 1m27.122s user 7m34.451s sys 0m37.330s [after] real 2m18.040s user 10m58.310s sys 0m41.549s That's not ideal, but it's more important for this to be correct than to be fast. And coccicheck is already fairly slow (and people don't run it for every single patch). So it's an acceptable tradeoff. There _is_ a better way to do it, which is to record the position at which we find hashcmp(), and then check it against the forbidden function list. Like: @@ position p : script:python() { p[0].current_element != "oidcmp" }; expression E1,E2; @@ - hashcmp@p(E1->hash, E2->hash) + oidcmp(E1, E2) This is only a little slower than the current code, and does the right thing in all cases. Unfortunately, not all builds of Coccinelle include python support (including the ones in Debian). Requiring it may mean that fewer people can easily run the tool, which is worse than it simply being a little slower. We may want to revisit this decision in the future if: - builds with python become more common - we find more uses for python support that tip the cost-benefit analysis But for now this patch sticks with the angle-bracket solution, and converts all existing cocci patches. This fixes only one missed case in the current code, though it makes a much better difference for some new rules I'm adding (converting "!hashcmp()" to "hasheq()" misses over half the possible conversions using the old form). [1] https://public-inbox.org/git/alpine.DEB.2.21.1808240652370.2344@hadrien/ Helped-by: Julia Lawall <julia.lawall@lip6.fr> Signed-off-by: Jeff King <peff@peff.net> --- contrib/coccinelle/commit.cocci | 4 ++-- contrib/coccinelle/object_id.cocci | 20 ++++++++++---------- sequencer.c | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/contrib/coccinelle/commit.cocci b/contrib/coccinelle/commit.cocci index aec3345adb..c49aa558f0 100644 --- a/contrib/coccinelle/commit.cocci +++ b/contrib/coccinelle/commit.cocci @@ -15,10 +15,10 @@ expression c; identifier f !~ "^(get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit)$"; expression c; @@ - f(...) {... + f(...) {<... - c->maybe_tree + get_commit_tree(c) - ...} + ...>} @@ expression c; diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci index 09afdbf994..5869979be7 100644 --- a/contrib/coccinelle/object_id.cocci +++ b/contrib/coccinelle/object_id.cocci @@ -20,10 +20,10 @@ expression E1; identifier f != oid_to_hex; expression E1; @@ - f(...) {... + f(...) {<... - sha1_to_hex(E1->hash) + oid_to_hex(E1) - ...} + ...>} @@ expression E1, E2; @@ -35,10 +35,10 @@ expression E1, E2; identifier f != oid_to_hex_r; expression E1, E2; @@ - f(...) {... + f(...) {<... - sha1_to_hex_r(E1, E2->hash) + oid_to_hex_r(E1, E2) - ...} + ...>} @@ expression E1; @@ -50,10 +50,10 @@ expression E1; identifier f != oidclr; expression E1; @@ - f(...) {... + f(...) {<... - hashclr(E1->hash) + oidclr(E1) - ...} + ...>} @@ expression E1, E2; @@ -65,10 +65,10 @@ expression E1, E2; identifier f != oidcmp; expression E1, E2; @@ - f(...) {... + f(...) {<... - hashcmp(E1->hash, E2->hash) + oidcmp(E1, E2) - ...} + ...>} @@ expression E1, E2; @@ -92,10 +92,10 @@ expression E1, E2; identifier f != oidcpy; expression E1, E2; @@ - f(...) {... + f(...) {<... - hashcpy(E1->hash, E2->hash) + oidcpy(E1, E2) - ...} + ...>} @@ expression E1, E2; diff --git a/sequencer.c b/sequencer.c index 65d371c746..6a11f49651 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4545,7 +4545,7 @@ int skip_unnecessary_picks(void) if (item->commit->parents->next) break; /* merge commit */ parent_oid = &item->commit->parents->item->object.oid; - if (hashcmp(parent_oid->hash, oid->hash)) + if (oidcmp(parent_oid, oid)) break; oid = &item->commit->object.oid; } -- 2.19.0.rc0.412.g7005db4e88 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 2/9] introduce hasheq() and oideq() 2018-08-25 8:00 [PATCH 0/9] introducing oideq() Jeff King 2018-08-25 8:03 ` [PATCH 1/9] coccinelle: use <...> for function exclusion Jeff King @ 2018-08-25 8:05 ` Jeff King 2018-08-25 10:58 ` Andrei Rybak 2018-08-25 8:07 ` [PATCH 3/9] convert "oidcmp() == 0" to oideq() Jeff King ` (7 subsequent siblings) 9 siblings, 1 reply; 30+ messages in thread From: Jeff King @ 2018-08-25 8:05 UTC (permalink / raw) To: git; +Cc: brian m. carlson, Derrick Stolee, Jacob Keller The main comparison functions we provide for comparing object ids are hashcmp() and oidcmp(). These are more flexible than a strict equality check, since they also express ordering. That makes them them useful for sorting and binary searching. However, it also makes them potentially slower than a strict equality check. Consider this C code, which is traditionally what our hashcmp has looked like: #include <string.h> int hashcmp(const unsigned char *a, const unsigned char *b) { return memcmp(a, b, 20); } Compiling with "gcc -O2 -S -fverbose-asm", the generated assembly shows that we actually call memcmp(). But if we change this to a strict equality check: return !memcmp(a, b, 20); we get a faster inline version: movq (%rdi), %rax # MEM[(void *)a_4(D)], MEM[(void *)a_4(D)] movq 8(%rdi), %rdx # MEM[(void *)a_4(D)], tmp101 xorq (%rsi), %rax # MEM[(void *)b_5(D)], tmp94 xorq 8(%rsi), %rdx # MEM[(void *)b_5(D)], tmp93 orq %rax, %rdx # tmp94, tmp93 jne .L2 #, movl 16(%rsi), %eax # MEM[(void *)b_5(D)], tmp104 cmpl %eax, 16(%rdi) # tmp104, MEM[(void *)a_4(D)] je .L5 #, Obviously our hashcmp() doesn't include the "!". But because it's an inline function, optimizing compilers are able to see "!hashcmp(a,b)" in calling code and take advantage of this case. So there has been no value thus far in introducing a more restricted interface for doing strict equality checks. But as Git learns about more values for the_hash_algo, our hashcmp() will grow more complicated and may even delegate at runtime to functions optimized specifically for that hash size. That breaks the inline connection we have, and the compiler will have to assume that the caller really cares about the sign and magnitude of the memcmp() result, even though the vast majority don't. We can solve that by introducing a hasheq() function (and matching oideq() wrapper), which callers can use to make clear that they only care about equality. For now, the implementation will literally be "!hashcmp()", but it frees us up later to introduce code optimized specifically for the equality check. Signed-off-by: Jeff King <peff@peff.net> --- cache.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/cache.h b/cache.h index 4d014541ab..f6d227fac7 100644 --- a/cache.h +++ b/cache.h @@ -1041,6 +1041,16 @@ static inline int oidcmp(const struct object_id *oid1, const struct object_id *o return hashcmp(oid1->hash, oid2->hash); } +static inline int hasheq(const unsigned char *sha1, const unsigned char *sha2) +{ + return !hashcmp(sha1, sha2); +} + +static inline int oideq(const struct object_id *oid1, const struct object_id *oid2) +{ + return hasheq(oid1->hash, oid2->hash); +} + static inline int is_null_sha1(const unsigned char *sha1) { return !hashcmp(sha1, null_sha1); -- 2.19.0.rc0.412.g7005db4e88 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 2/9] introduce hasheq() and oideq() 2018-08-25 8:05 ` [PATCH 2/9] introduce hasheq() and oideq() Jeff King @ 2018-08-25 10:58 ` Andrei Rybak 0 siblings, 0 replies; 30+ messages in thread From: Andrei Rybak @ 2018-08-25 10:58 UTC (permalink / raw) To: Jeff King, git; +Cc: brian m. carlson, Derrick Stolee, Jacob Keller On 25/08/18 10:05, Jeff King wrote: > The main comparison functions we provide for comparing > object ids are hashcmp() and oidcmp(). These are more > flexible than a strict equality check, since they also > express ordering. That makes them them useful for sorting s/them them/them/ > We can solve that by introducing a hasheq() function (and > matching oideq() wrapper), which callers can use to make s/make/& it/ > clear that they only care about equality. For now, the > implementation will literally be "!hashcmp()", but it frees > us up later to introduce code optimized specifically for the > equality check. > > Signed-off-by: Jeff King <peff@peff.net> ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 3/9] convert "oidcmp() == 0" to oideq() 2018-08-25 8:00 [PATCH 0/9] introducing oideq() Jeff King 2018-08-25 8:03 ` [PATCH 1/9] coccinelle: use <...> for function exclusion Jeff King 2018-08-25 8:05 ` [PATCH 2/9] introduce hasheq() and oideq() Jeff King @ 2018-08-25 8:07 ` Jeff King 2018-08-25 8:36 ` Jeff King 2018-08-25 8:08 ` [PATCH 4/9] convert "hashcmp() == 0" to hasheq() Jeff King ` (6 subsequent siblings) 9 siblings, 1 reply; 30+ messages in thread From: Jeff King @ 2018-08-25 8:07 UTC (permalink / raw) To: git; +Cc: brian m. carlson, Derrick Stolee, Jacob Keller Using the more restrictive oideq() should, in the long run, give the compiler more opportunities to optimize these callsites. For now, this conversion should be a complete noop with respect to the generated code. The result is also perhaps a little more readable, as it avoids the "zero is equal" idiom. Since it's so prevalent in C, I think seasoned programmers tend not to even notice it anymore, but it can sometimes make for awkward double negations (e.g., we can drop a few !!oidcmp() instances here). This patch was generated almost entirely by the included coccinelle patch. This mechanical conversion should be completely safe, because we check explicitly for cases where oidcmp() is compared to 0, which is what oideq() is doing under the hood. Note that we don't have to catch "!oidcmp()" separately; coccinelle's standard isomorphisms make sure the two are treated equivalently. I say "almost" because I did hand-edit the coccinelle output to fix up a few style violations (it mostly keeps the original formatting, but sometimes unwraps long lines). Signed-off-by: Jeff King <peff@peff.net> --- bisect.c | 4 ++-- blame.c | 4 ++-- builtin/am.c | 2 +- builtin/checkout.c | 2 +- builtin/describe.c | 4 ++-- builtin/diff.c | 2 +- builtin/difftool.c | 4 ++-- builtin/fast-export.c | 2 +- builtin/fetch.c | 4 ++-- builtin/fmt-merge-msg.c | 2 +- builtin/index-pack.c | 4 ++-- builtin/log.c | 6 +++--- builtin/merge-tree.c | 2 +- builtin/merge.c | 4 ++-- builtin/pack-objects.c | 4 ++-- builtin/pull.c | 2 +- builtin/receive-pack.c | 4 ++-- builtin/remote.c | 2 +- builtin/replace.c | 6 +++--- builtin/unpack-objects.c | 2 +- builtin/update-index.c | 4 ++-- bulk-checkin.c | 2 +- cache-tree.c | 2 +- cache.h | 4 ++-- combine-diff.c | 4 ++-- commit-graph.c | 2 +- connect.c | 2 +- contrib/coccinelle/object_id.cocci | 6 ++++++ diff-lib.c | 2 +- diff.c | 6 +++--- diffcore-break.c | 2 +- fast-import.c | 6 +++--- http-push.c | 2 +- log-tree.c | 6 +++--- merge-recursive.c | 4 ++-- notes-merge.c | 24 +++++++++++----------- notes.c | 4 ++-- pack-write.c | 2 +- read-cache.c | 2 +- ref-filter.c | 2 +- refs/files-backend.c | 4 ++-- remote.c | 6 +++--- revision.c | 2 +- sequencer.c | 32 +++++++++++++++--------------- sha1-array.c | 2 +- sha1-file.c | 4 ++-- sha1-name.c | 2 +- submodule.c | 2 +- transport.c | 2 +- unpack-trees.c | 6 +++--- wt-status.c | 10 +++++----- xdiff-interface.c | 2 +- 52 files changed, 117 insertions(+), 111 deletions(-) diff --git a/bisect.c b/bisect.c index e1275ba79e..41c56a665e 100644 --- a/bisect.c +++ b/bisect.c @@ -807,7 +807,7 @@ static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout) for (; result; result = result->next) { const struct object_id *mb = &result->item->object.oid; - if (!oidcmp(mb, current_bad_oid)) { + if (oideq(mb, current_bad_oid)) { handle_bad_merge_base(); } else if (0 <= oid_array_lookup(&good_revs, mb)) { continue; @@ -988,7 +988,7 @@ int bisect_next_all(const char *prefix, int no_checkout) bisect_rev = &revs.commits->item->object.oid; - if (!oidcmp(bisect_rev, current_bad_oid)) { + if (oideq(bisect_rev, current_bad_oid)) { exit_if_skipped_commits(tried, current_bad_oid); printf("%s is the first %s commit\n", oid_to_hex(bisect_rev), term_bad); diff --git a/blame.c b/blame.c index 08c0c6cf73..10d72e36dd 100644 --- a/blame.c +++ b/blame.c @@ -1459,14 +1459,14 @@ static void pass_blame(struct blame_scoreboard *sb, struct blame_origin *origin, porigin = find(p, origin); if (!porigin) continue; - if (!oidcmp(&porigin->blob_oid, &origin->blob_oid)) { + if (oideq(&porigin->blob_oid, &origin->blob_oid)) { pass_whole_blame(sb, origin, porigin); blame_origin_decref(porigin); goto finish; } for (j = same = 0; j < i; j++) if (sg_origin[j] && - !oidcmp(&sg_origin[j]->blob_oid, &porigin->blob_oid)) { + oideq(&sg_origin[j]->blob_oid, &porigin->blob_oid)) { same = 1; break; } diff --git a/builtin/am.c b/builtin/am.c index 9f7ecf6ecb..e54110d474 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2077,7 +2077,7 @@ static int safe_to_abort(const struct am_state *state) if (get_oid("HEAD", &head)) oidclr(&head); - if (!oidcmp(&head, &abort_safety)) + if (oideq(&head, &abort_safety)) return 1; warning(_("You seem to have moved HEAD since the last 'am' failure.\n" diff --git a/builtin/checkout.c b/builtin/checkout.c index 29ef50013d..082e3a9f19 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -96,7 +96,7 @@ static int update_some(const struct object_id *oid, struct strbuf *base, if (pos >= 0) { struct cache_entry *old = active_cache[pos]; if (ce->ce_mode == old->ce_mode && - !oidcmp(&ce->oid, &old->oid)) { + oideq(&ce->oid, &old->oid)) { old->ce_flags |= CE_UPDATE; discard_cache_entry(ce); return 0; diff --git a/builtin/describe.c b/builtin/describe.c index 41606c8a90..1e7c75ba82 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -190,7 +190,7 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi /* Is it annotated? */ if (!peel_ref(path, &peeled)) { - is_annotated = !!oidcmp(oid, &peeled); + is_annotated = !oideq(oid, &peeled); } else { oidcpy(&peeled, oid); is_annotated = 0; @@ -469,7 +469,7 @@ static void process_object(struct object *obj, const char *path, void *data) { struct process_commit_data *pcd = data; - if (!oidcmp(&pcd->looking_for, &obj->oid) && !pcd->dst->len) { + if (oideq(&pcd->looking_for, &obj->oid) && !pcd->dst->len) { reset_revision_walk(); describe_commit(&pcd->current_commit, pcd->dst); strbuf_addf(pcd->dst, ":%s", path); diff --git a/builtin/diff.c b/builtin/diff.c index 361a3c3ed3..b3a8ba488f 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -41,7 +41,7 @@ static void stuff_change(struct diff_options *opt, struct diff_filespec *one, *two; if (!is_null_oid(old_oid) && !is_null_oid(new_oid) && - !oidcmp(old_oid, new_oid) && (old_mode == new_mode)) + oideq(old_oid, new_oid) && (old_mode == new_mode)) return; if (opt->flags.reverse_diff) { diff --git a/builtin/difftool.c b/builtin/difftool.c index cdd585ca76..b41a9199ff 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -116,7 +116,7 @@ static int use_wt_file(const char *workdir, const char *name, if (is_null_oid(oid)) { oidcpy(oid, &wt_oid); use = 1; - } else if (!oidcmp(oid, &wt_oid)) + } else if (oideq(oid, &wt_oid)) use = 1; } } @@ -438,7 +438,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, strbuf_reset(&buf); strbuf_addf(&buf, "Subproject commit %s", oid_to_hex(&roid)); - if (!oidcmp(&loid, &roid)) + if (oideq(&loid, &roid)) strbuf_addstr(&buf, "-dirty"); add_left_or_right(&submodules, dst_path, buf.buf, 1); continue; diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 9bd8a14b57..74f3bf5c96 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -384,7 +384,7 @@ static void show_filemodify(struct diff_queue_struct *q, string_list_insert(changed, spec->path); putchar('\n'); - if (!oidcmp(&ospec->oid, &spec->oid) && + if (oideq(&ospec->oid, &spec->oid) && ospec->mode == spec->mode) break; } diff --git a/builtin/fetch.c b/builtin/fetch.c index 61bec5d213..32b1d5d383 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -507,7 +507,7 @@ static void adjust_refcol_width(const struct ref *ref) int max, rlen, llen, len; /* uptodate lines are only shown on high verbosity level */ - if (!verbosity && !oidcmp(&ref->peer_ref->old_oid, &ref->old_oid)) + if (!verbosity && oideq(&ref->peer_ref->old_oid, &ref->old_oid)) return; max = term_columns(); @@ -644,7 +644,7 @@ static int update_local_ref(struct ref *ref, if (type < 0) die(_("object %s not found"), oid_to_hex(&ref->new_oid)); - if (!oidcmp(&ref->old_oid, &ref->new_oid)) { + if (oideq(&ref->old_oid, &ref->new_oid)) { if (verbosity > 0) format_display(display, '=', _("[up to date]"), NULL, remote, pretty_ref, summary_width); diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index f35ff1612b..4c82c234cb 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -582,7 +582,7 @@ static void find_merge_parents(struct merge_parents *result, while (parents) { struct commit *cmit = pop_commit(&parents); for (i = 0; i < result->nr; i++) - if (!oidcmp(&result->item[i].commit, &cmit->object.oid)) + if (oideq(&result->item[i].commit, &cmit->object.oid)) result->item[i].used = 1; } diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 9582ead950..edcb0a3dca 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -719,9 +719,9 @@ static void find_ref_delta_children(const struct object_id *oid, *last_index = -1; return; } - while (first > 0 && !oidcmp(&ref_deltas[first - 1].oid, oid)) + while (first > 0 && oideq(&ref_deltas[first - 1].oid, oid)) --first; - while (last < end && !oidcmp(&ref_deltas[last + 1].oid, oid)) + while (last < end && oideq(&ref_deltas[last + 1].oid, oid)) ++last; *first_index = first; *last_index = last; diff --git a/builtin/log.c b/builtin/log.c index e094560d9a..98d668b56f 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -992,7 +992,7 @@ static char *find_branch_name(struct rev_info *rev) tip_oid = &rev->cmdline.rev[positive].item->oid; if (dwim_ref(ref, strlen(ref), &branch_oid, &full_ref) && skip_prefix(full_ref, "refs/heads/", &v) && - !oidcmp(tip_oid, &branch_oid)) + oideq(tip_oid, &branch_oid)) branch = xstrdup(v); free(full_ref); return branch; @@ -1703,7 +1703,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) /* Don't say anything if head and upstream are the same. */ if (rev.pending.nr == 2) { struct object_array_entry *o = rev.pending.objects; - if (oidcmp(&o[0].item->oid, &o[1].item->oid) == 0) + if (oideq(&o[0].item->oid, &o[1].item->oid)) return 0; } get_patch_ids(&rev, &ids); @@ -1949,7 +1949,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix) /* Don't say anything if head and upstream are the same. */ if (revs.pending.nr == 2) { struct object_array_entry *o = revs.pending.objects; - if (oidcmp(&o[0].item->oid, &o[1].item->oid) == 0) + if (oideq(&o[0].item->oid, &o[1].item->oid)) return 0; } diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index f8023bae1e..8cea8a74f2 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -155,7 +155,7 @@ static int same_entry(struct name_entry *a, struct name_entry *b) { return a->oid && b->oid && - !oidcmp(a->oid, b->oid) && + oideq(a->oid, b->oid) && a->mode == b->mode; } diff --git a/builtin/merge.c b/builtin/merge.c index 8f4a5065c2..57abff999b 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1189,7 +1189,7 @@ static int merging_a_throwaway_tag(struct commit *commit) tag_ref = xstrfmt("refs/tags/%s", ((struct tag *)merge_remote_util(commit)->obj)->tag); if (!read_ref(tag_ref, &oid) && - !oidcmp(&oid, &merge_remote_util(commit)->obj->oid)) + oideq(&oid, &merge_remote_util(commit)->obj->oid)) is_throwaway_tag = 0; else is_throwaway_tag = 1; @@ -1448,7 +1448,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) goto done; } else if (fast_forward != FF_NO && !remoteheads->next && !common->next && - !oidcmp(&common->item->object.oid, &head_commit->object.oid)) { + oideq(&common->item->object.oid, &head_commit->object.oid)) { /* Again the most common case of merging one remote. */ struct strbuf msg = STRBUF_INIT; struct commit *commit; diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index d1144a8f7e..64156f676b 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1202,7 +1202,7 @@ static struct pbase_tree_cache *pbase_tree_get(const struct object_id *oid) */ for (neigh = 0; neigh < 8; neigh++) { ent = pbase_tree_cache[my_ix]; - if (ent && !oidcmp(&ent->oid, oid)) { + if (ent && oideq(&ent->oid, oid)) { ent->ref++; return ent; } @@ -1384,7 +1384,7 @@ static void add_preferred_base(struct object_id *oid) return; for (it = pbase_tree; it; it = it->next) { - if (!oidcmp(&it->pcache.oid, &tree_oid)) { + if (oideq(&it->pcache.oid, &tree_oid)) { free(data); return; } diff --git a/builtin/pull.c b/builtin/pull.c index 53bc5facfd..27db0c69eb 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -799,7 +799,7 @@ static int run_rebase(const struct object_id *curr_head, struct argv_array args = ARGV_ARRAY_INIT; if (!get_octopus_merge_base(&oct_merge_base, curr_head, merge_head, fork_point)) - if (!is_null_oid(fork_point) && !oidcmp(&oct_merge_base, fork_point)) + if (!is_null_oid(fork_point) && oideq(&oct_merge_base, fork_point)) fork_point = NULL; argv_array_push(&args, "rebase"); diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index c17ce94e12..5bb163d4d2 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1222,8 +1222,8 @@ static void check_aliased_update(struct command *cmd, struct string_list *list) dst_cmd = (struct command *) item->util; - if (!oidcmp(&cmd->old_oid, &dst_cmd->old_oid) && - !oidcmp(&cmd->new_oid, &dst_cmd->new_oid)) + if (oideq(&cmd->old_oid, &dst_cmd->old_oid) && + oideq(&cmd->new_oid, &dst_cmd->new_oid)) return; dst_cmd->skip_update = 1; diff --git a/builtin/remote.c b/builtin/remote.c index 07bd51f8eb..54a763cf90 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -412,7 +412,7 @@ static int get_push_ref_states(const struct ref *remote_refs, if (is_null_oid(&ref->new_oid)) { info->status = PUSH_STATUS_DELETE; - } else if (!oidcmp(&ref->old_oid, &ref->new_oid)) + } else if (oideq(&ref->old_oid, &ref->new_oid)) info->status = PUSH_STATUS_UPTODATE; else if (is_null_oid(&ref->old_oid)) info->status = PUSH_STATUS_CREATE; diff --git a/builtin/replace.c b/builtin/replace.c index 4f05791f3e..8e67e09819 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -343,7 +343,7 @@ static int edit_and_replace(const char *object_ref, int force, int raw) } free(tmpfile); - if (!oidcmp(&old_oid, &new_oid)) + if (oideq(&old_oid, &new_oid)) return error(_("new object is the same as the old one: '%s'"), oid_to_hex(&old_oid)); return replace_object_oid(object_ref, &old_oid, "replacement", &new_oid, force); @@ -414,7 +414,7 @@ static int check_one_mergetag(struct commit *commit, if (get_oid(mergetag_data->argv[i], &oid) < 0) return error(_("not a valid object name: '%s'"), mergetag_data->argv[i]); - if (!oidcmp(&tag->tagged->oid, &oid)) + if (oideq(&tag->tagged->oid, &oid)) return 0; /* found */ } @@ -474,7 +474,7 @@ static int create_graft(int argc, const char **argv, int force, int gentle) strbuf_release(&buf); - if (!oidcmp(&old_oid, &new_oid)) { + if (oideq(&old_oid, &new_oid)) { if (gentle) { warning(_("graft for '%s' unnecessary"), oid_to_hex(&old_oid)); return 0; diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index 30d9413b4b..ad438f5b41 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -303,7 +303,7 @@ static void added_object(unsigned nr, enum object_type type, struct delta_info *info; while ((info = *p) != NULL) { - if (!oidcmp(&info->base_oid, &obj_list[nr].oid) || + if (oideq(&info->base_oid, &obj_list[nr].oid) || info->base_offset == obj_list[nr].offset) { *p = info->next; p = &delta_list; diff --git a/builtin/update-index.c b/builtin/update-index.c index fe84003b4f..e7fab78b3b 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -669,7 +669,7 @@ static int unresolve_one(const char *path) ret = -1; goto free_return; } - if (!oidcmp(&ce_2->oid, &ce_3->oid) && + if (oideq(&ce_2->oid, &ce_3->oid) && ce_2->ce_mode == ce_3->ce_mode) { fprintf(stderr, "%s: identical in both, skipping.\n", path); @@ -754,7 +754,7 @@ static int do_reupdate(int ac, const char **av, old = read_one_ent(NULL, &head_oid, ce->name, ce_namelen(ce), 0); if (old && ce->ce_mode == old->ce_mode && - !oidcmp(&ce->oid, &old->oid)) { + oideq(&ce->oid, &old->oid)) { discard_cache_entry(old); continue; /* unchanged */ } diff --git a/bulk-checkin.c b/bulk-checkin.c index 9f3b644811..409ecb566b 100644 --- a/bulk-checkin.c +++ b/bulk-checkin.c @@ -72,7 +72,7 @@ static int already_written(struct bulk_checkin_state *state, struct object_id *o /* Might want to keep the list sorted */ for (i = 0; i < state->nr_written; i++) - if (!oidcmp(&state->written[i]->oid, oid)) + if (oideq(&state->written[i]->oid, oid)) return 1; /* This is a new object we need to keep */ diff --git a/cache-tree.c b/cache-tree.c index 16ea022c46..b49bb5c5be 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -714,7 +714,7 @@ int cache_tree_matches_traversal(struct cache_tree *root, it = find_cache_tree_from_traversal(root, info); it = cache_tree_find(it, ent->path); - if (it && it->entry_count > 0 && !oidcmp(ent->oid, &it->oid)) + if (it && it->entry_count > 0 && oideq(ent->oid, &it->oid)) return it->entry_count; return 0; } diff --git a/cache.h b/cache.h index f6d227fac7..d090f71706 100644 --- a/cache.h +++ b/cache.h @@ -1100,7 +1100,7 @@ static inline int is_empty_blob_sha1(const unsigned char *sha1) static inline int is_empty_blob_oid(const struct object_id *oid) { - return !oidcmp(oid, the_hash_algo->empty_blob); + return oideq(oid, the_hash_algo->empty_blob); } static inline int is_empty_tree_sha1(const unsigned char *sha1) @@ -1110,7 +1110,7 @@ static inline int is_empty_tree_sha1(const unsigned char *sha1) static inline int is_empty_tree_oid(const struct object_id *oid) { - return !oidcmp(oid, the_hash_algo->empty_tree); + return oideq(oid, the_hash_algo->empty_tree); } const char *empty_tree_oid_hex(void); diff --git a/combine-diff.c b/combine-diff.c index de7695e728..0fed4ca529 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -1138,8 +1138,8 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent, for (i = 0; i < num_parent; i++) { int j; for (j = 0; j < i; j++) { - if (!oidcmp(&elem->parent[i].oid, - &elem->parent[j].oid)) { + if (oideq(&elem->parent[i].oid, + &elem->parent[j].oid)) { reuse_combine_diff(sline, cnt, i, j); break; } diff --git a/commit-graph.c b/commit-graph.c index 8a1bec7b8a..050c516b0d 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -779,7 +779,7 @@ void write_commit_graph(const char *obj_dir, num_extra_edges = 0; for (i = 0; i < oids.nr; i++) { int num_parents = 0; - if (i > 0 && !oidcmp(&oids.list[i-1], &oids.list[i])) + if (i > 0 && oideq(&oids.list[i - 1], &oids.list[i])) continue; commits.list[commits.nr] = lookup_commit(the_repository, &oids.list[i]); diff --git a/connect.c b/connect.c index 94547e5056..24281b6082 100644 --- a/connect.c +++ b/connect.c @@ -224,7 +224,7 @@ static int process_dummy_ref(const char *line) return 0; name++; - return !oidcmp(&null_oid, &oid) && !strcmp(name, "capabilities^{}"); + return oideq(&null_oid, &oid) && !strcmp(name, "capabilities^{}"); } static void check_no_capabilities(const char *line, int len) diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci index 5869979be7..548c02336d 100644 --- a/contrib/coccinelle/object_id.cocci +++ b/contrib/coccinelle/object_id.cocci @@ -108,3 +108,9 @@ expression E1, E2; @@ - hashcpy(E1.hash, E2->hash) + oidcpy(&E1, E2) + +@@ +expression E1, E2; +@@ +- oidcmp(E1, E2) == 0 ++ oideq(E1, E2) diff --git a/diff-lib.c b/diff-lib.c index 88a98b1c06..c1f5a52654 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -366,7 +366,7 @@ static int show_modified(struct rev_info *revs, } oldmode = old_entry->ce_mode; - if (mode == oldmode && !oidcmp(oid, &old_entry->oid) && !dirty_submodule && + if (mode == oldmode && oideq(oid, &old_entry->oid) && !dirty_submodule && !revs->diffopt.flags.find_copies_harder) return 0; diff --git a/diff.c b/diff.c index 145cfbae59..5cada68267 100644 --- a/diff.c +++ b/diff.c @@ -3404,7 +3404,7 @@ static void builtin_diff(const char *name_a, if (!one->data && !two->data && S_ISREG(one->mode) && S_ISREG(two->mode) && !o->flags.binary) { - if (!oidcmp(&one->oid, &two->oid)) { + if (oideq(&one->oid, &two->oid)) { if (must_show_header) emit_diff_symbol(o, DIFF_SYMBOL_HEADER, header.buf, header.len, @@ -3569,7 +3569,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b, return; } - same_contents = !oidcmp(&one->oid, &two->oid); + same_contents = oideq(&one->oid, &two->oid); if (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) { data->is_binary = 1; @@ -5323,7 +5323,7 @@ int diff_unmodified_pair(struct diff_filepair *p) * dealing with a change. */ if (one->oid_valid && two->oid_valid && - !oidcmp(&one->oid, &two->oid) && + oideq(&one->oid, &two->oid) && !one->dirty_submodule && !two->dirty_submodule) return 1; /* no change */ if (!one->oid_valid && !two->oid_valid) diff --git a/diffcore-break.c b/diffcore-break.c index c64359f489..e11fcfdb39 100644 --- a/diffcore-break.c +++ b/diffcore-break.c @@ -58,7 +58,7 @@ static int should_break(struct diff_filespec *src, } if (src->oid_valid && dst->oid_valid && - !oidcmp(&src->oid, &dst->oid)) + oideq(&src->oid, &dst->oid)) return 0; /* they are the same */ if (diff_populate_filespec(src, 0) || diff_populate_filespec(dst, 0)) diff --git a/fast-import.c b/fast-import.c index 89bb0c9db3..a731088f96 100644 --- a/fast-import.c +++ b/fast-import.c @@ -572,7 +572,7 @@ static struct object_entry *find_object(struct object_id *oid) unsigned int h = oid->hash[0] << 8 | oid->hash[1]; struct object_entry *e; for (e = object_table[h]; e; e = e->next) - if (!oidcmp(oid, &e->idx.oid)) + if (oideq(oid, &e->idx.oid)) return e; return NULL; } @@ -583,7 +583,7 @@ static struct object_entry *insert_object(struct object_id *oid) struct object_entry *e = object_table[h]; while (e) { - if (!oidcmp(oid, &e->idx.oid)) + if (oideq(oid, &e->idx.oid)) return e; e = e->next; } @@ -1533,7 +1533,7 @@ static int tree_content_set( if (!*slash1) { if (!S_ISDIR(mode) && e->versions[1].mode == mode - && !oidcmp(&e->versions[1].oid, oid)) + && oideq(&e->versions[1].oid, oid)) return 0; e->versions[1].mode = mode; oidcpy(&e->versions[1].oid, oid); diff --git a/http-push.c b/http-push.c index 5eaf551b51..283495c18a 100644 --- a/http-push.c +++ b/http-push.c @@ -1859,7 +1859,7 @@ int cmd_main(int argc, const char **argv) continue; } - if (!oidcmp(&ref->old_oid, &ref->peer_ref->new_oid)) { + if (oideq(&ref->old_oid, &ref->peer_ref->new_oid)) { if (push_verbosely) fprintf(stderr, "'%s': up-to-date\n", ref->name); if (helper_status) diff --git a/log-tree.c b/log-tree.c index 7443e5fcc7..2edff78cff 100644 --- a/log-tree.c +++ b/log-tree.c @@ -472,7 +472,7 @@ static int which_parent(const struct object_id *oid, const struct commit *commit const struct commit_list *parent; for (nth = 0, parent = commit->parents; parent; parent = parent->next) { - if (!oidcmp(&parent->item->object.oid, oid)) + if (oideq(&parent->item->object.oid, oid)) return nth; nth++; } @@ -506,8 +506,8 @@ static int show_one_mergetag(struct commit *commit, if (parse_tag_buffer(the_repository, tag, extra->value, extra->len)) strbuf_addstr(&verify_message, "malformed mergetag\n"); else if (is_common_merge(commit) && - !oidcmp(&tag->tagged->oid, - &commit->parents->next->item->object.oid)) + oideq(&tag->tagged->oid, + &commit->parents->next->item->object.oid)) strbuf_addf(&verify_message, "merged tag '%s'\n", tag->tag); else if ((nth = which_parent(&tag->tagged->oid, commit)) < 0) diff --git a/merge-recursive.c b/merge-recursive.c index dcdc93019c..2904cb825e 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -156,7 +156,7 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two, shift_tree_by(&one->object.oid, &two->object.oid, &shifted, subtree_shift); } - if (!oidcmp(&two->object.oid, &shifted)) + if (oideq(&two->object.oid, &shifted)) return two; return lookup_tree(the_repository, &shifted); } @@ -179,7 +179,7 @@ static int oid_eq(const struct object_id *a, const struct object_id *b) { if (!a && !b) return 2; - return a && b && oidcmp(a, b) == 0; + return a && b && oideq(a, b); } enum rename_type { diff --git a/notes-merge.c b/notes-merge.c index 76ab19e702..0a47e54cf8 100644 --- a/notes-merge.c +++ b/notes-merge.c @@ -151,7 +151,7 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o, mp = find_notes_merge_pair_pos(changes, len, &obj, 1, &occupied); if (occupied) { /* We've found an addition/deletion pair */ - assert(!oidcmp(&mp->obj, &obj)); + assert(oideq(&mp->obj, &obj)); if (is_null_oid(&p->one->oid)) { /* addition */ assert(is_null_oid(&mp->remote)); oidcpy(&mp->remote, &p->two->oid); @@ -218,7 +218,7 @@ static void diff_tree_local(struct notes_merge_options *o, continue; } - assert(!oidcmp(&mp->obj, &obj)); + assert(oideq(&mp->obj, &obj)); if (is_null_oid(&p->two->oid)) { /* deletion */ /* * Either this is a true deletion (1), or it is part @@ -229,7 +229,7 @@ static void diff_tree_local(struct notes_merge_options *o, * (3) mp->local is uninitialized; set it to null_sha1 * (will be overwritten by following addition) */ - if (!oidcmp(&mp->local, &uninitialized)) + if (oideq(&mp->local, &uninitialized)) oidclr(&mp->local); } else if (is_null_oid(&p->one->oid)) { /* addition */ /* @@ -241,7 +241,7 @@ static void diff_tree_local(struct notes_merge_options *o, * (3) mp->local is null_sha1; set to p->two->sha1 */ assert(is_null_oid(&mp->local) || - !oidcmp(&mp->local, &uninitialized)); + oideq(&mp->local, &uninitialized)); oidcpy(&mp->local, &p->two->oid); } else { /* modification */ /* @@ -249,8 +249,8 @@ static void diff_tree_local(struct notes_merge_options *o, * match mp->base, and mp->local shall be uninitialized. * Set mp->local to p->two->sha1. */ - assert(!oidcmp(&p->one->oid, &mp->base)); - assert(!oidcmp(&mp->local, &uninitialized)); + assert(oideq(&p->one->oid, &mp->base)); + assert(oideq(&mp->local, &uninitialized)); oidcpy(&mp->local, &p->two->oid); } trace_printf("\t\tStored local change for %s: %.7s -> %.7s\n", @@ -480,14 +480,14 @@ static int merge_changes(struct notes_merge_options *o, oid_to_hex(&p->local), oid_to_hex(&p->remote)); - if (!oidcmp(&p->base, &p->remote)) { + if (oideq(&p->base, &p->remote)) { /* no remote change; nothing to do */ trace_printf("\t\t\tskipping (no remote change)\n"); - } else if (!oidcmp(&p->local, &p->remote)) { + } else if (oideq(&p->local, &p->remote)) { /* same change in local and remote; nothing to do */ trace_printf("\t\t\tskipping (local == remote)\n"); - } else if (!oidcmp(&p->local, &uninitialized) || - !oidcmp(&p->local, &p->base)) { + } else if (oideq(&p->local, &uninitialized) || + oideq(&p->local, &p->base)) { /* no local change; adopt remote change */ trace_printf("\t\t\tno local change, adopted remote\n"); if (add_note(t, &p->obj, &p->remote, @@ -621,14 +621,14 @@ int notes_merge(struct notes_merge_options *o, oid_to_hex(&local->object.oid), oid_to_hex(base_oid)); - if (!oidcmp(&remote->object.oid, base_oid)) { + if (oideq(&remote->object.oid, base_oid)) { /* Already merged; result == local commit */ if (o->verbosity >= 2) printf("Already up to date!\n"); oidcpy(result_oid, &local->object.oid); goto found_result; } - if (!oidcmp(&local->object.oid, base_oid)) { + if (oideq(&local->object.oid, base_oid)) { /* Fast-forward; result == remote commit */ if (o->verbosity >= 2) printf("Fast-forward\n"); diff --git a/notes.c b/notes.c index 32d3dbcc1e..b3386d6c36 100644 --- a/notes.c +++ b/notes.c @@ -266,9 +266,9 @@ static int note_tree_insert(struct notes_tree *t, struct int_node *tree, case PTR_TYPE_NOTE: switch (type) { case PTR_TYPE_NOTE: - if (!oidcmp(&l->key_oid, &entry->key_oid)) { + if (oideq(&l->key_oid, &entry->key_oid)) { /* skip concatenation if l == entry */ - if (!oidcmp(&l->val_oid, &entry->val_oid)) + if (oideq(&l->val_oid, &entry->val_oid)) return 0; ret = combine_notes(&l->val_oid, diff --git a/pack-write.c b/pack-write.c index a9d46bc03f..7d14716c40 100644 --- a/pack-write.c +++ b/pack-write.c @@ -124,7 +124,7 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec } hashwrite(f, obj->oid.hash, the_hash_algo->rawsz); if ((opts->flags & WRITE_IDX_STRICT) && - (i && !oidcmp(&list[-2]->oid, &obj->oid))) + (i && oideq(&list[-2]->oid, &obj->oid))) die("The same object %s appears twice in the pack", oid_to_hex(&obj->oid)); } diff --git a/read-cache.c b/read-cache.c index 7b1354d759..eb4919e77f 100644 --- a/read-cache.c +++ b/read-cache.c @@ -767,7 +767,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, /* It was suspected to be racily clean, but it turns out to be Ok */ was_same = (alias && !ce_stage(alias) && - !oidcmp(&alias->oid, &ce->oid) && + oideq(&alias->oid, &ce->oid) && ce->ce_mode == alias->ce_mode); if (pretend) diff --git a/ref-filter.c b/ref-filter.c index 0bccfceff2..ccca317ce1 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1710,7 +1710,7 @@ struct contains_stack { static int in_commit_list(const struct commit_list *want, struct commit *c) { for (; want; want = want->next) - if (!oidcmp(&want->item->object.oid, &c->object.oid)) + if (oideq(&want->item->object.oid, &c->object.oid)) return 1; return 0; } diff --git a/refs/files-backend.c b/refs/files-backend.c index 1f1a98e4cb..aa45f5317f 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2307,7 +2307,7 @@ static int check_old_oid(struct ref_update *update, struct object_id *oid, struct strbuf *err) { if (!(update->flags & REF_HAVE_OLD) || - !oidcmp(oid, &update->old_oid)) + oideq(oid, &update->old_oid)) return 0; if (is_null_oid(&update->old_oid)) @@ -2443,7 +2443,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, !(update->flags & REF_DELETING) && !(update->flags & REF_LOG_ONLY)) { if (!(update->type & REF_ISSYMREF) && - !oidcmp(&lock->old_oid, &update->new_oid)) { + oideq(&lock->old_oid, &update->new_oid)) { /* * The reference already has the desired * value, so we don't need to write it. diff --git a/remote.c b/remote.c index 7f6277a145..6f1ee9d640 100644 --- a/remote.c +++ b/remote.c @@ -1388,7 +1388,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, ref->deletion = is_null_oid(&ref->new_oid); if (!ref->deletion && - !oidcmp(&ref->old_oid, &ref->new_oid)) { + oideq(&ref->old_oid, &ref->new_oid)) { ref->status = REF_STATUS_UPTODATE; continue; } @@ -2049,7 +2049,7 @@ struct ref *guess_remote_head(const struct ref *head, /* If refs/heads/master could be right, it is. */ if (!all) { r = find_ref_by_name(refs, "refs/heads/master"); - if (r && !oidcmp(&r->old_oid, &head->old_oid)) + if (r && oideq(&r->old_oid, &head->old_oid)) return copy_ref(r); } @@ -2057,7 +2057,7 @@ struct ref *guess_remote_head(const struct ref *head, for (r = refs; r; r = r->next) { if (r != head && starts_with(r->name, "refs/heads/") && - !oidcmp(&r->old_oid, &head->old_oid)) { + oideq(&r->old_oid, &head->old_oid)) { *tail = copy_ref(r); tail = &((*tail)->next); if (!all) diff --git a/revision.c b/revision.c index de4dce600d..a2a569bb3b 100644 --- a/revision.c +++ b/revision.c @@ -3238,7 +3238,7 @@ static void track_linear(struct rev_info *revs, struct commit *commit) struct commit_list *p; for (p = revs->previous_parents; p; p = p->next) if (p->item == NULL || /* first commit */ - !oidcmp(&p->item->object.oid, &commit->object.oid)) + oideq(&p->item->object.oid, &commit->object.oid)) break; revs->linear = p != NULL; } diff --git a/sequencer.c b/sequencer.c index 6a11f49651..66ed7c7b28 100644 --- a/sequencer.c +++ b/sequencer.c @@ -610,7 +610,7 @@ static int is_index_unchanged(void) if (!(cache_tree_oid = get_cache_tree_oid())) return -1; - return !oidcmp(cache_tree_oid, get_commit_tree_oid(head_commit)); + return oideq(cache_tree_oid, get_commit_tree_oid(head_commit)); } static int write_author_script(const char *message) @@ -1258,9 +1258,9 @@ static int try_to_commit(struct strbuf *msg, const char *author, goto out; } - if (!(flags & ALLOW_EMPTY) && !oidcmp(current_head ? - get_commit_tree_oid(current_head) : - the_hash_algo->empty_tree, &tree)) { + if (!(flags & ALLOW_EMPTY) && oideq(current_head ? + get_commit_tree_oid(current_head) : + the_hash_algo->empty_tree, &tree)) { res = 1; /* run 'git commit' to display error message */ goto out; } @@ -1365,7 +1365,7 @@ static int is_original_commit_empty(struct commit *commit) ptree_oid = the_hash_algo->empty_tree; /* commit is root */ } - return !oidcmp(ptree_oid, get_commit_tree_oid(commit)); + return oideq(ptree_oid, get_commit_tree_oid(commit)); } /* @@ -1645,7 +1645,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, unborn = get_oid("HEAD", &head); /* Do we want to generate a root commit? */ if (is_pick_or_similar(command) && opts->have_squash_onto && - !oidcmp(&head, &opts->squash_onto)) { + oideq(&head, &opts->squash_onto)) { if (is_fixup(command)) return error(_("cannot fixup root commit")); flags |= CREATE_ROOT_COMMIT; @@ -1688,7 +1688,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, oid_to_hex(&commit->object.oid)); if (opts->allow_ff && !is_fixup(command) && - ((parent && !oidcmp(&parent->object.oid, &head)) || + ((parent && oideq(&parent->object.oid, &head)) || (!parent && unborn))) { if (is_rebase_i(opts)) write_author_script(msg.message); @@ -2393,7 +2393,7 @@ static int rollback_is_safe(void) if (get_oid("HEAD", &actual_head)) oidclr(&actual_head); - return !oidcmp(&actual_head, &expected_head); + return oideq(&actual_head, &expected_head); } static int reset_for_rollback(const struct object_id *oid) @@ -2954,7 +2954,7 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len, } if (opts->have_squash_onto && - !oidcmp(&head_commit->object.oid, &opts->squash_onto)) { + oideq(&head_commit->object.oid, &opts->squash_onto)) { /* * When the user tells us to "merge" something into a * "[new root]", let's simply fast-forward to the merge head. @@ -3023,8 +3023,8 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len, * commit, we cannot fast-forward. */ can_fast_forward = opts->allow_ff && commit && commit->parents && - !oidcmp(&commit->parents->item->object.oid, - &head_commit->object.oid); + oideq(&commit->parents->item->object.oid, + &head_commit->object.oid); /* * If any merge head is different from the original one, we cannot @@ -3102,8 +3102,8 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len, write_message("no-ff", 5, git_path_merge_mode(the_repository), 0); bases = get_merge_bases(head_commit, merge_commit); - if (bases && !oidcmp(&merge_commit->object.oid, - &bases->item->object.oid)) { + if (bases && oideq(&merge_commit->object.oid, + &bases->item->object.oid)) { ret = 0; /* skip merging an ancestor of HEAD */ goto leave_merge; @@ -3349,9 +3349,9 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts) */ if (item->command == TODO_REWORD && !get_oid("HEAD", &oid) && - (!oidcmp(&item->commit->object.oid, &oid) || + (oideq(&item->commit->object.oid, &oid) || (opts->have_squash_onto && - !oidcmp(&opts->squash_onto, &oid)))) + oideq(&opts->squash_onto, &oid)))) to_amend = 1; return res | error_with_patch(item->commit, @@ -3578,7 +3578,7 @@ static int commit_staged_changes(struct replay_opts *opts, * the commit message and if there was a squash, let the user * edit it. */ - if (is_clean && !oidcmp(&head, &to_amend) && + if (is_clean && oideq(&head, &to_amend) && opts->current_fixup_count > 0 && file_exists(rebase_path_stopped_sha())) { const char *p = opts->current_fixups.buf; diff --git a/sha1-array.c b/sha1-array.c index 265941fbf4..b94e0ec0f5 100644 --- a/sha1-array.c +++ b/sha1-array.c @@ -69,7 +69,7 @@ int oid_array_for_each_unique(struct oid_array *array, for (i = 0; i < array->nr; i++) { int ret; - if (i > 0 && !oidcmp(array->oid + i, array->oid + i - 1)) + if (i > 0 && oideq(array->oid + i, array->oid + i - 1)) continue; ret = fn(array->oid + i, data); if (ret) diff --git a/sha1-file.c b/sha1-file.c index 97b7423848..71f49ee56d 100644 --- a/sha1-file.c +++ b/sha1-file.c @@ -149,10 +149,10 @@ static struct cached_object *find_cached_object(const struct object_id *oid) struct cached_object *co = cached_objects; for (i = 0; i < cached_object_nr; i++, co++) { - if (!oidcmp(&co->oid, oid)) + if (oideq(&co->oid, oid)) return co; } - if (!oidcmp(oid, the_hash_algo->empty_tree)) + if (oideq(oid, the_hash_algo->empty_tree)) return &empty_tree; return NULL; } diff --git a/sha1-name.c b/sha1-name.c index c9cc1318b7..a0c8451d55 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -44,7 +44,7 @@ static void update_candidates(struct disambiguate_state *ds, const struct object oidcpy(&ds->candidate, current); ds->candidate_exists = 1; return; - } else if (!oidcmp(&ds->candidate, current)) { + } else if (oideq(&ds->candidate, current)) { /* the same as what we already have seen */ return; } diff --git a/submodule.c b/submodule.c index 50cbf5f13e..a2ce58e9e7 100644 --- a/submodule.c +++ b/submodule.c @@ -536,7 +536,7 @@ static void show_submodule_header(struct diff_options *o, const char *path, fast_backward = 1; } - if (!oidcmp(one, two)) { + if (oideq(one, two)) { strbuf_release(&sb); return; } diff --git a/transport.c b/transport.c index 06ffea2774..1c76d64aba 100644 --- a/transport.c +++ b/transport.c @@ -1228,7 +1228,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs) nr_refs++; if (rm->peer_ref && !is_null_oid(&rm->old_oid) && - !oidcmp(&rm->peer_ref->old_oid, &rm->old_oid)) + oideq(&rm->peer_ref->old_oid, &rm->old_oid)) continue; ALLOC_GROW(heads, nr_heads + 1, nr_alloc); heads[nr_heads++] = rm; diff --git a/unpack-trees.c b/unpack-trees.c index f25089b878..4056a92d55 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -630,7 +630,7 @@ static int switch_cache_bottom(struct traverse_info *info) static inline int are_same_oid(struct name_entry *name_j, struct name_entry *name_k) { - return name_j->oid && name_k->oid && !oidcmp(name_j->oid, name_k->oid); + return name_j->oid && name_k->oid && oideq(name_j->oid, name_k->oid); } static int traverse_trees_recursive(int n, unsigned long dirmask, @@ -1484,7 +1484,7 @@ static int same(const struct cache_entry *a, const struct cache_entry *b) if ((a->ce_flags | b->ce_flags) & CE_CONFLICTED) return 0; return a->ce_mode == b->ce_mode && - !oidcmp(&a->oid, &b->oid); + oideq(&a->oid, &b->oid); } @@ -1616,7 +1616,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce, * If we are not going to update the submodule, then * we don't care. */ - if (!sub_head && !oidcmp(&oid, &ce->oid)) + if (!sub_head && oideq(&oid, &ce->oid)) return 0; return verify_clean_submodule(sub_head ? NULL : oid_to_hex(&oid), ce, error_type, o); diff --git a/wt-status.c b/wt-status.c index 5ffab61015..1c8746d0ea 100644 --- a/wt-status.c +++ b/wt-status.c @@ -453,8 +453,8 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q, d->worktree_status = p->status; if (S_ISGITLINK(p->two->mode)) { d->dirty_submodule = p->two->dirty_submodule; - d->new_submodule_commits = !!oidcmp(&p->one->oid, - &p->two->oid); + d->new_submodule_commits = !oideq(&p->one->oid, + &p->two->oid); if (s->status_format == STATUS_FORMAT_SHORT) d->worktree_status = short_submodule_status(d); } @@ -1487,10 +1487,10 @@ static void wt_status_get_detached_from(struct wt_status_state *state) if (dwim_ref(cb.buf.buf, cb.buf.len, &oid, &ref) == 1 && /* sha1 is a commit? match without further lookup */ - (!oidcmp(&cb.noid, &oid) || + (oideq(&cb.noid, &oid) || /* perhaps sha1 is a tag, try to dereference to a commit */ ((commit = lookup_commit_reference_gently(the_repository, &oid, 1)) != NULL && - !oidcmp(&cb.noid, &commit->object.oid)))) { + oideq(&cb.noid, &commit->object.oid)))) { const char *from = ref; if (!skip_prefix(from, "refs/tags/", &from)) skip_prefix(from, "refs/remotes/", &from); @@ -1500,7 +1500,7 @@ static void wt_status_get_detached_from(struct wt_status_state *state) xstrdup(find_unique_abbrev(&cb.noid, DEFAULT_ABBREV)); oidcpy(&state->detached_oid, &cb.noid); state->detached_at = !get_oid("HEAD", &oid) && - !oidcmp(&oid, &state->detached_oid); + oideq(&oid, &state->detached_oid); free(ref); strbuf_release(&cb.buf); diff --git a/xdiff-interface.c b/xdiff-interface.c index ec6e574e4a..e7af95db86 100644 --- a/xdiff-interface.c +++ b/xdiff-interface.c @@ -186,7 +186,7 @@ void read_mmblob(mmfile_t *ptr, const struct object_id *oid) unsigned long size; enum object_type type; - if (!oidcmp(oid, &null_oid)) { + if (oideq(oid, &null_oid)) { ptr->ptr = xstrdup(""); ptr->size = 0; return; -- 2.19.0.rc0.412.g7005db4e88 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 3/9] convert "oidcmp() == 0" to oideq() 2018-08-25 8:07 ` [PATCH 3/9] convert "oidcmp() == 0" to oideq() Jeff King @ 2018-08-25 8:36 ` Jeff King 2018-08-27 12:31 ` Derrick Stolee 0 siblings, 1 reply; 30+ messages in thread From: Jeff King @ 2018-08-25 8:36 UTC (permalink / raw) To: SZEDER Gábor; +Cc: git, brian m. carlson, Derrick Stolee, Jacob Keller On Sat, Aug 25, 2018 at 04:07:15AM -0400, Jeff King wrote: > diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci > index 5869979be7..548c02336d 100644 > --- a/contrib/coccinelle/object_id.cocci > +++ b/contrib/coccinelle/object_id.cocci > @@ -108,3 +108,9 @@ expression E1, E2; > @@ > - hashcpy(E1.hash, E2->hash) > + oidcpy(&E1, E2) > + > +@@ > +expression E1, E2; > +@@ > +- oidcmp(E1, E2) == 0 > ++ oideq(E1, E2) > > [...] > > diff --git a/cache.h b/cache.h > index f6d227fac7..d090f71706 100644 > --- a/cache.h > +++ b/cache.h > @@ -1100,7 +1100,7 @@ static inline int is_empty_blob_sha1(const unsigned char *sha1) > > static inline int is_empty_blob_oid(const struct object_id *oid) > { > - return !oidcmp(oid, the_hash_algo->empty_blob); > + return oideq(oid, the_hash_algo->empty_blob); > } By the way, one interesting thing I noticed is that coccinelle generates the hunk for cache.h for _every_ file that includes it, and the resulting patch is annoying to apply. I think this is related to the discussion we were having in: https://public-inbox.org/git/20180802115522.16107-1-szeder.dev@gmail.com/ Namely that we might do better to invoke one big spatch (and let it parallelize internally) instead of one perfile. Presumably that would also avoid looking at the headers repeatedly (which would be both faster and produce better output). I'm not planning to pursue that immediately, so just food for thought at this point. -Peff ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/9] convert "oidcmp() == 0" to oideq() 2018-08-25 8:36 ` Jeff King @ 2018-08-27 12:31 ` Derrick Stolee 2018-08-27 12:33 ` Derrick Stolee 0 siblings, 1 reply; 30+ messages in thread From: Derrick Stolee @ 2018-08-27 12:31 UTC (permalink / raw) To: Jeff King, SZEDER Gábor; +Cc: git, brian m. carlson, Jacob Keller On 8/25/2018 4:36 AM, Jeff King wrote: > On Sat, Aug 25, 2018 at 04:07:15AM -0400, Jeff King wrote: > >> diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci >> index 5869979be7..548c02336d 100644 >> --- a/contrib/coccinelle/object_id.cocci >> +++ b/contrib/coccinelle/object_id.cocci >> @@ -108,3 +108,9 @@ expression E1, E2; >> @@ >> - hashcpy(E1.hash, E2->hash) >> + oidcpy(&E1, E2) Is this change intended? It doesn't seem to match the intention of the rest of the patch. >> + >> +@@ >> +expression E1, E2; >> +@@ >> +- oidcmp(E1, E2) == 0 >> ++ oideq(E1, E2) >> >> [...] >> >> diff --git a/cache.h b/cache.h >> index f6d227fac7..d090f71706 100644 >> --- a/cache.h >> +++ b/cache.h >> @@ -1100,7 +1100,7 @@ static inline int is_empty_blob_sha1(const unsigned char *sha1) >> >> static inline int is_empty_blob_oid(const struct object_id *oid) >> { >> - return !oidcmp(oid, the_hash_algo->empty_blob); >> + return oideq(oid, the_hash_algo->empty_blob); >> } > By the way, one interesting thing I noticed is that coccinelle generates > the hunk for cache.h for _every_ file that includes it, and the > resulting patch is annoying to apply. I think this is related to the > discussion we were having in: > > https://public-inbox.org/git/20180802115522.16107-1-szeder.dev@gmail.com/ > > Namely that we might do better to invoke one big spatch (and let it > parallelize internally) instead of one perfile. Presumably that would > also avoid looking at the headers repeatedly (which would be both faster > and produce better output). > > I'm not planning to pursue that immediately, so just food for thought at > this point. The more we use Coccinelle, the more we will learn how to improve the workflow. Thanks, -Stolee ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/9] convert "oidcmp() == 0" to oideq() 2018-08-27 12:31 ` Derrick Stolee @ 2018-08-27 12:33 ` Derrick Stolee 0 siblings, 0 replies; 30+ messages in thread From: Derrick Stolee @ 2018-08-27 12:33 UTC (permalink / raw) To: Jeff King, SZEDER Gábor; +Cc: git, brian m. carlson, Jacob Keller On 8/27/2018 8:31 AM, Derrick Stolee wrote: > On 8/25/2018 4:36 AM, Jeff King wrote: >> On Sat, Aug 25, 2018 at 04:07:15AM -0400, Jeff King wrote: >> >>> diff --git a/contrib/coccinelle/object_id.cocci >>> b/contrib/coccinelle/object_id.cocci >>> index 5869979be7..548c02336d 100644 >>> --- a/contrib/coccinelle/object_id.cocci >>> +++ b/contrib/coccinelle/object_id.cocci >>> @@ -108,3 +108,9 @@ expression E1, E2; >>> @@ >>> - hashcpy(E1.hash, E2->hash) >>> + oidcpy(&E1, E2) > > Is this change intended? It doesn't seem to match the intention of the > rest of the patch. Ignore me. It's just confusing to read the +/- notation from a cocci script alongside the file diff. ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 4/9] convert "hashcmp() == 0" to hasheq() 2018-08-25 8:00 [PATCH 0/9] introducing oideq() Jeff King ` (2 preceding siblings ...) 2018-08-25 8:07 ` [PATCH 3/9] convert "oidcmp() == 0" to oideq() Jeff King @ 2018-08-25 8:08 ` Jeff King 2018-08-25 8:09 ` [PATCH 5/9] convert "oidcmp() != 0" to "!oideq()" Jeff King ` (5 subsequent siblings) 9 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2018-08-25 8:08 UTC (permalink / raw) To: git; +Cc: brian m. carlson, Derrick Stolee, Jacob Keller This is the partner patch to the previous one, but covering the "hash" variants instead of "oid". Note that our coccinelle rule is slightly more complex to avoid triggering the call in hasheq(). I didn't bother to add a new rule to convert: - hasheq(E1->hash, E2->hash) + oideq(E1, E2) Since these are new functions, there won't be any such existing callers. And since most of the code is already using oideq, we're not likely to introduce new ones. We might still see "!hashcmp(E1->hash, E2->hash)" from topics in flight. But because our new rule comes after the existing ones, that should first get converted to "!oidcmp(E1, E2)" and then to "oideq(E1, E2)". Signed-off-by: Jeff King <peff@peff.net> --- builtin/fetch.c | 2 +- cache.h | 8 ++++---- contrib/coccinelle/object_id.cocci | 9 +++++++++ http-walker.c | 2 +- notes.c | 2 +- object.c | 2 +- pack-objects.c | 2 +- packfile.c | 10 +++++----- 8 files changed, 23 insertions(+), 14 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 32b1d5d383..84e0e8080f 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -238,7 +238,7 @@ static int will_fetch(struct ref **head, const unsigned char *sha1) { struct ref *rm = *head; while (rm) { - if (!hashcmp(rm->old_oid.hash, sha1)) + if (hasheq(rm->old_oid.hash, sha1)) return 1; rm = rm->next; } diff --git a/cache.h b/cache.h index d090f71706..d97db26bb6 100644 --- a/cache.h +++ b/cache.h @@ -1053,12 +1053,12 @@ static inline int oideq(const struct object_id *oid1, const struct object_id *oi static inline int is_null_sha1(const unsigned char *sha1) { - return !hashcmp(sha1, null_sha1); + return hasheq(sha1, null_sha1); } static inline int is_null_oid(const struct object_id *oid) { - return !hashcmp(oid->hash, null_sha1); + return hasheq(oid->hash, null_sha1); } static inline void hashcpy(unsigned char *sha_dst, const unsigned char *sha_src) @@ -1095,7 +1095,7 @@ static inline void oidread(struct object_id *oid, const unsigned char *hash) static inline int is_empty_blob_sha1(const unsigned char *sha1) { - return !hashcmp(sha1, the_hash_algo->empty_blob->hash); + return hasheq(sha1, the_hash_algo->empty_blob->hash); } static inline int is_empty_blob_oid(const struct object_id *oid) @@ -1105,7 +1105,7 @@ static inline int is_empty_blob_oid(const struct object_id *oid) static inline int is_empty_tree_sha1(const unsigned char *sha1) { - return !hashcmp(sha1, the_hash_algo->empty_tree->hash); + return hasheq(sha1, the_hash_algo->empty_tree->hash); } static inline int is_empty_tree_oid(const struct object_id *oid) diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci index 548c02336d..d90ba8a040 100644 --- a/contrib/coccinelle/object_id.cocci +++ b/contrib/coccinelle/object_id.cocci @@ -114,3 +114,12 @@ expression E1, E2; @@ - oidcmp(E1, E2) == 0 + oideq(E1, E2) + +@@ +identifier f != hasheq; +expression E1, E2; +@@ + f(...) {<... +- hashcmp(E1, E2) == 0 ++ hasheq(E1, E2) + ...>} diff --git a/http-walker.c b/http-walker.c index 7cdfb2f24c..3a8edc7f2f 100644 --- a/http-walker.c +++ b/http-walker.c @@ -483,7 +483,7 @@ static int fetch_object(struct walker *walker, unsigned char *sha1) list_for_each(pos, head) { obj_req = list_entry(pos, struct object_request, node); - if (!hashcmp(obj_req->oid.hash, sha1)) + if (hasheq(obj_req->oid.hash, sha1)) break; } if (obj_req == NULL) diff --git a/notes.c b/notes.c index b3386d6c36..33d16c1ec3 100644 --- a/notes.c +++ b/notes.c @@ -147,7 +147,7 @@ static struct leaf_node *note_tree_find(struct notes_tree *t, void **p = note_tree_search(t, &tree, &n, key_sha1); if (GET_PTR_TYPE(*p) == PTR_TYPE_NOTE) { struct leaf_node *l = (struct leaf_node *) CLR_PTR_TYPE(*p); - if (!hashcmp(key_sha1, l->key_oid.hash)) + if (hasheq(key_sha1, l->key_oid.hash)) return l; } return NULL; diff --git a/object.c b/object.c index 51c4594515..e54160550c 100644 --- a/object.c +++ b/object.c @@ -95,7 +95,7 @@ struct object *lookup_object(struct repository *r, const unsigned char *sha1) first = i = hash_obj(sha1, r->parsed_objects->obj_hash_size); while ((obj = r->parsed_objects->obj_hash[i]) != NULL) { - if (!hashcmp(sha1, obj->oid.hash)) + if (hasheq(sha1, obj->oid.hash)) break; i++; if (i == r->parsed_objects->obj_hash_size) diff --git a/pack-objects.c b/pack-objects.c index 6ef87e5683..2bc7626997 100644 --- a/pack-objects.c +++ b/pack-objects.c @@ -16,7 +16,7 @@ static uint32_t locate_object_entry_hash(struct packing_data *pdata, while (pdata->index[i] > 0) { uint32_t pos = pdata->index[i] - 1; - if (!hashcmp(sha1, pdata->objects[pos].idx.oid.hash)) { + if (hasheq(sha1, pdata->objects[pos].idx.oid.hash)) { *found = 1; return i; } diff --git a/packfile.c b/packfile.c index ebcb5742ec..c2e96293ad 100644 --- a/packfile.c +++ b/packfile.c @@ -1015,7 +1015,7 @@ void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1) { unsigned i; for (i = 0; i < p->num_bad_objects; i++) - if (!hashcmp(sha1, p->bad_object_sha1 + GIT_SHA1_RAWSZ * i)) + if (hasheq(sha1, p->bad_object_sha1 + GIT_SHA1_RAWSZ * i)) return; p->bad_object_sha1 = xrealloc(p->bad_object_sha1, st_mult(GIT_MAX_RAWSZ, @@ -1031,8 +1031,8 @@ const struct packed_git *has_packed_and_bad(const unsigned char *sha1) for (p = the_repository->objects->packed_git; p; p = p->next) for (i = 0; i < p->num_bad_objects; i++) - if (!hashcmp(sha1, - p->bad_object_sha1 + the_hash_algo->rawsz * i)) + if (hasheq(sha1, + p->bad_object_sha1 + the_hash_algo->rawsz * i)) return p; return NULL; } @@ -1830,8 +1830,8 @@ static int fill_pack_entry(const struct object_id *oid, if (p->num_bad_objects) { unsigned i; for (i = 0; i < p->num_bad_objects; i++) - if (!hashcmp(oid->hash, - p->bad_object_sha1 + the_hash_algo->rawsz * i)) + if (hasheq(oid->hash, + p->bad_object_sha1 + the_hash_algo->rawsz * i)) return 0; } -- 2.19.0.rc0.412.g7005db4e88 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 5/9] convert "oidcmp() != 0" to "!oideq()" 2018-08-25 8:00 [PATCH 0/9] introducing oideq() Jeff King ` (3 preceding siblings ...) 2018-08-25 8:08 ` [PATCH 4/9] convert "hashcmp() == 0" to hasheq() Jeff King @ 2018-08-25 8:09 ` Jeff King 2018-08-25 8:10 ` [PATCH 6/9] convert "hashcmp() != 0" to "!hasheq()" Jeff King ` (4 subsequent siblings) 9 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2018-08-25 8:09 UTC (permalink / raw) To: git; +Cc: brian m. carlson, Derrick Stolee, Jacob Keller This is the flip side of the previous two patches: checking for a non-zero oidcmp() can be more strictly expressed as inequality. Like those patches, we write "!= 0" in the coccinelle transformation, which covers by isomorphism the more common: if (oidcmp(E1, E2)) As with the previous two patches, this patch can be achieved almost entirely by running "make coccicheck"; the only differences are manual line-wrap fixes to match the original code. There is one thing to note for anybody replicating this, though: coccinelle 1.0.4 seems to miss the case in builtin/tag.c, even though it's basically the same as all the others. Running with 1.0.7 does catch this, so presumably it's just a coccinelle bug that was fixed in the interim. Signed-off-by: Jeff King <peff@peff.net> --- bisect.c | 2 +- blame.c | 4 ++-- builtin/fmt-merge-msg.c | 4 ++-- builtin/merge.c | 2 +- builtin/pull.c | 2 +- builtin/rm.c | 2 +- builtin/show-branch.c | 4 ++-- builtin/tag.c | 2 +- bundle.c | 2 +- commit-graph.c | 6 +++--- commit.c | 2 +- contrib/coccinelle/object_id.cocci | 6 ++++++ diff-lib.c | 2 +- diff.c | 6 +++--- diffcore-rename.c | 2 +- dir.c | 6 +++--- fast-import.c | 4 ++-- fetch-pack.c | 2 +- match-trees.c | 2 +- notes.c | 2 +- read-cache.c | 2 +- refs.c | 8 ++++---- refs/files-backend.c | 2 +- refs/packed-backend.c | 2 +- refs/ref-cache.c | 2 +- remote.c | 2 +- sequencer.c | 8 ++++---- sha1-file.c | 6 +++--- submodule-config.c | 4 ++-- t/helper/test-dump-cache-tree.c | 2 +- tree-diff.c | 2 +- 31 files changed, 55 insertions(+), 49 deletions(-) diff --git a/bisect.c b/bisect.c index 41c56a665e..7c1d8f1a6d 100644 --- a/bisect.c +++ b/bisect.c @@ -595,7 +595,7 @@ static struct commit_list *skip_away(struct commit_list *list, int count) for (i = 0; cur; cur = cur->next, i++) { if (i == index) { - if (oidcmp(&cur->item->object.oid, current_bad_oid)) + if (!oideq(&cur->item->object.oid, current_bad_oid)) return cur; if (previous) return previous; diff --git a/blame.c b/blame.c index 10d72e36dd..538d0ab1aa 100644 --- a/blame.c +++ b/blame.c @@ -1834,7 +1834,7 @@ void setup_scoreboard(struct blame_scoreboard *sb, sb->revs->children.name = "children"; while (c->parents && - oidcmp(&c->object.oid, &sb->final->object.oid)) { + !oideq(&c->object.oid, &sb->final->object.oid)) { struct commit_list *l = xcalloc(1, sizeof(*l)); l->item = c; @@ -1844,7 +1844,7 @@ void setup_scoreboard(struct blame_scoreboard *sb, c = c->parents->item; } - if (oidcmp(&c->object.oid, &sb->final->object.oid)) + if (!oideq(&c->object.oid, &sb->final->object.oid)) die(_("--reverse --first-parent together require range along first-parent chain")); } diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 4c82c234cb..268f0c20ca 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -78,9 +78,9 @@ static struct merge_parent *find_merge_parent(struct merge_parents *table, { int i; for (i = 0; i < table->nr; i++) { - if (given && oidcmp(&table->item[i].given, given)) + if (given && !oideq(&table->item[i].given, given)) continue; - if (commit && oidcmp(&table->item[i].commit, commit)) + if (commit && !oideq(&table->item[i].commit, commit)) continue; return &table->item[i]; } diff --git a/builtin/merge.c b/builtin/merge.c index 57abff999b..8d85d31a61 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1521,7 +1521,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) * HEAD^^" would be missed. */ common_one = get_merge_bases(head_commit, j->item); - if (oidcmp(&common_one->item->object.oid, &j->item->object.oid)) { + if (!oideq(&common_one->item->object.oid, &j->item->object.oid)) { up_to_date = 0; break; } diff --git a/builtin/pull.c b/builtin/pull.c index 27db0c69eb..58c23a374c 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -902,7 +902,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) oidclr(&curr_head); if (!is_null_oid(&orig_head) && !is_null_oid(&curr_head) && - oidcmp(&orig_head, &curr_head)) { + !oideq(&orig_head, &curr_head)) { /* * The fetch involved updating the current branch. * diff --git a/builtin/rm.c b/builtin/rm.c index 2cbe89e0ae..17086d3d97 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -180,7 +180,7 @@ static int check_local_mod(struct object_id *head, int index_only) if (no_head || get_tree_entry(head, name, &oid, &mode) || ce->ce_mode != create_ce_mode(mode) - || oidcmp(&ce->oid, &oid)) + || !oideq(&ce->oid, &oid)) staged_changes = 1; /* diff --git a/builtin/show-branch.c b/builtin/show-branch.c index 363cf8509a..5f9432861b 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -412,7 +412,7 @@ static int append_head_ref(const char *refname, const struct object_id *oid, /* If both heads/foo and tags/foo exists, get_sha1 would * get confused. */ - if (get_oid(refname + ofs, &tmp) || oidcmp(&tmp, oid)) + if (get_oid(refname + ofs, &tmp) || !oideq(&tmp, oid)) ofs = 5; return append_ref(refname + ofs, oid, 0); } @@ -427,7 +427,7 @@ static int append_remote_ref(const char *refname, const struct object_id *oid, /* If both heads/foo and tags/foo exists, get_sha1 would * get confused. */ - if (get_oid(refname + ofs, &tmp) || oidcmp(&tmp, oid)) + if (get_oid(refname + ofs, &tmp) || !oideq(&tmp, oid)) ofs = 5; return append_ref(refname + ofs, oid, 0); } diff --git a/builtin/tag.c b/builtin/tag.c index 9a19ffb49f..f623632186 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -559,7 +559,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) ref_transaction_commit(transaction, &err)) die("%s", err.buf); ref_transaction_free(transaction); - if (force && !is_null_oid(&prev) && oidcmp(&prev, &object)) + if (force && !is_null_oid(&prev) && !oideq(&prev, &object)) printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(&prev, DEFAULT_ABBREV)); diff --git a/bundle.c b/bundle.c index 24cbe40986..14f2cfc248 100644 --- a/bundle.c +++ b/bundle.c @@ -369,7 +369,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs) * commit that is referenced by the tag, and not the tag * itself. */ - if (oidcmp(&oid, &e->item->oid)) { + if (!oideq(&oid, &e->item->oid)) { /* * Is this the positive end of a range expressed * in terms of a tag (e.g. v2.0 from the range diff --git a/commit-graph.c b/commit-graph.c index 050c516b0d..7aed9f5371 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -765,7 +765,7 @@ void write_commit_graph(const char *obj_dir, count_distinct = 1; for (i = 1; i < oids.nr; i++) { - if (oidcmp(&oids.list[i-1], &oids.list[i])) + if (!oideq(&oids.list[i - 1], &oids.list[i])) count_distinct++; } @@ -960,7 +960,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g) continue; } - if (oidcmp(&get_commit_tree_in_graph_one(g, graph_commit)->object.oid, + if (!oideq(&get_commit_tree_in_graph_one(g, graph_commit)->object.oid, get_commit_tree_oid(odb_commit))) graph_report("root tree OID for commit %s in commit-graph is %s != %s", oid_to_hex(&cur_oid), @@ -977,7 +977,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g) break; } - if (oidcmp(&graph_parents->item->object.oid, &odb_parents->item->object.oid)) + if (!oideq(&graph_parents->item->object.oid, &odb_parents->item->object.oid)) graph_report("commit-graph parent for %s is %s != %s", oid_to_hex(&cur_oid), oid_to_hex(&graph_parents->item->object.oid), diff --git a/commit.c b/commit.c index 30d1af2b20..347f09722d 100644 --- a/commit.c +++ b/commit.c @@ -46,7 +46,7 @@ struct commit *lookup_commit_or_die(const struct object_id *oid, const char *ref struct commit *c = lookup_commit_reference(the_repository, oid); if (!c) die(_("could not parse %s"), ref_name); - if (oidcmp(oid, &c->object.oid)) { + if (!oideq(oid, &c->object.oid)) { warning(_("%s %s is not a commit!"), ref_name, oid_to_hex(oid)); } diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci index d90ba8a040..4e1f1a7431 100644 --- a/contrib/coccinelle/object_id.cocci +++ b/contrib/coccinelle/object_id.cocci @@ -123,3 +123,9 @@ expression E1, E2; - hashcmp(E1, E2) == 0 + hasheq(E1, E2) ...>} + +@@ +expression E1, E2; +@@ +- oidcmp(E1, E2) != 0 ++ !oideq(E1, E2) diff --git a/diff-lib.c b/diff-lib.c index c1f5a52654..8866b1d60c 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -342,7 +342,7 @@ static int show_modified(struct rev_info *revs, } if (revs->combine_merges && !cached && - (oidcmp(oid, &old_entry->oid) || oidcmp(&old_entry->oid, &new_entry->oid))) { + (!oideq(oid, &old_entry->oid) || !oideq(&old_entry->oid, &new_entry->oid))) { struct combine_diff_path *p; int pathlen = ce_namelen(new_entry); diff --git a/diff.c b/diff.c index 5cada68267..5d3219b600 100644 --- a/diff.c +++ b/diff.c @@ -3765,7 +3765,7 @@ static int reuse_worktree_file(const char *name, const struct object_id *oid, in * This is not the sha1 we are looking for, or * unreusable because it is not a regular file. */ - if (oidcmp(oid, &ce->oid) || !S_ISREG(ce->ce_mode)) + if (!oideq(oid, &ce->oid) || !S_ISREG(ce->ce_mode)) return 0; /* @@ -4170,7 +4170,7 @@ static void fill_metainfo(struct strbuf *msg, default: *must_show_header = 0; } - if (one && two && oidcmp(&one->oid, &two->oid)) { + if (one && two && !oideq(&one->oid, &two->oid)) { const unsigned hexsz = the_hash_algo->hexsz; int abbrev = o->flags.full_index ? hexsz : DEFAULT_ABBREV; @@ -5457,7 +5457,7 @@ static void diff_resolve_rename_copy(void) else p->status = DIFF_STATUS_RENAMED; } - else if (oidcmp(&p->one->oid, &p->two->oid) || + else if (!oideq(&p->one->oid, &p->two->oid) || p->one->mode != p->two->mode || p->one->dirty_submodule || p->two->dirty_submodule || diff --git a/diffcore-rename.c b/diffcore-rename.c index d775183c2f..daddd9b28a 100644 --- a/diffcore-rename.c +++ b/diffcore-rename.c @@ -286,7 +286,7 @@ static int find_identical_files(struct hashmap *srcs, struct diff_filespec *source = p->filespec; /* False hash collision? */ - if (oidcmp(&source->oid, &target->oid)) + if (!oideq(&source->oid, &target->oid)) continue; /* Non-regular files? If so, the modes must match! */ if (!S_ISREG(source->mode) || !S_ISREG(target->mode)) { diff --git a/dir.c b/dir.c index aceb0d4869..8ee9fe81b4 100644 --- a/dir.c +++ b/dir.c @@ -1282,7 +1282,7 @@ static void prep_exclude(struct dir_struct *dir, * order, though, if you do that. */ if (untracked && - oidcmp(&oid_stat.oid, &untracked->exclude_oid)) { + !oideq(&oid_stat.oid, &untracked->exclude_oid)) { invalidate_gitignore(dir->untracked, untracked); oidcpy(&untracked->exclude_oid, &oid_stat.oid); } @@ -2248,12 +2248,12 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d /* Validate $GIT_DIR/info/exclude and core.excludesfile */ root = dir->untracked->root; - if (oidcmp(&dir->ss_info_exclude.oid, + if (!oideq(&dir->ss_info_exclude.oid, &dir->untracked->ss_info_exclude.oid)) { invalidate_gitignore(dir->untracked, root); dir->untracked->ss_info_exclude = dir->ss_info_exclude; } - if (oidcmp(&dir->ss_excludes_file.oid, + if (!oideq(&dir->ss_excludes_file.oid, &dir->untracked->ss_excludes_file.oid)) { invalidate_gitignore(dir->untracked, root); dir->untracked->ss_excludes_file = dir->ss_excludes_file; diff --git a/fast-import.c b/fast-import.c index a731088f96..67a53b79cb 100644 --- a/fast-import.c +++ b/fast-import.c @@ -2649,7 +2649,7 @@ static int parse_from(struct branch *b) struct object_entry *oe = find_mark(idnum); if (oe->type != OBJ_COMMIT) die("Mark :%" PRIuMAX " not a commit", idnum); - if (oidcmp(&b->oid, &oe->idx.oid)) { + if (!oideq(&b->oid, &oe->idx.oid)) { oidcpy(&b->oid, &oe->idx.oid); if (oe->pack_id != MAX_PACK_ID) { unsigned long size; @@ -2667,7 +2667,7 @@ static int parse_from(struct branch *b) else die("Invalid ref name or SHA1 expression: %s", from); - if (b->branch_tree.tree && oidcmp(&oid, &b->branch_tree.versions[1].oid)) { + if (b->branch_tree.tree && !oideq(&oid, &b->branch_tree.versions[1].oid)) { release_tree_content_recursive(b->branch_tree.tree); b->branch_tree.tree = NULL; } diff --git a/fetch-pack.c b/fetch-pack.c index 88a078e9be..75047a4b2a 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -599,7 +599,7 @@ static void filter_refs(struct fetch_pack_args *args, continue; if (parse_oid_hex(ref->name, &oid, &p) || *p != '\0' || - oidcmp(&oid, &ref->old_oid)) + !oideq(&oid, &ref->old_oid)) continue; if ((allow_unadvertised_object_request & diff --git a/match-trees.c b/match-trees.c index 37653308d3..2b6d31ef9d 100644 --- a/match-trees.c +++ b/match-trees.c @@ -106,7 +106,7 @@ static int score_trees(const struct object_id *hash1, const struct object_id *ha update_tree_entry(&two); } else { /* path appears in both */ - if (oidcmp(one.entry.oid, two.entry.oid)) { + if (!oideq(one.entry.oid, two.entry.oid)) { /* they are different */ score += score_differs(one.entry.mode, two.entry.mode, diff --git a/notes.c b/notes.c index 33d16c1ec3..25cdce28b7 100644 --- a/notes.c +++ b/notes.c @@ -206,7 +206,7 @@ static void note_tree_remove(struct notes_tree *t, if (GET_PTR_TYPE(*p) != PTR_TYPE_NOTE) return; /* type mismatch, nothing to remove */ l = (struct leaf_node *) CLR_PTR_TYPE(*p); - if (oidcmp(&l->key_oid, &entry->key_oid)) + if (!oideq(&l->key_oid, &entry->key_oid)) return; /* key mismatch, nothing to remove */ /* we have found a matching entry */ diff --git a/read-cache.c b/read-cache.c index eb4919e77f..88bb80326b 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2029,7 +2029,7 @@ int read_index_from(struct index_state *istate, const char *path, base_oid_hex = oid_to_hex(&split_index->base_oid); base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex); ret = do_read_index(split_index->base, base_path, 1); - if (oidcmp(&split_index->base_oid, &split_index->base->oid)) + if (!oideq(&split_index->base_oid, &split_index->base->oid)) die("broken index, expect %s in %s, got %s", base_oid_hex, base_path, oid_to_hex(&split_index->base->oid)); diff --git a/refs.c b/refs.c index de81c7be7c..a7a75b4cc0 100644 --- a/refs.c +++ b/refs.c @@ -702,7 +702,7 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid, pseudoref); rollback_lock_file(&lock); goto done; - } else if (oidcmp(&actual_old_oid, old_oid)) { + } else if (!oideq(&actual_old_oid, old_oid)) { strbuf_addf(err, _("unexpected object ID when writing '%s'"), pseudoref); rollback_lock_file(&lock); @@ -744,7 +744,7 @@ static int delete_pseudoref(const char *pseudoref, const struct object_id *old_o } if (read_ref(pseudoref, &actual_old_oid)) die(_("could not read ref '%s'"), pseudoref); - if (oidcmp(&actual_old_oid, old_oid)) { + if (!oideq(&actual_old_oid, old_oid)) { error(_("unexpected object ID when deleting '%s'"), pseudoref); rollback_lock_file(&lock); @@ -875,13 +875,13 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid, */ if (!is_null_oid(&cb->ooid)) { oidcpy(cb->oid, noid); - if (oidcmp(&cb->ooid, noid)) + if (!oideq(&cb->ooid, noid)) warning(_("log for ref %s has gap after %s"), cb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822))); } else if (cb->date == cb->at_time) oidcpy(cb->oid, noid); - else if (oidcmp(noid, cb->oid)) + else if (!oideq(noid, cb->oid)) warning(_("log for ref %s unexpectedly ended on %s"), cb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822))); diff --git a/refs/files-backend.c b/refs/files-backend.c index aa45f5317f..16ef9325e0 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -841,7 +841,7 @@ static int verify_lock(struct ref_store *ref_store, struct ref_lock *lock, return 0; } } - if (old_oid && oidcmp(&lock->old_oid, old_oid)) { + if (old_oid && !oideq(&lock->old_oid, old_oid)) { strbuf_addf(err, "ref '%s' is at %s but expected %s", lock->ref_name, oid_to_hex(&lock->old_oid), diff --git a/refs/packed-backend.c b/refs/packed-backend.c index d447a731da..74e2996e93 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1160,7 +1160,7 @@ static int write_with_updates(struct packed_ref_store *refs, "reference already exists", update->refname); goto error; - } else if (oidcmp(&update->old_oid, iter->oid)) { + } else if (!oideq(&update->old_oid, iter->oid)) { strbuf_addf(err, "cannot update ref '%s': " "is at %s but expected %s", update->refname, diff --git a/refs/ref-cache.c b/refs/ref-cache.c index 9b110c8494..b7052f72e2 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -272,7 +272,7 @@ static int is_dup_ref(const struct ref_entry *ref1, const struct ref_entry *ref2 /* This is impossible by construction */ die("Reference directory conflict: %s", ref1->name); - if (oidcmp(&ref1->u.value.oid, &ref2->u.value.oid)) + if (!oideq(&ref1->u.value.oid, &ref2->u.value.oid)) die("Duplicated ref, and SHA1s don't match: %s", ref1->name); warning("Duplicated ref: %s", ref1->name); diff --git a/remote.c b/remote.c index 6f1ee9d640..e23b7675c8 100644 --- a/remote.c +++ b/remote.c @@ -1403,7 +1403,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, * branch. */ if (ref->expect_old_sha1) { - if (oidcmp(&ref->old_oid, &ref->old_oid_expect)) + if (!oideq(&ref->old_oid, &ref->old_oid_expect)) reject_reason = REF_STATUS_REJECT_STALE; else /* If the ref isn't stale then force the update. */ diff --git a/sequencer.c b/sequencer.c index 66ed7c7b28..b0ae6755b0 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1188,7 +1188,7 @@ static int parse_head(struct commit **head) current_head = lookup_commit_reference(the_repository, &oid); if (!current_head) return error(_("could not parse HEAD")); - if (oidcmp(&oid, ¤t_head->object.oid)) { + if (!oideq(&oid, ¤t_head->object.oid)) { warning(_("HEAD %s is not a commit!"), oid_to_hex(&oid)); } @@ -3034,7 +3034,7 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len, struct commit_list *p = commit->parents->next; for (j = to_merge; j && p; j = j->next, p = p->next) - if (oidcmp(&j->item->object.oid, + if (!oideq(&j->item->object.oid, &p->item->object.oid)) { can_fast_forward = 0; break; @@ -3566,7 +3566,7 @@ static int commit_staged_changes(struct replay_opts *opts, if (get_oid_hex(rev.buf, &to_amend)) return error(_("invalid contents: '%s'"), rebase_path_amend()); - if (!is_clean && oidcmp(&head, &to_amend)) + if (!is_clean && !oideq(&head, &to_amend)) return error(_("\nYou have uncommitted changes in your " "working tree. Please, commit them\n" "first and then run 'git rebase " @@ -4545,7 +4545,7 @@ int skip_unnecessary_picks(void) if (item->commit->parents->next) break; /* merge commit */ parent_oid = &item->commit->parents->item->object.oid; - if (oidcmp(parent_oid, oid)) + if (!oideq(parent_oid, oid)) break; oid = &item->commit->object.oid; } diff --git a/sha1-file.c b/sha1-file.c index 71f49ee56d..cc8a196349 100644 --- a/sha1-file.c +++ b/sha1-file.c @@ -825,7 +825,7 @@ int check_object_signature(const struct object_id *oid, void *map, if (map) { hash_object_file(map, size, type, &real_oid); - return oidcmp(oid, &real_oid) ? -1 : 0; + return !oideq(oid, &real_oid) ? -1 : 0; } st = open_istream(oid, &obj_type, &size, NULL); @@ -852,7 +852,7 @@ int check_object_signature(const struct object_id *oid, void *map, } the_hash_algo->final_fn(real_oid.hash, &c); close_istream(st); - return oidcmp(oid, &real_oid) ? -1 : 0; + return !oideq(oid, &real_oid) ? -1 : 0; } int git_open_cloexec(const char *name, int flags) @@ -1671,7 +1671,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr, die(_("deflateEnd on object %s failed (%d)"), oid_to_hex(oid), ret); the_hash_algo->final_fn(parano_oid.hash, &c); - if (oidcmp(oid, ¶no_oid) != 0) + if (!oideq(oid, ¶no_oid)) die(_("confused by unstable object source data for %s"), oid_to_hex(oid)); diff --git a/submodule-config.c b/submodule-config.c index fc2c41b947..e04ba756d9 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -45,7 +45,7 @@ static int config_path_cmp(const void *unused_cmp_data, const struct submodule_entry *b = entry_or_key; return strcmp(a->config->path, b->config->path) || - oidcmp(&a->config->gitmodules_oid, &b->config->gitmodules_oid); + !oideq(&a->config->gitmodules_oid, &b->config->gitmodules_oid); } static int config_name_cmp(const void *unused_cmp_data, @@ -57,7 +57,7 @@ static int config_name_cmp(const void *unused_cmp_data, const struct submodule_entry *b = entry_or_key; return strcmp(a->config->name, b->config->name) || - oidcmp(&a->config->gitmodules_oid, &b->config->gitmodules_oid); + !oideq(&a->config->gitmodules_oid, &b->config->gitmodules_oid); } static struct submodule_cache *submodule_cache_alloc(void) diff --git a/t/helper/test-dump-cache-tree.c b/t/helper/test-dump-cache-tree.c index 98a4891f1d..6a3f88f5f5 100644 --- a/t/helper/test-dump-cache-tree.c +++ b/t/helper/test-dump-cache-tree.c @@ -33,7 +33,7 @@ static int dump_cache_tree(struct cache_tree *it, } else { dump_one(it, pfx, ""); - if (oidcmp(&it->oid, &ref->oid) || + if (!oideq(&it->oid, &ref->oid) || ref->entry_count != it->entry_count || ref->subtree_nr != it->subtree_nr) { /* claims to be valid but is lying */ diff --git a/tree-diff.c b/tree-diff.c index fe2e466ac1..0c962a4510 100644 --- a/tree-diff.c +++ b/tree-diff.c @@ -491,7 +491,7 @@ static struct combine_diff_path *ll_diff_tree_paths( continue; /* diff(t,pi) != ø */ - if (oidcmp(t.entry.oid, tp[i].entry.oid) || + if (!oideq(t.entry.oid, tp[i].entry.oid) || (t.entry.mode != tp[i].entry.mode)) continue; -- 2.19.0.rc0.412.g7005db4e88 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 6/9] convert "hashcmp() != 0" to "!hasheq()" 2018-08-25 8:00 [PATCH 0/9] introducing oideq() Jeff King ` (4 preceding siblings ...) 2018-08-25 8:09 ` [PATCH 5/9] convert "oidcmp() != 0" to "!oideq()" Jeff King @ 2018-08-25 8:10 ` Jeff King 2018-08-25 8:14 ` [PATCH 7/9] convert hashmap comparison functions to oideq() Jeff King ` (3 subsequent siblings) 9 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2018-08-25 8:10 UTC (permalink / raw) To: git; +Cc: brian m. carlson, Derrick Stolee, Jacob Keller This rounds out the previous three patches, covering the inequality logic for the "hash" variant of the functions. As with the previous three, the accompanying code changes are the mechanical result of applying the coccinelle patch; see those patches for more discussion. Signed-off-by: Jeff King <peff@peff.net> --- Obviously all of these patches are basically "see the last one". I can squash them together if people prefer, but they are so unwieldy I thought it best to break it down. builtin/index-pack.c | 4 ++-- builtin/show-branch.c | 2 +- builtin/unpack-objects.c | 2 +- commit-graph.c | 2 +- contrib/coccinelle/object_id.cocci | 9 +++++++++ http-walker.c | 2 +- http.c | 2 +- pack-check.c | 6 +++--- pack-write.c | 2 +- packfile.c | 2 +- read-cache.c | 4 ++-- sha1-file.c | 2 +- 12 files changed, 24 insertions(+), 15 deletions(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index edcb0a3dca..2004e25da2 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1166,7 +1166,7 @@ static void parse_pack_objects(unsigned char *hash) /* Check pack integrity */ flush(); the_hash_algo->final_fn(hash, &input_ctx); - if (hashcmp(fill(the_hash_algo->rawsz), hash)) + if (!hasheq(fill(the_hash_algo->rawsz), hash)) die(_("pack is corrupted (SHA1 mismatch)")); use(the_hash_algo->rawsz); @@ -1280,7 +1280,7 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha fixup_pack_header_footer(output_fd, pack_hash, curr_pack, nr_objects, read_hash, consumed_bytes-the_hash_algo->rawsz); - if (hashcmp(read_hash, tail_hash) != 0) + if (!hasheq(read_hash, tail_hash)) die(_("Unexpected tail checksum for %s " "(disk corruption?)"), curr_pack); } diff --git a/builtin/show-branch.c b/builtin/show-branch.c index 5f9432861b..65f4a4c83c 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -485,7 +485,7 @@ static void snarf_refs(int head, int remotes) static int rev_is_head(const char *head, const char *name, unsigned char *head_sha1, unsigned char *sha1) { - if (!head || (head_sha1 && sha1 && hashcmp(head_sha1, sha1))) + if (!head || (head_sha1 && sha1 && !hasheq(head_sha1, sha1))) return 0; skip_prefix(head, "refs/heads/", &head); if (!skip_prefix(name, "refs/heads/", &name)) diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index ad438f5b41..80478808b3 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -579,7 +579,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix) if (fsck_finish(&fsck_options)) die(_("fsck error in pack objects")); } - if (hashcmp(fill(the_hash_algo->rawsz), oid.hash)) + if (!hasheq(fill(the_hash_algo->rawsz), oid.hash)) die("final sha1 did not match"); use(the_hash_algo->rawsz); diff --git a/commit-graph.c b/commit-graph.c index 7aed9f5371..64ce79420d 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -900,7 +900,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g) f = hashfd(devnull, NULL); hashwrite(f, g->data, g->data_len - g->hash_len); finalize_hashfile(f, checksum.hash, CSUM_CLOSE); - if (hashcmp(checksum.hash, g->data + g->data_len - g->hash_len)) { + if (!hasheq(checksum.hash, g->data + g->data_len - g->hash_len)) { graph_report(_("the commit-graph file has incorrect checksum and is likely corrupt")); verify_commit_graph_error = VERIFY_COMMIT_GRAPH_ERROR_HASH; } diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci index 4e1f1a7431..d8bdb48712 100644 --- a/contrib/coccinelle/object_id.cocci +++ b/contrib/coccinelle/object_id.cocci @@ -129,3 +129,12 @@ expression E1, E2; @@ - oidcmp(E1, E2) != 0 + !oideq(E1, E2) + +@@ +identifier f != hasheq; +expression E1, E2; +@@ + f(...) {<... +- hashcmp(E1, E2) != 0 ++ !hasheq(E1, E2) + ...>} diff --git a/http-walker.c b/http-walker.c index 3a8edc7f2f..b3334bf657 100644 --- a/http-walker.c +++ b/http-walker.c @@ -543,7 +543,7 @@ static int fetch_object(struct walker *walker, unsigned char *sha1) } else if (req->zret != Z_STREAM_END) { walker->corrupt_object_found++; ret = error("File %s (%s) corrupt", hex, req->url); - } else if (hashcmp(obj_req->oid.hash, req->real_sha1)) { + } else if (!hasheq(obj_req->oid.hash, req->real_sha1)) { ret = error("File %s has bad hash", hex); } else if (req->rename < 0) { struct strbuf buf = STRBUF_INIT; diff --git a/http.c b/http.c index b4bfbceaeb..e32a34a7d1 100644 --- a/http.c +++ b/http.c @@ -2394,7 +2394,7 @@ int finish_http_object_request(struct http_object_request *freq) unlink_or_warn(freq->tmpfile.buf); return -1; } - if (hashcmp(freq->sha1, freq->real_sha1)) { + if (!hasheq(freq->sha1, freq->real_sha1)) { unlink_or_warn(freq->tmpfile.buf); return -1; } diff --git a/pack-check.c b/pack-check.c index d3a57df34f..fa5f0ff8fa 100644 --- a/pack-check.c +++ b/pack-check.c @@ -79,10 +79,10 @@ static int verify_packfile(struct packed_git *p, } while (offset < pack_sig_ofs); the_hash_algo->final_fn(hash, &ctx); pack_sig = use_pack(p, w_curs, pack_sig_ofs, NULL); - if (hashcmp(hash, pack_sig)) + if (!hasheq(hash, pack_sig)) err = error("%s pack checksum mismatch", p->pack_name); - if (hashcmp(index_base + index_size - the_hash_algo->hexsz, pack_sig)) + if (!hasheq(index_base + index_size - the_hash_algo->hexsz, pack_sig)) err = error("%s pack checksum does not match its index", p->pack_name); unuse_pack(w_curs); @@ -180,7 +180,7 @@ int verify_pack_index(struct packed_git *p) the_hash_algo->init_fn(&ctx); the_hash_algo->update_fn(&ctx, index_base, (unsigned int)(index_size - the_hash_algo->rawsz)); the_hash_algo->final_fn(hash, &ctx); - if (hashcmp(hash, index_base + index_size - the_hash_algo->rawsz)) + if (!hasheq(hash, index_base + index_size - the_hash_algo->rawsz)) err = error("Packfile index for %s hash mismatch", p->pack_name); return err; diff --git a/pack-write.c b/pack-write.c index 7d14716c40..29d17a9bec 100644 --- a/pack-write.c +++ b/pack-write.c @@ -260,7 +260,7 @@ void fixup_pack_header_footer(int pack_fd, if (partial_pack_offset == 0) { unsigned char hash[GIT_MAX_RAWSZ]; the_hash_algo->final_fn(hash, &old_hash_ctx); - if (hashcmp(hash, partial_pack_hash) != 0) + if (!hasheq(hash, partial_pack_hash)) die("Unexpected checksum for %s " "(disk corruption?)", pack_name); /* diff --git a/packfile.c b/packfile.c index c2e96293ad..1e9eacd9b3 100644 --- a/packfile.c +++ b/packfile.c @@ -517,7 +517,7 @@ static int open_packed_git_1(struct packed_git *p) if (read_result != hashsz) return error("packfile %s signature is unavailable", p->pack_name); idx_hash = ((unsigned char *)p->index_data) + p->index_size - hashsz * 2; - if (hashcmp(hash, idx_hash)) + if (!hasheq(hash, idx_hash)) return error("packfile %s does not match index", p->pack_name); return 0; } diff --git a/read-cache.c b/read-cache.c index 88bb80326b..421a7f4953 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1668,7 +1668,7 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size) 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 (hashcmp(hash, (unsigned char *)hdr + size - the_hash_algo->rawsz)) + if (!hasheq(hash, (unsigned char *)hdr + size - the_hash_algo->rawsz)) return error("bad index file sha1 signature"); return 0; } @@ -2395,7 +2395,7 @@ static int verify_index_from(const struct index_state *istate, const char *path) if (n != the_hash_algo->rawsz) goto out; - if (hashcmp(istate->oid.hash, hash)) + if (!hasheq(istate->oid.hash, hash)) goto out; close(fd); diff --git a/sha1-file.c b/sha1-file.c index cc8a196349..d85f4e93e1 100644 --- a/sha1-file.c +++ b/sha1-file.c @@ -2213,7 +2213,7 @@ static int check_stream_sha1(git_zstream *stream, } the_hash_algo->final_fn(real_sha1, &c); - if (hashcmp(expected_sha1, real_sha1)) { + if (!hasheq(expected_sha1, real_sha1)) { error(_("sha1 mismatch for %s (expected %s)"), path, sha1_to_hex(expected_sha1)); return -1; -- 2.19.0.rc0.412.g7005db4e88 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 7/9] convert hashmap comparison functions to oideq() 2018-08-25 8:00 [PATCH 0/9] introducing oideq() Jeff King ` (5 preceding siblings ...) 2018-08-25 8:10 ` [PATCH 6/9] convert "hashcmp() != 0" to "!hasheq()" Jeff King @ 2018-08-25 8:14 ` Jeff King 2018-08-25 8:15 ` [PATCH 8/9] read-cache: use oideq() in ce_compare functions Jeff King ` (2 subsequent siblings) 9 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2018-08-25 8:14 UTC (permalink / raw) To: git; +Cc: brian m. carlson, Derrick Stolee, Jacob Keller The comparison functions used for hashmaps don't care about strict ordering; they only want to compare entries for equality. Let's use the oideq() function instead, which can potentially be better optimized. Note that unlike the previous patches mass-converting calls like "!oidcmp()", this patch could actually provide an improvement even with the current implementation. Those comparison functions are passed around as function pointers, so at compile-time the compiler cannot realize that the caller (which is in another file completely) will treat the return value as a boolean. Note that this does change the return values in quite a subtle way (it's still an int, but now the sign bit is irrelevant for ordering). Because of their funny hashmap-specific signature, it's unlikely that any of these static functions would be reused for more generic ordering. But to be double-sure, let's stop using "cmp" in their names. Calling them "eq" doesn't quite work either, because the hashmap convention is actually _inverted_. "0" means "same", and non-zero means "different". So I've called them "neq" by convention here. Signed-off-by: Jeff King <peff@peff.net> --- I actually think the hashmap inversion is vaguely insane and I'd be happy to see it flipped. But that's definitely outside the scope of this patch. It would take some care to do so in a way that the compiler would catch topics in flight, since the signature would not change (and nor would changing the name "cmpfn" in the hash struct help, because it is usually filled in by passing to hashmap_init()). builtin/describe.c | 6 +++--- oidmap.c | 12 ++++++------ patch-ids.c | 8 ++++---- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/builtin/describe.c b/builtin/describe.c index 1e7c75ba82..22c0541da5 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -62,7 +62,7 @@ static const char *prio_names[] = { N_("head"), N_("lightweight"), N_("annotated"), }; -static int commit_name_cmp(const void *unused_cmp_data, +static int commit_name_neq(const void *unused_cmp_data, const void *entry, const void *entry_or_key, const void *peeled) @@ -70,7 +70,7 @@ static int commit_name_cmp(const void *unused_cmp_data, const struct commit_name *cn1 = entry; const struct commit_name *cn2 = entry_or_key; - return oidcmp(&cn1->peeled, peeled ? peeled : &cn2->peeled); + return !oideq(&cn1->peeled, peeled ? peeled : &cn2->peeled); } static inline struct commit_name *find_commit_name(const struct object_id *peeled) @@ -596,7 +596,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix) return cmd_name_rev(args.argc, args.argv, prefix); } - hashmap_init(&names, commit_name_cmp, NULL, 0); + hashmap_init(&names, commit_name_neq, NULL, 0); for_each_rawref(get_name, NULL); if (!hashmap_get_size(&names) && !always) die(_("No names found, cannot describe anything.")); diff --git a/oidmap.c b/oidmap.c index d9fb19ba6a..b0841a0f58 100644 --- a/oidmap.c +++ b/oidmap.c @@ -1,14 +1,14 @@ #include "cache.h" #include "oidmap.h" -static int cmpfn(const void *hashmap_cmp_fn_data, - const void *entry, const void *entry_or_key, - const void *keydata) +static int oidmap_neq(const void *hashmap_cmp_fn_data, + const void *entry, const void *entry_or_key, + const void *keydata) { const struct oidmap_entry *entry_ = entry; if (keydata) - return oidcmp(&entry_->oid, (const struct object_id *) keydata); - return oidcmp(&entry_->oid, + return !oideq(&entry_->oid, (const struct object_id *) keydata); + return !oideq(&entry_->oid, &((const struct oidmap_entry *) entry_or_key)->oid); } @@ -21,7 +21,7 @@ static int hash(const struct object_id *oid) void oidmap_init(struct oidmap *map, size_t initial_size) { - hashmap_init(&map->map, cmpfn, NULL, initial_size); + hashmap_init(&map->map, oidmap_neq, NULL, initial_size); } void oidmap_free(struct oidmap *map, int free_entries) diff --git a/patch-ids.c b/patch-ids.c index 8f7c25d5db..960ea24054 100644 --- a/patch-ids.c +++ b/patch-ids.c @@ -28,14 +28,14 @@ int commit_patch_id(struct commit *commit, struct diff_options *options, /* * When we cannot load the full patch-id for both commits for whatever * reason, the function returns -1 (i.e. return error(...)). Despite - * the "cmp" in the name of this function, the caller only cares about + * the "neq" in the name of this function, the caller only cares about * the return value being zero (a and b are equivalent) or non-zero (a * and b are different), and returning non-zero would keep both in the * result, even if they actually were equivalent, in order to err on * the side of safety. The actual value being negative does not have * any significance; only that it is non-zero matters. */ -static int patch_id_cmp(const void *cmpfn_data, +static int patch_id_neq(const void *cmpfn_data, const void *entry, const void *entry_or_key, const void *unused_keydata) @@ -53,7 +53,7 @@ static int patch_id_cmp(const void *cmpfn_data, commit_patch_id(b->commit, opt, &b->patch_id, 0)) return error("Could not get patch ID for %s", oid_to_hex(&b->commit->object.oid)); - return oidcmp(&a->patch_id, &b->patch_id); + return !oideq(&a->patch_id, &b->patch_id); } int init_patch_ids(struct patch_ids *ids) @@ -63,7 +63,7 @@ int init_patch_ids(struct patch_ids *ids) ids->diffopts.detect_rename = 0; ids->diffopts.flags.recursive = 1; diff_setup_done(&ids->diffopts); - hashmap_init(&ids->patches, patch_id_cmp, &ids->diffopts, 256); + hashmap_init(&ids->patches, patch_id_neq, &ids->diffopts, 256); return 0; } -- 2.19.0.rc0.412.g7005db4e88 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 8/9] read-cache: use oideq() in ce_compare functions 2018-08-25 8:00 [PATCH 0/9] introducing oideq() Jeff King ` (6 preceding siblings ...) 2018-08-25 8:14 ` [PATCH 7/9] convert hashmap comparison functions to oideq() Jeff King @ 2018-08-25 8:15 ` Jeff King 2018-08-25 8:17 ` [PATCH 9/9] show_dirstat: simplify same-content check Jeff King 2018-08-26 20:56 ` [PATCH 0/9] introducing oideq() brian m. carlson 9 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2018-08-25 8:15 UTC (permalink / raw) To: git; +Cc: brian m. carlson, Derrick Stolee, Jacob Keller These functions return the full oidcmp() value, but the callers really only care whether it is non-zero. We can use the more strict !oideq(), which a compiler may be able to optimize further. This does change the meaning of the return value subtly, but it's unlikely that anybody would try to use them for ordering. They're static-local in this file, and they already return other error values that would confuse an ordering (e.g., open() failure gives -1). Signed-off-by: Jeff King <peff@peff.net> --- read-cache.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/read-cache.c b/read-cache.c index 421a7f4953..eb7cea6272 100644 --- a/read-cache.c +++ b/read-cache.c @@ -213,7 +213,7 @@ static int ce_compare_data(const struct cache_entry *ce, struct stat *st) if (fd >= 0) { struct object_id oid; if (!index_fd(&oid, fd, st, OBJ_BLOB, ce->name, 0)) - match = oidcmp(&oid, &ce->oid); + match = !oideq(&oid, &ce->oid); /* index_fd() closed the file descriptor already */ } return match; @@ -254,7 +254,7 @@ static int ce_compare_gitlink(const struct cache_entry *ce) */ if (resolve_gitlink_ref(ce->name, "HEAD", &oid) < 0) return 0; - return oidcmp(&oid, &ce->oid); + return !oideq(&oid, &ce->oid); } static int ce_modified_check_fs(const struct cache_entry *ce, struct stat *st) -- 2.19.0.rc0.412.g7005db4e88 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH 9/9] show_dirstat: simplify same-content check 2018-08-25 8:00 [PATCH 0/9] introducing oideq() Jeff King ` (7 preceding siblings ...) 2018-08-25 8:15 ` [PATCH 8/9] read-cache: use oideq() in ce_compare functions Jeff King @ 2018-08-25 8:17 ` Jeff King 2018-08-25 8:20 ` Eric Sunshine 2018-08-26 20:56 ` [PATCH 0/9] introducing oideq() brian m. carlson 9 siblings, 1 reply; 30+ messages in thread From: Jeff King @ 2018-08-25 8:17 UTC (permalink / raw) To: git; +Cc: brian m. carlson, Derrick Stolee, Jacob Keller We two nested conditionals to store a content_changed variable, but only bother to look at the result once, directly after we set it. We can drop the variable entirely and just use a single "if". This needless complexity is the result of 2ff3a80334 (Teach --dirstat not to completely ignore rearranged lines within a file, 2011-04-11). Before that, we held onto the content_changed variable much longer. While we're touching the condition, we can swap out oidcmp() for !oideq(). Our coccinelle patches didn't previously find this case because of the intermediate variable, but now it's a simple boolean in a conditional. Signed-off-by: Jeff King <peff@peff.net> --- diff.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/diff.c b/diff.c index 5d3219b600..605ba4b6b8 100644 --- a/diff.c +++ b/diff.c @@ -2933,16 +2933,11 @@ static void show_dirstat(struct diff_options *options) struct diff_filepair *p = q->queue[i]; const char *name; unsigned long copied, added, damage; - int content_changed; name = p->two->path ? p->two->path : p->one->path; - if (p->one->oid_valid && p->two->oid_valid) - content_changed = oidcmp(&p->one->oid, &p->two->oid); - else - content_changed = 1; - - if (!content_changed) { + if (p->one->oid_valid && p->two->oid_valid && + oideq(&p->one->oid, &p->two->oid)) { /* * The SHA1 has not changed, so pre-/post-content is * identical. We can therefore skip looking at the @@ -2989,7 +2984,7 @@ static void show_dirstat(struct diff_options *options) * made to the preimage. * If the resulting damage is zero, we know that * diffcore_count_changes() considers the two entries to - * be identical, but since content_changed is true, we + * be identical, but since the oid changed, we * know that there must have been _some_ kind of change, * so we force all entries to have damage > 0. */ -- 2.19.0.rc0.412.g7005db4e88 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 9/9] show_dirstat: simplify same-content check 2018-08-25 8:17 ` [PATCH 9/9] show_dirstat: simplify same-content check Jeff King @ 2018-08-25 8:20 ` Eric Sunshine 2018-08-25 8:23 ` Jeff King 0 siblings, 1 reply; 30+ messages in thread From: Eric Sunshine @ 2018-08-25 8:20 UTC (permalink / raw) To: Jeff King; +Cc: Git List, brian m. carlson, Derrick Stolee, Jacob Keller On Sat, Aug 25, 2018 at 4:17 AM Jeff King <peff@peff.net> wrote: > We two nested conditionals to store a content_changed grammo... > variable, but only bother to look at the result once, > directly after we set it. We can drop the variable entirely > and just use a single "if". > > This needless complexity is the result of 2ff3a80334 (Teach > --dirstat not to completely ignore rearranged lines within a > file, 2011-04-11). Before that, we held onto the > content_changed variable much longer. > > While we're touching the condition, we can swap out oidcmp() > for !oideq(). Our coccinelle patches didn't previously find > this case because of the intermediate variable, but now it's > a simple boolean in a conditional. > > Signed-off-by: Jeff King <peff@peff.net> ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 9/9] show_dirstat: simplify same-content check 2018-08-25 8:20 ` Eric Sunshine @ 2018-08-25 8:23 ` Jeff King 0 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2018-08-25 8:23 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, brian m. carlson, Derrick Stolee, Jacob Keller On Sat, Aug 25, 2018 at 04:20:33AM -0400, Eric Sunshine wrote: > On Sat, Aug 25, 2018 at 4:17 AM Jeff King <peff@peff.net> wrote: > > We two nested conditionals to store a content_changed > > grammo... Heh. Edited in my MUA while proof-reading, of course. ;) It should be "We use two...". -Peff ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/9] introducing oideq() 2018-08-25 8:00 [PATCH 0/9] introducing oideq() Jeff King ` (8 preceding siblings ...) 2018-08-25 8:17 ` [PATCH 9/9] show_dirstat: simplify same-content check Jeff King @ 2018-08-26 20:56 ` brian m. carlson 2018-08-27 12:41 ` Derrick Stolee 2018-08-28 21:21 ` Jeff King 9 siblings, 2 replies; 30+ messages in thread From: brian m. carlson @ 2018-08-26 20:56 UTC (permalink / raw) To: Jeff King; +Cc: git, Derrick Stolee, Jacob Keller [-- Attachment #1: Type: text/plain, Size: 2293 bytes --] On Sat, Aug 25, 2018 at 04:00:31AM -0400, Jeff King wrote: > This is a follow-up to the discussion in: > > https://public-inbox.org/git/20180822030344.GA14684@sigill.intra.peff.net/ > > The general idea is that the majority of callers don't care about actual > plus/minus ordering from oidcmp() and hashcmp(); they just want to know > if two oids are equal. The stricter equality check can be optimized > better by the compiler. > > Due to the simplicity of the current code and our inlining, the compiler > can usually figure this out for now. So I wouldn't expect this patch to > actually improve performance right away. But as that discussion shows, > we are likely to take a performance hit as we move to more runtime > determination of the_hash_algo parameters. Having these callers use the > more strict form will potentially help us recover that. > > So in that sense we _could_ simply punt on this series until then (and > it's certainly post-v2.19 material). But I think it's worth doing now, > simply from a readability/annotation standpoint. IMHO the resulting code > is more clear (though I've long since taught myself to read !foocmp() as > equality). I would quite like to see this series picked up for v2.20. If we want to minimize performance regressions with the SHA-256 work, I think it's important. Applying the following patch on top of this series causes gcc to inline both branches, which is pretty much the best we can expect. I haven't benchmarked it, though, so I can't say what the actual performance consequence is. As for this series itself, other than the typos people have pointed out, it looks good to me. diff --git a/cache.h b/cache.h index 3bb88ac6d0..1c182c6ef6 100644 --- a/cache.h +++ b/cache.h @@ -1033,7 +1033,10 @@ static inline int oidcmp(const struct object_id *oid1, const struct object_id *o static inline int hasheq(const unsigned char *sha1, const unsigned char *sha2) { - return !hashcmp(sha1, sha2); + if (the_hash_algo->rawsz == 32) { + return !memcmp(sha1, sha2, 32); + } + return !memcmp(sha1, sha2, 20); } static inline int oideq(const struct object_id *oid1, const struct object_id *oid2) -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 867 bytes --] ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 0/9] introducing oideq() 2018-08-26 20:56 ` [PATCH 0/9] introducing oideq() brian m. carlson @ 2018-08-27 12:41 ` Derrick Stolee 2018-08-28 21:21 ` Jeff King 1 sibling, 0 replies; 30+ messages in thread From: Derrick Stolee @ 2018-08-27 12:41 UTC (permalink / raw) To: brian m. carlson, Jeff King, git, Jacob Keller On 8/26/2018 4:56 PM, brian m. carlson wrote: > On Sat, Aug 25, 2018 at 04:00:31AM -0400, Jeff King wrote: >> This is a follow-up to the discussion in: >> >> https://public-inbox.org/git/20180822030344.GA14684@sigill.intra.peff.net/ >> >> The general idea is that the majority of callers don't care about actual >> plus/minus ordering from oidcmp() and hashcmp(); they just want to know >> if two oids are equal. The stricter equality check can be optimized >> better by the compiler. >> >> Due to the simplicity of the current code and our inlining, the compiler >> can usually figure this out for now. So I wouldn't expect this patch to >> actually improve performance right away. But as that discussion shows, >> we are likely to take a performance hit as we move to more runtime >> determination of the_hash_algo parameters. Having these callers use the >> more strict form will potentially help us recover that. >> >> So in that sense we _could_ simply punt on this series until then (and >> it's certainly post-v2.19 material). But I think it's worth doing now, >> simply from a readability/annotation standpoint. IMHO the resulting code >> is more clear (though I've long since taught myself to read !foocmp() as >> equality). > I would quite like to see this series picked up for v2.20. If we want > to minimize performance regressions with the SHA-256 work, I think it's > important. Seconded. > Applying the following patch on top of this series causes gcc to inline > both branches, which is pretty much the best we can expect. I haven't > benchmarked it, though, so I can't say what the actual performance > consequence is. We should definitely measure the cost here, but when we have the option to move to newhash, that cost should be acceptable. Thanks, -Stolee ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/9] introducing oideq() 2018-08-26 20:56 ` [PATCH 0/9] introducing oideq() brian m. carlson 2018-08-27 12:41 ` Derrick Stolee @ 2018-08-28 21:21 ` Jeff King 2018-08-28 21:22 ` [PATCH v2 1/9] coccinelle: use <...> for function exclusion Jeff King ` (10 more replies) 1 sibling, 11 replies; 30+ messages in thread From: Jeff King @ 2018-08-28 21:21 UTC (permalink / raw) To: brian m. carlson, git, Derrick Stolee, Jacob Keller On Sun, Aug 26, 2018 at 08:56:21PM +0000, brian m. carlson wrote: > > Due to the simplicity of the current code and our inlining, the compiler > > can usually figure this out for now. So I wouldn't expect this patch to > > actually improve performance right away. But as that discussion shows, > > we are likely to take a performance hit as we move to more runtime > > determination of the_hash_algo parameters. Having these callers use the > > more strict form will potentially help us recover that. > > > > So in that sense we _could_ simply punt on this series until then (and > > it's certainly post-v2.19 material). But I think it's worth doing now, > > simply from a readability/annotation standpoint. IMHO the resulting code > > is more clear (though I've long since taught myself to read !foocmp() as > > equality). > > I would quite like to see this series picked up for v2.20. If we want > to minimize performance regressions with the SHA-256 work, I think it's > important. Thanks. One of the things I was worried about was causing unnecessary conflicts with existing topics, including your work. But if everybody is on board, I'd be happy to see this go in early in the next release cycle (the longer we wait, the more annoying conflicts Junio has to resolve). > Applying the following patch on top of this series causes gcc to inline > both branches, which is pretty much the best we can expect. I haven't > benchmarked it, though, so I can't say what the actual performance > consequence is. > [...] > diff --git a/cache.h b/cache.h > index 3bb88ac6d0..1c182c6ef6 100644 > --- a/cache.h > +++ b/cache.h > @@ -1033,7 +1033,10 @@ static inline int oidcmp(const struct object_id *oid1, const struct object_id *o > > static inline int hasheq(const unsigned char *sha1, const unsigned char *sha2) > { > - return !hashcmp(sha1, sha2); > + if (the_hash_algo->rawsz == 32) { > + return !memcmp(sha1, sha2, 32); > + } > + return !memcmp(sha1, sha2, 20); > } Yeah, I can reproduce that, too. I'm actually surprised that's enough to convince the compiler to inline, based on my earlier tests. I wonder if it is because it is more able to see the boolean nature of the memcmp() (i.e., in the earlier versions it was not willing to carry that information across the conditional boundary for some reason). Or perhaps it is that this is written slightly differently from my earlier case, where the fallback did not say "32" explicitly, but rather: return memcmp(sha1, sha2, the_hash_algo->rawsz); For your form, we'd probably want to add a third case arm with a BUG(), just in case. > As for this series itself, other than the typos people have pointed out, > it looks good to me. Thanks. Here it is again with those typos fixed, but no changes to the patches themselves. 1: 7bcf611959 = 1: 7bcf611959 coccinelle: use <...> for function exclusion 2: 6025ebe5d1 ! 2: 0e90944f5a introduce hasheq() and oideq() @@ -5,11 +5,10 @@ The main comparison functions we provide for comparing object ids are hashcmp() and oidcmp(). These are more flexible than a strict equality check, since they also - express ordering. That makes them them useful for sorting - and binary searching. However, it also makes them - potentially slower than a strict equality check. Consider - this C code, which is traditionally what our hashcmp has - looked like: + express ordering. That makes them useful for sorting and + binary searching. However, it also makes them potentially + slower than a strict equality check. Consider this C code, + which is traditionally what our hashcmp has looked like: #include <string.h> int hashcmp(const unsigned char *a, const unsigned char *b) @@ -51,7 +50,7 @@ though the vast majority don't. We can solve that by introducing a hasheq() function (and - matching oideq() wrapper), which callers can use to make + matching oideq() wrapper), which callers can use to make it clear that they only care about equality. For now, the implementation will literally be "!hashcmp()", but it frees us up later to introduce code optimized specifically for the 3: 35a05a7e5c = 3: 4f1ea17e79 convert "oidcmp() == 0" to oideq() 4: b18fc3994d = 4: 6614cc71e4 convert "hashcmp() == 0" to hasheq() 5: 0fbcfcf7cc = 5: c3dd2b2c80 convert "oidcmp() != 0" to "!oideq()" 6: 32e169b886 = 6: c94914f1ef convert "hashcmp() != 0" to "!hasheq()" 7: b90051c38b = 7: c773fa9ca4 convert hashmap comparison functions to oideq() 8: b1e6ad80f0 = 8: ada5809fab read-cache: use oideq() in ce_compare functions 9: 18be86e078 ! 9: 62f24d63a4 show_dirstat: simplify same-content check @@ -2,7 +2,7 @@ show_dirstat: simplify same-content check - We two nested conditionals to store a content_changed + We use two nested conditionals to store a content_changed variable, but only bother to look at the result once, directly after we set it. We can drop the variable entirely and just use a single "if". ^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v2 1/9] coccinelle: use <...> for function exclusion 2018-08-28 21:21 ` Jeff King @ 2018-08-28 21:22 ` Jeff King 2018-08-28 21:22 ` [PATCH v2 2/9] introduce hasheq() and oideq() Jeff King ` (9 subsequent siblings) 10 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2018-08-28 21:22 UTC (permalink / raw) To: brian m. carlson, git, Derrick Stolee, Jacob Keller Sometimes we want to suppress a coccinelle transformation inside a particular function. For example, in finding conversions of hashcmp() to oidcmp(), we should not convert the call in oidcmp() itself, since that would cause infinite recursion. We write that like this: @@ identifier f != oidcmp; expression E1, E2; @@ f(...) {... - hashcmp(E1->hash, E2->hash) + oidcmp(E1, E2) ...} to match the interior of any function _except_ oidcmp(). Unfortunately, this doesn't catch all cases (e.g., the one in sequencer.c that this patch fixes). The problem, as explained by one of the Coccinelle developers in [1], is: For transformation, A ... B requires that B occur on every execution path starting with A, unless that execution path ends up in error handling code. (eg, if (...) { ... return; }). Here your A is the start of the function. So you need a call to hashcmp on every path through the function, which fails when you add ifs. [...] Another issue with A ... B is that by default A and B should not appear in the matched region. So your original rule matches only the case where every execution path contains exactly one call to hashcmp, not more than one. One way to solve this is to put the pattern inside an angle-bracket pattern like "<... P ...>", which allows zero or more matches of P. That works (and is what this patch does), but it has one drawback: it matches more than we care about, and Coccinelle uses extra CPU. Here are timings for "make coccicheck" before and after this patch: [before] real 1m27.122s user 7m34.451s sys 0m37.330s [after] real 2m18.040s user 10m58.310s sys 0m41.549s That's not ideal, but it's more important for this to be correct than to be fast. And coccicheck is already fairly slow (and people don't run it for every single patch). So it's an acceptable tradeoff. There _is_ a better way to do it, which is to record the position at which we find hashcmp(), and then check it against the forbidden function list. Like: @@ position p : script:python() { p[0].current_element != "oidcmp" }; expression E1,E2; @@ - hashcmp@p(E1->hash, E2->hash) + oidcmp(E1, E2) This is only a little slower than the current code, and does the right thing in all cases. Unfortunately, not all builds of Coccinelle include python support (including the ones in Debian). Requiring it may mean that fewer people can easily run the tool, which is worse than it simply being a little slower. We may want to revisit this decision in the future if: - builds with python become more common - we find more uses for python support that tip the cost-benefit analysis But for now this patch sticks with the angle-bracket solution, and converts all existing cocci patches. This fixes only one missed case in the current code, though it makes a much better difference for some new rules I'm adding (converting "!hashcmp()" to "hasheq()" misses over half the possible conversions using the old form). [1] https://public-inbox.org/git/alpine.DEB.2.21.1808240652370.2344@hadrien/ Helped-by: Julia Lawall <julia.lawall@lip6.fr> Signed-off-by: Jeff King <peff@peff.net> --- contrib/coccinelle/commit.cocci | 4 ++-- contrib/coccinelle/object_id.cocci | 20 ++++++++++---------- sequencer.c | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/contrib/coccinelle/commit.cocci b/contrib/coccinelle/commit.cocci index aec3345adb..c49aa558f0 100644 --- a/contrib/coccinelle/commit.cocci +++ b/contrib/coccinelle/commit.cocci @@ -15,10 +15,10 @@ expression c; identifier f !~ "^(get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit)$"; expression c; @@ - f(...) {... + f(...) {<... - c->maybe_tree + get_commit_tree(c) - ...} + ...>} @@ expression c; diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci index 09afdbf994..5869979be7 100644 --- a/contrib/coccinelle/object_id.cocci +++ b/contrib/coccinelle/object_id.cocci @@ -20,10 +20,10 @@ expression E1; identifier f != oid_to_hex; expression E1; @@ - f(...) {... + f(...) {<... - sha1_to_hex(E1->hash) + oid_to_hex(E1) - ...} + ...>} @@ expression E1, E2; @@ -35,10 +35,10 @@ expression E1, E2; identifier f != oid_to_hex_r; expression E1, E2; @@ - f(...) {... + f(...) {<... - sha1_to_hex_r(E1, E2->hash) + oid_to_hex_r(E1, E2) - ...} + ...>} @@ expression E1; @@ -50,10 +50,10 @@ expression E1; identifier f != oidclr; expression E1; @@ - f(...) {... + f(...) {<... - hashclr(E1->hash) + oidclr(E1) - ...} + ...>} @@ expression E1, E2; @@ -65,10 +65,10 @@ expression E1, E2; identifier f != oidcmp; expression E1, E2; @@ - f(...) {... + f(...) {<... - hashcmp(E1->hash, E2->hash) + oidcmp(E1, E2) - ...} + ...>} @@ expression E1, E2; @@ -92,10 +92,10 @@ expression E1, E2; identifier f != oidcpy; expression E1, E2; @@ - f(...) {... + f(...) {<... - hashcpy(E1->hash, E2->hash) + oidcpy(E1, E2) - ...} + ...>} @@ expression E1, E2; diff --git a/sequencer.c b/sequencer.c index 84bf598c3e..305de743a0 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4545,7 +4545,7 @@ int skip_unnecessary_picks(void) if (item->commit->parents->next) break; /* merge commit */ parent_oid = &item->commit->parents->item->object.oid; - if (hashcmp(parent_oid->hash, oid->hash)) + if (oidcmp(parent_oid, oid)) break; oid = &item->commit->object.oid; } -- 2.19.0.rc0.584.g84d5b2a847 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 2/9] introduce hasheq() and oideq() 2018-08-28 21:21 ` Jeff King 2018-08-28 21:22 ` [PATCH v2 1/9] coccinelle: use <...> for function exclusion Jeff King @ 2018-08-28 21:22 ` Jeff King 2018-08-28 21:22 ` [PATCH v2 3/9] convert "oidcmp() == 0" to oideq() Jeff King ` (8 subsequent siblings) 10 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2018-08-28 21:22 UTC (permalink / raw) To: brian m. carlson, git, Derrick Stolee, Jacob Keller The main comparison functions we provide for comparing object ids are hashcmp() and oidcmp(). These are more flexible than a strict equality check, since they also express ordering. That makes them useful for sorting and binary searching. However, it also makes them potentially slower than a strict equality check. Consider this C code, which is traditionally what our hashcmp has looked like: #include <string.h> int hashcmp(const unsigned char *a, const unsigned char *b) { return memcmp(a, b, 20); } Compiling with "gcc -O2 -S -fverbose-asm", the generated assembly shows that we actually call memcmp(). But if we change this to a strict equality check: return !memcmp(a, b, 20); we get a faster inline version: movq (%rdi), %rax # MEM[(void *)a_4(D)], MEM[(void *)a_4(D)] movq 8(%rdi), %rdx # MEM[(void *)a_4(D)], tmp101 xorq (%rsi), %rax # MEM[(void *)b_5(D)], tmp94 xorq 8(%rsi), %rdx # MEM[(void *)b_5(D)], tmp93 orq %rax, %rdx # tmp94, tmp93 jne .L2 #, movl 16(%rsi), %eax # MEM[(void *)b_5(D)], tmp104 cmpl %eax, 16(%rdi) # tmp104, MEM[(void *)a_4(D)] je .L5 #, Obviously our hashcmp() doesn't include the "!". But because it's an inline function, optimizing compilers are able to see "!hashcmp(a,b)" in calling code and take advantage of this case. So there has been no value thus far in introducing a more restricted interface for doing strict equality checks. But as Git learns about more values for the_hash_algo, our hashcmp() will grow more complicated and may even delegate at runtime to functions optimized specifically for that hash size. That breaks the inline connection we have, and the compiler will have to assume that the caller really cares about the sign and magnitude of the memcmp() result, even though the vast majority don't. We can solve that by introducing a hasheq() function (and matching oideq() wrapper), which callers can use to make it clear that they only care about equality. For now, the implementation will literally be "!hashcmp()", but it frees us up later to introduce code optimized specifically for the equality check. Signed-off-by: Jeff King <peff@peff.net> --- cache.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/cache.h b/cache.h index 4d014541ab..f6d227fac7 100644 --- a/cache.h +++ b/cache.h @@ -1041,6 +1041,16 @@ static inline int oidcmp(const struct object_id *oid1, const struct object_id *o return hashcmp(oid1->hash, oid2->hash); } +static inline int hasheq(const unsigned char *sha1, const unsigned char *sha2) +{ + return !hashcmp(sha1, sha2); +} + +static inline int oideq(const struct object_id *oid1, const struct object_id *oid2) +{ + return hasheq(oid1->hash, oid2->hash); +} + static inline int is_null_sha1(const unsigned char *sha1) { return !hashcmp(sha1, null_sha1); -- 2.19.0.rc0.584.g84d5b2a847 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 3/9] convert "oidcmp() == 0" to oideq() 2018-08-28 21:21 ` Jeff King 2018-08-28 21:22 ` [PATCH v2 1/9] coccinelle: use <...> for function exclusion Jeff King 2018-08-28 21:22 ` [PATCH v2 2/9] introduce hasheq() and oideq() Jeff King @ 2018-08-28 21:22 ` Jeff King 2018-08-28 21:22 ` [PATCH v2 4/9] convert "hashcmp() == 0" to hasheq() Jeff King ` (7 subsequent siblings) 10 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2018-08-28 21:22 UTC (permalink / raw) To: brian m. carlson, git, Derrick Stolee, Jacob Keller Using the more restrictive oideq() should, in the long run, give the compiler more opportunities to optimize these callsites. For now, this conversion should be a complete noop with respect to the generated code. The result is also perhaps a little more readable, as it avoids the "zero is equal" idiom. Since it's so prevalent in C, I think seasoned programmers tend not to even notice it anymore, but it can sometimes make for awkward double negations (e.g., we can drop a few !!oidcmp() instances here). This patch was generated almost entirely by the included coccinelle patch. This mechanical conversion should be completely safe, because we check explicitly for cases where oidcmp() is compared to 0, which is what oideq() is doing under the hood. Note that we don't have to catch "!oidcmp()" separately; coccinelle's standard isomorphisms make sure the two are treated equivalently. I say "almost" because I did hand-edit the coccinelle output to fix up a few style violations (it mostly keeps the original formatting, but sometimes unwraps long lines). Signed-off-by: Jeff King <peff@peff.net> --- bisect.c | 4 ++-- blame.c | 4 ++-- builtin/am.c | 2 +- builtin/checkout.c | 2 +- builtin/describe.c | 4 ++-- builtin/diff.c | 2 +- builtin/difftool.c | 4 ++-- builtin/fast-export.c | 2 +- builtin/fetch.c | 4 ++-- builtin/fmt-merge-msg.c | 2 +- builtin/index-pack.c | 4 ++-- builtin/log.c | 6 +++--- builtin/merge-tree.c | 2 +- builtin/merge.c | 4 ++-- builtin/pack-objects.c | 4 ++-- builtin/pull.c | 2 +- builtin/receive-pack.c | 4 ++-- builtin/remote.c | 2 +- builtin/replace.c | 6 +++--- builtin/unpack-objects.c | 2 +- builtin/update-index.c | 4 ++-- bulk-checkin.c | 2 +- cache-tree.c | 2 +- cache.h | 4 ++-- combine-diff.c | 4 ++-- commit-graph.c | 2 +- connect.c | 2 +- contrib/coccinelle/object_id.cocci | 6 ++++++ diff-lib.c | 2 +- diff.c | 6 +++--- diffcore-break.c | 2 +- fast-import.c | 6 +++--- http-push.c | 2 +- log-tree.c | 6 +++--- merge-recursive.c | 4 ++-- notes-merge.c | 24 +++++++++++----------- notes.c | 4 ++-- pack-write.c | 2 +- read-cache.c | 2 +- ref-filter.c | 2 +- refs/files-backend.c | 4 ++-- remote.c | 6 +++--- revision.c | 2 +- sequencer.c | 32 +++++++++++++++--------------- sha1-array.c | 2 +- sha1-file.c | 4 ++-- sha1-name.c | 2 +- submodule.c | 2 +- transport.c | 2 +- unpack-trees.c | 6 +++--- wt-status.c | 10 +++++----- xdiff-interface.c | 2 +- 52 files changed, 117 insertions(+), 111 deletions(-) diff --git a/bisect.c b/bisect.c index e1275ba79e..41c56a665e 100644 --- a/bisect.c +++ b/bisect.c @@ -807,7 +807,7 @@ static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout) for (; result; result = result->next) { const struct object_id *mb = &result->item->object.oid; - if (!oidcmp(mb, current_bad_oid)) { + if (oideq(mb, current_bad_oid)) { handle_bad_merge_base(); } else if (0 <= oid_array_lookup(&good_revs, mb)) { continue; @@ -988,7 +988,7 @@ int bisect_next_all(const char *prefix, int no_checkout) bisect_rev = &revs.commits->item->object.oid; - if (!oidcmp(bisect_rev, current_bad_oid)) { + if (oideq(bisect_rev, current_bad_oid)) { exit_if_skipped_commits(tried, current_bad_oid); printf("%s is the first %s commit\n", oid_to_hex(bisect_rev), term_bad); diff --git a/blame.c b/blame.c index aca06f4b12..c47d7050d9 100644 --- a/blame.c +++ b/blame.c @@ -1457,14 +1457,14 @@ static void pass_blame(struct blame_scoreboard *sb, struct blame_origin *origin, porigin = find(p, origin); if (!porigin) continue; - if (!oidcmp(&porigin->blob_oid, &origin->blob_oid)) { + if (oideq(&porigin->blob_oid, &origin->blob_oid)) { pass_whole_blame(sb, origin, porigin); blame_origin_decref(porigin); goto finish; } for (j = same = 0; j < i; j++) if (sg_origin[j] && - !oidcmp(&sg_origin[j]->blob_oid, &porigin->blob_oid)) { + oideq(&sg_origin[j]->blob_oid, &porigin->blob_oid)) { same = 1; break; } diff --git a/builtin/am.c b/builtin/am.c index 9f7ecf6ecb..e54110d474 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2077,7 +2077,7 @@ static int safe_to_abort(const struct am_state *state) if (get_oid("HEAD", &head)) oidclr(&head); - if (!oidcmp(&head, &abort_safety)) + if (oideq(&head, &abort_safety)) return 1; warning(_("You seem to have moved HEAD since the last 'am' failure.\n" diff --git a/builtin/checkout.c b/builtin/checkout.c index 29ef50013d..082e3a9f19 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -96,7 +96,7 @@ static int update_some(const struct object_id *oid, struct strbuf *base, if (pos >= 0) { struct cache_entry *old = active_cache[pos]; if (ce->ce_mode == old->ce_mode && - !oidcmp(&ce->oid, &old->oid)) { + oideq(&ce->oid, &old->oid)) { old->ce_flags |= CE_UPDATE; discard_cache_entry(ce); return 0; diff --git a/builtin/describe.c b/builtin/describe.c index 41606c8a90..1e7c75ba82 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -190,7 +190,7 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi /* Is it annotated? */ if (!peel_ref(path, &peeled)) { - is_annotated = !!oidcmp(oid, &peeled); + is_annotated = !oideq(oid, &peeled); } else { oidcpy(&peeled, oid); is_annotated = 0; @@ -469,7 +469,7 @@ static void process_object(struct object *obj, const char *path, void *data) { struct process_commit_data *pcd = data; - if (!oidcmp(&pcd->looking_for, &obj->oid) && !pcd->dst->len) { + if (oideq(&pcd->looking_for, &obj->oid) && !pcd->dst->len) { reset_revision_walk(); describe_commit(&pcd->current_commit, pcd->dst); strbuf_addf(pcd->dst, ":%s", path); diff --git a/builtin/diff.c b/builtin/diff.c index 361a3c3ed3..b3a8ba488f 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -41,7 +41,7 @@ static void stuff_change(struct diff_options *opt, struct diff_filespec *one, *two; if (!is_null_oid(old_oid) && !is_null_oid(new_oid) && - !oidcmp(old_oid, new_oid) && (old_mode == new_mode)) + oideq(old_oid, new_oid) && (old_mode == new_mode)) return; if (opt->flags.reverse_diff) { diff --git a/builtin/difftool.c b/builtin/difftool.c index cdd585ca76..b41a9199ff 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -116,7 +116,7 @@ static int use_wt_file(const char *workdir, const char *name, if (is_null_oid(oid)) { oidcpy(oid, &wt_oid); use = 1; - } else if (!oidcmp(oid, &wt_oid)) + } else if (oideq(oid, &wt_oid)) use = 1; } } @@ -438,7 +438,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, strbuf_reset(&buf); strbuf_addf(&buf, "Subproject commit %s", oid_to_hex(&roid)); - if (!oidcmp(&loid, &roid)) + if (oideq(&loid, &roid)) strbuf_addstr(&buf, "-dirty"); add_left_or_right(&submodules, dst_path, buf.buf, 1); continue; diff --git a/builtin/fast-export.c b/builtin/fast-export.c index 9bd8a14b57..74f3bf5c96 100644 --- a/builtin/fast-export.c +++ b/builtin/fast-export.c @@ -384,7 +384,7 @@ static void show_filemodify(struct diff_queue_struct *q, string_list_insert(changed, spec->path); putchar('\n'); - if (!oidcmp(&ospec->oid, &spec->oid) && + if (oideq(&ospec->oid, &spec->oid) && ospec->mode == spec->mode) break; } diff --git a/builtin/fetch.c b/builtin/fetch.c index 61bec5d213..32b1d5d383 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -507,7 +507,7 @@ static void adjust_refcol_width(const struct ref *ref) int max, rlen, llen, len; /* uptodate lines are only shown on high verbosity level */ - if (!verbosity && !oidcmp(&ref->peer_ref->old_oid, &ref->old_oid)) + if (!verbosity && oideq(&ref->peer_ref->old_oid, &ref->old_oid)) return; max = term_columns(); @@ -644,7 +644,7 @@ static int update_local_ref(struct ref *ref, if (type < 0) die(_("object %s not found"), oid_to_hex(&ref->new_oid)); - if (!oidcmp(&ref->old_oid, &ref->new_oid)) { + if (oideq(&ref->old_oid, &ref->new_oid)) { if (verbosity > 0) format_display(display, '=', _("[up to date]"), NULL, remote, pretty_ref, summary_width); diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index f35ff1612b..4c82c234cb 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -582,7 +582,7 @@ static void find_merge_parents(struct merge_parents *result, while (parents) { struct commit *cmit = pop_commit(&parents); for (i = 0; i < result->nr; i++) - if (!oidcmp(&result->item[i].commit, &cmit->object.oid)) + if (oideq(&result->item[i].commit, &cmit->object.oid)) result->item[i].used = 1; } diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 9582ead950..edcb0a3dca 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -719,9 +719,9 @@ static void find_ref_delta_children(const struct object_id *oid, *last_index = -1; return; } - while (first > 0 && !oidcmp(&ref_deltas[first - 1].oid, oid)) + while (first > 0 && oideq(&ref_deltas[first - 1].oid, oid)) --first; - while (last < end && !oidcmp(&ref_deltas[last + 1].oid, oid)) + while (last < end && oideq(&ref_deltas[last + 1].oid, oid)) ++last; *first_index = first; *last_index = last; diff --git a/builtin/log.c b/builtin/log.c index e094560d9a..98d668b56f 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -992,7 +992,7 @@ static char *find_branch_name(struct rev_info *rev) tip_oid = &rev->cmdline.rev[positive].item->oid; if (dwim_ref(ref, strlen(ref), &branch_oid, &full_ref) && skip_prefix(full_ref, "refs/heads/", &v) && - !oidcmp(tip_oid, &branch_oid)) + oideq(tip_oid, &branch_oid)) branch = xstrdup(v); free(full_ref); return branch; @@ -1703,7 +1703,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) /* Don't say anything if head and upstream are the same. */ if (rev.pending.nr == 2) { struct object_array_entry *o = rev.pending.objects; - if (oidcmp(&o[0].item->oid, &o[1].item->oid) == 0) + if (oideq(&o[0].item->oid, &o[1].item->oid)) return 0; } get_patch_ids(&rev, &ids); @@ -1949,7 +1949,7 @@ int cmd_cherry(int argc, const char **argv, const char *prefix) /* Don't say anything if head and upstream are the same. */ if (revs.pending.nr == 2) { struct object_array_entry *o = revs.pending.objects; - if (oidcmp(&o[0].item->oid, &o[1].item->oid) == 0) + if (oideq(&o[0].item->oid, &o[1].item->oid)) return 0; } diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index f8023bae1e..8cea8a74f2 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -155,7 +155,7 @@ static int same_entry(struct name_entry *a, struct name_entry *b) { return a->oid && b->oid && - !oidcmp(a->oid, b->oid) && + oideq(a->oid, b->oid) && a->mode == b->mode; } diff --git a/builtin/merge.c b/builtin/merge.c index 8f4a5065c2..57abff999b 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1189,7 +1189,7 @@ static int merging_a_throwaway_tag(struct commit *commit) tag_ref = xstrfmt("refs/tags/%s", ((struct tag *)merge_remote_util(commit)->obj)->tag); if (!read_ref(tag_ref, &oid) && - !oidcmp(&oid, &merge_remote_util(commit)->obj->oid)) + oideq(&oid, &merge_remote_util(commit)->obj->oid)) is_throwaway_tag = 0; else is_throwaway_tag = 1; @@ -1448,7 +1448,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) goto done; } else if (fast_forward != FF_NO && !remoteheads->next && !common->next && - !oidcmp(&common->item->object.oid, &head_commit->object.oid)) { + oideq(&common->item->object.oid, &head_commit->object.oid)) { /* Again the most common case of merging one remote. */ struct strbuf msg = STRBUF_INIT; struct commit *commit; diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index d1144a8f7e..64156f676b 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1202,7 +1202,7 @@ static struct pbase_tree_cache *pbase_tree_get(const struct object_id *oid) */ for (neigh = 0; neigh < 8; neigh++) { ent = pbase_tree_cache[my_ix]; - if (ent && !oidcmp(&ent->oid, oid)) { + if (ent && oideq(&ent->oid, oid)) { ent->ref++; return ent; } @@ -1384,7 +1384,7 @@ static void add_preferred_base(struct object_id *oid) return; for (it = pbase_tree; it; it = it->next) { - if (!oidcmp(&it->pcache.oid, &tree_oid)) { + if (oideq(&it->pcache.oid, &tree_oid)) { free(data); return; } diff --git a/builtin/pull.c b/builtin/pull.c index 681c127a07..a720f58a96 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -799,7 +799,7 @@ static int run_rebase(const struct object_id *curr_head, struct argv_array args = ARGV_ARRAY_INIT; if (!get_octopus_merge_base(&oct_merge_base, curr_head, merge_head, fork_point)) - if (!is_null_oid(fork_point) && !oidcmp(&oct_merge_base, fork_point)) + if (!is_null_oid(fork_point) && oideq(&oct_merge_base, fork_point)) fork_point = NULL; argv_array_push(&args, "rebase"); diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index c17ce94e12..5bb163d4d2 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1222,8 +1222,8 @@ static void check_aliased_update(struct command *cmd, struct string_list *list) dst_cmd = (struct command *) item->util; - if (!oidcmp(&cmd->old_oid, &dst_cmd->old_oid) && - !oidcmp(&cmd->new_oid, &dst_cmd->new_oid)) + if (oideq(&cmd->old_oid, &dst_cmd->old_oid) && + oideq(&cmd->new_oid, &dst_cmd->new_oid)) return; dst_cmd->skip_update = 1; diff --git a/builtin/remote.c b/builtin/remote.c index 7876db1c20..c171323e0e 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -412,7 +412,7 @@ static int get_push_ref_states(const struct ref *remote_refs, if (is_null_oid(&ref->new_oid)) { info->status = PUSH_STATUS_DELETE; - } else if (!oidcmp(&ref->old_oid, &ref->new_oid)) + } else if (oideq(&ref->old_oid, &ref->new_oid)) info->status = PUSH_STATUS_UPTODATE; else if (is_null_oid(&ref->old_oid)) info->status = PUSH_STATUS_CREATE; diff --git a/builtin/replace.c b/builtin/replace.c index 4f05791f3e..8e67e09819 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -343,7 +343,7 @@ static int edit_and_replace(const char *object_ref, int force, int raw) } free(tmpfile); - if (!oidcmp(&old_oid, &new_oid)) + if (oideq(&old_oid, &new_oid)) return error(_("new object is the same as the old one: '%s'"), oid_to_hex(&old_oid)); return replace_object_oid(object_ref, &old_oid, "replacement", &new_oid, force); @@ -414,7 +414,7 @@ static int check_one_mergetag(struct commit *commit, if (get_oid(mergetag_data->argv[i], &oid) < 0) return error(_("not a valid object name: '%s'"), mergetag_data->argv[i]); - if (!oidcmp(&tag->tagged->oid, &oid)) + if (oideq(&tag->tagged->oid, &oid)) return 0; /* found */ } @@ -474,7 +474,7 @@ static int create_graft(int argc, const char **argv, int force, int gentle) strbuf_release(&buf); - if (!oidcmp(&old_oid, &new_oid)) { + if (oideq(&old_oid, &new_oid)) { if (gentle) { warning(_("graft for '%s' unnecessary"), oid_to_hex(&old_oid)); return 0; diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index 30d9413b4b..ad438f5b41 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -303,7 +303,7 @@ static void added_object(unsigned nr, enum object_type type, struct delta_info *info; while ((info = *p) != NULL) { - if (!oidcmp(&info->base_oid, &obj_list[nr].oid) || + if (oideq(&info->base_oid, &obj_list[nr].oid) || info->base_offset == obj_list[nr].offset) { *p = info->next; p = &delta_list; diff --git a/builtin/update-index.c b/builtin/update-index.c index fe84003b4f..e7fab78b3b 100644 --- a/builtin/update-index.c +++ b/builtin/update-index.c @@ -669,7 +669,7 @@ static int unresolve_one(const char *path) ret = -1; goto free_return; } - if (!oidcmp(&ce_2->oid, &ce_3->oid) && + if (oideq(&ce_2->oid, &ce_3->oid) && ce_2->ce_mode == ce_3->ce_mode) { fprintf(stderr, "%s: identical in both, skipping.\n", path); @@ -754,7 +754,7 @@ static int do_reupdate(int ac, const char **av, old = read_one_ent(NULL, &head_oid, ce->name, ce_namelen(ce), 0); if (old && ce->ce_mode == old->ce_mode && - !oidcmp(&ce->oid, &old->oid)) { + oideq(&ce->oid, &old->oid)) { discard_cache_entry(old); continue; /* unchanged */ } diff --git a/bulk-checkin.c b/bulk-checkin.c index 9f3b644811..409ecb566b 100644 --- a/bulk-checkin.c +++ b/bulk-checkin.c @@ -72,7 +72,7 @@ static int already_written(struct bulk_checkin_state *state, struct object_id *o /* Might want to keep the list sorted */ for (i = 0; i < state->nr_written; i++) - if (!oidcmp(&state->written[i]->oid, oid)) + if (oideq(&state->written[i]->oid, oid)) return 1; /* This is a new object we need to keep */ diff --git a/cache-tree.c b/cache-tree.c index 16ea022c46..b49bb5c5be 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -714,7 +714,7 @@ int cache_tree_matches_traversal(struct cache_tree *root, it = find_cache_tree_from_traversal(root, info); it = cache_tree_find(it, ent->path); - if (it && it->entry_count > 0 && !oidcmp(ent->oid, &it->oid)) + if (it && it->entry_count > 0 && oideq(ent->oid, &it->oid)) return it->entry_count; return 0; } diff --git a/cache.h b/cache.h index f6d227fac7..d090f71706 100644 --- a/cache.h +++ b/cache.h @@ -1100,7 +1100,7 @@ static inline int is_empty_blob_sha1(const unsigned char *sha1) static inline int is_empty_blob_oid(const struct object_id *oid) { - return !oidcmp(oid, the_hash_algo->empty_blob); + return oideq(oid, the_hash_algo->empty_blob); } static inline int is_empty_tree_sha1(const unsigned char *sha1) @@ -1110,7 +1110,7 @@ static inline int is_empty_tree_sha1(const unsigned char *sha1) static inline int is_empty_tree_oid(const struct object_id *oid) { - return !oidcmp(oid, the_hash_algo->empty_tree); + return oideq(oid, the_hash_algo->empty_tree); } const char *empty_tree_oid_hex(void); diff --git a/combine-diff.c b/combine-diff.c index de7695e728..0fed4ca529 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -1138,8 +1138,8 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent, for (i = 0; i < num_parent; i++) { int j; for (j = 0; j < i; j++) { - if (!oidcmp(&elem->parent[i].oid, - &elem->parent[j].oid)) { + if (oideq(&elem->parent[i].oid, + &elem->parent[j].oid)) { reuse_combine_diff(sline, cnt, i, j); break; } diff --git a/commit-graph.c b/commit-graph.c index 8a1bec7b8a..050c516b0d 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -779,7 +779,7 @@ void write_commit_graph(const char *obj_dir, num_extra_edges = 0; for (i = 0; i < oids.nr; i++) { int num_parents = 0; - if (i > 0 && !oidcmp(&oids.list[i-1], &oids.list[i])) + if (i > 0 && oideq(&oids.list[i - 1], &oids.list[i])) continue; commits.list[commits.nr] = lookup_commit(the_repository, &oids.list[i]); diff --git a/connect.c b/connect.c index 94547e5056..24281b6082 100644 --- a/connect.c +++ b/connect.c @@ -224,7 +224,7 @@ static int process_dummy_ref(const char *line) return 0; name++; - return !oidcmp(&null_oid, &oid) && !strcmp(name, "capabilities^{}"); + return oideq(&null_oid, &oid) && !strcmp(name, "capabilities^{}"); } static void check_no_capabilities(const char *line, int len) diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci index 5869979be7..548c02336d 100644 --- a/contrib/coccinelle/object_id.cocci +++ b/contrib/coccinelle/object_id.cocci @@ -108,3 +108,9 @@ expression E1, E2; @@ - hashcpy(E1.hash, E2->hash) + oidcpy(&E1, E2) + +@@ +expression E1, E2; +@@ +- oidcmp(E1, E2) == 0 ++ oideq(E1, E2) diff --git a/diff-lib.c b/diff-lib.c index 88a98b1c06..c1f5a52654 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -366,7 +366,7 @@ static int show_modified(struct rev_info *revs, } oldmode = old_entry->ce_mode; - if (mode == oldmode && !oidcmp(oid, &old_entry->oid) && !dirty_submodule && + if (mode == oldmode && oideq(oid, &old_entry->oid) && !dirty_submodule && !revs->diffopt.flags.find_copies_harder) return 0; diff --git a/diff.c b/diff.c index 145cfbae59..5cada68267 100644 --- a/diff.c +++ b/diff.c @@ -3404,7 +3404,7 @@ static void builtin_diff(const char *name_a, if (!one->data && !two->data && S_ISREG(one->mode) && S_ISREG(two->mode) && !o->flags.binary) { - if (!oidcmp(&one->oid, &two->oid)) { + if (oideq(&one->oid, &two->oid)) { if (must_show_header) emit_diff_symbol(o, DIFF_SYMBOL_HEADER, header.buf, header.len, @@ -3569,7 +3569,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b, return; } - same_contents = !oidcmp(&one->oid, &two->oid); + same_contents = oideq(&one->oid, &two->oid); if (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) { data->is_binary = 1; @@ -5323,7 +5323,7 @@ int diff_unmodified_pair(struct diff_filepair *p) * dealing with a change. */ if (one->oid_valid && two->oid_valid && - !oidcmp(&one->oid, &two->oid) && + oideq(&one->oid, &two->oid) && !one->dirty_submodule && !two->dirty_submodule) return 1; /* no change */ if (!one->oid_valid && !two->oid_valid) diff --git a/diffcore-break.c b/diffcore-break.c index c64359f489..e11fcfdb39 100644 --- a/diffcore-break.c +++ b/diffcore-break.c @@ -58,7 +58,7 @@ static int should_break(struct diff_filespec *src, } if (src->oid_valid && dst->oid_valid && - !oidcmp(&src->oid, &dst->oid)) + oideq(&src->oid, &dst->oid)) return 0; /* they are the same */ if (diff_populate_filespec(src, 0) || diff_populate_filespec(dst, 0)) diff --git a/fast-import.c b/fast-import.c index 89bb0c9db3..a731088f96 100644 --- a/fast-import.c +++ b/fast-import.c @@ -572,7 +572,7 @@ static struct object_entry *find_object(struct object_id *oid) unsigned int h = oid->hash[0] << 8 | oid->hash[1]; struct object_entry *e; for (e = object_table[h]; e; e = e->next) - if (!oidcmp(oid, &e->idx.oid)) + if (oideq(oid, &e->idx.oid)) return e; return NULL; } @@ -583,7 +583,7 @@ static struct object_entry *insert_object(struct object_id *oid) struct object_entry *e = object_table[h]; while (e) { - if (!oidcmp(oid, &e->idx.oid)) + if (oideq(oid, &e->idx.oid)) return e; e = e->next; } @@ -1533,7 +1533,7 @@ static int tree_content_set( if (!*slash1) { if (!S_ISDIR(mode) && e->versions[1].mode == mode - && !oidcmp(&e->versions[1].oid, oid)) + && oideq(&e->versions[1].oid, oid)) return 0; e->versions[1].mode = mode; oidcpy(&e->versions[1].oid, oid); diff --git a/http-push.c b/http-push.c index 5eaf551b51..283495c18a 100644 --- a/http-push.c +++ b/http-push.c @@ -1859,7 +1859,7 @@ int cmd_main(int argc, const char **argv) continue; } - if (!oidcmp(&ref->old_oid, &ref->peer_ref->new_oid)) { + if (oideq(&ref->old_oid, &ref->peer_ref->new_oid)) { if (push_verbosely) fprintf(stderr, "'%s': up-to-date\n", ref->name); if (helper_status) diff --git a/log-tree.c b/log-tree.c index 7443e5fcc7..2edff78cff 100644 --- a/log-tree.c +++ b/log-tree.c @@ -472,7 +472,7 @@ static int which_parent(const struct object_id *oid, const struct commit *commit const struct commit_list *parent; for (nth = 0, parent = commit->parents; parent; parent = parent->next) { - if (!oidcmp(&parent->item->object.oid, oid)) + if (oideq(&parent->item->object.oid, oid)) return nth; nth++; } @@ -506,8 +506,8 @@ static int show_one_mergetag(struct commit *commit, if (parse_tag_buffer(the_repository, tag, extra->value, extra->len)) strbuf_addstr(&verify_message, "malformed mergetag\n"); else if (is_common_merge(commit) && - !oidcmp(&tag->tagged->oid, - &commit->parents->next->item->object.oid)) + oideq(&tag->tagged->oid, + &commit->parents->next->item->object.oid)) strbuf_addf(&verify_message, "merged tag '%s'\n", tag->tag); else if ((nth = which_parent(&tag->tagged->oid, commit)) < 0) diff --git a/merge-recursive.c b/merge-recursive.c index dcdc93019c..2904cb825e 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -156,7 +156,7 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two, shift_tree_by(&one->object.oid, &two->object.oid, &shifted, subtree_shift); } - if (!oidcmp(&two->object.oid, &shifted)) + if (oideq(&two->object.oid, &shifted)) return two; return lookup_tree(the_repository, &shifted); } @@ -179,7 +179,7 @@ static int oid_eq(const struct object_id *a, const struct object_id *b) { if (!a && !b) return 2; - return a && b && oidcmp(a, b) == 0; + return a && b && oideq(a, b); } enum rename_type { diff --git a/notes-merge.c b/notes-merge.c index 76ab19e702..0a47e54cf8 100644 --- a/notes-merge.c +++ b/notes-merge.c @@ -151,7 +151,7 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o, mp = find_notes_merge_pair_pos(changes, len, &obj, 1, &occupied); if (occupied) { /* We've found an addition/deletion pair */ - assert(!oidcmp(&mp->obj, &obj)); + assert(oideq(&mp->obj, &obj)); if (is_null_oid(&p->one->oid)) { /* addition */ assert(is_null_oid(&mp->remote)); oidcpy(&mp->remote, &p->two->oid); @@ -218,7 +218,7 @@ static void diff_tree_local(struct notes_merge_options *o, continue; } - assert(!oidcmp(&mp->obj, &obj)); + assert(oideq(&mp->obj, &obj)); if (is_null_oid(&p->two->oid)) { /* deletion */ /* * Either this is a true deletion (1), or it is part @@ -229,7 +229,7 @@ static void diff_tree_local(struct notes_merge_options *o, * (3) mp->local is uninitialized; set it to null_sha1 * (will be overwritten by following addition) */ - if (!oidcmp(&mp->local, &uninitialized)) + if (oideq(&mp->local, &uninitialized)) oidclr(&mp->local); } else if (is_null_oid(&p->one->oid)) { /* addition */ /* @@ -241,7 +241,7 @@ static void diff_tree_local(struct notes_merge_options *o, * (3) mp->local is null_sha1; set to p->two->sha1 */ assert(is_null_oid(&mp->local) || - !oidcmp(&mp->local, &uninitialized)); + oideq(&mp->local, &uninitialized)); oidcpy(&mp->local, &p->two->oid); } else { /* modification */ /* @@ -249,8 +249,8 @@ static void diff_tree_local(struct notes_merge_options *o, * match mp->base, and mp->local shall be uninitialized. * Set mp->local to p->two->sha1. */ - assert(!oidcmp(&p->one->oid, &mp->base)); - assert(!oidcmp(&mp->local, &uninitialized)); + assert(oideq(&p->one->oid, &mp->base)); + assert(oideq(&mp->local, &uninitialized)); oidcpy(&mp->local, &p->two->oid); } trace_printf("\t\tStored local change for %s: %.7s -> %.7s\n", @@ -480,14 +480,14 @@ static int merge_changes(struct notes_merge_options *o, oid_to_hex(&p->local), oid_to_hex(&p->remote)); - if (!oidcmp(&p->base, &p->remote)) { + if (oideq(&p->base, &p->remote)) { /* no remote change; nothing to do */ trace_printf("\t\t\tskipping (no remote change)\n"); - } else if (!oidcmp(&p->local, &p->remote)) { + } else if (oideq(&p->local, &p->remote)) { /* same change in local and remote; nothing to do */ trace_printf("\t\t\tskipping (local == remote)\n"); - } else if (!oidcmp(&p->local, &uninitialized) || - !oidcmp(&p->local, &p->base)) { + } else if (oideq(&p->local, &uninitialized) || + oideq(&p->local, &p->base)) { /* no local change; adopt remote change */ trace_printf("\t\t\tno local change, adopted remote\n"); if (add_note(t, &p->obj, &p->remote, @@ -621,14 +621,14 @@ int notes_merge(struct notes_merge_options *o, oid_to_hex(&local->object.oid), oid_to_hex(base_oid)); - if (!oidcmp(&remote->object.oid, base_oid)) { + if (oideq(&remote->object.oid, base_oid)) { /* Already merged; result == local commit */ if (o->verbosity >= 2) printf("Already up to date!\n"); oidcpy(result_oid, &local->object.oid); goto found_result; } - if (!oidcmp(&local->object.oid, base_oid)) { + if (oideq(&local->object.oid, base_oid)) { /* Fast-forward; result == remote commit */ if (o->verbosity >= 2) printf("Fast-forward\n"); diff --git a/notes.c b/notes.c index 32d3dbcc1e..b3386d6c36 100644 --- a/notes.c +++ b/notes.c @@ -266,9 +266,9 @@ static int note_tree_insert(struct notes_tree *t, struct int_node *tree, case PTR_TYPE_NOTE: switch (type) { case PTR_TYPE_NOTE: - if (!oidcmp(&l->key_oid, &entry->key_oid)) { + if (oideq(&l->key_oid, &entry->key_oid)) { /* skip concatenation if l == entry */ - if (!oidcmp(&l->val_oid, &entry->val_oid)) + if (oideq(&l->val_oid, &entry->val_oid)) return 0; ret = combine_notes(&l->val_oid, diff --git a/pack-write.c b/pack-write.c index a9d46bc03f..7d14716c40 100644 --- a/pack-write.c +++ b/pack-write.c @@ -124,7 +124,7 @@ const char *write_idx_file(const char *index_name, struct pack_idx_entry **objec } hashwrite(f, obj->oid.hash, the_hash_algo->rawsz); if ((opts->flags & WRITE_IDX_STRICT) && - (i && !oidcmp(&list[-2]->oid, &obj->oid))) + (i && oideq(&list[-2]->oid, &obj->oid))) die("The same object %s appears twice in the pack", oid_to_hex(&obj->oid)); } diff --git a/read-cache.c b/read-cache.c index 7b1354d759..eb4919e77f 100644 --- a/read-cache.c +++ b/read-cache.c @@ -767,7 +767,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st, /* It was suspected to be racily clean, but it turns out to be Ok */ was_same = (alias && !ce_stage(alias) && - !oidcmp(&alias->oid, &ce->oid) && + oideq(&alias->oid, &ce->oid) && ce->ce_mode == alias->ce_mode); if (pretend) diff --git a/ref-filter.c b/ref-filter.c index 0bccfceff2..ccca317ce1 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -1710,7 +1710,7 @@ struct contains_stack { static int in_commit_list(const struct commit_list *want, struct commit *c) { for (; want; want = want->next) - if (!oidcmp(&want->item->object.oid, &c->object.oid)) + if (oideq(&want->item->object.oid, &c->object.oid)) return 1; return 0; } diff --git a/refs/files-backend.c b/refs/files-backend.c index 1f1a98e4cb..aa45f5317f 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2307,7 +2307,7 @@ static int check_old_oid(struct ref_update *update, struct object_id *oid, struct strbuf *err) { if (!(update->flags & REF_HAVE_OLD) || - !oidcmp(oid, &update->old_oid)) + oideq(oid, &update->old_oid)) return 0; if (is_null_oid(&update->old_oid)) @@ -2443,7 +2443,7 @@ static int lock_ref_for_update(struct files_ref_store *refs, !(update->flags & REF_DELETING) && !(update->flags & REF_LOG_ONLY)) { if (!(update->type & REF_ISSYMREF) && - !oidcmp(&lock->old_oid, &update->new_oid)) { + oideq(&lock->old_oid, &update->new_oid)) { /* * The reference already has the desired * value, so we don't need to write it. diff --git a/remote.c b/remote.c index 7f6277a145..6f1ee9d640 100644 --- a/remote.c +++ b/remote.c @@ -1388,7 +1388,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, ref->deletion = is_null_oid(&ref->new_oid); if (!ref->deletion && - !oidcmp(&ref->old_oid, &ref->new_oid)) { + oideq(&ref->old_oid, &ref->new_oid)) { ref->status = REF_STATUS_UPTODATE; continue; } @@ -2049,7 +2049,7 @@ struct ref *guess_remote_head(const struct ref *head, /* If refs/heads/master could be right, it is. */ if (!all) { r = find_ref_by_name(refs, "refs/heads/master"); - if (r && !oidcmp(&r->old_oid, &head->old_oid)) + if (r && oideq(&r->old_oid, &head->old_oid)) return copy_ref(r); } @@ -2057,7 +2057,7 @@ struct ref *guess_remote_head(const struct ref *head, for (r = refs; r; r = r->next) { if (r != head && starts_with(r->name, "refs/heads/") && - !oidcmp(&r->old_oid, &head->old_oid)) { + oideq(&r->old_oid, &head->old_oid)) { *tail = copy_ref(r); tail = &((*tail)->next); if (!all) diff --git a/revision.c b/revision.c index de4dce600d..a2a569bb3b 100644 --- a/revision.c +++ b/revision.c @@ -3238,7 +3238,7 @@ static void track_linear(struct rev_info *revs, struct commit *commit) struct commit_list *p; for (p = revs->previous_parents; p; p = p->next) if (p->item == NULL || /* first commit */ - !oidcmp(&p->item->object.oid, &commit->object.oid)) + oideq(&p->item->object.oid, &commit->object.oid)) break; revs->linear = p != NULL; } diff --git a/sequencer.c b/sequencer.c index 305de743a0..5f823c9ec7 100644 --- a/sequencer.c +++ b/sequencer.c @@ -610,7 +610,7 @@ static int is_index_unchanged(void) if (!(cache_tree_oid = get_cache_tree_oid())) return -1; - return !oidcmp(cache_tree_oid, get_commit_tree_oid(head_commit)); + return oideq(cache_tree_oid, get_commit_tree_oid(head_commit)); } static int write_author_script(const char *message) @@ -1258,9 +1258,9 @@ static int try_to_commit(struct strbuf *msg, const char *author, goto out; } - if (!(flags & ALLOW_EMPTY) && !oidcmp(current_head ? - get_commit_tree_oid(current_head) : - the_hash_algo->empty_tree, &tree)) { + if (!(flags & ALLOW_EMPTY) && oideq(current_head ? + get_commit_tree_oid(current_head) : + the_hash_algo->empty_tree, &tree)) { res = 1; /* run 'git commit' to display error message */ goto out; } @@ -1365,7 +1365,7 @@ static int is_original_commit_empty(struct commit *commit) ptree_oid = the_hash_algo->empty_tree; /* commit is root */ } - return !oidcmp(ptree_oid, get_commit_tree_oid(commit)); + return oideq(ptree_oid, get_commit_tree_oid(commit)); } /* @@ -1645,7 +1645,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, unborn = get_oid("HEAD", &head); /* Do we want to generate a root commit? */ if (is_pick_or_similar(command) && opts->have_squash_onto && - !oidcmp(&head, &opts->squash_onto)) { + oideq(&head, &opts->squash_onto)) { if (is_fixup(command)) return error(_("cannot fixup root commit")); flags |= CREATE_ROOT_COMMIT; @@ -1688,7 +1688,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit, oid_to_hex(&commit->object.oid)); if (opts->allow_ff && !is_fixup(command) && - ((parent && !oidcmp(&parent->object.oid, &head)) || + ((parent && oideq(&parent->object.oid, &head)) || (!parent && unborn))) { if (is_rebase_i(opts)) write_author_script(msg.message); @@ -2393,7 +2393,7 @@ static int rollback_is_safe(void) if (get_oid("HEAD", &actual_head)) oidclr(&actual_head); - return !oidcmp(&actual_head, &expected_head); + return oideq(&actual_head, &expected_head); } static int reset_for_rollback(const struct object_id *oid) @@ -2954,7 +2954,7 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len, } if (opts->have_squash_onto && - !oidcmp(&head_commit->object.oid, &opts->squash_onto)) { + oideq(&head_commit->object.oid, &opts->squash_onto)) { /* * When the user tells us to "merge" something into a * "[new root]", let's simply fast-forward to the merge head. @@ -3023,8 +3023,8 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len, * commit, we cannot fast-forward. */ can_fast_forward = opts->allow_ff && commit && commit->parents && - !oidcmp(&commit->parents->item->object.oid, - &head_commit->object.oid); + oideq(&commit->parents->item->object.oid, + &head_commit->object.oid); /* * If any merge head is different from the original one, we cannot @@ -3102,8 +3102,8 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len, write_message("no-ff", 5, git_path_merge_mode(the_repository), 0); bases = get_merge_bases(head_commit, merge_commit); - if (bases && !oidcmp(&merge_commit->object.oid, - &bases->item->object.oid)) { + if (bases && oideq(&merge_commit->object.oid, + &bases->item->object.oid)) { ret = 0; /* skip merging an ancestor of HEAD */ goto leave_merge; @@ -3349,9 +3349,9 @@ static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts) */ if (item->command == TODO_REWORD && !get_oid("HEAD", &oid) && - (!oidcmp(&item->commit->object.oid, &oid) || + (oideq(&item->commit->object.oid, &oid) || (opts->have_squash_onto && - !oidcmp(&opts->squash_onto, &oid)))) + oideq(&opts->squash_onto, &oid)))) to_amend = 1; return res | error_with_patch(item->commit, @@ -3578,7 +3578,7 @@ static int commit_staged_changes(struct replay_opts *opts, * the commit message and if there was a squash, let the user * edit it. */ - if (is_clean && !oidcmp(&head, &to_amend) && + if (is_clean && oideq(&head, &to_amend) && opts->current_fixup_count > 0 && file_exists(rebase_path_stopped_sha())) { const char *p = opts->current_fixups.buf; diff --git a/sha1-array.c b/sha1-array.c index 265941fbf4..b94e0ec0f5 100644 --- a/sha1-array.c +++ b/sha1-array.c @@ -69,7 +69,7 @@ int oid_array_for_each_unique(struct oid_array *array, for (i = 0; i < array->nr; i++) { int ret; - if (i > 0 && !oidcmp(array->oid + i, array->oid + i - 1)) + if (i > 0 && oideq(array->oid + i, array->oid + i - 1)) continue; ret = fn(array->oid + i, data); if (ret) diff --git a/sha1-file.c b/sha1-file.c index 97b7423848..71f49ee56d 100644 --- a/sha1-file.c +++ b/sha1-file.c @@ -149,10 +149,10 @@ static struct cached_object *find_cached_object(const struct object_id *oid) struct cached_object *co = cached_objects; for (i = 0; i < cached_object_nr; i++, co++) { - if (!oidcmp(&co->oid, oid)) + if (oideq(&co->oid, oid)) return co; } - if (!oidcmp(oid, the_hash_algo->empty_tree)) + if (oideq(oid, the_hash_algo->empty_tree)) return &empty_tree; return NULL; } diff --git a/sha1-name.c b/sha1-name.c index c9cc1318b7..a0c8451d55 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -44,7 +44,7 @@ static void update_candidates(struct disambiguate_state *ds, const struct object oidcpy(&ds->candidate, current); ds->candidate_exists = 1; return; - } else if (!oidcmp(&ds->candidate, current)) { + } else if (oideq(&ds->candidate, current)) { /* the same as what we already have seen */ return; } diff --git a/submodule.c b/submodule.c index 50cbf5f13e..a2ce58e9e7 100644 --- a/submodule.c +++ b/submodule.c @@ -536,7 +536,7 @@ static void show_submodule_header(struct diff_options *o, const char *path, fast_backward = 1; } - if (!oidcmp(one, two)) { + if (oideq(one, two)) { strbuf_release(&sb); return; } diff --git a/transport.c b/transport.c index 06ffea2774..1c76d64aba 100644 --- a/transport.c +++ b/transport.c @@ -1228,7 +1228,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs) nr_refs++; if (rm->peer_ref && !is_null_oid(&rm->old_oid) && - !oidcmp(&rm->peer_ref->old_oid, &rm->old_oid)) + oideq(&rm->peer_ref->old_oid, &rm->old_oid)) continue; ALLOC_GROW(heads, nr_heads + 1, nr_alloc); heads[nr_heads++] = rm; diff --git a/unpack-trees.c b/unpack-trees.c index f25089b878..4056a92d55 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -630,7 +630,7 @@ static int switch_cache_bottom(struct traverse_info *info) static inline int are_same_oid(struct name_entry *name_j, struct name_entry *name_k) { - return name_j->oid && name_k->oid && !oidcmp(name_j->oid, name_k->oid); + return name_j->oid && name_k->oid && oideq(name_j->oid, name_k->oid); } static int traverse_trees_recursive(int n, unsigned long dirmask, @@ -1484,7 +1484,7 @@ static int same(const struct cache_entry *a, const struct cache_entry *b) if ((a->ce_flags | b->ce_flags) & CE_CONFLICTED) return 0; return a->ce_mode == b->ce_mode && - !oidcmp(&a->oid, &b->oid); + oideq(&a->oid, &b->oid); } @@ -1616,7 +1616,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce, * If we are not going to update the submodule, then * we don't care. */ - if (!sub_head && !oidcmp(&oid, &ce->oid)) + if (!sub_head && oideq(&oid, &ce->oid)) return 0; return verify_clean_submodule(sub_head ? NULL : oid_to_hex(&oid), ce, error_type, o); diff --git a/wt-status.c b/wt-status.c index 5ffab61015..1c8746d0ea 100644 --- a/wt-status.c +++ b/wt-status.c @@ -453,8 +453,8 @@ static void wt_status_collect_changed_cb(struct diff_queue_struct *q, d->worktree_status = p->status; if (S_ISGITLINK(p->two->mode)) { d->dirty_submodule = p->two->dirty_submodule; - d->new_submodule_commits = !!oidcmp(&p->one->oid, - &p->two->oid); + d->new_submodule_commits = !oideq(&p->one->oid, + &p->two->oid); if (s->status_format == STATUS_FORMAT_SHORT) d->worktree_status = short_submodule_status(d); } @@ -1487,10 +1487,10 @@ static void wt_status_get_detached_from(struct wt_status_state *state) if (dwim_ref(cb.buf.buf, cb.buf.len, &oid, &ref) == 1 && /* sha1 is a commit? match without further lookup */ - (!oidcmp(&cb.noid, &oid) || + (oideq(&cb.noid, &oid) || /* perhaps sha1 is a tag, try to dereference to a commit */ ((commit = lookup_commit_reference_gently(the_repository, &oid, 1)) != NULL && - !oidcmp(&cb.noid, &commit->object.oid)))) { + oideq(&cb.noid, &commit->object.oid)))) { const char *from = ref; if (!skip_prefix(from, "refs/tags/", &from)) skip_prefix(from, "refs/remotes/", &from); @@ -1500,7 +1500,7 @@ static void wt_status_get_detached_from(struct wt_status_state *state) xstrdup(find_unique_abbrev(&cb.noid, DEFAULT_ABBREV)); oidcpy(&state->detached_oid, &cb.noid); state->detached_at = !get_oid("HEAD", &oid) && - !oidcmp(&oid, &state->detached_oid); + oideq(&oid, &state->detached_oid); free(ref); strbuf_release(&cb.buf); diff --git a/xdiff-interface.c b/xdiff-interface.c index ec6e574e4a..e7af95db86 100644 --- a/xdiff-interface.c +++ b/xdiff-interface.c @@ -186,7 +186,7 @@ void read_mmblob(mmfile_t *ptr, const struct object_id *oid) unsigned long size; enum object_type type; - if (!oidcmp(oid, &null_oid)) { + if (oideq(oid, &null_oid)) { ptr->ptr = xstrdup(""); ptr->size = 0; return; -- 2.19.0.rc0.584.g84d5b2a847 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 4/9] convert "hashcmp() == 0" to hasheq() 2018-08-28 21:21 ` Jeff King ` (2 preceding siblings ...) 2018-08-28 21:22 ` [PATCH v2 3/9] convert "oidcmp() == 0" to oideq() Jeff King @ 2018-08-28 21:22 ` Jeff King 2018-08-28 21:22 ` [PATCH v2 5/9] convert "oidcmp() != 0" to "!oideq()" Jeff King ` (6 subsequent siblings) 10 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2018-08-28 21:22 UTC (permalink / raw) To: brian m. carlson, git, Derrick Stolee, Jacob Keller This is the partner patch to the previous one, but covering the "hash" variants instead of "oid". Note that our coccinelle rule is slightly more complex to avoid triggering the call in hasheq(). I didn't bother to add a new rule to convert: - hasheq(E1->hash, E2->hash) + oideq(E1, E2) Since these are new functions, there won't be any such existing callers. And since most of the code is already using oideq, we're not likely to introduce new ones. We might still see "!hashcmp(E1->hash, E2->hash)" from topics in flight. But because our new rule comes after the existing ones, that should first get converted to "!oidcmp(E1, E2)" and then to "oideq(E1, E2)". Signed-off-by: Jeff King <peff@peff.net> --- builtin/fetch.c | 2 +- cache.h | 8 ++++---- contrib/coccinelle/object_id.cocci | 9 +++++++++ http-walker.c | 2 +- notes.c | 2 +- object.c | 2 +- pack-objects.c | 2 +- packfile.c | 10 +++++----- 8 files changed, 23 insertions(+), 14 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 32b1d5d383..84e0e8080f 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -238,7 +238,7 @@ static int will_fetch(struct ref **head, const unsigned char *sha1) { struct ref *rm = *head; while (rm) { - if (!hashcmp(rm->old_oid.hash, sha1)) + if (hasheq(rm->old_oid.hash, sha1)) return 1; rm = rm->next; } diff --git a/cache.h b/cache.h index d090f71706..d97db26bb6 100644 --- a/cache.h +++ b/cache.h @@ -1053,12 +1053,12 @@ static inline int oideq(const struct object_id *oid1, const struct object_id *oi static inline int is_null_sha1(const unsigned char *sha1) { - return !hashcmp(sha1, null_sha1); + return hasheq(sha1, null_sha1); } static inline int is_null_oid(const struct object_id *oid) { - return !hashcmp(oid->hash, null_sha1); + return hasheq(oid->hash, null_sha1); } static inline void hashcpy(unsigned char *sha_dst, const unsigned char *sha_src) @@ -1095,7 +1095,7 @@ static inline void oidread(struct object_id *oid, const unsigned char *hash) static inline int is_empty_blob_sha1(const unsigned char *sha1) { - return !hashcmp(sha1, the_hash_algo->empty_blob->hash); + return hasheq(sha1, the_hash_algo->empty_blob->hash); } static inline int is_empty_blob_oid(const struct object_id *oid) @@ -1105,7 +1105,7 @@ static inline int is_empty_blob_oid(const struct object_id *oid) static inline int is_empty_tree_sha1(const unsigned char *sha1) { - return !hashcmp(sha1, the_hash_algo->empty_tree->hash); + return hasheq(sha1, the_hash_algo->empty_tree->hash); } static inline int is_empty_tree_oid(const struct object_id *oid) diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci index 548c02336d..d90ba8a040 100644 --- a/contrib/coccinelle/object_id.cocci +++ b/contrib/coccinelle/object_id.cocci @@ -114,3 +114,12 @@ expression E1, E2; @@ - oidcmp(E1, E2) == 0 + oideq(E1, E2) + +@@ +identifier f != hasheq; +expression E1, E2; +@@ + f(...) {<... +- hashcmp(E1, E2) == 0 ++ hasheq(E1, E2) + ...>} diff --git a/http-walker.c b/http-walker.c index 7cdfb2f24c..3a8edc7f2f 100644 --- a/http-walker.c +++ b/http-walker.c @@ -483,7 +483,7 @@ static int fetch_object(struct walker *walker, unsigned char *sha1) list_for_each(pos, head) { obj_req = list_entry(pos, struct object_request, node); - if (!hashcmp(obj_req->oid.hash, sha1)) + if (hasheq(obj_req->oid.hash, sha1)) break; } if (obj_req == NULL) diff --git a/notes.c b/notes.c index b3386d6c36..33d16c1ec3 100644 --- a/notes.c +++ b/notes.c @@ -147,7 +147,7 @@ static struct leaf_node *note_tree_find(struct notes_tree *t, void **p = note_tree_search(t, &tree, &n, key_sha1); if (GET_PTR_TYPE(*p) == PTR_TYPE_NOTE) { struct leaf_node *l = (struct leaf_node *) CLR_PTR_TYPE(*p); - if (!hashcmp(key_sha1, l->key_oid.hash)) + if (hasheq(key_sha1, l->key_oid.hash)) return l; } return NULL; diff --git a/object.c b/object.c index 51c4594515..e54160550c 100644 --- a/object.c +++ b/object.c @@ -95,7 +95,7 @@ struct object *lookup_object(struct repository *r, const unsigned char *sha1) first = i = hash_obj(sha1, r->parsed_objects->obj_hash_size); while ((obj = r->parsed_objects->obj_hash[i]) != NULL) { - if (!hashcmp(sha1, obj->oid.hash)) + if (hasheq(sha1, obj->oid.hash)) break; i++; if (i == r->parsed_objects->obj_hash_size) diff --git a/pack-objects.c b/pack-objects.c index 6ef87e5683..2bc7626997 100644 --- a/pack-objects.c +++ b/pack-objects.c @@ -16,7 +16,7 @@ static uint32_t locate_object_entry_hash(struct packing_data *pdata, while (pdata->index[i] > 0) { uint32_t pos = pdata->index[i] - 1; - if (!hashcmp(sha1, pdata->objects[pos].idx.oid.hash)) { + if (hasheq(sha1, pdata->objects[pos].idx.oid.hash)) { *found = 1; return i; } diff --git a/packfile.c b/packfile.c index ebcb5742ec..c2e96293ad 100644 --- a/packfile.c +++ b/packfile.c @@ -1015,7 +1015,7 @@ void mark_bad_packed_object(struct packed_git *p, const unsigned char *sha1) { unsigned i; for (i = 0; i < p->num_bad_objects; i++) - if (!hashcmp(sha1, p->bad_object_sha1 + GIT_SHA1_RAWSZ * i)) + if (hasheq(sha1, p->bad_object_sha1 + GIT_SHA1_RAWSZ * i)) return; p->bad_object_sha1 = xrealloc(p->bad_object_sha1, st_mult(GIT_MAX_RAWSZ, @@ -1031,8 +1031,8 @@ const struct packed_git *has_packed_and_bad(const unsigned char *sha1) for (p = the_repository->objects->packed_git; p; p = p->next) for (i = 0; i < p->num_bad_objects; i++) - if (!hashcmp(sha1, - p->bad_object_sha1 + the_hash_algo->rawsz * i)) + if (hasheq(sha1, + p->bad_object_sha1 + the_hash_algo->rawsz * i)) return p; return NULL; } @@ -1830,8 +1830,8 @@ static int fill_pack_entry(const struct object_id *oid, if (p->num_bad_objects) { unsigned i; for (i = 0; i < p->num_bad_objects; i++) - if (!hashcmp(oid->hash, - p->bad_object_sha1 + the_hash_algo->rawsz * i)) + if (hasheq(oid->hash, + p->bad_object_sha1 + the_hash_algo->rawsz * i)) return 0; } -- 2.19.0.rc0.584.g84d5b2a847 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 5/9] convert "oidcmp() != 0" to "!oideq()" 2018-08-28 21:21 ` Jeff King ` (3 preceding siblings ...) 2018-08-28 21:22 ` [PATCH v2 4/9] convert "hashcmp() == 0" to hasheq() Jeff King @ 2018-08-28 21:22 ` Jeff King 2018-08-28 21:22 ` [PATCH v2 6/9] convert "hashcmp() != 0" to "!hasheq()" Jeff King ` (5 subsequent siblings) 10 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2018-08-28 21:22 UTC (permalink / raw) To: brian m. carlson, git, Derrick Stolee, Jacob Keller This is the flip side of the previous two patches: checking for a non-zero oidcmp() can be more strictly expressed as inequality. Like those patches, we write "!= 0" in the coccinelle transformation, which covers by isomorphism the more common: if (oidcmp(E1, E2)) As with the previous two patches, this patch can be achieved almost entirely by running "make coccicheck"; the only differences are manual line-wrap fixes to match the original code. There is one thing to note for anybody replicating this, though: coccinelle 1.0.4 seems to miss the case in builtin/tag.c, even though it's basically the same as all the others. Running with 1.0.7 does catch this, so presumably it's just a coccinelle bug that was fixed in the interim. Signed-off-by: Jeff King <peff@peff.net> --- bisect.c | 2 +- blame.c | 4 ++-- builtin/fmt-merge-msg.c | 4 ++-- builtin/merge.c | 2 +- builtin/pull.c | 2 +- builtin/rm.c | 2 +- builtin/show-branch.c | 4 ++-- builtin/tag.c | 2 +- bundle.c | 2 +- commit-graph.c | 6 +++--- commit.c | 2 +- contrib/coccinelle/object_id.cocci | 6 ++++++ diff-lib.c | 2 +- diff.c | 6 +++--- diffcore-rename.c | 2 +- dir.c | 6 +++--- fast-import.c | 4 ++-- fetch-pack.c | 2 +- match-trees.c | 2 +- notes.c | 2 +- read-cache.c | 2 +- refs.c | 8 ++++---- refs/files-backend.c | 2 +- refs/packed-backend.c | 2 +- refs/ref-cache.c | 2 +- remote.c | 2 +- sequencer.c | 8 ++++---- sha1-file.c | 6 +++--- submodule-config.c | 4 ++-- t/helper/test-dump-cache-tree.c | 2 +- tree-diff.c | 2 +- 31 files changed, 55 insertions(+), 49 deletions(-) diff --git a/bisect.c b/bisect.c index 41c56a665e..7c1d8f1a6d 100644 --- a/bisect.c +++ b/bisect.c @@ -595,7 +595,7 @@ static struct commit_list *skip_away(struct commit_list *list, int count) for (i = 0; cur; cur = cur->next, i++) { if (i == index) { - if (oidcmp(&cur->item->object.oid, current_bad_oid)) + if (!oideq(&cur->item->object.oid, current_bad_oid)) return cur; if (previous) return previous; diff --git a/blame.c b/blame.c index c47d7050d9..d5f7b7237c 100644 --- a/blame.c +++ b/blame.c @@ -1832,7 +1832,7 @@ void setup_scoreboard(struct blame_scoreboard *sb, sb->revs->children.name = "children"; while (c->parents && - oidcmp(&c->object.oid, &sb->final->object.oid)) { + !oideq(&c->object.oid, &sb->final->object.oid)) { struct commit_list *l = xcalloc(1, sizeof(*l)); l->item = c; @@ -1842,7 +1842,7 @@ void setup_scoreboard(struct blame_scoreboard *sb, c = c->parents->item; } - if (oidcmp(&c->object.oid, &sb->final->object.oid)) + if (!oideq(&c->object.oid, &sb->final->object.oid)) die(_("--reverse --first-parent together require range along first-parent chain")); } diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c index 4c82c234cb..268f0c20ca 100644 --- a/builtin/fmt-merge-msg.c +++ b/builtin/fmt-merge-msg.c @@ -78,9 +78,9 @@ static struct merge_parent *find_merge_parent(struct merge_parents *table, { int i; for (i = 0; i < table->nr; i++) { - if (given && oidcmp(&table->item[i].given, given)) + if (given && !oideq(&table->item[i].given, given)) continue; - if (commit && oidcmp(&table->item[i].commit, commit)) + if (commit && !oideq(&table->item[i].commit, commit)) continue; return &table->item[i]; } diff --git a/builtin/merge.c b/builtin/merge.c index 57abff999b..8d85d31a61 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1521,7 +1521,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) * HEAD^^" would be missed. */ common_one = get_merge_bases(head_commit, j->item); - if (oidcmp(&common_one->item->object.oid, &j->item->object.oid)) { + if (!oideq(&common_one->item->object.oid, &j->item->object.oid)) { up_to_date = 0; break; } diff --git a/builtin/pull.c b/builtin/pull.c index a720f58a96..09b02695de 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -902,7 +902,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) oidclr(&curr_head); if (!is_null_oid(&orig_head) && !is_null_oid(&curr_head) && - oidcmp(&orig_head, &curr_head)) { + !oideq(&orig_head, &curr_head)) { /* * The fetch involved updating the current branch. * diff --git a/builtin/rm.c b/builtin/rm.c index 2cbe89e0ae..17086d3d97 100644 --- a/builtin/rm.c +++ b/builtin/rm.c @@ -180,7 +180,7 @@ static int check_local_mod(struct object_id *head, int index_only) if (no_head || get_tree_entry(head, name, &oid, &mode) || ce->ce_mode != create_ce_mode(mode) - || oidcmp(&ce->oid, &oid)) + || !oideq(&ce->oid, &oid)) staged_changes = 1; /* diff --git a/builtin/show-branch.c b/builtin/show-branch.c index 363cf8509a..5f9432861b 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -412,7 +412,7 @@ static int append_head_ref(const char *refname, const struct object_id *oid, /* If both heads/foo and tags/foo exists, get_sha1 would * get confused. */ - if (get_oid(refname + ofs, &tmp) || oidcmp(&tmp, oid)) + if (get_oid(refname + ofs, &tmp) || !oideq(&tmp, oid)) ofs = 5; return append_ref(refname + ofs, oid, 0); } @@ -427,7 +427,7 @@ static int append_remote_ref(const char *refname, const struct object_id *oid, /* If both heads/foo and tags/foo exists, get_sha1 would * get confused. */ - if (get_oid(refname + ofs, &tmp) || oidcmp(&tmp, oid)) + if (get_oid(refname + ofs, &tmp) || !oideq(&tmp, oid)) ofs = 5; return append_ref(refname + ofs, oid, 0); } diff --git a/builtin/tag.c b/builtin/tag.c index 9a19ffb49f..f623632186 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -559,7 +559,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) ref_transaction_commit(transaction, &err)) die("%s", err.buf); ref_transaction_free(transaction); - if (force && !is_null_oid(&prev) && oidcmp(&prev, &object)) + if (force && !is_null_oid(&prev) && !oideq(&prev, &object)) printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(&prev, DEFAULT_ABBREV)); diff --git a/bundle.c b/bundle.c index 24cbe40986..14f2cfc248 100644 --- a/bundle.c +++ b/bundle.c @@ -369,7 +369,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs) * commit that is referenced by the tag, and not the tag * itself. */ - if (oidcmp(&oid, &e->item->oid)) { + if (!oideq(&oid, &e->item->oid)) { /* * Is this the positive end of a range expressed * in terms of a tag (e.g. v2.0 from the range diff --git a/commit-graph.c b/commit-graph.c index 050c516b0d..7aed9f5371 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -765,7 +765,7 @@ void write_commit_graph(const char *obj_dir, count_distinct = 1; for (i = 1; i < oids.nr; i++) { - if (oidcmp(&oids.list[i-1], &oids.list[i])) + if (!oideq(&oids.list[i - 1], &oids.list[i])) count_distinct++; } @@ -960,7 +960,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g) continue; } - if (oidcmp(&get_commit_tree_in_graph_one(g, graph_commit)->object.oid, + if (!oideq(&get_commit_tree_in_graph_one(g, graph_commit)->object.oid, get_commit_tree_oid(odb_commit))) graph_report("root tree OID for commit %s in commit-graph is %s != %s", oid_to_hex(&cur_oid), @@ -977,7 +977,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g) break; } - if (oidcmp(&graph_parents->item->object.oid, &odb_parents->item->object.oid)) + if (!oideq(&graph_parents->item->object.oid, &odb_parents->item->object.oid)) graph_report("commit-graph parent for %s is %s != %s", oid_to_hex(&cur_oid), oid_to_hex(&graph_parents->item->object.oid), diff --git a/commit.c b/commit.c index 1a6e632185..1b94a8c96f 100644 --- a/commit.c +++ b/commit.c @@ -46,7 +46,7 @@ struct commit *lookup_commit_or_die(const struct object_id *oid, const char *ref struct commit *c = lookup_commit_reference(the_repository, oid); if (!c) die(_("could not parse %s"), ref_name); - if (oidcmp(oid, &c->object.oid)) { + if (!oideq(oid, &c->object.oid)) { warning(_("%s %s is not a commit!"), ref_name, oid_to_hex(oid)); } diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci index d90ba8a040..4e1f1a7431 100644 --- a/contrib/coccinelle/object_id.cocci +++ b/contrib/coccinelle/object_id.cocci @@ -123,3 +123,9 @@ expression E1, E2; - hashcmp(E1, E2) == 0 + hasheq(E1, E2) ...>} + +@@ +expression E1, E2; +@@ +- oidcmp(E1, E2) != 0 ++ !oideq(E1, E2) diff --git a/diff-lib.c b/diff-lib.c index c1f5a52654..8866b1d60c 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -342,7 +342,7 @@ static int show_modified(struct rev_info *revs, } if (revs->combine_merges && !cached && - (oidcmp(oid, &old_entry->oid) || oidcmp(&old_entry->oid, &new_entry->oid))) { + (!oideq(oid, &old_entry->oid) || !oideq(&old_entry->oid, &new_entry->oid))) { struct combine_diff_path *p; int pathlen = ce_namelen(new_entry); diff --git a/diff.c b/diff.c index 5cada68267..5d3219b600 100644 --- a/diff.c +++ b/diff.c @@ -3765,7 +3765,7 @@ static int reuse_worktree_file(const char *name, const struct object_id *oid, in * This is not the sha1 we are looking for, or * unreusable because it is not a regular file. */ - if (oidcmp(oid, &ce->oid) || !S_ISREG(ce->ce_mode)) + if (!oideq(oid, &ce->oid) || !S_ISREG(ce->ce_mode)) return 0; /* @@ -4170,7 +4170,7 @@ static void fill_metainfo(struct strbuf *msg, default: *must_show_header = 0; } - if (one && two && oidcmp(&one->oid, &two->oid)) { + if (one && two && !oideq(&one->oid, &two->oid)) { const unsigned hexsz = the_hash_algo->hexsz; int abbrev = o->flags.full_index ? hexsz : DEFAULT_ABBREV; @@ -5457,7 +5457,7 @@ static void diff_resolve_rename_copy(void) else p->status = DIFF_STATUS_RENAMED; } - else if (oidcmp(&p->one->oid, &p->two->oid) || + else if (!oideq(&p->one->oid, &p->two->oid) || p->one->mode != p->two->mode || p->one->dirty_submodule || p->two->dirty_submodule || diff --git a/diffcore-rename.c b/diffcore-rename.c index d775183c2f..daddd9b28a 100644 --- a/diffcore-rename.c +++ b/diffcore-rename.c @@ -286,7 +286,7 @@ static int find_identical_files(struct hashmap *srcs, struct diff_filespec *source = p->filespec; /* False hash collision? */ - if (oidcmp(&source->oid, &target->oid)) + if (!oideq(&source->oid, &target->oid)) continue; /* Non-regular files? If so, the modes must match! */ if (!S_ISREG(source->mode) || !S_ISREG(target->mode)) { diff --git a/dir.c b/dir.c index aceb0d4869..8ee9fe81b4 100644 --- a/dir.c +++ b/dir.c @@ -1282,7 +1282,7 @@ static void prep_exclude(struct dir_struct *dir, * order, though, if you do that. */ if (untracked && - oidcmp(&oid_stat.oid, &untracked->exclude_oid)) { + !oideq(&oid_stat.oid, &untracked->exclude_oid)) { invalidate_gitignore(dir->untracked, untracked); oidcpy(&untracked->exclude_oid, &oid_stat.oid); } @@ -2248,12 +2248,12 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d /* Validate $GIT_DIR/info/exclude and core.excludesfile */ root = dir->untracked->root; - if (oidcmp(&dir->ss_info_exclude.oid, + if (!oideq(&dir->ss_info_exclude.oid, &dir->untracked->ss_info_exclude.oid)) { invalidate_gitignore(dir->untracked, root); dir->untracked->ss_info_exclude = dir->ss_info_exclude; } - if (oidcmp(&dir->ss_excludes_file.oid, + if (!oideq(&dir->ss_excludes_file.oid, &dir->untracked->ss_excludes_file.oid)) { invalidate_gitignore(dir->untracked, root); dir->untracked->ss_excludes_file = dir->ss_excludes_file; diff --git a/fast-import.c b/fast-import.c index a731088f96..67a53b79cb 100644 --- a/fast-import.c +++ b/fast-import.c @@ -2649,7 +2649,7 @@ static int parse_from(struct branch *b) struct object_entry *oe = find_mark(idnum); if (oe->type != OBJ_COMMIT) die("Mark :%" PRIuMAX " not a commit", idnum); - if (oidcmp(&b->oid, &oe->idx.oid)) { + if (!oideq(&b->oid, &oe->idx.oid)) { oidcpy(&b->oid, &oe->idx.oid); if (oe->pack_id != MAX_PACK_ID) { unsigned long size; @@ -2667,7 +2667,7 @@ static int parse_from(struct branch *b) else die("Invalid ref name or SHA1 expression: %s", from); - if (b->branch_tree.tree && oidcmp(&oid, &b->branch_tree.versions[1].oid)) { + if (b->branch_tree.tree && !oideq(&oid, &b->branch_tree.versions[1].oid)) { release_tree_content_recursive(b->branch_tree.tree); b->branch_tree.tree = NULL; } diff --git a/fetch-pack.c b/fetch-pack.c index 88a078e9be..75047a4b2a 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -599,7 +599,7 @@ static void filter_refs(struct fetch_pack_args *args, continue; if (parse_oid_hex(ref->name, &oid, &p) || *p != '\0' || - oidcmp(&oid, &ref->old_oid)) + !oideq(&oid, &ref->old_oid)) continue; if ((allow_unadvertised_object_request & diff --git a/match-trees.c b/match-trees.c index 37653308d3..2b6d31ef9d 100644 --- a/match-trees.c +++ b/match-trees.c @@ -106,7 +106,7 @@ static int score_trees(const struct object_id *hash1, const struct object_id *ha update_tree_entry(&two); } else { /* path appears in both */ - if (oidcmp(one.entry.oid, two.entry.oid)) { + if (!oideq(one.entry.oid, two.entry.oid)) { /* they are different */ score += score_differs(one.entry.mode, two.entry.mode, diff --git a/notes.c b/notes.c index 33d16c1ec3..25cdce28b7 100644 --- a/notes.c +++ b/notes.c @@ -206,7 +206,7 @@ static void note_tree_remove(struct notes_tree *t, if (GET_PTR_TYPE(*p) != PTR_TYPE_NOTE) return; /* type mismatch, nothing to remove */ l = (struct leaf_node *) CLR_PTR_TYPE(*p); - if (oidcmp(&l->key_oid, &entry->key_oid)) + if (!oideq(&l->key_oid, &entry->key_oid)) return; /* key mismatch, nothing to remove */ /* we have found a matching entry */ diff --git a/read-cache.c b/read-cache.c index eb4919e77f..88bb80326b 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2029,7 +2029,7 @@ int read_index_from(struct index_state *istate, const char *path, base_oid_hex = oid_to_hex(&split_index->base_oid); base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_oid_hex); ret = do_read_index(split_index->base, base_path, 1); - if (oidcmp(&split_index->base_oid, &split_index->base->oid)) + if (!oideq(&split_index->base_oid, &split_index->base->oid)) die("broken index, expect %s in %s, got %s", base_oid_hex, base_path, oid_to_hex(&split_index->base->oid)); diff --git a/refs.c b/refs.c index de81c7be7c..a7a75b4cc0 100644 --- a/refs.c +++ b/refs.c @@ -702,7 +702,7 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid, pseudoref); rollback_lock_file(&lock); goto done; - } else if (oidcmp(&actual_old_oid, old_oid)) { + } else if (!oideq(&actual_old_oid, old_oid)) { strbuf_addf(err, _("unexpected object ID when writing '%s'"), pseudoref); rollback_lock_file(&lock); @@ -744,7 +744,7 @@ static int delete_pseudoref(const char *pseudoref, const struct object_id *old_o } if (read_ref(pseudoref, &actual_old_oid)) die(_("could not read ref '%s'"), pseudoref); - if (oidcmp(&actual_old_oid, old_oid)) { + if (!oideq(&actual_old_oid, old_oid)) { error(_("unexpected object ID when deleting '%s'"), pseudoref); rollback_lock_file(&lock); @@ -875,13 +875,13 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid, */ if (!is_null_oid(&cb->ooid)) { oidcpy(cb->oid, noid); - if (oidcmp(&cb->ooid, noid)) + if (!oideq(&cb->ooid, noid)) warning(_("log for ref %s has gap after %s"), cb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822))); } else if (cb->date == cb->at_time) oidcpy(cb->oid, noid); - else if (oidcmp(noid, cb->oid)) + else if (!oideq(noid, cb->oid)) warning(_("log for ref %s unexpectedly ended on %s"), cb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822))); diff --git a/refs/files-backend.c b/refs/files-backend.c index aa45f5317f..16ef9325e0 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -841,7 +841,7 @@ static int verify_lock(struct ref_store *ref_store, struct ref_lock *lock, return 0; } } - if (old_oid && oidcmp(&lock->old_oid, old_oid)) { + if (old_oid && !oideq(&lock->old_oid, old_oid)) { strbuf_addf(err, "ref '%s' is at %s but expected %s", lock->ref_name, oid_to_hex(&lock->old_oid), diff --git a/refs/packed-backend.c b/refs/packed-backend.c index d447a731da..74e2996e93 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1160,7 +1160,7 @@ static int write_with_updates(struct packed_ref_store *refs, "reference already exists", update->refname); goto error; - } else if (oidcmp(&update->old_oid, iter->oid)) { + } else if (!oideq(&update->old_oid, iter->oid)) { strbuf_addf(err, "cannot update ref '%s': " "is at %s but expected %s", update->refname, diff --git a/refs/ref-cache.c b/refs/ref-cache.c index 9b110c8494..b7052f72e2 100644 --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -272,7 +272,7 @@ static int is_dup_ref(const struct ref_entry *ref1, const struct ref_entry *ref2 /* This is impossible by construction */ die("Reference directory conflict: %s", ref1->name); - if (oidcmp(&ref1->u.value.oid, &ref2->u.value.oid)) + if (!oideq(&ref1->u.value.oid, &ref2->u.value.oid)) die("Duplicated ref, and SHA1s don't match: %s", ref1->name); warning("Duplicated ref: %s", ref1->name); diff --git a/remote.c b/remote.c index 6f1ee9d640..e23b7675c8 100644 --- a/remote.c +++ b/remote.c @@ -1403,7 +1403,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror, * branch. */ if (ref->expect_old_sha1) { - if (oidcmp(&ref->old_oid, &ref->old_oid_expect)) + if (!oideq(&ref->old_oid, &ref->old_oid_expect)) reject_reason = REF_STATUS_REJECT_STALE; else /* If the ref isn't stale then force the update. */ diff --git a/sequencer.c b/sequencer.c index 5f823c9ec7..b14e5dc13c 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1188,7 +1188,7 @@ static int parse_head(struct commit **head) current_head = lookup_commit_reference(the_repository, &oid); if (!current_head) return error(_("could not parse HEAD")); - if (oidcmp(&oid, ¤t_head->object.oid)) { + if (!oideq(&oid, ¤t_head->object.oid)) { warning(_("HEAD %s is not a commit!"), oid_to_hex(&oid)); } @@ -3034,7 +3034,7 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len, struct commit_list *p = commit->parents->next; for (j = to_merge; j && p; j = j->next, p = p->next) - if (oidcmp(&j->item->object.oid, + if (!oideq(&j->item->object.oid, &p->item->object.oid)) { can_fast_forward = 0; break; @@ -3566,7 +3566,7 @@ static int commit_staged_changes(struct replay_opts *opts, if (get_oid_hex(rev.buf, &to_amend)) return error(_("invalid contents: '%s'"), rebase_path_amend()); - if (!is_clean && oidcmp(&head, &to_amend)) + if (!is_clean && !oideq(&head, &to_amend)) return error(_("\nYou have uncommitted changes in your " "working tree. Please, commit them\n" "first and then run 'git rebase " @@ -4545,7 +4545,7 @@ int skip_unnecessary_picks(void) if (item->commit->parents->next) break; /* merge commit */ parent_oid = &item->commit->parents->item->object.oid; - if (oidcmp(parent_oid, oid)) + if (!oideq(parent_oid, oid)) break; oid = &item->commit->object.oid; } diff --git a/sha1-file.c b/sha1-file.c index 71f49ee56d..cc8a196349 100644 --- a/sha1-file.c +++ b/sha1-file.c @@ -825,7 +825,7 @@ int check_object_signature(const struct object_id *oid, void *map, if (map) { hash_object_file(map, size, type, &real_oid); - return oidcmp(oid, &real_oid) ? -1 : 0; + return !oideq(oid, &real_oid) ? -1 : 0; } st = open_istream(oid, &obj_type, &size, NULL); @@ -852,7 +852,7 @@ int check_object_signature(const struct object_id *oid, void *map, } the_hash_algo->final_fn(real_oid.hash, &c); close_istream(st); - return oidcmp(oid, &real_oid) ? -1 : 0; + return !oideq(oid, &real_oid) ? -1 : 0; } int git_open_cloexec(const char *name, int flags) @@ -1671,7 +1671,7 @@ static int write_loose_object(const struct object_id *oid, char *hdr, die(_("deflateEnd on object %s failed (%d)"), oid_to_hex(oid), ret); the_hash_algo->final_fn(parano_oid.hash, &c); - if (oidcmp(oid, ¶no_oid) != 0) + if (!oideq(oid, ¶no_oid)) die(_("confused by unstable object source data for %s"), oid_to_hex(oid)); diff --git a/submodule-config.c b/submodule-config.c index fc2c41b947..e04ba756d9 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -45,7 +45,7 @@ static int config_path_cmp(const void *unused_cmp_data, const struct submodule_entry *b = entry_or_key; return strcmp(a->config->path, b->config->path) || - oidcmp(&a->config->gitmodules_oid, &b->config->gitmodules_oid); + !oideq(&a->config->gitmodules_oid, &b->config->gitmodules_oid); } static int config_name_cmp(const void *unused_cmp_data, @@ -57,7 +57,7 @@ static int config_name_cmp(const void *unused_cmp_data, const struct submodule_entry *b = entry_or_key; return strcmp(a->config->name, b->config->name) || - oidcmp(&a->config->gitmodules_oid, &b->config->gitmodules_oid); + !oideq(&a->config->gitmodules_oid, &b->config->gitmodules_oid); } static struct submodule_cache *submodule_cache_alloc(void) diff --git a/t/helper/test-dump-cache-tree.c b/t/helper/test-dump-cache-tree.c index 98a4891f1d..6a3f88f5f5 100644 --- a/t/helper/test-dump-cache-tree.c +++ b/t/helper/test-dump-cache-tree.c @@ -33,7 +33,7 @@ static int dump_cache_tree(struct cache_tree *it, } else { dump_one(it, pfx, ""); - if (oidcmp(&it->oid, &ref->oid) || + if (!oideq(&it->oid, &ref->oid) || ref->entry_count != it->entry_count || ref->subtree_nr != it->subtree_nr) { /* claims to be valid but is lying */ diff --git a/tree-diff.c b/tree-diff.c index 553bc0e63a..425668e1e0 100644 --- a/tree-diff.c +++ b/tree-diff.c @@ -491,7 +491,7 @@ static struct combine_diff_path *ll_diff_tree_paths( continue; /* diff(t,pi) != ø */ - if (oidcmp(t.entry.oid, tp[i].entry.oid) || + if (!oideq(t.entry.oid, tp[i].entry.oid) || (t.entry.mode != tp[i].entry.mode)) continue; -- 2.19.0.rc0.584.g84d5b2a847 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 6/9] convert "hashcmp() != 0" to "!hasheq()" 2018-08-28 21:21 ` Jeff King ` (4 preceding siblings ...) 2018-08-28 21:22 ` [PATCH v2 5/9] convert "oidcmp() != 0" to "!oideq()" Jeff King @ 2018-08-28 21:22 ` Jeff King 2018-08-28 21:22 ` [PATCH v2 7/9] convert hashmap comparison functions to oideq() Jeff King ` (4 subsequent siblings) 10 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2018-08-28 21:22 UTC (permalink / raw) To: brian m. carlson, git, Derrick Stolee, Jacob Keller This rounds out the previous three patches, covering the inequality logic for the "hash" variant of the functions. As with the previous three, the accompanying code changes are the mechanical result of applying the coccinelle patch; see those patches for more discussion. Signed-off-by: Jeff King <peff@peff.net> --- builtin/index-pack.c | 4 ++-- builtin/show-branch.c | 2 +- builtin/unpack-objects.c | 2 +- commit-graph.c | 2 +- contrib/coccinelle/object_id.cocci | 9 +++++++++ http-walker.c | 2 +- http.c | 2 +- pack-check.c | 6 +++--- pack-write.c | 2 +- packfile.c | 2 +- read-cache.c | 4 ++-- sha1-file.c | 2 +- 12 files changed, 24 insertions(+), 15 deletions(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index edcb0a3dca..2004e25da2 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1166,7 +1166,7 @@ static void parse_pack_objects(unsigned char *hash) /* Check pack integrity */ flush(); the_hash_algo->final_fn(hash, &input_ctx); - if (hashcmp(fill(the_hash_algo->rawsz), hash)) + if (!hasheq(fill(the_hash_algo->rawsz), hash)) die(_("pack is corrupted (SHA1 mismatch)")); use(the_hash_algo->rawsz); @@ -1280,7 +1280,7 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha fixup_pack_header_footer(output_fd, pack_hash, curr_pack, nr_objects, read_hash, consumed_bytes-the_hash_algo->rawsz); - if (hashcmp(read_hash, tail_hash) != 0) + if (!hasheq(read_hash, tail_hash)) die(_("Unexpected tail checksum for %s " "(disk corruption?)"), curr_pack); } diff --git a/builtin/show-branch.c b/builtin/show-branch.c index 5f9432861b..65f4a4c83c 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -485,7 +485,7 @@ static void snarf_refs(int head, int remotes) static int rev_is_head(const char *head, const char *name, unsigned char *head_sha1, unsigned char *sha1) { - if (!head || (head_sha1 && sha1 && hashcmp(head_sha1, sha1))) + if (!head || (head_sha1 && sha1 && !hasheq(head_sha1, sha1))) return 0; skip_prefix(head, "refs/heads/", &head); if (!skip_prefix(name, "refs/heads/", &name)) diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c index ad438f5b41..80478808b3 100644 --- a/builtin/unpack-objects.c +++ b/builtin/unpack-objects.c @@ -579,7 +579,7 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix) if (fsck_finish(&fsck_options)) die(_("fsck error in pack objects")); } - if (hashcmp(fill(the_hash_algo->rawsz), oid.hash)) + if (!hasheq(fill(the_hash_algo->rawsz), oid.hash)) die("final sha1 did not match"); use(the_hash_algo->rawsz); diff --git a/commit-graph.c b/commit-graph.c index 7aed9f5371..64ce79420d 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -900,7 +900,7 @@ int verify_commit_graph(struct repository *r, struct commit_graph *g) f = hashfd(devnull, NULL); hashwrite(f, g->data, g->data_len - g->hash_len); finalize_hashfile(f, checksum.hash, CSUM_CLOSE); - if (hashcmp(checksum.hash, g->data + g->data_len - g->hash_len)) { + if (!hasheq(checksum.hash, g->data + g->data_len - g->hash_len)) { graph_report(_("the commit-graph file has incorrect checksum and is likely corrupt")); verify_commit_graph_error = VERIFY_COMMIT_GRAPH_ERROR_HASH; } diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci index 4e1f1a7431..d8bdb48712 100644 --- a/contrib/coccinelle/object_id.cocci +++ b/contrib/coccinelle/object_id.cocci @@ -129,3 +129,12 @@ expression E1, E2; @@ - oidcmp(E1, E2) != 0 + !oideq(E1, E2) + +@@ +identifier f != hasheq; +expression E1, E2; +@@ + f(...) {<... +- hashcmp(E1, E2) != 0 ++ !hasheq(E1, E2) + ...>} diff --git a/http-walker.c b/http-walker.c index 3a8edc7f2f..b3334bf657 100644 --- a/http-walker.c +++ b/http-walker.c @@ -543,7 +543,7 @@ static int fetch_object(struct walker *walker, unsigned char *sha1) } else if (req->zret != Z_STREAM_END) { walker->corrupt_object_found++; ret = error("File %s (%s) corrupt", hex, req->url); - } else if (hashcmp(obj_req->oid.hash, req->real_sha1)) { + } else if (!hasheq(obj_req->oid.hash, req->real_sha1)) { ret = error("File %s has bad hash", hex); } else if (req->rename < 0) { struct strbuf buf = STRBUF_INIT; diff --git a/http.c b/http.c index 4162860ee3..98ff122585 100644 --- a/http.c +++ b/http.c @@ -2394,7 +2394,7 @@ int finish_http_object_request(struct http_object_request *freq) unlink_or_warn(freq->tmpfile.buf); return -1; } - if (hashcmp(freq->sha1, freq->real_sha1)) { + if (!hasheq(freq->sha1, freq->real_sha1)) { unlink_or_warn(freq->tmpfile.buf); return -1; } diff --git a/pack-check.c b/pack-check.c index d3a57df34f..fa5f0ff8fa 100644 --- a/pack-check.c +++ b/pack-check.c @@ -79,10 +79,10 @@ static int verify_packfile(struct packed_git *p, } while (offset < pack_sig_ofs); the_hash_algo->final_fn(hash, &ctx); pack_sig = use_pack(p, w_curs, pack_sig_ofs, NULL); - if (hashcmp(hash, pack_sig)) + if (!hasheq(hash, pack_sig)) err = error("%s pack checksum mismatch", p->pack_name); - if (hashcmp(index_base + index_size - the_hash_algo->hexsz, pack_sig)) + if (!hasheq(index_base + index_size - the_hash_algo->hexsz, pack_sig)) err = error("%s pack checksum does not match its index", p->pack_name); unuse_pack(w_curs); @@ -180,7 +180,7 @@ int verify_pack_index(struct packed_git *p) the_hash_algo->init_fn(&ctx); the_hash_algo->update_fn(&ctx, index_base, (unsigned int)(index_size - the_hash_algo->rawsz)); the_hash_algo->final_fn(hash, &ctx); - if (hashcmp(hash, index_base + index_size - the_hash_algo->rawsz)) + if (!hasheq(hash, index_base + index_size - the_hash_algo->rawsz)) err = error("Packfile index for %s hash mismatch", p->pack_name); return err; diff --git a/pack-write.c b/pack-write.c index 7d14716c40..29d17a9bec 100644 --- a/pack-write.c +++ b/pack-write.c @@ -260,7 +260,7 @@ void fixup_pack_header_footer(int pack_fd, if (partial_pack_offset == 0) { unsigned char hash[GIT_MAX_RAWSZ]; the_hash_algo->final_fn(hash, &old_hash_ctx); - if (hashcmp(hash, partial_pack_hash) != 0) + if (!hasheq(hash, partial_pack_hash)) die("Unexpected checksum for %s " "(disk corruption?)", pack_name); /* diff --git a/packfile.c b/packfile.c index c2e96293ad..1e9eacd9b3 100644 --- a/packfile.c +++ b/packfile.c @@ -517,7 +517,7 @@ static int open_packed_git_1(struct packed_git *p) if (read_result != hashsz) return error("packfile %s signature is unavailable", p->pack_name); idx_hash = ((unsigned char *)p->index_data) + p->index_size - hashsz * 2; - if (hashcmp(hash, idx_hash)) + if (!hasheq(hash, idx_hash)) return error("packfile %s does not match index", p->pack_name); return 0; } diff --git a/read-cache.c b/read-cache.c index 88bb80326b..421a7f4953 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1668,7 +1668,7 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size) 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 (hashcmp(hash, (unsigned char *)hdr + size - the_hash_algo->rawsz)) + if (!hasheq(hash, (unsigned char *)hdr + size - the_hash_algo->rawsz)) return error("bad index file sha1 signature"); return 0; } @@ -2395,7 +2395,7 @@ static int verify_index_from(const struct index_state *istate, const char *path) if (n != the_hash_algo->rawsz) goto out; - if (hashcmp(istate->oid.hash, hash)) + if (!hasheq(istate->oid.hash, hash)) goto out; close(fd); diff --git a/sha1-file.c b/sha1-file.c index cc8a196349..d85f4e93e1 100644 --- a/sha1-file.c +++ b/sha1-file.c @@ -2213,7 +2213,7 @@ static int check_stream_sha1(git_zstream *stream, } the_hash_algo->final_fn(real_sha1, &c); - if (hashcmp(expected_sha1, real_sha1)) { + if (!hasheq(expected_sha1, real_sha1)) { error(_("sha1 mismatch for %s (expected %s)"), path, sha1_to_hex(expected_sha1)); return -1; -- 2.19.0.rc0.584.g84d5b2a847 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 7/9] convert hashmap comparison functions to oideq() 2018-08-28 21:21 ` Jeff King ` (5 preceding siblings ...) 2018-08-28 21:22 ` [PATCH v2 6/9] convert "hashcmp() != 0" to "!hasheq()" Jeff King @ 2018-08-28 21:22 ` Jeff King 2018-08-28 21:22 ` [PATCH v2 8/9] read-cache: use oideq() in ce_compare functions Jeff King ` (3 subsequent siblings) 10 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2018-08-28 21:22 UTC (permalink / raw) To: brian m. carlson, git, Derrick Stolee, Jacob Keller The comparison functions used for hashmaps don't care about strict ordering; they only want to compare entries for equality. Let's use the oideq() function instead, which can potentially be better optimized. Note that unlike the previous patches mass-converting calls like "!oidcmp()", this patch could actually provide an improvement even with the current implementation. Those comparison functions are passed around as function pointers, so at compile-time the compiler cannot realize that the caller (which is in another file completely) will treat the return value as a boolean. Note that this does change the return values in quite a subtle way (it's still an int, but now the sign bit is irrelevant for ordering). Because of their funny hashmap-specific signature, it's unlikely that any of these static functions would be reused for more generic ordering. But to be double-sure, let's stop using "cmp" in their names. Calling them "eq" doesn't quite work either, because the hashmap convention is actually _inverted_. "0" means "same", and non-zero means "different". So I've called them "neq" by convention here. Signed-off-by: Jeff King <peff@peff.net> --- builtin/describe.c | 6 +++--- oidmap.c | 12 ++++++------ patch-ids.c | 8 ++++---- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/builtin/describe.c b/builtin/describe.c index 1e7c75ba82..22c0541da5 100644 --- a/builtin/describe.c +++ b/builtin/describe.c @@ -62,7 +62,7 @@ static const char *prio_names[] = { N_("head"), N_("lightweight"), N_("annotated"), }; -static int commit_name_cmp(const void *unused_cmp_data, +static int commit_name_neq(const void *unused_cmp_data, const void *entry, const void *entry_or_key, const void *peeled) @@ -70,7 +70,7 @@ static int commit_name_cmp(const void *unused_cmp_data, const struct commit_name *cn1 = entry; const struct commit_name *cn2 = entry_or_key; - return oidcmp(&cn1->peeled, peeled ? peeled : &cn2->peeled); + return !oideq(&cn1->peeled, peeled ? peeled : &cn2->peeled); } static inline struct commit_name *find_commit_name(const struct object_id *peeled) @@ -596,7 +596,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix) return cmd_name_rev(args.argc, args.argv, prefix); } - hashmap_init(&names, commit_name_cmp, NULL, 0); + hashmap_init(&names, commit_name_neq, NULL, 0); for_each_rawref(get_name, NULL); if (!hashmap_get_size(&names) && !always) die(_("No names found, cannot describe anything.")); diff --git a/oidmap.c b/oidmap.c index d9fb19ba6a..b0841a0f58 100644 --- a/oidmap.c +++ b/oidmap.c @@ -1,14 +1,14 @@ #include "cache.h" #include "oidmap.h" -static int cmpfn(const void *hashmap_cmp_fn_data, - const void *entry, const void *entry_or_key, - const void *keydata) +static int oidmap_neq(const void *hashmap_cmp_fn_data, + const void *entry, const void *entry_or_key, + const void *keydata) { const struct oidmap_entry *entry_ = entry; if (keydata) - return oidcmp(&entry_->oid, (const struct object_id *) keydata); - return oidcmp(&entry_->oid, + return !oideq(&entry_->oid, (const struct object_id *) keydata); + return !oideq(&entry_->oid, &((const struct oidmap_entry *) entry_or_key)->oid); } @@ -21,7 +21,7 @@ static int hash(const struct object_id *oid) void oidmap_init(struct oidmap *map, size_t initial_size) { - hashmap_init(&map->map, cmpfn, NULL, initial_size); + hashmap_init(&map->map, oidmap_neq, NULL, initial_size); } void oidmap_free(struct oidmap *map, int free_entries) diff --git a/patch-ids.c b/patch-ids.c index 8f7c25d5db..960ea24054 100644 --- a/patch-ids.c +++ b/patch-ids.c @@ -28,14 +28,14 @@ int commit_patch_id(struct commit *commit, struct diff_options *options, /* * When we cannot load the full patch-id for both commits for whatever * reason, the function returns -1 (i.e. return error(...)). Despite - * the "cmp" in the name of this function, the caller only cares about + * the "neq" in the name of this function, the caller only cares about * the return value being zero (a and b are equivalent) or non-zero (a * and b are different), and returning non-zero would keep both in the * result, even if they actually were equivalent, in order to err on * the side of safety. The actual value being negative does not have * any significance; only that it is non-zero matters. */ -static int patch_id_cmp(const void *cmpfn_data, +static int patch_id_neq(const void *cmpfn_data, const void *entry, const void *entry_or_key, const void *unused_keydata) @@ -53,7 +53,7 @@ static int patch_id_cmp(const void *cmpfn_data, commit_patch_id(b->commit, opt, &b->patch_id, 0)) return error("Could not get patch ID for %s", oid_to_hex(&b->commit->object.oid)); - return oidcmp(&a->patch_id, &b->patch_id); + return !oideq(&a->patch_id, &b->patch_id); } int init_patch_ids(struct patch_ids *ids) @@ -63,7 +63,7 @@ int init_patch_ids(struct patch_ids *ids) ids->diffopts.detect_rename = 0; ids->diffopts.flags.recursive = 1; diff_setup_done(&ids->diffopts); - hashmap_init(&ids->patches, patch_id_cmp, &ids->diffopts, 256); + hashmap_init(&ids->patches, patch_id_neq, &ids->diffopts, 256); return 0; } -- 2.19.0.rc0.584.g84d5b2a847 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 8/9] read-cache: use oideq() in ce_compare functions 2018-08-28 21:21 ` Jeff King ` (6 preceding siblings ...) 2018-08-28 21:22 ` [PATCH v2 7/9] convert hashmap comparison functions to oideq() Jeff King @ 2018-08-28 21:22 ` Jeff King 2018-08-28 21:23 ` [PATCH v2 9/9] show_dirstat: simplify same-content check Jeff King ` (2 subsequent siblings) 10 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2018-08-28 21:22 UTC (permalink / raw) To: brian m. carlson, git, Derrick Stolee, Jacob Keller These functions return the full oidcmp() value, but the callers really only care whether it is non-zero. We can use the more strict !oideq(), which a compiler may be able to optimize further. This does change the meaning of the return value subtly, but it's unlikely that anybody would try to use them for ordering. They're static-local in this file, and they already return other error values that would confuse an ordering (e.g., open() failure gives -1). Signed-off-by: Jeff King <peff@peff.net> --- read-cache.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/read-cache.c b/read-cache.c index 421a7f4953..eb7cea6272 100644 --- a/read-cache.c +++ b/read-cache.c @@ -213,7 +213,7 @@ static int ce_compare_data(const struct cache_entry *ce, struct stat *st) if (fd >= 0) { struct object_id oid; if (!index_fd(&oid, fd, st, OBJ_BLOB, ce->name, 0)) - match = oidcmp(&oid, &ce->oid); + match = !oideq(&oid, &ce->oid); /* index_fd() closed the file descriptor already */ } return match; @@ -254,7 +254,7 @@ static int ce_compare_gitlink(const struct cache_entry *ce) */ if (resolve_gitlink_ref(ce->name, "HEAD", &oid) < 0) return 0; - return oidcmp(&oid, &ce->oid); + return !oideq(&oid, &ce->oid); } static int ce_modified_check_fs(const struct cache_entry *ce, struct stat *st) -- 2.19.0.rc0.584.g84d5b2a847 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 9/9] show_dirstat: simplify same-content check 2018-08-28 21:21 ` Jeff King ` (7 preceding siblings ...) 2018-08-28 21:22 ` [PATCH v2 8/9] read-cache: use oideq() in ce_compare functions Jeff King @ 2018-08-28 21:23 ` Jeff King 2018-08-28 21:36 ` [PATCH 0/9] introducing oideq() Derrick Stolee 2018-08-29 0:08 ` brian m. carlson 10 siblings, 0 replies; 30+ messages in thread From: Jeff King @ 2018-08-28 21:23 UTC (permalink / raw) To: brian m. carlson, git, Derrick Stolee, Jacob Keller We use two nested conditionals to store a content_changed variable, but only bother to look at the result once, directly after we set it. We can drop the variable entirely and just use a single "if". This needless complexity is the result of 2ff3a80334 (Teach --dirstat not to completely ignore rearranged lines within a file, 2011-04-11). Before that, we held onto the content_changed variable much longer. While we're touching the condition, we can swap out oidcmp() for !oideq(). Our coccinelle patches didn't previously find this case because of the intermediate variable, but now it's a simple boolean in a conditional. Signed-off-by: Jeff King <peff@peff.net> --- diff.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/diff.c b/diff.c index 5d3219b600..605ba4b6b8 100644 --- a/diff.c +++ b/diff.c @@ -2933,16 +2933,11 @@ static void show_dirstat(struct diff_options *options) struct diff_filepair *p = q->queue[i]; const char *name; unsigned long copied, added, damage; - int content_changed; name = p->two->path ? p->two->path : p->one->path; - if (p->one->oid_valid && p->two->oid_valid) - content_changed = oidcmp(&p->one->oid, &p->two->oid); - else - content_changed = 1; - - if (!content_changed) { + if (p->one->oid_valid && p->two->oid_valid && + oideq(&p->one->oid, &p->two->oid)) { /* * The SHA1 has not changed, so pre-/post-content is * identical. We can therefore skip looking at the @@ -2989,7 +2984,7 @@ static void show_dirstat(struct diff_options *options) * made to the preimage. * If the resulting damage is zero, we know that * diffcore_count_changes() considers the two entries to - * be identical, but since content_changed is true, we + * be identical, but since the oid changed, we * know that there must have been _some_ kind of change, * so we force all entries to have damage > 0. */ -- 2.19.0.rc0.584.g84d5b2a847 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH 0/9] introducing oideq() 2018-08-28 21:21 ` Jeff King ` (8 preceding siblings ...) 2018-08-28 21:23 ` [PATCH v2 9/9] show_dirstat: simplify same-content check Jeff King @ 2018-08-28 21:36 ` Derrick Stolee 2018-08-29 0:08 ` brian m. carlson 10 siblings, 0 replies; 30+ messages in thread From: Derrick Stolee @ 2018-08-28 21:36 UTC (permalink / raw) To: Jeff King, brian m. carlson, git, Jacob Keller On 8/28/2018 5:21 PM, Jeff King wrote: > On Sun, Aug 26, 2018 at 08:56:21PM +0000, brian m. carlson wrote: > >>> Due to the simplicity of the current code and our inlining, the compiler >>> can usually figure this out for now. So I wouldn't expect this patch to >>> actually improve performance right away. But as that discussion shows, >>> we are likely to take a performance hit as we move to more runtime >>> determination of the_hash_algo parameters. Having these callers use the >>> more strict form will potentially help us recover that. >>> >>> So in that sense we _could_ simply punt on this series until then (and >>> it's certainly post-v2.19 material). But I think it's worth doing now, >>> simply from a readability/annotation standpoint. IMHO the resulting code >>> is more clear (though I've long since taught myself to read !foocmp() as >>> equality). >> I would quite like to see this series picked up for v2.20. If we want >> to minimize performance regressions with the SHA-256 work, I think it's >> important. > Thanks. One of the things I was worried about was causing unnecessary > conflicts with existing topics, including your work. But if everybody is > on board, I'd be happy to see this go in early in the next release cycle > (the longer we wait, the more annoying conflicts Junio has to resolve). I'm happy to take this change whenever. In my opinion, right after v2.19.0 is cut would be a great time to merge it into master. This v2 is good. Reviewed-by: Derrick Stolee <dstolee@microsoft.com> ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 0/9] introducing oideq() 2018-08-28 21:21 ` Jeff King ` (9 preceding siblings ...) 2018-08-28 21:36 ` [PATCH 0/9] introducing oideq() Derrick Stolee @ 2018-08-29 0:08 ` brian m. carlson 10 siblings, 0 replies; 30+ messages in thread From: brian m. carlson @ 2018-08-29 0:08 UTC (permalink / raw) To: Jeff King; +Cc: git, Derrick Stolee, Jacob Keller [-- Attachment #1: Type: text/plain, Size: 1004 bytes --] On Tue, Aug 28, 2018 at 05:21:27PM -0400, Jeff King wrote: > On Sun, Aug 26, 2018 at 08:56:21PM +0000, brian m. carlson wrote: > > I would quite like to see this series picked up for v2.20. If we want > > to minimize performance regressions with the SHA-256 work, I think it's > > important. > > Thanks. One of the things I was worried about was causing unnecessary > conflicts with existing topics, including your work. But if everybody is > on board, I'd be happy to see this go in early in the next release cycle > (the longer we wait, the more annoying conflicts Junio has to resolve). I can send out work that doesn't conflict with this while it makes its way to master. There are a lot of test fixes that can go in in the mean time. I will be sending out a series that actually implements SHA-256 as an RFC soon, but I don't think there will be any conflicts (and it will just be an RFC anyway). -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 867 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2018-08-29 0:08 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-25 8:00 [PATCH 0/9] introducing oideq() Jeff King 2018-08-25 8:03 ` [PATCH 1/9] coccinelle: use <...> for function exclusion Jeff King 2018-08-25 8:05 ` [PATCH 2/9] introduce hasheq() and oideq() Jeff King 2018-08-25 10:58 ` Andrei Rybak 2018-08-25 8:07 ` [PATCH 3/9] convert "oidcmp() == 0" to oideq() Jeff King 2018-08-25 8:36 ` Jeff King 2018-08-27 12:31 ` Derrick Stolee 2018-08-27 12:33 ` Derrick Stolee 2018-08-25 8:08 ` [PATCH 4/9] convert "hashcmp() == 0" to hasheq() Jeff King 2018-08-25 8:09 ` [PATCH 5/9] convert "oidcmp() != 0" to "!oideq()" Jeff King 2018-08-25 8:10 ` [PATCH 6/9] convert "hashcmp() != 0" to "!hasheq()" Jeff King 2018-08-25 8:14 ` [PATCH 7/9] convert hashmap comparison functions to oideq() Jeff King 2018-08-25 8:15 ` [PATCH 8/9] read-cache: use oideq() in ce_compare functions Jeff King 2018-08-25 8:17 ` [PATCH 9/9] show_dirstat: simplify same-content check Jeff King 2018-08-25 8:20 ` Eric Sunshine 2018-08-25 8:23 ` Jeff King 2018-08-26 20:56 ` [PATCH 0/9] introducing oideq() brian m. carlson 2018-08-27 12:41 ` Derrick Stolee 2018-08-28 21:21 ` Jeff King 2018-08-28 21:22 ` [PATCH v2 1/9] coccinelle: use <...> for function exclusion Jeff King 2018-08-28 21:22 ` [PATCH v2 2/9] introduce hasheq() and oideq() Jeff King 2018-08-28 21:22 ` [PATCH v2 3/9] convert "oidcmp() == 0" to oideq() Jeff King 2018-08-28 21:22 ` [PATCH v2 4/9] convert "hashcmp() == 0" to hasheq() Jeff King 2018-08-28 21:22 ` [PATCH v2 5/9] convert "oidcmp() != 0" to "!oideq()" Jeff King 2018-08-28 21:22 ` [PATCH v2 6/9] convert "hashcmp() != 0" to "!hasheq()" Jeff King 2018-08-28 21:22 ` [PATCH v2 7/9] convert hashmap comparison functions to oideq() Jeff King 2018-08-28 21:22 ` [PATCH v2 8/9] read-cache: use oideq() in ce_compare functions Jeff King 2018-08-28 21:23 ` [PATCH v2 9/9] show_dirstat: simplify same-content check Jeff King 2018-08-28 21:36 ` [PATCH 0/9] introducing oideq() Derrick Stolee 2018-08-29 0:08 ` brian m. carlson
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).