[PATCH RFC 0/5] Patches to avoid reporting conversion changes.

[PATCH RFC 0/5] Patches to avoid reporting conversion changes.

[Henrik Grubbström (Grubba)]

This is the first go at having the git index keep track of the
conversion mode and corresponding normalized blob for files.

The approach is relatively straight-forward:
  * struct cache_entry has been extended with two fields,
    norm_flags and norm_sha1, to keep track of the data.
  * An accessor function ce_norm_sha1() has been added,
    which checks that the fields seem valid, and otherwise
    recalculates them.
  * run_diff_files() and ce_compare_data() compare against
    the normalized blob.

Still missing is a testsuite, and I may have missed some place
that ought to use ce_norm_sha1().

Thanks to Junio C Hamano for the suggestion.

Henrik Grubbström (Grubba) (5):
  sha1_file: Added index_blob().
  cache: Added ce_norm_sha1() and related cache_entry fields.
  cache: Added index extension "NORM".
  reachable: Made the gc aware of the ce_norm_sha1.
  cache: Use ce_norm_sha1().

 cache.h      |   29 +++++++++++++++++++++++++++++
 convert.c    |   35 +++++++++++++++++++++++++++++++++++
 diff-lib.c   |    9 ++++++---
 reachable.c  |    2 ++
 read-cache.c |   51 ++++++++++++++++++++++++++++++++++++++++++++-------
 sha1_file.c  |   19 +++++++++++++++++++
 6 files changed, 135 insertions(+), 10 deletions(-)

[PATCH RFC 1/5] sha1_file: Added index_blob().

[Henrik Grubbström (Grubba)]

When conversion attributes have changed, it
is useful to be able to easily reconvert an
existing blob.

Signed-off-by: Henrik Grubbström <grubba@grubba.org>
---
 cache.h     |    1 +
 sha1_file.c |   19 +++++++++++++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/cache.h b/cache.h
index 5eb0573..1fe2d7d 100644
--- a/cache.h
+++ b/cache.h
@@ -494,6 +494,7 @@ extern int ie_match_stat(const struct index_state *, struct cache_entry *, struc
 extern int ie_modified(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);
 
 extern int ce_path_match(const struct cache_entry *ce, const char **pathspec);
+extern int index_blob(unsigned char *dst_sha1, const unsigned char *src_sha1, int write_object, const char *path);
 extern int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object, enum object_type type, const char *path);
 extern int index_path(unsigned char *sha1, const char *path, struct stat *st, int write_object);
 extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);
diff --git a/sha1_file.c b/sha1_file.c
index ff65328..c162321 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2434,6 +2434,25 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,
 
 #define SMALL_FILE_SIZE (32*1024)
 
+int index_blob(unsigned char *dst_sha1, const unsigned char *src_sha1,
+	       int write_object, const char *path)
+{
+	void *buf;
+	unsigned long buflen = 0;
+	int ret;
+
+	memcpy(dst_sha1, src_sha1, 20);
+	buf = read_object_with_reference(src_sha1, typename(OBJ_BLOB),
+					 &buflen, dst_sha1);
+	if (!buf)
+		return 0;
+
+	ret = index_mem(dst_sha1, buf, buflen, write_object, OBJ_BLOB, path);
+	free(buf);
+
+	return ret;
+}
+
 int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 	     enum object_type type, const char *path)
 {
-- 
1.7.0.4.369.g81e89

[PATCH RFC 2/5] cache: Added ce_norm_sha1() and related cache_entry fields.

[Henrik Grubbström (Grubba)]

Added function to retrieve the sha1 for a (re-)normalized cache_entry.
It makes a reasonable effort at detecting conversion mode changes.

Signed-off-by: Henrik Grubbström <grubba@grubba.org>
---
Note: This implementation will miss a changed custom filter.

 cache.h   |   21 +++++++++++++++++++++
 convert.c |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/cache.h b/cache.h
index 1fe2d7d..536697d 100644
--- a/cache.h
+++ b/cache.h
@@ -151,10 +151,17 @@ struct cache_entry {
 	unsigned int ce_size;
 	unsigned int ce_flags;
 	unsigned char sha1[20];
+	unsigned int norm_flags;
+	unsigned char norm_sha1[20];
 	struct cache_entry *next;
 	char name[FLEX_ARRAY]; /* more */
 };
 
+#define NORM_CONV_CRLF	0x0001
+#define NORM_CONV_GUESS	0x0002
+#define NORM_CONV_IDENT	0x0004
+#define NORM_CONV_FILT	0x0008
+
 #define CE_NAMEMASK  (0x0fff)
 #define CE_STAGEMASK (0x3000)
 #define CE_EXTENDED  (0x4000)
@@ -1014,6 +1021,7 @@ extern void trace_argv_printf(const char **argv, const char *format, ...);
 
 /* convert.c */
 /* returns 1 if *dst was used */
+extern unsigned int git_norm_flags(const char *path);
 extern int convert_to_git(const char *path, const char *src, size_t len,
                           struct strbuf *dst, enum safe_crlf checksafe);
 extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);
