Re: [RFC PATCH v2 04/22] blame: move origin and entry structures to header

[Junio C Hamano <gitster@pobox.com>]

Jeff Smith <whydoubt@gmail.com> writes:

> The origin and blame_entry structures are core to the blame interface
> and reference each other. Since origin will be more exposed, rename it
> to blame_origin to clarify what it is a part of.
>
> Signed-off-by: Jeff Smith <whydoubt@gmail.com>
> ---
>  blame.h         |  86 ++++++++++++++++++++++++++
>  builtin/blame.c | 185 +++++++++++++++++---------------------------------------
>  2 files changed, 140 insertions(+), 131 deletions(-)
>  create mode 100644 blame.h
>
> diff --git a/blame.h b/blame.h
> new file mode 100644
> index 0000000..f52d0fc
> --- /dev/null
> +++ b/blame.h
> @@ -0,0 +1,86 @@
> +#ifndef BLAME_H
> +#define BLAME_H
> +
> +#include "cache.h"
> +#include "commit.h"
> +#include "xdiff-interface.h"
> +
> +/*
> + * One blob in a commit that is being suspected
> + */
> +struct blame_origin {
> +	int refcnt;
> +...
>   ...
> -/*
> - * One blob in a commit that is being suspected
> - */
> -struct origin {
> -	int refcnt;

I hate to say this AFTER you sent your second attempt, but could you
have another preparetory step before this patch, that does many
renames of symbols (e.g. s/origin/blame_origin/) and nothing else,
while the lines are still in builtin/blame.c?

To review a step like yoru 04/22 to make sure nothing else other
than moving the lines is going on, an easy way to do so is to run
"blame -C" on the resulting blame.h file---that lets the reviewers'
eyes coast over the lines that came from contiguous region of the
original builtin/blame.c and concentrate on things like what used to
be static to the file now getting extern.

If you rename symbols while moving lines across files, that becomes
impossible because many lines are now different (i.e. a line that
talked about "origin" in builtin/blame.c in the preimage may now
talk about "blame_origin" in blame.h, hence attributed to the
new commit's blame.h).

It may even be OK to do the "rename the symbols" step in a single
patch (e.g. your 04/22 and 05/22 are separate patches and the former
renames "origin" while the latter does "scoreboard"---I am saying
that the preparatory step that renames the symbols without
introducing the new "blame.h" may not have to have these two as
separate patches).

If rename of symbols are done first while the lines are still in the
original file, mechanical moving of lines gets much easier to
review.  Of course, a patch that only renames symbols is also much
easier to review, even if it is a large one.

Thanks.