Re: [PATCH v3 1/1] convert_to_git(): checksafe becomes int conv_flags

[Torsten Bögershausen]

On 2018-01-05 20:00, Lars Schneider wrote:
> 
>> On 01 Jan 2018, at 22:59, tbo...@web.de wrote:
>>
>> From: Torsten Bögershausen 
>>
>> When calling convert_to_git(), the checksafe parameter has been used to
>> check if commit would give a non-roundtrip conversion of EOL.
>>
>> When checksafe was introduced, 3 values had been in use:
>> SAFE_CRLF_FALSE: no warning
>> SAFE_CRLF_FAIL:  reject the commit if EOL do not roundtrip
>> SAFE_CRLF_WARN:  warn the user if EOL do not roundtrip
>>
>> Already today the integer value 0 is passed as the parameter checksafe
>> instead of the correct enum value SAFE_CRLF_FALSE.
>>
>> Turn the whole call chain to use an integer with single bits, which
>> can be extended in the next commits:
>> - The global configuration variable safe_crlf is now conv_flags_eol.
>> - The parameter checksafe is renamed into conv_flags.
>>
>> Helped-By: Lars Schneider 
>> Signed-off-by: Torsten Bögershausen 
>> ---
>> This is my suggestion.
>> (1) The flag bits had been renamed.
>> (2) The (theoretical ?) mix of WARN/FAIL is still there,
>>I am not sure if this is a real problem.
>>
>> (3) There are 2 reasons that CONV_EOL_RENORMALIZE is set.
>>Either in a renormalizing merge, or by running
>>git add --renormalize .
>>Therefor HASH_RENORMALIZE is not the same as CONV_EOL_RENORMALIZE.
> 
> Can you elaborate a bit? I am diving into the code but I am still confused.
> 
> I also noticed that the "flags" integer is potentially double booked with
> the following values (see read-cache.c:add_to_index()):
> 
> #define ADD_CACHE_VERBOSE 1
> #define ADD_CACHE_PRETEND 2
> #define ADD_CACHE_IGNORE_ERRORS   4
> 
> #define HASH_WRITE_OBJECT 1
> #define HASH_FORMAT_CHECK 2
> #define HASH_RENORMALIZE  4
> 
> Is this intentional? 

All these flags have a different context,
and the right #define must be used in the right context/function call.
So there is no intention.
You start with 1, then use 2 and so on.

The same way as family Schmidt and family Meier both call
their child "Hans".
There is no connection.


> 
> Thanks,
> Lars
> 
> 
> More context:
>   https://public-inbox.org/git/96b6cd4c-0a0c-47f5-922d-b8bafb832...@gmail.com/
>   (3) We kind of replicate some flags defined in cache.h:
>  #define HASH_WRITE_OBJECT 1
>  #define HASH_RENORMALIZE  4
> 
> 

Re: [PATCH v3 1/1] convert_to_git(): checksafe becomes int conv_flags

[Lars Schneider]

> On 01 Jan 2018, at 22:59, tbo...@web.de wrote:
> 
> From: Torsten Bögershausen 
> 
> When calling convert_to_git(), the checksafe parameter has been used to
> check if commit would give a non-roundtrip conversion of EOL.
> 
> When checksafe was introduced, 3 values had been in use:
> SAFE_CRLF_FALSE: no warning
> SAFE_CRLF_FAIL:  reject the commit if EOL do not roundtrip
> SAFE_CRLF_WARN:  warn the user if EOL do not roundtrip
> 
> Already today the integer value 0 is passed as the parameter checksafe
> instead of the correct enum value SAFE_CRLF_FALSE.
> 
> Turn the whole call chain to use an integer with single bits, which
> can be extended in the next commits:
> - The global configuration variable safe_crlf is now conv_flags_eol.
> - The parameter checksafe is renamed into conv_flags.
> 
> Helped-By: Lars Schneider 
> Signed-off-by: Torsten Bögershausen 
> ---
> This is my suggestion.
> (1) The flag bits had been renamed.
> (2) The (theoretical ?) mix of WARN/FAIL is still there,
>I am not sure if this is a real problem.
> 
> (3) There are 2 reasons that CONV_EOL_RENORMALIZE is set.
>Either in a renormalizing merge, or by running
>git add --renormalize .
>Therefor HASH_RENORMALIZE is not the same as CONV_EOL_RENORMALIZE.

Can you elaborate a bit? I am diving into the code but I am still confused.

I also noticed that the "flags" integer is potentially double booked with
the following values (see read-cache.c:add_to_index()):