@@ -1062,4 +1070,17 @@ int split_cmdline(char *cmdline, const char ***argv);
 /* builtin/merge.c */
 int checkout_fast_forward(const unsigned char *from, const unsigned char *to);
 
+static inline unsigned char *ce_norm_sha1(struct cache_entry *ce)
+{
+	unsigned int norm_flags = git_norm_flags(ce->name);
+
+	if (!norm_flags)
+		return ce->sha1;
+	if (norm_flags == ce->norm_flags)
+		return ce->norm_sha1;
+	index_blob(ce->norm_sha1, ce->sha1, 1, ce->name);
+   	ce->norm_flags = norm_flags;
+	return ce->norm_sha1;
+}
+
 #endif /* CACHE_H */
diff --git a/convert.c b/convert.c
index 4f8fcb7..6378ef5 100644
--- a/convert.c
+++ b/convert.c
@@ -568,6 +568,41 @@ static int git_path_check_ident(const char *path, struct git_attr_check *check)
 	return !!ATTR_TRUE(value);
 }
 
+unsigned int git_norm_flags(const char *path)
+{
+	struct git_attr_check check[3];
+	int crlf = CRLF_GUESS;
+	int ident = 0;
+	unsigned ret = 0;
+	struct convert_driver *drv = NULL;
+
+	setup_convert_check(check);
+	if (!git_checkattr(path, ARRAY_SIZE(check), check)) {
+		crlf = git_path_check_crlf(path, check + 0);
+		ident = git_path_check_ident(path, check + 1);
+		drv = git_path_check_convert(path, check + 2);
+	}
+
+	switch(crlf) {
+	case CRLF_INPUT:
+	case CRLF_TEXT:
+		ret |= NORM_CONV_CRLF;
+		break;
+	case CRLF_GUESS:
+		ret |= NORM_CONV_GUESS;
+		break;
+	case CRLF_BINARY:
+		break;
+	}
+	if (ident) {
+		ret |= NORM_CONV_IDENT;
+	}
+	if (drv) {
+		ret |= NORM_CONV_FILT;
+	}
+	return ret;
+}
+
 int convert_to_git(const char *path, const char *src, size_t len,
                    struct strbuf *dst, enum safe_crlf checksafe)
 {
-- 
1.7.0.4.369.g81e89

[PATCH RFC 3/5] cache: Added index extension "NORM".

[Henrik Grubbström (Grubba)]

The index can now store and retrieve the ce_norm_sha1 data.

Signed-off-by: Henrik Grubbström <grubba@grubba.org>
---
 cache.h      |    7 +++++++
 read-cache.c |   49 +++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/cache.h b/cache.h
index 536697d..0cb3110 100644
--- a/cache.h
+++ b/cache.h
@@ -157,6 +157,13 @@ struct cache_entry {
 	char name[FLEX_ARRAY]; /* more */
 };
 
+struct ondisk_norm_sha1 {
+	unsigned int entry_no;
+	unsigned int norm_flags;
+	unsigned int norm_size;
+	unsigned char norm_sha1[20];
+};
+
 #define NORM_CONV_CRLF	0x0001
 #define NORM_CONV_GUESS	0x0002
 #define NORM_CONV_IDENT	0x0004
diff --git a/read-cache.c b/read-cache.c
index f1f789b..002160e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -27,6 +27,7 @@ static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int reall
 #define CACHE_EXT(s) ( (s[0]<<24)|(s[1]<<16)|(s[2]<<8)|(s[3]) )
 #define CACHE_EXT_TREE 0x54524545	/* "TREE" */
 #define CACHE_EXT_RESOLVE_UNDO 0x52455543 /* "REUC" */
+#define CACHE_EXT_NORM_SHA1 0x4e4f524d	/* "NORM" */
 
 struct index_state the_index;
 
@@ -1178,6 +1179,21 @@ static int verify_hdr(struct cache_header *hdr, unsigned long size)
 	return 0;
 }
 
