describe: use strbuf_add_unique_abbrev() for adding short hashes
PATCH 1/1 describe: use strbuf_add_unique_abbrev() for adding short hashes
Call strbuf_add_unique_abbrev() to add an abbreviated hash to a strbuf instead of taking a detour through find_unique_abbrev() and its static buffer. This is shorter and a bit more efficient.
Patch generated by Coccinelle (and contrib/coccinelle/strbuf.cocci).
---
The changed line was added by 4dbc59a4cc (builtin/describe.c: factor out describe_commit).
"make coccicheck" doesn't propose any other changes for current master.
builtin/describe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/describe.c +1 -1
@@ -380,7 +380,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)if (!match_cnt) {struct object_id *cmit_oid = &cmit->object.oid;if (always) { -strbuf_addstr (dst,find_unique_abbrev (cmit_oid->hash, abbrev)); +strbuf_add_unique_abbrev (dst, cmit_oid->hash, abbrev);if (suffix)strbuf_addstr (dst, suffix);return ;
Signature · 1 line
2.15.1
Quoted 21 lines · René Scharfe
Call
strbuf_add_unique_abbrev()to add an abbreviated hash to a strbuf instead of taking a detour throughfind_unique_abbrev()and its static buffer. This is shorter and a bit more efficient.Patch generated by Coccinelle (and
contrib/coccinelle/strbuf.cocci).Signed-off-by Rene Scharfe <[email protected]>---
The changed line was added by4dbc59a4cc(builtin/describe.c: factor outdescribe_commit)."make coccicheck" doesn't propose any other changes for current master.
builtin/describe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)builtin/describe.c +0
-0
builtin/describe.c static void describe_commit(struct object_id *oid, struct strbuf *dst) · line 386
Quoted 8 lines
builtin/describe.c +1
-1@@ -380,7 +380,7 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)if (!match_cnt) {struct object_id *cmit_oid = &cmit->object.oid;if (always) { -strbuf_addstr (dst,find_unique_abbrev (cmit_oid->hash, abbrev)); +strbuf_add_unique_abbrev (dst, cmit_oid->hash, abbrev);if (suffix)strbuf_addstr (dst, suffix);return ;
René,
Thanks for this cleanup! I just learned about strbuf_add_unique_abbrev() and like that it uses the reentrant find_unique_abbrev_r() instead.
Looks good to me.
-Stolee
Quoted 1 line
Patch generated by Coccinelle (and
contrib/coccinelle/strbuf.cocci).
Interesting.
The static analysis build job on Travis CI runs 'make coccicheck', so it should have caught this. However, I've looked at more build job results than I could count while working on some Travis CI related patches in the last few weeks, but I've never noticed Coccinelle proposing anything.
Now I see that Coccinelle's, and in turn 'make coccicheck's exit code only indicates that Coccinelle managed to finish without any errors (e.g. no wrong --options, missing files or whatnot), but it has nothing to do with whether it found something to transform or not. To find out the latter, we have to check the resulting 'contrib/coccinelle/*.cocci.patch' files or look closer at 'make coccicheck's standard output. If any of those '*.cocci.patch' files are not empty and make's output contains lines like:
SPATCH result: contrib/coccinelle/strbuf.cocci.patch
then Coccinelle found something.
Well, OK, I think that might be an acceptable behavior for developers running 'make coccicheck' themselves, but totally unsuitable for automated Travis CI build jobs, because it doesn't draw our attention to Coccinelle's findings. And the only means to draw attention in an automated setting is to fail the buid job.
Good, I quickly whipped up a patch to fail the build job if any of the resulting '*.cocci.patch' files are not empty and also to include their content in the trace log, to see how that would work out, and...
Quoted 1 line
"make coccicheck" doesn't propose any other changes for current master.
... and found that 'make coccicheck' on Travis CI _does_ propose further changes for current master. You can find the build job's output including those proposed changes here:
https://travis-ci.org/szeder/git/jobs/329296813#L586
The proposed changes coming from 'array.cocci' are replacing memmove() calls with MOVE_ARRAY(). After a (very) cursory look they all seem to make sense to me; at least after applying those changes Git can be built and the test suite still succeeds.
Unfortunately, most of the changes coming from 'strbuf.cocci' don't make any sense, they appear to be the mis-application of the "use strbuf_addstr() instead of strbuf_addf() to add a single string" rule:
- strbuf_addf(&sb_repo, "%d", counter);
+ strbuf_addstr(&sb_repo, counter);
It seems that those rules need some refinement, but I have no idea about Coccinelle and this is not the time for me to dig deeper.
What makes all this weird is that running 'make coccicheck' on my own machine doesn't produce any of these additional proposed changes, just like at René's. Can it be related to differing Coccinelle versions? Travis CI installs 1.0.0~rc19.deb-3; I have 1.0.4.deb-2.
Gábor
Am 16.01.2018 um 18:11 schrieb SZEDER Gábor:
Quoted 7 lines
Unfortunately, most of the changes coming from '
strbuf.cocci' don't make any sense, they appear to be the mis-application of the "usestrbuf_addstr()instead ofstrbuf_addf()to add a single string" rule:-
strbuf_addf(&sb_repo, "%d", counter);
+strbuf_addstr(&sb_repo, counter);
Quoted 7 lines
It seems that those rules need some refinement, but I have no idea about Coccinelle and this is not the time for me to dig deeper.
What makes all this weird is that running 'make coccicheck' on my own machine doesn't produce any of these additional proposed changes, just like at René's. Can it be related to differing Coccinelle versions? Travis CI installs 1.0.0~rc19.deb-3; I have 1.0.4.deb-2.
The version difference may explain it, but I couldn't find a matching bugfix in http://coccinelle.lip6.fr/distrib/changes.html when I just skimmed it. I wonder if the following patch could make a difference:
--- contrib/coccinelle/strbuf.cocci | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
contrib/coccinelle/strbuf.cocci +2 -1
@@ -29,8 +29,9 @@ cocci.include_match("%" not in fmt) @@ expression E1, E2; +format F =~ "s"; @@ -- strbuf_addf(E1, "%s", E2); +- strbuf_addf(E1, "%@F@", E2); + strbuf_addstr(E1, E2); @@
Signature · 1 line
2.16.0
Quoted 11 lines · René Scharfe
Am 16.01.2018 um 18:11 schrieb SZEDER Gábor:
Quoted 10 lines
Unfortunately, most of the changes coming from '
strbuf.cocci' don't make any sense, they appear to be the mis-application of the "usestrbuf_addstr()instead ofstrbuf_addf()to add a single string" rule:-
strbuf_addf(&sb_repo, "%d", counter);
+strbuf_addstr(&sb_repo, counter);It seems that those rules need some refinement, but I have no idea about Coccinelle and this is not the time for me to dig deeper.
Quoted 8 lines
Quoted 4 lines
What makes all this weird is that running 'make coccicheck' on my own machine doesn't produce any of these additional proposed changes, just like at René's. Can it be related to differing Coccinelle versions? Travis CI installs 1.0.0~rc19.deb-3; I have 1.0.4.deb-2.
The version difference may explain it, but I couldn't find a matching bugfix in http://coccinelle.lip6.fr/distrib/changes.html when I just skimmed it. I wonder if the following patch could make a difference:
Yes, it does, now all those nonsense suggestions are gone on Travis CI.
https://travis-ci.org/szeder/git/jobs/330572425#L713
Those "memmove() -> MOVE_ARRAY" suggestions are still there, of course.
Quoted 21 lines
--- contrib/coccinelle/strbuf.cocci | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)contrib/coccinelle/strbuf.cocci +2
-2@@ -29,8 +29,9 @@ cocci.include_match("%" not in fmt) @@ expression E1, E2; +format F =~ "s"; @@ -- strbuf_addf(E1, "%s", E2); +- strbuf_addf(E1, "%@F@", E2); + strbuf_addstr(E1, E2); @@ --2.16.0
Quoted 24 lines · SZEDER Gábor
Quoted 11 lines · René Scharfe
Am 16.01.2018 um 18:11 schrieb SZEDER Gábor:
Quoted 10 lines
Unfortunately, most of the changes coming from '
strbuf.cocci' don't make any sense, they appear to be the mis-application of the "usestrbuf_addstr()instead ofstrbuf_addf()to add a single string" rule:-
strbuf_addf(&sb_repo, "%d", counter);
+strbuf_addstr(&sb_repo, counter);It seems that those rules need some refinement, but I have no idea about Coccinelle and this is not the time for me to dig deeper.
Quoted 8 lines
Quoted 4 lines
What makes all this weird is that running 'make coccicheck' on my own machine doesn't produce any of these additional proposed changes, just like at René's. Can it be related to differing Coccinelle versions? Travis CI installs 1.0.0~rc19.deb-3; I have 1.0.4.deb-2.
The version difference may explain it, but I couldn't find a matching bugfix in http://coccinelle.lip6.fr/distrib/changes.html when I just skimmed it. I wonder if the following patch could make a difference:
Quoted 5 lines
Yes, it does, now all those nonsense suggestions are gone on Travis CI.
https://travis-ci.org/szeder/git/jobs/330572425#L713
Those "
memmove()->MOVE_ARRAY" suggestions are still there, of course.
Nice!
Travis CI runs Ubuntu Trusty as current base image with Coccinelle version "1.0.0~rc19.deb-3": https://packages.ubuntu.com/trusty/coccinelle
I was experimenting with a docker container that has the latest stable version of Coccinelle installed ("1.0.4.deb-2"). We could run this docker container inside of the Travis CI job similar to the 32-bit job. But with René's changes that doesn't seem to be necessary anymore!
- Lars
PS: Thanks a lot Gábor for recognizing and fixing the invalid
Travis CI Coccinelle build!
Am 18.01.2018 um 23:40 schrieb SZEDER Gábor:
Quoted 17 lines · René Scharfe
Quoted 16 lines
Am 16.01.2018 um 18:11 schrieb SZEDER Gábor:
Quoted 14 lines
Unfortunately, most of the changes coming from '
strbuf.cocci' don't make any sense, they appear to be the mis-application of the "usestrbuf_addstr()instead ofstrbuf_addf()to add a single string" rule:-
strbuf_addf(&sb_repo, "%d", counter);
+strbuf_addstr(&sb_repo, counter);It seems that those rules need some refinement, but I have no idea about Coccinelle and this is not the time for me to dig deeper.
What makes all this weird is that running 'make coccicheck' on my own machine doesn't produce any of these additional proposed changes, just like at René's. Can it be related to differing Coccinelle versions? Travis CI installs 1.0.0~rc19.deb-3; I have 1.0.4.deb-2.
Quoted 5 lines
Quoted 3 lines
The version difference may explain it, but I couldn't find a matching bugfix in http://coccinelle.lip6.fr/distrib/changes.html when I just skimmed it. I wonder if the following patch could make a difference:
Yes, it does, now all those nonsense suggestions are gone on Travis CI.
I would have expected matching a literal "%s" to be easier than dissecting that (admittedly simple) format string, but if it all works out fine then I'm not complaining. :) Sent the patch again properly.
Quoted 3 lines
https://travis-ci.org/szeder/git/jobs/330572425#L713Those "
memmove()->MOVE_ARRAY" suggestions are still there, of course.
They look valid and nice to have in that report. I wonder why we don't get them locally, though. Are you going to submit them as a patch?
(NB: The patches generated by coccicheck apply with "patch -p0", unlike
those generated by git diff and friends.)
Thanks,
René
PATCH 1/1 Use MOVE_ARRAY
Use the helper macro MOVE_ARRAY to move arrays. This is shorter and safer, as it automatically infers the size of elements.
Patch generated by Coccinelle and contrib/coccinelle/array.cocci in Travis CI's static analysis build job.
--- cache-tree.c | 5 ++--- commit.c | 6 ++---- diffcore-rename.c | 8 ++++---- dir.c | 4 ++-- parse-options.c | 2 +- read-cache.c | 5 ++--- refs/ref-cache.c | 6 ++---- replace_object.c | 6 ++---- rerere.c | 4 ++-- 9 files changed, 19 insertions(+), 27 deletions(-)
cache-tree.c +2 -3
@@ -84,9 +84,8 @@ static struct cache_tree_sub *find_subtree(struct cache_tree *it, down->namelen = pathlen;if (pos < it->subtree_nr) -memmove (it->down + pos +1 , - it->down + pos, -sizeof (down) * (it->subtree_nr - pos -1 )); +MOVE_ARRAY (it->down + pos +1 , it->down + pos, + it->subtree_nr - pos -1 ); it->down[pos] = down;return down; }
commit.c +2 -4
@@ -126,10 +126,8 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups)ALLOC_GROW (commit_graft, commit_graft_nr +1 , commit_graft_alloc); commit_graft_nr++;if (pos < commit_graft_nr) -memmove (commit_graft + pos +1 , - commit_graft + pos, - (commit_graft_nr - pos -1 ) * -sizeof (*commit_graft)); +MOVE_ARRAY (commit_graft + pos +1 , commit_graft + pos, + commit_graft_nr - pos -1 ); commit_graft[pos] = graft;return 0 ; }
diffcore-rename.c +4 -4
@@ -57,8 +57,8 @@ static int add_rename_dst(struct diff_filespec *two)ALLOC_GROW (rename_dst, rename_dst_nr +1 , rename_dst_alloc); rename_dst_nr++;if (first < rename_dst_nr) -memmove (rename_dst + first +1 , rename_dst + first, - (rename_dst_nr - first -1 ) *sizeof (*rename_dst)); +MOVE_ARRAY (rename_dst + first +1 , rename_dst + first, + rename_dst_nr - first -1 ); rename_dst[first].two =alloc_filespec (two->path);fill_filespec (rename_dst[first].two, &two->oid, two->oid_valid, two->mode);
@@ -98,8 +98,8 @@ static struct diff_rename_src *register_rename_src(struct diff_filepair *p)ALLOC_GROW (rename_src, rename_src_nr +1 , rename_src_alloc); rename_src_nr++;if (first < rename_src_nr) -memmove (rename_src + first +1 , rename_src + first, - (rename_src_nr - first -1 ) *sizeof (*rename_src)); +MOVE_ARRAY (rename_src + first +1 , rename_src + first, + rename_src_nr - first -1 ); rename_src[first].p = p; rename_src[first].score = score;return &(rename_src[first]);
dir.c +2 -2
@@ -747,8 +747,8 @@ static struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,FLEX_ALLOC_MEM (d, name, name, len);ALLOC_GROW (dir->dirs, dir->dirs_nr +1 , dir->dirs_alloc); -memmove (dir->dirs + first +1 , dir->dirs + first, - (dir->dirs_nr - first) *sizeof (*dir->dirs)); +MOVE_ARRAY (dir->dirs + first +1 , dir->dirs + first, + dir->dirs_nr - first); dir->dirs_nr++; dir->dirs[first] = d;return d;
parse-options.c +1 -1
@@ -525,7 +525,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,int parse_options_end (struct parse_opt_ctx_t *ctx) { -memmove (ctx->out + ctx->cpidx, ctx->argv, ctx->argc *sizeof (*ctx->out)); +MOVE_ARRAY (ctx->out + ctx->cpidx, ctx->argv, ctx->argc); ctx->out[ctx->cpidx + ctx->argc] =NULL ;return ctx->cpidx + ctx->argc; }
read-cache.c +2 -3
@@ -1217,9 +1217,8 @@ int add_index_entry(struct index_state *istate, struct cache_entry *ce, int opti/* Add it in.. */ istate->cache_nr++;if (istate->cache_nr > pos +1 ) -memmove (istate->cache + pos +1 , - istate->cache + pos, - (istate->cache_nr - pos -1 ) *sizeof (ce)); +MOVE_ARRAY (istate->cache + pos +1 , istate->cache + pos, + istate->cache_nr - pos -1 );set_index_entry (istate, pos, ce); istate->cache_changed |=CE_ENTRY_ADDED ;return 0 ;
refs/ref-cache.c +2 -4
@@ -238,10 +238,8 @@ int remove_entry_from_dir(struct ref_dir *dir, const char *refname)return -1 ; entry = dir->entries[entry_index]; -memmove (&dir->entries[entry_index], - &dir->entries[entry_index +1 ], - (dir->nr - entry_index -1 ) *sizeof (*dir->entries) - ); +MOVE_ARRAY (&dir->entries[entry_index], + &dir->entries[entry_index +1 ], dir->nr - entry_index -1 ); dir->nr--;if (dir->sorted > entry_index) dir->sorted--;
replace_object.c +2 -4
@@ -44,10 +44,8 @@ static int register_replace_object(struct replace_object *replace,ALLOC_GROW (replace_object, replace_object_nr +1 , replace_object_alloc); replace_object_nr++;if (pos < replace_object_nr) -memmove (replace_object + pos +1 , - replace_object + pos, - (replace_object_nr - pos -1 ) * -sizeof (*replace_object)); +MOVE_ARRAY (replace_object + pos +1 , replace_object + pos, + replace_object_nr - pos -1 ); replace_object[pos] = replace;return 0 ; }
rerere.c +2 -2
@@ -159,8 +159,8 @@ static struct rerere_dir *find_rerere_dir(const char *hex)ALLOC_GROW (rerere_dir, rerere_dir_nr +1 , rerere_dir_alloc);/* ... and add it in. */ rerere_dir_nr++; -memmove (rerere_dir + pos +1 , rerere_dir + pos, - (rerere_dir_nr - pos -1 ) *sizeof (*rerere_dir)); +MOVE_ARRAY (rerere_dir + pos +1 , rerere_dir + pos, + rerere_dir_nr - pos -1 ); rerere_dir[pos] = rr_dir;scan_rerere_dir (rr_dir); }
Signature · 1 line
2.16.1.80.gc0eec9753d
Re: [PATCH] Use MOVE_ARRAY
Quoted 5 lines · SZEDER Gábor
Use the helper macro
MOVE_ARRAYto move arrays. This is shorter and safer, as it automatically infers the size of elements.Patch generated by Coccinelle and
contrib/coccinelle/array.cocciin Travis CI's static analysis build job.
Seems pretty straightforward. One thing I did notice...
cache-tree.c static struct cache_tree_sub *find_subtree(struct cache_tree *it, · line 88
Quoted 13 lines
cache-tree.c +2
-3@@ -84,9 +84,8 @@ static struct cache_tree_sub *find_subtree(struct cache_tree *it, down->namelen = pathlen;if (pos < it->subtree_nr) -memmove (it->down + pos +1 , - it->down + pos, -sizeof (down) * (it->subtree_nr - pos -1 )); +MOVE_ARRAY (it->down + pos +1 , it->down + pos, + it->subtree_nr - pos -1 );
Most of these are "shift part of the array". I wonder if it would make sense to encapsulate that pattern in a helper, like:
#define SHIFT_ARRAY(a, nr, pos, slots) \
MOVE_ARRAY(a + pos + slots, a + pos, nr - pos - slots)
...
SHIFT_ARRAY(it->down, it->subtree_nr, pos, 1);I'm not sure if that's more readable because it describes a higher-level operation, or if it's less because it adds yet another non-standard helper for the reader to learn.
-Peff
Re: [PATCH] Use MOVE_ARRAY
Quoted 7 lines · Jeff King
Most of these are "shift part of the array". I wonder if it would make sense to encapsulate that pattern in a helper, like:
#define SHIFT_ARRAY(a, nr, pos, slots) \ MOVE_ARRAY(a + pos + slots, a + pos, nr - pos - slots) ... SHIFT_ARRAY(it->down, it->subtree_nr, pos, 1);
Exactly my thought when I was queuing it, but I was wondering about this more from "can we use the higher level abstraction for reducing errors?" point of view. If we are shifting an array by 3 slots to the right, we should at least have enough slots allocated to hold them (i.e. a->nr - a->alloc must be 3 or more). But after realizing that the level these macros operate at is still a bit too low to do something like that, I quickly lost interest ;-)
Quoted 3 lines
I'm not sure if that's more readable because it describes a higher-level operation, or if it's less because it adds yet another non-standard helper for the reader to learn.
Yeah, conflicting goals. It indeed is easier to see what is going on when reading the code, once the reader gets used to them.
Thanks.
Re: [PATCH] Use MOVE_ARRAY
Quoted 10 lines · Junio C Hamano
Quoted 7 lines · Jeff King
Most of these are "shift part of the array". I wonder if it would make sense to encapsulate that pattern in a helper, like:
#define SHIFT_ARRAY(a, nr, pos, slots) \ MOVE_ARRAY(a + pos + slots, a + pos, nr - pos - slots) ... SHIFT_ARRAY(it->down, it->subtree_nr, pos, 1);
Quoted 7 lines
Exactly my thought when I was queuing it, but I was wondering about this more from "can we use the higher level abstraction for reducing errors?" point of view. If we are shifting an array by 3 slots to the right, we should at least have enough slots allocated to hold them (i.e.
a->nr-a->allocmust be 3 or more). But after realizing that the level these macros operate at is still a bit too low to do something like that, I quickly lost interest ;-)
Yeah, you'd need to know the "alloc" number to right-shift correctly, since by definition we're pushing off the end of a->nr. Left-shifts just need to make sure they don't go past "0", which we can do here, but I'd think they're pretty uncommon.
The right macro level may actually be something more like "make room for N items at pos". E.g.:
#define CREATE_ARRAY_HOLE(a, nr, alloc, pos, slots) do { \
ALLOC_GROW(a, nr + slots, alloc);
MOVE_ARRAY(a + pos + slots, a + pos, nr - pos - slots);} while (0)
but I didn't investigate the surrounding code. And it surely would need a catchier name. ;)
-Peff