show/diff --check clarification needed

I am experiencing an unexpected behaviour, related to whitespace (non-)checking.

In a my local repository I have a pre-commit hook checking for "git diff-index --check --cached" (borrowed from the sample template), and the server I push to does a "git diff-tree --check" on each pushed commit to make sure someone who forgot the pre-commit hook does not push a whitespace-unclean commit by error.

Now I had to import some 3rd-party stuff (an eclipse deltapack if you wonder) which includes CRLF line-endings, but must not get modified because of checksums. So I added the following .gitattributes to the tree where I import this stuff:

$ cat deltapack/.gitattributes
* binary
.gitattributes -binary

That is just fine for the pre-commit hook, and "git show --check" on it in my local repository reports no problems.

Now when I attempt to push, the update hook refuses my commit; and if I run the same "git show --check" on the server (in the bare repo where the ref has not been updated by the push, but where the pushed commit has not been pruned yet), I get the complaints which I have intended the .gitattributes to silent.

What happens is apparently that --check does not know which repository revision to take .gitattributes from, and takes them from working tree. Indeed, removing deltapack/.gitattributes from my worktree and rerunning "show --check" on that unmodified commit does trigger the errors.

While having "diff-tree --check" defaulting to looking at working tree by default may make sense, it would seem more natural for "show" to use by default those of the commit we're looking at - and one could argue that "diff-tree" could use the positive treeish for that matter as well. But defaulting is one matter, it would be really useful to be able to specify from where the attributes should be taken for any given operation. Did not look at the code - anyone knows how easy that would be ?

Unless I missed something, I suggest the following plan:

- document in maint that --check only takes worktree into account when
  looking for .gitattributes, and more globally add a note to the
  gitattributes manpage to explicitely say that too.

- add a global flag to allow something like
  "git --attributes-tree=<treeish> <command>"

- adjust defaults to agreed-upon values

- add any config entries that would be meaningful

It may just be me having badly written the update hook - here is the relevant part for reference. Under the "refs/heads/*,commit)" case of the sample update hook, I have something like:

                # checks to be done in all commits

                # exclude all commits previously-reachable by any ref
                exclude_oldrevs=$( git for-each-ref --format='^%(refname)' )

                for rev in $(git rev-list $exclude_oldrevs $newrev); do
		    	[...]
                        # special case for root commits
                        if git rev-parse HEAD^ >/dev/null 2>&1; then
                                ref=""
                        else
                                # use an empty tree to compare against
                                ref=4b825dc642cb6eb9a060e54bf8d69288fbee4904
                        fi
                        # checks for whitespace
                        if ! git diff-tree --check $ref $rev >/dev/null; then
                                echo "*** Whitespace problems in $rev" >&2
                                fails_whitespace=1
                        fi
			[...]
		done

"Yann Dirson" <ydirson@altern.org> writes:

It is not limited to "diff --check". The current implementation reads attributes only from the checked out work tree and/or from the index, depending on the direction of operation (e.g. checking out files to work tree reads from the index, I think). The same issue would affect "git archive" when generating a tarball from an older revision.

The exception is $GIT_DIR/info/gitattributes, and for your particular purpose, I think it is the right one to use, because the entries in that file will apply regardless of which version you are examining patches from.

I am not sure if this is what we really want. It seems to me that it would make more sense to read from a relevant tree that the <command> is operating on, if we are to enhance the attributes implementation. If that <treeish> is a fixed one, it is not much better than having necessary entries in your $GIT_DIR/info/gitattributes file.

Sorry, I have no idea what you mean by these two points.

Thanks.

On Thu, Jan 28, 2010 at 12:25:22PM -0800, Junio C Hamano wrote:

Well, it would have serious disavantages:
- if used alone, info/attributes is not obtained as part of the fetch (so
  needs additional logic to deploy into dev's workspaces)
- if used together with .gitattribute files, would need to be synchronized
  on pull, but then:
  - would also need a policy to tell from which head to update
  - mapping from a set of .gitattributes distributed in the tree is possibly
    not trivial (I have not succeeded to affect files in a specific subdir
    from the toplevel .gitattributes, and the manpage does not seem to mention
    that info/gitattributes allows more flexibility in this respect)

The idea with this flag is not to have <treeish> fixed, but more for use by other commands/porcelain, who would make themselves the decision of which treeish to take attributes from.

That was partly intentionally vague, partly to blame on me being tired :)

- once the --attributes-tree feature would be in place, various commands like
diff-tree and show will be able to decide what makes more sense as a default
for the requested operation.  Depending on the command and the particular
invocation, the default to use may not always be obvious, so I expect the
general default will be kept to the working tree.

- as ususal, people may want to use different settings than the default for
some commands, so support for overriding them via the config mechanism should
be put in place.

Anyway, how we specify the rule of which treeish to use for this will not be possible with simply a treeish. That will probably have to use keywords that would be meaningful in the context of the operation.

I would expect that all operations would be able to be told "worktree", "index", and to be given any treeish - this in itself is a problem, I don't think we already have a "treeish" syntax for worktree and index.

Additionally, "show" applied to a treeish would allow to tell to use the treeish it is showing (or one of its parents); similarly when applied to "<committish>:<path>" syntax; "diff-tree" would allow either the positive or negative reference; etc. All that would also require a special syntax, so it can be specified as a generally-useful value in the config file.