Message ID | 20220430041406.164719-2-gitter.spiros@gmail.com (mailing list archive) |
---|---|
State | Accepted |
Commit | 7a618493facb79639231f797e492fab51fac2ba4 |
Headers | show |
Series | add a new coccinelle semantic patch to enforce a | expand |
On 30/04/2022 05:13, Elia Pinto wrote: > Add a coccinelle semantic patch necessary to reinforce the git coding style > guideline: > > "Do not explicitly compute an integral value with constant 0 or '\ 0', or a s/compute/compare/ > pointer value with constant NULL." If this gets re-rolled, perhaps include a simple example for those who don't immediately understand that quoted sentence. It will also help decode the coccinelle script so: `if (ptr == NULL)` becomes `if (!ptr)` etc. -- Philip > > Signed-off-by: Elia Pinto <gitter.spiros@gmail.com> > --- > contrib/coccinelle/equals-null.cocci | 30 ++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > create mode 100644 contrib/coccinelle/equals-null.cocci > > diff --git a/contrib/coccinelle/equals-null.cocci b/contrib/coccinelle/equals-null.cocci > new file mode 100644 > index 0000000000..92c7054013 > --- /dev/null > +++ b/contrib/coccinelle/equals-null.cocci > @@ -0,0 +1,30 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +@@ > +expression e; > +statement s; > +@@ > +if ( > +( > +!e > +| > +- e == NULL > ++ !e > +) > + ) > + {...} > +else s > + > +@@ > +expression e; > +statement s; > +@@ > +if ( > +( > +e > +| > +- e != NULL > ++ e > +) > + ) > + {...} > +else s
Philip Oakley <philipoakley@iee.email> writes: > On 30/04/2022 05:13, Elia Pinto wrote: >> Add a coccinelle semantic patch necessary to reinforce the git coding style >> guideline: >> >> "Do not explicitly compute an integral value with constant 0 or '\ 0', or a > > s/compute/compare/ >> pointer value with constant NULL." > > If this gets re-rolled, perhaps include a simple example for those who > don't immediately understand that quoted sentence. It will also help > decode the coccinelle script > > so: `if (ptr == NULL)` becomes `if (!ptr)` etc. That is certainly a good suggestion, but I am wondering if we want to also emphasize another more generally applicable rule that appears much earlier in the guideline document: - Fixing style violations while working on a real change as a preparatory clean-up step is good, but otherwise avoid useless code churn for the sake of conforming to the style. "Once it _is_ in the tree, it's not really worth the patch noise to go and fix it up." Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html
On 30/04/2022 21:56, Junio C Hamano wrote: > Philip Oakley <philipoakley@iee.email> writes: > >> On 30/04/2022 05:13, Elia Pinto wrote: >>> Add a coccinelle semantic patch necessary to reinforce the git coding style >>> guideline: >>> >>> "Do not explicitly compute an integral value with constant 0 or '\ 0', or a >> s/compute/compare/ >>> pointer value with constant NULL." >> If this gets re-rolled, perhaps include a simple example for those who >> don't immediately understand that quoted sentence. It will also help >> decode the coccinelle script >> >> so: `if (ptr == NULL)` becomes `if (!ptr)` etc. > That is certainly a good suggestion, but I am wondering if we want > to also emphasize another more generally applicable rule that > appears much earlier in the guideline document: > > - Fixing style violations while working on a real change as a > preparatory clean-up step is good, but otherwise avoid useless code > churn for the sake of conforming to the style. > > "Once it _is_ in the tree, it's not really worth the patch noise to > go and fix it up." > Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html I think it goes both ways when the 'bad' style can be cargo-cult copied too easily, negating the value of the guidance. That said, having 22 patches to renormalise the codebase does end up as being excessive. And it's not clear if the first cocci patch ends up as part of the regular lint checking (I'm not a user of cocci..). I suspect that all the renormalising fixes were the result of a single cocci check, so having a single patch that makes the codebase clean would be better, if accepted. -- Philip
Philip Oakley <philipoakley@iee.email> writes: > I think it goes both ways when the 'bad' style can be cargo-cult copied > too easily, negating the value of the guidance. Yes, and the cocci rules by themselves do not help protecting our codebase against it all that much. In order to help developers follow the guideline to avoid adding _new_ instances (by copying-and-pasting), it should be easy to use such a checker in such a way that we can notice only _new_ breakges while ignoring existing offenders. I do not think the current cocci check target in our Makefile is prepared for that. And there are two ways to deal with that shortcoming. One, which often appears easier to implement but in the medium term is very costly, is to freeze the codebase and apply tree-wide code churn to make warning disappear. Then _any_ breakages noticed by an inadequate tool, which does not allow us to notice only the new breakages, after applying a patch to such a cleansed codebase by definition are coming from the patch. But it is costly. The codebase is rarely frozen, so there isn't a good time to apply such a patch, whether it is 22-patch series or a single patch that concatenates everything into one. There may be more urgent issues than style fixes that would force us to revert a change made before such a tree-wide clean-up, and when that happens, such a "clean-up for clean-up's sake because we cannot check incrementally" will inevitably conflict with such a change. The other approach is to make it possible (and easy) to check incrementally, so that we can detect new instances made by copying and pasting. Thanks.
What I found curious is that the result of applying these patches to v2.36.0 and running coccicheck reveals that we are not making the codebase clean wrt this new coccinelle rule. diff -u -p a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c --- a/compat/fsmonitor/fsm-listen-darwin.c +++ b/compat/fsmonitor/fsm-listen-darwin.c @@ -342,7 +342,7 @@ int fsm_listen__ctor(struct fsmonitor_da data->cfar_paths_to_watch, kFSEventStreamEventIdSinceNow, 0.001, flags); - if (data->stream == NULL) + if (!data->stream) goto failed; /* diff -u -p a/compat/mingw.c b/compat/mingw.c --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1060,7 +1060,7 @@ char *mingw_mktemp(char *template) int mkstemp(char *template) { char *filename = mktemp(template); - if (filename == NULL) + if (!filename) return -1; return open(filename, O_RDWR | O_CREAT, 0600); } @@ -2332,7 +2332,7 @@ int setitimer(int type, struct itimerval static const struct timeval zero; static int atexit_done; - if (out != NULL) + if (out) return errno = EINVAL, error("setitimer param 3 != NULL not implemented"); if (!is_timeval_eq(&in->it_interval, &zero) && @@ -2361,7 +2361,7 @@ int sigaction(int sig, struct sigaction if (sig != SIGALRM) return errno = EINVAL, error("sigaction only implemented for SIGALRM"); - if (out != NULL) + if (out) return errno = EINVAL, error("sigaction: param 3 != NULL not implemented"); diff -u -p a/compat/mkdir.c b/compat/mkdir.c --- a/compat/mkdir.c +++ b/compat/mkdir.c @@ -9,7 +9,7 @@ int compat_mkdir_wo_trailing_slash(const size_t len = strlen(dir); if (len && dir[len-1] == '/') { - if ((tmp_dir = strdup(dir)) == NULL) + if (!(tmp_dir = strdup(dir))) return -1; tmp_dir[len-1] = '\0'; } diff -u -p a/compat/mmap.c b/compat/mmap.c --- a/compat/mmap.c +++ b/compat/mmap.c @@ -13,7 +13,7 @@ void *git_mmap(void *start, size_t lengt } start = malloc(length); - if (start == NULL) { + if (!start) { errno = ENOMEM; return MAP_FAILED; } diff -u -p a/compat/mingw.c b/compat/mingw.c --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1060,7 +1060,7 @@ char *mingw_mktemp(char *template) int mkstemp(char *template) { char *filename = mktemp(template); - if (filename == NULL) + if (!filename) return -1; return open(filename, O_RDWR | O_CREAT, 0600); } @@ -2332,7 +2332,7 @@ int setitimer(int type, struct itimerval static const struct timeval zero; static int atexit_done; - if (out != NULL) + if (out) return errno = EINVAL, error("setitimer param 3 != NULL not implemented"); if (!is_timeval_eq(&in->it_interval, &zero) && @@ -2361,7 +2361,7 @@ int sigaction(int sig, struct sigaction if (sig != SIGALRM) return errno = EINVAL, error("sigaction only implemented for SIGALRM"); - if (out != NULL) + if (out) return errno = EINVAL, error("sigaction: param 3 != NULL not implemented"); diff -u -p a/config.c b/config.c --- a/config.c +++ b/config.c @@ -3190,7 +3190,7 @@ int git_config_set_multivar_in_file_gent goto out_free; } /* if nothing to unset, error out */ - if (value == NULL) { + if (!value) { ret = CONFIG_NOTHING_SET; goto out_free; } @@ -3206,7 +3206,7 @@ int git_config_set_multivar_in_file_gent int i, new_line = 0; struct config_options opts; - if (value_pattern == NULL) + if (!value_pattern) store.value_pattern = NULL; else if (value_pattern == CONFIG_REGEX_NONE) store.value_pattern = CONFIG_REGEX_NONE; @@ -3346,7 +3346,7 @@ int git_config_set_multivar_in_file_gent } /* write the pair (value == NULL means unset) */ - if (value != NULL) { + if (value) { if (!store.section_seen) { if (write_section(fd, key, &store) < 0) goto write_err_out; @@ -3567,7 +3567,7 @@ static int git_config_copy_or_rename_sec offset = section_name_match(&buf[i], old_name); if (offset > 0) { ret++; - if (new_name == NULL) { + if (!new_name) { remove = 1; continue; } diff -u -p a/daemon.c b/daemon.c --- a/daemon.c +++ b/daemon.c @@ -447,7 +447,7 @@ static void copy_to_log(int fd) FILE *fp; fp = fdopen(fd, "r"); - if (fp == NULL) { + if (!fp) { logerror("fdopen of error channel failed"); close(fd); return; diff -u -p a/dir.c b/dir.c --- a/dir.c +++ b/dir.c @@ -3054,7 +3054,7 @@ char *git_url_basename(const char *repo, * Skip scheme. */ start = strstr(repo, "://"); - if (start == NULL) + if (!start) start = repo; else start += 3; diff -u -p a/ewah/bitmap.c b/ewah/bitmap.c --- a/ewah/bitmap.c +++ b/ewah/bitmap.c @@ -223,7 +223,7 @@ void bitmap_reset(struct bitmap *bitmap) void bitmap_free(struct bitmap *bitmap) { - if (bitmap == NULL) + if (!bitmap) return; free(bitmap->words); diff -u -p a/ewah/ewah_bitmap.c b/ewah/ewah_bitmap.c --- a/ewah/ewah_bitmap.c +++ b/ewah/ewah_bitmap.c @@ -451,7 +451,7 @@ struct ewah_bitmap *ewah_pool_new(void) void ewah_pool_free(struct ewah_bitmap *self) { - if (self == NULL) + if (!self) return; if (bitmap_pool_size == BITMAP_POOL_MAX || diff -u -p a/http-fetch.c b/http-fetch.c --- a/http-fetch.c +++ b/http-fetch.c @@ -55,7 +55,7 @@ static void fetch_single_packfile(struct http_init(NULL, url, 0); preq = new_direct_http_pack_request(packfile_hash->hash, xstrdup(url)); - if (preq == NULL) + if (!preq) die("couldn't create http pack request"); preq->slot->results = &results; preq->index_pack_args = index_pack_args; diff -u -p a/http-push.c b/http-push.c --- a/http-push.c +++ b/http-push.c @@ -253,7 +253,7 @@ static void start_fetch_loose(struct tra struct http_object_request *obj_req; obj_req = new_http_object_request(repo->url, &request->obj->oid); - if (obj_req == NULL) { + if (!obj_req) { request->state = ABORTED; return; } @@ -318,7 +318,7 @@ static void start_fetch_packed(struct tr fprintf(stderr, " which contains %s\n", oid_to_hex(&request->obj->oid)); preq = new_http_pack_request(target->hash, repo->url); - if (preq == NULL) { + if (!preq) { repo->can_update_info_refs = 0; return; } @@ -520,7 +520,7 @@ static void finish_request(struct transf /* Keep locks active */ check_locks(); - if (request->headers != NULL) + if (request->headers) curl_slist_free_all(request->headers); /* URL is reused for MOVE after PUT and used during FETCH */ @@ -783,7 +783,7 @@ xml_start_tag(void *userData, const char const char *c = strchr(name, ':'); int old_namelen, new_len; - if (c == NULL) + if (!c) c = name; else c++; @@ -811,7 +811,7 @@ xml_end_tag(void *userData, const char * ctx->userFunc(ctx, 1); - if (c == NULL) + if (!c) c = name; else c++; @@ -1893,7 +1893,7 @@ int cmd_main(int argc, const char **argv /* Lock remote branch ref */ ref_lock = lock_remote(ref->name, LOCK_TIME); - if (ref_lock == NULL) { + if (!ref_lock) { fprintf(stderr, "Unable to lock remote branch %s\n", ref->name); if (helper_status) diff -u -p a/http-walker.c b/http-walker.c --- a/http-walker.c +++ b/http-walker.c @@ -59,7 +59,7 @@ static void start_object_request(struct struct http_object_request *req; req = new_http_object_request(obj_req->repo->base, &obj_req->oid); - if (req == NULL) { + if (!req) { obj_req->state = ABORTED; return; } @@ -106,7 +106,7 @@ static void process_object_response(void /* Use alternates if necessary */ if (missing_target(obj_req->req)) { fetch_alternates(walker, alt->base); - if (obj_req->repo->next != NULL) { + if (obj_req->repo->next) { obj_req->repo = obj_req->repo->next; release_http_object_request(obj_req->req); @@ -225,12 +225,12 @@ static void process_alternates_response( alt_req->url->buf); active_requests++; slot->in_use = 1; - if (slot->finished != NULL) + if (slot->finished) (*slot->finished) = 0; if (!start_active_slot(slot)) { cdata->got_alternates = -1; slot->in_use = 0; - if (slot->finished != NULL) + if (slot->finished) (*slot->finished) = 1; } return; @@ -443,7 +443,7 @@ static int http_fetch_pack(struct walker } preq = new_http_pack_request(target->hash, repo->base); - if (preq == NULL) + if (!preq) goto abort; preq->slot->results = &results; @@ -489,11 +489,11 @@ static int fetch_object(struct walker *w if (hasheq(obj_req->oid.hash, hash)) break; } - if (obj_req == NULL) + if (!obj_req) return error("Couldn't find request for %s in the queue", hex); if (has_object_file(&obj_req->oid)) { - if (obj_req->req != NULL) + if (obj_req->req) abort_http_object_request(obj_req->req); abort_object_request(obj_req); return 0; diff -u -p a/http.c b/http.c --- a/http.c +++ b/http.c @@ -197,11 +197,11 @@ static void finish_active_slot(struct ac closedown_active_slot(slot); curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code); - if (slot->finished != NULL) + if (slot->finished) (*slot->finished) = 1; /* Store slot results so they can be read after the slot is reused */ - if (slot->results != NULL) { + if (slot->results) { slot->results->curl_result = slot->curl_result; slot->results->http_code = slot->http_code; curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL, @@ -212,7 +212,7 @@ static void finish_active_slot(struct ac } /* Run callback if appropriate */ - if (slot->callback_func != NULL) + if (slot->callback_func) slot->callback_func(slot->callback_data); } @@ -234,7 +234,7 @@ static void process_curl_messages(void) while (slot != NULL && slot->curl != curl_message->easy_handle) slot = slot->next; - if (slot != NULL) { + if (slot) { xmulti_remove_handle(slot); slot->curl_result = curl_result; finish_active_slot(slot); @@ -838,16 +838,16 @@ static CURL *get_curl_handle(void) curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST, ssl_cipherlist); - if (ssl_cert != NULL) + if (ssl_cert) curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert); if (has_cert_password()) curl_easy_setopt(result, CURLOPT_KEYPASSWD, cert_auth.password); - if (ssl_key != NULL) + if (ssl_key) curl_easy_setopt(result, CURLOPT_SSLKEY, ssl_key); - if (ssl_capath != NULL) + if (ssl_capath) curl_easy_setopt(result, CURLOPT_CAPATH, ssl_capath); #ifdef GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY - if (ssl_pinnedkey != NULL) + if (ssl_pinnedkey) curl_easy_setopt(result, CURLOPT_PINNEDPUBLICKEY, ssl_pinnedkey); #endif if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) && @@ -857,10 +857,10 @@ static CURL *get_curl_handle(void) curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, NULL); #endif } else if (ssl_cainfo != NULL || http_proxy_ssl_ca_info != NULL) { - if (ssl_cainfo != NULL) + if (ssl_cainfo) curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo); #ifdef GIT_CURL_HAVE_CURLOPT_PROXY_CAINFO - if (http_proxy_ssl_ca_info != NULL) + if (http_proxy_ssl_ca_info) curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, http_proxy_ssl_ca_info); #endif } @@ -1050,7 +1050,7 @@ void http_init(struct remote *remote, co { char *http_max_requests = getenv("GIT_HTTP_MAX_REQUESTS"); - if (http_max_requests != NULL) + if (http_max_requests) max_requests = atoi(http_max_requests); } @@ -1069,10 +1069,10 @@ void http_init(struct remote *remote, co set_from_env(&user_agent, "GIT_HTTP_USER_AGENT"); low_speed_limit = getenv("GIT_HTTP_LOW_SPEED_LIMIT"); - if (low_speed_limit != NULL) + if (low_speed_limit) curl_low_speed_limit = strtol(low_speed_limit, NULL, 10); low_speed_time = getenv("GIT_HTTP_LOW_SPEED_TIME"); - if (low_speed_time != NULL) + if (low_speed_time) curl_low_speed_time = strtol(low_speed_time, NULL, 10); if (curl_ssl_verify == -1) @@ -1109,7 +1109,7 @@ void http_cleanup(void) while (slot != NULL) { struct active_request_slot *next = slot->next; - if (slot->curl != NULL) { + if (slot->curl) { xmulti_remove_handle(slot); curl_easy_cleanup(slot->curl); } @@ -1147,13 +1147,13 @@ void http_cleanup(void) free((void *)http_proxy_authmethod); http_proxy_authmethod = NULL; - if (cert_auth.password != NULL) { + if (cert_auth.password) { memset(cert_auth.password, 0, strlen(cert_auth.password)); FREE_AND_NULL(cert_auth.password); } ssl_cert_password_required = 0; - if (proxy_cert_auth.password != NULL) { + if (proxy_cert_auth.password) { memset(proxy_cert_auth.password, 0, strlen(proxy_cert_auth.password)); FREE_AND_NULL(proxy_cert_auth.password); } @@ -1179,14 +1179,14 @@ struct active_request_slot *get_active_s while (slot != NULL && slot->in_use) slot = slot->next; - if (slot == NULL) { + if (!slot) { newslot = xmalloc(sizeof(*newslot)); newslot->curl = NULL; newslot->in_use = 0; newslot->next = NULL; slot = active_queue_head; - if (slot == NULL) { + if (!slot) { active_queue_head = newslot; } else { while (slot->next != NULL) @@ -1196,7 +1196,7 @@ struct active_request_slot *get_active_s slot = newslot; } - if (slot->curl == NULL) { + if (!slot->curl) { slot->curl = curl_easy_duphandle(curl_default); curl_session_count++; } @@ -1768,7 +1768,7 @@ static int http_request(const char *url, slot = get_active_slot(); curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); - if (result == NULL) { + if (!result) { curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1); } else { curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0); @@ -2100,7 +2100,7 @@ cleanup: void release_http_pack_request(struct http_pack_request *preq) { - if (preq->packfile != NULL) { + if (preq->packfile) { fclose(preq->packfile); preq->packfile = NULL; } @@ -2391,7 +2391,7 @@ abort: void process_http_object_request(struct http_object_request *freq) { - if (freq->slot == NULL) + if (!freq->slot) return; freq->curl_result = freq->slot->curl_result; freq->http_code = freq->slot->http_code; @@ -2448,7 +2448,7 @@ void release_http_object_request(struct freq->localfile = -1; } FREE_AND_NULL(freq->url); - if (freq->slot != NULL) { + if (freq->slot) { freq->slot->callback_func = NULL; freq->slot->callback_data = NULL; release_active_slot(freq->slot); diff -u -p a/kwset.c b/kwset.c --- a/kwset.c +++ b/kwset.c @@ -477,7 +477,7 @@ kwsprep (kwset_t kws) next[i] = NULL; treenext(kwset->trie->links, next); - if ((trans = kwset->trans) != NULL) + if ((trans = kwset->trans)) for (i = 0; i < NCHAR; ++i) kwset->next[i] = next[U(trans[i])]; else @@ -485,7 +485,7 @@ kwsprep (kwset_t kws) } /* Fix things up for any translation table. */ - if ((trans = kwset->trans) != NULL) + if ((trans = kwset->trans)) for (i = 0; i < NCHAR; ++i) kwset->delta[i] = delta[U(trans[i])]; else diff -u -p a/ll-merge.c b/ll-merge.c --- a/ll-merge.c +++ b/ll-merge.c @@ -207,7 +207,7 @@ static enum ll_merge_result ll_ext_merge dict[4].placeholder = "P"; dict[4].value = path_sq.buf; dict[5].placeholder = NULL; dict[5].value = NULL; - if (fn->cmdline == NULL) + if (!fn->cmdline) die("custom merge driver %s lacks command line.", fn->name); result->ptr = NULL; diff -u -p a/log-tree.c b/log-tree.c --- a/log-tree.c +++ b/log-tree.c @@ -88,7 +88,7 @@ static int match_ref_pattern(const char const struct string_list_item *item) { int matched = 0; - if (item->util == NULL) { + if (!item->util) { if (!wildmatch(item->string, refname, 0)) matched = 1; } else { diff -u -p a/mailinfo.c b/mailinfo.c --- a/mailinfo.c +++ b/mailinfo.c @@ -698,7 +698,7 @@ static int is_scissors_line(const char * continue; } last_nonblank = c; - if (first_nonblank == NULL) + if (!first_nonblank) first_nonblank = c; if (*c == '-') { in_perforation = 1; @@ -1094,7 +1094,7 @@ static void handle_body(struct mailinfo */ lines = strbuf_split(line, '\n'); for (it = lines; (sb = *it); it++) { - if (*(it + 1) == NULL) /* The last line */ + if (!*(it + 1)) /* The last line */ if (sb->buf[sb->len - 1] != '\n') { /* Partial line, save it for later. */ strbuf_addbuf(&prev, sb); diff -u -p a/mailmap.c b/mailmap.c --- a/mailmap.c +++ b/mailmap.c @@ -77,7 +77,7 @@ static void add_mapping(struct string_li struct mailmap_entry *me; struct string_list_item *item; - if (old_email == NULL) { + if (!old_email) { old_email = new_email; new_email = NULL; } @@ -92,7 +92,7 @@ static void add_mapping(struct string_li item->util = me; } - if (old_name == NULL) { + if (!old_name) { debug_mm("mailmap: adding (simple) entry for '%s'\n", old_email); /* Replace current name and new email for simple entry */ @@ -123,9 +123,9 @@ static char *parse_name_and_email(char * char *left, *right, *nstart, *nend; *name = *email = NULL; - if ((left = strchr(buffer, '<')) == NULL) + if (!(left = strchr(buffer, '<'))) return NULL; - if ((right = strchr(left+1, '>')) == NULL) + if (!(right = strchr(left + 1, '>'))) return NULL; if (!allow_empty_email && (left+1 == right)) return NULL; @@ -153,7 +153,7 @@ static void read_mailmap_line(struct str if (buffer[0] == '#') return; - if ((name2 = parse_name_and_email(buffer, &name1, &email1, 0)) != NULL) + if ((name2 = parse_name_and_email(buffer, &name1, &email1, 0))) parse_name_and_email(name2, &name2, &email2, 1); if (email1) @@ -320,7 +320,7 @@ int map_user(struct string_list *map, (int)*emaillen, debug_str(*email)); item = lookup_prefix(map, *email, *emaillen); - if (item != NULL) { + if (item) { me = (struct mailmap_entry *)item->util; if (me->namemap.nr) { /* @@ -334,7 +334,7 @@ int map_user(struct string_list *map, item = subitem; } } - if (item != NULL) { + if (item) { struct mailmap_info *mi = (struct mailmap_info *)item->util; if (mi->name == NULL && mi->email == NULL) { debug_mm("map_user: -- (no simple mapping)\n"); diff -u -p a/merge-ort.c b/merge-ort.c --- a/merge-ort.c +++ b/merge-ort.c @@ -2068,7 +2068,7 @@ static char *handle_path_level_conflicts * to ensure that's the case. */ c_info = strmap_get(collisions, new_path); - if (c_info == NULL) + if (!c_info) BUG("c_info is NULL"); /* @@ -4640,7 +4640,7 @@ static void merge_ort_internal(struct me } merged_merge_bases = pop_commit(&merge_bases); - if (merged_merge_bases == NULL) { + if (!merged_merge_bases) { /* if there is no common ancestor, use an empty tree */ struct tree *tree; diff -u -p a/merge-recursive.c b/merge-recursive.c --- a/merge-recursive.c +++ b/merge-recursive.c @@ -82,7 +82,7 @@ static struct dir_rename_entry *dir_rena { struct dir_rename_entry key; - if (dir == NULL) + if (!dir) return NULL; hashmap_entry_init(&key.ent, strhash(dir)); key.dir = dir; @@ -1990,14 +1990,14 @@ static void get_renamed_dir_portion(cons * renamed means the root directory can never be renamed -- because * the root directory always exists). */ - if (end_of_old == NULL) + if (!end_of_old) return; /* Note: *old_dir and *new_dir are still NULL */ /* * If new_path contains no directory (end_of_new is NULL), then we * have a rename of old_path's directory to the root directory. */ - if (end_of_new == NULL) { + if (!end_of_new) { *old_dir = xstrndup(old_path, end_of_old - old_path); *new_dir = xstrdup(""); return; @@ -2116,7 +2116,7 @@ static char *handle_path_level_conflicts * to ensure that's the case. */ collision_ent = collision_find_entry(collisions, new_path); - if (collision_ent == NULL) + if (!collision_ent) BUG("collision_ent is NULL"); /* @@ -2996,7 +2996,7 @@ static void final_cleanup_rename(struct const struct rename *re; int i; - if (rename == NULL) + if (!rename) return; for (i = 0; i < rename->nr; i++) { @@ -3605,7 +3605,7 @@ static int merge_recursive_internal(stru } merged_merge_bases = pop_commit(&merge_bases); - if (merged_merge_bases == NULL) { + if (!merged_merge_bases) { /* if there is no common ancestor, use an empty tree */ struct tree *tree; diff -u -p a/object-file.c b/object-file.c --- a/object-file.c +++ b/object-file.c @@ -1728,7 +1728,7 @@ void *read_object_file_extended(struct r die(_("loose object %s (stored in %s) is corrupt"), oid_to_hex(repl), path); - if ((p = has_packed_and_bad(r, repl)) != NULL) + if ((p = has_packed_and_bad(r, repl))) die(_("packed object %s (stored in %s) is corrupt"), oid_to_hex(repl), p->pack_name); obj_read_unlock(); diff -u -p a/pack-bitmap.c b/pack-bitmap.c --- a/pack-bitmap.c +++ b/pack-bitmap.c @@ -111,7 +111,7 @@ static struct ewah_bitmap *lookup_stored struct ewah_bitmap *parent; struct ewah_bitmap *composed; - if (st->xor == NULL) + if (!st->xor) return st->root; composed = ewah_pool_new(); @@ -279,7 +279,7 @@ static int load_bitmap_entries_v1(struct if (xor_offset > 0) { xor_bitmap = recent_bitmaps[(i - xor_offset) % MAX_XOR_OFFSET]; - if (xor_bitmap == NULL) + if (!xor_bitmap) return error("Invalid XOR offset in bitmap pack index"); } @@ -728,7 +728,7 @@ static int add_commit_to_bitmap(struct b if (!or_with) return 0; - if (*base == NULL) + if (!*base) *base = ewah_to_bitmap(or_with); else bitmap_or_ewah(*base, or_with); @@ -771,7 +771,7 @@ static struct bitmap *find_objects(struc * Best case scenario: We found bitmaps for all the roots, * so the resulting `or` bitmap has the full reachability analysis */ - if (not_mapped == NULL) + if (!not_mapped) return base; roots = not_mapped; @@ -805,7 +805,7 @@ static struct bitmap *find_objects(struc struct include_data incdata; struct bitmap_show_data show_data; - if (base == NULL) + if (!base) base = bitmap_new(); incdata.bitmap_git = bitmap_git; @@ -1299,7 +1299,7 @@ struct bitmap_index *prepare_bitmap_walk reset_revision_walk(); revs->ignore_missing_links = 0; - if (haves_bitmap == NULL) + if (!haves_bitmap) BUG("failed to perform bitmap walk"); } @@ -1698,7 +1698,7 @@ void test_bitmap_walk(struct rev_info *r result = ewah_to_bitmap(bm); } - if (result == NULL) + if (!result) die("Commit %s doesn't have an indexed bitmap", oid_to_hex(&root->oid)); revs->tag_objects = 1; diff -u -p a/packfile.c b/packfile.c --- a/packfile.c +++ b/packfile.c @@ -116,7 +116,7 @@ int load_idx(const char *path, const uns if (idx_size < 4 * 256 + hashsz + hashsz) return error("index file %s is too small", path); - if (idx_map == NULL) + if (!idx_map) return error("empty data"); if (hdr->idx_signature == htonl(PACK_IDX_SIGNATURE)) { diff -u -p a/path.c b/path.c --- a/path.c +++ b/path.c @@ -733,7 +733,7 @@ char *interpolate_path(const char *path, struct strbuf user_path = STRBUF_INIT; const char *to_copy = path; - if (path == NULL) + if (!path) goto return_null; if (skip_prefix(path, "%(prefix)/", &path)) diff -u -p a/prio-queue.c b/prio-queue.c --- a/prio-queue.c +++ b/prio-queue.c @@ -19,7 +19,7 @@ void prio_queue_reverse(struct prio_queu { int i, j; - if (queue->compare != NULL) + if (queue->compare) BUG("prio_queue_reverse() on non-LIFO queue"); for (i = 0; i < (j = (queue->nr - 1) - i); i++) swap(queue, i, j); diff -u -p a/promisor-remote.c b/promisor-remote.c --- a/promisor-remote.c +++ b/promisor-remote.c @@ -84,7 +84,7 @@ static void promisor_remote_move_to_tail struct promisor_remote *r, struct promisor_remote *previous) { - if (r->next == NULL) + if (!r->next) return; if (previous) diff -u -p a/ref-filter.c b/ref-filter.c --- a/ref-filter.c +++ b/ref-filter.c @@ -1261,7 +1261,7 @@ static void grab_date(const char *buf, s * ":" means no format is specified, and use the default. */ formatp = strchr(atomname, ':'); - if (formatp != NULL) { + if (formatp) { formatp++; parse_date_format(formatp, &date_mode); } @@ -1509,7 +1509,7 @@ static void fill_missing_values(struct a int i; for (i = 0; i < used_atom_cnt; i++) { struct atom_value *v = &val[i]; - if (v->s == NULL) + if (!v->s) v->s = xstrdup(""); } } @@ -1619,7 +1619,7 @@ static const char *rstrip_ref_components while (remaining-- > 0) { char *p = strrchr(start, '/'); - if (p == NULL) { + if (!p) { free((char *)to_free); return xstrdup(""); } else diff -u -p a/refs/ref-cache.c b/refs/ref-cache.c --- a/refs/ref-cache.c +++ b/refs/ref-cache.c @@ -134,7 +134,7 @@ int search_ref_dir(struct ref_dir *dir, r = bsearch(&key, dir->entries, dir->nr, sizeof(*dir->entries), ref_entry_cmp_sslice); - if (r == NULL) + if (!r) return -1; return r - dir->entries; diff -u -p a/reftable/stack_test.c b/reftable/stack_test.c --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -35,7 +35,7 @@ static int count_dir_entries(const char DIR *dir = opendir(dirname); int len = 0; struct dirent *d; - if (dir == NULL) + if (!dir) return 0; while ((d = readdir(dir))) { diff -u -p a/reftable/tree.c b/reftable/tree.c --- a/reftable/tree.c +++ b/reftable/tree.c @@ -16,7 +16,7 @@ struct tree_node *tree_search(void *key, int insert) { int res; - if (*rootp == NULL) { + if (!*rootp) { if (!insert) { return NULL; } else { @@ -50,7 +50,7 @@ void infix_walk(struct tree_node *t, voi void tree_free(struct tree_node *t) { - if (t == NULL) { + if (!t) { return; } if (t->left) { diff -u -p a/reftable/writer.c b/reftable/writer.c --- a/reftable/writer.c +++ b/reftable/writer.c @@ -183,7 +183,7 @@ static void writer_index_hash(struct ref struct tree_node *node = tree_search(&want, &w->obj_index_tree, &obj_index_tree_node_compare, 0); struct obj_index_tree_node *key = NULL; - if (node == NULL) { + if (!node) { struct obj_index_tree_node empty = OBJ_INDEX_TREE_NODE_INIT; key = reftable_malloc(sizeof(struct obj_index_tree_node)); *key = empty; @@ -222,7 +222,7 @@ static int writer_add_record(struct reft strbuf_reset(&w->last_key); strbuf_addbuf(&w->last_key, &key); - if (w->block_writer == NULL) { + if (!w->block_writer) { writer_reinit_block_writer(w, reftable_record_type(rec)); } @@ -263,7 +263,7 @@ int reftable_writer_add_ref(struct refta }; int err = 0; - if (ref->refname == NULL) + if (!ref->refname) return REFTABLE_API_ERROR; if (ref->update_index < w->min_update_index || ref->update_index > w->max_update_index) @@ -336,7 +336,7 @@ int reftable_writer_add_log(struct refta if (log->value_type == REFTABLE_LOG_DELETION) return reftable_writer_add_log_verbatim(w, log); - if (log->refname == NULL) + if (!log->refname) return REFTABLE_API_ERROR; input_log_message = log->value.update.message; @@ -545,7 +545,7 @@ static int writer_finish_public_section( uint8_t typ = 0; int err = 0; - if (w->block_writer == NULL) + if (!w->block_writer) return 0; typ = block_writer_type(w->block_writer); @@ -694,7 +694,7 @@ static int writer_flush_nonempty_block(s static int writer_flush_block(struct reftable_writer *w) { - if (w->block_writer == NULL) + if (!w->block_writer) return 0; if (w->block_writer->entries == 0) return 0; diff -u -p a/rerere.c b/rerere.c --- a/rerere.c +++ b/rerere.c @@ -591,7 +591,7 @@ int rerere_remaining(struct repository * else if (conflict_type == RESOLVED) { struct string_list_item *it; it = string_list_lookup(merge_rr, (const char *)e->name); - if (it != NULL) { + if (it) { free_rerere_id(it); it->util = RERERE_RESOLVED; } diff -u -p a/revision.c b/revision.c --- a/revision.c +++ b/revision.c @@ -2833,7 +2833,7 @@ int setup_revisions(int argc, const char } strvec_clear(&prune_data); - if (revs->def == NULL) + if (!revs->def) revs->def = opt ? opt->def : NULL; if (opt && opt->tweak) opt->tweak(revs, opt); @@ -3652,7 +3652,7 @@ static enum rewrite_result rewrite_one_1 return rewrite_one_ok; if (!p->parents) return rewrite_one_noparents; - if ((p = one_relevant_parent(revs, p->parents)) == NULL) + if (!(p = one_relevant_parent(revs, p->parents))) return rewrite_one_ok; *pp = p; } diff -u -p a/setup.c b/setup.c --- a/setup.c +++ b/setup.c @@ -1470,7 +1470,7 @@ int git_config_perm(const char *var, con int i; char *endptr; - if (value == NULL) + if (!value) return PERM_GROUP; if (!strcmp(value, "umask")) diff -u -p a/sh-i18n--envsubst.c b/sh-i18n--envsubst.c --- a/sh-i18n--envsubst.c +++ b/sh-i18n--envsubst.c @@ -397,7 +397,7 @@ subst_from_stdin (void) /* Substitute the variable's value from the environment. */ const char *env_value = getenv (buffer); - if (env_value != NULL) + if (env_value) fputs (env_value, stdout); } else diff -u -p a/shallow.c b/shallow.c --- a/shallow.c +++ b/shallow.c @@ -560,7 +560,7 @@ static void paint_down(struct paint_info else c->object.flags |= SEEN; - if (*refs == NULL) + if (!*refs) *refs = bitmap; else { memcpy(tmp, *refs, bitmap_size); diff -u -p a/trailer.c b/trailer.c --- a/trailer.c +++ b/trailer.c @@ -1029,7 +1029,7 @@ static FILE *create_in_place_tempfile(co /* Create temporary file in the same directory as the original */ tail = strrchr(file, '/'); - if (tail != NULL) + if (tail) strbuf_add(&filename_template, file, tail - file + 1); strbuf_addstr(&filename_template, "git-interpret-trailers-XXXXXX"); diff -u -p a/transport.c b/transport.c --- a/transport.c +++ b/transport.c @@ -438,7 +438,7 @@ static int fetch_refs_via_pack(struct tr args.self_contained_and_connected; data->options.connectivity_checked = args.connectivity_checked; - if (refs == NULL) + if (!refs) ret = -1; if (report_unmatched_refs(to_fetch, nr_heads)) ret = -1; diff -u -p a/wildmatch.c b/wildmatch.c --- a/wildmatch.c +++ b/wildmatch.c @@ -113,7 +113,7 @@ static int dowild(const uchar *p, const /* Trailing "**" matches everything. Trailing "*" matches * only if there are no more slash characters. */ if (!match_slash) { - if (strchr((char*)text, '/') != NULL) + if (strchr((char *)text, '/')) return WM_NOMATCH; } return WM_MATCH; diff -u -p a/worktree.c b/worktree.c --- a/worktree.c +++ b/worktree.c @@ -483,7 +483,7 @@ int submodule_uses_worktrees(const char return 0; d = readdir_skip_dot_and_dotdot(dir); - if (d != NULL) + if (d) ret = 1; closedir(dir); return ret; diff -u -p a/wrapper.c b/wrapper.c --- a/wrapper.c +++ b/wrapper.c @@ -393,7 +393,7 @@ FILE *xfopen(const char *path, const cha FILE *xfdopen(int fd, const char *mode) { FILE *stream = fdopen(fd, mode); - if (stream == NULL) + if (!stream) die_errno("Out of memory? fdopen failed"); return stream; } diff -u -p a/xdiff-interface.c b/xdiff-interface.c --- a/xdiff-interface.c +++ b/xdiff-interface.c @@ -159,7 +159,7 @@ int read_mmfile(mmfile_t *ptr, const cha if (stat(filename, &st)) return error_errno("Could not stat %s", filename); - if ((f = fopen(filename, "rb")) == NULL) + if (!(f = fopen(filename, "rb"))) return error_errno("Could not open %s", filename); sz = xsize_t(st.st_size); ptr->ptr = xmalloc(sz ? sz : 1); diff -u -p a/xdiff/xemit.c b/xdiff/xemit.c --- a/xdiff/xemit.c +++ b/xdiff/xemit.c @@ -65,7 +65,7 @@ xdchange_t *xdl_get_hunk(xdchange_t **xs *xscr = xch; } - if (*xscr == NULL) + if (!*xscr) return NULL; lxch = *xscr; diff -u -p a/xdiff/xprepare.c b/xdiff/xprepare.c --- a/xdiff/xprepare.c +++ b/xdiff/xprepare.c @@ -188,7 +188,7 @@ static int xdl_prepare_ctx(unsigned int memset(rhash, 0, hsize * sizeof(xrecord_t *)); nrec = 0; - if ((cur = blk = xdl_mmfile_first(mf, &bsize)) != NULL) { + if ((cur = blk = xdl_mmfile_first(mf, &bsize))) { for (top = blk + bsize; cur < top; ) { prev = cur; hav = xdl_hash_record(&cur, top, xpp->flags); diff -u -p a/xdiff/xutils.c b/xdiff/xutils.c --- a/xdiff/xutils.c +++ b/xdiff/xutils.c @@ -122,7 +122,7 @@ long xdl_guess_lines(mmfile_t *mf, long long nl = 0, size, tsize = 0; char const *data, *cur, *top; - if ((cur = data = xdl_mmfile_first(mf, &size)) != NULL) { + if ((cur = data = xdl_mmfile_first(mf, &size))) { for (top = data + size; nl < sample && cur < top; ) { nl++; if (!(cur = memchr(cur, '\n', top - cur)))
Il giorno dom 1 mag 2022 alle ore 02:20 Junio C Hamano <gitster@pobox.com> ha scritto: > > What I found curious is that the result of applying these patches to > v2.36.0 and running coccicheck reveals that we are not making the > codebase clean wrt this new coccinelle rule. > It is possible, I did not use coccicheck to apply the semantic patch (on next) but i use a my script which I think is slightly more efficient but perhaps it is not so correct. Anyway, given the discussion that has taken place so far, what do you think is best for me to do? Do a reroll (perhaps with only 2 patches in total ) or wait for the "right" moment in the future as foreseen by the Documentation and dedicate the time to more useful contributions for git? Thank you all for the review > > > diff -u -p a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c > --- a/compat/fsmonitor/fsm-listen-darwin.c > +++ b/compat/fsmonitor/fsm-listen-darwin.c > @@ -342,7 +342,7 @@ int fsm_listen__ctor(struct fsmonitor_da > data->cfar_paths_to_watch, > kFSEventStreamEventIdSinceNow, > 0.001, flags); > - if (data->stream == NULL) > + if (!data->stream) > goto failed; > > /* > diff -u -p a/compat/mingw.c b/compat/mingw.c > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -1060,7 +1060,7 @@ char *mingw_mktemp(char *template) > int mkstemp(char *template) > { > char *filename = mktemp(template); > - if (filename == NULL) > + if (!filename) > return -1; > return open(filename, O_RDWR | O_CREAT, 0600); > } > @@ -2332,7 +2332,7 @@ int setitimer(int type, struct itimerval > static const struct timeval zero; > static int atexit_done; > > - if (out != NULL) > + if (out) > return errno = EINVAL, > error("setitimer param 3 != NULL not implemented"); > if (!is_timeval_eq(&in->it_interval, &zero) && > @@ -2361,7 +2361,7 @@ int sigaction(int sig, struct sigaction > if (sig != SIGALRM) > return errno = EINVAL, > error("sigaction only implemented for SIGALRM"); > - if (out != NULL) > + if (out) > return errno = EINVAL, > error("sigaction: param 3 != NULL not implemented"); > > diff -u -p a/compat/mkdir.c b/compat/mkdir.c > --- a/compat/mkdir.c > +++ b/compat/mkdir.c > @@ -9,7 +9,7 @@ int compat_mkdir_wo_trailing_slash(const > size_t len = strlen(dir); > > if (len && dir[len-1] == '/') { > - if ((tmp_dir = strdup(dir)) == NULL) > + if (!(tmp_dir = strdup(dir))) > return -1; > tmp_dir[len-1] = '\0'; > } > diff -u -p a/compat/mmap.c b/compat/mmap.c > --- a/compat/mmap.c > +++ b/compat/mmap.c > @@ -13,7 +13,7 @@ void *git_mmap(void *start, size_t lengt > } > > start = malloc(length); > - if (start == NULL) { > + if (!start) { > errno = ENOMEM; > return MAP_FAILED; > } > diff -u -p a/compat/mingw.c b/compat/mingw.c > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -1060,7 +1060,7 @@ char *mingw_mktemp(char *template) > int mkstemp(char *template) > { > char *filename = mktemp(template); > - if (filename == NULL) > + if (!filename) > return -1; > return open(filename, O_RDWR | O_CREAT, 0600); > } > @@ -2332,7 +2332,7 @@ int setitimer(int type, struct itimerval > static const struct timeval zero; > static int atexit_done; > > - if (out != NULL) > + if (out) > return errno = EINVAL, > error("setitimer param 3 != NULL not implemented"); > if (!is_timeval_eq(&in->it_interval, &zero) && > @@ -2361,7 +2361,7 @@ int sigaction(int sig, struct sigaction > if (sig != SIGALRM) > return errno = EINVAL, > error("sigaction only implemented for SIGALRM"); > - if (out != NULL) > + if (out) > return errno = EINVAL, > error("sigaction: param 3 != NULL not implemented"); > > diff -u -p a/config.c b/config.c > --- a/config.c > +++ b/config.c > @@ -3190,7 +3190,7 @@ int git_config_set_multivar_in_file_gent > goto out_free; > } > /* if nothing to unset, error out */ > - if (value == NULL) { > + if (!value) { > ret = CONFIG_NOTHING_SET; > goto out_free; > } > @@ -3206,7 +3206,7 @@ int git_config_set_multivar_in_file_gent > int i, new_line = 0; > struct config_options opts; > > - if (value_pattern == NULL) > + if (!value_pattern) > store.value_pattern = NULL; > else if (value_pattern == CONFIG_REGEX_NONE) > store.value_pattern = CONFIG_REGEX_NONE; > @@ -3346,7 +3346,7 @@ int git_config_set_multivar_in_file_gent > } > > /* write the pair (value == NULL means unset) */ > - if (value != NULL) { > + if (value) { > if (!store.section_seen) { > if (write_section(fd, key, &store) < 0) > goto write_err_out; > @@ -3567,7 +3567,7 @@ static int git_config_copy_or_rename_sec > offset = section_name_match(&buf[i], old_name); > if (offset > 0) { > ret++; > - if (new_name == NULL) { > + if (!new_name) { > remove = 1; > continue; > } > diff -u -p a/daemon.c b/daemon.c > --- a/daemon.c > +++ b/daemon.c > @@ -447,7 +447,7 @@ static void copy_to_log(int fd) > FILE *fp; > > fp = fdopen(fd, "r"); > - if (fp == NULL) { > + if (!fp) { > logerror("fdopen of error channel failed"); > close(fd); > return; > diff -u -p a/dir.c b/dir.c > --- a/dir.c > +++ b/dir.c > @@ -3054,7 +3054,7 @@ char *git_url_basename(const char *repo, > * Skip scheme. > */ > start = strstr(repo, "://"); > - if (start == NULL) > + if (!start) > start = repo; > else > start += 3; > diff -u -p a/ewah/bitmap.c b/ewah/bitmap.c > --- a/ewah/bitmap.c > +++ b/ewah/bitmap.c > @@ -223,7 +223,7 @@ void bitmap_reset(struct bitmap *bitmap) > > void bitmap_free(struct bitmap *bitmap) > { > - if (bitmap == NULL) > + if (!bitmap) > return; > > free(bitmap->words); > diff -u -p a/ewah/ewah_bitmap.c b/ewah/ewah_bitmap.c > --- a/ewah/ewah_bitmap.c > +++ b/ewah/ewah_bitmap.c > @@ -451,7 +451,7 @@ struct ewah_bitmap *ewah_pool_new(void) > > void ewah_pool_free(struct ewah_bitmap *self) > { > - if (self == NULL) > + if (!self) > return; > > if (bitmap_pool_size == BITMAP_POOL_MAX || > diff -u -p a/http-fetch.c b/http-fetch.c > --- a/http-fetch.c > +++ b/http-fetch.c > @@ -55,7 +55,7 @@ static void fetch_single_packfile(struct > http_init(NULL, url, 0); > > preq = new_direct_http_pack_request(packfile_hash->hash, xstrdup(url)); > - if (preq == NULL) > + if (!preq) > die("couldn't create http pack request"); > preq->slot->results = &results; > preq->index_pack_args = index_pack_args; > diff -u -p a/http-push.c b/http-push.c > --- a/http-push.c > +++ b/http-push.c > @@ -253,7 +253,7 @@ static void start_fetch_loose(struct tra > struct http_object_request *obj_req; > > obj_req = new_http_object_request(repo->url, &request->obj->oid); > - if (obj_req == NULL) { > + if (!obj_req) { > request->state = ABORTED; > return; > } > @@ -318,7 +318,7 @@ static void start_fetch_packed(struct tr > fprintf(stderr, " which contains %s\n", oid_to_hex(&request->obj->oid)); > > preq = new_http_pack_request(target->hash, repo->url); > - if (preq == NULL) { > + if (!preq) { > repo->can_update_info_refs = 0; > return; > } > @@ -520,7 +520,7 @@ static void finish_request(struct transf > /* Keep locks active */ > check_locks(); > > - if (request->headers != NULL) > + if (request->headers) > curl_slist_free_all(request->headers); > > /* URL is reused for MOVE after PUT and used during FETCH */ > @@ -783,7 +783,7 @@ xml_start_tag(void *userData, const char > const char *c = strchr(name, ':'); > int old_namelen, new_len; > > - if (c == NULL) > + if (!c) > c = name; > else > c++; > @@ -811,7 +811,7 @@ xml_end_tag(void *userData, const char * > > ctx->userFunc(ctx, 1); > > - if (c == NULL) > + if (!c) > c = name; > else > c++; > @@ -1893,7 +1893,7 @@ int cmd_main(int argc, const char **argv > > /* Lock remote branch ref */ > ref_lock = lock_remote(ref->name, LOCK_TIME); > - if (ref_lock == NULL) { > + if (!ref_lock) { > fprintf(stderr, "Unable to lock remote branch %s\n", > ref->name); > if (helper_status) > diff -u -p a/http-walker.c b/http-walker.c > --- a/http-walker.c > +++ b/http-walker.c > @@ -59,7 +59,7 @@ static void start_object_request(struct > struct http_object_request *req; > > req = new_http_object_request(obj_req->repo->base, &obj_req->oid); > - if (req == NULL) { > + if (!req) { > obj_req->state = ABORTED; > return; > } > @@ -106,7 +106,7 @@ static void process_object_response(void > /* Use alternates if necessary */ > if (missing_target(obj_req->req)) { > fetch_alternates(walker, alt->base); > - if (obj_req->repo->next != NULL) { > + if (obj_req->repo->next) { > obj_req->repo = > obj_req->repo->next; > release_http_object_request(obj_req->req); > @@ -225,12 +225,12 @@ static void process_alternates_response( > alt_req->url->buf); > active_requests++; > slot->in_use = 1; > - if (slot->finished != NULL) > + if (slot->finished) > (*slot->finished) = 0; > if (!start_active_slot(slot)) { > cdata->got_alternates = -1; > slot->in_use = 0; > - if (slot->finished != NULL) > + if (slot->finished) > (*slot->finished) = 1; > } > return; > @@ -443,7 +443,7 @@ static int http_fetch_pack(struct walker > } > > preq = new_http_pack_request(target->hash, repo->base); > - if (preq == NULL) > + if (!preq) > goto abort; > preq->slot->results = &results; > > @@ -489,11 +489,11 @@ static int fetch_object(struct walker *w > if (hasheq(obj_req->oid.hash, hash)) > break; > } > - if (obj_req == NULL) > + if (!obj_req) > return error("Couldn't find request for %s in the queue", hex); > > if (has_object_file(&obj_req->oid)) { > - if (obj_req->req != NULL) > + if (obj_req->req) > abort_http_object_request(obj_req->req); > abort_object_request(obj_req); > return 0; > diff -u -p a/http.c b/http.c > --- a/http.c > +++ b/http.c > @@ -197,11 +197,11 @@ static void finish_active_slot(struct ac > closedown_active_slot(slot); > curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CODE, &slot->http_code); > > - if (slot->finished != NULL) > + if (slot->finished) > (*slot->finished) = 1; > > /* Store slot results so they can be read after the slot is reused */ > - if (slot->results != NULL) { > + if (slot->results) { > slot->results->curl_result = slot->curl_result; > slot->results->http_code = slot->http_code; > curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL, > @@ -212,7 +212,7 @@ static void finish_active_slot(struct ac > } > > /* Run callback if appropriate */ > - if (slot->callback_func != NULL) > + if (slot->callback_func) > slot->callback_func(slot->callback_data); > } > > @@ -234,7 +234,7 @@ static void process_curl_messages(void) > while (slot != NULL && > slot->curl != curl_message->easy_handle) > slot = slot->next; > - if (slot != NULL) { > + if (slot) { > xmulti_remove_handle(slot); > slot->curl_result = curl_result; > finish_active_slot(slot); > @@ -838,16 +838,16 @@ static CURL *get_curl_handle(void) > curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST, > ssl_cipherlist); > > - if (ssl_cert != NULL) > + if (ssl_cert) > curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert); > if (has_cert_password()) > curl_easy_setopt(result, CURLOPT_KEYPASSWD, cert_auth.password); > - if (ssl_key != NULL) > + if (ssl_key) > curl_easy_setopt(result, CURLOPT_SSLKEY, ssl_key); > - if (ssl_capath != NULL) > + if (ssl_capath) > curl_easy_setopt(result, CURLOPT_CAPATH, ssl_capath); > #ifdef GIT_CURL_HAVE_CURLOPT_PINNEDPUBLICKEY > - if (ssl_pinnedkey != NULL) > + if (ssl_pinnedkey) > curl_easy_setopt(result, CURLOPT_PINNEDPUBLICKEY, ssl_pinnedkey); > #endif > if (http_ssl_backend && !strcmp("schannel", http_ssl_backend) && > @@ -857,10 +857,10 @@ static CURL *get_curl_handle(void) > curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, NULL); > #endif > } else if (ssl_cainfo != NULL || http_proxy_ssl_ca_info != NULL) { > - if (ssl_cainfo != NULL) > + if (ssl_cainfo) > curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo); > #ifdef GIT_CURL_HAVE_CURLOPT_PROXY_CAINFO > - if (http_proxy_ssl_ca_info != NULL) > + if (http_proxy_ssl_ca_info) > curl_easy_setopt(result, CURLOPT_PROXY_CAINFO, http_proxy_ssl_ca_info); > #endif > } > @@ -1050,7 +1050,7 @@ void http_init(struct remote *remote, co > > { > char *http_max_requests = getenv("GIT_HTTP_MAX_REQUESTS"); > - if (http_max_requests != NULL) > + if (http_max_requests) > max_requests = atoi(http_max_requests); > } > > @@ -1069,10 +1069,10 @@ void http_init(struct remote *remote, co > set_from_env(&user_agent, "GIT_HTTP_USER_AGENT"); > > low_speed_limit = getenv("GIT_HTTP_LOW_SPEED_LIMIT"); > - if (low_speed_limit != NULL) > + if (low_speed_limit) > curl_low_speed_limit = strtol(low_speed_limit, NULL, 10); > low_speed_time = getenv("GIT_HTTP_LOW_SPEED_TIME"); > - if (low_speed_time != NULL) > + if (low_speed_time) > curl_low_speed_time = strtol(low_speed_time, NULL, 10); > > if (curl_ssl_verify == -1) > @@ -1109,7 +1109,7 @@ void http_cleanup(void) > > while (slot != NULL) { > struct active_request_slot *next = slot->next; > - if (slot->curl != NULL) { > + if (slot->curl) { > xmulti_remove_handle(slot); > curl_easy_cleanup(slot->curl); > } > @@ -1147,13 +1147,13 @@ void http_cleanup(void) > free((void *)http_proxy_authmethod); > http_proxy_authmethod = NULL; > > - if (cert_auth.password != NULL) { > + if (cert_auth.password) { > memset(cert_auth.password, 0, strlen(cert_auth.password)); > FREE_AND_NULL(cert_auth.password); > } > ssl_cert_password_required = 0; > > - if (proxy_cert_auth.password != NULL) { > + if (proxy_cert_auth.password) { > memset(proxy_cert_auth.password, 0, strlen(proxy_cert_auth.password)); > FREE_AND_NULL(proxy_cert_auth.password); > } > @@ -1179,14 +1179,14 @@ struct active_request_slot *get_active_s > while (slot != NULL && slot->in_use) > slot = slot->next; > > - if (slot == NULL) { > + if (!slot) { > newslot = xmalloc(sizeof(*newslot)); > newslot->curl = NULL; > newslot->in_use = 0; > newslot->next = NULL; > > slot = active_queue_head; > - if (slot == NULL) { > + if (!slot) { > active_queue_head = newslot; > } else { > while (slot->next != NULL) > @@ -1196,7 +1196,7 @@ struct active_request_slot *get_active_s > slot = newslot; > } > > - if (slot->curl == NULL) { > + if (!slot->curl) { > slot->curl = curl_easy_duphandle(curl_default); > curl_session_count++; > } > @@ -1768,7 +1768,7 @@ static int http_request(const char *url, > slot = get_active_slot(); > curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1); > > - if (result == NULL) { > + if (!result) { > curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1); > } else { > curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0); > @@ -2100,7 +2100,7 @@ cleanup: > > void release_http_pack_request(struct http_pack_request *preq) > { > - if (preq->packfile != NULL) { > + if (preq->packfile) { > fclose(preq->packfile); > preq->packfile = NULL; > } > @@ -2391,7 +2391,7 @@ abort: > > void process_http_object_request(struct http_object_request *freq) > { > - if (freq->slot == NULL) > + if (!freq->slot) > return; > freq->curl_result = freq->slot->curl_result; > freq->http_code = freq->slot->http_code; > @@ -2448,7 +2448,7 @@ void release_http_object_request(struct > freq->localfile = -1; > } > FREE_AND_NULL(freq->url); > - if (freq->slot != NULL) { > + if (freq->slot) { > freq->slot->callback_func = NULL; > freq->slot->callback_data = NULL; > release_active_slot(freq->slot); > diff -u -p a/kwset.c b/kwset.c > --- a/kwset.c > +++ b/kwset.c > @@ -477,7 +477,7 @@ kwsprep (kwset_t kws) > next[i] = NULL; > treenext(kwset->trie->links, next); > > - if ((trans = kwset->trans) != NULL) > + if ((trans = kwset->trans)) > for (i = 0; i < NCHAR; ++i) > kwset->next[i] = next[U(trans[i])]; > else > @@ -485,7 +485,7 @@ kwsprep (kwset_t kws) > } > > /* Fix things up for any translation table. */ > - if ((trans = kwset->trans) != NULL) > + if ((trans = kwset->trans)) > for (i = 0; i < NCHAR; ++i) > kwset->delta[i] = delta[U(trans[i])]; > else > diff -u -p a/ll-merge.c b/ll-merge.c > --- a/ll-merge.c > +++ b/ll-merge.c > @@ -207,7 +207,7 @@ static enum ll_merge_result ll_ext_merge > dict[4].placeholder = "P"; dict[4].value = path_sq.buf; > dict[5].placeholder = NULL; dict[5].value = NULL; > > - if (fn->cmdline == NULL) > + if (!fn->cmdline) > die("custom merge driver %s lacks command line.", fn->name); > > result->ptr = NULL; > diff -u -p a/log-tree.c b/log-tree.c > --- a/log-tree.c > +++ b/log-tree.c > @@ -88,7 +88,7 @@ static int match_ref_pattern(const char > const struct string_list_item *item) > { > int matched = 0; > - if (item->util == NULL) { > + if (!item->util) { > if (!wildmatch(item->string, refname, 0)) > matched = 1; > } else { > diff -u -p a/mailinfo.c b/mailinfo.c > --- a/mailinfo.c > +++ b/mailinfo.c > @@ -698,7 +698,7 @@ static int is_scissors_line(const char * > continue; > } > last_nonblank = c; > - if (first_nonblank == NULL) > + if (!first_nonblank) > first_nonblank = c; > if (*c == '-') { > in_perforation = 1; > @@ -1094,7 +1094,7 @@ static void handle_body(struct mailinfo > */ > lines = strbuf_split(line, '\n'); > for (it = lines; (sb = *it); it++) { > - if (*(it + 1) == NULL) /* The last line */ > + if (!*(it + 1)) /* The last line */ > if (sb->buf[sb->len - 1] != '\n') { > /* Partial line, save it for later. */ > strbuf_addbuf(&prev, sb); > diff -u -p a/mailmap.c b/mailmap.c > --- a/mailmap.c > +++ b/mailmap.c > @@ -77,7 +77,7 @@ static void add_mapping(struct string_li > struct mailmap_entry *me; > struct string_list_item *item; > > - if (old_email == NULL) { > + if (!old_email) { > old_email = new_email; > new_email = NULL; > } > @@ -92,7 +92,7 @@ static void add_mapping(struct string_li > item->util = me; > } > > - if (old_name == NULL) { > + if (!old_name) { > debug_mm("mailmap: adding (simple) entry for '%s'\n", old_email); > > /* Replace current name and new email for simple entry */ > @@ -123,9 +123,9 @@ static char *parse_name_and_email(char * > char *left, *right, *nstart, *nend; > *name = *email = NULL; > > - if ((left = strchr(buffer, '<')) == NULL) > + if (!(left = strchr(buffer, '<'))) > return NULL; > - if ((right = strchr(left+1, '>')) == NULL) > + if (!(right = strchr(left + 1, '>'))) > return NULL; > if (!allow_empty_email && (left+1 == right)) > return NULL; > @@ -153,7 +153,7 @@ static void read_mailmap_line(struct str > if (buffer[0] == '#') > return; > > - if ((name2 = parse_name_and_email(buffer, &name1, &email1, 0)) != NULL) > + if ((name2 = parse_name_and_email(buffer, &name1, &email1, 0))) > parse_name_and_email(name2, &name2, &email2, 1); > > if (email1) > @@ -320,7 +320,7 @@ int map_user(struct string_list *map, > (int)*emaillen, debug_str(*email)); > > item = lookup_prefix(map, *email, *emaillen); > - if (item != NULL) { > + if (item) { > me = (struct mailmap_entry *)item->util; > if (me->namemap.nr) { > /* > @@ -334,7 +334,7 @@ int map_user(struct string_list *map, > item = subitem; > } > } > - if (item != NULL) { > + if (item) { > struct mailmap_info *mi = (struct mailmap_info *)item->util; > if (mi->name == NULL && mi->email == NULL) { > debug_mm("map_user: -- (no simple mapping)\n"); > diff -u -p a/merge-ort.c b/merge-ort.c > --- a/merge-ort.c > +++ b/merge-ort.c > @@ -2068,7 +2068,7 @@ static char *handle_path_level_conflicts > * to ensure that's the case. > */ > c_info = strmap_get(collisions, new_path); > - if (c_info == NULL) > + if (!c_info) > BUG("c_info is NULL"); > > /* > @@ -4640,7 +4640,7 @@ static void merge_ort_internal(struct me > } > > merged_merge_bases = pop_commit(&merge_bases); > - if (merged_merge_bases == NULL) { > + if (!merged_merge_bases) { > /* if there is no common ancestor, use an empty tree */ > struct tree *tree; > > diff -u -p a/merge-recursive.c b/merge-recursive.c > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -82,7 +82,7 @@ static struct dir_rename_entry *dir_rena > { > struct dir_rename_entry key; > > - if (dir == NULL) > + if (!dir) > return NULL; > hashmap_entry_init(&key.ent, strhash(dir)); > key.dir = dir; > @@ -1990,14 +1990,14 @@ static void get_renamed_dir_portion(cons > * renamed means the root directory can never be renamed -- because > * the root directory always exists). > */ > - if (end_of_old == NULL) > + if (!end_of_old) > return; /* Note: *old_dir and *new_dir are still NULL */ > > /* > * If new_path contains no directory (end_of_new is NULL), then we > * have a rename of old_path's directory to the root directory. > */ > - if (end_of_new == NULL) { > + if (!end_of_new) { > *old_dir = xstrndup(old_path, end_of_old - old_path); > *new_dir = xstrdup(""); > return; > @@ -2116,7 +2116,7 @@ static char *handle_path_level_conflicts > * to ensure that's the case. > */ > collision_ent = collision_find_entry(collisions, new_path); > - if (collision_ent == NULL) > + if (!collision_ent) > BUG("collision_ent is NULL"); > > /* > @@ -2996,7 +2996,7 @@ static void final_cleanup_rename(struct > const struct rename *re; > int i; > > - if (rename == NULL) > + if (!rename) > return; > > for (i = 0; i < rename->nr; i++) { > @@ -3605,7 +3605,7 @@ static int merge_recursive_internal(stru > } > > merged_merge_bases = pop_commit(&merge_bases); > - if (merged_merge_bases == NULL) { > + if (!merged_merge_bases) { > /* if there is no common ancestor, use an empty tree */ > struct tree *tree; > > diff -u -p a/object-file.c b/object-file.c > --- a/object-file.c > +++ b/object-file.c > @@ -1728,7 +1728,7 @@ void *read_object_file_extended(struct r > die(_("loose object %s (stored in %s) is corrupt"), > oid_to_hex(repl), path); > > - if ((p = has_packed_and_bad(r, repl)) != NULL) > + if ((p = has_packed_and_bad(r, repl))) > die(_("packed object %s (stored in %s) is corrupt"), > oid_to_hex(repl), p->pack_name); > obj_read_unlock(); > diff -u -p a/pack-bitmap.c b/pack-bitmap.c > --- a/pack-bitmap.c > +++ b/pack-bitmap.c > @@ -111,7 +111,7 @@ static struct ewah_bitmap *lookup_stored > struct ewah_bitmap *parent; > struct ewah_bitmap *composed; > > - if (st->xor == NULL) > + if (!st->xor) > return st->root; > > composed = ewah_pool_new(); > @@ -279,7 +279,7 @@ static int load_bitmap_entries_v1(struct > if (xor_offset > 0) { > xor_bitmap = recent_bitmaps[(i - xor_offset) % MAX_XOR_OFFSET]; > > - if (xor_bitmap == NULL) > + if (!xor_bitmap) > return error("Invalid XOR offset in bitmap pack index"); > } > > @@ -728,7 +728,7 @@ static int add_commit_to_bitmap(struct b > if (!or_with) > return 0; > > - if (*base == NULL) > + if (!*base) > *base = ewah_to_bitmap(or_with); > else > bitmap_or_ewah(*base, or_with); > @@ -771,7 +771,7 @@ static struct bitmap *find_objects(struc > * Best case scenario: We found bitmaps for all the roots, > * so the resulting `or` bitmap has the full reachability analysis > */ > - if (not_mapped == NULL) > + if (!not_mapped) > return base; > > roots = not_mapped; > @@ -805,7 +805,7 @@ static struct bitmap *find_objects(struc > struct include_data incdata; > struct bitmap_show_data show_data; > > - if (base == NULL) > + if (!base) > base = bitmap_new(); > > incdata.bitmap_git = bitmap_git; > @@ -1299,7 +1299,7 @@ struct bitmap_index *prepare_bitmap_walk > reset_revision_walk(); > revs->ignore_missing_links = 0; > > - if (haves_bitmap == NULL) > + if (!haves_bitmap) > BUG("failed to perform bitmap walk"); > } > > @@ -1698,7 +1698,7 @@ void test_bitmap_walk(struct rev_info *r > result = ewah_to_bitmap(bm); > } > > - if (result == NULL) > + if (!result) > die("Commit %s doesn't have an indexed bitmap", oid_to_hex(&root->oid)); > > revs->tag_objects = 1; > diff -u -p a/packfile.c b/packfile.c > --- a/packfile.c > +++ b/packfile.c > @@ -116,7 +116,7 @@ int load_idx(const char *path, const uns > > if (idx_size < 4 * 256 + hashsz + hashsz) > return error("index file %s is too small", path); > - if (idx_map == NULL) > + if (!idx_map) > return error("empty data"); > > if (hdr->idx_signature == htonl(PACK_IDX_SIGNATURE)) { > diff -u -p a/path.c b/path.c > --- a/path.c > +++ b/path.c > @@ -733,7 +733,7 @@ char *interpolate_path(const char *path, > struct strbuf user_path = STRBUF_INIT; > const char *to_copy = path; > > - if (path == NULL) > + if (!path) > goto return_null; > > if (skip_prefix(path, "%(prefix)/", &path)) > diff -u -p a/prio-queue.c b/prio-queue.c > --- a/prio-queue.c > +++ b/prio-queue.c > @@ -19,7 +19,7 @@ void prio_queue_reverse(struct prio_queu > { > int i, j; > > - if (queue->compare != NULL) > + if (queue->compare) > BUG("prio_queue_reverse() on non-LIFO queue"); > for (i = 0; i < (j = (queue->nr - 1) - i); i++) > swap(queue, i, j); > diff -u -p a/promisor-remote.c b/promisor-remote.c > --- a/promisor-remote.c > +++ b/promisor-remote.c > @@ -84,7 +84,7 @@ static void promisor_remote_move_to_tail > struct promisor_remote *r, > struct promisor_remote *previous) > { > - if (r->next == NULL) > + if (!r->next) > return; > > if (previous) > diff -u -p a/ref-filter.c b/ref-filter.c > --- a/ref-filter.c > +++ b/ref-filter.c > @@ -1261,7 +1261,7 @@ static void grab_date(const char *buf, s > * ":" means no format is specified, and use the default. > */ > formatp = strchr(atomname, ':'); > - if (formatp != NULL) { > + if (formatp) { > formatp++; > parse_date_format(formatp, &date_mode); > } > @@ -1509,7 +1509,7 @@ static void fill_missing_values(struct a > int i; > for (i = 0; i < used_atom_cnt; i++) { > struct atom_value *v = &val[i]; > - if (v->s == NULL) > + if (!v->s) > v->s = xstrdup(""); > } > } > @@ -1619,7 +1619,7 @@ static const char *rstrip_ref_components > > while (remaining-- > 0) { > char *p = strrchr(start, '/'); > - if (p == NULL) { > + if (!p) { > free((char *)to_free); > return xstrdup(""); > } else > diff -u -p a/refs/ref-cache.c b/refs/ref-cache.c > --- a/refs/ref-cache.c > +++ b/refs/ref-cache.c > @@ -134,7 +134,7 @@ int search_ref_dir(struct ref_dir *dir, > r = bsearch(&key, dir->entries, dir->nr, sizeof(*dir->entries), > ref_entry_cmp_sslice); > > - if (r == NULL) > + if (!r) > return -1; > > return r - dir->entries; > diff -u -p a/reftable/stack_test.c b/reftable/stack_test.c > --- a/reftable/stack_test.c > +++ b/reftable/stack_test.c > @@ -35,7 +35,7 @@ static int count_dir_entries(const char > DIR *dir = opendir(dirname); > int len = 0; > struct dirent *d; > - if (dir == NULL) > + if (!dir) > return 0; > > while ((d = readdir(dir))) { > diff -u -p a/reftable/tree.c b/reftable/tree.c > --- a/reftable/tree.c > +++ b/reftable/tree.c > @@ -16,7 +16,7 @@ struct tree_node *tree_search(void *key, > int insert) > { > int res; > - if (*rootp == NULL) { > + if (!*rootp) { > if (!insert) { > return NULL; > } else { > @@ -50,7 +50,7 @@ void infix_walk(struct tree_node *t, voi > > void tree_free(struct tree_node *t) > { > - if (t == NULL) { > + if (!t) { > return; > } > if (t->left) { > diff -u -p a/reftable/writer.c b/reftable/writer.c > --- a/reftable/writer.c > +++ b/reftable/writer.c > @@ -183,7 +183,7 @@ static void writer_index_hash(struct ref > struct tree_node *node = tree_search(&want, &w->obj_index_tree, > &obj_index_tree_node_compare, 0); > struct obj_index_tree_node *key = NULL; > - if (node == NULL) { > + if (!node) { > struct obj_index_tree_node empty = OBJ_INDEX_TREE_NODE_INIT; > key = reftable_malloc(sizeof(struct obj_index_tree_node)); > *key = empty; > @@ -222,7 +222,7 @@ static int writer_add_record(struct reft > > strbuf_reset(&w->last_key); > strbuf_addbuf(&w->last_key, &key); > - if (w->block_writer == NULL) { > + if (!w->block_writer) { > writer_reinit_block_writer(w, reftable_record_type(rec)); > } > > @@ -263,7 +263,7 @@ int reftable_writer_add_ref(struct refta > }; > int err = 0; > > - if (ref->refname == NULL) > + if (!ref->refname) > return REFTABLE_API_ERROR; > if (ref->update_index < w->min_update_index || > ref->update_index > w->max_update_index) > @@ -336,7 +336,7 @@ int reftable_writer_add_log(struct refta > if (log->value_type == REFTABLE_LOG_DELETION) > return reftable_writer_add_log_verbatim(w, log); > > - if (log->refname == NULL) > + if (!log->refname) > return REFTABLE_API_ERROR; > > input_log_message = log->value.update.message; > @@ -545,7 +545,7 @@ static int writer_finish_public_section( > uint8_t typ = 0; > int err = 0; > > - if (w->block_writer == NULL) > + if (!w->block_writer) > return 0; > > typ = block_writer_type(w->block_writer); > @@ -694,7 +694,7 @@ static int writer_flush_nonempty_block(s > > static int writer_flush_block(struct reftable_writer *w) > { > - if (w->block_writer == NULL) > + if (!w->block_writer) > return 0; > if (w->block_writer->entries == 0) > return 0; > diff -u -p a/rerere.c b/rerere.c > --- a/rerere.c > +++ b/rerere.c > @@ -591,7 +591,7 @@ int rerere_remaining(struct repository * > else if (conflict_type == RESOLVED) { > struct string_list_item *it; > it = string_list_lookup(merge_rr, (const char *)e->name); > - if (it != NULL) { > + if (it) { > free_rerere_id(it); > it->util = RERERE_RESOLVED; > } > diff -u -p a/revision.c b/revision.c > --- a/revision.c > +++ b/revision.c > @@ -2833,7 +2833,7 @@ int setup_revisions(int argc, const char > } > strvec_clear(&prune_data); > > - if (revs->def == NULL) > + if (!revs->def) > revs->def = opt ? opt->def : NULL; > if (opt && opt->tweak) > opt->tweak(revs, opt); > @@ -3652,7 +3652,7 @@ static enum rewrite_result rewrite_one_1 > return rewrite_one_ok; > if (!p->parents) > return rewrite_one_noparents; > - if ((p = one_relevant_parent(revs, p->parents)) == NULL) > + if (!(p = one_relevant_parent(revs, p->parents))) > return rewrite_one_ok; > *pp = p; > } > diff -u -p a/setup.c b/setup.c > --- a/setup.c > +++ b/setup.c > @@ -1470,7 +1470,7 @@ int git_config_perm(const char *var, con > int i; > char *endptr; > > - if (value == NULL) > + if (!value) > return PERM_GROUP; > > if (!strcmp(value, "umask")) > diff -u -p a/sh-i18n--envsubst.c b/sh-i18n--envsubst.c > --- a/sh-i18n--envsubst.c > +++ b/sh-i18n--envsubst.c > @@ -397,7 +397,7 @@ subst_from_stdin (void) > /* Substitute the variable's value from the environment. */ > const char *env_value = getenv (buffer); > > - if (env_value != NULL) > + if (env_value) > fputs (env_value, stdout); > } > else > diff -u -p a/shallow.c b/shallow.c > --- a/shallow.c > +++ b/shallow.c > @@ -560,7 +560,7 @@ static void paint_down(struct paint_info > else > c->object.flags |= SEEN; > > - if (*refs == NULL) > + if (!*refs) > *refs = bitmap; > else { > memcpy(tmp, *refs, bitmap_size); > diff -u -p a/trailer.c b/trailer.c > --- a/trailer.c > +++ b/trailer.c > @@ -1029,7 +1029,7 @@ static FILE *create_in_place_tempfile(co > > /* Create temporary file in the same directory as the original */ > tail = strrchr(file, '/'); > - if (tail != NULL) > + if (tail) > strbuf_add(&filename_template, file, tail - file + 1); > strbuf_addstr(&filename_template, "git-interpret-trailers-XXXXXX"); > > diff -u -p a/transport.c b/transport.c > --- a/transport.c > +++ b/transport.c > @@ -438,7 +438,7 @@ static int fetch_refs_via_pack(struct tr > args.self_contained_and_connected; > data->options.connectivity_checked = args.connectivity_checked; > > - if (refs == NULL) > + if (!refs) > ret = -1; > if (report_unmatched_refs(to_fetch, nr_heads)) > ret = -1; > diff -u -p a/wildmatch.c b/wildmatch.c > --- a/wildmatch.c > +++ b/wildmatch.c > @@ -113,7 +113,7 @@ static int dowild(const uchar *p, const > /* Trailing "**" matches everything. Trailing "*" matches > * only if there are no more slash characters. */ > if (!match_slash) { > - if (strchr((char*)text, '/') != NULL) > + if (strchr((char *)text, '/')) > return WM_NOMATCH; > } > return WM_MATCH; > diff -u -p a/worktree.c b/worktree.c > --- a/worktree.c > +++ b/worktree.c > @@ -483,7 +483,7 @@ int submodule_uses_worktrees(const char > return 0; > > d = readdir_skip_dot_and_dotdot(dir); > - if (d != NULL) > + if (d) > ret = 1; > closedir(dir); > return ret; > diff -u -p a/wrapper.c b/wrapper.c > --- a/wrapper.c > +++ b/wrapper.c > @@ -393,7 +393,7 @@ FILE *xfopen(const char *path, const cha > FILE *xfdopen(int fd, const char *mode) > { > FILE *stream = fdopen(fd, mode); > - if (stream == NULL) > + if (!stream) > die_errno("Out of memory? fdopen failed"); > return stream; > } > diff -u -p a/xdiff-interface.c b/xdiff-interface.c > --- a/xdiff-interface.c > +++ b/xdiff-interface.c > @@ -159,7 +159,7 @@ int read_mmfile(mmfile_t *ptr, const cha > > if (stat(filename, &st)) > return error_errno("Could not stat %s", filename); > - if ((f = fopen(filename, "rb")) == NULL) > + if (!(f = fopen(filename, "rb"))) > return error_errno("Could not open %s", filename); > sz = xsize_t(st.st_size); > ptr->ptr = xmalloc(sz ? sz : 1); > diff -u -p a/xdiff/xemit.c b/xdiff/xemit.c > --- a/xdiff/xemit.c > +++ b/xdiff/xemit.c > @@ -65,7 +65,7 @@ xdchange_t *xdl_get_hunk(xdchange_t **xs > *xscr = xch; > } > > - if (*xscr == NULL) > + if (!*xscr) > return NULL; > > lxch = *xscr; > diff -u -p a/xdiff/xprepare.c b/xdiff/xprepare.c > --- a/xdiff/xprepare.c > +++ b/xdiff/xprepare.c > @@ -188,7 +188,7 @@ static int xdl_prepare_ctx(unsigned int > memset(rhash, 0, hsize * sizeof(xrecord_t *)); > > nrec = 0; > - if ((cur = blk = xdl_mmfile_first(mf, &bsize)) != NULL) { > + if ((cur = blk = xdl_mmfile_first(mf, &bsize))) { > for (top = blk + bsize; cur < top; ) { > prev = cur; > hav = xdl_hash_record(&cur, top, xpp->flags); > diff -u -p a/xdiff/xutils.c b/xdiff/xutils.c > --- a/xdiff/xutils.c > +++ b/xdiff/xutils.c > @@ -122,7 +122,7 @@ long xdl_guess_lines(mmfile_t *mf, long > long nl = 0, size, tsize = 0; > char const *data, *cur, *top; > > - if ((cur = data = xdl_mmfile_first(mf, &size)) != NULL) { > + if ((cur = data = xdl_mmfile_first(mf, &size))) { > for (top = data + size; nl < sample && cur < top; ) { > nl++; > if (!(cur = memchr(cur, '\n', top - cur)))
On 01/05/2022 18:04, Elia Pinto wrote: > Il giorno dom 1 mag 2022 alle ore 02:20 Junio C Hamano > <gitster@pobox.com> ha scritto: >> What I found curious is that the result of applying these patches to >> v2.36.0 and running coccicheck reveals that we are not making the >> codebase clean wrt this new coccinelle rule. >> > It is possible, I did not use coccicheck to apply the semantic patch > (on next) but i use a my script which I think is slightly more > efficient but perhaps it is not so correct. Anyway, given the > discussion that has taken place so far, what do you think is best for > me to do? Do a reroll (perhaps with only 2 patches in total ) or wait > for the "right" moment in the future as foreseen by the Documentation > and dedicate the time to more useful contributions for git? Thank you > all for the review > Hi Elia, Given Junio's comment regarding the potential for patch churn, it may, as an alternative, be possible to create a script that will check/extract just those those changes that are either: A) mistakes in patches between say master and seen `master..seen` so that they aren't incorporated, though `next..seen` may be more appropriate. B) detect 'while you are at it' fixes that could be applied to a file that is being modified with the same range query. The file fix-up patch could/would then be inserted as preparatory step to the relevant series (i.e. fed to the series author), allowing easy reversion with/without the fix-up. C) suggest to coccinelle that maybe they include a similar Git based range check option, to avoid needing a fancy script [using Junio's rationale]. D) more clearly identify _why_ the particular instances that are corrected are _worth_ changing now (e.g. fixing _one_ may be a worthy GSOC or Outreachy activity, or .. <insert own reasons>). Hope that helps. Philip >> diff -u -p a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c >> --- a/compat/fsmonitor/fsm-listen-darwin.c >> +++ b/compat/fsmonitor/fsm-listen-darwin.c >> @@ -342,7 +342,7 @@ int fsm_listen__ctor(struct fsmonitor_da >> data->cfar_paths_to_watch, >> kFSEventStreamEventIdSinceNow, >> 0.001, flags); >> - if (data->stream == NULL) >> + if (!data->stream) >> goto failed; [snipped]
Elia Pinto <gitter.spiros@gmail.com> writes: >> What I found curious is that the result of applying these patches to >> v2.36.0 and running coccicheck reveals that we are not making the >> codebase clean wrt this new coccinelle rule. >> > It is possible, I did not use coccicheck to apply the semantic patch > (on next) but i use a my script which I think is slightly more > efficient but perhaps it is not so correct. Anyway, given the > discussion that has taken place so far, what do you think is best for > me to do? Do a reroll (perhaps with only 2 patches in total ) or wait > for the "right" moment in the future as foreseen by the Documentation > and dedicate the time to more useful contributions for git? Thank you > all for the review Hmph. Even if these patches were created by coccicheck we should sanity check them to make sure we are not applying some stupid and obvious mistakes, but if they were created by an ad-hoc tool, it means we would need to check a lot more careful than a patch that was done with a known tool with a clear rule (which is what running "make coccicheck" with your new rule file would have given us). To avoid unnecessary conflicts with in-flight topics, ideally, we perhaps could do something along this line: * Pick a recent stable point that is an ancestor of all topics in flight. Add the new coccinelle rule file, take "make coccicheck" output and create a two-patch series like Philip suggested. Queue the result in a topic branch B. * For each topic in flight T, make a trial merge of T into B, and examine "make coccicheck" output. Any new breakages such a test finds are new violations the topic T introduces. Discard the result of the trial merge, and add one commit to topic T that corrects the violations the topic introduced, and send that fixup to the author of the topic for consideration when the topic is rerolled (or if the topic is in 'next', acked to be queued on top). Do not fix the violations that is corrected when branch B was prepared above. As I assumed that applying the patches in this series would create the branch B, and then I saw that the tip of 'seen' after merging this topic still needed to have a lot more fixes according to "make coccicheck", I got a (false) impression that there are too many new violations from topics in flight, which was the primary source of my negative reaction against potential code churn. If we try the above exercise, perhaps there may not be too many topics that need fix-up beyond what we fix in the branch B, and if that is the case, I would not be so negative. Thanks.
Il giorno lun 2 mag 2022 alle ore 01:14 Junio C Hamano <gitster@pobox.com> ha scritto: > > Elia Pinto <gitter.spiros@gmail.com> writes: > > >> What I found curious is that the result of applying these patches to > >> v2.36.0 and running coccicheck reveals that we are not making the > >> codebase clean wrt this new coccinelle rule. > >> > > It is possible, I did not use coccicheck to apply the semantic patch > > (on next) but i use a my script which I think is slightly more > > efficient but perhaps it is not so correct. Anyway, given the > > discussion that has taken place so far, what do you think is best for > > me to do? Do a reroll (perhaps with only 2 patches in total ) or wait > > for the "right" moment in the future as foreseen by the Documentation > > and dedicate the time to more useful contributions for git? Thank you > > all for the review > > Hmph. Even if these patches were created by coccicheck we should > sanity check them to make sure we are not applying some stupid and > obvious mistakes, but if they were created by an ad-hoc tool, it > means we would need to check a lot more careful than a patch that > was done with a known tool with a clear rule (which is what running > "make coccicheck" with your new rule file would have given us). > > To avoid unnecessary conflicts with in-flight topics, ideally, we > perhaps could do something along this line: > > * Pick a recent stable point that is an ancestor of all topics in > flight. Add the new coccinelle rule file, take "make coccicheck" > output and create a two-patch series like Philip suggested. Queue > the result in a topic branch B. > > * For each topic in flight T, make a trial merge of T into B, and > examine "make coccicheck" output. Any new breakages such a test > finds are new violations the topic T introduces. Discard the > result of the trial merge, and add one commit to topic T that > corrects the violations the topic introduced, and send that fixup > to the author of the topic for consideration when the topic is > rerolled (or if the topic is in 'next', acked to be queued on > top). Do not fix the violations that is corrected when branch B > was prepared above. > > As I assumed that applying the patches in this series would create > the branch B, and then I saw that the tip of 'seen' after merging > this topic still needed to have a lot more fixes according to "make > coccicheck", I got a (false) impression that there are too many new > violations from topics in flight, which was the primary source of my > negative reaction against potential code churn. If we try the above > exercise, perhaps there may not be too many topics that need fix-up > beyond what we fix in the branch B, and if that is the case, I would > not be so negative. > > Thanks. Thank you but it seems like a very heavy work for something that had to be much simpler. IMHO my contribution, if deemed useful, ends with the patch that adds ONLY the cocci script, which can solve a style problem identified by the documentation. And it sure works, if applied. But it is not necessarily the case that there must be to the same time a patch series that actually applies the script to the git codebase, in any development branch. This can be done at the most opportune moment by anyone who wishes it, from my POV. Again, thanks you all Best Regards > > >
Junio C Hamano <gitster@pobox.com> writes: > To avoid unnecessary conflicts with in-flight topics, ideally, we > perhaps could do something along this line: > > * Pick a recent stable point that is an ancestor of all topics in > flight. Add the new coccinelle rule file, take "make coccicheck" > output and create a two-patch series like Philip suggested. Queue > the result in a topic branch B. > > * For each topic in flight T, make a trial merge of T into B, and > examine "make coccicheck" output. Any new breakages such a test > finds are new violations the topic T introduces. Discard the > result of the trial merge, and add one commit to topic T that > corrects the violations the topic introduced, and send that fixup > to the author of the topic for consideration when the topic is > rerolled (or if the topic is in 'next', acked to be queued on > top). Do not fix the violations that is corrected when branch B > was prepared above. > > As I assumed that applying the patches in this series would create > the branch B, and then I saw that the tip of 'seen' after merging > this topic still needed to have a lot more fixes according to "make > coccicheck", I got a (false) impression that there are too many new > violations from topics in flight, which was the primary source of my > negative reaction against potential code churn. If we try the above > exercise, perhaps there may not be too many topics that need fix-up > beyond what we fix in the branch B, and if that is the case, I would > not be so negative. So I tried that myself, and the topic branch B was fairly straightforward to create. We have ~60 topics in flight (not counting this one), and it turns out that there is no topic that introduces new code that fails the equals-null.cocci rule. IOW, the follow-up fixup per topic turns out to be an empty set. So, I'd probably use the [01/23] and then a single ~5k lines patch that was generated with equals-null.cocci rule as the branch B above, let it percolate down from 'seen' to 'next' to eventually 'master'. Thanks.
On 02/05/2022 07:22, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> To avoid unnecessary conflicts with in-flight topics, ideally, we >> perhaps could do something along this line: >> >> * Pick a recent stable point that is an ancestor of all topics in >> flight. Add the new coccinelle rule file, take "make coccicheck" >> output and create a two-patch series like Philip suggested. Queue >> the result in a topic branch B. >> >> * For each topic in flight T, make a trial merge of T into B, and >> examine "make coccicheck" output. Any new breakages such a test >> finds are new violations the topic T introduces. Discard the >> result of the trial merge, and add one commit to topic T that >> corrects the violations the topic introduced, and send that fixup >> to the author of the topic for consideration when the topic is >> rerolled (or if the topic is in 'next', acked to be queued on >> top). Do not fix the violations that is corrected when branch B >> was prepared above. >> >> As I assumed that applying the patches in this series would create >> the branch B, and then I saw that the tip of 'seen' after merging >> this topic still needed to have a lot more fixes according to "make >> coccicheck", I got a (false) impression that there are too many new >> violations from topics in flight, which was the primary source of my >> negative reaction against potential code churn. If we try the above >> exercise, perhaps there may not be too many topics that need fix-up >> beyond what we fix in the branch B, and if that is the case, I would >> not be so negative. > So I tried that myself, and the topic branch B was fairly > straightforward to create. > > We have ~60 topics in flight (not counting this one), and it turns > out that there is no topic that introduces new code that fails the > equals-null.cocci rule. IOW, the follow-up fixup per topic turns > out to be an empty set. > > So, I'd probably use the [01/23] and then a single ~5k lines patch > that was generated with equals-null.cocci rule as the branch B > above, let it percolate down from 'seen' to 'next' to eventually > 'master'. > > Thanks. That sounds like a good result. It may also be worth Elia cross checking against a previous release (v2.35.0?) for relatively recent introductions, to cover the potential revert scenario, just in case.. Philip
On Sat, Apr 30, 2022 at 05:20:03PM -0700, Junio C Hamano wrote: > diff -u -p a/compat/mkdir.c b/compat/mkdir.c > --- a/compat/mkdir.c > +++ b/compat/mkdir.c > @@ -9,7 +9,7 @@ int compat_mkdir_wo_trailing_slash(const > size_t len = strlen(dir); > > if (len && dir[len-1] == '/') { > - if ((tmp_dir = strdup(dir)) == NULL) > + if (!(tmp_dir = strdup(dir))) did not check them all, but wanted to raise that this specific case, seems to be regressing in readability. sure; it fails another rule of "not doing assignments inside if", but maybe when it was introduced with 0539ecfdfce (compat: some mkdir() do not like a slash at the end, 2012-08-24), that wasn't valid or it was considered ok for code in compat. but more importantly it serves as an example of why most of those rules use "try"; after all the assignment WITH the parenthesis and == is clear enough that might not warrant the need of two lines, but without the == it is likely to cause trouble or confuse someone. Carlo
Philip Oakley <philipoakley@iee.email> writes: >>> As I assumed that applying the patches in this series would create >>> the branch B, and then I saw that the tip of 'seen' after merging >>> this topic still needed to have a lot more fixes according to "make >>> coccicheck", I got a (false) impression that there are too many new >>> violations from topics in flight, which was the primary source of my >>> negative reaction against potential code churn. If we try the above >>> exercise, perhaps there may not be too many topics that need fix-up >>> beyond what we fix in the branch B, and if that is the case, I would >>> not be so negative. >> So I tried that myself, and the topic branch B was fairly >> straightforward to create. >> >> We have ~60 topics in flight (not counting this one), and it turns >> out that there is no topic that introduces new code that fails the >> equals-null.cocci rule. IOW, the follow-up fixup per topic turns >> out to be an empty set. >> >> So, I'd probably use the [01/23] and then a single ~5k lines patch >> that was generated with equals-null.cocci rule as the branch B >> above, let it percolate down from 'seen' to 'next' to eventually >> 'master'. >> >> Thanks. > That sounds like a good result. > > It may also be worth Elia cross checking against a previous release > (v2.35.0?) for relatively recent introductions, to cover the potential > revert scenario, just in case.. Sounds sensible. We do have some changes between 2.35 and 2.36 and the fork-points of many topics predate 2.36 (and may even 2.35). Here is an experiment I just did: * Applied the patch to add equals-null.cocci to maint-2.35. * Ran "coccicheck", applied the resulting equals-null fix and committed the result. * Merged the "branch B" from last night to it. The resulting tree exactly matched "branch B" (i.e. 2.36.0 fixed with equals-null.cocci check). If I instead merge vanilla 2.36 with the result of fixing maint-2.35, that differs at two places from "branch B" (i.e. we added two new violations to 2.36 relative to 2.35). Doing the same between maint-2.35 and maint-2.34 seems to indicate that we didn't add any new violations during that period. So in short, 2.35 may probably be a good place to start, but basing on 2.36 seems to be good enough. Thanks. branch.c | 2 +- compat/fsmonitor/fsm-listen-darwin.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git c/branch.c w/branch.c index bde705b092..d0ca2b76d2 100644 --- c/branch.c +++ w/branch.c @@ -653,7 +653,7 @@ void create_branches_recursively(struct repository *r, const char *name, * be created in every submodule. */ for (i = 0; i < submodule_entry_list.entry_nr; i++) { - if (!submodule_entry_list.entries[i].repo) { + if (submodule_entry_list.entries[i].repo == NULL) { int code = die_message( _("submodule '%s': unable to find submodule"), submodule_entry_list.entries[i].submodule->name); diff --git c/compat/fsmonitor/fsm-listen-darwin.c w/compat/fsmonitor/fsm-listen-darwin.c index dc8a33130a..0741fe834c 100644 --- c/compat/fsmonitor/fsm-listen-darwin.c +++ w/compat/fsmonitor/fsm-listen-darwin.c @@ -342,7 +342,7 @@ int fsm_listen__ctor(struct fsmonitor_daemon_state *state) data->cfar_paths_to_watch, kFSEventStreamEventIdSinceNow, 0.001, flags); - if (!data->stream) + if (data->stream == NULL) goto failed; /*
diff --git a/contrib/coccinelle/equals-null.cocci b/contrib/coccinelle/equals-null.cocci new file mode 100644 index 0000000000..92c7054013 --- /dev/null +++ b/contrib/coccinelle/equals-null.cocci @@ -0,0 +1,30 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +@@ +expression e; +statement s; +@@ +if ( +( +!e +| +- e == NULL ++ !e +) + ) + {...} +else s + +@@ +expression e; +statement s; +@@ +if ( +( +e +| +- e != NULL ++ e +) + ) + {...} +else s
Add a coccinelle semantic patch necessary to reinforce the git coding style guideline: "Do not explicitly compute an integral value with constant 0 or '\ 0', or a pointer value with constant NULL." Signed-off-by: Elia Pinto <gitter.spiros@gmail.com> --- contrib/coccinelle/equals-null.cocci | 30 ++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 contrib/coccinelle/equals-null.cocci