{"thread":{"id":"18248","subject":"Re: [PATCH 1/2] grep Added --blame so that grep can show result tagged with blame entries","startedAt":"2009-03-10T09:46:54Z","lastAt":"2009-03-10T11:46:55Z","messageCount":3,"participants":["Junio C Hamano","pi song","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"107558","messageId":"7v7i2xaewh.fsf@gitster.siamese.dyndns.org","threadId":"18248","inReplyTo":"49B51791.4030801@gmail.com","subject":"Re: [PATCH 1/2] grep Added --blame so that grep can show result tagged with blame entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-10T09:46:54Z","receivedAt":"2009-03-10T09:46:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"pi song writes:\n\n> This part:-\n> 1) Move reusable bits from builtin-blame.c into blame.c, blame.h\n> 2) Replace static variables with context struct\n>\n> Signed-off-by: Pi Song \n> ---\n> Makefile | 2 +\n> blame.c | 1919 +++++++++++++++++++++++++++++++++++++++++++++++++++\n> blame.h | 222 ++++++\n> builtin-blame.c | 2072\n> +++----------------------------------------------------\n> 4 files changed, 2245 insertions(+), 1970 deletions(-)\n> create mode 100644 blame.c\n> create mode 100644 blame.h\n\nThis is simply too big to ask anybody sane to review (besides as we can\nclearly see in the above the patch is severely whitespace damaged to be\nusable nor mechanically reviewable). \n\nI suspect that most of the file-scope static variables can be moved to the\nscoreboard, and when the reusable parts are made public, some structure\nand function names may need to become a bit more blame specific to avoid\nnamespace contamination.\n\nPerhaps the first two patches in an equivalent series that is rerolled to\nbe reviewable would look like:\n\n (1) move file-scope static variables to the scoreboard, and necessary\n code changes associated with it;\n\n renaming of some structures and functions (if needed---I didn't\n check);\n\n (2) create blame.c and blame.h, and move lines from builtin-blame.c *and*\n do NOTHING OTHER THAN\n\n - adding #include \"blame.h\" to builtin-blame.c,\n\n - adding necessary #include at the top of blame.c,\n\n - surrounding blame.h with necessary\n\n #ifndef BLAME_H\n #define BLAME_H\n\t ...\n #endif\n\n and finally \n\n - updating Makefile to add blame.c and blame.h\n\n This step will make a HUGE patch, and it is crucial for reviewability\n for it not to do anything other than line movement. Ideally, the\n patch shouldn't even reorder the structures and function definitions\n during this step.\n\n Then we can use \"git blame\" itself to make sure that no other changes\n were sneaked in by mistake. \"git blame -C blame.h\" and \"git blame -C\n blame.c\" would show everything came from builtin-blame.c.\n\nAt this point, there shouldn't still be any behaviour change nor new\nfeature. And the titles of these preparatory step will never say anything\nabout \"grep\". They are only refactoring \"blame\".\n\nOnce this becomes solid, you can start adding features to blame.c to\nsupport your new caller, and we can be reasonably sure that such patches\ncan be reviewed to decide if its change breaks the existing (and so far\nthe only) caller builtin_blame() or not.\n"},{"id":"107567","messageId":"1b29507a0903100429v5b4a66d7sa9e3b32010fae2e0@mail.gmail.com","threadId":"18248","inReplyTo":"7v7i2xaewh.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] grep Added --blame so that grep can show result tagged with blame entries","fromName":"pi song","fromEmail":"pi.songs@gmail.com","sentAt":"2009-03-10T11:29:49Z","receivedAt":"2009-03-10T11:29:49Z","isPatch":true,"sender":{"key":"pi.songs@gmail.com","avatar":null},"body":"OK, I'll move my work to \"next\" branch. BTW, can anyone tell me what\nthe \"next\" and \"pu\" branch are for ?\n\nPi Song\n\nOn Tue, Mar 10, 2009 at 8:46 PM, Junio C Hamano wrote:\n> pi song writes:\n>\n>> This part:-\n>> 1) Move reusable bits from builtin-blame.c into blame.c, blame.h\n>> 2) Replace static variables with context struct\n>>\n>> Signed-off-by: Pi Song \n>> ---\n>> Makefile        |    2 +\n>> blame.c         | 1919 +++++++++++++++++++++++++++++++++++++++++++++++++++\n>> blame.h         |  222 ++++++\n>> builtin-blame.c | 2072\n>> +++----------------------------------------------------\n>> 4 files changed, 2245 insertions(+), 1970 deletions(-)\n>> create mode 100644 blame.c\n>> create mode 100644 blame.h\n>\n> This is simply too big to ask anybody sane to review (besides as we can\n> clearly see in the above the patch is severely whitespace damaged to be\n> usable nor mechanically reviewable).\n>\n> I suspect that most of the file-scope static variables can be moved to the\n> scoreboard, and when the reusable parts are made public, some structure\n> and function names may need to become a bit more blame specific to avoid\n> namespace contamination.\n>\n> Perhaps the first two patches in an equivalent series that is rerolled to\n> be reviewable would look like:\n>\n>  (1) move file-scope static variables to the scoreboard, and necessary\n>     code changes associated with it;\n>\n>     renaming of some structures and functions (if needed---I didn't\n>     check);\n>\n>  (2) create blame.c and blame.h, and move lines from builtin-blame.c *and*\n>     do NOTHING OTHER THAN\n>\n>     - adding #include \"blame.h\" to builtin-blame.c,\n>\n>     - adding necessary #include at the top of blame.c,\n>\n>     - surrounding blame.h with necessary\n>\n>         #ifndef BLAME_H\n>         #define BLAME_H\n>         ...\n>         #endif\n>\n>       and finally\n>\n>     - updating Makefile to add blame.c and blame.h\n>\n>     This step will make a HUGE patch, and it is crucial for reviewability\n>     for it not to do anything other than line movement.  Ideally, the\n>     patch shouldn't even reorder the structures and function definitions\n>     during this step.\n>\n>     Then we can use \"git blame\" itself to make sure that no other changes\n>     were sneaked in by mistake.  \"git blame -C blame.h\" and \"git blame -C\n>     blame.c\" would show everything came from builtin-blame.c.\n>\n> At this point, there shouldn't still be any behaviour change nor new\n> feature.  And the titles of these preparatory step will never say anything\n> about \"grep\".  They are only refactoring \"blame\".\n>\n> Once this becomes solid, you can start adding features to blame.c to\n> support your new caller, and we can be reasonably sure that such patches\n> can be reviewed to decide if its change breaks the existing (and so far\n> the only) caller builtin_blame() or not.\n>\n"},{"id":"107569","messageId":"vpq8wnd7g7k.fsf@bauges.imag.fr","threadId":"18248","inReplyTo":"1b29507a0903100429v5b4a66d7sa9e3b32010fae2e0@mail.gmail.com","subject":"Re: [PATCH 1/2] grep Added --blame so that grep can show result tagged with blame entries","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2009-03-10T11:46:55Z","receivedAt":"2009-03-10T11:46:55Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"pi song writes:\n\n> OK, I'll move my work to \"next\" branch. BTW, can anyone tell me what\n> the \"next\" and \"pu\" branch are for ?\n\ngit/Documentation/gitworkflows.txt\n\nIn short : next = things that _should_ enter master in the future.\n pu = proposed updates = things that _may_ enter master.\n\n-- \nMatthieu\n"}]}