+static int norm_sha1_read(struct cache_entry **cache, unsigned int entries,
+			  const struct ondisk_norm_sha1 *data, unsigned long sz)
+{
+	while (sz >= sizeof(*data)) {
+		unsigned int entry_no = ntohl(data->entry_no);
+		if (entry_no < entries) {
+			cache[entry_no]->norm_flags = ntohl(data->norm_flags);
+			memcpy(cache[entry_no]->norm_sha1, data->norm_sha1, 20);
+		}
+		sz -= sizeof(*data);
+		data++;
+	}
+	return 0;
+}
+
 static int read_index_extension(struct index_state *istate,
 				const char *ext, void *data, unsigned long sz)
 {
@@ -1188,6 +1204,9 @@ static int read_index_extension(struct index_state *istate,
 	case CACHE_EXT_RESOLVE_UNDO:
 		istate->resolve_undo = resolve_undo_read(data, sz);
 		break;
+	case CACHE_EXT_NORM_SHA1:
+		return norm_sha1_read(istate->cache, istate->cache_nr, data, sz);
+		break;
 	default:
 		if (*ext < 'A' || 'Z' < *ext)
 			return error("index uses %.4s extension, which we do not understand",
@@ -1511,6 +1530,16 @@ static void ce_smudge_racily_clean_entry(struct cache_entry *ce)
 	}
 }
 
+static void norm_sha1_write(struct strbuf *sb, const struct cache_entry *ce,
+			    int entry_no)
+{
+	struct ondisk_norm_sha1 entry;
+	entry.entry_no = htonl(entry_no);
+	entry.norm_flags = htonl(ce->norm_flags);
+	memcpy(entry.norm_sha1, ce->norm_sha1, 20);
+	strbuf_add(sb, &entry, sizeof(entry));
+}
+
 static int ce_write_entry(git_SHA_CTX *c, int fd, struct cache_entry *ce)
 {
 	int size = ondisk_ce_size(ce);
@@ -1546,10 +1575,11 @@ int write_index(struct index_state *istate, int newfd)
 {
 	git_SHA_CTX c;
 	struct cache_header hdr;
-	int i, err, removed, extended;
+	int i, j, err, removed, extended;
 	struct cache_entry **cache = istate->cache;
 	int entries = istate->cache_nr;
 	struct stat st;
+	struct strbuf sb = STRBUF_INIT;
 
 	for (i = removed = extended = 0; i < entries; i++) {
 		if (cache[i]->ce_flags & CE_REMOVE)
@@ -1572,7 +1602,7 @@ int write_index(struct index_state *istate, int newfd)
 	if (ce_write(&c, newfd, &hdr, sizeof(hdr)) < 0)
 		return -1;
 
-	for (i = 0; i < entries; i++) {
+	for (i = j = 0; i < entries; i++) {
 		struct cache_entry *ce = cache[i];
 		if (ce->ce_flags & CE_REMOVE)
 			continue;
@@ -1580,12 +1610,21 @@ int write_index(struct index_state *istate, int newfd)
 			ce_smudge_racily_clean_entry(ce);
 		if (ce_write_entry(&c, newfd, ce) < 0)
 			return -1;
+		if (ce->norm_flags)
+			norm_sha1_write(&sb, ce, j);
+		j++;
 	}
 
 	/* Write extension data here */
+	if (sb.len) {
+		err = write_index_ext_header(&c, newfd, CACHE_EXT_NORM_SHA1,
+					     sb.len) < 0
+			|| ce_write(&c, newfd, sb.buf, sb.len) < 0;
+		strbuf_release(&sb);
+		if (err)
+			return -1;
+	}
 	if (istate->cache_tree) {
-		struct strbuf sb = STRBUF_INIT;
-
 		cache_tree_write(&sb, istate->cache_tree);
 		err = write_index_ext_header(&c, newfd, CACHE_EXT_TREE, sb.len) < 0
 			|| ce_write(&c, newfd, sb.buf, sb.len) < 0;
@@ -1594,8 +1633,6 @@ int write_index(struct index_state *istate, int newfd)
 			return -1;
 	}
 	if (istate->resolve_undo) {
-		struct strbuf sb = STRBUF_INIT;
-
 		resolve_undo_write(&sb, istate->resolve_undo);
 		err = write_index_ext_header(&c, newfd, CACHE_EXT_RESOLVE_UNDO,
 					     sb.len) < 0
-- 
1.7.0.4.369.g81e89

[PATCH RFC 4/5] reachable: Made the gc aware of the ce_norm_sha1.

[Henrik Grubbström (Grubba)]

Avoid having the gc zap the normalized blobs in the index.

Signed-off-by: Henrik Grubbström <grubba@grubba.org>
---
 reachable.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/reachable.c b/reachable.c
index b515fa2..61ea623 100644
--- a/reachable.c
+++ b/reachable.c
@@ -191,6 +191,8 @@ static void add_cache_refs(struct rev_info *revs)
 		 * added them as objects, we've really done everything
 		 * there is to do for a blob
 		 */
+		if (active_cache[i]->norm_flags)
+			lookup_blob(active_cache[i]->norm_sha1);
 	}
 	if (active_cache_tree)
 		add_cache_tree(active_cache_tree, revs);
-- 
1.7.0.4.369.g81e89

[PATCH RFC 5/5] cache: Use ce_norm_sha1().

[Henrik Grubbström (Grubba)]

When the conversion filter for a file is changed, the file may get
listed as modified even though the user has not made any changes to it.
This patch makes the index ignore such changes. It also makes git-diff
compare with the normalized content rather than the original content.

Signed-off-by: Henrik Grubbström <grubba@grubba.org>
---
 diff-lib.c   |    9 ++++++---
 read-cache.c |    2 +-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index c9f6e05..ae6118d 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -95,6 +95,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		struct cache_entry *ce = active_cache[i];
 		int changed;
 		unsigned dirty_submodule = 0;
+		const unsigned char *norm_sha1;
 
 		if (DIFF_OPT_TST(&revs->diffopt, QUICK) &&
 			DIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))
@@ -147,7 +148,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 				if (2 <= stage) {
 					int mode = nce->ce_mode;
 					num_compare_stages++;
-					hashcpy(dpath->parent[stage-2].sha1, nce->sha1);
+					hashcpy(dpath->parent[stage-2].sha1,
+						ce_norm_sha1(nce));
 					dpath->parent[stage-2].mode = ce_mode_from_stat(nce, mode);
 					dpath->parent[stage-2].status =
 						DIFF_STATUS_MODIFIED;
@@ -195,7 +197,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			if (silent_on_removed)
 				continue;
 			diff_addremove(&revs->diffopt, '-', ce->ce_mode,
-				       ce->sha1, ce->name, 0);
+				       ce_norm_sha1(ce), ce->name, 0);
 			continue;
 		}
 		changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
@@ -207,8 +209,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		}
 		oldmode = ce->ce_mode;
 		newmode = ce_mode_from_stat(ce, st.st_mode);
+		norm_sha1 = ce_norm_sha1(ce);
 		diff_change(&revs->diffopt, oldmode, newmode,
-			    ce->sha1, (changed ? null_sha1 : ce->sha1),
+			    norm_sha1, (changed ? null_sha1 : norm_sha1),
 			    ce->name, 0, dirty_submodule);
 
 	}
diff --git a/read-cache.c b/read-cache.c
index 002160e..b631de9 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -94,7 +94,7 @@ static int ce_compare_data(struct cache_entry *ce, struct stat *st)
 	if (fd >= 0) {
 		unsigned char sha1[20];
 		if (!index_fd(sha1, fd, st, 0, OBJ_BLOB, ce->name))
-			match = hashcmp(sha1, ce->sha1);
+			match = hashcmp(sha1, ce_norm_sha1(ce));
 		/* index_fd() closed the file descriptor already */
 	}
 	return match;
-- 
1.7.0.4.369.g81e89

Re: [PATCH RFC 0/5] Patches to avoid reporting conversion changes.

[Jari Aalto]

merlyn@stonehenge.com (Randal L. Schwartz) writes:

>>>>>> "Jari" == Jari Aalto <jari.aalto@cante.net> writes:
>
> Jari> It would be interesting to know why not. These magic variables are hard
> Jari> to read and remember without consulting the manual pages.
>
> Because it was observed over time that the aliases were *also* hard to
> remember without consulting the manpages. :)
>
> So you were merely trading one problem for another, and since far more
> code is out there that does *not* use English than does, we agreed that
> use English was an interesting but failed experiment.

