| Message ID | c43de19d867cb5e63fe6689b2b7d645dc4741950.1712878339.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | docs: recommend using contrib/contacts/git-contacts | expand |
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Linus Arver <linusa@google.com> > > Use a dash ("git-contacts", not "git contacts") because the script > is not a core builtin command that is compiled into the `git` binary. Pedantic, but "git mergetool" is how it is spelled even though it is not a core builtin command and is not compiled into the binary. The reason why "git-contacts" is better is because we do not install it to be usable by user's "git". ... because the script is not installed as part of "git" toolset. An obvious alternative of course is to promote "contacts" out of "contrib/" and install it as part of the standard toolset. I gave a brief scan of the script and did not find anything (other than "only the recent 5 years worth of history matters") that is too specific to our project and I suspect it should do a reasonable job when run in any repository/working tree of a git-managed project. But it is outside the scope of this series. I'd still welcome the thought to do that after the dust settles, though. > This also puts the script on one line, which should make it easier to > grep for with a loose search query, such as > > $ git grep git.contacts Documentation > > . Also add a footnote to describe where the script could actually be Let's drop ". "; it may leave the previous sentence appear hanging unterminated, but the capital A that begins a new sentence is a good enough sign that we finished the previous sentence, isn't it? > diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches > index e734a3f0f17..8b6e4bf0300 100644 > --- a/Documentation/SubmittingPatches > +++ b/Documentation/SubmittingPatches > @@ -493,9 +493,16 @@ security relevant should not be submitted to the public mailing list > mentioned below, but should instead be sent privately to the Git > Security mailing list{security-ml-ref}. > > +:contrib-scripts: footnoteref:[contrib-scripts,Scripts under `contrib/` are not + > +part of the core `git` binary and must be called separately. Consult your + > +package manager to determine where it is located. For example, on Ubuntu-based + > +systems it could be installed under + > +`/usr/share/doc/git/contrib/contacts/git-contacts` and may need to be called + > +with `perl ...` if it does not have the executable bit set.] I wouldn't call anything in /usr/share/doc/ "installed", though. In the context of _this_ document where the user is working on _git_ project towards submitting patches to _us_, it is far simpler to drop the above paragraph and tell them how to run the script in contrib/, e.g. $ perl contrib/contacts/git-contacts <args>... without hinting there is anything platform/distro specific, and instead to have them all work from our sources. I am assuming that any user who are reading this part of the document would have a reasonably recent version of our sources checked out (after all, they already have a patch or two to send but they are learning the way to find whom to send them to).
On Fri, Apr 12, 2024 at 1:09 PM Junio C Hamano <gitster@pobox.com> wrote: > "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes: > > Use a dash ("git-contacts", not "git contacts") because the script > > is not a core builtin command that is compiled into the `git` binary. > > Pedantic, but "git mergetool" is how it is spelled even though it is > not a core builtin command and is not compiled into the binary. The > reason why "git-contacts" is better is because we do not install it > to be usable by user's "git". > > ... because the script is not installed as part of "git" > toolset. > > An obvious alternative of course is to promote "contacts" out of > "contrib/" and install it as part of the standard toolset. I gave a > brief scan of the script and did not find anything (other than "only > the recent 5 years worth of history matters") that is too specific > to our project and I suspect it should do a reasonable job when run > in any repository/working tree of a git-managed project. > > But it is outside the scope of this series. I'd still welcome the > thought to do that after the dust settles, though. An alternative would be to deprecate and/or remove `git-contacts` from "contrib" and instead point people at Felipe's `git-related`[1], which is the direct parent[2] of `git-contacts`, as well as a more functional drop-in replacement for `git-contacts`. [1]: https://github.com/felipec/git-related [2]: https://lore.kernel.org/git/1372590512-21341-1-git-send-email-sunshine@sunshineco.com/
Eric Sunshine <sunshine@sunshineco.com> writes: > An alternative would be to deprecate and/or remove `git-contacts` from > "contrib" and instead point people at Felipe's `git-related`[1], which > is the direct parent[2] of `git-contacts`, as well as a more > functional drop-in replacement for `git-contacts`. I am not sure it is wise to add an external dependency on a tool that is not very well known ([1] has only 68 stars), not packaged for distros [*] and is more or less dormant (the last update was April last year). Unless the one we locally carry is vastly inadequate in comparison, I somehow doubt that it is a better alternative to ask our target audience to use it, than letting them use git-contacts from in-tree. [Footnote] * "git imerge" and "tig" are what I compare with to use as a yardstick, when trying to judge how well-known and easily obtainable a package related to us is.
Junio C Hamano <gitster@pobox.com> writes: > "Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Linus Arver <linusa@google.com> >> >> Use a dash ("git-contacts", not "git contacts") because the script >> is not a core builtin command that is compiled into the `git` binary. > > Pedantic, but "git mergetool" is how it is spelled even though it is > not a core builtin command and is not compiled into the binary. The > reason why "git-contacts" is better is because we do not install it > to be usable by user's "git". > > ... because the script is not installed as part of "git" > toolset. Noted; I will use this wording. > An obvious alternative of course is to promote "contacts" out of > "contrib/" and install it as part of the standard toolset. I gave a > brief scan of the script and did not find anything (other than "only > the recent 5 years worth of history matters") that is too specific > to our project and I suspect it should do a reasonable job when run > in any repository/working tree of a git-managed project. > > But it is outside the scope of this series. I'd still welcome the > thought to do that after the dust settles, though. Ack. Ideally we would translate it to C (or, dare I say, into a C lib + some other higher level language, Perl or otherwise), but I agree that's for another series. >> This also puts the script on one line, which should make it easier to >> grep for with a loose search query, such as >> >> $ git grep git.contacts Documentation >> >> . Also add a footnote to describe where the script could actually be > > Let's drop ". " Will do. > ; it may leave the previous sentence appear hanging > unterminated, but the capital A that begins a new sentence is a good > enough sign that we finished the previous sentence, isn't it? In hindsight I agree. From a quick Google search [1] it looks like your style is what's preferred in academia also. >> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches >> index e734a3f0f17..8b6e4bf0300 100644 >> --- a/Documentation/SubmittingPatches >> +++ b/Documentation/SubmittingPatches >> @@ -493,9 +493,16 @@ security relevant should not be submitted to the public mailing list >> mentioned below, but should instead be sent privately to the Git >> Security mailing list{security-ml-ref}. >> >> +:contrib-scripts: footnoteref:[contrib-scripts,Scripts under `contrib/` are not + >> +part of the core `git` binary and must be called separately. Consult your + >> +package manager to determine where it is located. For example, on Ubuntu-based + >> +systems it could be installed under + >> +`/usr/share/doc/git/contrib/contacts/git-contacts` and may need to be called + >> +with `perl ...` if it does not have the executable bit set.] > > I wouldn't call anything in /usr/share/doc/ "installed", though. > > In the context of _this_ document where the user is working on _git_ > project towards submitting patches to _us_, it is far simpler to > drop the above paragraph and tell them how to run the script in > contrib/, e.g. > > $ perl contrib/contacts/git-contacts <args>... > > without hinting there is anything platform/distro specific, and > instead to have them all work from our sources. Indeed. One small change is that the script already has the execute bit set so I can drop `perl` as $0 (the execute bit is removed when it is copied into /usr/share/... on my system). > I am assuming that any user who are reading this part of the > document would have a reasonably recent version of our sources > checked out (after all, they already have a patch or two to send but > they are learning the way to find whom to send them to). Agreed. [1] https://camosun.libguides.com/Chicago-17thEd/quotations
Linus Arver <linusa@google.com> writes: >> In the context of _this_ document where the user is working on _git_ >> project towards submitting patches to _us_, it is far simpler to >> drop the above paragraph and tell them how to run the script in >> contrib/, e.g. >> >> $ perl contrib/contacts/git-contacts <args>... >> >> without hinting there is anything platform/distro specific, and >> instead to have them all work from our sources. > > Indeed. One small change is that the script already has the execute bit > set so I can drop `perl` as $0 (the execute bit is removed when it is > copied into /usr/share/... on my system). We want to be a bit careful here, though. The script begins with "#!/usr/bin/perl", but on some systems ther eis no such command (but /usr/local/bin is on user's PATH and perl exists there).
Junio C Hamano <gitster@pobox.com> writes: > Linus Arver <linusa@google.com> writes: > >>> In the context of _this_ document where the user is working on _git_ >>> project towards submitting patches to _us_, it is far simpler to >>> drop the above paragraph and tell them how to run the script in >>> contrib/, e.g. >>> >>> $ perl contrib/contacts/git-contacts <args>... >>> >>> without hinting there is anything platform/distro specific, and >>> instead to have them all work from our sources. >> >> Indeed. One small change is that the script already has the execute bit >> set so I can drop `perl` as $0 (the execute bit is removed when it is >> copied into /usr/share/... on my system). > > We want to be a bit careful here, though. > > The script begins with "#!/usr/bin/perl", but on some systems ther > eis no such command (but /usr/local/bin is on user's PATH and perl > exists there). Doh, I already sent a v5. Sorry about that. <wears cone of shame> Anyway, should I do something like "#!/usr/bin/env perl" or similar as another patch? It should be more portable than the hardcoded path we have to /usr/bin/perl.
Linus Arver <linusa@google.com> writes: > Anyway, should I do something like "#!/usr/bin/env perl" or similar as > another patch? It should be more portable than the hardcoded path we > have to /usr/bin/perl. The project preference always has been to replace #!/usr/bin/$prog when installing to match the system's path, without having to assume that "env" is available and is installed in /usr/bin/ We are not installing this thing (yet), so how about giving an instruction to run "perl contrib/contacts/git-contacts", only assuming that the user is intelligent enough to be able to react to "perl: not found" by installing it on their path?
Junio C Hamano <gitster@pobox.com> writes: > We are not installing this thing (yet), so how about giving an > instruction to run "perl contrib/contacts/git-contacts", only > assuming that the user is intelligent enough to be able to react to > "perl: not found" by installing it on their path? That is, something like this, perhaps. As the string given to --cc-cmd is stored in $cc_cmd, and is used in this call: push @cc, recipients_cmd("cc-cmd", "cc", $cc_cmd, $t, $quiet) where recipients_cmd takes ($prefix, $what, $cmd, $file, $quiet) and runs execute_cmd($prefix, $cmd, $file). execute_cmd in turn takes ($prefix, $cmd, $file) and does this: open my $fh, "-|", "$cmd \Q$file\E" or die sprintf(__("(%s) Could not execute '%s'"), $prefix, $cmd); IOW, $cmd is just an early part of a shell command line that takes a filename as its last argument, so I think it would be fine for $cmd to be "perl contrib/contacts/git-contacts". I did not test it, and it would be appreciated if people can test it. Documentation/MyFirstContribution.txt | 6 +++--- Documentation/SubmittingPatches | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git c/Documentation/MyFirstContribution.txt w/Documentation/MyFirstContribution.txt index 9665951aeb..9eb6b98a82 100644 --- c/Documentation/MyFirstContribution.txt +++ w/Documentation/MyFirstContribution.txt @@ -1118,12 +1118,12 @@ valuable, such as changing the Reply-to address or adding more CC and BCC lines. :contrib-scripts: footnoteref:[contrib-scripts,Scripts under `contrib/` are + not part of the core `git` binary and must be called directly. Clone the Git + -codebase and run `contrib/contacts/git-contacts` (you must have Perl installed + -in your system).] +codebase and run `perl contrib/contacts/git-contacts` (you must have Perl + +installed in your system).] NOTE: If you're not sure whom to CC, running `contrib/contacts/git-contacts` can list potential reviewers. In addition, you can do `git send-email ---cc-cmd='contrib/contacts/git-contacts' feature/*.patch`{contrib-scripts} to +--cc-cmd='perl contrib/contacts/git-contacts' feature/*.patch`{contrib-scripts} to automatically pass this list of emails to `send-email`. NOTE: When you are sending a real patch, it will go to git@vger.kernel.org - but diff --git c/Documentation/SubmittingPatches w/Documentation/SubmittingPatches index b616422197..1099040d7e 100644 --- c/Documentation/SubmittingPatches +++ w/Documentation/SubmittingPatches @@ -407,8 +407,8 @@ mailing list{security-ml}, instead of the public mailing list. :contrib-scripts: footnoteref:[contrib-scripts,Scripts under `contrib/` are + not part of the core `git` binary and must be called directly. Clone the Git + -codebase and run `contrib/contacts/git-contacts` (you must have Perl installed + -in your system).] +codebase and run `perl contrib/contacts/git-contacts` (you must have Perl + +installed in your system).] Send your patch with "To:" set to the mailing list, with "cc:" listing people who are involved in the area you are touching (the `git-contacts` @@ -422,7 +422,7 @@ If you are using `send-email`, you can feed it the output of `git-contacts` like this: .... - git send-email --cc-cmd='contrib/contacts/git-contacts' feature/*.patch + git send-email --cc-cmd='perl contrib/contacts/git-contacts' feature/*.patch .... :current-maintainer: footnote:[The current maintainer: gitster@pobox.com]
On Wed, Apr 17, 2024 at 1:38 AM Junio C Hamano <gitster@pobox.com> wrote: > IOW, $cmd is just an early part of a shell command line that takes a > filename as its last argument, so I think it would be fine for $cmd > to be "perl contrib/contacts/git-contacts". I did not test it, and > it would be appreciated if people can test it. > > diff --git c/Documentation/MyFirstContribution.txt w/Documentation/MyFirstContribution.txt > @@ -1118,12 +1118,12 @@ valuable, such as changing the Reply-to address or adding more CC and BCC lines. > -codebase and run `contrib/contacts/git-contacts` (you must have Perl installed + > -in your system).] > +codebase and run `perl contrib/contacts/git-contacts` (you must have Perl + > +installed in your system).] I wonder if we really need to hand-hold so much to tell people that they must have Perl installed, especially since the command being run _is_ `perl`. It might be sufficient simply to say: ... codebase and run `perl contrib/contacts/git-contacts`.] Anyhow, it's a minor point.
Eric Sunshine <sunshine@sunshineco.com> writes: >> @@ -1118,12 +1118,12 @@ valuable, such as changing the Reply-to address or adding more CC and BCC lines. >> -codebase and run `contrib/contacts/git-contacts` (you must have Perl installed + >> -in your system).] >> +codebase and run `perl contrib/contacts/git-contacts` (you must have Perl + >> +installed in your system).] > > I wonder if we really need to hand-hold so much to tell people that > they must have Perl installed, especially since the command being run > _is_ `perl`. It might be sufficient simply to say: > > ... codebase and run `perl contrib/contacts/git-contacts`.] > > Anyhow, it's a minor point. True. In the original it was a good idea, but once we show the invocation that is explicitly done with 'perl', we no longer need to say that. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> We are not installing this thing (yet), so how about giving an >> instruction to run "perl contrib/contacts/git-contacts", only >> assuming that the user is intelligent enough to be able to react to >> "perl: not found" by installing it on their path? > > That is, something like this, perhaps. > > As the string given to --cc-cmd is stored in $cc_cmd, and is used in > this call: > > push @cc, recipients_cmd("cc-cmd", "cc", $cc_cmd, $t, $quiet) > > where recipients_cmd takes ($prefix, $what, $cmd, $file, $quiet) and > runs execute_cmd($prefix, $cmd, $file). execute_cmd in turn takes > ($prefix, $cmd, $file) and does this: > > open my $fh, "-|", "$cmd \Q$file\E" > or die sprintf(__("(%s) Could not execute '%s'"), $prefix, $cmd); > > IOW, $cmd is just an early part of a shell command line that takes a > filename as its last argument, so I think it would be fine for $cmd > to be "perl contrib/contacts/git-contacts". I did not test it, and > it would be appreciated if people can test it. I should be able to test this later this week.
Junio C Hamano <gitster@pobox.com> writes: > Eric Sunshine <sunshine@sunshineco.com> writes: > >>> @@ -1118,12 +1118,12 @@ valuable, such as changing the Reply-to address or adding more CC and BCC lines. >>> -codebase and run `contrib/contacts/git-contacts` (you must have Perl installed + >>> -in your system).] >>> +codebase and run `perl contrib/contacts/git-contacts` (you must have Perl + >>> +installed in your system).] >> >> I wonder if we really need to hand-hold so much to tell people that >> they must have Perl installed, especially since the command being run >> _is_ `perl`. It might be sufficient simply to say: >> >> ... codebase and run `perl contrib/contacts/git-contacts`.] >> >> Anyhow, it's a minor point. > > True. In the original it was a good idea, but once we show the > invocation that is explicitly done with 'perl', we no longer need to > say that. > Agreed. Will update (but will first try to test the 'perl ...' arg to --cc-cmd).
Linus Arver <linusa@google.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Junio C Hamano <gitster@pobox.com> writes: >> >>> We are not installing this thing (yet), so how about giving an >>> instruction to run "perl contrib/contacts/git-contacts", only >>> assuming that the user is intelligent enough to be able to react to >>> "perl: not found" by installing it on their path? >> >> That is, something like this, perhaps. >> >> As the string given to --cc-cmd is stored in $cc_cmd, and is used in >> this call: >> >> push @cc, recipients_cmd("cc-cmd", "cc", $cc_cmd, $t, $quiet) >> >> where recipients_cmd takes ($prefix, $what, $cmd, $file, $quiet) and >> runs execute_cmd($prefix, $cmd, $file). execute_cmd in turn takes >> ($prefix, $cmd, $file) and does this: >> >> open my $fh, "-|", "$cmd \Q$file\E" >> or die sprintf(__("(%s) Could not execute '%s'"), $prefix, $cmd); >> >> IOW, $cmd is just an early part of a shell command line that takes a >> filename as its last argument, so I think it would be fine for $cmd >> to be "perl contrib/contacts/git-contacts". I did not test it, and >> it would be appreciated if people can test it. > > I should be able to test this later this week. Looks like --cc-cmd="perl contrib/contacts/git-contacts" works as expected! I tested by setting up a working git-send-mail config and running with --dry-run to check the CC list. Will reroll later today. Cheers.
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index e734a3f0f17..8b6e4bf0300 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -493,9 +493,16 @@ security relevant should not be submitted to the public mailing list mentioned below, but should instead be sent privately to the Git Security mailing list{security-ml-ref}. +:contrib-scripts: footnoteref:[contrib-scripts,Scripts under `contrib/` are not + +part of the core `git` binary and must be called separately. Consult your + +package manager to determine where it is located. For example, on Ubuntu-based + +systems it could be installed under + +`/usr/share/doc/git/contrib/contacts/git-contacts` and may need to be called + +with `perl ...` if it does not have the executable bit set.] + Send your patch with "To:" set to the mailing list, with "cc:" listing -people who are involved in the area you are touching (the `git -contacts` command in `contrib/contacts/` can help to +people who are involved in the area you are touching (the `git-contacts` +script in `contrib/contacts/`{contrib-scripts} can help to identify them), to solicit comments and reviews. Also, when you made trial merges of your topic to `next` and `seen`, you may have noticed work by others conflicting with your changes. There is a good possibility