[PATCH 0/2] minor diff orderfile documentation improvements

[PATCH 0/2] minor diff orderfile documentation improvements

[Richard Hansen]

[-- Attachment #1: Type: text/plain, Size: 295 bytes --]

Richard Hansen (2):
  diff: document behavior of relative diff.orderFile
  diff: document the pattern format for diff.orderFile

 Documentation/diff-config.txt  | 5 ++++-
 Documentation/diff-options.txt | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

[PATCH 1/2] diff: document behavior of relative diff.orderFile

[Richard Hansen]

[-- Attachment #1: Type: text/plain, Size: 790 bytes --]

Document that a relative pathname for diff.orderFile is interpreted as
relative to the top-level work directory.

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 Documentation/diff-config.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 58f4bd6af..875212045 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -101,6 +101,8 @@ diff.noprefix::
 diff.orderFile::
 	File indicating how to order files within a diff, using
 	one shell glob pattern per line.
+	If `diff.orderFile` is a relative pathname, it is treated as
+	relative to the top of the work tree.
 	Can be overridden by the '-O' option to linkgit:git-diff[1].
 
 diff.renameLimit::
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

[PATCH 2/2] diff: document the pattern format for diff.orderFile

[Richard Hansen]

[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]

Document the format of the patterns used for the diff.orderFile
setting and diff's '-O' option by referring the reader to the
gitignore[5] page.

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 Documentation/diff-config.txt  | 3 ++-
 Documentation/diff-options.txt | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 875212045..a35ecdd6b 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -100,7 +100,8 @@ diff.noprefix::
 
 diff.orderFile::
 	File indicating how to order files within a diff, using
-	one shell glob pattern per line.
+	one glob pattern per line.
+	See linkgit:gitignore[5] for the pattern format.
 	If `diff.orderFile` is a relative pathname, it is treated as
 	relative to the top of the work tree.
 	Can be overridden by the '-O' option to linkgit:git-diff[1].
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index e6215c372..dc6b1af71 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -467,7 +467,8 @@ endif::git-format-patch[]
 
 -O<orderfile>::
 	Output the patch in the order specified in the
-	<orderfile>, which has one shell glob pattern per line.
+	<orderfile>, which has one glob pattern per line.
+	See linkgit:gitignore[5] for the pattern format.
 	This overrides the `diff.orderFile` configuration variable
 	(see linkgit:git-config[1]).  To cancel `diff.orderFile`,
 	use `-O/dev/null`.
-- 
2.11.0.390.gc69c2f50cf-goog


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile

[Jeff King]

On Mon, Jan 09, 2017 at 07:40:30PM -0500, Richard Hansen wrote:

> Document that a relative pathname for diff.orderFile is interpreted as
> relative to the top-level work directory.
> 
> Signed-off-by: Richard Hansen <hansenr@google.com>
> ---
>  Documentation/diff-config.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
> index 58f4bd6af..875212045 100644
> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -101,6 +101,8 @@ diff.noprefix::
>  diff.orderFile::
>  	File indicating how to order files within a diff, using
>  	one shell glob pattern per line.
> +	If `diff.orderFile` is a relative pathname, it is treated as
> +	relative to the top of the work tree.
>  	Can be overridden by the '-O' option to linkgit:git-diff[1].

What happens in a bare repository?

I'm guessing it's relative to the top-level of the repository, but we
should probably spell it out.

The other case is --no-index when we are not in a repository at all, but
that should just be relative to the current directory, which isn't
really worth mentioning.

-Peff

Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile

[Richard Hansen]

[-- Attachment #1: Type: text/plain, Size: 1743 bytes --]

On 2017-01-10 01:58, Jeff King wrote:
> On Mon, Jan 09, 2017 at 07:40:30PM -0500, Richard Hansen wrote:
>
>> Document that a relative pathname for diff.orderFile is interpreted as
>> relative to the top-level work directory.
>>
>> Signed-off-by: Richard Hansen <hansenr@google.com>
>> ---
>>  Documentation/diff-config.txt | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
>> index 58f4bd6af..875212045 100644
>> --- a/Documentation/diff-config.txt
>> +++ b/Documentation/diff-config.txt
>> @@ -101,6 +101,8 @@ diff.noprefix::
>>  diff.orderFile::
>>  	File indicating how to order files within a diff, using
>>  	one shell glob pattern per line.
>> +	If `diff.orderFile` is a relative pathname, it is treated as
>> +	relative to the top of the work tree.
>>  	Can be overridden by the '-O' option to linkgit:git-diff[1].
>
> What happens in a bare repository?

I didn't know which is why this patch is silent on that topic.  :)

>
> I'm guessing it's relative to the top-level of the repository,

I just tried it out and it's relative to $PWD.

> but we should probably spell it out.

Do we want it to be relative to $PWD?  I think relative to $GIT_DIR is 
more useful.  If we want it to be relative to $GIT_DIR, then I think we 
should stay silent regarding bare repositories so that the behavior 
remains unspecified until we update the logic.

>
> The other case is --no-index when we are not in a repository at all, but
> that should just be relative to the current directory,

It is.

> which isn't really worth mentioning.

Agreed.

I'll post a re-roll if people prefer the current behavior over relative 
to $GIT_DIR.

Thanks for reviewing,
Richard

>
> -Peff


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4845 bytes --]

Re: [PATCH 2/2] diff: document the pattern format for diff.orderFile

[Junio C Hamano]

Richard Hansen <hansenr@google.com> writes:

> Document the format of the patterns used for the diff.orderFile
> setting and diff's '-O' option by referring the reader to the
> gitignore[5] page.
>
> Signed-off-by: Richard Hansen <hansenr@google.com>
> ---
>  Documentation/diff-config.txt  | 3 ++-
>  Documentation/diff-options.txt | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
> index 875212045..a35ecdd6b 100644
> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -100,7 +100,8 @@ diff.noprefix::
>  
>  diff.orderFile::
>  	File indicating how to order files within a diff, using
> -	one shell glob pattern per line.
> +	one glob pattern per line.
> +	See linkgit:gitignore[5] for the pattern format.


I do not think it is wise to suggest referring to gitignore, as the
logic of matching is quite different, other than the fact that they
both use wildmatch() internally.  Also, unlike gitignore, orderfile
does not allow any negative matching i.e. "!<pattern>".

>  	If `diff.orderFile` is a relative pathname, it is treated as
>  	relative to the top of the work tree.
>  	Can be overridden by the '-O' option to linkgit:git-diff[1].
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index e6215c372..dc6b1af71 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -467,7 +467,8 @@ endif::git-format-patch[]
>  
>  -O<orderfile>::
>  	Output the patch in the order specified in the
> -	<orderfile>, which has one shell glob pattern per line.
> +	<orderfile>, which has one glob pattern per line.
> +	See linkgit:gitignore[5] for the pattern format.
>  	This overrides the `diff.orderFile` configuration variable
>  	(see linkgit:git-config[1]).  To cancel `diff.orderFile`,
>  	use `-O/dev/null`.

Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile

[Junio C Hamano]

Richard Hansen <hansenr@google.com> writes:

> On 2017-01-10 01:58, Jeff King wrote:
> ...
>> What happens in a bare repository?
>>
>> I'm guessing it's relative to the top-level of the repository,
>
> I just tried it out and it's relative to $PWD.

That is understandable.  When the user says

    $ cmd -O $file

with any option -O that takes a filename, it is most natural if we
used $PWD/$file when $file is not absolute path.

Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Richard Hansen <hansenr@google.com> writes:
>
>> On 2017-01-10 01:58, Jeff King wrote:
>> ...
>>> What happens in a bare repository?
>>>
>>> I'm guessing it's relative to the top-level of the repository,
>>
>> I just tried it out and it's relative to $PWD.
>
> That is understandable.  When the user says
>
>     $ cmd -O $file
>
> with any option -O that takes a filename, it is most natural if we
> used $PWD/$file when $file is not absolute path.

Ahh, ignore me in the above.  The discussion is about the
configuration variable, and I agree that being relative to GIT_DIR
would have made more sense.  In fact taking it as relative to PWD
does not make any sense.

We should have been a lot more careful when we added 6d8940b562
("diff: add diff.orderfile configuration variable", 2013-12-18), but
it is too late to complain now.

A related tangent.  

I wonder if anything that uses git_config_pathname() should be
relative to GIT_DIR when it is not absolute.

Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile

[Richard Hansen]

On 2017-01-10 15:23, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Richard Hansen <hansenr@google.com> writes:
>>
>>> On 2017-01-10 01:58, Jeff King wrote:
>>> ...
>>>> What happens in a bare repository?
>>>>
>>>> I'm guessing it's relative to the top-level of the repository,
>>>
>>> I just tried it out and it's relative to $PWD.
>>
>> That is understandable.  When the user says
>>
>>     $ cmd -O $file
>>
>> with any option -O that takes a filename, it is most natural if we
>> used $PWD/$file when $file is not absolute path.
>
> Ahh, ignore me in the above.  The discussion is about the
> configuration variable, and I agree that being relative to GIT_DIR
> would have made more sense.  In fact taking it as relative to PWD
> does not make any sense.

I'll stay silent regarding bare repositories then.

>
> We should have been a lot more careful when we added 6d8940b562
> ("diff: add diff.orderfile configuration variable", 2013-12-18), but
> it is too late to complain now.
>
> A related tangent.
>
> I wonder if anything that uses git_config_pathname() should be
> relative to GIT_DIR when it is not absolute.

I think so.  (For bare repositories anyway; non-bare should be relative 
to GIT_WORK_TREE.)  Perhaps git_config_pathname() itself should convert 
relative paths to absolute so that every pathname setting automatically 
works without changing any calling code.

-Richard

Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile

[Junio C Hamano]

Richard Hansen <hansenr@google.com> writes:

>> A related tangent.
>>
>> I wonder if anything that uses git_config_pathname() should be
>> relative to GIT_DIR when it is not absolute.
>
> I think so.  (For bare repositories anyway; non-bare should be
> relative to GIT_WORK_TREE.)  Perhaps git_config_pathname() itself
> should convert relative paths to absolute so that every pathname
> setting automatically works without changing any calling code.

Yes, that was what I was alluding to.  We might have to wait until
major version boundary to do so, but I think that it is the sensible
way forward in the longer term to convert relative to absolute in
git_config_pathname().

Thanks.

Re: [PATCH 2/2] diff: document the pattern format for diff.orderFile

[Richard Hansen]

On 2017-01-10 15:14, Junio C Hamano wrote:
> Richard Hansen <hansenr@google.com> writes:
>
>> Document the format of the patterns used for the diff.orderFile
>> setting and diff's '-O' option by referring the reader to the
>> gitignore[5] page.
>>
>> Signed-off-by: Richard Hansen <hansenr@google.com>
>> ---
>>  Documentation/diff-config.txt  | 3 ++-
>>  Documentation/diff-options.txt | 3 ++-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
>> index 875212045..a35ecdd6b 100644
>> --- a/Documentation/diff-config.txt
>> +++ b/Documentation/diff-config.txt
>> @@ -100,7 +100,8 @@ diff.noprefix::
>>
>>  diff.orderFile::
>>  	File indicating how to order files within a diff, using
>> -	one shell glob pattern per line.
>> +	one glob pattern per line.
>> +	See linkgit:gitignore[5] for the pattern format.
>
>
> I do not think it is wise to suggest referring to gitignore, as the
> logic of matching is quite different, other than the fact that they
> both use wildmatch() internally.  Also, unlike gitignore, orderfile
> does not allow any negative matching i.e. "!<pattern>".

I was looking at the code to see how the two file formats differed and 
noticed that match_order() doesn't set the WM_PATHNAME flag when it 
calls wildmatch().  That's unintentional (a bug), right?

-Richard


>
>>  	If `diff.orderFile` is a relative pathname, it is treated as
>>  	relative to the top of the work tree.
>>  	Can be overridden by the '-O' option to linkgit:git-diff[1].
>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
>> index e6215c372..dc6b1af71 100644
>> --- a/Documentation/diff-options.txt
>> +++ b/Documentation/diff-options.txt
>> @@ -467,7 +467,8 @@ endif::git-format-patch[]
>>
>>  -O<orderfile>::
>>  	Output the patch in the order specified in the
>> -	<orderfile>, which has one shell glob pattern per line.
>> +	<orderfile>, which has one glob pattern per line.
>> +	See linkgit:gitignore[5] for the pattern format.
>>  	This overrides the `diff.orderFile` configuration variable
>>  	(see linkgit:git-config[1]).  To cancel `diff.orderFile`,
>>  	use `-O/dev/null`.

[PATCH v2 0/2] diff orderfile documentation improvements

[Richard Hansen]

Changes from v1:
  * Don't reference gitignore for the file format because they're not
    quite the same.

Richard Hansen (2):
  diff: document behavior of relative diff.orderFile
  diff: document the format of the -O (diff.orderFile) file

 Documentation/diff-config.txt  |  7 +++---
 Documentation/diff-options.txt | 54 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 56 insertions(+), 5 deletions(-)

-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v2 1/2] diff: document behavior of relative diff.orderFile

[Richard Hansen]

Document that a relative pathname for diff.orderFile is interpreted as
relative to the top-level work directory.

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 Documentation/diff-config.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 58f4bd6af..875212045 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -101,6 +101,8 @@ diff.noprefix::
 diff.orderFile::
 	File indicating how to order files within a diff, using
 	one shell glob pattern per line.
+	If `diff.orderFile` is a relative pathname, it is treated as
+	relative to the top of the work tree.
 	Can be overridden by the '-O' option to linkgit:git-diff[1].
 
 diff.renameLimit::
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v2 2/2] diff: document the format of the -O (diff.orderFile) file

