Re: [PATCH v7] safecrlf: Add mechanism to warn about irreversible crlf conversions
From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:44:10
Hi, On Sun, 3 Feb 2008, Steffen Prohaska wrote:
quoted hunk ↗ jump to hunk
diff --git a/convert.c b/convert.c index 80f114b..d580a62 100644 --- a/convert.c +++ b/convert.c@@ -95,9 +95,6 @@ static int crlf_to_git(const char *path, const char *src, size_t len, return 0; gather_stats(src, len, &stats); - /* No CR? Nothing to convert, regardless. */ - if (!stats.cr) - return 0;
Yes, you add it later, but would this not be "more correct" if you prepended a "!checksafe &&" and kept the if here? Of course, you'd _also_ need it later, but then _inside_ the "if (checksafe)" clause.
quoted hunk ↗ jump to hunk
@@ -115,6 +112,30 @@ static int crlf_to_git(const char *path, const char *src, size_t len, return 0; } + if (checksafe) { + if (action == CRLF_INPUT || auto_crlf <= 0) { + /* CRLFs would not be restored by checkout: check if we'd remove CRLFs */
There are lines in your patch that are substantially longer than 80 characters / line. Please fix.
quoted hunk ↗ jump to hunk
diff --git a/environment.c b/environment.c index 18a1c4e..e351e99 100644 --- a/environment.c +++ b/environment.c@@ -35,6 +35,7 @@ int pager_use_color = 1; char *editor_program; char *excludes_file; int auto_crlf = 0; /* 1: both ways, -1: only when adding git objects */ +enum safe_crlf safe_crlf = SAFE_CRLF_WARN;
I think this choice needs some defending. At first I thought: "no way that this will be the default; it affects too many users it should _not_ affect". The thing is, Unix users should not need to suffer, only because other users are cursed by insane choices of their OS developers. However, safe_crlf != SAFE_CRLF_FALSE does not affect people who did not set core.crlf = input or core.crlf = true. And for those who set core.crlf, the default makes sense, absolutely. So I think this patch should go in, with shortened lines (and possibly the changes to the "if (!stats.cr)" if you agree with me). Ciao, Dscho