Re: [RFC PATCH 00/10] Add blame to libgit

From: Junio C Hamano <gitster@pobox.com>
Date: May 5, 2017, 17:54 UTC
Message-ID: <xmqqr3036vf1.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170505052729.7576-1-whydoubt@gmail.com>

Jeff Smith <whydoubt@gmail.com> writes:

Show 11 quoted lines

> Jeff Smith (10):
>   Remove unneeded dependency on blob.h from blame
>   Move textconv_object to be with other textconv methods
>   Add some missing definitions to header files
>   Remove unused parameter from get_origin()
>   Split blame origin into its own file
>   Move fake_working_tree_commit() to lib
>   Break out scoreboard a little better
>   Split blame scoreboard into its own file
>   Break out scoreboard init and setup
>   Move scoreboard init and setup to lib

Make sure these shortlog entries, when eventually appear along with other people's changes, let readers know what these are about easily, and formatted similarly to others'. Write the "<area>: " prefix in front is a good pratice to help that, e.g.

    blame: remove unneeded dependency on blob.h

In general I am OK with the idea of splitting builtin/blame.c into the blame Porcelain part (i.e. cmd_blame()) and some form of reusable "blame library".

However, you'd need to be cafreful about names. Are contents of "commit-fake", "origin" and "scoreboard" generally useful outside the context of running a "blame"? They are not good filenames if saying "origin.c" in the context of the entire Git codebase, anybody would understand that it is about "the origin" concept used by the blame engine (similarly for other new files). I would imagine that having blame.[ch] at the toplevel instead of creating that many "blame library" files would be a good way to go (mirrors how other parts of the system is laid out, e.g. diff.[ch] and diff-*.[ch] are library-ish files, while builtin/diff*.c implements the Porcelain).

Have you renamed the names of structures, functions and variables that used to be private to builtin/blame.c and now shared between builtin/blame.c and these new files so that they are specific enough and are obvious that they are about blame infrastructure? Names that were perfectly acceptable and appropriate as private ones inside a subsystem are often too vague or generic when exposed to outside world as part of the library-ish code.