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

From: Henrik Grubbström <grubba@roxen.com>
Date: Jun 4, 2010, 11:59 UTC
Message-ID: <Pine.GSO.4.63.1006041212200.27465@shipon.roxen.com>
In-Reply-To: <20100604005603.GA25806@progeny.tock>

On Thu, 3 Jun 2010, Jonathan Nieder wrote:

> Hi Henrik,

Hi.

Show 14 quoted lines

> Henrik Grubbström 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.
>
> 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.)
>
>> This set of patches allows for an incremental, on-demand normalization.

[...]

Show 8 quoted lines

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

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

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

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

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

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.

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

:-)

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