Re: [PATCH v2 0/1] Documentation/ToolsForGit.txt: Tools for developing Git
On 4/17/22 11:35, COGONI Guillaume wrote:
Show 16 quoted lines
> MOY Matthieu wrote:
>
>>> +In particular, this script enables using the VS Code visual debugger, including
>>> +setting breakpoints, logpoints, conditional breakpoints and more in the editor.
>>
>> I don't think the last sentence is needed, and if it is, it would be
>> better within contrib/vscode/README.md (so that someone reaching this
>> README directly do see the information too).
>
> I think so too. However, I already make a PATCH last week for
> contrib/vscode/README:
> (see <20220407204001.112287-2-cogoni.guillaume@gmail.com>).
> And, I see that in What's cooking in git.git (Apr 2022, #04; Thu, 14)
> it will be merge in Next. So, do I take this PATCH from the last week
> and I add it this part in contrib/vscode/README or I just add this part
> here in this new PATCH but where the subject is different?
I think you can just drop that sentence. For someone a bit familiar with either VS code or any other IDE, it's no big surprise that the debugger integration allows such feature. For someone not familiar with VS code, the patch about to land in next already contains a link to a page explaining that.
If you want to add more explanation, then I think it should be added to contrib/vscode/README. You will probably want to add in next to the lines touched by the previous patch, hence either Junio will have to solve a conflict, or you need to write your patch on top of the previous one.
Show 22 quoted lines
>> - For Emacs, it's useful to put the following in
>> GIT_CHECKOUT/.dir-locals.el, assuming you use cperl-mode:
>
>> ;; note the first part is useful for C editing, too
>> ((nil . ((indent-tabs-mode . t)
>> (tab-width . 8)
>> (fill-column . 80)))
>> (cperl-mode . ((cperl-indent-level . 8)
>> (cperl-extra-newline-before-brace . nil)
>> (cperl-merge-trailing-else . t))))
>
>> Actually, the Linux kernel's CodingStyle contains more relevant stuff
>> (for C, not Perl):
>
>> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#you-ve-made-a-mess-of-it
>
> I add this part directly in ToolsForGit.txt and not in the README in contrib/emacs.
> But, from this document in Documentation/RelNotes/2.18.0.txt, I read this:
> "The scripts in contrib/emacs/ have outlived their usefulness and have been
> replaced with a stub that errors out and tells the user there are replacements."
> So, for the next version of this PATCH, can I replace what is in the README by the
> configuration that I write in ToolsForGit.txt?
contrib/emacs was really not meant for developers hacking on Git. Since it contains only pointers to obsolete stuff, we may want to just discard its current content and make it the place to put documentation for people hacking on Git with Emacs, just like contrib/vscode/ is for VS code and Git. But we probably have only a few (tens of) lines of documentation, so adding the doc directly in ToolsForGit.txt is probably better.
Show 9 quoted lines
> OAKLEY Philip wrote:
>
>> I'm of the view that a README is a positive indicator that there is some
>> informational value regarding the tool's use for developing Git being
>> made available. It doesn't always have to be code before it is of
>> assistance in developing Git.
>
> I agreed with OAKLEY, the README is good indicator to say that we have some
> information besides the scripts.
A good indicator, yes. But reading only the summary ...
> == Summary
>
> This document aims to gather tools that have a README and/or scripts in
> the Git project.
I have no idea whether this document's audience is "people hacking ON Git's codebase" or "people using Git (somewhere else)". I believe the first sentences of the document should answer "is this document for me?" (i.e. "shall I continue reading?"), and currently it doesn't.
Saying instead something like
This documents gathers tips, scripts and configuration file to help people working on Git's codebase use their favorite tools while following Git's coding style.
would make the target audience much clearer IMHO.
Show 13 quoted lines
> +- To follow rules of the CodingGuideline, it's useful to put the following in
> +GIT_CHECKOUT/.dir-locals.el, assuming you use cperl-mode:
> +----
> +;; note the first part is useful for C editing, too
> +((nil . ((indent-tabs-mode . t)
> + (tab-width . 8)
> + (fill-column . 80)))
> + (cperl-mode . ((cperl-indent-level . 8)
> + (cperl-extra-newline-before-brace . nil)
> + (cperl-merge-trailing-else . t))))
> +----
> +
> +- The version for C:
The version for Perl already contains some configuration applicable to C (indent-tabs-mode, tab-width and fill-column).
Show 6 quoted lines
> +(add-hook 'c-mode-hook
> + (lambda ()
> + (let ((filename (buffer-file-name)))
> + ;; Enable kernel mode for the appropriate files
> + (when (and filename
> + (string-match (expand-file-name "~/src/linux-trees")
This works only if the user checked out Git's source code in ~/src/linux-trees, which is unlikely ;-). I guess this path is not to be taken literally (i.e. it's more clearly "adapt the configuration to your actual directory layout"), but using linux-trees here won't make it clear that it should be the place to Git's source code.
> + filename))
> + (setq indent-tabs-mode t)
This one is redundant with the above for example.
Actually, while the tips given in Linux's kernel CodingStyle are relevant for us, I'm not sure copy-pasting them in Git's tree actually has added value. We may as well just link to them, like
For a more complete setup, since Git's codebase uses a coding style similar to the Linux kernel's style, tips given in Linux's CodingStyle document can be applied here too.
(Plus a link to the file on kernel.org)
If you copy paste here, you should cite the source and say stg like "This is adapted from Linux's suggestion in its CodingStyle document".
Show 9 quoted lines
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -492,17 +492,6 @@ For Perl programs:
>
> - Learn and use Git.pm if you need that functionality.
>
> - - For Emacs, it's useful to put the following in
> - GIT_CHECKOUT/.dir-locals.el, assuming you use cperl-mode:
> -
I agree that removing Emacs-specific code from a general document is nice, but then you should replace it with a link to ToolsForGit.txt like "Tips to make your editor follow this style can be found in ToolsForGit.txt" (without being specific to Emacs, that's the point of the document, it also applies to VS code and may be extended in the future to other editors).
Cheers,
--
Matthieu Moy
https://matthieu-moy.fr/
COGONI Guillaume
