v4, 5 partsPatches to avoid reporting conversion changes.

This is the fourth go at having the git index keep track of the conversion mode for blobs.

This is useful for repositorys not containing fully normalized files (eg containing CRLF's or expanded $Id$ strings), where a later attribute change implies a conversion mode change. Without this set of patches the user would need to recommit semantically unchanged files to get a clean index.

Changes since last time:

  o The patch set has been rebased upon 0ed6711 (aka eb/core-eol),
    to be able to take advantage of convert.c:get_output_conversion()
    and convert.c:determine_action().

  o As a consequence of using those functions, the on-disk format for
    the CONV extension has changed slightly. The change should have
    minimal impact, since the index will in most cases self-repair.

  o The t0025-crlf-auto.sh tests have been updated to still test
    the same behaviour.

Junio: This should be close to what you envisioned in
       <7vsk6qio1f.fsf@alter.siamese.dyndns.org>.

Henrik Grubbström (Grubba) (5):
  sha1_file: Add index_blob().
  strbuf: Add strbuf_add_uint32().
  cache: Keep track of conversion mode changes.
  cache: Add index extension "CONV".
  t/t0021: Test that conversion changes are detected.

 cache.h               |   12 ++++++
 convert.c             |   46 ++++++++++++++++++++++
 read-cache.c          |  102 ++++++++++++++++++++++++++++++++++++++++++++----
 sha1_file.c           |   19 +++++++++
 strbuf.h              |    4 ++
 t/t0021-conversion.sh |   54 ++++++++++++++++++++++++++
 t/t0025-crlf-auto.sh  |   20 ++++++----
 7 files changed, 240 insertions(+), 17 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>
---
No changes since v1.

 cache.h     |    1 +
 sha1_file.c |   19 +++++++++++++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)

Adds a convenience function for adding an unsigned 32bit integer in network byte-order to a strbuf.

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

The index now keeps track of the conversion mode that was active when the entry was created. This can be used to detect the most common cases of when the conversion mode has changed.

Signed-off-by: Henrik Grubbström <grubba@grubba.org>
---
Rebased on 0ed6711 (aka eb/core-eol).

The conversion mode flags are now based on the EOL_* set of flags, and to avoid code duplication convert.c:get_output_conversion() and convert.c:determine_action() are used to determine the eol conversion mode.

The denormalized eol tests in t0025-crlf-auto.sh have been altered to still test the intended eol conversion properties.

 cache.h              |   11 +++++++++++
 convert.c            |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 read-cache.c         |   37 ++++++++++++++++++++++++++++++++++---
 t/t0025-crlf-auto.sh |   20 ++++++++++++--------
 4 files changed, 103 insertions(+), 11 deletions(-)

The index can now store and retrieve the ce_conv_flags data.

Signed-off-by: Henrik Grubbström <grubba@grubba.org>
---
The on disk format has changed slightly due to the changes in the
previous patch, but the change should have minimal impact, since
it typically only would force a one-time reindex.

 read-cache.c |   65 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 59 insertions(+), 6 deletions(-)

Signed-off-by Henrik Grubbström <grubba@grubba.org>
---
 t/t0021-conversion.sh |   54 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 54 insertions(+), 0 deletions(-)

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

A more fundamental (or perhaps "silly") question is if that "user would need to" is necessarily a bad thing. If the user wants to cleanse such abnormality in the recorded blobs, shouldn't there be a conscious act, iow, a commit that records that "I am fixing that mistake, and from now on, the recorded data are normalized"?

Perhaps I am missing something very trivial that you have already explained to the list but I forgot amid my moving and other confusion, and if that is the case I apologize in advance ;-).

On Tue, 1 Jun 2010, Junio C Hamano wrote:

I believe that users typically aren't interested in if data in the repository is on normalized form or not (witness the autocrlf=true discussion a few weeks ago, where one of the main complaints was that it required a renormalization (which fg/autocrlf attempts to solve for that specific case by not normalizing)), as long as they get the expected content on checkout.

This set of patches allows for an incremental, on-demand normalization. Eg the user could switch the attributes for a group of files from

   *.bat -crlf