[Richard Hansen]

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 Documentation/diff-config.txt  |  5 ++--
 Documentation/diff-options.txt | 54 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 875212045..9e4111320 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -99,11 +99,10 @@ diff.noprefix::
 	If set, 'git diff' does not show any source or destination prefix.
 
 diff.orderFile::
-	File indicating how to order files within a diff, using
-	one shell glob pattern per line.
+	File indicating how to order files within a diff.
+	See the '-O' option to linkgit:git-diff[1] for details.
 	If `diff.orderFile` is a relative pathname, it is treated as
 	relative to the top of the work tree.
-	Can be overridden by the '-O' option to linkgit:git-diff[1].
 
 diff.renameLimit::
 	The number of files to consider when performing the copy/rename
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index e6215c372..e57e9f810 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -466,11 +466,61 @@ information.
 endif::git-format-patch[]
 
 -O<orderfile>::
-	Output the patch in the order specified in the
-	<orderfile>, which has one shell glob pattern per line.
+	Control the order in which files appear in the output.
 	This overrides the `diff.orderFile` configuration variable
 	(see linkgit:git-config[1]).  To cancel `diff.orderFile`,
 	use `-O/dev/null`.
++
+The output order is determined by the order of glob patterns in
+<orderfile>.
+All files with pathnames that match the first pattern are output
+first, all files with pathnames that match the second pattern (but not
+the first) are output next, and so on.
+All files with pathnames that do not match any pattern are output
+last, as if there was an implicit match-all pattern at the end of the
+file.
+If multiple pathnames have the same rank, their output order relative
+to each other is the normal order.
++
+<orderfile> is parsed as follows:
++
+--
+ - Blank lines are ignored, so they can be used as separators for
+   readability.
+
+ - Lines starting with a hash ("`#`") are ignored, so they can be used
+   for comments.  Add a backslash ("`\`") to the beginning of the
+   pattern if it starts with a hash.
+
+ - Each other line contains a single pattern.
+--
++
+Patterns have the same syntax and semantics as patterns used for
+fnmantch(3) with the FNM_PATHNAME flag, except multiple consecutive
+unescaped asterisks (e.g., "`**`") have a special meaning:
++
+--
+ - A pattern beginning with "`**/`" means match in all directories.
+   For example, "`**/foo`" matches filename "`foo`" anywhere, and
+   "`**/foo/bar`" matches filename "`bar`" anywhere that is directly
+   under directory "`foo`".
+
+ - A pattern ending with "`/**`" matches everything inside a
+   directory, with infinite depth.  For example, "`abc/**`" matches
+   "`abc/def/ghi`" but not "`foo/abc/def`".
+
+ - A slash followed by two consecutive asterisks then a slash
+   ("`/**/`") matches zero or more directory components.  For example,
+   "`a/**/b`" matches "`a/b`", "`a/x/b`", "`a/x/y/b`" and so on.
+
+ - A pattern with more than one consecutive unescaped asterisk is
+   invalid.
+--
++
+In addition, a pathname matches a pattern if the pathname with any
+number of its final pathname components removed matches the pattern.
+For example, the pattern "`foo/*bar`" matches "`foo/asdfbar`" and
+"`foo/bar/baz`" but not "`foo/barx`".
 
 ifndef::git-format-patch[]
 -R::
