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

From: David Aguilar <davvid@gmail.com>
Date: Aug 18, 2016, 07:00 UTC
Message-ID: <20160818070023.GA30666@gmail.com>
In-Reply-To: <20160818005131.31600-7-jacob.e.keller@intel.com>

On Wed, Aug 17, 2016 at 05:51:30PM -0700, Jacob Keller wrote:

Show 18 quoted lines

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

Show 15 quoted lines

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