RFC patch, 5 partsreachable: Made the gc aware of the ce_norm_sha1.

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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

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?

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

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

Ok.

Ok.

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 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.

Yes, the same should apply to other conversions.

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

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

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.

	# 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

	# 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!

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 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".

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".

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

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.

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

[...]

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

[...]

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

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

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

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

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.

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

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.

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

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.

Heya,

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

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.

Sverre Rabbelier <srabbelier@gmail.com> writes:

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;

Heya,

On Sun, Apr 18, 2010 at 00:32, Jakub Narebski <jnareb@gmail.com> wrote:

Agreed.

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 :).

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.