Re: [PATCH v2 7/8] range-diff: passthrough --[no-]notes to `git log`

From: Denton Liu <liu.denton@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Thomas Gummerer <t.gummerer@gmail.com>
Subject: Re: [PATCH v2 7/8] range-diff: passthrough --[no-]notes to `git log`
Date: Wed, 20 Nov 2019 11:12:58 -0800	[thread overview]
Message-ID: <20191120191258.GA73969@generichostname> (raw)
In-Reply-To: <xmqqwobvb2cj.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Wed, Nov 20, 2019 at 01:26:04PM +0900, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > When a commit being range-diff'd has a note attached to it, the note
> > will be compared as well. However, if a user has multiple notes refs or
> > if they want to suppress notes from being printed, there is currently no
> > way to do this.
> >
> > Passthrough `---no--notes` to the `git log` call so that this option is
> 
> "`--[no-]notes`" or perhaps "`--[no-]notes` and `--notes=<ref>`"?

Whoops, I probably typed `ys-` in vim instead of `ys[` so I surrounded
the `no-` with the wrong characters.

> 
> I think the verb phrase is two words, "pass through", by the way.

Thanks, I didn't know this.

> 
> > +--[no-]notes[=<treeish>]::
> > +	This flag is passed to the `git log` program
> > +	(see linkgit:git-log[1]) that generates the patches.
> 
> I can see this was taken from "git log --help", and it probably
> needs fixing for consistency as well, but I think --notes=<ref>
> would be easier to click users' minds with notes.displayRef
> configuration variable.

I'll put in a cleanup patch for this as well.

> 
> > @@ -61,8 +62,11 @@ static int read_patches(const char *range, struct string_list *list)
> >  			"--output-indicator-new=>",
> >  			"--output-indicator-old=<",
> >  			"--output-indicator-context=#",
> > -			"--no-abbrev-commit", range,
> > +			"--no-abbrev-commit",
> >  			NULL);
> > +	if (other_arg)
> > +		argv_array_pushv(&cp.args, other_arg->argv);
> > +	argv_array_push(&cp.args, range);
> 
> Makes sense.
> 
> > diff --git a/range-diff.h b/range-diff.h
> > index 08a50b6e98..7d918ab9ed 100644
> > --- a/range-diff.h
> > +++ b/range-diff.h
> > @@ -2,6 +2,7 @@
> >  #define RANGE_DIFF_H
> >  
> >  #include "diff.h"
> > +#include "argv-array.h"
> >  
> >  #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60
> >  
> > @@ -12,6 +13,7 @@
> >   */
> >  int show_range_diff(const char *range1, const char *range2,
> >  		    int creation_factor, int dual_color,
> > -		    struct diff_options *diffopt);
> > +		    struct diff_options *diffopt,
> > +		    struct argv_array *other_arg);
> >  
> >  #endif
> 
> I thought a mere use of "pointer to a struct" did not have to bring
> the definition of the struct into the picture?  In other words,
> wouldn't it be fine to leave the other_arg a pointer to an opaque
> structure by not including "argv-array.h" in this file?

Without including "argv-array.h", we get the following hdr-check error:

	$ make range-diff.hco
	    HDR range-diff.h
	In file included from range-diff.hcc:2:
	./range-diff.h:16:14: error: declaration of 'struct argv_array' will not be visible outside of this function [-Werror,-Wvisibility]
			    struct argv_array *other_arg);
				   ^
	1 error generated.
	make: *** [range-diff.hco] Error 1

I am currently using this compiler for reference:

	$ clang --version
	Apple LLVM version 10.0.1 (clang-1001.0.46.4)
	Target: x86_64-apple-darwin18.7.0
	Thread model: posix
	InstalledDir: /Library/Developer/CommandLineTools/usr/bin

Thanks,

Denton

> 
> Thanks.