Re: [PATCH v3 5/7] convert_to_git(): safe_crlf/checksafe becomes int conv_flags

[Torsten Bögershausen]

On Mon, Jan 08, 2018 at 11:47:20PM +0100, Lars Schneider wrote:
> 
> > On 08 Jan 2018, at 22:28, Junio C Hamano  wrote:
> > 
> > lars.schnei...@autodesk.com writes:
> > 
> >> diff --git a/sha1_file.c b/sha1_file.c
> >> index afe4b90f6e..dcb02e9ffd 100644
> >> --- a/sha1_file.c
> >> +++ b/sha1_file.c
> >> @@ -75,14 +75,14 @@ static struct cached_object *find_cached_object(const 
> >> unsigned char *sha1)
> >> }
> >> 
> >> 
> >> -static enum safe_crlf get_safe_crlf(unsigned flags)
> >> +static int get_conv_flags(unsigned flags)
> >> {
> >>if (flags & HASH_RENORMALIZE)
> >> -  return SAFE_CRLF_RENORMALIZE;
> >> +  return CONV_EOL_RENORMALIZE;
> >>else if (flags & HASH_WRITE_OBJECT)
> >> -  return safe_crlf;
> >> +  return global_conv_flags_eol | CONV_WRITE_OBJECT;
> > 
> > This macro has not yet introduced at this point (it appears in 6/7
> > if I am not mistaken).
> 
> Nice catch. I'll fix that in the next iteration.
> 
> Is it OK if I send the next iteration soon or would you prefer
> it if I wait until after 2.16 release?
> 
> Plus, is it ok to keep the base of the series or would you prefer
> it if I rebase it to the latest master (because of a minor conflict)?
> 
> Thanks,
> Lars

I noticed the missing macro as well, while doing the rebase
to git.git/master, but forget to mention it here on the list

Lars, if you want, please have a look here:
https://github.com/tboegi/git/tree/180108-1858-For-lars-schneider-encode-V3C

Re: [PATCH v3 5/7] convert_to_git(): safe_crlf/checksafe becomes int conv_flags

[Junio C Hamano]

Lars Schneider  writes:

> Nice catch. I'll fix that in the next iteration.
>
> Is it OK if I send the next iteration soon or would you prefer
> it if I wait until after 2.16 release?
>
> Plus, is it ok to keep the base of the series or would you prefer
> it if I rebase it to the latest master (because of a minor conflict)?

I do not see this topic as a fix for grave bug that needs to go to
older maintenance track---it is rather a new feature, isn't it?  So
a rebased series that cleanly applies on top of 2.16 final would be
a reasonable way to go forward.

Thanks.

Re: [PATCH v3 5/7] convert_to_git(): safe_crlf/checksafe becomes int conv_flags

[Lars Schneider]

> On 08 Jan 2018, at 22:28, Junio C Hamano  wrote:
> 
> lars.schnei...@autodesk.com writes:
> 
>> diff --git a/sha1_file.c b/sha1_file.c
>> index afe4b90f6e..dcb02e9ffd 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -75,14 +75,14 @@ static struct cached_object *find_cached_object(const 
>> unsigned char *sha1)
>> }
>> 
>> 
>> -static enum safe_crlf get_safe_crlf(unsigned flags)
>> +static int get_conv_flags(unsigned flags)
>> {
>>  if (flags & HASH_RENORMALIZE)
>> -return SAFE_CRLF_RENORMALIZE;
>> +return CONV_EOL_RENORMALIZE;
>>  else if (flags & HASH_WRITE_OBJECT)
>> -return safe_crlf;
>> +return global_conv_flags_eol | CONV_WRITE_OBJECT;
> 
> This macro has not yet introduced at this point (it appears in 6/7
> if I am not mistaken).

Nice catch. I'll fix that in the next iteration.

Is it OK if I send the next iteration soon or would you prefer
it if I wait until after 2.16 release?

Plus, is it ok to keep the base of the series or would you prefer
it if I rebase it to the latest master (because of a minor conflict)?

Thanks,
Lars

Re: [PATCH v3 5/7] convert_to_git(): safe_crlf/checksafe becomes int conv_flags

[Junio C Hamano]

lars.schnei...@autodesk.com writes:

> diff --git a/sha1_file.c b/sha1_file.c
> index afe4b90f6e..dcb02e9ffd 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -75,14 +75,14 @@ static struct cached_object *find_cached_object(const 
> unsigned char *sha1)
>  }
>  
>  
> -static enum safe_crlf get_safe_crlf(unsigned flags)
> +static int get_conv_flags(unsigned flags)
>  {
>   if (flags & HASH_RENORMALIZE)
> - return SAFE_CRLF_RENORMALIZE;
> + return CONV_EOL_RENORMALIZE;
>   else if (flags & HASH_WRITE_OBJECT)
> - return safe_crlf;
> + return global_conv_flags_eol | CONV_WRITE_OBJECT;

This macro has not yet introduced at this point (it appears in 6/7
if I am not mistaken).

[PATCH v3 5/7] convert_to_git(): safe_crlf/checksafe becomes int conv_flags

[lars . schneider]

From: Torsten Bögershausen 

When calling convert_to_git(), the checksafe parameter defined what
should happen if the EOL conversion (CRLF --> LF --> CRLF) does not
roundtrip cleanly. In addition, it also defined if line endings should
be renormalized (CRLF --> LF) or kept as they are.

checksafe was an safe_crlf enum with these values:
SAFE_CRLF_FALSE:   do nothing in case of EOL roundtrip errors
SAFE_CRLF_FAIL:die in case of EOL roundtrip errors
SAFE_CRLF_WARN:print a warning in case of EOL roundtrip errors
SAFE_CRLF_RENORMALIZE: change CRLF to LF
SAFE_CRLF_KEEP_CRLF:   keep all line endings as they are

In some cases the integer value 0 was passed as checksafe parameter
instead of the correct enum value SAFE_CRLF_FALSE. That was no problem
because SAFE_CRLF_FALSE is defined as 0.

FALSE/FAIL/WARN are different from RENORMALIZE and KEEP_CRLF. Therefore,
an enum is not ideal. Let's use a integer bit pattern instead and rename
the parameter to conv_flags to make it more generically usable. This
allows us to extend the bit pattern in a subsequent commit.

Helped-By: Lars Schneider 
Signed-off-by: Torsten Bögershausen 
Signed-off-by: Lars Schneider 
---
 apply.c|  6 +++---
 combine-diff.c |  2 +-
 config.c   |  7 +--
 convert.c  | 38 +++---
 convert.h  | 17 +++--
 diff.c |  8 
 environment.c  |  2 +-
 sha1_file.c| 12 ++--
 8 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/apply.c b/apply.c
index 321a9fa68d..f8b67bfee2 100644
--- a/apply.c
+++ b/apply.c
@@ -2263,8 +2263,8 @@ static void show_stats(struct apply_state *state, struct 
patch *patch)
 static int read_old_data(struct stat *st, struct patch *patch,
 const char *path, struct strbuf *buf)
 {
-   enum safe_crlf safe_crlf = patch->crlf_in_old ?
-   SAFE_CRLF_KEEP_CRLF : SAFE_CRLF_RENORMALIZE;
+   int conv_flags = patch->crlf_in_old ?
+   CONV_EOL_KEEP_CRLF : CONV_EOL_RENORMALIZE;
switch (st->st_mode & S_IFMT) {
case S_IFLNK:
if (strbuf_readlink(buf, path, st->st_size) < 0)
@@ -2281,7 +2281,7 @@ static int read_old_data(struct stat *st, struct patch 
*patch,
 * should never look at the index when explicit crlf option
 * is given.
 */
-   convert_to_git(NULL, path, buf->buf, buf->len, buf, safe_crlf);
+   convert_to_git(NULL, path, buf->buf, buf->len, buf, conv_flags);
return 0;
default:
return -1;
diff --git a/combine-diff.c b/combine-diff.c
index 2505de119a..19f30c3353 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1053,7 +1053,7 @@ static void show_patch_diff(struct combine_diff_path 
*elem, int num_parent,
if (is_file) {
struct strbuf buf = STRBUF_INIT;
 
-   if (convert_to_git(_index, elem->path, 
result, len, , safe_crlf)) {
+   if (convert_to_git(_index, elem->path, 
result, len, , global_conv_flags_eol)) {
free(result);
result = strbuf_detach(, );
result_size = len;
diff --git a/config.c b/config.c
index e617c2018d..1f003fbb90 100644
--- a/config.c
+++ b/config.c
@@ -1149,11 +1149,14 @@ static int git_default_core_config(const char *var, 
const char *value)
}
 
if (!strcmp(var, "core.safecrlf")) {
+   int eol_rndtrp_die;
if (value && !strcasecmp(value, "warn")) {
-   safe_crlf = SAFE_CRLF_WARN;
+   global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
return 0;
}
-   safe_crlf = git_config_bool(var, value);
+   eol_rndtrp_die = git_config_bool(var, value);
+   global_conv_flags_eol = eol_rndtrp_die ?
+   CONV_EOL_RNDTRP_DIE : CONV_EOL_RNDTRP_WARN;
return 0;
}
 
diff --git a/convert.c b/convert.c
index 20d7ab67bd..f39150cde9 100644
--- a/convert.c
+++ b/convert.c
@@ -193,30 +193,30 @@ static enum eol output_eol(enum crlf_action crlf_action)
return core_eol;
 }
 
-static void check_safe_crlf(const char *path, enum crlf_action crlf_action,
+static void check_global_conv_flags_eol(const char *path, enum crlf_action 
crlf_action,
struct text_stat *old_stats, struct text_stat 
*new_stats,
-   enum safe_crlf checksafe)
+   int conv_flags)
 {
if (old_stats->crlf && !new_stats->crlf ) {
/*
 * CRLFs would not be restored by checkout
 */
-   if (checksafe == SAFE_CRLF_WARN)
+