git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Heiko Voigt <hvoigt@hvoigt.net>
Date
May 18, 2015, 12:30 UTC
Message-ID
<20150518123036.GB16841@book.hvoigt.net>
In-Reply-To
<CAHd499Do2aB5E_=aDzkoDssEbgz181rH36X28Oe7Zcok2f=zBQ@mail.gmail.com>
Hi,
On Fri, May 15, 2015 at 03:33:07PM -0500, Robert Dailey wrote:
Show 22 quoted lines
> 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.

Show 6 quoted lines
> 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
Show 34 quoted lines
> 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 (...
Show 9 quoted lines
>  	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)

Show 9 quoted lines
>  			(!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.
Show 52 quoted lines
>  		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.
Show 17 quoted lines
>  		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
Previous: Robert DaileyNext: Robert Dailey
Message 9 of 24 in “Diffing submodule does not yield complete logs for merge commits”
  1. Robert DaileyApr 29, 2015
  2. Heiko VoigtMay 1, 2015
  3. Robert DaileyMay 4, 2015
  4. Jens LehmannMay 4, 2015
  5. Robert DaileyMay 4, 2015
  6. Heiko VoigtMay 4, 2015
  7. Johannes SchindelinMay 5, 2015
  8. Robert DaileyMay 15, 2015
  9. Heiko VoigtMay 18, 2015
  10. Robert DaileyMay 18, 2015
  11. Heiko VoigtMay 19, 2015
  12. Robert DaileyMay 19, 2015
  13. Stefan BellerMay 19, 2015
  14. Roberto TyleyMay 22, 2015
  15. Heiko VoigtMay 21, 2015
  16. Robert DaileyMay 30, 2015
  17. Heiko VoigtMay 30, 2015
  18. Junio C HamanoMay 30, 2015
  19. Robert DaileyMay 30, 2015
  20. Robert DaileyMay 30, 2015
  21. Junio C HamanoMay 30, 2015
  22. Robert DaileyMay 30, 2015
  23. Heiko VoigtJun 2, 2015
  24. Junio C HamanoMay 4, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.