Re: Diffing submodule does not yield complete logs for merge commits

[Heiko Voigt <hvoigt@hvoigt.net>]

Hi,

On Fri, May 15, 2015 at 03:33:07PM -0500, Robert Dailey wrote:
> On Tue, May 5, 2015 at 12:49 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > Hi Robert,
> >
> > On 2015-05-04 22:21, Robert Dailey wrote:
> >
> >> Since I am not a linux user, I have implemented this feature against
> >> the Git for Windows fork of git. I am not able to verify changes if I
> >> make them directly against the Git repository.
> >
> > That is why I worked so hard on support of Vagrant: https://github.com/msysgit/msysgit/wiki/Vagrant -- in short, it makes it dead easy for you to set up a *minimal* Linux VM inside your Git SDK and interact with it via ssh.
> >
> > With the Vagrant solution, you can easily test Linux Git even on Windows.
> >
> > Ciao,
> > Johannes
> 
> At the moment I have a "half-ass" patch attached. This implements the
> feature itself. I'm able to test this and it seems to be working.
> Please note I'm a C++ developer and straight C / Bash are not my
> strong suits. I apologize in advance for any mistakes. I am open to
> taking recommendations for corrections.

Please inline the patch, so people can easily comment. Have a look at
Documentation/SubmittingPatches and patches on this list for an example.
I have inlined your patch below for comments.

> I'm not sure how I can verify the feature in a unit test. In addition
> to learning bash scripting well enough to write the test, I am not
> sure how to use git to check for the additional commits. Plus the repo
> for the test will need to handle a submodule change to a merge commit
> as well. Any advice on setting up a good test case for this? What
> conditions should I check for, as far as log output goes?

The testsuite can be found in t/ the README there describes most of it.
Have a look at t4041-diff-submodule-option.sh and imitate the tests for
the existing log option. What they basically do is: Write a file with
the expected output of the diff and then compare the actual output with
it. That should also be possible for your option.

As for the merge commit: If there is no merge commit in the submodule
that is used for testing you can simply add a sequence of git commands
that manufactures the situation in the test repository as you need it.

'test_pause' is a helpful command to interactively debug/develop tests.
Run the test with the -v -i switches (maybe -d) when developing.

Comments for your patch please see below.

Cheers Heiko

> From: Robert Dailey <rcdailey@gmail.com>
> Subject: [PATCH] Add 'full-log' option to diff.submodule
> 
> Like the 'log' option to `diff --submodule`, 'full-log' provides
> logs without the `--first-parent` option.
> ---
>  diff.c      | 16 ++++++++++++----
>  diff.h      |  1 +
>  submodule.c |  9 +++++----
>  submodule.h |  3 ++-
>  4 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/diff.c b/diff.c
> index 7500c55..58c4872 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -128,10 +128,18 @@ static int parse_dirstat_params(struct diff_options *options, const char *params
>  
>  static int parse_submodule_params(struct diff_options *options, const char *value)
>  {
> -	if (!strcmp(value, "log"))
> +	if (!strcmp(value, "log")) {
>  		DIFF_OPT_SET(options, SUBMODULE_LOG);
> -	else if (!strcmp(value, "short"))
> +		DIFF_OPT_CLR(options, SUBMODULE_FULL_LOG);
> +	}
> +	else if (!strcmp(value, "full-log")) {
> +		DIFF_OPT_SET(options, SUBMODULE_FULL_LOG);
> +		DIFF_OPT_CLR(options, SUBMODULE_LOG);
> +	}
> +	else if (!strcmp(value, "short")) {
>  		DIFF_OPT_CLR(options, SUBMODULE_LOG);
> +		DIFF_OPT_CLR(options, SUBMODULE_FULL_LOG);
> +	}

Here I think clearing the bits first and then setting them would be
simpler and less error prone for further extensions. E.g. in the
beginning of the function:

	DIFF_OPT_CLR(options, SUBMODULE_LOG);
	DIFF_OPT_CLR(options, SUBMODULE_FULL_LOG);

and then

	if (!strcmp(value, "log"))
		DIFF_OPT_SET(options, SUBMODULE_LOG);
	else if (...


>  	else
>  		return -1;
>  	return 0;
> @@ -2240,7 +2248,7 @@ static void builtin_diff(const char *name_a,
>  	struct strbuf header = STRBUF_INIT;
>  	const char *line_prefix = diff_line_prefix(o);
>  
> -	if (DIFF_OPT_TST(o, SUBMODULE_LOG) &&
> +	if ((DIFF_OPT_TST(o, SUBMODULE_LOG) || DIFF_OPT_TST(o, SUBMODULE_FULL_LOG)) &&

Try to keep your line length less than 80 characters.
(Documentation/CodingGuidelines)

>  			(!one->mode || S_ISGITLINK(one->mode)) &&
>  			(!two->mode || S_ISGITLINK(two->mode))) {
>  		const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
> @@ -2248,7 +2256,7 @@ static void builtin_diff(const char *name_a,
>  		show_submodule_summary(o->file, one->path ? one->path : two->path,
>  				line_prefix,
>  				one->sha1, two->sha1, two->dirty_submodule,
> -				meta, del, add, reset);
> +				meta, del, add, reset, DIFF_OPT_TST(o, SUBMODULE_FULL_LOG));

Same as above.

>  		return;
>  	}
>  
> diff --git a/diff.h b/diff.h
> index b4a624d..95f319c 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -90,6 +90,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
>  #define DIFF_OPT_DIRSTAT_BY_LINE     (1 << 28)
>  #define DIFF_OPT_FUNCCONTEXT         (1 << 29)
>  #define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)
> +#define DIFF_OPT_SUBMODULE_FULL_LOG  (1 << 31)
>  
>  #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
>  #define DIFF_OPT_TOUCHED(opts, flag)    ((opts)->touched_flags & DIFF_OPT_##flag)
> diff --git a/submodule.c b/submodule.c
> index d37d400..f98173e 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -290,14 +290,14 @@ 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)
> +		int *fast_forward, int *fast_backward, unsigned full_log)
>  {
>  	struct commit_list *merge_bases, *list;
>  
>  	init_revisions(rev, NULL);
>  	setup_revisions(0, NULL, rev, NULL);
>  	rev->left_right = 1;
> -	rev->first_parent_only = 1;
> +	rev->first_parent_only = full_log ? 0 : 1;
>  	left->object.flags |= SYMMETRIC_LEFT;
>  	add_pending_object(rev, &left->object, path);
>  	add_pending_object(rev, &right->object, path);
> @@ -363,7 +363,8 @@ 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)
> +		const char *del, const char *add, const char *reset,
> +		unsigned full_log)
>  {
>  	struct rev_info rev;
>  	struct commit *left = NULL, *right = NULL;
> @@ -381,7 +382,7 @@ void show_submodule_summary(FILE *f, const char *path,
>  		 !(right = lookup_commit_reference(two)))
>  		message = "(commits not present)";
>  	else if (prepare_submodule_summary(&rev, path, left, right,
> -					   &fast_forward, &fast_backward))
> +					   &fast_forward, &fast_backward, full_log))

Line length.

>  		message = "(revision walker failed)";
>  
>  	if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
> diff --git a/submodule.h b/submodule.h
> index 7beec48..301358b 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -26,7 +26,8 @@ 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);
> +		const char *del, const char *add, const char *reset,
> +		unsigned full_log);
>  void set_config_fetch_recurse_submodules(int value);
>  void check_for_new_submodule_commits(unsigned char new_sha1[20]);
>  int fetch_populated_submodules(const struct argv_array *options,

Apart from the comments above, your patch looks good to me.

Cheers Heiko