Who found out? In what study?

Honestly, that's not believable. There is no way reading the most common
alphabetic variables would not be readable over puctuation:

    $OUTPUT_AUTOFLUSH = 1;
    if ( $OS_ERROR ) ...
    if ( $EVAL_ERROR ) ...
    open(...)   or die "$ERRNO"

Versus:

    $| = 1;
    if ($?) ...
    if ($@) ...
    open(...) or die $!

It *may* be the case with exotic (seldomly used) ones, but they are even
more exotic in they short form:

     print $^V;

Eh?

    print $PERL_VERSION;

I compute sir,
Jari

Re: [PATCH RFC 0/5] Patches to avoid reporting conversion changes.

[Randal L. Schwartz]

>>>>> "Jari" == Jari Aalto <jari.aalto@cante.net> writes:

Jari>     if ( $OS_ERROR ) ...

Right, but without looking, is it $OS_ERROR or $OSERROR?

That's the problem.

You're trading a list of single punctuation characters, pretty
unambiguous, for things that could have been named a dozen different
ways each.

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion

Re: [PATCH RFC 0/5] Patches to avoid reporting conversion changes.

[Jari Aalto]

merlyn@stonehenge.com (Randal L. Schwartz) writes:

>>>>>> "Jari" == Jari Aalto <jari.aalto@cante.net> writes:
>
> Jari>     if ( $OS_ERROR ) ...
>
> Right, but without looking, is it $OS_ERROR or $OSERROR?

