[PATCH] RFC: Introduce '.gitorderfile'

[PATCH] RFC: Introduce '.gitorderfile'

[Stefan Beller]

Conceptually the file order as set with command line -O or via the config
'diff.orderFile' is interesting to both the author (when I run a quick git
 diff locally) as well as reviewer (a patch floating on the mailing list),
so it is not just the author who should be responsible for getting their
config in order, but a project would benefit when they could give a good
default for such an order.

While the change in this RFC patch to diff.c may look uncontroversial,
(Oh look! it's just another knob we can turn!), the change to the
newly introduced '.gitorderfile' may be more controversial. Here is my
rationale for proposing it:

  I want to force myself to think about the design before pointing out
  memory leaks and coding style, so the least I would wish for is:
    *.h
    *.c
  but as we have more to look at, I would want to have the most abstract
  thing to come first. And most abstract from the actual code is the
  user interaction, the documentation.  I heard the claim that the git
  project deliberately names the directory 'Documentation/' with a capital
  D such that we had this property by default already. With a patch like
  this we could rename Documentation/ to docs and still enjoy reading the
  docs first.
  Given this alibi, I would claim that t/ is misnamed though! I personally
  would prefer to review tests just after the documentation instead of
  after the code as the tests are more abstract and encode promises to the
  user unlike the code itself that is truth at the end of the day.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

I wrote:
> offtopic: As a general thing for our patches, can we configure
> (or even convince Git in general), that headers ought to be sent *before*
> its accompanying source? I think that would help reviewers like me, who
> tend to start reading linearly and then giving random thoughts, because the
> header prepares the reviewer for the source code with expectations. Also
> by having it the other way around, the review first focuses on design
> (Is this function signature sane; the docs said it would do X while not
> doing Y, is that sane?) instead of code.

and hence I came up with this patch, as I think we would want to expose
such a good feature ('diff.orderFile') even for those who are not looking
for it themselves.

Thanks,
Stefan


 .gitorderfile |  6 ++++++
 diff.c        | 11 +++++++++++
 2 files changed, 17 insertions(+)
 create mode 100644 .gitorderfile

diff --git a/.gitorderfile b/.gitorderfile
new file mode 100644
index 0000000000..5131ede927
--- /dev/null
+++ b/.gitorderfile
@@ -0,0 +1,6 @@
+Documentation/*
+t/*
+*.sh
+*.h
+*.c
+Makefile
diff --git a/diff.c b/diff.c
index 00b4c86698..8d537db06a 100644
--- a/diff.c
+++ b/diff.c
@@ -3398,6 +3398,17 @@ void diff_setup(struct diff_options *options)
 	if (diff_indent_heuristic)
 		DIFF_XDL_SET(options, INDENT_HEURISTIC);
 
+	if (!diff_order_file_cfg) {
+		struct stat st;
+		int c = lstat(".gitorderfile", &st);
+		if (c == 0 && S_ISREG(st.st_mode))
+			diff_order_file_cfg = ".gitorderfile";
+		else if (c < 0 && errno == ENOENT)
+			; /* File does not exist. no preset. */
+		else
+			die_errno("stat '.gitorderfile'");
+	}
+
 	options->orderfile = diff_order_file_cfg;
 
 	if (diff_no_prefix) {
-- 
2.13.2.695.g117ddefdb4

Re: [PATCH] RFC: Introduce '.gitorderfile'

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>   I want to force myself to think about the design before pointing out
>   memory leaks and coding style, so the least I would wish for is:
>     *.h
>     *.c
>   but as we have more to look at, I would want to have the most abstract
>   thing to come first. And most abstract from the actual code is the
>   user interaction, the documentation.

This is exactly why I invented the orderfile in the first place.
But such a "policy" is not something a project would want to enforce
its users all the time; it is left to personal preference of the
participants.

Just set diff.orderFile to suit your taste without bothering other
people, I would say.

Re: [PATCH] RFC: Introduce '.gitorderfile'

[Jeff King]

On Wed, Jul 12, 2017 at 01:44:46PM -0700, Junio C Hamano wrote:

> Stefan Beller <sbeller@google.com> writes:
> 
> >   I want to force myself to think about the design before pointing out
> >   memory leaks and coding style, so the least I would wish for is:
> >     *.h
> >     *.c
> >   but as we have more to look at, I would want to have the most abstract
> >   thing to come first. And most abstract from the actual code is the
> >   user interaction, the documentation.
> 
> This is exactly why I invented the orderfile in the first place.
> But such a "policy" is not something a project would want to enforce
> its users all the time; it is left to personal preference of the
> participants.
> 
> Just set diff.orderFile to suit your taste without bothering other
> people, I would say.

I could see somebody arguing that format-patch should respect a project
preference, since its primary purpose is to communicate your work to the
rest of the project.

But then you could make a similar argument for other diff options, too.

-Peff

Re: [PATCH] RFC: Introduce '.gitorderfile'

[Stefan Beller]

On Wed, Jul 12, 2017 at 1:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:


>
> Just set diff.orderFile to suit your taste without bothering other
> people, I would say.

I must have explained it very badly, I'll try again:

There are 2 different use cases to use diffs.
1. my personal use case to look at logs, diffs.
    I'll just set diff.orderFile and I'll be fine.
2. collaboration.
    When I want to review a patch from the mailing list,
    I could (a) download the patch, apply locally, see the diff
    formatted nicely according to diff.orderFile. Go back to my
    mail client, type an answer, quoting the patch out of order.
    This is very much work for me, so ...

    Alternatively (b) I could ask anyone who wants me to review
    patches to set diff.orderFile to my liking. Then I can stay in
    my mail client and have the diff hunks presented in an order
    that is most effective for review.   (This is nuts, because
    the author would just ask someone else to review their code.
    "Who am I to suggest obscure rules in collaboration?")

    Another alternative (c) is to have this 'suggested' order file
    in place. Anyone that does not care about an order would use
    the suggested order, that coincidentally matches what reviewers
    in this project think is best for review.

    With that, the author has no additional burden and the reviewers
    get better patch presentations by default.


>
>>   I want to force myself to think about the design before pointing out
>>   memory leaks and coding style, so the least I would wish for is:
>>     *.h
>>     *.c
>>   but as we have more to look at, I would want to have the most abstract
>>   thing to come first. And most abstract from the actual code is the
>>   user interaction, the documentation.
>
> This is exactly why I invented the orderfile in the first place.

This is my personal use case.

> But such a "policy" is not something a project would want to enforce
> its users all the time; it is left to personal preference of the
> participants.

I kindly ask that we enforce a policy on people sending patches.
And to do that I would not want to bloat SubmittingPatches, but
rather give a good default, so that people not thinking about this
detail, still get it right.

Thanks,
Stefan

Re: [PATCH] RFC: Introduce '.gitorderfile'

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 12 2017, Junio C. Hamano jotted:

> Stefan Beller <sbeller@google.com> writes:
>
>>   I want to force myself to think about the design before pointing out
>>   memory leaks and coding style, so the least I would wish for is:
>>     *.h
>>     *.c
>>   but as we have more to look at, I would want to have the most abstract
>>   thing to come first. And most abstract from the actual code is the
>>   user interaction, the documentation.
>
> This is exactly why I invented the orderfile in the first place.
> But such a "policy" is not something a project would want to enforce
> its users all the time; it is left to personal preference of the
> participants.
>
> Just set diff.orderFile to suit your taste without bothering other
> people, I would say.

I very much like Stefan's approach here[1]. It's very useful to be able
to set something like this in a config on a project-wide basis, so that
wherever patches are reviewed (in Gitweb, Git[hub|lab], git-format-patch
etc.) they look as the project prefers.

As Stefan points out we're already accomplishing this with Documentation
as a result of a hack.

But I see why you'd be concerned about this. If you're looking at random
git repos there's a limit to what we'd like to allow them to customize
without causing confusion. The diff order is relatively innocent, but
you might make the case that other diff settings should be adjusted too
(e.g. -U0 on certain file types), and that's quite the rabbit hole.

Still, I think this is innocuous enough to stright the right balance
between intrusion and usability. Some would find this *very* useful,
think projects that for whatever reason need to commit e.g. verbose XML
config along with their code, and the framework they're using happens to
rely on conf/ and src/ directories.

1. Although nit: let's call it .gitorder not .gitorderfile, that it's a
   file is implicit. We don't have .gitignorefile, .gitattributesfile
   etc.

Re: [PATCH] RFC: Introduce '.gitorderfile'

[Stefan Beller]

On Wed, Jul 12, 2017 at 1:57 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Jul 12, 2017 at 01:44:46PM -0700, Junio C Hamano wrote:
>
>> Stefan Beller <sbeller@google.com> writes:
>>
>> >   I want to force myself to think about the design before pointing out
>> >   memory leaks and coding style, so the least I would wish for is:
>> >     *.h
>> >     *.c
>> >   but as we have more to look at, I would want to have the most abstract
>> >   thing to come first. And most abstract from the actual code is the
>> >   user interaction, the documentation.
>>
>> This is exactly why I invented the orderfile in the first place.
>> But such a "policy" is not something a project would want to enforce
>> its users all the time; it is left to personal preference of the
>> participants.
>>
>> Just set diff.orderFile to suit your taste without bothering other
>> people, I would say.
>
> I could see somebody arguing that format-patch should respect a project
> preference, since its primary purpose is to communicate your work to the
> rest of the project.
>
> But then you could make a similar argument for other diff options, too.

This similar argument would be to have a in-tree configuration for
--unified=<N> for example?

This triggers two reactions for me:

(a) We should totally do that.
  Different projects have different coding styles (e.g. opening brace
  in its own new line or at the end of the condition), which very much impacts
  how useful the context is. So, sure, the project knows best what their
  preference is.

(b) It's a rabbit hole to go down.
  Any config option that format-patch respects can be argued to be useful
  to be preset by a project. So in the end we'd have a ".gitconfig"
file offering
  good defaults for people using that project. This may have security
implications.
  And it's a lot of work.

I see your point for (b), I'll think about it more.

Thanks,
Stefan

Re: [PATCH] RFC: Introduce '.gitorderfile'

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 2. collaboration.
>     When I want to review a patch from the mailing list,
>     I could (a) download the patch, apply locally, see the diff
>     formatted nicely according to diff.orderFile.

If you are not doing a review of a patch with complex changes that
benefits by a local diff.orderfile (i.e. only in the mail client
without applying and viewing the changes in wider context), then you
either (1) have a much greater memory than I do and know all the
code outside the patch context by heart, or (2) not reviewing them
properly in context.

I tend to suspect that it is the latter case, so that argument does
not sound convincing at least to me.

No, I do not apply all patches before commenting from my mailbox; a
one or two-pager patch can often be viewed and judged without much
surrounding context, and can be answered in the mail client, perhaps
while running "less" on some related files that may or may not be
touched by the patch in another terminal, without applying the
patch.  But such a one or two-pager patch can be viewed in any
presentation order and do not behefit much fro diff.orderfile
anyway.

Re: [PATCH] RFC: Introduce '.gitorderfile'

[Stefan Beller]

On Wed, Jul 12, 2017 at 2:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> 2. collaboration.
>>     When I want to review a patch from the mailing list,
>>     I could (a) download the patch, apply locally, see the diff
>>     formatted nicely according to diff.orderFile.
>
> If you are not doing a review of a patch with complex changes that
> benefits by a local diff.orderfile (i.e. only in the mail client
> without applying and viewing the changes in wider context), then you
> either (1) have a much greater memory than I do and know all the
> code outside the patch context by heart, or (2) not reviewing them
> properly in context.
>
> I tend to suspect that it is the latter case, so that argument does
> not sound convincing at least to me.

Note that this request originated from
https://public-inbox.org/git/cover.1499800530.git.jonathantanmy@google.com/

There are different sorts of patches to review:
(a) major new features, introducing radically new concepts.
    Example given above.

    In this case I neither need (1) nor (2). I want to get the abstract
    design and then decide if it is worth my time to pursue reviewing the
    details of the patch.

    The order file makes an impact!

(b) minor new features (in the big picture of a major feature), refactorings
    Examples: 96dc883b3c, repository: enable initialization of submodules
    lb/status-stash-count

    Reviewing these patches requires more diving into code, but you'd
    still want to make a call early on whether to reject the design
    before calling out the memory leak that you found after applying
    the patch.

    The order file *may* be useful.

(c) updating existing things
    (bug fixes, documentation, improving performance)
    Example: jk/reflog-walk-maint

    When looking at these changes, (2) is the answer.
    I look at surrounding code (there may be no need to apply
    the patch though, it depends)

    The order file is useless, IMHO, but also does not produce harm.

> No, I do not apply all patches before commenting from my mailbox; a
> one or two-pager patch can often be viewed and judged without much
> surrounding context, and can be answered in the mail client, perhaps
> while running "less" on some related files that may or may not be
> touched by the patch in another terminal, without applying the
> patch.  But such a one or two-pager patch can be viewed in any
> presentation order and do not behefit much fro diff.orderfile
> anyway.

This sounds like (c), which we have a lot more of than (a) or (b).

Thanks for your thoughts,
Stefan

Re: [PATCH] RFC: Introduce '.gitorderfile'

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I could see somebody arguing that format-patch should respect a project
> preference, since its primary purpose is to communicate your work to the
> rest of the project.
>
> But then you could make a similar argument for other diff options, too.

Yeah, and that opens a whole can of worms.

We let projects to ship clean/smudge or textconv filters and also
mark paths to which these tools may be of help, but we do not let
projects to automatically enable them in the cloned repository.  The
projects must _tell_ the user how to run the last step (e.g. "There
is a tools/setup-my-clone script shipped with the source; running it
will add necessary configurations to work better with our project").

I do not think usefulness of diff.orderfile is being questioned, but
I think it is something we should treat just like any other thing
that affects repository configuration.  A .gitorderfile that allows
the project to behave as if we allowed to auto-enable just one thing
in the clone, while not allowing others, a source of issues and
unnecessary headaches later.

Besides, diff-order is *not* the only order that matters in the use
of the system, and we _will_ regret the name ".gitorderfile" later,
as people would start making noises about forcing ls-files and other
things to also show the list following that order.

Re: [PATCH] RFC: Introduce '.gitorderfile'

[Jeff King]

On Wed, Jul 12, 2017 at 02:08:35PM -0700, Stefan Beller wrote:

> > I could see somebody arguing that format-patch should respect a project
> > preference, since its primary purpose is to communicate your work to the
> > rest of the project.
> >
> > But then you could make a similar argument for other diff options, too.
> 
> This similar argument would be to have a in-tree configuration for
> --unified=<N> for example?

Yes, that was exactly the option I was thinking of. :)

> This triggers two reactions for me:
> 
> (a) We should totally do that.
>   Different projects have different coding styles (e.g. opening brace
>   in its own new line or at the end of the condition), which very much impacts
>   how useful the context is. So, sure, the project knows best what their
>   preference is.
> 
> (b) It's a rabbit hole to go down.
>   Any config option that format-patch respects can be argued to be useful
>   to be preset by a project. So in the end we'd have a ".gitconfig"
> file offering
>   good defaults for people using that project. This may have security
> implications.
>   And it's a lot of work.
> 
> I see your point for (b), I'll think about it more.

And yes, I had both of those reactions, too. We've had the
"project-level .gitconfig" discussion many times over the years. And it
generally comes back to "you can ship a snippet of config and then give
people a script which adds it to their repo".

-Peff

Re: [PATCH] RFC: Introduce '.gitorderfile'

[Jeff King]

On Wed, Jul 12, 2017 at 04:54:38PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I could see somebody arguing that format-patch should respect a project
> > preference, since its primary purpose is to communicate your work to the
> > rest of the project.
> >
> > But then you could make a similar argument for other diff options, too.
> 
> Yeah, and that opens a whole can of worms.
> 
> We let projects to ship clean/smudge or textconv filters and also
> mark paths to which these tools may be of help, but we do not let
> projects to automatically enable them in the cloned repository.  The
> projects must _tell_ the user how to run the last step (e.g. "There
> is a tools/setup-my-clone script shipped with the source; running it
> will add necessary configurations to work better with our project").
> 
> I do not think usefulness of diff.orderfile is being questioned, but
> I think it is something we should treat just like any other thing
> that affects repository configuration.  A .gitorderfile that allows
> the project to behave as if we allowed to auto-enable just one thing
> in the clone, while not allowing others, a source of issues and
> unnecessary headaches later.

Thanks for writing this out. That was exactly what I was trying to imply
with my final statement, but you said it much better. :)

> Besides, diff-order is *not* the only order that matters in the use
> of the system, and we _will_ regret the name ".gitorderfile" later,
> as people would start making noises about forcing ls-files and other
> things to also show the list following that order.

I'd also agree with this.

-Peff

Re: [PATCH] RFC: Introduce '.gitorderfile'

[Stefan Beller]

On Thu, Jul 13, 2017 at 8:59 AM, Jeff King <peff@peff.net> wrote:
>> This triggers two reactions for me:
>>
>> (a) We should totally do that.
>
>> (b) It's a rabbit hole to go down.
>
> And yes, I had both of those reactions, too. We've had the
> "project-level .gitconfig" discussion many times over the years. And it
> generally comes back to "you can ship a snippet of config and then give
> people a script which adds it to their repo".

I see this "project-level .gitconfig" via the .gitmodules file.
See GITMODULES(5), anything except submodule.<name>.path is
just project-level .gitconfig, so in that sense we already threw out the
baby with the bathwater. I think we want to be extra careful to not add
more possible options into the .gitmodules file, now that we established
a strong stance on not shipping a project-level .gitconfig.

Thanks,
Stefan

Re: [PATCH] RFC: Introduce '.gitorderfile'

[Brandon Williams]

On 07/13, Stefan Beller wrote:
> On Thu, Jul 13, 2017 at 8:59 AM, Jeff King <peff@peff.net> wrote:
> >> This triggers two reactions for me:
> >>
> >> (a) We should totally do that.
> >
> >> (b) It's a rabbit hole to go down.
> >
> > And yes, I had both of those reactions, too. We've had the
> > "project-level .gitconfig" discussion many times over the years. And it
> > generally comes back to "you can ship a snippet of config and then give
> > people a script which adds it to their repo".
> 
> I see this "project-level .gitconfig" via the .gitmodules file.
> See GITMODULES(5), anything except submodule.<name>.path is
> just project-level .gitconfig, so in that sense we already threw out the
> baby with the bathwater. I think we want to be extra careful to not add
> more possible options into the .gitmodules file, now that we established
> a strong stance on not shipping a project-level .gitconfig.

I'm trying to work on cleaning up the submodule-config a bit and as a
result I should be able to make it more difficult to ship more
project-level configurations in .gitmodules

-- 
Brandon Williams

Re: [PATCH] RFC: Introduce '.gitorderfile'

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Thu, Jul 13, 2017 at 8:59 AM, Jeff King <peff@peff.net> wrote:
>>> This triggers two reactions for me:
>>>
>>> (a) We should totally do that.
>>
>>> (b) It's a rabbit hole to go down.
>>
>> And yes, I had both of those reactions, too. We've had the
>> "project-level .gitconfig" discussion many times over the years. And it
>> generally comes back to "you can ship a snippet of config and then give
>> people a script which adds it to their repo".
>
> I see this "project-level .gitconfig" via the .gitmodules file.
> See GITMODULES(5), anything except submodule.<name>.path is
> just project-level .gitconfig,...

They were from day one meant as a suggestion made by the project.
You do not have to follow them and you are free to update them,
i.e. after "submodule init" copied URL to point at a closer mirror,
for example.

Re: [PATCH] RFC: Introduce '.gitorderfile'

[Stefan Beller]

On Thu, Jul 13, 2017 at 12:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Thu, Jul 13, 2017 at 8:59 AM, Jeff King <peff@peff.net> wrote:
>>>> This triggers two reactions for me:
>>>>
>>>> (a) We should totally do that.
>>>
>>>> (b) It's a rabbit hole to go down.
>>>
>>> And yes, I had both of those reactions, too. We've had the
>>> "project-level .gitconfig" discussion many times over the years. And it
>>> generally comes back to "you can ship a snippet of config and then give
>>> people a script which adds it to their repo".
>>
>> I see this "project-level .gitconfig" via the .gitmodules file.
>> See GITMODULES(5), anything except submodule.<name>.path is
>> just project-level .gitconfig,...
>
> They were from day one meant as a suggestion made by the project.
> You do not have to follow them and you are free to update them,
> i.e. after "submodule init" copied URL to point at a closer mirror,
> for example.

The URL is somewhat special as its copying into the .git/config
also marks the submodule as interesting (no matter if the URL is
changed by the user).

The point I was trying to make is best demonstrated in
t5526-fetch-submodules.sh:

> ok 7 - using fetchRecurseSubmodules=true in .gitmodules recurses into submodules
> ok 8 - --no-recurse-submodules overrides .gitmodules config
> ok 9 - using fetchRecurseSubmodules=false in .git/config overrides setting in .gitmodules

They were not suggestions, but defaults dictated by the project.

If the user did not change their config, they did as the project
said. I was not there on day one to remember if they are merely
meant as suggestions, but their behavior is asserting.

Re: [PATCH] RFC: Introduce '.gitorderfile'

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The point I was trying to make is best demonstrated in
> t5526-fetch-submodules.sh:
>
>> ok 7 - using fetchRecurseSubmodules=true in .gitmodules recurses into submodules
>> ok 8 - --no-recurse-submodules overrides .gitmodules config
>> ok 9 - using fetchRecurseSubmodules=false in .git/config overrides setting in .gitmodules
>
> They were not suggestions, but defaults dictated by the project.

Yeah, and that is why I kept thinking that recurse-submodules may be
a huge mistake.