Re: [PATCH v3 1/1] convert_to_git(): checksafe becomes int conv_flags
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
> 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
> On 03 Jan 2018, at 06:36, Torsten Bögershausenwrote: > > 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
> 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
From: Torsten BögershausenWhen 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