{"thread":{"id":"18322","subject":"[PATCH1/2] Libify blame","startedAt":"2009-03-16T13:25:23Z","lastAt":"2009-03-18T07:01:43Z","messageCount":6,"participants":["pi song","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"108080","messageId":"49BE5343.60900@gmail.com","threadId":"18322","inReplyTo":null,"subject":"[PATCH1/2] Libify blame","fromName":"pi song","fromEmail":"pi.songs@gmail.com","sentAt":"2009-03-16T13:25:23Z","receivedAt":"2009-03-16T13:25:23Z","isPatch":false,"sender":{"key":"pi.songs@gmail.com","avatar":null},"body":"The patch does extract blame.h\n\nSigned-off-by: Pi Song <pi.songs@gmail.com>\n---\n Makefile        |    1 +\n blame.h         |  166 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n builtin-blame.c |  145 +-----------------------------------------------\n 3 files changed, 169 insertions(+), 143 deletions(-)\n create mode 100644 blame.h\n\ndiff --git a/Makefile b/Makefile\nindex 38ede68..cc0fb5a 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -359,6 +359,7 @@ XDIFF_LIB=xdiff/lib.a\n \n LIB_H += archive.h\n LIB_H += attr.h\n+LIB_H += blame.h\n LIB_H += blob.h\n LIB_H += builtin.h\n LIB_H += cache.h\ndiff --git a/blame.h b/blame.h\nnew file mode 100644\nindex 0000000..72d1e2a\n--- /dev/null\n+++ b/blame.h\n@@ -0,0 +1,166 @@\n+#ifndef BLAME_H\n+#define BLAME_H\n+\n+#include \"cache.h\"\n+#include \"builtin.h\"\n+#include \"revision.h\"\n+#include \"commit.h\"\n+#include \"string-list.h\"\n+#include \"xdiff-interface.h\"\n+\n+\n+/*\n+ * for storing stats. it can be used\n+ * across multiple blame operations\n+ */\n+struct blame_stat {\n+\tint num_read_blob;\n+\tint num_get_patch;\n+\tint num_commits;\n+};\n+\n+#define PICKAXE_BLAME_MOVE\t\t01\n+#define PICKAXE_BLAME_COPY\t\t02\n+#define PICKAXE_BLAME_COPY_HARDER\t04\n+#define PICKAXE_BLAME_COPY_HARDEST\t010\n+\n+#define BLAME_DEFAULT_MOVE_SCORE\t20\n+#define BLAME_DEFAULT_COPY_SCORE\t40\n+\n+/* bits #0..7 in revision.h, #8..11 used for merge_bases() in commit.c */\n+#define METAINFO_SHOWN\t\t(1u<<12)\n+#define MORE_THAN_ONE_PATH\t(1u<<13)\n+\n+/* output formatting constants */\n+#define OUTPUT_ANNOTATE_COMPAT  001\n+#define OUTPUT_LONG_OBJECT_NAME 002\n+#define OUTPUT_RAW_TIMESTAMP    004\n+#define OUTPUT_PORCELAIN        010\n+#define OUTPUT_SHOW_NAME        020\n+#define OUTPUT_SHOW_NUMBER      040\n+#define OUTPUT_SHOW_SCORE      0100\n+#define OUTPUT_NO_AUTHOR       0200\n+\n+/*\n+ * One blob in a commit that is being suspected\n+ */\n+struct origin {\n+\tint refcnt;\n+\tstruct origin *previous;\n+\tstruct commit *commit;\n+\tmmfile_t file;\n+\tunsigned char blob_sha1[20];\n+\tchar path[FLEX_ARRAY];\n+};\n+\n+/*\n+ * stores things that survive across multiple blame operations\n+ * 1) for blame specific global parameters\n+ * 2) for reusable structures (possibly for optimization purpose)\n+ */\n+struct blame_info {\n+\t/*\n+\t * miscellaneous parameters collected during processing\n+\t * for pretty formatting purpose\n+\t */\n+\tint longest_file;\n+\tint longest_author;\n+\tint max_orig_digits;\n+\tint max_digits;\n+\tint max_score_digits;\n+\n+\t/* formatting parameters */\n+\tint show_root;\n+\tint reverse;\n+\tint blank_boundary;\n+\tint incremental;\n+\tint xdl_opts;\n+\n+\t/*\n+\t * blame for a blame_entry with score lower than these thresholds\n+\t * is not passed to the parent using move/copy logic.\n+\t */\n+\tunsigned blame_move_score;\n+\tunsigned blame_copy_score;\n+\n+\t/* date formatting related */\n+\tenum date_mode blame_date_mode;\n+\tsize_t blame_date_width;\n+\n+\t/* for fast mailmap lookup */\n+\tstruct string_list mailmap;\n+\n+\t/* for stat collecting purpose */\n+\tstruct blame_stat *stat;\n+};\n+\n+/*\n+ * Each group of lines is described by a blame_entry; it can be split\n+ * as we pass blame to the parents.  They form a linked list in the\n+ * scoreboard structure, sorted by the target line number.\n+ */\n+struct blame_entry {\n+\tstruct blame_entry *prev;\n+\tstruct blame_entry *next;\n+\n+\t/* the first line of this group in the final image;\n+\t * internally all line numbers are 0 based.\n+\t */\n+\tint lno;\n+\n+\t/* how many lines this group has */\n+\tint num_lines;\n+\n+\t/* the commit that introduced this group into the final image */\n+\tstruct origin *suspect;\n+\n+\t/* true if the suspect is truly guilty; false while we have not\n+\t * checked if the group came from one of its parents.\n+\t */\n+\tchar guilty;\n+\n+\t/* true if the entry has been scanned for copies in the current parent\n+\t */\n+\tchar scanned;\n+\n+\t/* the line number of the first line of this group in the\n+\t * suspect's file; internally all line numbers are 0 based.\n+\t */\n+\tint s_lno;\n+\n+\t/* how significant this entry is -- cached to avoid\n+\t * scanning the lines over and over.\n+\t */\n+\tunsigned score;\n+};\n+\n+/*\n+ * The current state of the blame assignment.\n+ */\n+struct blame_scoreboard {\n+\t/* the final commit (i.e. where we started digging from) */\n+\tstruct commit *final;\n+\tstruct rev_info *revs;\n+\tconst char *path;\n+\n+\t/*\n+\t * The contents in the final image.\n+\t * Used by many functions to obtain contents of the nth line,\n+\t * indexed with scoreboard.lineno[blame_entry.lno].\n+\t */\n+\tconst char *final_buf;\n+\tunsigned long final_buf_size;\n+\n+\t/* linked list of blames */\n+\tstruct blame_entry *ent;\n+\n+\t/* look-up a line in the final buffer */\n+\tint num_lines;\n+\tint *lineno;\n+\n+\tstruct blame_info *ssb;\n+};\n+\n+extern void assign_blame(struct blame_scoreboard *sb, int opt) ;\n+\n+#endif\ndiff --git a/builtin-blame.c b/builtin-blame.c\nindex fac79be..d4f812b 100644\n--- a/builtin-blame.c\n+++ b/builtin-blame.c\n@@ -20,6 +20,7 @@\n #include \"mailmap.h\"\n #include \"parse-options.h\"\n #include \"utf8.h\"\n+#include \"blame.h\"\n \n static char blame_usage[] = \"git blame [options] [rev-opts] [rev] [--] file\";\n \n@@ -39,81 +40,6 @@ static unsigned opt_blame_copy_score;\n #endif\n \n /*\n- * for storing stats. it can be used\n- * across multiple blame operations\n- */\n-struct blame_stat {\n-\tint num_read_blob;\n-\tint num_get_patch;\n-\tint num_commits;\n-};\n-\n-#define PICKAXE_BLAME_MOVE\t\t01\n-#define PICKAXE_BLAME_COPY\t\t02\n-#define PICKAXE_BLAME_COPY_HARDER\t04\n-#define PICKAXE_BLAME_COPY_HARDEST\t010\n-\n-#define BLAME_DEFAULT_MOVE_SCORE\t20\n-#define BLAME_DEFAULT_COPY_SCORE\t40\n-\n-/* bits #0..7 in revision.h, #8..11 used for merge_bases() in commit.c */\n-#define METAINFO_SHOWN\t\t(1u<<12)\n-#define MORE_THAN_ONE_PATH\t(1u<<13)\n-\n-/*\n- * One blob in a commit that is being suspected\n- */\n-struct origin {\n-\tint refcnt;\n-\tstruct origin *previous;\n-\tstruct commit *commit;\n-\tmmfile_t file;\n-\tunsigned char blob_sha1[20];\n-\tchar path[FLEX_ARRAY];\n-};\n-\n-/*\n- * stores things that survive across multiple blame operations\n- * 1) for blame specific global parameters\n- * 2) for reusable structures (possibly for optimization purpose)\n- */\n-struct blame_info {\n-\t/*\n-\t * miscellaneous parameters collected during processing\n-\t * for pretty formatting purpose\n-\t */\n-\tint longest_file;\n-\tint longest_author;\n-\tint max_orig_digits;\n-\tint max_digits;\n-\tint max_score_digits;\n-\n-\t/* formatting parameters */\n-\tint show_root;\n-\tint reverse;\n-\tint blank_boundary;\n-\tint incremental;\n-\tint xdl_opts;\n-\n-\t/*\n-\t * blame for a blame_entry with score lower than these thresholds\n-\t * is not passed to the parent using move/copy logic.\n-\t */\n-\tunsigned blame_move_score;\n-\tunsigned blame_copy_score;\n-\n-\t/* date formatting related */\n-\tenum date_mode blame_date_mode;\n-\tsize_t blame_date_width;\n-\n-\t/* for fast mailmap lookup */\n-\tstruct string_list mailmap;\n-\n-\t/* for stat collecting purpose */\n-\tstruct blame_stat *stat;\n-};\n-\n-/*\n  * Given an origin, prepare mmfile_t structure to be used by the\n  * diff machinery\n  */\n@@ -163,73 +89,6 @@ static void drop_origin_blob(struct origin *o)\n \t}\n }\n \n-/*\n- * Each group of lines is described by a blame_entry; it can be split\n- * as we pass blame to the parents.  They form a linked list in the\n- * scoreboard structure, sorted by the target line number.\n- */\n-struct blame_entry {\n-\tstruct blame_entry *prev;\n-\tstruct blame_entry *next;\n-\n-\t/* the first line of this group in the final image;\n-\t * internally all line numbers are 0 based.\n-\t */\n-\tint lno;\n-\n-\t/* how many lines this group has */\n-\tint num_lines;\n-\n-\t/* the commit that introduced this group into the final image */\n-\tstruct origin *suspect;\n-\n-\t/* true if the suspect is truly guilty; false while we have not\n-\t * checked if the group came from one of its parents.\n-\t */\n-\tchar guilty;\n-\n-\t/* true if the entry has been scanned for copies in the current parent\n-\t */\n-\tchar scanned;\n-\n-\t/* the line number of the first line of this group in the\n-\t * suspect's file; internally all line numbers are 0 based.\n-\t */\n-\tint s_lno;\n-\n-\t/* how significant this entry is -- cached to avoid\n-\t * scanning the lines over and over.\n-\t */\n-\tunsigned score;\n-};\n-\n-/*\n- * The current state of the blame assignment.\n- */\n-struct blame_scoreboard {\n-\t/* the final commit (i.e. where we started digging from) */\n-\tstruct commit *final;\n-\tstruct rev_info *revs;\n-\tconst char *path;\n-\n-\t/*\n-\t * The contents in the final image.\n-\t * Used by many functions to obtain contents of the nth line,\n-\t * indexed with scoreboard.lineno[blame_entry.lno].\n-\t */\n-\tconst char *final_buf;\n-\tunsigned long final_buf_size;\n-\n-\t/* linked list of blames */\n-\tstruct blame_entry *ent;\n-\n-\t/* look-up a line in the final buffer */\n-\tint num_lines;\n-\tint *lineno;\n-\n-\tstruct blame_info *ssb;\n-};\n-\n static inline int same_suspect(struct origin *a, struct origin *b)\n {\n \tif (a == b)\n@@ -1520,7 +1379,7 @@ static void found_guilty_entry(struct blame_info *ssb, struct blame_entry *ent)\n  * is still unknown, pick one blame_entry, and allow its current\n  * suspect to pass blames to its parents.\n  */\n-static void assign_blame(struct blame_scoreboard *sb, int opt)\n+void assign_blame(struct blame_scoreboard *sb, int opt)\n {\n \tstruct rev_info *revs = sb->revs;\n \n-- \n1.5.4.3\n"},{"id":"108334","messageId":"7vocvzmlqf.fsf@gitster.siamese.dyndns.org","threadId":"18322","inReplyTo":"49BE5343.60900@gmail.com","subject":"Re: [PATCH1/2] Libify blame","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-18T05:41:12Z","receivedAt":"2009-03-18T05:41:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"pi song <pi.songs@gmail.com> writes:\n\n> diff --git a/blame.h b/blame.h\n> new file mode 100644\n> index 0000000..72d1e2a\n> --- /dev/null\n> +++ b/blame.h\n> @@ -0,0 +1,166 @@\n> +/*\n> + * for storing stats. it can be used\n> + * across multiple blame operations\n> + */\n> +struct blame_stat {\n> +\tint num_read_blob;\n> +\tint num_get_patch;\n> +\tint num_commits;\n> +};\n\nAs I said in my previous message, I do not understand why this is not part\nof the super-scoreboard (now blame_info).\n\n> +#define PICKAXE_BLAME_MOVE\t\t01\n> +#define PICKAXE_BLAME_COPY\t\t02\n> +#define PICKAXE_BLAME_COPY_HARDER\t04\n> +#define PICKAXE_BLAME_COPY_HARDEST\t010\n> +\n> +#define BLAME_DEFAULT_MOVE_SCORE\t20\n> +#define BLAME_DEFAULT_COPY_SCORE\t40\n> +\n> +/* bits #0..7 in revision.h, #8..11 used for merge_bases() in commit.c */\n> +#define METAINFO_SHOWN\t\t(1u<<12)\n> +#define MORE_THAN_ONE_PATH\t(1u<<13)\n\nDo we need to expose all of these constants outside blame.c?  I think the\nlibrary caller needs access to the first four above, but I tend to think\nthe latter four are purely internal implementation detail that should be\nkept in blame.c.\n\n> +/* output formatting constants */\n> +#define OUTPUT_ANNOTATE_COMPAT  001\n> +#define OUTPUT_LONG_OBJECT_NAME 002\n> +#define OUTPUT_RAW_TIMESTAMP    004\n> +#define OUTPUT_PORCELAIN        010\n> +#define OUTPUT_SHOW_NAME        020\n> +#define OUTPUT_SHOW_NUMBER      040\n> +#define OUTPUT_SHOW_SCORE      0100\n> +#define OUTPUT_NO_AUTHOR       0200\n\nI think these can be public.\n\n> +/*\n> + * One blob in a commit that is being suspected\n> + */\n> +struct origin {\n> +\tint refcnt;\n> +\tstruct origin *previous;\n> +\tstruct commit *commit;\n> +\tmmfile_t file;\n> +\tunsigned char blob_sha1[20];\n> +\tchar path[FLEX_ARRAY];\n> +};\n\nI somehow doubt we would want to expose this level of implementation\ndetail to the callers of the library.  If we need to, the structure needs\nto be renamed---\"origin\" is way too generic a name.\n\n> +extern void assign_blame(struct blame_scoreboard *sb, int opt) ;\n\nLose the extra SP before \";\".  I had to fix them in your previous patch\nand there were many.\n"},{"id":"108340","messageId":"1b29507a0903172259t348cb4d5n70f5b3003b1eeb00@mail.gmail.com","threadId":"18322","inReplyTo":"7vocvzmlqf.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH1/2] Libify blame","fromName":"pi song","fromEmail":"pi.songs@gmail.com","sentAt":"2009-03-18T05:59:07Z","receivedAt":"2009-03-18T05:59:07Z","isPatch":false,"sender":{"key":"pi.songs@gmail.com","avatar":null},"body":"Don't you think we should rather split up into smaller files before\nstart reorganizing things?\n\nPi Song\n\nOn Wed, Mar 18, 2009 at 4:41 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> pi song <pi.songs@gmail.com> writes:\n>\n>> diff --git a/blame.h b/blame.h\n>> new file mode 100644\n>> index 0000000..72d1e2a\n>> --- /dev/null\n>> +++ b/blame.h\n>> @@ -0,0 +1,166 @@\n>> +/*\n>> + * for storing stats. it can be used\n>> + * across multiple blame operations\n>> + */\n>> +struct blame_stat {\n>> +     int num_read_blob;\n>> +     int num_get_patch;\n>> +     int num_commits;\n>> +};\n>\n> As I said in my previous message, I do not understand why this is not part\n> of the super-scoreboard (now blame_info).\n>\n>> +#define PICKAXE_BLAME_MOVE           01\n>> +#define PICKAXE_BLAME_COPY           02\n>> +#define PICKAXE_BLAME_COPY_HARDER    04\n>> +#define PICKAXE_BLAME_COPY_HARDEST   010\n>> +\n>> +#define BLAME_DEFAULT_MOVE_SCORE     20\n>> +#define BLAME_DEFAULT_COPY_SCORE     40\n>> +\n>> +/* bits #0..7 in revision.h, #8..11 used for merge_bases() in commit.c */\n>> +#define METAINFO_SHOWN               (1u<<12)\n>> +#define MORE_THAN_ONE_PATH   (1u<<13)\n>\n> Do we need to expose all of these constants outside blame.c?  I think the\n> library caller needs access to the first four above, but I tend to think\n> the latter four are purely internal implementation detail that should be\n> kept in blame.c.\n>\n>> +/* output formatting constants */\n>> +#define OUTPUT_ANNOTATE_COMPAT  001\n>> +#define OUTPUT_LONG_OBJECT_NAME 002\n>> +#define OUTPUT_RAW_TIMESTAMP    004\n>> +#define OUTPUT_PORCELAIN        010\n>> +#define OUTPUT_SHOW_NAME        020\n>> +#define OUTPUT_SHOW_NUMBER      040\n>> +#define OUTPUT_SHOW_SCORE      0100\n>> +#define OUTPUT_NO_AUTHOR       0200\n>\n> I think these can be public.\n>\n>> +/*\n>> + * One blob in a commit that is being suspected\n>> + */\n>> +struct origin {\n>> +     int refcnt;\n>> +     struct origin *previous;\n>> +     struct commit *commit;\n>> +     mmfile_t file;\n>> +     unsigned char blob_sha1[20];\n>> +     char path[FLEX_ARRAY];\n>> +};\n>\n> I somehow doubt we would want to expose this level of implementation\n> detail to the callers of the library.  If we need to, the structure needs\n> to be renamed---\"origin\" is way too generic a name.\n>\n>> +extern void assign_blame(struct blame_scoreboard *sb, int opt) ;\n>\n> Lose the extra SP before \";\".  I had to fix them in your previous patch\n> and there were many.\n>\n"},{"id":"108341","messageId":"7v3adbmjwy.fsf@gitster.siamese.dyndns.org","threadId":"18322","inReplyTo":"1b29507a0903172259t348cb4d5n70f5b3003b1eeb00@mail.gmail.com","subject":"Re: [PATCH1/2] Libify blame","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-18T06:20:29Z","receivedAt":"2009-03-18T06:20:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"pi song <pi.songs@gmail.com> writes:\n\n> Don't you think we should rather split up into smaller files before\n> start reorganizing things?\n\nYes, but splitting it wrong is, eh, wrong ;-)\n"},{"id":"108345","messageId":"1b29507a0903172352x7864911fm1104e22eddde54f1@mail.gmail.com","threadId":"18322","inReplyTo":"7v3adbmjwy.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH1/2] Libify blame","fromName":"pi song","fromEmail":"pi.songs@gmail.com","sentAt":"2009-03-18T06:52:38Z","receivedAt":"2009-03-18T06:52:38Z","isPatch":false,"sender":{"key":"pi.songs@gmail.com","avatar":null},"body":"Wait. If you look at the builtin-blame.c, out of question it is very\nmessy. Things like print_usage() or -L parameter parsing for example\nis not done upfront but hiding somewhere. Some functions are not very\nclear if they are frontend or backend. I would say nobody would be\nable to split it right in the first place. What you could do is to\nsplit it to something \"roughly right\" and then work from that.\n\nMy latest two patches really do nothing but just splitting files. I\nhaven't changed any logics or renamed any thing only to make this big\nbeast more *manageable* rather than tackling the problem directly.\nYes, some bits are  still wrong but I believe 70% of the functions\nshould already stay in the right place. The following patches will\nmake the structure more right *gradually*.\n\nPi Song\n\n\nOn Wed, Mar 18, 2009 at 5:20 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> pi song <pi.songs@gmail.com> writes:\n>\n>> Don't you think we should rather split up into smaller files before\n>> start reorganizing things?\n>\n> Yes, but splitting it wrong is, eh, wrong ;-)\n>\n"},{"id":"108346","messageId":"1b29507a0903180001yd510e49ld6d41a171f94e211@mail.gmail.com","threadId":"18322","inReplyTo":"1b29507a0903172352x7864911fm1104e22eddde54f1@mail.gmail.com","subject":"Re: [PATCH1/2] Libify blame","fromName":"pi song","fromEmail":"pi.songs@gmail.com","sentAt":"2009-03-18T07:01:43Z","receivedAt":"2009-03-18T07:01:43Z","isPatch":false,"sender":{"key":"pi.songs@gmail.com","avatar":null},"body":"BTW, following patches are not available yet : P\n\nOn Wed, Mar 18, 2009 at 5:52 PM, pi song <pi.songs@gmail.com> wrote:\n> Wait. If you look at the builtin-blame.c, out of question it is very\n> messy. Things like print_usage() or -L parameter parsing for example\n> is not done upfront but hiding somewhere. Some functions are not very\n> clear if they are frontend or backend. I would say nobody would be\n> able to split it right in the first place. What you could do is to\n> split it to something \"roughly right\" and then work from that.\n>\n> My latest two patches really do nothing but just splitting files. I\n> haven't changed any logics or renamed any thing only to make this big\n> beast more *manageable* rather than tackling the problem directly.\n> Yes, some bits are  still wrong but I believe 70% of the functions\n> should already stay in the right place. The following patches will\n> make the structure more right *gradually*.\n>\n> Pi Song\n>\n>\n> On Wed, Mar 18, 2009 at 5:20 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> pi song <pi.songs@gmail.com> writes:\n>>\n>>> Don't you think we should rather split up into smaller files before\n>>> start reorganizing things?\n>>\n>> Yes, but splitting it wrong is, eh, wrong ;-)\n>>\n>\n"}]}