[PATCH1/2] Libify blame

[PATCH1/2] Libify blame

[pi song]

The patch does extract blame.h

Signed-off-by: Pi Song <pi.songs@gmail.com>
---
 Makefile        |    1 +
 blame.h         |  166 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 builtin-blame.c |  145 +-----------------------------------------------
 3 files changed, 169 insertions(+), 143 deletions(-)
 create mode 100644 blame.h

diff --git a/Makefile b/Makefile
index 38ede68..cc0fb5a 100644
--- a/Makefile
+++ b/Makefile
@@ -359,6 +359,7 @@ XDIFF_LIB=xdiff/lib.a
 
 LIB_H += archive.h
 LIB_H += attr.h
+LIB_H += blame.h
 LIB_H += blob.h
 LIB_H += builtin.h
 LIB_H += cache.h
diff --git a/blame.h b/blame.h
new file mode 100644
index 0000000..72d1e2a
--- /dev/null
+++ b/blame.h
@@ -0,0 +1,166 @@
+#ifndef BLAME_H
+#define BLAME_H
+
+#include "cache.h"
+#include "builtin.h"
+#include "revision.h"
+#include "commit.h"
+#include "string-list.h"
+#include "xdiff-interface.h"
+
+
+/*
+ * for storing stats. it can be used
+ * across multiple blame operations
+ */
+struct blame_stat {
+	int num_read_blob;
+	int num_get_patch;
+	int num_commits;
+};
+
+#define PICKAXE_BLAME_MOVE		01
+#define PICKAXE_BLAME_COPY		02
+#define PICKAXE_BLAME_COPY_HARDER	04
+#define PICKAXE_BLAME_COPY_HARDEST	010
+
+#define BLAME_DEFAULT_MOVE_SCORE	20
+#define BLAME_DEFAULT_COPY_SCORE	40
+
+/* bits #0..7 in revision.h, #8..11 used for merge_bases() in commit.c */
+#define METAINFO_SHOWN		(1u<<12)
+#define MORE_THAN_ONE_PATH	(1u<<13)
+
+/* output formatting constants */
+#define OUTPUT_ANNOTATE_COMPAT  001
+#define OUTPUT_LONG_OBJECT_NAME 002
+#define OUTPUT_RAW_TIMESTAMP    004
+#define OUTPUT_PORCELAIN        010
+#define OUTPUT_SHOW_NAME        020
+#define OUTPUT_SHOW_NUMBER      040
+#define OUTPUT_SHOW_SCORE      0100
+#define OUTPUT_NO_AUTHOR       0200
+
+/*
+ * One blob in a commit that is being suspected
+ */
+struct origin {
+	int refcnt;
+	struct origin *previous;
+	struct commit *commit;
+	mmfile_t file;
+	unsigned char blob_sha1[20];
+	char path[FLEX_ARRAY];
+};
+
+/*
+ * stores things that survive across multiple blame operations
+ * 1) for blame specific global parameters
+ * 2) for reusable structures (possibly for optimization purpose)
+ */
+struct blame_info {
+	/*
+	 * miscellaneous parameters collected during processing
+	 * for pretty formatting purpose
+	 */
+	int longest_file;
+	int longest_author;
+	int max_orig_digits;
+	int max_digits;
+	int max_score_digits;
+
+	/* formatting parameters */
+	int show_root;
+	int reverse;
+	int blank_boundary;
+	int incremental;
+	int xdl_opts;
+
+	/*
+	 * blame for a blame_entry with score lower than these thresholds
+	 * is not passed to the parent using move/copy logic.
+	 */
+	unsigned blame_move_score;
+	unsigned blame_copy_score;
+
+	/* date formatting related */
+	enum date_mode blame_date_mode;
+	size_t blame_date_width;
+
+	/* for fast mailmap lookup */
+	struct string_list mailmap;
+
+	/* for stat collecting purpose */
+	struct blame_stat *stat;
+};
+
+/*
+ * Each group of lines is described by a blame_entry; it can be split
+ * as we pass blame to the parents.  They form a linked list in the
+ * scoreboard structure, sorted by the target line number.
+ */
+struct blame_entry {
+	struct blame_entry *prev;
+	struct blame_entry *next;
+
+	/* the first line of this group in the final image;
+	 * internally all line numbers are 0 based.
+	 */
+	int lno;
+
+	/* how many lines this group has */
+	int num_lines;
+
+	/* the commit that introduced this group into the final image */
+	struct origin *suspect;
+
+	/* true if the suspect is truly guilty; false while we have not
+	 * checked if the group came from one of its parents.
+	 */
+	char guilty;
+
+	/* true if the entry has been scanned for copies in the current parent
+	 */
+	char scanned;
+
+	/* the line number of the first line of this group in the
+	 * suspect's file; internally all line numbers are 0 based.
+	 */
+	int s_lno;
+
+	/* how significant this entry is -- cached to avoid
+	 * scanning the lines over and over.
+	 */
+	unsigned score;
+};
+
+/*
+ * The current state of the blame assignment.
+ */
+struct blame_scoreboard {
+	/* the final commit (i.e. where we started digging from) */
+	struct commit *final;
+	struct rev_info *revs;
+	const char *path;
+
+	/*
+	 * The contents in the final image.
+	 * Used by many functions to obtain contents of the nth line,
+	 * indexed with scoreboard.lineno[blame_entry.lno].
+	 */
+	const char *final_buf;
+	unsigned long final_buf_size;
+
+	/* linked list of blames */
+	struct blame_entry *ent;
+
+	/* look-up a line in the final buffer */
+	int num_lines;
+	int *lineno;
+
+	struct blame_info *ssb;
+};
+
+extern void assign_blame(struct blame_scoreboard *sb, int opt) ;
+
+#endif
diff --git a/builtin-blame.c b/builtin-blame.c
index fac79be..d4f812b 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -20,6 +20,7 @@
 #include "mailmap.h"
 #include "parse-options.h"
 #include "utf8.h"
