{"thread":{"id":"22408","subject":"show/diff --check clarification needed","startedAt":"2010-01-27T10:41:28Z","lastAt":"2010-01-28T21:38:37Z","messageCount":3,"participants":["Yann Dirson","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"132765","messageId":"d26982ce92a061ee24d04aba6e314fa4.squirrel@intranet.linagora.com","threadId":"22408","inReplyTo":null,"subject":"show/diff --check clarification needed","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2010-01-27T10:41:28Z","receivedAt":"2010-01-27T10:41:28Z","isPatch":false,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"I am experiencing an unexpected behaviour, related to whitespace\n(non-)checking.\n\nIn a my local repository I have a pre-commit hook checking for \"git\ndiff-index --check --cached\" (borrowed from the sample template), and\nthe server I push to does a \"git diff-tree --check\" on each pushed\ncommit to make sure someone who forgot the pre-commit hook does not\npush a whitespace-unclean commit by error.\n\nNow I had to import some 3rd-party stuff (an eclipse deltapack if you\nwonder) which includes CRLF line-endings, but must not get modified\nbecause of checksums. So I added the following .gitattributes to the\ntree where I import this stuff:\n\n$ cat deltapack/.gitattributes\n* binary\n.gitattributes -binary\n\nThat is just fine for the pre-commit hook, and \"git show --check\" on\nit in my local repository reports no problems.\n\nNow when I attempt to push, the update hook refuses my commit; and if\nI run the same \"git show --check\" on the server (in the bare repo\nwhere the ref has not been updated by the push, but where the pushed\ncommit has not been pruned yet), I get the complaints which I have\nintended the .gitattributes to silent.\n\nWhat happens is apparently that --check does not know which repository\nrevision to take .gitattributes from, and takes them from working\ntree. Indeed, removing deltapack/.gitattributes from my worktree and\nrerunning \"show --check\" on that unmodified commit does trigger the errors.\n\nWhile having \"diff-tree --check\" defaulting to looking at working tree\nby default may make sense, it would seem more natural for \"show\" to\nuse by default those of the commit we're looking at - and one could\nargue that \"diff-tree\" could use the positive treeish for that matter\nas well. But defaulting is one matter, it would be really useful to\nbe able to specify from where the attributes should be taken for any\ngiven operation. Did not look at the code - anyone knows how easy\nthat would be ?\n\n\nUnless I missed something, I suggest the following plan:\n\n- document in maint that --check only takes worktree into account when\n looking for .gitattributes, and more globally add a note to the\n gitattributes manpage to explicitely say that too.\n\n- add a global flag to allow something like\n \"git --attributes-tree= \"\n\n- adjust defaults to agreed-upon values\n\n- add any config entries that would be meaningful\n\n\nIt may just be me having badly written the update hook - here is the\nrelevant part for reference. Under the \"refs/heads/*,commit)\" case of\nthe sample update hook, I have something like:\n\n # checks to be done in all commits\n\n # exclude all commits previously-reachable by any ref\n exclude_oldrevs=$( git for-each-ref --format='^%(refname)' )\n\n for rev in $(git rev-list $exclude_oldrevs $newrev); do\n\t\t \t[...]\n # special case for root commits\n if git rev-parse HEAD^ >/dev/null 2>&1; then\n ref=\"\"\n else\n # use an empty tree to compare against\n ref=4b825dc642cb6eb9a060e54bf8d69288fbee4904\n fi\n # checks for whitespace\n if ! git diff-tree --check $ref $rev >/dev/null; then\n echo \"*** Whitespace problems in $rev\" >&2\n fails_whitespace=1\n fi\n\t\t\t[...]\n\t\tdone\n"},{"id":"132906","messageId":"7vmxzykmdp.fsf@alter.siamese.dyndns.org","threadId":"22408","inReplyTo":"d26982ce92a061ee24d04aba6e314fa4.squirrel@intranet.linagora.com","subject":"Re: show/diff --check clarification needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-28T20:25:22Z","receivedAt":"2010-01-28T20:25:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Yann Dirson\" writes:\n\n> Unless I missed something, I suggest the following plan:\n>\n> - document in maint that --check only takes worktree into account when\n> looking for .gitattributes, and more globally add a note to the\n> gitattributes manpage to explicitely say that too.\n\nIt is not limited to \"diff --check\". The current implementation reads\nattributes only from the checked out work tree and/or from the index,\ndepending on the direction of operation (e.g. checking out files to work\ntree reads from the index, I think). The same issue would affect \"git\narchive\" when generating a tarball from an older revision.\n\nThe exception is $GIT_DIR/info/gitattributes, and for your particular\npurpose, I think it is the right one to use, because the entries in that\nfile will apply regardless of which version you are examining patches\nfrom.\n\n> - add a global flag to allow something like\n> \"git --attributes-tree= \"\n\nI am not sure if this is what we really want. It seems to me that it\nwould make more sense to read from a relevant tree that the \nis operating on, if we are to enhance the attributes implementation. If\nthat is a fixed one, it is not much better than having necessary\nentries in your $GIT_DIR/info/gitattributes file.\n\n> - adjust defaults to agreed-upon values\n> - add any config entries that would be meaningful\n\nSorry, I have no idea what you mean by these two points.\n\nThanks.\n"},{"id":"132913","messageId":"20100128213837.GF2725@nan92-1-81-57-214-146.fbx.proxad.net","threadId":"22408","inReplyTo":"7vmxzykmdp.fsf@alter.siamese.dyndns.org","subject":"Re: show/diff --check clarification needed","fromName":"Yann Dirson","fromEmail":"ydirson@altern.org","sentAt":"2010-01-28T21:38:37Z","receivedAt":"2010-01-28T21:38:37Z","isPatch":false,"sender":{"key":"ydirson@altern.org","avatar":"https://avatars.githubusercontent.com/u/1190950?v=4"},"body":"On Thu, Jan 28, 2010 at 12:25:22PM -0800, Junio C Hamano wrote:\n> \"Yann Dirson\" writes:\n> \n> > Unless I missed something, I suggest the following plan:\n> >\n> > - document in maint that --check only takes worktree into account when\n> > looking for .gitattributes, and more globally add a note to the\n> > gitattributes manpage to explicitely say that too.\n> \n> It is not limited to \"diff --check\". The current implementation reads\n> attributes only from the checked out work tree and/or from the index,\n> depending on the direction of operation (e.g. checking out files to work\n> tree reads from the index, I think). The same issue would affect \"git\n> archive\" when generating a tarball from an older revision.\n> \n> The exception is $GIT_DIR/info/gitattributes, and for your particular\n> purpose, I think it is the right one to use, because the entries in that\n> file will apply regardless of which version you are examining patches\n> from.\n\nWell, it would have serious disavantages:\n- if used alone, info/attributes is not obtained as part of the fetch (so\n needs additional logic to deploy into dev's workspaces)\n- if used together with .gitattribute files, would need to be synchronized\n on pull, but then:\n - would also need a policy to tell from which head to update\n - mapping from a set of .gitattributes distributed in the tree is possibly\n not trivial (I have not succeeded to affect files in a specific subdir\n from the toplevel .gitattributes, and the manpage does not seem to mention\n that info/gitattributes allows more flexibility in this respect)\n\n\n> > - add a global flag to allow something like\n> > \"git --attributes-tree= \"\n> \n> I am not sure if this is what we really want. It seems to me that it\n> would make more sense to read from a relevant tree that the \n> is operating on, if we are to enhance the attributes implementation. If\n> that is a fixed one, it is not much better than having necessary\n> entries in your $GIT_DIR/info/gitattributes file.\n\nThe idea with this flag is not to have fixed, but more for use by\nother commands/porcelain, who would make themselves the decision of which\ntreeish to take attributes from.\n\n\n> > - adjust defaults to agreed-upon values\n> > - add any config entries that would be meaningful\n> \n> Sorry, I have no idea what you mean by these two points.\n\nThat was partly intentionally vague, partly to blame on me being tired :)\n\n- once the --attributes-tree feature would be in place, various commands like\ndiff-tree and show will be able to decide what makes more sense as a default\nfor the requested operation. Depending on the command and the particular\ninvocation, the default to use may not always be obvious, so I expect the\ngeneral default will be kept to the working tree.\n\n- as ususal, people may want to use different settings than the default for\nsome commands, so support for overriding them via the config mechanism should\nbe put in place.\n\n\nAnyway, how we specify the rule of which treeish to use for this will not be\npossible with simply a treeish. That will probably have to use keywords\nthat would be meaningful in the context of the operation.\n\nI would expect that all operations would be able to be told \"worktree\", \"index\",\nand to be given any treeish - this in itself is a problem, I don't think we\nalready have a \"treeish\" syntax for worktree and index.\n\nAdditionally, \"show\" applied to a treeish would allow to tell to use the treeish\nit is showing (or one of its parents); similarly when applied to \":\"\nsyntax; \"diff-tree\" would allow either the positive or negative reference; etc.\nAll that would also require a special syntax, so it can be specified as a\ngenerally-useful value in the config file.\n"}]}