show/diff --check clarification needed

show/diff --check clarification needed

[Yann Dirson]

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

Re: show/diff --check clarification needed

[Junio C Hamano]

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

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

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.

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

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.

> - adjust defaults to agreed-upon values
> - add any config entries that would be meaningful

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

Thanks.

Re: show/diff --check clarification needed

[Yann Dirson]

On Thu, Jan 28, 2010 at 12:25:22PM -0800, Junio C Hamano wrote:
> "Yann Dirson" <ydirson@altern.org> writes:
> 
> > 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.
> 
> 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.

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)


> > - add a global flag to allow something like
> >   "git --attributes-tree=<treeish> <command>"
> 
> 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.

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.


> > - adjust defaults to agreed-upon values
> > - add any config entries that would be meaningful
> 
> Sorry, I have no idea what you mean by these two points.

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.