Re: [PATCH v2 6/7] correct blame for files commited with CRLF

[Junio C Hamano <gitster@pobox.com>]

tboegi@web.de writes:

> Whenever a CLRF conversion is needed (or any filter us set), load the
> index temporally, before calling convert_to_git()

I am not sure if this is needed.  We should just do read_cache()
unconditionally at the beginning of fake_working_tree_commit(),
without even discarding it.  Most Git commands that work on the
working tree files follow that pattern, and I do not think the
"optimization" this patch does over that simple pattern, while it
may be nice, is an unnecessary complication.

By the way, thanks for pushing back in the [v1 6/7] discussion
thread to show me where convert_to_git() was failing.

> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  builtin/blame.c               |  6 +++++-
>  convert.c                     | 10 ++++++++++
>  convert.h                     |  1 +
>  t/t8003-blame-corner-cases.sh | 14 ++++++++++++++
>  4 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index e982fb8..a219068 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2376,7 +2376,11 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
>  		if (strbuf_read(&buf, 0, 0) < 0)
>  			die_errno("failed to read from stdin");
>  	}
> -	convert_to_git(path, buf.buf, buf.len, &buf, 0);
> +	if (convert_needs_conversion(path)) {
> +		read_cache();
> +		convert_to_git(path, buf.buf, buf.len, &buf, 0);
> +		discard_cache();
> +	}
>  	origin->file.ptr = buf.buf;
>  	origin->file.size = buf.len;
>  	pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);
> diff --git a/convert.c b/convert.c
> index 4ed5d89..02c50da 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -903,6 +903,16 @@ const char *get_convert_attr_ascii(const char *path)
>  	return "";
>  }
>  
> +int convert_needs_conversion(const char *path)
> +{
> +	struct conv_attrs ca;
> +
> +	convert_attrs(&ca, path);
> +	if (ca.drv || ca.ident || ca.crlf_action != CRLF_BINARY)
> +		return 1;
> +	return 0;
> +}
> +
>  int convert_to_git(const char *path, const char *src, size_t len,
>                     struct strbuf *dst, enum safe_crlf checksafe)
>  {
> diff --git a/convert.h b/convert.h
> index ccf436b..ffd9c32 100644
> --- a/convert.h
> +++ b/convert.h
> @@ -35,6 +35,7 @@ extern enum eol core_eol;
>  extern const char *get_cached_convert_stats_ascii(const char *path);
>  extern const char *get_wt_convert_stats_ascii(const char *path);
>  extern const char *get_convert_attr_ascii(const char *path);
> +int convert_needs_conversion(const char *path);
>  
>  /* returns 1 if *dst was used */
>  extern int convert_to_git(const char *path, const char *src, size_t len,
> diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
> index 6568429..a9b266f 100755
> --- a/t/t8003-blame-corner-cases.sh
> +++ b/t/t8003-blame-corner-cases.sh
> @@ -212,4 +212,18 @@ test_expect_success 'blame file with CRLF attributes text' '
>  	grep "A U Thor" actual
>  '
>  
> +test_expect_success 'blame file with CRLF core.autocrlf=true' '
> +	git config core.autocrlf false &&
> +	printf "testcase\r\n" >crlfinrepo &&
> +	>.gitattributes &&
> +	git add crlfinrepo &&
> +	git commit -m "add crlfinrepo" &&
> +	git config core.autocrlf true &&
> +	mv crlfinrepo tmp &&
> +	git checkout crlfinrepo &&
> +	rm tmp &&
> +	git blame crlfinrepo >actual &&
> +	grep "A U Thor" actual
> +'
> +
>  test_done