Re: .clang-format: how useful, how often used, and how well maintained?

[Junio C Hamano <gitster@pobox.com>]

Toon Claes <toon@iotcl.com> writes:

> I think the only way we can stop bikeshedding about formatting, is by
> adopting clang-format and make it's output the golden standard. We might
> not like it's output (similar to many people do not like `gofmt`s
> output), but it's a standard.

Having other people to blame when the tool leaves unreadable code
that is not easy to read is certainly handy.  But I care more about
the tool giving a reasonably readable result.  It is not about
people not "liking" it.

Here is what clang-format suggests on top of your "last-modified"
series.  For this particular example, the tool gets the resulting
format mostly right, except for an extra space after "foreach"
before the opening parenthesis.  I presume that this is some setting
in the .clang-format file can tweak?

There is one instance of 80-column line wrapping making the result
less easy to view.  If you need to wrap, keep related things
together, i.e. when you rewrite this line ...

-int last_modified_run(struct last_modified *lm, last_modified_callback cb, void *cbdata)

... we should not wrap after "cb," only because it is still shorter
than 80 columns.

+int last_modified_run(struct last_modified *lm, last_modified_callback cb,
+		      void *cbdata)

In fact, slightly busting the 80-column limit like the original had
would be easier to understand while coasting your eyes over the line.
If we need to wrap, we should do this instead ...

+int last_modified_run(struct last_modified *lm,
+		      last_modified_callback cb, void *cbdata)

... so that related things (i.e., the callback function and the
data fed to it) are kept together.

In this particular patch, there was only one such instance that the
tools output was noticeably more irritating than the original.  With
more excercise like this with enough positive experience (I do count
this one as positive, if we fix the space after "foreach"), I might
change my mind.

Anyway, here is the patch.

 builtin.h               |  3 ++-
 builtin/last-modified.c |  6 ++----
 git.c                   | 20 +++++++++++---------
 last-modified.c         | 26 ++++++++++++--------------
 last-modified.h         | 14 +++++---------
 5 files changed, 32 insertions(+), 37 deletions(-)

diff --git c/builtin.h i/builtin.h
index 6ed6759ec4..5f89e51c61 100644
--- c/builtin.h
+++ i/builtin.h
@@ -176,7 +176,8 @@ int cmd_hook(int argc, const char **argv, const char *prefix, struct repository
 int cmd_index_pack(int argc, const char **argv, const char *prefix, struct repository *repo);
 int cmd_init_db(int argc, const char **argv, const char *prefix, struct repository *repo);
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix, struct repository *repo);
-int cmd_last_modified(int argc, const char **argv, const char *prefix, struct repository *repo);
+int cmd_last_modified(int argc, const char **argv, const char *prefix,
+		      struct repository *repo);
 int cmd_log_reflog(int argc, const char **argv, const char *prefix, struct repository *repo);
 int cmd_log(int argc, const char **argv, const char *prefix, struct repository *repo);
 int cmd_ls_files(int argc, const char **argv, const char *prefix, struct repository *repo);
diff --git c/builtin/last-modified.c i/builtin/last-modified.c
index 4ff058c302..86b98e7a46 100644
--- c/builtin/last-modified.c
+++ i/builtin/last-modified.c
@@ -23,10 +23,8 @@ static void show_entry(const char *path, const struct commit *commit, void *d)
 	fflush(stdout);
 }
 
