On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller <jacob.e.keller@xxxxxxxxx> wrote:
> From: Jacob Keller <jacob.keller@xxxxxxxxx>
>
> A future patch is going to add a new submodule diff format which
> displays an inline diff of the submodule changes. To make this easier,
> and to ensure that both submodule diff formats use the same initial
> header, factor out show_submodule_header() function which will print the
> current submodule header line, and then leave the show_submodule_summary
> function to lookup and print the submodule log format.
>
> This does create one format change in that "(revision walker failed)"
> will now be displayed on its own line rather than as part of the message
> because we no longer perform this step directly in the header display
> flow. However, this is a rare case and shouldn't impact much. In
> addition, since we no longer need to run prepare_submodule_summary to
> get the fast_backward and fast_forward variables, these have been
> removed from that call, and the show_submodule_header() function does
> its own mergebase lookup.
>
> Signed-off-by: Jacob Keller <jacob.keller@xxxxxxxxx>
> ---
> submodule.c | 103 +++++++++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 74 insertions(+), 29 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index e1a51b7506ff..e341ca7ffefd 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -277,8 +277,7 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt,
> }
>
> static int prepare_submodule_summary(struct rev_info *rev, const char *path,
> - struct commit *left, struct commit *right,
> - int *fast_forward, int *fast_backward)
> + struct commit *left, struct commit *right)
> {
> struct commit_list *merge_bases, *list;
>
> @@ -290,12 +289,6 @@ static int prepare_submodule_summary(struct rev_info *rev, const char *path,
> add_pending_object(rev, &left->object, path);
> add_pending_object(rev, &right->object, path);
> merge_bases = get_merge_bases(left, right);
> - if (merge_bases) {
> - if (merge_bases->item == left)
> - *fast_forward = 1;
> - else if (merge_bases->item == right)
> - *fast_backward = 1;
> - }
> for (list = merge_bases; list; list = list->next) {
> list->item->object.flags |= UNINTERESTING;
> add_pending_object(rev, &list->item->object,
> @@ -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)
> {
> - struct rev_info rev;
> - struct commit *left = NULL, *right = NULL;
> + struct commit_list *merge_bases;
> const char *message = NULL;
> struct strbuf sb = STRBUF_INIT;
> int fast_forward = 0, fast_backward = 0;
>
> - if (is_null_sha1(two))
> - message = "(submodule deleted)";
> - else if (add_submodule_odb(path))
> - message = "(not initialized)";
> - else if (is_null_sha1(one))
> - message = "(new submodule)";
> - else if (!(left = lookup_commit_reference(one)) ||
> - !(right = lookup_commit_reference(two)))
> - message = "(commits not present)";
> - else if (prepare_submodule_summary(&rev, path, left, right,
> - &fast_forward, &fast_backward))
> - message = "(revision walker failed)";
> -
> if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
> fprintf(f, "%sSubmodule %s contains untracked content\n",
> line_prefix, path);
> @@ -365,11 +350,46 @@ void show_submodule_summary(FILE *f, const char *path,
> fprintf(f, "%sSubmodule %s contains modified content\n",
> line_prefix, path);
>
> + if (is_null_sha1(one))
> + message = "(new submodule)";
> + else if (is_null_sha1(two))
> + message = "(submodule deleted)";
> +
> + if (add_submodule_odb(path)) {
> + if (!message)
> + message = "(submodule not initialized)";
Before it was "(not initialized)" for this case, I think we'd rather keep that?
Though this code path is only used by the porcelain commands, we'd rather not
want to change this in a subtle way in this commit.
If we were to change those, we could discuss if we want to go with
full sentences
all the time:
submodule is new
submodule is deleted
submodule is not initialized
> + goto output_header;
> + }
> +
> + /*
> + * Attempt to lookup the commit references, and determine if this is
> + * a fast forward or fast backwards update
nit: end the sentence with a period
> + */
> + *left = lookup_commit_reference(one);
> + *right = lookup_commit_reference(two);
> +
> + /*
> + * Warn about missing commits in the submodule project, but only if
> + * they aren't null
nit: end the sentence with a period
> +
> +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)
> +{
> + struct rev_info rev;
> + struct commit *left = NULL, *right = NULL;
> +
> + show_submodule_header(f, path, line_prefix, one, two, dirty_submodule,
> + meta, reset, &left, &right);
> +
> + /*
> + * if we don't have both a left and a right pointer, then we can't
> + * display a summary
> + */
nit: Start the sentence with capital letters and end the sentence with a period.
Do we need to do another thing here before the return? (Maybe that could
go into the comment as well?)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html