Re: [PATCH 1/2] support for --no-relative and diff.relative

From: kelson@shysecurity.com <kelson@shysecurity.com>
Date: Jan 7, 2015, 18:46 UTC
Message-ID: <54AD7F1A.3060500@shysecurity.com>
In-Reply-To: <xmqqiogijwdp.fsf@gitster.dls.corp.google.com>

>> Content-Type: text/plain; charset=utf-8; format=flowed
>Please.  No format=flawed.  Really.

I'll figure out the line-wrapping.

> Also this step is not about --no-relative and diff.relative but is only about --no-relative option.

Should I submit as two independent patches then? I took the approach of splitting them out into 1/2 vs 2/2 to distinguish, but it sounds like that isn't optimal.

> When "--relative" is given, options->prefix is set to something as we can see above.
>
> The --relative option is described as optionally taking <path> in the doc:

...

> Doesn't "--no-relative" codepath have to undo the effect of that assignment to options->prefix?

diff_setup_done NULLs options->prefix when DIFF_OPT_TST(options, RELATIVE_NAME) is cleared (--no-relative clears this option).

On review, this may be a bad approach though. Non-locality makes it harder to follow/understand and introduces a subtle bug. current: "git-diff --relative=path --no-relative --relative" == "git-diff --relative=path" expected: "git-diff --relative=path --no-relative --relative" == "git-diff --relative"

> So I can agree with the design decision but only after spending 6
> lines to think about it.  For the end-users, this design decision
> needs to be explained and spelled out in the documentation.

Agreed; update to come.

-----Original Message-----
From: Junio C Hamano <gitster@pobox.com>
Sent: 01/07/2015 01:09 PM
To: kelson@shysecurity.com
CC: Git Mailing List <git@vger.kernel.org>,  Philip Oakley 
<philipoakley@iee.org>,  Duy Nguyen <pclouds@gmail.com>,  Jonathan 
Nieder <jrnieder@gmail.com>
Subject: Re: [PATCH 1/2] support for --no-relative and diff.relative

> kelson@shysecurity.com writes:

> Content-Type: text/plain; charset=utf-8; format=flowed

Please.  No format=flawed.  Really.

> Subject: Re: [PATCH 1/2] support for --no-relative and diff.relative

"diff: teach --no-relative to override earlier --relative" or something. Saying the affeced area upfront, terminated with a colon ':', will make it easier to spot in "git shortlog" output later.

Also this step is not about --no-relative and diff.relative but is only about --no-relative option.

> added --no-relative option for git-diff (code, documentation, and tests)

"Add --no-relative option..."; we write in imperative, as if we are giving an order to the project secretary to "make the code do/be so".

> --no-relative overrides --relative causing a return to standard behavior

OK (modulo missing full-stop).

>
> Signed-off-by: Brandon Phillips <kelson@shysecurity.com>

Please also have

	From: Brandon Phillips <kelson@shysecurity.com>

as the first line of the body of your e-mail message, if you are letting your MUA only give your e-mail address without name. Alternatively, please ask/configure your MUA to put your name as well as your address on From: header of the e-mail (which is preferrable).

Show 6 quoted lines

> diff --git a/diff.c b/diff.c
> index d1bd534..7bceba8 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3695,6 +3695,8 @@ int diff_opt_parse(struct diff_options *options,
> const char **av, int ac)

Line-wrapped.

Show 6 quoted lines

>   		DIFF_OPT_SET(options, RELATIVE_NAME);
>   		options->prefix = arg;
>   	}
> +	else if (!strcmp(arg, "--no-relative"))
> +		DIFF_OPT_CLR(options, RELATIVE_NAME);
>

When "--relative" is given, options->prefix is set to something as we can see above.

The --relative option is described as optionally taking <path> in the doc:

   --relative[=<path>]::
          When run from a subdirectory of the project, it can be
          told to exclude changes outside the directory and show
          pathnames relative to it with this option.  When you are
          not in a subdirectory (e.g. in a bare repository), you
          can name which subdirectory to make the output relative
          to by giving a <path> as an argument.

Doesn't "--no-relative" codepath have to undo the effect of that assignment to options->prefix?

For example, after applying this patch, shouldn't

	$ cd t
	$ git show --relative=Documentation --no-relative --relative

work the same way as

	$ cd t
	$ git show --relative

i.e. limiting its output to the changes in the 't/' directory and not to the changes in the 'Documentation/' directory?

Patch 2/2 also seems to share similar line-wrapping breakages that make it unappliable, but more importantly, the configuration that is supposed to correspond to --relative option only parses a boolean. Is that the right design, or should it also be able to substitute a command line `--relative=<path>` with an argument?

The last was a half-way rhetorical question and my answer is that boolean-only is the best you could do, because we cannot do the usual "bool or string" trick when "string" can be arbitrary. In other words, "diff.relative=true" could mean "limit to the current subdirectory" aka "--relative" or it could mean "limit to true/ subdirectory" aka "--relative=true", and there is no good way to disambiguate between the two [*1*].

So I can agree with the design decision but only after spending 6 lines to think about it. For the end-users, this design decision needs to be explained and spelled out in the documentation. Saying "equivalent to `--relative`" is not sufficient, because the way `--relative` option itself is described elsewhere. The option appears as `--relative[=<path>]` (see above), so some people _will_ read "equivalent to `--relative`" to mean "Setting diff.relative=t should be equivalent to --relative=t", which is not what actually happens.

[Footnote]

*1* Actually, you could declare that "diff.relative=true/" means the
      'true/' directory while "diff.relative=true" means the boolean
      'true' aka 'diff --relative', but I think it is too confusing.
      Let's not make it worse by going that route.