{"thread":{"id":"4644","subject":"[PATCH 0/5] Rework diff options","startedAt":"2006-06-23T22:15:38Z","lastAt":"2006-06-24T02:51:09Z","messageCount":8,"participants":["Timo Hirvonen","Johannes Schindelin","Linus Torvalds","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"22377","messageId":"20060624011538.9bb179e7.tihirvon@gmail.com","threadId":"4644","inReplyTo":null,"subject":"[PATCH 0/5] Rework diff options","fromName":"Timo Hirvonen","fromEmail":"tihirvon@gmail.com","sentAt":"2006-06-23T22:15:38Z","receivedAt":"2006-06-23T22:15:38Z","isPatch":true,"sender":{"key":"tihirvon@gmail.com","avatar":null},"body":"This patch series cleans up diff output format options.\n\nThis makes it possible to use any combination of --raw, -p, --stat and\n--summary options and they work as you would expect.\n\nThese patches passed all tests but patch 2/5 is quite intrusive...\n\n b/builtin-diff-files.c | 10 +-\n b/builtin-diff-index.c | 4 \n b/builtin-diff-stages.c | 3 \n b/builtin-diff-tree.c | 3 \n b/builtin-diff.c | 74 ++++++-----------\n b/builtin-log.c | 12 +-\n b/combine-diff.c | 45 +++-------\n b/diff.c | 207 +++++++++++++++++++++++-------------------------\n b/diff.h | 29 +++---\n b/git-merge.sh | 3 \n b/log-tree.c | 15 ++-\n b/revision.c | 5 -\n 12 files changed, 196 insertions(+), 214 deletions(-)\n\n-- \nhttp://onion.dynserv.net/~timo/\n"},{"id":"22382","messageId":"Pine.LNX.4.63.0606240024460.29667@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"4644","inReplyTo":"20060624011538.9bb179e7.tihirvon@gmail.com","subject":"Re: [PATCH 0/5] Rework diff options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-06-23T22:28:41Z","receivedAt":"2006-06-23T22:28:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 24 Jun 2006, Timo Hirvonen wrote:\n\n> This patch series cleans up diff output format options.\n\nVery good.\n\nAlthough I understand that to convert all users to the new convention, it \nis sensible to rename the constants, I think it is not good to change \nsomething as DIFF_FORMAT_RAW to OUTPUT_FMT_RAW in the resulting patch.\n\nIMHO it is an unnecessary change, and accounts for a lot of the diffstat.\n\nCiao,\nDscho\n"},{"id":"22384","messageId":"20060624014420.2c3df276.tihirvon@gmail.com","threadId":"4644","inReplyTo":"Pine.LNX.4.63.0606240024460.29667@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 0/5] Rework diff options","fromName":"Timo Hirvonen","fromEmail":"tihirvon@gmail.com","sentAt":"2006-06-23T22:44:20Z","receivedAt":"2006-06-23T22:44:20Z","isPatch":true,"sender":{"key":"tihirvon@gmail.com","avatar":null},"body":"Johannes Schindelin wrote:\n\n> Hi,\n> \n> On Sat, 24 Jun 2006, Timo Hirvonen wrote:\n> \n> > This patch series cleans up diff output format options.\n> \n> Very good.\n> \n> Although I understand that to convert all users to the new convention, it \n> is sensible to rename the constants, I think it is not good to change \n> something as DIFF_FORMAT_RAW to OUTPUT_FMT_RAW in the resulting patch.\n> \n> IMHO it is an unnecessary change, and accounts for a lot of the diffstat.\n\nI did it because you can't have many DIFF_FORMAT_* options at the same\ntime but OUTPUT_FMT_* can be combined.\n\n-- \nhttp://onion.dynserv.net/~timo/\n"},{"id":"22387","messageId":"Pine.LNX.4.64.0606231615150.6483@g5.osdl.org","threadId":"4644","inReplyTo":"20060624011538.9bb179e7.tihirvon@gmail.com","subject":"Re: [PATCH 0/5] Rework diff options","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-06-23T23:16:22Z","receivedAt":"2006-06-23T23:16:22Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 24 Jun 2006, Timo Hirvonen wrote:\n>\n> This patch series cleans up diff output format options.\n> \n> This makes it possible to use any combination of --raw, -p, --stat and\n> --summary options and they work as you would expect.\n\nLooks good to me. I'll be very happy never having to remember the option \n(or type) --patch-with-stat ever again. Doing just \"-p --stat\" is just \n_so_ much better.\n\n\t\tLinus\n"},{"id":"22388","messageId":"Pine.LNX.4.63.0606240111590.29667@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"4644","inReplyTo":"20060624014420.2c3df276.tihirvon@gmail.com","subject":"Re: [PATCH 0/5] Rework diff options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-06-23T23:18:06Z","receivedAt":"2006-06-23T23:18:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 24 Jun 2006, Timo Hirvonen wrote:\n\n> Johannes Schindelin wrote:\n> \n> > Although I understand that to convert all users to the new convention, it \n> > is sensible to rename the constants, I think it is not good to change \n> > something as DIFF_FORMAT_RAW to OUTPUT_FMT_RAW in the resulting patch.\n\nNote that I understand this for the purpose of not forgetting to change \nthings over to \"|=\" and \"&\": the compiler will warn you about that now.\n\nBut after it compiles, you can change the names back to reduce patch size \nand to avoid confusing of dumb people like me.\n\n> > IMHO it is an unnecessary change, and accounts for a lot of the diffstat.\n> \n> I did it because you can't have many DIFF_FORMAT_* options at the same\n> time but OUTPUT_FMT_* can be combined.\n\nBut you just renamed them! The name alone does not say \"you cannot combine \nthem\".\n\n-- snip --\n@@ -150,15 +162,6 @@ #define COMMON_DIFF_OPTIONS_HELP \\\n \" show all files diff when -S is used and hit is found.\\n\"\n \n extern int diff_queue_is_empty(void);\n-\n-#define DIFF_FORMAT_RAW 1\n-#define DIFF_FORMAT_PATCH 2\n-#define DIFF_FORMAT_NO_OUTPUT 3\n-#define DIFF_FORMAT_NAME 4\n-#define DIFF_FORMAT_NAME_STATUS 5\n-#define DIFF_FORMAT_DIFFSTAT 6\n-#define DIFF_FORMAT_CHECKDIFF 7\n-\n-- snap --\n\nYou also sneak in some other things, such as renaming output_format to \noutput_fmt in struct diff_options, making a function static, and expanding \na \"(a ? b : c)\", without accounting for it in the commit message.\n\nCiao,\nDscho\n"},{"id":"22389","messageId":"7vodwj8n9s.fsf@assigned-by-dhcp.cox.net","threadId":"4644","inReplyTo":"Pine.LNX.4.63.0606240024460.29667@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 0/5] Rework diff options","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-06-23T23:28:47Z","receivedAt":"2006-06-23T23:28:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi,\n>\n> On Sat, 24 Jun 2006, Timo Hirvonen wrote:\n>\n>> This patch series cleans up diff output format options.\n>\n> Very good.\n>\n> Although I understand that to convert all users to the new convention, it \n> is sensible to rename the constants, I think it is not good to change \n> something as DIFF_FORMAT_RAW to OUTPUT_FMT_RAW in the resulting patch.\n\nI personally feel that the benefit of being able to make sure\nyou covered everything outweighs the size of initial diff.\n\nThanks Timo. Will take a look.\n"},{"id":"22392","messageId":"Pine.LNX.4.63.0606240201580.29667@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"4644","inReplyTo":"7vodwj8n9s.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 0/5] Rework diff options","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-06-24T00:04:44Z","receivedAt":"2006-06-24T00:04:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 23 Jun 2006, Junio C Hamano wrote:\n\n> I personally feel that the benefit of being able to make sure you \n> covered everything outweighs the size of initial diff.\n\nIMHO the difficulty of finding bugs is proportional to the square of the \ndiff size, while the number of people willing to review it is proportional \nto its square root. So, if it is not difficult (which it is not at all in \nthis case), I politely ask to cut the patch size down.\n\nCiao,\nDscho\n"},{"id":"22415","messageId":"7vac835krm.fsf@assigned-by-dhcp.cox.net","threadId":"4644","inReplyTo":"Pine.LNX.4.63.0606240201580.29667@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 0/5] Rework diff options","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-06-24T02:51:09Z","receivedAt":"2006-06-24T02:51:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Fri, 23 Jun 2006, Junio C Hamano wrote:\n>\n>> I personally feel that the benefit of being able to make sure you \n>> covered everything outweighs the size of initial diff.\n>\n> IMHO the difficulty of finding bugs is proportional to the square of the \n> diff size, while the number of people willing to review it is proportional \n> to its square root. So, if it is not difficult (which it is not at all in \n> this case), I politely ask to cut the patch size down.\n\nYeah, your \"renaming in private while you are making sure but\nrename them back\" suggestion actually would work in this case.\n"}]}