I don't see any difference, because that variable is always within the
close context of previous statements. The reader would consult the lines
above.

And if that's the only problem, which I don't believe it is for a Perl
programmer, there is also $ERRNO.

> You're trading a list of single punctuation characters, pretty
> unambiguous, for things that could have been named a dozen different
> ways each.

A typical Perl program used those "single puctuation variables" and they
are not immediately understandable; unless you know them by heart.

They are cute for one-liners, but not suitable for maintainable
programs, edited by N developers, with different backgrounds and skills.

To make the code actually readable by anyone, not just by a Perl coder
breathing the language 24/7, is what software, any software would be
better off.

It's akin to the opening "magic values" in a program:

    100
    130
    140

Good programmers write instead (pseudo code):

    ERROR_MINOR = 100
    ...


    return ERROR_MINOR

Jari

Re: [PATCH RFC 0/5] Patches to avoid reporting conversion changes.

[Randal L. Schwartz]

>>>>> "Jari" == Jari Aalto <jari.aalto@cante.net> writes:

>> Right, but without looking, is it $OS_ERROR or $OSERROR?

Jari> I don't see any difference, because that variable is always within the
Jari> close context of previous statements. The reader would consult the lines
Jari> above.

Yes, once it's already *in* the program.  But I bet you had to *look
them up* to add them.

And if you weren't familiar with Perl, you'd still have to *look them
up* to get the punctuation variables.

You're just trading one problem for another.

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion

Re: [PATCH RFC 0/5] Patches to avoid reporting conversion changes.

[Sverre Rabbelier]

Heya,

On Sat, Apr 17, 2010 at 21:34, Randal L. Schwartz <merlyn@stonehenge.com> wrote:
> Yes, once it's already *in* the program.  But I bet you had to *look
> them up* to add them.

Yes, but once they're there nobody has to look them up. It's moving
the problem from having to look up what it means on _and_ write, to
just write.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH RFC 0/5] Patches to avoid reporting conversion changes.

[Jakub Narebski]

Sverre Rabbelier <srabbelier@gmail.com> writes:
> On Sat, Apr 17, 2010 at 21:34, Randal L. Schwartz <merlyn@stonehenge.com> wrote:

> > Yes, once it's already *in* the program.  But I bet you had to *look
> > them up* to add them.
> 
> Yes, but once they're there nobody has to look them up. It's moving
> the problem from having to look up what it means on _and_ write, to
> just write.

We use idiomatic C, e.g.

  if (!strcmp(option, "warn")) {

not

  if (strcmp(option, "warn") == 0) {


We use idiomatic Perl, e.g.

  %hash = map { $_ => 1 } @list;

not

  use English qw(-no_match_vars);

  %hash = map { $ARG => 1 } @list;

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH RFC 0/5] Patches to avoid reporting conversion changes.

[Sverre Rabbelier]

Heya,

On Sun, Apr 18, 2010 at 00:32, Jakub Narebski <jnareb@gmail.com> wrote:
> We use idiomatic C, e.g.

Agreed.

> We use idiomatic Perl, e.g.

Ah, I thought that the discussion was about whether the "$_" syntax
was idiomatic or not. It got the impression that the "$ARG etc. is a
failed experiment" was Randal's personal opinion, but if that's how
the perl community has decided that things should be done than that's
of course how we should do it :).

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH RFC 0/5] Patches to avoid reporting conversion changes.

[Randal L. Schwartz]

>>>>> "Sverre" == Sverre Rabbelier <srabbelier@gmail.com> writes:

>> We use idiomatic Perl, e.g.

Sverre> Ah, I thought that the discussion was about whether the "$_" syntax
Sverre> was idiomatic or not. It got the impression that the "$ARG etc. is a
Sverre> failed experiment" was Randal's personal opinion, but if that's how
Sverre> the perl community has decided that things should be done than that's
Sverre> of course how we should do it :).

Yes.  Within the Perl community, $_ is idiomatic.

"use English" is discouraged.  It was an interesting experiment, but it
failed.

Please stop making me repeat myself.

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Smalltalk/Perl/Unix consulting, Technical writing, Comedy, etc. etc.
See http://methodsandmessages.vox.com/ for Smalltalk and Seaside discussion

Re: [PATCH RFC 5/5] cache: Use ce_norm_sha1().

[Junio C Hamano]

"Henrik Grubbström (Grubba)"  <grubba@grubba.org> writes:

> When the conversion filter for a file is changed, the file may get
> listed as modified even though the user has not made any changes to it.
> This patch makes the index ignore such changes. It also makes git-diff
> compare with the normalized content rather than the original content.

