Re: [PATCH v7 6/7] submodule: refactor show_submodule_summary with helper function

[David Aguilar <davvid@gmail.com>]

On Wed, Aug 17, 2016 at 05:51:30PM -0700, Jacob Keller wrote:
> [snip]
> @@ -333,31 +326,23 @@ static void print_submodule_summary(struct rev_info *rev, FILE *f,
>  	strbuf_release(&sb);
>  }
>  
> -void show_submodule_summary(FILE *f, const char *path,
> +/* Helper function to display the submodule header line prior to the full
> + * summary output. If it can locate the submodule objects directory it will
> + * attempt to lookup both the left and right commits and put them into the
> + * left and right pointers.
> + */
> +static void show_submodule_header(FILE *f, const char *path,
>  		const char *line_prefix,
>  		unsigned char one[20], unsigned char two[20],
>  		unsigned dirty_submodule, const char *meta,
> -		const char *del, const char *add, const char *reset)
> +		const char *reset,
> +		struct commit **left, struct commit **right)


Note the pre-existing unsigned char[20]s in the signature above...


> [snip]
> @@ -381,14 +401,39 @@ void show_submodule_summary(FILE *f, const char *path,
>  		strbuf_addf(&sb, "%s:%s\n", fast_backward ? " (rewind)" : "", reset);
>  	fwrite(sb.buf, sb.len, 1, f);
>  
> -	if (!message) /* only NULL if we succeeded in setting up the walk */
> -		print_submodule_summary(&rev, f, line_prefix, del, add, reset);
> +	strbuf_release(&sb);
> +}
> +
> +void show_submodule_summary(FILE *f, const char *path,
> +		const char *line_prefix,
> +		unsigned char one[20], unsigned char two[20],
> +		unsigned dirty_submodule, const char *meta,
> +		const char *del, const char *add, const char *reset)


... and here too.

There's an ongoing effort to replace unsigned char sha1[20]
with struct object_id.  It might make sense to pass "one" and
"two" as "unsigned char*" instead of hard-coding the SHA1-length
here.  Alternatively, we could pass in the struct object_id*
instead.

The [20] in the show_submodule_header() function above is
pre-existing and not added by this patch, but that function's
signature is touched by this patch so it might make sense to
tidy that up while we're in here, possibly as a separate patch.
-- 
David