-int cmd_last_modified(int argc,
-		   const char **argv,
-		   const char *prefix,
-		   struct repository *repo)
+int cmd_last_modified(int argc, const char **argv, const char *prefix,
+		      struct repository *repo)
 {
 	struct last_modified lm;
 
diff --git c/git.c i/git.c
index 65afc0d0e7..918d83762a 100644
--- c/git.c
+++ i/git.c
@@ -516,12 +516,10 @@ static struct cmd_struct commands[] = {
 	{ "check-attr", cmd_check_attr, RUN_SETUP },
 	{ "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
 	{ "check-mailmap", cmd_check_mailmap, RUN_SETUP },
-	{ "check-ref-format", cmd_check_ref_format, NO_PARSEOPT  },
+	{ "check-ref-format", cmd_check_ref_format, NO_PARSEOPT },
 	{ "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
-	{ "checkout--worker", cmd_checkout__worker,
-		RUN_SETUP | NEED_WORK_TREE },
-	{ "checkout-index", cmd_checkout_index,
-		RUN_SETUP | NEED_WORK_TREE},
+	{ "checkout--worker", cmd_checkout__worker, RUN_SETUP | NEED_WORK_TREE },
+	{ "checkout-index", cmd_checkout_index, RUN_SETUP | NEED_WORK_TREE },
 	{ "cherry", cmd_cherry, RUN_SETUP },
 	{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
 	{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
@@ -578,10 +576,14 @@ static struct cmd_struct commands[] = {
 	{ "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
 	{ "merge-index", cmd_merge_index, RUN_SETUP | NO_PARSEOPT },
 	{ "merge-ours", cmd_merge_ours, RUN_SETUP | NO_PARSEOPT },
-	{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
-	{ "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
-	{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
-	{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-recursive", cmd_merge_recursive,
+	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-recursive-ours", cmd_merge_recursive,
+	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-recursive-theirs", cmd_merge_recursive,
+	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-subtree", cmd_merge_recursive,
+	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-tree", cmd_merge_tree, RUN_SETUP },
 	{ "mktag", cmd_mktag, RUN_SETUP },
 	{ "mktree", cmd_mktree, RUN_SETUP },
diff --git c/last-modified.c i/last-modified.c
index 2097894c6e..f7f6a67d3b 100644
--- c/last-modified.c
+++ i/last-modified.c
@@ -19,8 +19,7 @@ struct last_modified_entry {
 };
 
 static void add_path_from_diff(struct diff_queue_struct *q,
-			       struct diff_options *opt UNUSED,
-			       void *data)
+			       struct diff_options *opt UNUSED, void *data)
 {
 	struct last_modified *lm = data;
 
@@ -72,9 +71,9 @@ static int populate_paths_from_revs(struct last_modified *lm)
 }
 
 static int last_modified_entry_hashcmp(const void *unused UNUSED,
-				    const struct hashmap_entry *hent1,
-				    const struct hashmap_entry *hent2,
-				    const void *path)
+				       const struct hashmap_entry *hent1,
+				       const struct hashmap_entry *hent2,
+				       const void *path)
 {
 	const struct last_modified_entry *ent1 =
 		container_of(hent1, const struct last_modified_entry, hashent);
@@ -83,10 +82,8 @@ static int last_modified_entry_hashcmp(const void *unused UNUSED,
 	return strcmp(ent1->path, path ? path : ent2->path);
 }
 
-int last_modified_init(struct last_modified *lm,
-		     struct repository *r,
-		     const char *prefix,
-		     int argc, const char **argv)
+int last_modified_init(struct last_modified *lm, struct repository *r,
+		       const char *prefix, int argc, const char **argv)
 {
 	memset(lm, 0, sizeof(*lm));
 	hashmap_init(&lm->paths, last_modified_entry_hashcmp, NULL, 0);
@@ -119,7 +116,7 @@ void last_modified_release(struct last_modified *lm)
 	struct hashmap_iter iter;
 	struct last_modified_entry *ent;
 
-	hashmap_for_each_entry(&lm->paths, &iter, ent, hashent)
+	hashmap_for_each_entry (&lm->paths, &iter, ent, hashent)
 		clear_bloom_key(&ent->key);
 
 	hashmap_clear_and_free(&lm->paths, struct last_modified_entry, hashent);
@@ -213,7 +210,7 @@ static int maybe_changed_path(struct last_modified *lm, struct commit *origin)
 	if (!filter)
 		return 1;
 
-	hashmap_for_each_entry(&lm->paths, &iter, ent, hashent) {
+	hashmap_for_each_entry (&lm->paths, &iter, ent, hashent) {
 		if (bloom_filter_contains(filter, &ent->key,
 					  lm->rev.bloom_filter_settings))
 			return 1;
@@ -221,7 +218,8 @@ static int maybe_changed_path(struct last_modified *lm, struct commit *origin)
 	return 0;
 }
 
-int last_modified_run(struct last_modified *lm, last_modified_callback cb, void *cbdata)
+int last_modified_run(struct last_modified *lm, last_modified_callback cb,
+		      void *cbdata)
 {
 	struct last_modified_callback_data data;
 
@@ -245,8 +243,8 @@ int last_modified_run(struct last_modified *lm, last_modified_callback cb, void
 
 		if (data.commit->object.flags & BOUNDARY) {
 			diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
-				       &data.commit->object.oid,
-				       "", &lm->rev.diffopt);
+				      &data.commit->object.oid, "",
+				      &lm->rev.diffopt);
 			diff_flush(&lm->rev.diffopt);
 		} else {
 			log_tree_commit(&lm->rev, data.commit);
diff --git c/last-modified.h i/last-modified.h
index 04d5a1a5b6..3e83094d77 100644
--- c/last-modified.h
+++ i/last-modified.h
@@ -13,23 +13,19 @@ struct last_modified {
 /*
  * Initialize the last-modified machinery using command line arguments.
  */
-int last_modified_init(struct last_modified *lm,
-		     struct repository *r,
-		     const char *prefix,
-		     int argc, const char **argv);
+int last_modified_init(struct last_modified *lm, struct repository *r,
+		       const char *prefix, int argc, const char **argv);
 
 void last_modified_release(struct last_modified *);
 
 typedef void (*last_modified_callback)(const char *path,
-				    const struct commit *commit,
-				    void *data);
+				       const struct commit *commit, void *data);
 
 /*
  * Run the last-modified traversal. For each path found the callback is called
  * passing the path, the commit, and the cbdata.
  */
-int last_modified_run(struct last_modified *lm,
-		   last_modified_callback cb,
-		   void *cbdata);
+int last_modified_run(struct last_modified *lm, last_modified_callback cb,
+		      void *cbdata);
 
 #endif /* LAST_MODIFIED_H */