Hmm, I am not happy with this.  A typical use case I am imagining goes
like this:

 0. You have a project with LF line ending.  You clone to a filesystem
    that needs autocrlf but somehow it is not set, and end up with files
    with LF line ending in your working tree.

 1. You notice the mistake, and set autocrlf.  "git diff" does not say
    anything, as the index is clean.

 2. Once you fixed the line endings in the working tree files, however,
    "git diff" will say the files are different, but there is no actual
    change (i.e. you see "diff --git a/file b/file" and nothing else).

 3. "git update-index --refresh" does not improve the situation, as it
    (thinks) it knows the blob and the working tree file are different.

I was hoping to see a solution where you will add a stronger version of
"refresh" without having to do anything else other than recording "how did
I munge the file in the working tree to produce the blob".  The third step
would change to:

 3. "git update-index --refresh" notices that the conversion parameters
    are different since the last time the files in the working tree were
    looked at (i.e. immediately after a "clone", working tree files are
    what git wrote out using convert_to_working_tree() and you know what
    conversion you used; after the user modified files in the working tree
    and said "git add", you know you what conversion parameters you ran
    convert_to_git() with to produce blobs).  The paths that has different
    conversion parameters are re-indexed to see if they hash to the same
    sha1 as recorded in the index.  If they have changed, their index
    entries are left intact (i.e. you will still show the differences);
    otherwise you update the cached stat information for their index
    entries.

The above example scenario is about crlf conversion, but the same idea
should apply to other types of conversions (e.g. smudge/clear filter
pair), no?

I can see that it would be benefitial to store what conversions were used
to turn the input into the canonical version that resulted in the object
store and registered in the index, but I am not sure why the re-indexed
versions need to be even stored in the index (either in-core, let alone
on-disk) nor produce new blob objects.  What am I missing?

Re: [PATCH RFC 5/5] cache: Use ce_norm_sha1().

[Henrik Grubbström]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5265 bytes --]

On Tue, 20 Apr 2010, Junio C Hamano wrote:

> "Henrik Grubbström (Grubba)"  <grubba@grubba.org> writes:
>
>> When the conversion filter for a file is changed, the file may get
>> listed as modified even though the user has not made any changes to it.
>> This patch makes the index ignore such changes. It also makes git-diff
>> compare with the normalized content rather than the original content.
>
> Hmm, I am not happy with this.  A typical use case I am imagining goes
> like this:
>
> 0. You have a project with LF line ending.  You clone to a filesystem
>    that needs autocrlf but somehow it is not set, and end up with files
>    with LF line ending in your working tree.

Ok, but note that LF is the canonical line ending in git.

> 1. You notice the mistake, and set autocrlf.  "git diff" does not say
>    anything, as the index is clean.

Ok.

> 2. Once you fixed the line endings in the working tree files, however,
>    "git diff" will say the files are different, but there is no actual
>    change (i.e. you see "diff --git a/file b/file" and nothing else).

Ok.

> 3. "git update-index --refresh" does not improve the situation, as it
>    (thinks) it knows the blob and the working tree file are different.

False. "git update-index --refresh" uses read-cache.c:ce_compare_data() to 
compare the content of the blob with the normalized content of the working 
tree, which both now and with my patch will return true (since the blob in 
the repository already is normalized with respect to CRLF).

Let's take the reverse case instead:

  0. For some reason a file using CRLF line endings has entered the
     repository.

  1. The user notices the mistake, and sets crlf. The index is still
     clean, but the user wants the file with LF line endings, so the
     user does a "git checkout -- the_file".

  2. The index is now dirty, so the user performs a "git update-index
     --refresh".

  Unpatched:
     update-index had no effect, since the normalized working tree
     file is compared with the unnormalized repository file. The
     user will have a dirty working tree until either the normalized
     file is committed, or the attribute change is reverted.

  Patched:
     update-index will compare with the normalized repository file,
     which will be equal to the normalized working tree file. The
     user now has a clean working tree.

> I was hoping to see a solution where you will add a stronger version of
> "refresh" without having to do anything else other than recording "how did
> I munge the file in the working tree to produce the blob".  The third step
> would change to:
>
> 3. "git update-index --refresh" notices that the conversion parameters
>    are different since the last time the files in the working tree were
>    looked at (i.e. immediately after a "clone", working tree files are
>    what git wrote out using convert_to_working_tree() and you know what
>    conversion you used; after the user modified files in the working tree
>    and said "git add", you know you what conversion parameters you ran
>    convert_to_git() with to produce blobs).  The paths that has different
>    conversion parameters are re-indexed to see if they hash to the same
>    sha1 as recorded in the index.  If they have changed, their index
>    entries are left intact (i.e. you will still show the differences);
>    otherwise you update the cached stat information for their index
>    entries.

