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

From: Junio C Hamano <gitster@pobox.com>
Date: May 14, 2017, 08:10 UTC
Message-ID: <xmqq8tlz504w.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170514031513.9042-5-whydoubt@gmail.com>

Jeff Smith <whydoubt@gmail.com> writes:

Show 36 quoted lines

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