Re: [PATCH v1 2/2] convert: missing `LF will be replaced by CRLF

[Junio C Hamano]

tbo...@web.de writes:

> From: Torsten Bögershausen 
>
> When the "new safer autocrlf-handling" was introduced in c4805393, the logic
> around check_safe_crlf() should have been changed:

Should have been changed from what to what?  And instead it was
changed to which other logic?  What is the difference between the
ideal this change tries to bring in and what c4805393 did?

> Once a file has been commited with CRLF (and core.autocrlf is true),
> line endings are no longer converted by "git add" or "git checkout".
> The line endings will not normalized by Git.

s/will not/& be/;

> The user may run e.g. dos2unix and commit the file.

It is unclear what this sentence wants to say.  The user may overwrite
the file with random contents and commit it, too.  So what?

What is missing from this statement is what problem is being worked
around.  Do you mean: "Because Git does not normalize once CRLF is
in the index, if the user wants to have a normalized content in the
repository to correct that mistake, the user needs to do dos2unix to
fix it and commit the fixed result"?

> Factor out will_convert_lf_to_crlf() which returns when LF are converted
> into CRLF at checkout.
>
> Update the logic around check_safe_crlf() and do the possible CRLF-LF
> conversion in "git add".
> Simulate the checkout (and a possible LF-CRLF conversion) with help of
> will_convert_lf_to_crlf().

These two paragraphs are all "what I did to solve X", but what X is
is not very clear.

> ---

Missing sign-off.

>  convert.c| 97 
> ++--
>  t/t0027-auto-crlf.sh |  6 ++--
>  2 files changed, 60 insertions(+), 43 deletions(-)
>
> diff --git a/convert.c b/convert.c
> index 67d69b5..077f5e6 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -189,33 +189,25 @@ static enum eol output_eol(enum crlf_action crlf_action)
>  }
>  
>  static void check_safe_crlf(const char *path, enum crlf_action crlf_action,
> -struct text_stat *stats, enum safe_crlf 
> checksafe)
> + struct text_stat *old_stats, struct text_stat 
> *new_stats,
> + enum safe_crlf checksafe)
>  {
> - if (!checksafe)
> - return;
> -
> - if (output_eol(crlf_action) == EOL_LF) {
> + if (old_stats->crlf && !new_stats->crlf ) {

Space before ")"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 2/2] convert: missing `LF will be replaced by CRLF

[tboegi]

From: Torsten Bögershausen 

When the "new safer autocrlf-handling" was introduced in c4805393, the logic
around check_safe_crlf() should have been changed:
Once a file has been commited with CRLF (and core.autocrlf is true),
line endings are no longer converted by "git add" or "git checkout".
The line endings will not normalized by Git.
The user may run e.g. dos2unix and commit the file.

Factor out will_convert_lf_to_crlf() which returns when LF are converted
into CRLF at checkout.

Update the logic around check_safe_crlf() and do the possible CRLF-LF
conversion in "git add".
Simulate the checkout (and a possible LF-CRLF conversion) with help of
will_convert_lf_to_crlf().
---
 convert.c| 97 ++--
 t/t0027-auto-crlf.sh |  6 ++--
 2 files changed, 60 insertions(+), 43 deletions(-)

diff --git a/convert.c b/convert.c
index 67d69b5..077f5e6 100644
--- a/convert.c
+++ b/convert.c
@@ -189,33 +189,25 @@ static enum eol output_eol(enum crlf_action crlf_action)
 }
 
 static void check_safe_crlf(const char *path, enum crlf_action crlf_action,
-struct text_stat *stats, enum safe_crlf checksafe)
+   struct text_stat *old_stats, struct text_stat 
*new_stats,
+   enum safe_crlf checksafe)
 {
-   if (!checksafe)
-   return;
-
-   if (output_eol(crlf_action) == EOL_LF) {
+   if (old_stats->crlf && !new_stats->crlf ) {
/*
-* CRLFs would not be restored by checkout:
-* check if we'd remove CRLFs
+* CRLFs would not be restored by checkout
 */
-   if (stats->crlf) {
-   if (checksafe == SAFE_CRLF_WARN)
-   warning("CRLF will be replaced by LF in 
%s.\nThe file will have its original line endings in your working directory.", 
path);
-   else /* i.e. SAFE_CRLF_FAIL */
-   die("CRLF would be replaced by LF in %s.", 
path);
-   }
-   } else if (output_eol(crlf_action) == EOL_CRLF) {
+   if (checksafe == SAFE_CRLF_WARN)
+   warning("CRLF will be replaced by LF in %s.\nThe file 
will have its original line endings in your working directory.", path);
+   else /* i.e. SAFE_CRLF_FAIL */
+   die("CRLF would be replaced by LF in %s.", path);
+   } else if (old_stats->lonelf && !new_stats->lonelf ) {
/*
-* CRLFs would be added by checkout:
-* check if we have "naked" LFs
+* CRLFs would be added by checkout
 */
-   if (stats->lonelf) {
-   if (checksafe == SAFE_CRLF_WARN)
-   warning("LF will be replaced by CRLF in 
%s.\nThe file will have its original line endings in your working directory.", 
path);
-   else /* i.e. SAFE_CRLF_FAIL */
-   die("LF would be replaced by CRLF in %s", path);
-   }
+   if (checksafe == SAFE_CRLF_WARN)
+   warning("LF will be replaced by CRLF in %s.\nThe file 
will have its original line endings in your working directory.", path);
+   else /* i.e. SAFE_CRLF_FAIL */
+   die("LF would be replaced by CRLF in %s", path);
}
 }
 
@@ -233,12 +225,35 @@ static int has_cr_in_index(const char *path)
return has_cr;
 }
 
+static int will_convert_lf_to_crlf(size_t len, struct text_stat *stats,
+  enum crlf_action crlf_action)
+{
+   if (output_eol(crlf_action) != EOL_CRLF)
+   return 0;
+   /* No "naked" LF? Nothing to convert, regardless. */
+   if (!stats->lonelf)
+   return 0;
+
+   if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || 
crlf_action == CRLF_AUTO_CRLF) {
+   /* If we have any CR or CRLF line endings, we do not touch it */
+   /* This is the new safer autocrlf-handling */
+   if (stats->lonecr || stats->crlf)
+   return 0;
+
+   if (convert_is_binary(len, stats))
+   return 0;
+   }
+   return 1;
+
+}
+
 static int crlf_to_git(const char *path, const char *src, size_t len,
   struct strbuf *buf,
   enum crlf_action crlf_action, enum safe_crlf checksafe)
 {
struct text_stat stats;
char *dst;
+   int convert_crlf_into_lf;
 
if (crlf_action == CRLF_BINARY ||
(src && !len))
@@ -252,6 +267,8 @@ static int crlf_to_git(const char *path, const char *src, 
size_t len,
return 1;
 
gather_stats(src, len, );
+   /* Optimization: No CRLF? Nothing to convert, regardless. */
+   convert_crlf_into_lf = !!stats.crlf;