> On 01 Jan 2018, at 22:59, tboegi@xxxxxx wrote:
>
> From: Torsten Bögershausen <tboegi@xxxxxx>
>
> 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 <larsxschneider@xxxxxxxxx>
> Signed-off-by: Torsten Bögershausen <tboegi@xxxxxx>
> ---
> 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/20171229152222.39680-1-lars.schneider@xxxxxxxxxxxx/
Thanks,
Lars
