Re: [PATCH 1/5] convert_to_git(): checksafe becomes an integer

2018-01-05 Thread Lars Schneider

> On 06 Jan 2018, at 00:22, Junio C Hamano  wrote:
> 
> Lars Schneider  writes:
> 
>>> On 31 Dec 2017, at 09:05, 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
>> 
>> In general, I really like the direction as this simplifies
>> my patch later on in 5/5. However, I see a few problems:
>> ...
> 
> Yes, this looks like a sensible way to go.  I saw Torsten's v3 for
> 1/5 but will end up queuing v2b, as I suspect 5/5 would need to be
> adjusted for the change between the two versions.

I just send out my v3 which integrates the latest version of Torsten's
patch into my series:

https://public-inbox.org/git/20180106004808.77513-1-lars.schnei...@autodesk.com/

Please note: I did not rebase my series. Therefore, there is a small
conflict with 86ff70a0f0 (convert: tighten the safe autocrlf handling, 
2017-11-26)
because has_cr_in_index() was renamed to has_crlf_in_index().

@Junio: What do you prefer in these cases? A rebased series or the conflict?

Thanks,
Lars

Re: [PATCH 1/5] convert_to_git(): checksafe becomes an integer

2018-01-05 Thread Junio C Hamano
Lars Schneider  writes:

>> On 31 Dec 2017, at 09:05, 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
>
> In general, I really like the direction as this simplifies
> my patch later on in 5/5. However, I see a few problems:
> ...

Yes, this looks like a sensible way to go.  I saw Torsten's v3 for
1/5 but will end up queuing v2b, as I suspect 5/5 would need to be
adjusted for the change between the two versions.

Thanks.


Re: [PATCH 1/5] convert_to_git(): checksafe becomes an integer

2017-12-31 Thread Lars Schneider

> On 31 Dec 2017, at 09:05, 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

In general, I really like the direction as this simplifies
my patch later on in 5/5. However, I see a few problems:

(1) The prefix "SAFE_CRLF" confuses me because the main theme
is EOL and CRLF just happens to be a EOL type.

What do you think about something like this:
CONVERT_ERROR_IGNORE 0
CONVERT_EOL_ERROR_DIE(1<<0)
CONVERT_EOL_ERROR_WARN   (1<<1)
CONVERT_EOL_TO_LF(1<<2)
CONVERT_EOL_KEEP_CRLF(1<<3)
CONVERT_ENCODE_ERROR_DIE (1<<4)

(2) We mix error reporting switches (FALSE/FAIL/WARN) with
switches that change the content (RENORMALIZE/KEEP_CRLF).
Plus, these the switches should be mutually exclusive
(e.g. we don't want to enable the FAIL and WARN bit at
the same time).

(3) We kind of replicate some flags defined in cache.h:
#define HASH_WRITE_OBJECT 1
#define HASH_RENORMALIZE  4


- Lars




[PATCH 1/5] convert_to_git(): checksafe becomes an integer

2017-12-31 Thread 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

Today a small flaw is found in the code base:
An integer with the value 0 is passed as the parameter checksafe
instead of the correct enum value SAFE_CRLF_FALSE.

In the next commit there is a need to turn checksafe into a bitmap, which
allows to tell convert_to_git() to obey the encoding attribute or not.

Signed-off-by: Torsten Bögershausen 
---
 apply.c   |  4 ++--
 convert.c | 20 ++--
 convert.h | 18 --
 diff.c|  4 ++--
 environment.c |  2 +-
 sha1_file.c   |  6 +++---
 6 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/apply.c b/apply.c
index 321a9fa68d..a422516062 100644
--- a/apply.c
+++ b/apply.c
@@ -2263,7 +2263,7 @@ 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 ?
+   int checksafe = patch->crlf_in_old ?
SAFE_CRLF_KEEP_CRLF : SAFE_CRLF_RENORMALIZE;
switch (st->st_mode & S_IFMT) {
case S_IFLNK:
@@ -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, checksafe);
return 0;
default:
return -1;
diff --git a/convert.c b/convert.c
index 1a41a48e15..5efcc3b73b 100644
--- a/convert.c
+++ b/convert.c
@@ -195,13 +195,13 @@ 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 *old_stats, struct text_stat 
*new_stats,
-   enum safe_crlf checksafe)
+   int checksafe)
 {
if (old_stats->crlf && !new_stats->crlf ) {
/*
 * CRLFs would not be restored by checkout
 */
-   if (checksafe == SAFE_CRLF_WARN)
+   if (checksafe & SAFE_CRLF_WARN)
warning(_("CRLF will be replaced by LF in %s.\n"
  "The file will have its original line"
  " endings in your working directory."), path);
@@ -211,7 +211,7 @@ static void check_safe_crlf(const char *path, enum 
crlf_action crlf_action,
/*
 * CRLFs would be added by checkout
 */
-   if (checksafe == SAFE_CRLF_WARN)
+   if (checksafe & SAFE_CRLF_WARN)
warning(_("LF will be replaced by CRLF in %s.\n"
  "The file will have its original line"
  " endings in your working directory."), path);
@@ -268,7 +268,7 @@ static int will_convert_lf_to_crlf(size_t len, struct 
text_stat *stats,
 static int crlf_to_git(const struct index_state *istate,
   const char *path, const char *src, size_t len,
   struct strbuf *buf,
-  enum crlf_action crlf_action, enum safe_crlf checksafe)
+  enum crlf_action crlf_action, int checksafe)
 {
struct text_stat stats;
char *dst;
@@ -298,12 +298,12 @@ static int crlf_to_git(const struct index_state *istate,
 * unless we want to renormalize in a merge or
 * cherry-pick.
 */
-   if ((checksafe != SAFE_CRLF_RENORMALIZE) &&
+   if ((!(checksafe & SAFE_CRLF_RENORMALIZE)) &&
has_crlf_in_index(istate, path))
convert_crlf_into_lf = 0;
}
-   if ((checksafe == SAFE_CRLF_WARN ||
-   (checksafe == SAFE_CRLF_FAIL)) && len) {
+   if (((checksafe & SAFE_CRLF_WARN) ||
+((checksafe & SAFE_CRLF_FAIL) && len))) {
struct text_stat new_stats;
memcpy(&new_stats, &stats, sizeof(new_stats));
/* simulate "git add" */
@@ -1129,7 +1129,7 @@ const char *get_convert_attr_ascii(const char *path)
 
 int convert_to_git(const struct index_state *istate,
   const char *path, const char *src, size_t len,
-   struct strbuf *dst, enum safe_crlf checksafe)
+  struct strbuf *dst, int checksafe)
 {
int ret = 0;
struct conv_attrs ca;
@@ -1144,7 +1144,7 @@ in