[3/9] fsck_tree(): wrap some long lines

Commit Message

Jeff King May 1, 2021, 3:41 p.m. UTC

Many calls to report() in fsck_tree() are kept on a single line and are
quite long. Most were pretty big to begin with, but have gotten even
longer over the years as we've added more parameters. Let's accept the
churn of wrapping them in order to conform to our usual line limits.

Signed-off-by: Jeff King <[e-mail]>
---
 fsck.c | 48 ++++++++++++++++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 12 deletions(-)

Comments

Ævar Arnfjörð Bjarmason May 3, 2021, 11:22 a.m. UTC | #1

On Sat, May 01 2021, Jeff King wrote:

> Many calls to report() in fsck_tree() are kept on a single line and are
> quite long. Most were pretty big to begin with, but have gotten even
> longer over the years as we've added more parameters. Let's accept the
> churn of wrapping them in order to conform to our usual line limits.

If we're going to have the churn I'd say just wrap the rest of the file
as well, now it's mostly-consistent in having these long lines.

FWIW I think having the long lines makes things more readable in this
case, but the inconsistency is worse.

If you do take me up on refactoring it more generally, there's also the
one mis-alignment of an argument list to report() in verify_headers().

I wonder if this whole thing wouldn't be better by declaring the format
in the msg_id_info struct. I.e. add this to fsck.h, but that's an even
bigger change...

Jeff King May 3, 2021, 8:23 p.m. UTC | #2

On Mon, May 03, 2021 at 01:22:13PM +0200, Ævar Arnfjörð Bjarmason wrote:

> On Sat, May 01 2021, Jeff King wrote:
> 
> > Many calls to report() in fsck_tree() are kept on a single line and are
> > quite long. Most were pretty big to begin with, but have gotten even
> > longer over the years as we've added more parameters. Let's accept the
> > churn of wrapping them in order to conform to our usual line limits.
> 
> If we're going to have the churn I'd say just wrap the rest of the file
> as well, now it's mostly-consistent in having these long lines.
> 
> FWIW I think having the long lines makes things more readable in this
> case, but the inconsistency is worse.

I'm not sure I agree. It depends on how big a chunk you consider for
consistency: a function, a file, or the whole project.

fsck_tree() was already inconsistent, so this is making that function
totally consistent. Since that was the function I was working in, that
seemed like the limit of "while I'm here", and I'd prefer to keep it
there for the series.

I certainly don't mind extra clean up on top, though.

As far as preferring the long lines, I don't mind lines a _little_ long,
but some of these are 120+ characters. They wrap awkwardly even on my
extra-wide terminals. ;) I guess we can have a discussion on whether
long lines are OK, but it should probably center on what we put into
CodingGuidelines, and not these particular lines.

> I wonder if this whole thing wouldn't be better by declaring the format
> in the msg_id_info struct. I.e. add this to fsck.h, but that's an even
> bigger change...

I think it gets tricky, as not all of the strings have the same number
and type of format specifiers (most don't have any, but verify_headers()
for example uses %ld).

-Peff

Patch

diff --git a/fsck.c b/fsck.c
index dd31ed1413..db94817898 100644
--- a/fsck.c
+++ b/fsck.c
@@ -579,7 +579,9 @@  static int fsck_tree(const struct object_id *tree_oid,
 	struct name_stack df_dup_candidates = { NULL };
 
 	if (init_tree_desc_gently(&desc, buffer, size)) {
-		retval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
+		retval += report(options, tree_oid, OBJ_TREE,
+				 FSCK_MSG_BAD_TREE,
+				 "cannot be parsed as a tree");
 		return retval;
 	}
 
@@ -630,7 +632,9 @@  static int fsck_tree(const struct object_id *tree_oid,
 		}
 
 		if (update_tree_entry_gently(&desc)) {
-			retval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_TREE, "cannot be parsed as a tree");
+			retval += report(options, tree_oid, OBJ_TREE,
+					 FSCK_MSG_BAD_TREE,
+					 "cannot be parsed as a tree");
 			break;
 		}
 
@@ -678,25 +682,45 @@  static int fsck_tree(const struct object_id *tree_oid,
 	name_stack_clear(&df_dup_candidates);
 
 	if (has_null_sha1)
-		retval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_NULL_SHA1, "contains entries pointing to null sha1");
+		retval += report(options, tree_oid, OBJ_TREE,
+				 FSCK_MSG_NULL_SHA1,
+				 "contains entries pointing to null sha1");
 	if (has_full_path)
-		retval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_FULL_PATHNAME, "contains full pathnames");
+		retval += report(options, tree_oid, OBJ_TREE,
+				 FSCK_MSG_FULL_PATHNAME,
+				 "contains full pathnames");
 	if (has_empty_name)
-		retval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_EMPTY_NAME, "contains empty pathname");
+		retval += report(options, tree_oid, OBJ_TREE,
+				 FSCK_MSG_EMPTY_NAME,
+				 "contains empty pathname");
 	if (has_dot)
-		retval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOT, "contains '.'");
+		retval += report(options, tree_oid, OBJ_TREE,
+				 FSCK_MSG_HAS_DOT,
+				 "contains '.'");
 	if (has_dotdot)
-		retval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTDOT, "contains '..'");
+		retval += report(options, tree_oid, OBJ_TREE,
+				 FSCK_MSG_HAS_DOTDOT,
+				 "contains '..'");
 	if (has_dotgit)
-		retval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_HAS_DOTGIT, "contains '.git'");
+		retval += report(options, tree_oid, OBJ_TREE,
+				 FSCK_MSG_HAS_DOTGIT,
+				 "contains '.git'");
 	if (has_zero_pad)
-		retval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_ZERO_PADDED_FILEMODE, "contains zero-padded file modes");
+		retval += report(options, tree_oid, OBJ_TREE,
+				 FSCK_MSG_ZERO_PADDED_FILEMODE,
+				 "contains zero-padded file modes");
 	if (has_bad_modes)
-		retval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, "contains bad file modes");
+		retval += report(options, tree_oid, OBJ_TREE,
+				 FSCK_MSG_BAD_FILEMODE,
+				 "contains bad file modes");
 	if (has_dup_entries)
-		retval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, "contains duplicate file entries");
+		retval += report(options, tree_oid, OBJ_TREE,
+				 FSCK_MSG_DUPLICATE_ENTRIES,
+				 "contains duplicate file entries");
 	if (not_properly_sorted)
-		retval += report(options, tree_oid, OBJ_TREE, FSCK_MSG_TREE_NOT_SORTED, "not properly sorted");
+		retval += report(options, tree_oid, OBJ_TREE,
+				 FSCK_MSG_TREE_NOT_SORTED,
+				 "not properly sorted");
 	return retval;
 }