Re: [PATCH1/2] Libify blame

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 [off-list ref] wrote:

pi song [off-list ref] writes:

quoted

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).

quoted

+#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.

quoted

+/* 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.

quoted

+/*
+ * 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.

quoted

+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.