+#include "blame.h"
 
 static char blame_usage[] = "git blame [options] [rev-opts] [rev] [--] file";
 
@@ -39,81 +40,6 @@ static unsigned opt_blame_copy_score;
 #endif
 
 /*
- * for storing stats. it can be used
- * across multiple blame operations
- */
-struct blame_stat {
-	int num_read_blob;
-	int num_get_patch;
-	int num_commits;
-};
-
-#define PICKAXE_BLAME_MOVE		01
-#define PICKAXE_BLAME_COPY		02
-#define PICKAXE_BLAME_COPY_HARDER	04
-#define PICKAXE_BLAME_COPY_HARDEST	010
-
-#define BLAME_DEFAULT_MOVE_SCORE	20
-#define BLAME_DEFAULT_COPY_SCORE	40
-
-/* bits #0..7 in revision.h, #8..11 used for merge_bases() in commit.c */
-#define METAINFO_SHOWN		(1u<<12)
-#define MORE_THAN_ONE_PATH	(1u<<13)
-
-/*
- * One blob in a commit that is being suspected
- */
-struct origin {
-	int refcnt;
-	struct origin *previous;
-	struct commit *commit;
-	mmfile_t file;
-	unsigned char blob_sha1[20];
-	char path[FLEX_ARRAY];
-};
-
-/*
- * stores things that survive across multiple blame operations
- * 1) for blame specific global parameters
- * 2) for reusable structures (possibly for optimization purpose)
- */
-struct blame_info {
-	/*
-	 * miscellaneous parameters collected during processing
-	 * for pretty formatting purpose
-	 */
-	int longest_file;
-	int longest_author;
-	int max_orig_digits;
-	int max_digits;
-	int max_score_digits;
-
-	/* formatting parameters */
-	int show_root;
-	int reverse;
-	int blank_boundary;
-	int incremental;
-	int xdl_opts;
-
-	/*
-	 * blame for a blame_entry with score lower than these thresholds
-	 * is not passed to the parent using move/copy logic.
-	 */
-	unsigned blame_move_score;
-	unsigned blame_copy_score;
-
-	/* date formatting related */
-	enum date_mode blame_date_mode;
-	size_t blame_date_width;
-
-	/* for fast mailmap lookup */
-	struct string_list mailmap;
-
-	/* for stat collecting purpose */
-	struct blame_stat *stat;
-};
-
-/*
  * Given an origin, prepare mmfile_t structure to be used by the
  * diff machinery
  */
@@ -163,73 +89,6 @@ static void drop_origin_blob(struct origin *o)
 	}
 }
 