-- 
2.11.0.390.gc69c2f50cf-goog

Re: [PATCH 2/2] diff: document the pattern format for diff.orderFile

[Junio C Hamano]

Richard Hansen <hansenr@google.com> writes:

> I was looking at the code to see how the two file formats differed and
> noticed that match_order() doesn't set the WM_PATHNAME flag when it
> calls wildmatch().  That's unintentional (a bug), right?

It has been that way from day one IIRC even before we introduced
wildmatch()---IOW it may be intentional that the current code that
uses wildmatch() does not use WM_PATHNAME.

Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile

[Jeff King]

On Tue, Jan 10, 2017 at 02:15:11PM -0800, Junio C Hamano wrote:

> Richard Hansen <hansenr@google.com> writes:
> 
> >> A related tangent.
> >>
> >> I wonder if anything that uses git_config_pathname() should be
> >> relative to GIT_DIR when it is not absolute.
> >
> > I think so.  (For bare repositories anyway; non-bare should be
> > relative to GIT_WORK_TREE.)  Perhaps git_config_pathname() itself
> > should convert relative paths to absolute so that every pathname
> > setting automatically works without changing any calling code.
> 
> Yes, that was what I was alluding to.  We might have to wait until
> major version boundary to do so, but I think that it is the sensible
> way forward in the longer term to convert relative to absolute in
> git_config_pathname().