(let's assume *.bat files use crlf linebreaks) to

   *.bat -crlf text eol=crlf

and then have git normalize the individual files when there's actually a semantic reason for a change. With the current eb/core-eol patches this change would cause a dirty index on checkout. The user who committed the change however has a clean index until any of the files affected is touched.

In my case, I have repositories containing files both requiring crlf and lf line endings, and additionally have expanded $Id$-strings that I want changed on first semantic change (but not before). To be able to use a git binary without this patchset I'd have to do a

   git commit -a -m 'Normalized'

as the first thing after a checkout.

I could of course add a config option to control the behaviour (hmm, or maybe an attribute?).

No problem.

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

Hi Henrik,

Henrik Grubbström wrote:

I agree. (In the case of autocrlf, it is also not very easy to renormalize. The usual recommendation I have seen is "git rm -r \ --cached . && git add .", which is not exactly simple.)

... but if I understand correctly, I don’t agree with this at all.

Imagine someone with an old copy of git that does not do normalization. If you convert everything at once, she sees a single enormous, semantically uninteresting cleanup patch (and she can check the result with ‘diff -w’ or sed if suspicious). If you wait for some real change to piggy-back onto, on the other hand, then the per-file normalization patches will make it hard to find what changed.

Of course, very few people use such old copies of git. The real problem is that git itself sees what this person would see; you are asking to slow down everyone who tries to use diff or blame on your repository by implicitly requiring the -w option.

The Right Thing would be to not set the relevant attributes until it is time for the file to be normalized. I can understand that that might be hard and could require tool support.

This is not an argument against your patches, since I haven’t read them (for all I know, they make everything better :)).

Regards, Jonathan

On Thu, 3 Jun 2010, Jonathan Nieder wrote:

Hi.

[...]

This seems more like an argument against repositories where renormalizations have occurred, than against the feature as such.

Well, diff and blame would be confused by a crlf renormalization regardless of whether the renormalization was piggy-backed or not. I haven't looked at the implementation of blame, but it was possible to reduce the confusion in the diff case by letting it normalize the old blob according to the current set of attributes (this was part of my original patch set, but Junio didn't like the feature).

True, but then the .gitattributes file would start to resemble a manifest file for the entire repository, which would be a pain to maintain.

I did do an experiment with a .gitattributes file like:

   *.c crlf ident
   [attr]foreign_ident -ident block_commit=Remove-foreign_ident-attribute-before-commit.
   # A list of files that haven't been changed since import follows.
   /foo.c foreign_ident
   /bar.c foreign_ident
   # etc

and a suitable pre-commit hook that looked at the block_commit attribute, but there were two problems in addition to the long list of files in the .gitattributes file:

   * The attributes file parsing was broken (recently fixed in the
     master branch), and the above actually caused foo.c and bar.c
     to have the ident attribute.

   * Hooks are not copied by git clone. Support for copying of hooks
     to non-POSIX-like systems is not something I'd like to attempt.

The latter problem could in this case be solved by adding support for the block_commit attribute to the core of git, but it doesn't seem like something most users would understand how to use, and I doubt that Junio would accept such a patch.

:-)

I don't believe that they make everything better, but that they're a step on the way.

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

Henrik Grubbström wrote:

No, it is an argument against making the process of renormalization more painful than it has to be (and against piggy-backing in general). It is kindest to have a flag day and yank the carriage returns off all at once like a bandage.

Only if they cross the revision where renormalization occurred.

This looks more sane.  Ident strings usually touch only a few lines.

Wouldn’t something like

 /foo.c -ident has_foreign_ident

work?  (Thanks for fixing that attributes macro processing bug, btw.)

Can’t you include a hooks/pre-commit file and a HACKING file: "copy this file to .git/hooks if you want your patches to be accepted"?

Thanks for your hard work, Jonathan

On Fri, 4 Jun 2010, Jonathan Nieder wrote:

In the crlf case, yes, probably. In the ident case there might be good reasons to want to have the ident strings stay unmodified as long as possible (since otherwise there'll be two ident strings that identify the same code).

Currently (as I believe you know), git has no detection of when the conversion mode for a file has changed, and it might even take a while before the users notice that the repository is not normalized. eg:

   0) There's a repository with some files containing crlf line endings.

   1) User A notices that git now has native support for crlf
      line endings, and adds the attribute eol=crlf for the
      affected files.

   2) User A does a git status, sees that .gitattributes is
      modified, and commits it.

   3) User A does a new git status, and has a clean index.

   4) User B who has been developing on the project for a while
      does a git pull from user A, and gets the new .gitattributes.

   5) User B does a git status, and has a clean index.

   6) User C is new to the project and does an initial git clone,
      and ends up with a dirty index.

I believe we both agree that the above is undesireable behaviour. The above behaviour also means that it's likely that there are repositorys out there which contain unnormalized files.

