{"thread":{"id":"33475","subject":"git log -p unexpected behaviour - security risk?","startedAt":"2013-04-11T10:36:26Z","lastAt":"2013-05-01T07:23:32Z","messageCount":22,"participants":["John Tapsell","Tay Ray Chuan","Simon Ruderich","Junio C Hamano","Jonathan Nieder","Thomas Rast","John Szakmeister","shawn wilson","Matthieu Moy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"213921","messageId":"CAHQ6N+qdA5Lck1_ByOYPOG4ngsztz3HQSw8c_U_K8OnDapj4bQ@mail.gmail.com","threadId":"33475","inReplyTo":null,"subject":"git log -p unexpected behaviour - security risk?","fromName":"John Tapsell","fromEmail":"johnflux@gmail.com","sentAt":"2013-04-11T10:36:26Z","receivedAt":"2013-04-11T10:36:26Z","isPatch":false,"sender":{"key":"johnflux@gmail.com","avatar":"https://gravatar.com/avatar/25f70d4c0f96396b84a2e34bcd9bdc233462c7b4be29b5fdca8266fc53f30b0c?d=mp&s=160"},"body":"Hi,\n\n I noticed that code that you put in merge will not be visible by\ndefault. This seems like a pretty horrible security problem, no?\n\nI made the following test tree, with just 3 commits:\n\nhttps://github.com/johnflux/ExampleEvilness.git\n\nDoing \"git log -p\" shows all very innocent commits. Completely\nhidden is the change to add \"EVIL CODE MUWHAHAHA\".\n\nThis seems really dangerous!\n\nThe evil code only shows up with the non-default --cc or -m option.\n\nIs there a way to make --cc default?\n\nJohn\n"},{"id":"213960","messageId":"CALUzUxrp4+S-Nm-Scb9sT9sBw1mLEb3-CBc_P0KqL20qNmFO3w@mail.gmail.com","threadId":"33475","inReplyTo":"CAHQ6N+qdA5Lck1_ByOYPOG4ngsztz3HQSw8c_U_K8OnDapj4bQ@mail.gmail.com","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2013-04-11T15:19:32Z","receivedAt":"2013-04-11T15:19:32Z","isPatch":false,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Thu, Apr 11, 2013 at 6:36 PM, John Tapsell wrote:\n> I noticed that code that you put in merge will not be visible by\n> default. This seems like a pretty horrible security problem, no?\n>\n> I made the following test tree, with just 3 commits:\n>\n> https://github.com/johnflux/ExampleEvilness.git\n>\n> Doing \"git log -p\" shows all very innocent commits. Completely\n> hidden is the change to add \"EVIL CODE MUWHAHAHA\".\n>\n> This seems really dangerous!\n>\n> The evil code only shows up with the non-default --cc or -m option.\n\nFor email-based patch workflows (eg. git, linux kernel), then this is\nnot a problem - the diff doesn't even show up, so nothing is applied\nwhen git-am is run.\n\nFor github with pull-requests, a diff is shown between trees, so this\nwill show up.\n\n--\nCheers,\nRay Chuan\n"},{"id":"214920","messageId":"20130420140051.GB29454@ruderich.org","threadId":"33475","inReplyTo":"CAHQ6N+qdA5Lck1_ByOYPOG4ngsztz3HQSw8c_U_K8OnDapj4bQ@mail.gmail.com","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2013-04-20T14:00:52Z","receivedAt":"2013-04-20T14:00:52Z","isPatch":false,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Thu, Apr 11, 2013 at 11:36:26AM +0100, John Tapsell wrote:\n> Is there a way to make --cc default?\n\nIf you use aliases, something like this is easy:\n\n git config --global --add alias.lp 'log --patch --cc'\n\nI use aliases heavily, so that's my fix for now.\n\n\nBut I think the current behaviour is unexpected for most (new?)\nusers (including me). I thought -p would display all changes in\nall commits, including merges.\n\nI guess changing -p's default behaviour to imply --cc is\nproblematic, so I think we should document that -p doesn't\ngenerate patches for merges. Maybe something like this:\n\n-- 8< --\nSubject: [PATCH] Documentation/diff-options.txt: -p doesn't display merge changes\n\nSigned-off-by: Simon Ruderich \n---\n Documentation/diff-options.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 104579d..cd35ec7 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -24,6 +24,10 @@ ifndef::git-format-patch[]\n --patch::\n \tGenerate patch (see section on generating patches).\n \t{git-diff? This is the default.}\n+ifdef::git-log[]\n+\tChanges introduced in merge commits are not displayed. Use `-c`,\n+\t`--cc` or `-m` to include them.\n+endif::git-log[]\n endif::git-format-patch[]\n \n -U::\n-- \n1.8.2.1.513.gdedbb69.dirty\n\n-- 8< --\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"214970","messageId":"7vd2towdiq.fsf@alter.siamese.dyndns.org","threadId":"33475","inReplyTo":"20130420140051.GB29454@ruderich.org","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-21T07:26:05Z","receivedAt":"2013-04-21T07:26:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Simon Ruderich writes:\n\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 104579d..cd35ec7 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -24,6 +24,10 @@ ifndef::git-format-patch[]\n> --patch::\n> \tGenerate patch (see section on generating patches).\n> \t{git-diff? This is the default.}\n> +ifdef::git-log[]\n> +\tChanges introduced in merge commits are not displayed. Use `-c`,\n> +\t`--cc` or `-m` to include them.\n> +endif::git-log[]\n\nIt probably is a better change to drop \"Use `-c`...\" and refer to\nthe \"Diff formatting\" section.\n\nAnd then add '-p' and the fact that by default it will not show\npairwise diff for merge commits to the \"Diff Formatting\" section.\nThat is where -c/--cc/-m are already described.\n"},{"id":"215001","messageId":"CAHQ6N+pKb-44rOM7ocYMvSDyimvAGZppX1Gc=st59aVKzJSBKw@mail.gmail.com","threadId":"33475","inReplyTo":"7vd2towdiq.fsf@alter.siamese.dyndns.org","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"John Tapsell","fromEmail":"johnflux@gmail.com","sentAt":"2013-04-21T08:56:49Z","receivedAt":"2013-04-21T08:56:49Z","isPatch":false,"sender":{"key":"johnflux@gmail.com","avatar":"https://gravatar.com/avatar/25f70d4c0f96396b84a2e34bcd9bdc233462c7b4be29b5fdca8266fc53f30b0c?d=mp&s=160"},"body":"On 21 April 2013 08:26, Junio C Hamano wrote:\n> Simon Ruderich writes:\n>\n>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n>> index 104579d..cd35ec7 100644\n>> --- a/Documentation/diff-options.txt\n>> +++ b/Documentation/diff-options.txt\n>> @@ -24,6 +24,10 @@ ifndef::git-format-patch[]\n>> --patch::\n>> Generate patch (see section on generating patches).\n>> {git-diff? This is the default.}\n>> +ifdef::git-log[]\n>> + Changes introduced in merge commits are not displayed. Use `-c`,\n>> + `--cc` or `-m` to include them.\n>> +endif::git-log[]\n>\n> It probably is a better change to drop \"Use `-c`...\" and refer to\n> the \"Diff formatting\" section.\n>\n> And then add '-p' and the fact that by default it will not show\n> pairwise diff for merge commits to the \"Diff Formatting\" section.\n> That is where -c/--cc/-m are already described.\n\nWhy not have it in both places? This is really important.\n\nI'm concerned that noone is taking this security risk seriously. Just\nbecause it doesn't show up in certain workflows doesn't make the risk\ngo away.\n\nWhat about all the people who use git internally? They aren't using\ngithub and almost certainly aren't using a mail based system.\n\nIt's bad that we can't even set the right behaviour as a default.\n\nJohn\n"},{"id":"215006","messageId":"20130421102150.GJ10429@elie.Belkin","threadId":"33475","inReplyTo":"CAHQ6N+pKb-44rOM7ocYMvSDyimvAGZppX1Gc=st59aVKzJSBKw@mail.gmail.com","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-04-21T10:21:50Z","receivedAt":"2013-04-21T10:21:50Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"John Tapsell wrote:\n\n> I'm concerned that noone is taking this security risk seriously.\n\nIf anyone relies on \"git log -p\" or \"git log -p --cc\" output to make\nsure that the untrusted code they use doesn't introduce unwanted\nbehavior, they are making a serious mistake. A merge can completely\nundo important changes made in a side branch and \"-c\" and \"--cc\" will\nnot show it. The lack of \"-c\" cannot be a security issue here,\nbecause in normal life adding \"-c\" isn't a secure deployment strategy.\n\nThat's why if you want to review the code you are pulling in as a\nwhole, it is worthwhile to do\n\n\tgit diff HEAD...FETCH_HEAD\n\nThat is how you ask \"What code changes does FETCH_HEAD introduce?\"\nbefore putting your stamp of approval on them by merging and pushing\nout the result. Unfortunately that doesn't protect you from\nmaliciously written commits that will be encountered when bisecting.\nAt some point you have to be able to trust people.\n\nHope that helps,\nJonathan\n"},{"id":"215018","messageId":"CAHQ6N+rXE42NOyQPfLiDN8jYfL8w06hEE5MFLeFNxMR4ORD0aw@mail.gmail.com","threadId":"33475","inReplyTo":"20130421102150.GJ10429@elie.Belkin","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"John Tapsell","fromEmail":"johnflux@gmail.com","sentAt":"2013-04-21T13:46:34Z","receivedAt":"2013-04-21T13:46:34Z","isPatch":false,"sender":{"key":"johnflux@gmail.com","avatar":"https://gravatar.com/avatar/25f70d4c0f96396b84a2e34bcd9bdc233462c7b4be29b5fdca8266fc53f30b0c?d=mp&s=160"},"body":"On 21 April 2013 11:21, Jonathan Nieder wrote:\n> John Tapsell wrote:\n>\n>> I'm concerned that noone is taking this security risk seriously.\n>\n> If anyone relies on \"git log -p\" or \"git log -p --cc\" output to make\n> sure that the untrusted code they use doesn't introduce unwanted\n> behavior, they are making a serious mistake.\n\nWhich is exactly my problem.\n\nGo and ask the average person using git this very question, and I bet\nyou the vast majority will not know about -cc etc.\n\nYou can't just push all the blame on the user for bad defaults.\nHiding code changes is a bad default.\n\n> A merge can completely\n> undo important changes made in a side branch and \"-c\" and \"--cc\" will\n> not show it.\n\nWait, what? This is getting even worse then! Can you expand on this please?\n\nAnd then how do I show all of these important changes with a git log -p ?\nOr is it impossible to get a sane output?\n\n> The lack of \"-c\" cannot be a security issue here,\n> because in normal life adding \"-c\" isn't a secure deployment strategy.\n\nSo, is it impossible to make git log -p a \"secure deployment strategy\" ?\n\n> That's why if you want to review the code you are pulling in as a\n> whole, it is worthwhile to do\n>\n> git diff HEAD...FETCH_HEAD\n\nWhich basically means that you're asking the review the same code\ntwice. Once that way, and once using git log -p (to check for the\nexact reason that you said).\n\n> Unfortunately that doesn't protect you from\n> maliciously written commits that will be encountered when bisecting.\n> At some point you have to be able to trust people.\n\nSeriously? Your reasoning for awful defaults is that you should just\ntrust people?\n\nThis is getting worse and worse!\n\nJohn\n"},{"id":"215020","messageId":"8738ujubbs.fsf@hexa.v.cablecom.net","threadId":"33475","inReplyTo":"CAHQ6N+rXE42NOyQPfLiDN8jYfL8w06hEE5MFLeFNxMR4ORD0aw@mail.gmail.com","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-04-21T15:56:23Z","receivedAt":"2013-04-21T15:56:23Z","isPatch":false,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"John Tapsell writes:\n\n> On 21 April 2013 11:21, Jonathan Nieder wrote:\n>\n>> A merge can completely\n>> undo important changes made in a side branch and \"-c\" and \"--cc\" will\n>> not show it.\n>\n> Wait, what? This is getting even worse then! Can you expand on this please?\n>\n> And then how do I show all of these important changes with a git log -p ?\n> Or is it impossible to get a sane output?\n\nIt pretty much by definition does not show changes if the merge picks\none side unchanged:\n\n -c\n [...] lists only files which were modified from all parents.\n\n --cc\n This flag implies the -c option and further compresses the patch\n output [...]\n\nOn top of that, the default history simplification when you specify a\npathspec will only walk the (or any one) unchanged side of such a merge,\nso if you filter for a file you wouldn't even find the offending commit\nfurther back in history.\n\nI don't think this can be improved easily with the current one-pass[1]\nhistory/diff generation. To know what the merge *should* have done,\nyou'd need to somehow get an idea what parts of the resulting files\nshould be affected, which AFAICS boils down to redoing the merge. And\nto do that, you need to scan history so far that you can compute the\nmerge-bases. Not to mention that redoing all merges while walking\nhistory is somewhat expensive.\n\nYou could hack up a script that does the verification manually, by\nactually running a merge and comparing the result with what the merge\ngave you. But it's not something that you would want to run by default.\n\n\n[1] some things like --simplify-merges are actually another pass, but\nthe default is to generate everything -- including diffs -- as we go.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"215021","messageId":"20130421160939.GA29341@elie.Belkin","threadId":"33475","inReplyTo":"CAHQ6N+rXE42NOyQPfLiDN8jYfL8w06hEE5MFLeFNxMR4ORD0aw@mail.gmail.com","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-04-21T16:09:39Z","receivedAt":"2013-04-21T16:09:39Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"John Tapsell wrote:\n> Jonathan Nieder wrote:\n\n>> If anyone relies on \"git log -p\" or \"git log -p --cc\" output to make\n>> sure that the untrusted code they use doesn't introduce unwanted\n>> behavior, they are making a serious mistake.\n[...]\n> You can't just push all the blame on the user for bad defaults.\n\nThe thing is, I'm not convinced this is a bad default. \"Shows no diff\nat all for merges\" is easy for a person to understand. It is much\neasier to understand its limitations than -c and --cc. For that\nreason, it is a much *better* default for security than --cc or -c\n(even though I believe one of the latter would be a better default for\nconvenience).\n\nI agree that this is an important documentation bug, since\nintroductory documentation does not explain clearly enough how\n\"git log -p\" will act for merges.\n\n>> A merge can completely\n>> undo important changes made in a side branch and \"-c\" and \"--cc\" will\n>> not show it.\n>\n> Wait, what? This is getting even worse then! Can you expand on this please?\n\nIf a given file matches one of its parents, there is nothing to show\nin the combined diff format. Otherwise every merge would have a very\nlong diff.\n\nIf what you really want is the diff against the first parent, you\ncan use -m --first-parent with -p. If you want the diffs against each\nparent, you can use -m -p.\n\n[...]\n>> Unfortunately that doesn't protect you from\n>> maliciously written commits that will be encountered when bisecting.\n>> At some point you have to be able to trust people.\n>\n> Seriously? Your reasoning for awful defaults is that you should just\n> trust people?\n\nI didn't set the defaults. I'm explaining how the tool currently\nbehaves in response to your question. A person can do many\nunfortunate things if you blindly trust them and merge from them.\n\nFor example, whenever git adds (or plans) support for a new header\nline in commit objects, before you've upgraded, a prankster can\nprovide a bad value for that header line in objects they hand-craft.\n\"git fsck\" in your older version of git will accept the resulting\nobjects on the assumption that they came from a newer version of git,\nso you won't notice. Later you upgrade Git and \"git fsck\" considers\nthe objects malformed. Clients with \"[transfer] fsckobjects\" enabled\nstart to reject your history. That is, this person has made your\nrepository corrupt in the eyes of \"git fsck\".\n\nThe usual excellent integrity checking will let you pinpoint the\nproblem to the merge from that untrusted person so you can avoid\ntrusting them again, and all the data will be there to recover without\nthem. So it is auditable later. But this does mean that with the\ncurrent design, there is some level of trust required to let someone\ncommit into your history unless you inspect their work with a\nfine-toothed comb.\n\nAll that said, if someone has ideas for improving git's support for\nsuch inspection, that would be great. \"-c\" just isn't it. \"-c\" can\nbe a good tool for finding honest mistakes, but it doesn't protect\nwell against an adversary.\n\nIn the meantime, if you didn't intend to trust those people this much,\nthis might mean your procedures (and git's documentation, for the sake\nof others in the same boat) need some changes. Sorry to be the bearer\nof bad news.\n\nHope that helps,\nJonathan\n"},{"id":"215024","messageId":"7vzjwru4ev.fsf@alter.siamese.dyndns.org","threadId":"33475","inReplyTo":"20130421102150.GJ10429@elie.Belkin","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-21T18:25:44Z","receivedAt":"2013-04-21T18:25:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> That's why if you want to review the code you are pulling in as a\n> whole, it is worthwhile to do\n>\n> \tgit diff HEAD...FETCH_HEAD\n>\n> That is how you ask \"What code changes does FETCH_HEAD introduce?\"\n> before putting your stamp of approval on them by merging and pushing\n> out the result.\n\nAnd the only way to retroactively review that a merge C did not do\nanything funly is to check \"git diff C^1 C\", assuming that you\nalready trust C^1, the state before you performed the merge.\nIncidentally, this works for non-merge commits just as well.\n\n\"git log -m -p\" is not the default because most of the time people\nare not interested in seeing what it shows over \"--cc\" or \"-c\". It\nis a repetition of what you would get from individual patches on the\nside branch merged that you will later see in the traversal of that\ncommand. \"--cc/-c\" gives a representation for tricky merge cases\nwhere people could likely have made a mistake, or had correctly\nresolved semantic conflicts (e.g. one side renames a function, the\nother side adds a callsite, the merge result renames the function\nnew caller calls).\n\nFor the purpose of a \"merge audit\" John seems to want, the only way\nis to wade through \"log -m -p\" output. \n"},{"id":"215025","messageId":"7vli8bu3ne.fsf@alter.siamese.dyndns.org","threadId":"33475","inReplyTo":"20130421160939.GA29341@elie.Belkin","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-21T18:42:13Z","receivedAt":"2013-04-21T18:42:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> The thing is, I'm not convinced this is a bad default. \"Shows no diff\n> at all for merges\" is easy for a person to understand. It is much\n> easier to understand its limitations than -c and --cc.\n\nMaking \"log -p -m\" a default before -c/--cc was introduced would\nhave been the stupidest thing to do, as it would make the command\nmostly useless. Nobody would want to see repetitious output from a\nmerge that he would eventually get when the traversal drills down to\nindividual commits on the merged side branch.\n\nWhen I added -c/--cc, I contemplated making -p imply --cc, but\ndecided against it primarily because it is a change in traditional\nbehaviour, and it is easy for users to say --cc instead of -p from\nthe command line.\n\nOn the other hand, \"show\" was a newer command and it was easy to\nturn its default to --cc without having to worry too much about\nexisting users.\n\n> For that\n> reason, it is a much *better* default for security than --cc or -c\n> (even though I believe one of the latter would be a better default for\n> convenience).\n\nYes. I do not fundamentally oppose to the idea of \"log -p\" to imply\n\"log --cc\" when \"-m\" is not given (\"log -p -m\" is specifically\ndeclining the combined diff simplification). It may be a usability\nimprovement.\n\nBut \"--cc/-c\" does not have anything to do with Tapsell's \"security\nworries\". The only real audit he can do is with \"log -m -p\",\npossibly with --first-parent (only if he trusts his first-parent\nhistory).\n\nThe \"recreate mechanical merge and compare recorded merge against\nit\" mode may highlight a malicious merger, but it will not show a\ncleanly merged hunk of malicious code in the merge, so it cannot be\nused with --first-parent when used as a \"security audit tool\".\nTapsell still needs to drill down to the merged side branch that\nintroduced the malicious change that merged cleanly with \"-p\".\n"},{"id":"215976","messageId":"CAEBDL5VspccUmkkYBf17soGTyT3sinjnnNzRB_kytnOr3OBVQw@mail.gmail.com","threadId":"33475","inReplyTo":"7vli8bu3ne.fsf@alter.siamese.dyndns.org","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2013-04-30T10:09:17Z","receivedAt":"2013-04-30T10:09:17Z","isPatch":false,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Sun, Apr 21, 2013 at 2:42 PM, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n>\n>> The thing is, I'm not convinced this is a bad default. \"Shows no diff\n>> at all for merges\" is easy for a person to understand. It is much\n>> easier to understand its limitations than -c and --cc.\n>\n> Making \"log -p -m\" a default before -c/--cc was introduced would\n> have been the stupidest thing to do, as it would make the command\n> mostly useless. Nobody would want to see repetitious output from a\n> merge that he would eventually get when the traversal drills down to\n> individual commits on the merged side branch.\n>\n> When I added -c/--cc, I contemplated making -p imply --cc, but\n> decided against it primarily because it is a change in traditional\n> behaviour, and it is easy for users to say --cc instead of -p from\n> the command line.\n\nFWIW, security aside, I would've like to have seen that. I find it\nconfusing that merge commits that introduce code don't have a diff\nshown when using -p. And I find it hard to remember --cc. BTW,\nwhat's the mnemonic for it? -p => patch, --cc => ?\n\n> On the other hand, \"show\" was a newer command and it was easy to\n> turn its default to --cc without having to worry too much about\n> existing users.\n>\n>> For that\n>> reason, it is a much *better* default for security than --cc or -c\n>> (even though I believe one of the latter would be a better default for\n>> convenience).\n>\n> Yes. I do not fundamentally oppose to the idea of \"log -p\" to imply\n> \"log --cc\" when \"-m\" is not given (\"log -p -m\" is specifically\n> declining the combined diff simplification). It may be a usability\n> improvement.\n\nWould you consider such a patch?\n\n-John\n"},{"id":"215983","messageId":"CAH_OBidM2D4Nkb9tHTWqPRVz0GVUEnn9NJ++rzWGYSd+7sgTMg@mail.gmail.com","threadId":"33475","inReplyTo":"20130421160939.GA29341@elie.Belkin","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"shawn wilson","fromEmail":"ag4ve.us@gmail.com","sentAt":"2013-04-30T11:48:58Z","receivedAt":"2013-04-30T11:48:58Z","isPatch":false,"sender":{"key":"ag4ve.us@gmail.com","avatar":null},"body":"Sorta OT, but I'm curious,\n\nOn Sun, Apr 21, 2013 at 12:09 PM, Jonathan Nieder wrote:\n\n> For example, whenever git adds (or plans) support for a new header\n> line in commit objects, before you've upgraded, a prankster can\n> provide a bad value for that header line in objects they hand-craft.\n> \"git fsck\" in your older version of git will accept the resulting\n> objects on the assumption that they came from a newer version of git,\n> so you won't notice. Later you upgrade Git and \"git fsck\" considers\n> the objects malformed. Clients with \"[transfer] fsckobjects\" enabled\n> start to reject your history. That is, this person has made your\n> repository corrupt in the eyes of \"git fsck\".\n>\n> The usual excellent integrity checking will let you pinpoint the\n> problem to the merge from that untrusted person so you can avoid\n> trusting them again, and all the data will be there to recover without\n> them. So it is auditable later. But this does mean that with the\n> current design, there is some level of trust required to let someone\n> commit into your history unless you inspect their work with a\n> fine-toothed comb.\n>\n\nHas anyone written a test case for this?\n"},{"id":"216000","messageId":"7va9ogezzx.fsf@alter.siamese.dyndns.org","threadId":"33475","inReplyTo":"CAEBDL5VspccUmkkYBf17soGTyT3sinjnnNzRB_kytnOr3OBVQw@mail.gmail.com","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-30T16:37:22Z","receivedAt":"2013-04-30T16:37:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Szakmeister writes:\n\n>> When I added -c/--cc, I contemplated making -p imply --cc, but\n>> decided against it primarily because it is a change in traditional\n>> behaviour, and it is easy for users to say --cc instead of -p from\n>> the command line.\n>\n> FWIW, security aside, I would've like to have seen that. I find it\n> confusing that merge commits that introduce code don't have a diff\n> shown when using -p. And I find it hard to remember --cc. BTW,\n> what's the mnemonic for it? -p => patch, --cc => ?\n\nCompact combined.\n\nBy the way, these options are _not_ about \"showing merge commits\nthat introduce code\", and they do not help your kind of \"security\".\nAs I repeatedly said, you would need \"-p -m\" for that.\n"},{"id":"216002","messageId":"CAEBDL5W8YWu8_TV7o0s3ZZomETz8RPWnr8oOmy0xQ=U8o0xe0Q@mail.gmail.com","threadId":"33475","inReplyTo":"7va9ogezzx.fsf@alter.siamese.dyndns.org","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2013-04-30T16:47:36Z","receivedAt":"2013-04-30T16:47:36Z","isPatch":false,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Tue, Apr 30, 2013 at 12:37 PM, Junio C Hamano wrote:\n> John Szakmeister writes:\n>\n>>> When I added -c/--cc, I contemplated making -p imply --cc, but\n>>> decided against it primarily because it is a change in traditional\n>>> behaviour, and it is easy for users to say --cc instead of -p from\n>>> the command line.\n>>\n>> FWIW, security aside, I would've like to have seen that. I find it\n>> confusing that merge commits that introduce code don't have a diff\n>> shown when using -p. And I find it hard to remember --cc. BTW,\n>> what's the mnemonic for it? -p => patch, --cc => ?\n>\n> Compact combined.\n\nThank you.\n\n> By the way, these options are _not_ about \"showing merge commits\n> that introduce code\", and they do not help your kind of \"security\".\n> As I repeatedly said, you would need \"-p -m\" for that.\n\nI'm sorry, I didn't mean to imply that it's useful for security, just\nthat it better meets my expectations when -p is turned on. I realize\nthere are some edges in the logic, but I'm fine with those edges.\n\n-John\n"},{"id":"216009","messageId":"vpqy5c0oson.fsf@grenoble-inp.fr","threadId":"33475","inReplyTo":"7va9ogezzx.fsf@alter.siamese.dyndns.org","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-04-30T17:05:12Z","receivedAt":"2013-04-30T17:05:12Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> By the way, these options are _not_ about \"showing merge commits\n> that introduce code\", and they do not help your kind of \"security\".\n> As I repeatedly said, you would need \"-p -m\" for that.\n\nActually, while defaulting to --cc may be convenient, it would indeed\nincrease the security risk: currently, \"git log -p\" shows nothing for\nmerges, so it's rather clear that _everything_ is omitted. With --cc,\nthe user would see a diff, and could hardly guess that not everything is\nshown without reading the doc very carefully.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"216030","messageId":"CAEBDL5W-xuNhyL81TBGhriAr2jM7CC3FtLhfcbEfEAf9GjCJAQ@mail.gmail.com","threadId":"33475","inReplyTo":"vpqy5c0oson.fsf@grenoble-inp.fr","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"John Szakmeister","fromEmail":"john@szakmeister.net","sentAt":"2013-04-30T17:58:42Z","receivedAt":"2013-04-30T17:58:42Z","isPatch":false,"sender":{"key":"john@szakmeister.net","avatar":"https://avatars.githubusercontent.com/u/448087?v=4"},"body":"On Tue, Apr 30, 2013 at 1:05 PM, Matthieu Moy\n wrote:\n> Junio C Hamano writes:\n>\n>> By the way, these options are _not_ about \"showing merge commits\n>> that introduce code\", and they do not help your kind of \"security\".\n>> As I repeatedly said, you would need \"-p -m\" for that.\n>\n> Actually, while defaulting to --cc may be convenient, it would indeed\n> increase the security risk: currently, \"git log -p\" shows nothing for\n> merges, so it's rather clear that _everything_ is omitted. With --cc,\n> the user would see a diff, and could hardly guess that not everything is\n> shown without reading the doc very carefully.\n\nI don't believe it's that clear. I bet people assume there's nothing\nto show, and unless you dig in and discover that `-p` doesn't include\nmerges. In git 1.8.2, `git help log` doesn't seem to make any mention\nof `-p` not showing a diff for merges.\n\nJust to see, I asked several people around here whether they knew `-p`\ndidn't show diffs for merges, and they were all surprised that diffs\nwere being omitted for merge commits.\n\n-John\n"},{"id":"216047","messageId":"CAHQ6N+pDeeZBabiArTXJy9POv10xCBU+=46YdYmW0Ge1qVgUCA@mail.gmail.com","threadId":"33475","inReplyTo":"CAEBDL5W-xuNhyL81TBGhriAr2jM7CC3FtLhfcbEfEAf9GjCJAQ@mail.gmail.com","subject":"Re: git log -p unexpected behaviour - security risk?","fromName":"John Tapsell","fromEmail":"johnflux@gmail.com","sentAt":"2013-04-30T19:31:03Z","receivedAt":"2013-04-30T19:31:03Z","isPatch":false,"sender":{"key":"johnflux@gmail.com","avatar":"https://gravatar.com/avatar/25f70d4c0f96396b84a2e34bcd9bdc233462c7b4be29b5fdca8266fc53f30b0c?d=mp&s=160"},"body":"On 30 April 2013 18:58, John Szakmeister wrote:\n> On Tue, Apr 30, 2013 at 1:05 PM, Matthieu Moy\n> wrote:\n>> Junio C Hamano writes:\n>>\n>>> By the way, these options are _not_ about \"showing merge commits\n>>> that introduce code\", and they do not help your kind of \"security\".\n>>> As I repeatedly said, you would need \"-p -m\" for that.\n>>\n>> Actually, while defaulting to --cc may be convenient, it would indeed\n>> increase the security risk: currently, \"git log -p\" shows nothing for\n>> merges, so it's rather clear that _everything_ is omitted. With --cc,\n>> the user would see a diff, and could hardly guess that not everything is\n>> shown without reading the doc very carefully.\n>\n> I don't believe it's that clear. I bet people assume there's nothing\n> to show, and unless you dig in and discover that `-p` doesn't include\n> merges. In git 1.8.2, `git help log` doesn't seem to make any mention\n> of `-p` not showing a diff for merges.\n>\n> Just to see, I asked several people around here whether they knew `-p`\n> didn't show diffs for merges, and they were all surprised that diffs\n> were being omitted for merge commits.\n\nIs there no way to fix --cc to work even in the edge cases?\n\nJohn\n"},{"id":"216049","messageId":"7vd2tbdcsa.fsf_-_@alter.siamese.dyndns.org","threadId":"33475","inReplyTo":"CAHQ6N+pDeeZBabiArTXJy9POv10xCBU+=46YdYmW0Ge1qVgUCA@mail.gmail.com","subject":"Re: git log -p unexpected behaviour","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-30T19:44:05Z","receivedAt":"2013-04-30T19:44:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Tapsell writes:\n\n> Is there no way to fix --cc to work even in the edge cases?\n\nCan you clarify what you mean by \"fix\" and \"edge cases\"?\n"},{"id":"216056","messageId":"CAHQ6N+pgz3yzFCumgRd3yzpxpqFbkMSzB=tHxmY5hdhzTjeXAg@mail.gmail.com","threadId":"33475","inReplyTo":"7vd2tbdcsa.fsf_-_@alter.siamese.dyndns.org","subject":"Re: git log -p unexpected behaviour","fromName":"John Tapsell","fromEmail":"johnflux@gmail.com","sentAt":"2013-04-30T20:12:49Z","receivedAt":"2013-04-30T20:12:49Z","isPatch":false,"sender":{"key":"johnflux@gmail.com","avatar":"https://gravatar.com/avatar/25f70d4c0f96396b84a2e34bcd9bdc233462c7b4be29b5fdca8266fc53f30b0c?d=mp&s=160"},"body":"On 30 April 2013 20:44, Junio C Hamano wrote:\n> John Tapsell writes:\n>\n>> Is there no way to fix --cc to work even in the edge cases?\n>\n> Can you clarify what you mean by \"fix\" and \"edge cases\"?\n\nMy understanding is that even with -cc there will be changes that\nwon't be seen - and hence why --cc could be even more of a \"security\nrisk\", no?\n\nJohn\n"},{"id":"216061","messageId":"7vvc73bvp9.fsf@alter.siamese.dyndns.org","threadId":"33475","inReplyTo":"CAHQ6N+pgz3yzFCumgRd3yzpxpqFbkMSzB=tHxmY5hdhzTjeXAg@mail.gmail.com","subject":"Re: git log -p unexpected behaviour","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-30T20:38:26Z","receivedAt":"2013-04-30T20:38:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Tapsell writes:\n\n> On 30 April 2013 20:44, Junio C Hamano wrote:\n>> John Tapsell writes:\n>>\n>>> Is there no way to fix --cc to work even in the edge cases?\n>>\n>> Can you clarify what you mean by \"fix\" and \"edge cases\"?\n>\n> My understanding is that even with -cc there will be changes that\n> won't be seen - and hence why --cc could be even more of a \"security\n> risk\", no?\n\nCombined diff is a way to show a tricky conflict resolved in a\ntricky way, so that the tricky-ness of the resolution can be\nexamined. A trivial resolution that takes one side is not shown\nbecause it is not usually interesting. This design choice of course\nhave to trust people involved in the project do not pull from\nuntrustworthy sources.\n\nYou would need \"log -p -m\" (without any pathspec) for the kind of\n\"security\" you are talking about. Note that \"-p -m --first-parent\"\nis not necessarily enough.\n"},{"id":"216125","messageId":"CAHQ6N+rs1miLLUWsGvu5W-nUxU9NK30JEo8gcjXpdGLLXvqK7g@mail.gmail.com","threadId":"33475","inReplyTo":"7vvc73bvp9.fsf@alter.siamese.dyndns.org","subject":"Re: git log -p unexpected behaviour","fromName":"John Tapsell","fromEmail":"johnflux@gmail.com","sentAt":"2013-05-01T07:23:32Z","receivedAt":"2013-05-01T07:23:32Z","isPatch":false,"sender":{"key":"johnflux@gmail.com","avatar":"https://gravatar.com/avatar/25f70d4c0f96396b84a2e34bcd9bdc233462c7b4be29b5fdca8266fc53f30b0c?d=mp&s=160"},"body":"On 30 April 2013 21:38, Junio C Hamano wrote:\n> John Tapsell writes:\n>\n>> On 30 April 2013 20:44, Junio C Hamano wrote:\n>>> John Tapsell writes:\n>>>\n>>>> Is there no way to fix --cc to work even in the edge cases?\n>>>\n>>> Can you clarify what you mean by \"fix\" and \"edge cases\"?\n>>\n>> My understanding is that even with -cc there will be changes that\n>> won't be seen - and hence why --cc could be even more of a \"security\n>> risk\", no?\n>\n> Combined diff is a way to show a tricky conflict resolved in a\n> tricky way, so that the tricky-ness of the resolution can be\n> examined. A trivial resolution that takes one side is not shown\n> because it is not usually interesting.\n\nI don't really understand your point sorry. In this trivial\nresolution case, you would still just see the commit that added the\ncode in a later commit. No?\n\nThere couldn't be a case where you add or change a line of code, but\nnot see it with --cc ?\n\n> This design choice of course\n> have to trust people involved in the project do not pull from\n> untrustworthy sources.\n>\n> You would need \"log -p -m\" (without any pathspec) for the kind of\n> \"security\" you are talking about. Note that \"-p -m --first-parent\"\n> is not necessarily enough.\n\nThis results in seeing the same change more than once though, right?\n"}]}