Yeah, I'd agree.

I'm undecided on whether it would need to happen at a major version
bump. The existing semantics are fairly insane, and would cause a lot of
confusing breakages. We can imagine use of relative paths in a bare
repository falls into one of a few categories:

  1. The user generally runs by "cd /path/to/bare.git && git ...". This
     would be unaffected, as relative and $GIT_DIR are the same.

  2. The user runs via "cd /path/to/bare.git/some-subdir". This would be
     broken, but I have trouble imagining that they really wanted to
     read something like "objects/orderfile".

  3. The user runs via "GIT_DIR=/path/to/bare.git" from various
     directories. This case is probably horribly broken, as things like
     diff.orderFile will complain if they ever run from a directory that
     doesn't have the order file.

  4. They run GIT_DIR=/path/to/bare.git from a consistent origin
     directory. This _does_ work, and we'd be breaking it. Though I kind
     of question why the config in $GIT_DIR is meant to apply to a file
     in a totally unrelated directory.

     I suppose somebody could be relying on the behavior where setting
     GIT_DIR uses the current directory as the working tree (i.e., if
     core.bare is "true" in bare.git). But then, we'd consider their
     working directory as the working tree and read from that anyway. So
     the behavior would stay the same.

So I dunno. I do hate to break even corner cases, but I'm having trouble
imagining the scenario where somebody is actually using the current
behavior in a useful way.