What my patch set achieves is that user C above also gets a clean index.

What it seems you want is that user A above should have all files that got denormalized by the attribute change marked dirty at 2 (and 3).

With a minor change of the read-cache.c:ce_compare_data() patch (returning 1 if conv_flags has CONV_NORM_NEEDED), you should get the behaviour you want (all files which are unnormalized in the repository will be dirty).

As I believe both behaviours may be desireable a config option and/or attribute is needed. Any suggestions for a name (and default value)?

Which is true in both cases.

Yes, but it sort of reduces the usefulness of macros if you need to expand them by hand... :-)

Better to fix the bug and wait for the fix to enter a released version.

Yes, but you still have to remember to do it everytime you clone the repository.

Thanks for taking the time to discuss the problem.

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

On Sun, Jun 06, 2010 at 12:50:08PM +0200, Henrik Grubbström wrote: [...]

I think it would be best if git at this time could decide that the affected files also become dirty. The ideal commit is one that both alters the .gitattributes _and_ the affected files at the same time, and git should make it easy to create that commit.

And the reason for this is mostly that unless you perform some special actions, you will commit attributes and contents that are mismatched.

In your suggested mode, whay would happen if you did this:

$ git clone ...... (which has files that are "wrong" wrt line endings and attributes for some .c files) $ touch *.c

Would it still believe all *.c files were clean? Does it require an actual other change at the same time to allow you to normalize the file? That would be detrimental I think. Changing newlines is best done as a separate commit, intermingling newline changes and real changes in the same commmit is not where you want to go.

However, for your ID string you obviously want this behaviour. I'm guessing that hook is alreasy set up so that if you just touch the file, it will still be treated as unmodified?

That would indeed be a very welcome change.

I think the default behaviour should be to mark files dirty if there are ANY attribute changes that could cause content changes done to them at all. I'm not sure that is exactly what your patch series is allowing us to track though?

Just to be clear:

If you add this to your .gitattributes

*.c eol=lf

I think it would be very helpful if git then would treat all .c files as "stat-dirty" the next time it updates its index.

A for config variables, what about:

core.rereadOnAttributeChanges = [true]/false    (default = true)

Which makes some sense for detecting it in 2, but not so much for ignoring it in 6.

- Finn Arne

Hi Finn,

On Mon, 7 Jun 2010, Finn Arne Gangstad wrote:

I agree in the case of newly added attributes. In the case of repositories already containing unnormalized files this however leads to problems. eg

   Consider the case above, but a while later when the repository has been
   fixed at HEAD. If an old version from before the normalization is
   checked out, the index will once again become dirty, which means that
   git will refuse the user to check out some other version unless the
   --force flag is given. Excessive use of --force is not a good thing.
   If the user is aware of the problem, and checking out old versions is
   a common operation, toggling the suggested option might be a good
   solution.

With the patch set as submitted, it would still claim that the files are clean. With the additional two-liner I suggested to Jonathan, the files would be dirty.

Yes.

What it does, is to track the attributes affecting conversion that were active when the entries in the index were last updated, and also whether the entry is on normalized form in the repository. This makes it possible to invalidate entries when attributes change.

Ok.

I was thinking more along the lines of

   core.allowUnnormalizedIndex = true/[false]	(default = false)

which unfortunately contains a negation.

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

On Mon, Jun 07, 2010 at 06:37:56PM +0200, Henrik Grubbström wrote:

Maybe I misunderstand something, but if you check out an older version, the .gitattributes file will change to match the old version. The old version should not have the conversion attributes set, and should therefore result in a clean checkout?

- Finn Arne

On Mon, 7 Jun 2010, Finn Arne Gangstad wrote:

True, there's no problem before the attribute change, but there is for commits between the attribute change and when the repository got normalized (which can be a while with the current git).

Re: configuration option naming:

   I've settled for core.normalizationPolicy, with the values
   'strict' (default) for the behaviour requested by you and Jonathan,
   and 'relaxed' for my initial behaviour.