-/*
- * Each group of lines is described by a blame_entry; it can be split
- * as we pass blame to the parents.  They form a linked list in the
- * scoreboard structure, sorted by the target line number.
- */
-struct blame_entry {
-	struct blame_entry *prev;
-	struct blame_entry *next;
-
-	/* the first line of this group in the final image;
-	 * internally all line numbers are 0 based.
-	 */
-	int lno;
-
-	/* how many lines this group has */
-	int num_lines;
-
-	/* the commit that introduced this group into the final image */
-	struct origin *suspect;
-
-	/* true if the suspect is truly guilty; false while we have not
-	 * checked if the group came from one of its parents.
-	 */
-	char guilty;
-
-	/* true if the entry has been scanned for copies in the current parent
-	 */
-	char scanned;
-
-	/* the line number of the first line of this group in the
-	 * suspect's file; internally all line numbers are 0 based.
-	 */
-	int s_lno;
-
-	/* how significant this entry is -- cached to avoid
-	 * scanning the lines over and over.
-	 */
-	unsigned score;
-};
-
-/*
- * The current state of the blame assignment.
- */
-struct blame_scoreboard {
-	/* the final commit (i.e. where we started digging from) */
-	struct commit *final;
-	struct rev_info *revs;
-	const char *path;
-
-	/*
-	 * The contents in the final image.
-	 * Used by many functions to obtain contents of the nth line,
-	 * indexed with scoreboard.lineno[blame_entry.lno].
-	 */
-	const char *final_buf;
-	unsigned long final_buf_size;
-
-	/* linked list of blames */
-	struct blame_entry *ent;
-
-	/* look-up a line in the final buffer */
-	int num_lines;
-	int *lineno;
-
-	struct blame_info *ssb;
-};
-
 static inline int same_suspect(struct origin *a, struct origin *b)
 {
 	if (a == b)
@@ -1520,7 +1379,7 @@ static void found_guilty_entry(struct blame_info *ssb, struct blame_entry *ent)
  * is still unknown, pick one blame_entry, and allow its current
  * suspect to pass blames to its parents.
  */
-static void assign_blame(struct blame_scoreboard *sb, int opt)
+void assign_blame(struct blame_scoreboard *sb, int opt)
 {
 	struct rev_info *revs = sb->revs;
 
-- 
1.5.4.3

Re: [PATCH1/2] Libify blame

[Junio C Hamano]

pi song <pi.songs@gmail.com> writes:

> diff --git a/blame.h b/blame.h
> new file mode 100644
> index 0000000..72d1e2a
> --- /dev/null
> +++ b/blame.h
> @@ -0,0 +1,166 @@
> +/*
> + * for storing stats. it can be used
> + * across multiple blame operations
> + */
> +struct blame_stat {
> +	int num_read_blob;
> +	int num_get_patch;
> +	int num_commits;
> +};

As I said in my previous message, I do not understand why this is not part
of the super-scoreboard (now blame_info).

> +#define PICKAXE_BLAME_MOVE		01
> +#define PICKAXE_BLAME_COPY		02
> +#define PICKAXE_BLAME_COPY_HARDER	04
> +#define PICKAXE_BLAME_COPY_HARDEST	010
> +
> +#define BLAME_DEFAULT_MOVE_SCORE	20
> +#define BLAME_DEFAULT_COPY_SCORE	40
> +
> +/* bits #0..7 in revision.h, #8..11 used for merge_bases() in commit.c */
> +#define METAINFO_SHOWN		(1u<<12)
> +#define MORE_THAN_ONE_PATH	(1u<<13)

Do we need to expose all of these constants outside blame.c?  I think the
library caller needs access to the first four above, but I tend to think
the latter four are purely internal implementation detail that should be
kept in blame.c.

> +/* output formatting constants */
> +#define OUTPUT_ANNOTATE_COMPAT  001
> +#define OUTPUT_LONG_OBJECT_NAME 002
> +#define OUTPUT_RAW_TIMESTAMP    004
> +#define OUTPUT_PORCELAIN        010
> +#define OUTPUT_SHOW_NAME        020
> +#define OUTPUT_SHOW_NUMBER      040
> +#define OUTPUT_SHOW_SCORE      0100
> +#define OUTPUT_NO_AUTHOR       0200

I think these can be public.

> +/*
> + * One blob in a commit that is being suspected
> + */
> +struct origin {
> +	int refcnt;
> +	struct origin *previous;
> +	struct commit *commit;
> +	mmfile_t file;
> +	unsigned char blob_sha1[20];
> +	char path[FLEX_ARRAY];
> +};

I somehow doubt we would want to expose this level of implementation
detail to the callers of the library.  If we need to, the structure needs
to be renamed---"origin" is way too generic a name.

> +extern void assign_blame(struct blame_scoreboard *sb, int opt) ;

Lose the extra SP before ";".  I had to fix them in your previous patch
and there were many.

Re: [PATCH1/2] Libify blame

[pi song]

Don't you think we should rather split up into smaller files before
start reorganizing things?

Pi Song

On Wed, Mar 18, 2009 at 4:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
> pi song <pi.songs@gmail.com> writes:
>
>> diff --git a/blame.h b/blame.h
>> new file mode 100644
>> index 0000000..72d1e2a
>> --- /dev/null
>> +++ b/blame.h
>> @@ -0,0 +1,166 @@
>> +/*
>> + * for storing stats. it can be used
>> + * across multiple blame operations
>> + */
>> +struct blame_stat {
>> +     int num_read_blob;
>> +     int num_get_patch;
>> +     int num_commits;
>> +};
>
> As I said in my previous message, I do not understand why this is not part
> of the super-scoreboard (now blame_info).
>
>> +#define PICKAXE_BLAME_MOVE           01
>> +#define PICKAXE_BLAME_COPY           02
>> +#define PICKAXE_BLAME_COPY_HARDER    04
>> +#define PICKAXE_BLAME_COPY_HARDEST   010
>> +
>> +#define BLAME_DEFAULT_MOVE_SCORE     20
>> +#define BLAME_DEFAULT_COPY_SCORE     40
>> +
>> +/* bits #0..7 in revision.h, #8..11 used for merge_bases() in commit.c */
>> +#define METAINFO_SHOWN               (1u<<12)
>> +#define MORE_THAN_ONE_PATH   (1u<<13)
>
> Do we need to expose all of these constants outside blame.c?  I think the
> library caller needs access to the first four above, but I tend to think
> the latter four are purely internal implementation detail that should be
> kept in blame.c.
>
>> +/* output formatting constants */
>> +#define OUTPUT_ANNOTATE_COMPAT  001
>> +#define OUTPUT_LONG_OBJECT_NAME 002
>> +#define OUTPUT_RAW_TIMESTAMP    004
>> +#define OUTPUT_PORCELAIN        010
>> +#define OUTPUT_SHOW_NAME        020
>> +#define OUTPUT_SHOW_NUMBER      040
>> +#define OUTPUT_SHOW_SCORE      0100
>> +#define OUTPUT_NO_AUTHOR       0200
>
> I think these can be public.
>
>> +/*
>> + * One blob in a commit that is being suspected
>> + */
>> +struct origin {
>> +     int refcnt;
>> +     struct origin *previous;
>> +     struct commit *commit;
>> +     mmfile_t file;
>> +     unsigned char blob_sha1[20];
>> +     char path[FLEX_ARRAY];
>> +};
>
> I somehow doubt we would want to expose this level of implementation
> detail to the callers of the library.  If we need to, the structure needs
> to be renamed---"origin" is way too generic a name.
>
>> +extern void assign_blame(struct blame_scoreboard *sb, int opt) ;
>
> Lose the extra SP before ";".  I had to fix them in your previous patch
> and there were many.
>

Re: [PATCH1/2] Libify blame

[Junio C Hamano]

pi song <pi.songs@gmail.com> writes:

> Don't you think we should rather split up into smaller files before
> start reorganizing things?

Yes, but splitting it wrong is, eh, wrong ;-)

Re: [PATCH1/2] Libify blame

[pi song]

Wait. If you look at the builtin-blame.c, out of question it is very
messy. Things like print_usage() or -L parameter parsing for example
is not done upfront but hiding somewhere. Some functions are not very
clear if they are frontend or backend. I would say nobody would be
able to split it right in the first place. What you could do is to
split it to something "roughly right" and then work from that.

My latest two patches really do nothing but just splitting files. I
haven't changed any logics or renamed any thing only to make this big
beast more *manageable* rather than tackling the problem directly.
Yes, some bits are  still wrong but I believe 70% of the functions
should already stay in the right place. The following patches will
make the structure more right *gradually*.

Pi Song


On Wed, Mar 18, 2009 at 5:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
> pi song <pi.songs@gmail.com> writes:
>
>> Don't you think we should rather split up into smaller files before
>> start reorganizing things?
>
> Yes, but splitting it wrong is, eh, wrong ;-)
>

Re: [PATCH1/2] Libify blame

[pi song]

BTW, following patches are not available yet : P

On Wed, Mar 18, 2009 at 5:52 PM, pi song <pi.songs@gmail.com> wrote:
> Wait. If you look at the builtin-blame.c, out of question it is very
> messy. Things like print_usage() or -L parameter parsing for example
> is not done upfront but hiding somewhere. Some functions are not very
> clear if they are frontend or backend. I would say nobody would be
> able to split it right in the first place. What you could do is to
> split it to something "roughly right" and then work from that.
>
> My latest two patches really do nothing but just splitting files. I
> haven't changed any logics or renamed any thing only to make this big
> beast more *manageable* rather than tackling the problem directly.
> Yes, some bits are  still wrong but I believe 70% of the functions
> should already stay in the right place. The following patches will
> make the structure more right *gradually*.
>
> Pi Song
>
>
> On Wed, Mar 18, 2009 at 5:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> pi song <pi.songs@gmail.com> writes:
>>
>>> Don't you think we should rather split up into smaller files before
>>> start reorganizing things?
>>
>> Yes, but splitting it wrong is, eh, wrong ;-)
>>
>