-Peff

Re: [PATCH 2/2] diff: document the pattern format for diff.orderFile

[Richard Hansen]

On 2017-01-10 21:46, Junio C Hamano wrote:
> Richard Hansen <hansenr@google.com> writes:
>
>> I was looking at the code to see how the two file formats differed and
>> noticed that match_order() doesn't set the WM_PATHNAME flag when it
>> calls wildmatch().  That's unintentional (a bug), right?
>
> It has been that way from day one IIRC even before we introduced
> wildmatch()---IOW it may be intentional that the current code that
> uses wildmatch() does not use WM_PATHNAME.

You are the original author (af5323e027 2005-05-30).  Do you remember 
what your intention was?

I would like to change it to pass WM_PATHNAME, but I'm not sure if that 
would break anyone.  I'm guessing probably not, because users probably 
expect WM_PATHNAME and would be surprised (like I was) to learn otherwise.

If we want to keep it as-is, I'll have to adjust [PATCH v2 2/2].

-Richard

Re: [PATCH 2/2] diff: document the pattern format for diff.orderFile

[Junio C Hamano]

Richard Hansen <hansenr@google.com> writes:

> On 2017-01-10 21:46, Junio C Hamano wrote:
>> Richard Hansen <hansenr@google.com> writes:
>>
>>> I was looking at the code to see how the two file formats differed and
>>> noticed that match_order() doesn't set the WM_PATHNAME flag when it
>>> calls wildmatch().  That's unintentional (a bug), right?
>>
>> It has been that way from day one IIRC even before we introduced
>> wildmatch()---IOW it may be intentional that the current code that
>> uses wildmatch() does not use WM_PATHNAME.
>
> You are the original author (af5323e027 2005-05-30).  Do you remember
> what your intention was?