I believe that most people that have edited a file that has changed CRLF 
convention aren't interested in that all lines have changed, but in only 
the lines that have actually been edited.

> The above example scenario is about crlf conversion, but the same idea
> should apply to other types of conversions (e.g. smudge/clear filter
> pair), no?

Yes, the same should apply to other conversions.

> I can see that it would be benefitial to store what conversions were used
> to turn the input into the canonical version that resulted in the object
> store and registered in the index, but I am not sure why the re-indexed
> versions need to be even stored in the index (either in-core, let alone
> on-disk) nor produce new blob objects.  What am I missing?

Storing the normalized sha1 is needed to reduce the amount of double work 
(eg having "git update-index --refresh" reperform convert_to_git() for
the repository blob every time a file is dirty, instead of (as now) just 
comparing the sha1 values).

Storing the normalized blob data helps reduce (or rather not increase) 
code complexity in eg "git diff", since the only change to the code is 
that a different blob sha1 is used.

The stored normalized blobs should take minimal space in the repository,
since:

  1). The most common case is most likely that the normalized blob is the
      same as the original blob (otherwise somebody else would have done
      this before).

  2). The normalized blob will probably often be stored as a delta, either
      against the original blob (eg ident), or backward against the next
      commit of the file (eg crlf).

  3). If the normalized blob isn't used in deltas, it will sooner or later
      be cleaned up by the gc.

--
Henrik Grubbström					grubba@grubba.org
Roxen Internet Software AB				grubba@roxen.com

Re: [PATCH RFC 5/5] cache: Use ce_norm_sha1().

[Junio C Hamano]

Henrik Grubbström <grubba@roxen.com> writes:

>> 3. "git update-index --refresh" does not improve the situation, as it
>>    (thinks) it knows the blob and the working tree file are different.
>
> False. "git update-index --refresh" uses
> read-cache.c:ce_compare_data() to compare the content of the blob with
> the normalized content of the working tree,...

I don't think you tried it yourself.  Here is what should happen with the
current code.

	# step 0 & 1. a project with LF ending
	$ git init two && cd two
        $ echo a quick brown fox >kuzu
        $ git add kuzu && git commit -m kuzu

        # step 2. you want CRLF in your work area
        $ echo -e "a quick brown fox\015" >kuzu
        $ git config core.autocrlf true
	$ git diff
        diff --git a/kuzu b/kuzu

        # step 3. oops, refresh
        $ git update-index --refresh
        kuzu: needs update

And it is a common thing people wish to do.  Admittedly, this is an one-off
event, so it is not _that_ urgent to fix.  You can for example do:

	# step 4. you haven't changed anything really, so you can add
        $ git add kuzu
        $ git diff
        $ git diff --cached ;# empty!

to force re-index.

> Let's take the reverse case instead:
>
>  0. For some reason a file using CRLF line endings has entered the
>     repository.

	# step 0. a blob with CRLF
        $ git init one && cd one
        $ echo -e "a quick brown fox\015" >kuzu
        $ git add kuzu && git commit -m kuzu

>  1. The user notices the mistake, and sets crlf. The index is still
>     clean, but the user wants the file with LF line endings, so the
>     user does a "git checkout -- the_file".

	# step 1. you want CRLF in work area, LF in repository
        $ git config core.autocrlf true
        $ git diff ;# clean!
        $ git checkout -- kuzu
        $ git diff ;# clean!
        $ cat -v kuzu
        a quick brown fox^M

One glitch is that this "checkout" becomes a no-op because the file is
stat-clean.  This is something your "record in the index entry what
normalization was used when we checked it out (or when we read and
hashed)" approach should be able to fix.  It however does not need the
re-indexed object name.

	Side note: if you want to have LF in both work tree and in
        repository, then you wouldn't use core.autocrlf.  Instead you
        would do:

	# step 1 (irrelevant alternative). you want LF in both
        $ dos2unix kuzu
        $ git diff ;# clean!

>  2. The index is now dirty, so the user performs a "git update-index
>     --refresh".

I think you see exactly the same behaviour in the example sequence I gave
you (blobs with LF with working files with CRLF, core.autocrlf set) and in
your example sequence (blobs with CRLF with working files with LF,
core.autocrlf set) in this case.  What happens to my example are already
shown above.  Continuing your example, because in reality the index is not
dirty, we would need to make it stat-dirty first.

	# step 2. you try to refresh
        $ touch kuzu
        $ git update-index --refresh
        kuzu: needs update
        $ git checkout -- kuzu
        $ cat -v kuzu
        a quick brown fox^M
        $ git diff ;# shows changes!
        diff --git a/kuzu b/kuzu
        index ....