#define ADD_CACHE_VERBOSE 1
#define ADD_CACHE_PRETEND 2
#define ADD_CACHE_IGNORE_ERRORS 4

#define HASH_WRITE_OBJECT 1
#define HASH_FORMAT_CHECK 2
#define HASH_RENORMALIZE  4

Is this intentional? 

Thanks,
Lars


More context:
  https://public-inbox.org/git/96b6cd4c-0a0c-47f5-922d-b8bafb832...@gmail.com/
  (3) We kind of replicate some flags defined in cache.h:
 #define HASH_WRITE_OBJECT 1
 #define HASH_RENORMALIZE  4

Re: [PATCH v3 1/1] convert_to_git(): checksafe becomes int conv_flags

[Lars Schneider]

> On 03 Jan 2018, at 06:36, Torsten Bögershausen  wrote:
> 
> On Tue, Jan 02, 2018 at 08:11:51PM +0100, Lars Schneider wrote:
> 
> [snip]
> 
>>> /*
>>> diff --git a/diff.c b/diff.c
>>> index fb22b19f09..2470af52b2 100644
>>> --- a/diff.c
>>> +++ b/diff.c
>>> @@ -3524,9 +3524,9 @@ int diff_populate_filespec(struct diff_filespec *s, 
>>> unsigned int flags)
>>>  * demote FAIL to WARN to allow inspecting the situation
>>>  * instead of refusing.
>>>  */
>>> -   enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL
>>> -   ? SAFE_CRLF_WARN
>>> -   : safe_crlf);
>>> +   int conv_flags = (conv_flags_eol == CONV_EOL_RNDTRP_DIE
>>> +   ? CONV_EOL_RNDTRP_WARN
>>> +   : conv_flags_eol);
>> 
>> If there is garbage in conv_flags_eol then we would not demote the DIE
>> flag here.
>> 
>> How about something like that:
>> int conv_flags = conv_flags_eol & ~CONV_EOL_RNDTRP_DIE;
> 
> The next version will probably look like this:
> 
> int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
> {
>   int size_only = flags & CHECK_SIZE_ONLY;
>   int err = 0;
>   int conv_flags = conv_flags_eol;
>   /*
>* demote FAIL to WARN to allow inspecting the situation
>* instead of refusing.
>*/
>   if (conv_flags & CONV_EOL_RNDTRP_DIE)
>   conv_flags =CONV_EOL_RNDTRP_WARN;

This looks good. Sorry, I just realized that my suggestion 
was garbage as it only disabled the bit. It did not demote 
DIE to WARN.

One final nit:
Can we be sure that 'conv_flags_eol' is only ever used for CONV_EOL_RNDTRP_DIE
and CONV_EOL_RNDTRP_WARN? Maybe a solution like that would be more future
proof?

  if (conv_flags & CONV_EOL_RNDTRP_DIE) {
  conv_flags &= ~CONV_EOL_RNDTRP_DIE;
  conv_flags |= CONV_EOL_RNDTRP_WARN;
  }


>> 
>> ---
>> 
>> In general I like the patch as I think the variables are a bit easier to 
>> understand. 
>> One thing I stumbled over while reading the patch:
>> 
>> The global variable "conv_flags_eol". I think the Git coding guidelines
>> have no recommendation for naming global variables. I think a 
>> "global_conv_flags_eol"
>> or something would have helped me. I can also see how others might frown 
>> upon such 
>> a naming scheme.
> 
> I don't have a problem with "global_conv_flags_eol".
> Thank for the comments, let's wait for more comments before I send out V4.

Sounds good. A "global_" like prefix is not used anywhere else in Git...

- Lars

Re: [PATCH v3 1/1] convert_to_git(): checksafe becomes int conv_flags

[Lars Schneider]

> On 01 Jan 2018, at 22:59, tbo...@web.de wrote:
> 
> From: Torsten Bögershausen 
> 
> When calling convert_to_git(), the checksafe parameter has been used to
> check if commit would give a non-roundtrip conversion of EOL.
> 
> When checksafe was introduced, 3 values had been in use:
> SAFE_CRLF_FALSE: no warning
> SAFE_CRLF_FAIL:  reject the commit if EOL do not roundtrip
> SAFE_CRLF_WARN:  warn the user if EOL do not roundtrip
> 
> Already today the integer value 0 is passed as the parameter checksafe
> instead of the correct enum value SAFE_CRLF_FALSE.
> 
> Turn the whole call chain to use an integer with single bits, which
> can be extended in the next commits:
> - The global configuration variable safe_crlf is now conv_flags_eol.
> - The parameter checksafe is renamed into conv_flags.
> 
> Helped-By: Lars Schneider 
> Signed-off-by: Torsten Bögershausen 
> ---
> This is my suggestion.
> (1) The flag bits had been renamed.
> (2) The (theoretical ?) mix of WARN/FAIL is still there,
>I am not sure if this is a real problem.
> 
> (3) There are 2 reasons that CONV_EOL_RENORMALIZE is set.
>Either in a renormalizing merge, or by running
>git add --renormalize .
>Therefor HASH_RENORMALIZE is not the same as CONV_EOL_RENORMALIZE.
> 
> 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(-)
> ...
> -static void check_safe_crlf(const char *path, enum crlf_action crlf_action,
> +static void check_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)
> + if (conv_flags & CONV_EOL_RNDTRP_DIE)
> + die(_("CRLF would be replaced by LF in %s."), path);
> + else if (conv_flags & CONV_EOL_RNDTRP_WARN)

Here we go with CONV_EOL_RNDTRP_DIE if there is garbage in conv_flags.
Garbage example: CONV_EOL_RNDTRP_DIE *and* CONV_EOL_RNDTRP_WARN are set.

Good!

>   ...
> /*
> diff --git a/diff.c b/diff.c
> index fb22b19f09..2470af52b2 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3524,9 +3524,9 @@ int diff_populate_filespec(struct diff_filespec *s, 
> unsigned int flags)
>* demote FAIL to WARN to allow inspecting the situation
>* instead of refusing.
>*/
> - enum safe_crlf crlf_warn = (safe_crlf == SAFE_CRLF_FAIL
> - ? SAFE_CRLF_WARN
> - : safe_crlf);
> + int conv_flags = (conv_flags_eol == CONV_EOL_RNDTRP_DIE
> + ? CONV_EOL_RNDTRP_WARN
> + : conv_flags_eol);

If there is garbage in conv_flags_eol then we would not demote the DIE
flag here.

How about something like that:
int conv_flags = conv_flags_eol & ~CONV_EOL_RNDTRP_DIE;

---

In general I like the patch as I think the variables are a bit easier to 
understand. 
One thing I stumbled over while reading the patch:

The global variable "conv_flags_eol". I think the Git coding guidelines
have no recommendation for naming global variables. I think a 
"global_conv_flags_eol"
or something would have helped me. I can also see how others might frown upon 
such 
a naming scheme.

If this patch gets accepted then I will rebase my encoding series on top of it:
https://public-inbox.org/git/2017122915.39680-1-lars.schnei...@autodesk.com/


Thanks,
Lars

[PATCH v3 1/1] convert_to_git(): checksafe becomes int conv_flags

[tboegi]

From: Torsten Bögershausen 

When calling convert_to_git(), the checksafe parameter has been used to
check if commit would give a non-roundtrip conversion of EOL.

When checksafe was introduced, 3 values had been in use:
SAFE_CRLF_FALSE: no warning
SAFE_CRLF_FAIL:  reject the commit if EOL do not roundtrip
SAFE_CRLF_WARN:  warn the user if EOL do not roundtrip

Already today the integer value 0 is passed as the parameter checksafe
instead of the correct enum value SAFE_CRLF_FALSE.

Turn the whole call chain to use an integer with single bits, which
can be extended in the next commits:
- The global configuration variable safe_crlf is now conv_flags_eol.
- The parameter checksafe is renamed into conv_flags.

Helped-By: Lars Schneider 
Signed-off-by: Torsten Bögershausen 
---
This is my suggestion.
(1) The flag bits had been renamed.
(2) The (theoretical ?) mix of WARN/FAIL is still there,
I am not sure if this is a real problem.

(3) There are 2 reasons that CONV_EOL_RENORMALIZE is set.
Either in a renormalizing merge, or by running
git add --renormalize .
Therefor HASH_RENORMALIZE is not the same as CONV_EOL_RENORMALIZE.

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..dbc877d0fe 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, , conv_flags_eol)) {
free(result);
result = strbuf_detach(, );
result_size = len;
diff --git a/config.c b/config.c
index e617c2018d..bdc7ce2a7e 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;
+   conv_flags_eol = CONV_EOL_RNDTRP_WARN;
return 0;
}
-   safe_crlf = git_config_bool(var, value);
+   eol_rndtrp_die = git_config_bool(var, value);
+   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 1a41a48e15..0207ddab24 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_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)
+   if (conv_flags & CONV_EOL_RNDTRP_DIE)
+   die(_("CRLF