Yes.  

Back then we didn't even have wildmatch(), and used fnmatch()
instead, so forcing FNM_PATHNAME would have meant that people
wouldn't be able to say "foo*bar" to match "foo/other/bar"; with
wildmatch, "foo**bar" lets you defeat WM_PATHNAME so having
WM_PATHNAME always in effect is less of an issue, but with
fnmatch(), having FNM_PATHNAME always in effect has a lot of
downside.

I'd expect that orderfile people have today will be broken and
require tweaking if you switched WM_PATHNAME on.

Re: [PATCH 2/2] diff: document the pattern format for diff.orderFile

[Richard Hansen]

On 2017-01-11 13:15, Junio C Hamano wrote:
> Richard Hansen <hansenr@google.com> writes:
>
>> On 2017-01-10 21:46, Junio C Hamano wrote:
>>> Richard Hansen <hansenr@google.com> writes:
>>>
>>>> I was looking at the code to see how the two file formats differed and
>>>> noticed that match_order() doesn't set the WM_PATHNAME flag when it
>>>> calls wildmatch().  That's unintentional (a bug), right?
>>>
>>> It has been that way from day one IIRC even before we introduced
>>> wildmatch()---IOW it may be intentional that the current code that
>>> uses wildmatch() does not use WM_PATHNAME.
>>
>> You are the original author (af5323e027 2005-05-30).  Do you remember
>> what your intention was?
>
> Yes.
>
> Back then we didn't even have wildmatch(), and used fnmatch()
> instead, so forcing FNM_PATHNAME would have meant that people
> wouldn't be able to say "foo*bar" to match "foo/other/bar"; with
> wildmatch, "foo**bar" lets you defeat WM_PATHNAME so having
> WM_PATHNAME always in effect is less of an issue, but with
> fnmatch(), having FNM_PATHNAME always in effect has a lot of
> downside.

Ah, that makes sense.

>
> I'd expect that orderfile people have today will be broken and
> require tweaking if you switched WM_PATHNAME on.

OK, so we don't want to turn on WM_PATHNAME unless we do it for a new 
major version.

I'll do another re-roll and document the non-WM_PATHNAME behavior. 
Perhaps I'll encourage users to prefer ** over * if they want to match 
slash (even though they are equivalent) so that migration is easier if 
we ever do turn on WM_PATHNAME.

-Richard

Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jan 10, 2017 at 02:15:11PM -0800, Junio C Hamano wrote:
>
>> Richard Hansen <hansenr@google.com> writes:
>> ...
>> > I think so.  (For bare repositories anyway; non-bare should be
>> > relative to GIT_WORK_TREE.)  Perhaps git_config_pathname() itself
>> > should convert relative paths to absolute so that every pathname
>> > setting automatically works without changing any calling code.
>> 
>> Yes, that was what I was alluding to.  We might have to wait until
>> major version boundary to do so, but I think that it is the sensible
>> way forward in the longer term to convert relative to absolute in
>> git_config_pathname().
>
> Yeah, I'd agree.
>
> I'm undecided on whether it would need to happen at a major version
> bump. ...
>
> So I dunno. I do hate to break even corner cases, but I'm having trouble
> imagining the scenario where somebody is actually using the current
> behavior in a useful way.

Thanks for a detailed analysis (I probably should have spelt them
out when I said "we might have to" to save you the trouble).

Re: [PATCH 2/2] diff: document the pattern format for diff.orderFile

[Junio C Hamano]

Richard Hansen <hansenr@google.com> writes:

>> Back then we didn't even have wildmatch(), and used fnmatch()
>> instead, so forcing FNM_PATHNAME would have meant that people
>> wouldn't be able to say "foo*bar" to match "foo/other/bar"; with
>> wildmatch, "foo**bar" lets you defeat WM_PATHNAME so having
>> WM_PATHNAME always in effect is less of an issue, but with
>> fnmatch(), having FNM_PATHNAME always in effect has a lot of
>> downside.
>
> Ah, that makes sense.
>>
>> I'd expect that orderfile people have today will be broken and
>> require tweaking if you switched WM_PATHNAME on.
>
> OK, so we don't want to turn on WM_PATHNAME unless we do it for a new
> major version.

