fix: added new BANNED_EXPL macro for better error messages, has a par…

[hgm-king]

Extend Banned Function Error Messages

As a newer user, I want to be able to understand why certain functions are banned as well as know what functions to use instead.

Changes

• Added new macro named BANNED_EXPL(func, expl) which added the expl onto the error message
• Used strcpy as an example

cc: Taylor Blau ttaylorr@github.com
cc: Jeff King peff@peff.net

[gitgitgadget]

Welcome to GitGitGadget

Hi @HGHimself, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

• the lines should not exceed 76 columns,
• the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
• the commit messages' body should be describing the "why?" of the change.
• Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

• Search: is:pr is:open "/allow"

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the FreeNode IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Freenode. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

[gitgitgadget]

There is an issue in commit f09fd43:
First line of commit message is too long (> 76 columns): fix: added new BANNED_EXPL macro for better error messages, has a parameter to allow for an explanation

[dscho]

/allow

[gitgitgadget]

User HGHimself is now allowed to use GitGitGadget.

[hgm-king]

/preview

[gitgitgadget]

Preview email sent as pull.896.git.1614991693935.gitgitgadget@gmail.com

[hgm-king]

/submit

[gitgitgadget]

Submitted as pull.896.git.1614991897210.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-896/HGHimself/fix/add-new-ban-macro-with-explanation-parameter-v1

To fetch this version to local tag pr-896/HGHimself/fix/add-new-ban-macro-with-explanation-parameter-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-896/HGHimself/fix/add-new-ban-macro-with-explanation-parameter-v1

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

"HG King via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  #undef strcpy
> -#define strcpy(x,y) BANNED(strcpy)
> +#define strcpy(x,y) BANNED_EXPL(strcpy, buffer_overflow_risk)

That does not help programmers that much (the above does not say
what to use instead, for example), and the mechanism inherently
does not give you sufficient space to give helpful guidance.

Adding a comment around each of these definition may be OK.  Upon
seeing foo_is_a_banned_function, somebody new to the codebase would
look for where it is banned, and find the above, so that is a good
place to give guidance.

[gitgitgadget]

On the Git mailing list, Taylor Blau wrote (reply to this):

On Sun, Mar 07, 2021 at 12:34:57PM -0800, Junio C Hamano wrote:
> "HG King via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> >  #undef strcpy
> > -#define strcpy(x,y) BANNED(strcpy)
> > +#define strcpy(x,y) BANNED_EXPL(strcpy, buffer_overflow_risk)
>
> That does not help programmers that much (the above does not say
> what to use instead, for example), and the mechanism inherently
> does not give you sufficient space to give helpful guidance.

Trying to cram information like "why is this function unsafe?" and "what
function should I use instead?" seems ill-fitted to trying to a macro
which is supposed to have a field for each.

I'm certainly not opposed to making these banned functions clearer, but
I do not think that this is the way to do it.

> Adding a comment around each of these definition may be OK.  Upon
> seeing foo_is_a_banned_function, somebody new to the codebase would
> look for where it is banned, and find the above, so that is a good
> place to give guidance.

Perhaps, but all of this information is already covered accurately in
the patches that introduced each banned function. So I'm not sure that I
even agree that this information is difficult to discover to begin with,
but I may be biased.

Thanks,
Taylor

[gitgitgadget]

User Taylor Blau <ttaylorr@github.com> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Jeff King wrote (reply to this):

On Mon, Mar 08, 2021 at 01:05:46PM -0500, Taylor Blau wrote:

> > Adding a comment around each of these definition may be OK.  Upon
> > seeing foo_is_a_banned_function, somebody new to the codebase would
> > look for where it is banned, and find the above, so that is a good
> > place to give guidance.
> 
> Perhaps, but all of this information is already covered accurately in
> the patches that introduced each banned function. So I'm not sure that I
> even agree that this information is difficult to discover to begin with,
> but I may be biased.

Yeah, certainly my intent when I introduced banned.h was that people
would get the full reasoning from the commit log. I figured Git
developers could run "git log -S" or "git blame".

I'm not opposed to comments if somebody wants to write them, but it's
not clear to whether people who are actively writing patches for Git
have actually run into this situation and been confused, or if this is
bikeshedding from the recent posting of banned.h on Hacker News. (Even
if it is the latter, I am OK taking a patch that adds comments; I just
doubt that it's a good use of anybody's time).

-Peff

[gitgitgadget]

User Jeff King <peff@peff.net> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Taylor Blau <ttaylorr@github.com> writes:

>> Adding a comment around each of these definition may be OK.  Upon
>> seeing foo_is_a_banned_function, somebody new to the codebase would
>> look for where it is banned, and find the above, so that is a good
>> place to give guidance.
>
> Perhaps, but all of this information is already covered accurately in
> the patches that introduced each banned function. So I'm not sure that I
> even agree that this information is difficult to discover to begin with,
> but I may be biased.

To help those who are not yet familiar with this codebase but are
willing to learn, I tend to agree with you that it is a good idea
not to miss opportunities to encourage them to run "git blame" to
find out about the project history in the parts of the system that
pique their interest.  They will make more motivated contributors
who are invested in the project, if they stick around.

It would discourage and filter out "drive-by" contributors, though.

Somebody may not yet be interested enough to "git clone" us, but
happens to have an extract of distro source tarball and enough
motivation to try peeking and tweaking around.  To such a curious
developer, the "blame" information and log messages with the change
are not readily available, so there probably is some balance to be
struck.

[dscho]

@hgm-king Are you still interested in this PR?

[dscho]

/allow

[gitgitgadget]

User hgm-king is now allowed to use GitGitGadget.