git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 1/1] Introduce git add --renormalize .

From
Junio C Hamano <gitster@pobox.com>
Date
Nov 7, 2017, 05:50 UTC
Message-ID
<xmqqvaimeixm.fsf@gitster.mtv.corp.google.com>
In-Reply-To
<20171030162913.23164-1-tboegi@web.de>
tboegi@web.de writes:
Show 8 quoted lines
> From: Torsten Bögershausen <tboegi@web.de>
>
> Make it safer to normalize the line endings in a repository:
> Files that had been commited with CRLF will be commited with LF.
> ...
> Helped-By: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
Nicely explained.
Show 10 quoted lines
> @@ -175,6 +175,12 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files.
>  	warning (e.g., if you are manually performing operations on
>  	submodules).
>  
> +--renormalize::
> +	Normalizes the line endings from CRLF to LF of tracked files.
> +	This applies to files which are either "text" or "text=auto"
> +	in .gitattributes (or core.autocrlf is true or input)
> +	--renormalize implies -u
> +
OK.
Show 14 quoted lines
> +static int renormalize_tracked_files(const struct pathspec *pathspec, int flags)
> +{
> +	int i, retval = 0;
> +
> +	for (i = 0; i < active_nr; i++) {
> +		struct cache_entry *ce = active_cache[i];
> +
> +		if (ce_stage(ce))
> +			continue; /* do not touch unmerged paths */
> +		if (!S_ISREG(ce->ce_mode) && !S_ISLNK(ce->ce_mode))
> +			continue; /* do not touch non blobs */
> +		if (pathspec && !ce_path_match(ce, pathspec, NULL))
> +			continue;
> +		retval |= add_file_to_cache(ce->name, flags | HASH_RENORMALIZE);

We are removing the entry and then adding the same entry under the same name, and iteration over the active_cache[] from 0 through active_nr should be safe, I guess.

> ...
> -	add_new_files = !take_worktree_changes && !refresh_only;
> +	add_new_files = !take_worktree_changes && !refresh_only && !add_renormalize;
If renormalize is given, we will *not* take new files, good.
Show 12 quoted lines
> @@ -500,7 +521,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  
>  	plug_bulk_checkin();
>  
> -	exit_status |= add_files_to_cache(prefix, &pathspec, flags);
> +	if (add_renormalize)
> +		exit_status |= renormalize_tracked_files(&pathspec, flags);
> +	else
> +		exit_status |= add_files_to_cache(prefix, &pathspec, flags);
>  
>  	if (add_new_files)
>  		exit_status |= add_files(&dir, flags);
OK.
Show 22 quoted lines
> ...
>  	int add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|
>  			  (intent_only ? ADD_CACHE_NEW_ONLY : 0));
> +	int newflags = HASH_WRITE_OBJECT;
> +
> +	if (flags & HASH_RENORMALIZE)
> +		newflags |= HASH_RENORMALIZE;
> ...
> @@ -678,19 +682,23 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>  	if (ignore_case) {
>  		adjust_dirname_case(istate, ce->name);
>  	}
> +	if (!(flags & HASH_RENORMALIZE)) {
> +		alias = index_file_exists(istate, ce->name,
> +					  ce_namelen(ce), ignore_case);
> +		if (alias &&
> +		    !ce_stage(alias) &&
> +		    !ie_match_stat(istate, alias, st, ce_option)) {
> +			/* Nothing changed, really */
> +			if (!S_ISGITLINK(alias->ce_mode))
> +				ce_mark_uptodate(alias);
> +			alias->ce_flags |= CE_ADDED;

OK, so RENORMALIZE option forces the code to skip the "does the path exist already? maybe we can do without adding it?" check.

>  	if (!intent_only) {
> -		if (index_path(&ce->oid, path, st, HASH_WRITE_OBJECT)) {
> +		if (index_path(&ce->oid, path, st, newflags)) {

And then we do hash it. We later do add_index_entry() on this thing and we have OK_TO_REPLACE bit in the add_option, so we are good to go.

Show 41 quoted lines
> diff --git a/sha1_file.c b/sha1_file.c
> index 10c3a0083d..15abb184c2 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -74,6 +74,18 @@ static struct cached_object *find_cached_object(const unsigned char *sha1)
>  	return NULL;
>  }
>  
> +
> +static enum safe_crlf get_safe_crlf(unsigned flags)
> +{
> +	if (flags & HASH_RENORMALIZE)
> +		return SAFE_CRLF_RENORMALIZE;
> +	else if (flags & HASH_WRITE_OBJECT)
> +		return safe_crlf;
> +	else
> +		return SAFE_CRLF_FALSE;
> +}
> +
> +
>  int mkdir_in_gitdir(const char *path)
>  {
>  	if (mkdir(path, 0777)) {
> @@ -1680,7 +1692,7 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,
>  	if ((type == OBJ_BLOB) && path) {
>  		struct strbuf nbuf = STRBUF_INIT;
>  		if (convert_to_git(&the_index, path, buf, size, &nbuf,
> -				   write_object ? safe_crlf : SAFE_CRLF_FALSE)) {
> +				   get_safe_crlf(flags))) {
>  			buf = strbuf_detach(&nbuf, &size);
>  			re_allocated = 1;
>  		}
> @@ -1714,7 +1726,7 @@ static int index_stream_convert_blob(unsigned char *sha1, int fd,
>  	assert(would_convert_to_git_filter_fd(path));
>  
>  	convert_to_git_filter_fd(&the_index, path, fd, &sbuf,
> -				 write_object ? safe_crlf : SAFE_CRLF_FALSE);
> +				 get_safe_crlf(flags));
>  
>  	if (write_object)
>  		ret = write_sha1_file(sbuf.buf, sbuf.len, typename(OBJ_BLOB),

OK. We used to force CRLF_FALSE when we are not writing it out; now we have three choices, and a new helper helps us isolating the logic to make that choice.

Show 17 quoted lines
> diff --git a/t/t0025-crlf-renormalize.sh b/t/t0025-crlf-renormalize.sh
> new file mode 100755
> index 0000000000..fb1ed631d2
> --- /dev/null
> +++ b/t/t0025-crlf-renormalize.sh
> @@ -0,0 +1,30 @@
> +#!/bin/sh
> +
> +test_description='CRLF renormalization'
> +
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +	git config core.autocrlf false &&
> +	printf "LINEONE\nLINETWO\nLINETHREE"     >LF.txt &&
> +	printf "LINEONE\r\nLINETWO\r\nLINETHREE" >CRLF.txt &&
> +	printf "LINEONE\r\nLINETWO\nLINETHREE"   >CRLF_mix_LF.txt &&

Did you mean to make all these files end with an incomplete line? I think it does not hurt but it is misleading---the reader would try to see if the incomplete lines are significant and necessary part of the test, which is not, and would end up wasting time, no?

Show 12 quoted lines
> +	git add . &&
> +	git commit -m initial
> +'
> +
> +test_expect_success 'renormalize CRLF in repo' '
> +	echo "*.txt text=auto" >.gitattributes &&
> +	git add --renormalize "*.txt" &&
> +cat >expect <<EOF &&
> +i/lf w/crlf attr/text=auto CRLF.txt
> +i/lf w/lf attr/text=auto LF.txt
> +i/lf w/mixed attr/text=auto CRLF_mix_LF.txt
> +EOF
Perhaps use the <<-\EOF pattern?

I'd suggest squashing this in (or I can do so myself if there is no other change needed).

Thanks.  Looks mostly good.
 t/t0025-crlf-renormalize.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/t/t0025-crlf-renormalize.sh b/t/t0025-crlf-renormalize.sh
index fb1ed631d2..ea4f82ea27 100755
--- a/t/t0025-crlf-renormalize.sh
+++ b/t/t0025-crlf-renormalize.sh
@@ -6,9 +6,9 @@ test_description='CRLF renormalization'
 
 test_expect_success setup '
 	git config core.autocrlf false &&
-	printf "LINEONE\nLINETWO\nLINETHREE"     >LF.txt &&
-	printf "LINEONE\r\nLINETWO\r\nLINETHREE" >CRLF.txt &&
-	printf "LINEONE\r\nLINETWO\nLINETHREE"   >CRLF_mix_LF.txt &&
+	printf "LINEONE\nLINETWO\nLINETHREE\n"       >LF.txt &&
+	printf "LINEONE\r\nLINETWO\r\nLINETHREE\r\n" >CRLF.txt &&
+	printf "LINEONE\r\nLINETWO\nLINETHREE\n"     >CRLF_mix_LF.txt &&
 	git add . &&
 	git commit -m initial
 '
@@ -16,11 +16,11 @@ test_expect_success setup '
 test_expect_success 'renormalize CRLF in repo' '
 	echo "*.txt text=auto" >.gitattributes &&
 	git add --renormalize "*.txt" &&
-cat >expect <<EOF &&
-i/lf w/crlf attr/text=auto CRLF.txt
-i/lf w/lf attr/text=auto LF.txt
-i/lf w/mixed attr/text=auto CRLF_mix_LF.txt
-EOF
+	cat >expect <<-\EOF &&
+	i/lf w/crlf attr/text=auto CRLF.txt
+	i/lf w/lf attr/text=auto LF.txt
+	i/lf w/mixed attr/text=auto CRLF_mix_LF.txt
+	EOF
 	git ls-files --eol |
 	sed -e "s/	/ /g" -e "s/  */ /g" |
 	sort >actual &&
Previous: tboegi@web.deNext: Torsten Bögershausen
Message 20 of 34 in “Line ending normalization doesn't work as expected”
  1. Robert DaileyOct 3, 2017
  2. Torsten BögershausenOct 3, 2017
  3. Robert DaileyOct 3, 2017
  4. Torsten BögershausenOct 3, 2017
  5. Junio C HamanoOct 4, 2017
  6. Robert DaileyOct 4, 2017
  7. Jonathan NiederOct 4, 2017
  8. Robert DaileyOct 4, 2017
  9. Junio C HamanoOct 5, 2017
  10. Jonathan NiederOct 5, 2017
  11. Torsten BögershausenOct 4, 2017
  12. Junio C HamanoOct 5, 2017
  13. Junio C HamanoOct 5, 2017
  14. Torsten BögershausenOct 5, 2017
  15. Junio C HamanoOct 6, 2017
  16. Torsten BögershausenOct 6, 2017
  17. 1/1 Introduce git add --renormalize .tboegi@web.de, Oct 16, 2017
  18. Junio C HamanoOct 16, 2017
  19. 1/1 Introduce git add --renormalize .tboegi@web.de, Oct 30, 2017
  20. Junio C HamanoNov 7, 2017
  21. Torsten BögershausenNov 7, 2017
  22. Junio C HamanoNov 8, 2017
  23. Torsten BögershausenNov 9, 2017
  24. Junio C HamanoNov 10, 2017
  25. Torsten BögershausenNov 12, 2017
  26. 1/1 Introduce git add --renormalize .tboegi@web.de, Nov 16, 2017
  27. Junio C HamanoNov 17, 2017
  28. Eric SunshineNov 17, 2017
  29. Junio C HamanoNov 18, 2017
  30. Robert DaileyFeb 15, 2018
  31. Junio C HamanoFeb 15, 2018
  32. Robert DaileyFeb 15, 2018
  33. Torsten BögershausenFeb 16, 2018
  34. Robert DaileyFeb 16, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.