I do agree with you that if we were starting Git from scratch, or at
least if we were adding diff.orderfile feature today, we would have
used wildmatch(WM_PATHNAME) for this matching.  We would also have
used the same parser as used to read the exclude files (and when we
see negative matching entries in the parsed result, either errored
out or ignored them with warning).  That kind of change unfortunately
would require a major version bump, I am afraid.

[PATCH v3 0/2] diff orderfile documentation improvements

[Richard Hansen]

Changes from v2:
  * The orderfile feature doesn't set the WM_PATHNAME flag when it
    calls wildmatch(), so document the pattern format accordingly.

Richard Hansen (2):
  diff: document behavior of relative diff.orderFile
  diff: document the format of the -O (diff.orderFile) file

 Documentation/diff-config.txt  |  7 ++++---
 Documentation/diff-options.txt | 34 ++++++++++++++++++++++++++++++++--
 2 files changed, 36 insertions(+), 5 deletions(-)

-- 
2.11.0.483.g087da7b7c-goog

[PATCH v3 1/2] diff: document behavior of relative diff.orderFile

[Richard Hansen]

Document that a relative pathname for diff.orderFile is interpreted as
relative to the top-level work directory.

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 Documentation/diff-config.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 58f4bd6af..875212045 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -101,6 +101,8 @@ diff.noprefix::
 diff.orderFile::
 	File indicating how to order files within a diff, using
 	one shell glob pattern per line.
+	If `diff.orderFile` is a relative pathname, it is treated as
+	relative to the top of the work tree.
 	Can be overridden by the '-O' option to linkgit:git-diff[1].
 
 diff.renameLimit::
-- 
2.11.0.483.g087da7b7c-goog

[PATCH v3 2/2] diff: document the format of the -O (diff.orderFile) file

[Richard Hansen]

Signed-off-by: Richard Hansen <hansenr@google.com>
---
 Documentation/diff-config.txt  |  5 ++---
 Documentation/diff-options.txt | 34 ++++++++++++++++++++++++++++++++--
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 875212045..9e4111320 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -99,11 +99,10 @@ diff.noprefix::
 	If set, 'git diff' does not show any source or destination prefix.
 
 diff.orderFile::
-	File indicating how to order files within a diff, using
-	one shell glob pattern per line.
+	File indicating how to order files within a diff.
+	See the '-O' option to linkgit:git-diff[1] for details.
 	If `diff.orderFile` is a relative pathname, it is treated as
 	relative to the top of the work tree.
-	Can be overridden by the '-O' option to linkgit:git-diff[1].
 
 diff.renameLimit::
 	The number of files to consider when performing the copy/rename
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index e6215c372..d4fb70704 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -466,11 +466,41 @@ information.
 endif::git-format-patch[]
 
 -O<orderfile>::
-	Output the patch in the order specified in the
-	<orderfile>, which has one shell glob pattern per line.
+	Control the order in which files appear in the output.
 	This overrides the `diff.orderFile` configuration variable
 	(see linkgit:git-config[1]).  To cancel `diff.orderFile`,
 	use `-O/dev/null`.
++
+The output order is determined by the order of glob patterns in
+<orderfile>.
+All files with pathnames that match the first pattern are output
+first, all files with pathnames that match the second pattern (but not
+the first) are output next, and so on.
+All files with pathnames that do not match any pattern are output
+last, as if there was an implicit match-all pattern at the end of the
+file.
+If multiple pathnames have the same rank (they match the same pattern
+but no earlier patterns), their output order relative to each other is
+the normal order.
++
+<orderfile> is parsed as follows:
++
+--
+ - Blank lines are ignored, so they can be used as separators for
+   readability.
+
+ - Lines starting with a hash ("`#`") are ignored, so they can be used
+   for comments.  Add a backslash ("`\`") to the beginning of the
+   pattern if it starts with a hash.
+
+ - Each other line contains a single pattern.
+--
++
+Patterns have the same syntax and semantics as patterns used for
+fnmantch(3) without the FNM_PATHNAME flag, except a pathname also
+matches a pattern if removing any number of the final pathname
+components matches the pattern.  For example, the pattern "`foo*bar`"
+matches "`fooasdfbar`" and "`foo/bar/baz/asdf`" but not "`foobarx`".
 
 ifndef::git-format-patch[]
 -R::
-- 
2.11.0.483.g087da7b7c-goog