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@xxxxxxxxx>
Sent: 01/07/2015 01:09 PM
To: kelson@xxxxxxxxxxxxxxx
CC: Git Mailing List <git@xxxxxxxxxxxxxxx>, Philip Oakley
<philipoakley@xxxxxxx>, Duy Nguyen <pclouds@xxxxxxxxx>, Jonathan
Nieder <jrnieder@xxxxxxxxx>
Subject: Re: [PATCH 1/2] support for --no-relative and diff.relative
kelson@xxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxx>
Please also have
From: Brandon Phillips <kelson@xxxxxxxxxxxxxxx>
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).
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.
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.
--
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