If you are trying to somehow make this last "git diff" silent, then I
think you are solving a _wrong_ problem.  By setting retroactively the
CRLF setting, you are saying that you do not want to have CRLF in the
blobs recorded in the repository, and it is a _good thing_ that there are
differences (tons of them) between what is recorded currently and what you
are going to commit to fix the earlier mistake.

>> I was hoping to see a solution where you will add a stronger version of
>> "refresh" without having to do anything else other than recording "how did
>> I munge the file in the working tree to produce the blob".  The third step
>> would change to:
>>
>> 3. "git update-index --refresh" notices that the conversion parameters
>>    are different since the last time the files in the working tree were
>>    looked at (i.e. immediately after a "clone", working tree files are
>>    what git wrote out using convert_to_working_tree() and you know what
>>    conversion you used; after the user modified files in the working tree
>>    and said "git add", you know you what conversion parameters you ran
>>    convert_to_git() with to produce blobs).  The paths that has different
>>    conversion parameters are re-indexed to see if they hash to the same
>>    sha1 as recorded in the index.  If they have changed, their index
>>    entries are left intact (i.e. you will still show the differences);
>>    otherwise you update the cached stat information for their index
>>    entries.
>
> I believe that most people that have edited a file that has changed
> CRLF convention aren't interested in that all lines have changed, but
> in only the lines that have actually been edited.

I think that is not just solving a wrong problem but also is encouraging a
bad workflow at the same time, which is even worse.  If you recorded CRLF
blobs in a project that you do not want to have CRLF blob, fixing that
mistake is one logical change that people who later browse the history
would not want to see intermixed with the "lines that actually have been
edited".

> Storing the normalized sha1 is needed to reduce the amount of double
> work (eg having "git update-index --refresh" reperform
> convert_to_git() for
> the repository blob every time a file is dirty, instead of (as now)
> just comparing the sha1 values).

As I already said, I agree that it would be beneficial to store what
normalization settings were used and comparing that with what settings are
in effect to detect the possible phamtom difference caused by the change
of the settings.  But once we know that the result of a re-normalization
is different from what is recorded in the index (or tree), then the
difference should be shown.  The actual difference would change every time
the work tree file is edited, so I don't see the benefit of contaminate
the object database with intermediate "blobs" that is not "added".

Re: [PATCH RFC 5/5] cache: Use ce_norm_sha1().

[Henrik Grubbström]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2588 bytes --]

On Tue, 20 Apr 2010, Junio C Hamano wrote:

> Henrik Grubbström <grubba@roxen.com> writes:
>
>>> 3. "git update-index --refresh" does not improve the situation, as it
>>>    (thinks) it knows the blob and the working tree file are different.
>>
>> False. "git update-index --refresh" uses
>> read-cache.c:ce_compare_data() to compare the content of the blob with
>> the normalized content of the working tree,...
>
> I don't think you tried it yourself.  Here is what should happen with the
> current code.
>
> 	# step 0 & 1. a project with LF ending
> 	$ git init two && cd two
>        $ echo a quick brown fox >kuzu
>        $ git add kuzu && git commit -m kuzu
>
>        # step 2. you want CRLF in your work area
>        $ echo -e "a quick brown fox\015" >kuzu
>        $ git config core.autocrlf true

Ok, right. I thought step 2 was

 	$ git config core.autocrlf true
 	$ rm kuzu
 	$ git checkout -- kuzu

In which case the index will be clean.

> And it is a common thing people wish to do.  Admittedly, this is an one-off
> event, so it is not _that_ urgent to fix.  You can for example do:

I've fixed this case in my current version of the patches.

>> Let's take the reverse case instead:
[...]

Seems to depend on whether is_racy_timestamp is true or not. As you've 
noted, adding a touch forces the issue.

[...]
> If you are trying to somehow make this last "git diff" silent, then I
> think you are solving a _wrong_ problem.  By setting retroactively the
> CRLF setting, you are saying that you do not want to have CRLF in the
> blobs recorded in the repository, and it is a _good thing_ that there are
> differences (tons of them) between what is recorded currently and what you
> are going to commit to fix the earlier mistake.

Ok, I've removed the diff-related changes from the current set of patches.

> As I already said, I agree that it would be beneficial to store what
> normalization settings were used and comparing that with what settings are
> in effect to detect the possible phamtom difference caused by the change
> of the settings.  But once we know that the result of a re-normalization
> is different from what is recorded in the index (or tree), then the
> difference should be shown.  The actual difference would change every time
> the work tree file is edited, so I don't see the benefit of contaminate
> the object database with intermediate "blobs" that is not "added".

Ok. With the diff patches gone, there's no need to store the normalized 
blob data.

--
Henrik Grubbström					grubba@grubba.org
Roxen Internet Software AB				grubba@roxen.com