{"thread":{"id":"32820","subject":"[RFC] Should \"log --cc\" imply \"log --cc -p\"?","startedAt":"2013-02-03T23:10:50Z","lastAt":"2013-02-05T15:51:00Z","messageCount":10,"participants":["Junio C Hamano","Michael J Gruber","Jeff King","Ævar Arnfjörð Bjarmason"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"208573","messageId":"7vmwvl6lj9.fsf@alter.siamese.dyndns.org","threadId":"32820","inReplyTo":null,"subject":"[RFC] Should \"log --cc\" imply \"log --cc -p\"?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-03T23:10:50Z","receivedAt":"2013-02-03T23:10:50Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"I think a natural way to ask reviewing the recent merges while\nshowing tricky ones would be to say:\n\n\t$ git log --first-parent --cc master..pu\n\nBut this does not to show what I expect to see, which is an output\nof:\n\n\t$ git log --first-parent --cc -p master..pu\n\nThis is only a minor irritation, but I think it might make sense to\nmake it notice the --cc in the former and turn -p on automatically.\n\nThe same for\n\n\t$ git log --cc next~3..next\n\nwhich may make sense to turn into \"git log -p --cc next~3..next\".\n\nWhen deciding if the above makes sense, there are a few things to\nknow to be true as prerequisites for the discussion:\n\n * Neither of these\n\n\t$ git log --first-parent -p master..pu\n\t$ git log -p master..pu\n\n shows any patches, and it is not a bug. No patches are shown for\n merges unless -m is given, and when -m is given, we give pairwise\n 2-way diffs for the number of parents.\n\n * We recently tweaked this:\n\n\t$ git log --first-parent -m -p master..pu\n\n to omit diffs with second and later parents, as that is what the\n user wishes with --first-parent.\n\n * The \"--cc\" option, when comparing two trees (i.e. showing a\n non-merge commit), is designed to show a normal patch. In other\n words, you can view \"--cc\" as a modifier when you request a patch\n output format with \"-p\". For \"git show\", \"--cc -p\" is turned on\n by default, and giving \"-m\" explicity (i.e. \"git show -m\") you\n can turn it off and have it do \"-m -p\" instead.\n"},{"id":"208601","messageId":"510F95D7.6010107@drmicha.warpmail.net","threadId":"32820","inReplyTo":"7vmwvl6lj9.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC] Should \"log --cc\" imply \"log --cc -p\"?","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-04T11:04:55Z","receivedAt":"2013-02-04T11:04:55Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 04.02.2013 00:10:\n> I think a natural way to ask reviewing the recent merges while\n> showing tricky ones would be to say:\n> \n> \t$ git log --first-parent --cc master..pu\n> \n> But this does not to show what I expect to see, which is an output\n> of:\n> \n> \t$ git log --first-parent --cc -p master..pu\n> \n\nI'm not Junio, but I guess he would respond that it's a matter of\nexpectations: \"--cc\" is a diff option, and just like any other diff\noption, it has an effect on \"log\" only if \"log\" has been told to show a\ndiff.\n\nOh wait, you *are* Junio ;)\n\n> This is only a minor irritation, but I think it might make sense to\n> make it notice the --cc in the former and turn -p on automatically.\n\nI think we have a clear distiction between rev-list/log options and diff\noptions. \"log\" comes without a diff, \"-p\" turns on diffs.\n\n\"log\" passes diff-options to \"diff\". If the user gives a diff-option to\n\"log\" we can conclude that he meant to request a diff and turn them on\nautomatically (as if \"-p\" was there also).\n\nBut I'm wondering whether that has adverse effects on scripts/aliases.\nFor example, I could have a special alias\n\nnewpu = log first-parent --cc next..pu\n\nwhich allows me to use \"git newpu\" as well as \"git newpu -p\" to get\nthose new commits with or without diff in my preferred format, but not\nany more after the change you suggest. (I could use a second alias, of\ncourse; and yes, I'm (ab)using current option parser features.)\n\n> The same for\n> \n> \t$ git log --cc next~3..next\n> \n> which may make sense to turn into \"git log -p --cc next~3..next\".\n> \n> When deciding if the above makes sense, there are a few things to\n> know to be true as prerequisites for the discussion:\n> \n> * Neither of these\n> \n> \t$ git log --first-parent -p master..pu\n> \t$ git log -p master..pu\n> \n> shows any patches, and it is not a bug. No patches are shown for\n> merges unless -m is given, and when -m is given, we give pairwise\n> 2-way diffs for the number of parents.\n\nBut diffs are on here (\"-p\"), it's just that the default diff option for\nmerges is to not display them. Well, I admit there's two different ways\nof thinking here:\n\nA) \"git log -p\" turns on diffs for all commits, and the default diff\noptions is the (non-existing) \"--no-show\" diff-option for merges.\n\nB) \"git log -p\" applies \"-p\" to all commits except merge commits.\n\nI'm strongly in the A camp, because I think that this gives a clearer\ninterface. A really describes the user facing side, whereas B is closer\nto the implementation.\n\n> * We recently tweaked this:\n> \n> \t$ git log --first-parent -m -p master..pu\n> \n> to omit diffs with second and later parents, as that is what the\n> user wishes with --first-parent.\n\nThat made --first-parent into a dual-purpose option, i.e. it modifies\nthe rev-listing to one parent as well as the diff creation. It does not\nturn on diffs by itself.\n\n> * The \"--cc\" option, when comparing two trees (i.e. showing a\n> non-merge commit), is designed to show a normal patch. In other\n> words, you can view \"--cc\" as a modifier when you request a patch\n> output format with \"-p\". For \"git show\", \"--cc -p\" is turned on\n> by default, and giving \"-m\" explicity (i.e. \"git show -m\") you\n> can turn it off and have it do \"-m -p\" instead.\n> \n\nYes, I really think of --cc is a diff-option, and that this point of\nview gives the clearest ui.\n\nWe could argue that any diff-option could switch on diffs (imply -p),\nbut that change seems to be quite radical. \"show\" having \"-p\" as a\ndefault is quite natural, but that is different.\n\nHaving only \"--cc\" imply \"-p\" would be too much special case auto-magic\nfor my taste. We have too many of these already.\n\nI really think we should leave it as is.\n\nMichael\n"},{"id":"208615","messageId":"7vfw1c3ujo.fsf@alter.siamese.dyndns.org","threadId":"32820","inReplyTo":"510F95D7.6010107@drmicha.warpmail.net","subject":"Re: [RFC] Should \"log --cc\" imply \"log --cc -p\"?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-04T16:36:43Z","receivedAt":"2013-02-04T16:36:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber writes:\n\n> But diffs are on here (\"-p\"), it's just that the default diff option for\n> merges is to not display them. Well, I admit there's two different ways\n> of thinking here:\n>\n> A) \"git log -p\" turns on diffs for all commits, and the default diff\n> options is the (non-existing) \"--no-show\" diff-option for merges.\n>\n> B) \"git log -p\" applies \"-p\" to all commits except merge commits.\n>\n> I'm strongly in the A camp,...\n\nI can personally be trained to think either way, but I suspect that\nwe already came halfway to\n\n C) \"-p\" asks for two-way patches, and \"-c/--cc\" ask for n-way\n patches (including but not limited to n==2); it is not that -p\n asks for patch generation and others modify the finer behaviour\n of patch generation.\n\n\"git log/diff-files -U8\" do not need \"-p\" to enable textual patches,\nfor example. It is \"I already told you that I want 8-line context.\nFor what else, other than showing textual diff, do you think I told\nyou that for?\" and replacing \"8-line context\" with various other\noptions that affect patch generation will give us a variety of end\nuser complaints that would tell us that C) is more intuitive to\nthem.\n\nBut I do not feel very strongly that I am *right* on this issue, so\nI won't do anything about it hastily right now.\n"},{"id":"208696","messageId":"5110C9B3.10102@drmicha.warpmail.net","threadId":"32820","inReplyTo":"7vfw1c3ujo.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC] Should \"log --cc\" imply \"log --cc -p\"?","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2013-02-05T08:58:27Z","receivedAt":"2013-02-05T08:58:27Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 04.02.2013 17:36:\n> Michael J Gruber writes:\n> \n>> But diffs are on here (\"-p\"), it's just that the default diff option for\n>> merges is to not display them. Well, I admit there's two different ways\n>> of thinking here:\n>>\n>> A) \"git log -p\" turns on diffs for all commits, and the default diff\n>> options is the (non-existing) \"--no-show\" diff-option for merges.\n>>\n>> B) \"git log -p\" applies \"-p\" to all commits except merge commits.\n>>\n>> I'm strongly in the A camp,...\n> \n> I can personally be trained to think either way, but I suspect that\n> we already came halfway to\n> \n> C) \"-p\" asks for two-way patches, and \"-c/--cc\" ask for n-way\n> patches (including but not limited to n==2); it is not that -p\n> asks for patch generation and others modify the finer behaviour\n> of patch generation.\n> \n> \"git log/diff-files -U8\" do not need \"-p\" to enable textual patches,\n> for example. It is \"I already told you that I want 8-line context.\n\nThat's a good point that I wasn't aware of.\n\n> For what else, other than showing textual diff, do you think I told\n> you that for?\" and replacing \"8-line context\" with various other\n> options that affect patch generation will give us a variety of end\n> user complaints that would tell us that C) is more intuitive to\n> them.\n> \n> But I do not feel very strongly that I am *right* on this issue, so\n> I won't do anything about it hastily right now.\n\nI'm not sure how many of these we have already, but to me it indicates\nthat we should turn diffs on for log with any diff option that only\nmakes sense with a diff. That would make the ui more consistent without\ntaking anything away.\n\nFor scripting/aliasing purposes, we have \"-s\" in order to override any\nimplied \"-p\" (as I just learned).\n\nMichael\n"},{"id":"208699","messageId":"20130205093307.GC24973@sigill.intra.peff.net","threadId":"32820","inReplyTo":"7vfw1c3ujo.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC] Should \"log --cc\" imply \"log --cc -p\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-05T09:33:07Z","receivedAt":"2013-02-05T09:33:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 04, 2013 at 08:36:43AM -0800, Junio C Hamano wrote:\n\n> \"git log/diff-files -U8\" do not need \"-p\" to enable textual patches,\n> for example. It is \"I already told you that I want 8-line context.\n> For what else, other than showing textual diff, do you think I told\n> you that for?\" and replacing \"8-line context\" with various other\n> options that affect patch generation will give us a variety of end\n> user complaints that would tell us that C) is more intuitive to\n> them.\n\nYeah, I'd agree with this. My feeling is that when there are two\noptions, A and B, and A is a no-op if B is not also specified, that it\nmakes sense for A to imply B. We do it in several places already (and I\njust added some for \"git branch --list\" recently).\n\nIs \"--cc\" a no-op when \"-p\" is not specified? Certainly \"-c\" is not, but\nI do not think you are proposing that. At first glance, \"--cc\" is\nnonsensical without \"-p\", but what about other xdiff callers? For\nexample, in:\n\n git log --cc --stat\n\nthe \"--cc\" is significant. So I don't think it is right for \"--cc\" to\nalways imply \"-p\". But if the rule kicked in only when no other format\nhad been specified, that might make sense.\n\n-Peff\n"},{"id":"208700","messageId":"CACBZZX7FJ39sereptAQ40Q4MwEpf15v0H=++WvB7+BpMknxoYw@mail.gmail.com","threadId":"32820","inReplyTo":"7vfw1c3ujo.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC] Should \"log --cc\" imply \"log --cc -p\"?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2013-02-05T10:16:52Z","receivedAt":"2013-02-05T10:16:52Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Feb 4, 2013 at 5:36 PM, Junio C Hamano wrote:\n> \"git log/diff-files -U8\" do not need \"-p\" to enable textual patches,\n> for example. It is \"I already told you that I want 8-line context.\n> For what else, other than showing textual diff, do you think I told\n> you that for?\" and replacing \"8-line context\" with various other\n> options that affect patch generation will give us a variety of end\n> user complaints that would tell us that C) is more intuitive to\n> them.\n\nOn a related note I think \"--full-diff\" should imply \"-p\" too.\n"},{"id":"208706","messageId":"20130205112249.GE24973@sigill.intra.peff.net","threadId":"32820","inReplyTo":"CACBZZX7FJ39sereptAQ40Q4MwEpf15v0H=++WvB7+BpMknxoYw@mail.gmail.com","subject":"Re: [RFC] Should \"log --cc\" imply \"log --cc -p\"?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-05T11:22:49Z","receivedAt":"2013-02-05T11:22:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 05, 2013 at 11:16:52AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> On Mon, Feb 4, 2013 at 5:36 PM, Junio C Hamano wrote:\n> > \"git log/diff-files -U8\" do not need \"-p\" to enable textual patches,\n> > for example. It is \"I already told you that I want 8-line context.\n> > For what else, other than showing textual diff, do you think I told\n> > you that for?\" and replacing \"8-line context\" with various other\n> > options that affect patch generation will give us a variety of end\n> > user complaints that would tell us that C) is more intuitive to\n> > them.\n> \n> On a related note I think \"--full-diff\" should imply \"-p\" too.\n\nI don't think that is in the same class. --full-diff is quite useful for\nmany other diff formats. E.g.:\n\n git log --full-diff --raw Makefile\n\nIf you are proposing to default to \"-p\" when \"--full-diff\" is used but\nno format is given, that is a slightly different thing. The --full-diff\nin such a case is indeed useless, but I do not think it necessarily\nfollows that \"-p\" was what the user wanted.\n\n-Peff\n"},{"id":"208727","messageId":"CACBZZX7JNwZYHn--Cp4g1rLPratJ9nEa-Q8HsYk65jjDYAkFqA@mail.gmail.com","threadId":"32820","inReplyTo":"20130205112249.GE24973@sigill.intra.peff.net","subject":"Re: [RFC] Should \"log --cc\" imply \"log --cc -p\"?","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2013-02-05T14:27:28Z","receivedAt":"2013-02-05T14:27:28Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Feb 5, 2013 at 12:22 PM, Jeff King wrote:\n> On Tue, Feb 05, 2013 at 11:16:52AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> On Mon, Feb 4, 2013 at 5:36 PM, Junio C Hamano wrote:\n>> > \"git log/diff-files -U8\" do not need \"-p\" to enable textual patches,\n>> > for example. It is \"I already told you that I want 8-line context.\n>> > For what else, other than showing textual diff, do you think I told\n>> > you that for?\" and replacing \"8-line context\" with various other\n>> > options that affect patch generation will give us a variety of end\n>> > user complaints that would tell us that C) is more intuitive to\n>> > them.\n>>\n>> On a related note I think \"--full-diff\" should imply \"-p\" too.\n>\n> I don't think that is in the same class. --full-diff is quite useful for\n> many other diff formats. E.g.:\n>\n> git log --full-diff --raw Makefile\n>\n> If you are proposing to default to \"-p\" when \"--full-diff\" is used but\n> no format is given, that is a slightly different thing. The --full-diff\n> in such a case is indeed useless, but I do not think it necessarily\n> follows that \"-p\" was what the user wanted.\n\nYou're right. I didn't notice that it could work with other things besides -p.\n\nOn a related note then, it's a bit confusing that it's called\n\"--full-diff\" when it doesn't actually show a diff.\n"},{"id":"208731","messageId":"7vsj5avk43.fsf@alter.siamese.dyndns.org","threadId":"32820","inReplyTo":"20130205093307.GC24973@sigill.intra.peff.net","subject":"Re: [RFC] Should \"log --cc\" imply \"log --cc -p\"?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-05T15:46:52Z","receivedAt":"2013-02-05T15:46:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> git log --cc --stat\n>\n> the \"--cc\" is significant. So I don't think it is right for \"--cc\" to\n> always imply \"-p\". But if the rule kicked in only when no other format\n> had been specified, that might make sense.\n\nYes, it was my mistake that I left it unsaid. Obviously the\ndescription of the \"minor irritation\" must be qualified with \"unless\nno other output options like --raw, --stat, etc. are given\".\n\nThanks.\n"},{"id":"208732","messageId":"7vobfyvjx7.fsf@alter.siamese.dyndns.org","threadId":"32820","inReplyTo":"CACBZZX7JNwZYHn--Cp4g1rLPratJ9nEa-Q8HsYk65jjDYAkFqA@mail.gmail.com","subject":"Re: [RFC] Should \"log --cc\" imply \"log --cc -p\"?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-05T15:51:00Z","receivedAt":"2013-02-05T15:51:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On a related note then, it's a bit confusing that it's called\n> \"--full-diff\" when it doesn't actually show a diff.\n\nIt is too late to change the name of the option, but we could add a\nsynonym if it makes easier to understand what the option is for. It\nis about saying \"my pathspec are only to limit the commits to be\nshown and do not limit the diff output\". In that sense, the current\nname that asks to give me the diff output fully is not that wrong,\nbut I wouldn't be surprised if other people can come up with better\nnames.\n"}]}