Teaser:

   $ git init foo
   warning: templates not found /home/grubba/share/git-core/templates
   Initialized empty Git repository in /tmp/grubba/foo/.git/
   $ cd foo
   $ cat >expanded-keywords
   $Id: some id string $
   $ git add expanded-keywords
   $ git commit -m 'Initial commit.'
   [master (root-commit) 755d1f6] Initial commit.
    1 files changed, 1 insertions(+), 0 deletions(-)
    create mode 100644 expanded-keywords
   $ git status
   # On branch master
   nothing to commit (working directory clean)
   $ cat >.gitattributes
   * ident
   $ git status
   # On branch master
   # Changed but not updated:
   #   (use "git add <file>..." to update what will be committed)
   #   (use "git checkout -- <file>..." to discard changes in working directory)
   #
   #       modified:   expanded-keywords
   #
   # Untracked files:
   #   (use "git add <file>..." to include in what will be committed)
   #
   #       .gitattributes
   no changes added to commit (use "git add" and/or "git commit -a")
   $ git config core.normalizationPolicy relaxed
   $ git status
   # On branch master
   # Untracked files:
   #   (use "git add <file>..." to include in what will be committed)
   #
   #       .gitattributes
   nothing added to commit but untracked files present (use "git add" to track)
   $ git config core.normalizationPolicy strict
   $ git status
   # On branch master
   # Changed but not updated:
   #   (use "git add <file>..." to update what will be committed)
   #   (use "git checkout -- <file>..." to discard changes in working directory)
   #
   #       modified:   expanded-keywords
   #
   # Untracked files:
   #   (use "git add <file>..." to include in what will be committed)
   #
   #       .gitattributes
   no changes added to commit (use "git add" and/or "git commit -a")
   $ rm .gitattributes
   $ git status
   # On branch master
   nothing to commit (working directory clean)

Which I believe matches all the behaviours that have been requested.

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

On Tue, Jun 08, 2010 at 05:52:37PM +0200, Henrik Grubbström wrote:

As you say, the current git makes it easy to commit something where the attributes and the contents do not match. I think this needs to be fixed, and that your proposed patch in relaxed mode makes the problem _worse_, since it will then take even longer before these commits are fixed. But see below.

The name might be a bit vague, maybe there are other things that could be normalized? Maybe adding the word "index" is an improvement - e.g. core.indexNormalizationPolicy or just core.indexNormalization.

THIS behaviour is what I find scary. In this case, "ident" is clearly a newly added attribute, and git should not hide this from you. If you add a mode where git will hide this permanently, chances are the repositories will never be fixed.

The ident attribute may be a bit special since in your case it is only supposed to change if some other contents in the file change as well, but please also think how this will work with the text/eol attributes. Setting the text attribute and then having to CHANGE a file before getting it normalized is not good.

Still, I think your original problem description of cloning something and ending up with a dirty tree is indeed an annoying problem. So what about having the relaxed mode behave as follows:

If both of these are true:
 - the current attributes for a file are the same as it is registered as
   in the index with your new patch
 - a checkout of the file would result in identical contents to what is 
   currently in the working directory
Then behave as if the file is not modified.

Or, in other words: If attributes are unchanged, a file is unmodified not only if it would result in the same contents after being added, but also if it would result in the current working directory contents after being checked out again.

This should work for both text and ident on clone at least.

- Finn Arne

On Wed, 9 Jun 2010, Finn Arne Gangstad wrote:

Hmm... Maybe.

[...]

True.

Ok, so the expanded-keywords file in the example should show up as modified in relaxed mode as well, but be cleaned if the modified attributes file is added to the index? Or only after being committed?

I'll see what I can do...

Currently, I'm trying to understand the semantic difference between ce_modified() and ce_match_stat().

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

On Wed, Jun 09, 2010 at 08:04:34PM +0200, Henrik Grubbström wrote:

Not before being committed, since you would otherwise have to add all other files before adding .gitattributes, but I am not sure even that is sufficient reason to claim the files are unmodified (see case 3 below).

I think we agree on the following:

If there is a discrepancy between .gitattributes and the contents in the repository, the following should be true:

git checkout -f (or git reset --hard) git status -> ALWAYS report modified files in strict mode sleep 1 touch * git status -> NEVER report modified files in relaxed mode

The case I think you are asking about above is the following in "relaxed" mode:

echo "something that causes a discrepancy" >> .gitattributes git status -> MODIFIED (1) git add .gitattributes git status -> MODIFIED (2) git commit -m "bad commit" git status -> ??????? (3) <<-- Do you want this to be CLEAN? git reset --hard (or git checkout -f) sleep 1 touch * git status -> CLEAN (4)

1 and 4 should be uncontroversial and 2 I think is necessary because you should be able to git add in several steps. Whether 3 should be clean or modified I'm not so sure about, I think that it would make it more likely to get the repo normalized properly if it was still seen as modified there.

- Finn Arne