Re: [PATCH/RFC] File commited with CRLF should roundtrip diff and apply
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 14, 2017, 17:37 UTC
- Message-ID
- <xmqqa832vymw.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170812145625.31560-1-tboegi@web.de>
tboegi@web.de writes:
Show 9 quoted lines
> From: Torsten Bögershausen <tboegi@web.de> > > When a file had been commited with CRLF and core.autocrlf is true, > the following does not roundtrip, `git apply` fails: > > printf "Added line\r\n" >>file && > git diff >patch && > git checkout -- . && > git apply patch
Should this tweak be in effect when we are paying attention to the contents of the index? Or does it matter only when we are doing "git apply" without either "--index" or "--cache"?
The fact that the new flag that is passed from load_preimage() down to read_old_data(), the latter of which is only about the "behave as a better 'patch -p1', ignoring the index" mode, hints that this should not affect anything when we are paying attention to the index, but then calling the load_patch_target() helper with a very generic CR_AT_EOL flag looks a bit misleading, by making it appear as if the caller is telling all three cases to do the funny CR business. I wonder if we instead want to pass the "patch" structure down the callchain and have _only_ read_old_data() pay attention to the has_crlf bit in it---that way, it becomes more obvious what is going on.
I also notice that read_old_data() still passes &the_index to convert_to_git(). Can we fix convert_to_git() further to allow NULL as the istate parameter when we tell it not to look at the index ( I am reading your passing SAFE_CRLF_KEEP and SAFE_CRLF_FALSE as a way for the caller to tell that the caller knows what it wants already and does not want covnert_to_git() to look at the index)?
With or without CR in the patch, I do not see any reason for the codepath from read_old_data() down to the convert API to look at the index, ever, as read_old_data() is called only when (!state->cached && !state->check_index), i.e. when we are working as a better 'patch -p1' without even having to know that we are in a Git repository, by load_patch_target().
Show 18 quoted lines
> diff --git a/apply.c b/apply.c
> index f2d599141d..63455cd65f 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -220,6 +220,7 @@ struct patch {
> unsigned int recount:1;
> unsigned int conflicted_threeway:1;
> unsigned int direct_to_threeway:1;
> + unsigned int has_crlf:1;
> struct fragment *fragments;
> char *result;
> size_t resultsize;
> @@ -1662,6 +1663,17 @@ static void check_whitespace(struct apply_state *state,
> record_ws_error(state, result, line + 1, len - 2, state->linenr);
> }
>
> +/* Check if the patch has context lines with CRLF or
> + the patch wants to remove lines with CRLF */Style.
Show 7 quoted lines
> +static void check_old_for_crlf(struct patch *patch, const char *line, int len)
> +{
> + if (len >= 2 && line[len-1] == '\n' && line[len-2] == '\r') {
> + patch->ws_rule |= WS_CR_AT_EOL;
> + patch->has_crlf = 1;
> + }
> +}I am wondering where the discrepancy between the names (old crlf here, as opposed to "struct patch" that says "has_crlf" implying it does not care which side between old and new has one) comes from.
Is the intention that you only care about what is expected of the preimage?
Show 9 quoted lines
> @@ -1712,11 +1724,13 @@ static int parse_fragment(struct apply_state *state, > if (!deleted && !added) > leading++; > trailing++; > + check_old_for_crlf(patch, line, len); > if (!state->apply_in_reverse && > state->ws_error_action == correct_ws_error) > check_whitespace(state, line, len, patch->ws_rule); > break;
If check_old is about "we care about the lines that are supposed to match what currently exist in the target file to be patched", then the call to it also must pay attention to apply_in_reverse. What appears on '+' lines in a patch will become the "expected old contents the patched target is supposed to have" when we are running "apply -R".
Show 5 quoted lines
> case '-': > + check_old_for_crlf(patch, line, len); > if (state->apply_in_reverse && > state->ws_error_action != nowarn_ws_error) > check_whitespace(state, line, len, patch->ws_rule);
Likewise.
Thanks for working on this; unlike the previous one I commented, I think this one I can sort of see how this is an approach to fix it.