Re: [PATCH1/2] Libify blame

[pi song <pi.songs@gmail.com>]

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