11 ms·

It would be nice if the error messages generated would suggest replacement functions that they deem appropriate. I see that I'm not supposed to use gmtime, loc

It would be nice if the error messages generated would suggest replacement functions that they deem appropriate. I see that I'm not supposed to use gmtime, localtime, ctime, ctime_r, asctime, and asctime_r; but what do they think I should use?

colordrops 6y ago
Also, why the functions are banned.
cle 6y ago
From the commit messages > The ctime_r() and asctime_r() functions are reentrant, but have no check that the buffer we pass in is long enough (the manpage says it "should have room for at least 26 bytes"). Since this is such an easy-to-get-wrong interface, and since we have the much safer strftime() as well as its more convenient strbuf_addftime() wrapper, let's ban both of those. (https://github.com/git/git/commit/91aef030152d121f6b4bc3b933c696073ba073e2 https://github.com/git/git/commit/91aef030152d121f6b4bc3b933...) > The traditional gmtime(), localtime(), ctime(), and asctime() functions return pointers to shared storage. This means they're not thread-safe, and they also run the risk of somebody holding onto the result across multiple calls (where each call invalidates the previous result). All callers should be using their reentrant counterparts. (https://github.com/git/git/commit/1fbfdf556f2abc708183caca53ae4e2881b46ae2 https://github.com/git/git/commit/1fbfdf556f2abc708183caca53...)
- drfuchs 6y ago
  Yes, but every hapless user shouldn't have to go searching through a bunch of commit messages to find the suggested replacement. Bad UX.
  - capableweb 6y ago
    The UX of using this list is not by manually searching through the list and seeing the reason behind them. You include the file together with the rest of your sources and now you get compilation errors if you try to use them. Can't think of a better UX for banned functions. Discovering why the thing is banned you only have to do once, if you care. If you're just modifying something quickly and minor in Git, you might not even care why.
  - grncdr 6y ago
    It seems pretty safe to assume a developer contributing C code to git itself would know how to use git blame (or the GitHub interface for it).
    - orf 6y ago
      Why make it harder, and why make it impossible to update if there are other suggested alternatives that are available since whenever the commit was made?
      - masklinn 6y ago
        > Why make it harder Because there is no way for a commit message to become outdated or detached from what it talks about, both of which are very much issues with comments. > why make it impossible to update if there are other suggested alternatives that are available since whenever the commit was made? Because that doesn't really matter.
        orf 6y ago
        > Because that doesn't really matter. Ok, so maybe rather than have this file we should run “git log | grep BANNED” and build a list of functions from that? Or maybe we could change all error messages to be “go look at the commit history to work out why this happened”. No? Maybe putting context in source files (or better yet, an error message!) rather than in a side channel like the commit message has value when it comes to understanding and updating, and it won’t be lost under the weight of future commits.
        deleted 6y ago
        [deleted]
        capableweb 6y ago
        Your source code should describe what the program should do today. It should not contain all historical artifacts about your source code, as it'll grow to big and unmanageable then. Instead, use Git to store temporal information, data that is about change and reasoning behind it. Git is basically a timeline, instead of hard facts of today. That's why it makes sense to describe the background and reasoning behind a change in a Git commit, instead of inside your source files as comments.
  - hzhou321 6y ago
    I think you are confused this is not for any hapless user. Developers search through and read commit messages all the time.
- tinus_hn 6y ago
  Strangely there is no mention of strtok which has a similar issue.
deleted 6y ago
[deleted]
chris_wot 6y ago
The commits actually do give that info. Take for instance this commit: https://github.com/git/git/commit/c8af66ab8ad7cd78557f0f9f5ef6a52fd46ee6dd#diff-fb44671421e652bbdc91bbe73b55da6547c851be1d604ce4d6c79b85a2e03654 https://github.com/git/git/commit/c8af66ab8ad7cd78557f0f9f5e... It actually gives examples and a lengthy explanation and reasoning behind the ban.
- mamon 6y ago
  But why put that info in commit message instead of a comment in the file itself?
  - colordrops 6y ago
    Or even in the compile error message itself.
  - chris_wot 6y ago
    Because comments can be tedious and get out of sync with the repo. Why not check the git history? I wish more repos could be like this!
    - adrianmonk 6y ago
      > Why not check the git history? Because that is effort every person who uses the file has to do over and over again, whereas maintaining the file is effort that has to be done once by one person.
      - chris_wot 6y ago
        It sounds like you want a manual. Personal preference I guess. The maintainers seem to have decided to keep it in the history. It's not like this was ever meant for anything other than git itself.
        astrobe_ 6y ago
        If all you have is Git everything looks like a commit.
deleted 6y ago
[deleted]
deleted 6y ago
[deleted]
dev_tty01 6y ago
It would be even nicer if it redefined the call to a safe version and then generated a warning message informing the programmer of the substitution.
- pjc50 6y ago
  You can't do that because the semantics are different in most cases.

colordrops 6y ago

Also, why the functions are banned.

cle 6y ago

From the commit messages > The ctime_r() and asctime_r() functions are reentrant, but have no check that the buffer we pass in is long enough (the manpage says it "should have room for at least 26 bytes"). Since this is such an easy-to-get-wrong interface, and since we have the much safer strftime() as well as its more convenient strbuf_addftime() wrapper, let's ban both of those. (https://github.com/git/git/commit/91aef030152d121f6b4bc3b933c696073ba073e2 https://github.com/git/git/commit/91aef030152d121f6b4bc3b933...) > The traditional gmtime(), localtime(), ctime(), and asctime() functions return pointers to shared storage. This means they're not thread-safe, and they also run the risk of somebody holding onto the result across multiple calls (where each call invalidates the previous result). All callers should be using their reentrant counterparts. (https://github.com/git/git/commit/1fbfdf556f2abc708183caca53ae4e2881b46ae2 https://github.com/git/git/commit/1fbfdf556f2abc708183caca53...)

drfuchs 6y ago
Yes, but every hapless user shouldn't have to go searching through a bunch of commit messages to find the suggested replacement. Bad UX.
- capableweb 6y ago
  The UX of using this list is not by manually searching through the list and seeing the reason behind them. You include the file together with the rest of your sources and now you get compilation errors if you try to use them. Can't think of a better UX for banned functions. Discovering why the thing is banned you only have to do once, if you care. If you're just modifying something quickly and minor in Git, you might not even care why.
- grncdr 6y ago
  It seems pretty safe to assume a developer contributing C code to git itself would know how to use git blame (or the GitHub interface for it).
  - orf 6y ago
    Why make it harder, and why make it impossible to update if there are other suggested alternatives that are available since whenever the commit was made?
    - masklinn 6y ago
      > Why make it harder Because there is no way for a commit message to become outdated or detached from what it talks about, both of which are very much issues with comments. > why make it impossible to update if there are other suggested alternatives that are available since whenever the commit was made? Because that doesn't really matter.
      - orf 6y ago
        > Because that doesn't really matter. Ok, so maybe rather than have this file we should run “git log | grep BANNED” and build a list of functions from that? Or maybe we could change all error messages to be “go look at the commit history to work out why this happened”. No? Maybe putting context in source files (or better yet, an error message!) rather than in a side channel like the commit message has value when it comes to understanding and updating, and it won’t be lost under the weight of future commits.
        deleted 6y ago
        [deleted]
        capableweb 6y ago
        Your source code should describe what the program should do today. It should not contain all historical artifacts about your source code, as it'll grow to big and unmanageable then. Instead, use Git to store temporal information, data that is about change and reasoning behind it. Git is basically a timeline, instead of hard facts of today. That's why it makes sense to describe the background and reasoning behind a change in a Git commit, instead of inside your source files as comments.
- hzhou321 6y ago
  I think you are confused this is not for any hapless user. Developers search through and read commit messages all the time.
tinus_hn 6y ago
Strangely there is no mention of strtok which has a similar issue.

deleted 6y ago

[deleted]

chris_wot 6y ago

The commits actually do give that info. Take for instance this commit: https://github.com/git/git/commit/c8af66ab8ad7cd78557f0f9f5ef6a52fd46ee6dd#diff-fb44671421e652bbdc91bbe73b55da6547c851be1d604ce4d6c79b85a2e03654 https://github.com/git/git/commit/c8af66ab8ad7cd78557f0f9f5e... It actually gives examples and a lengthy explanation and reasoning behind the ban.

mamon 6y ago
But why put that info in commit message instead of a comment in the file itself?
- colordrops 6y ago
  Or even in the compile error message itself.
- chris_wot 6y ago
  Because comments can be tedious and get out of sync with the repo. Why not check the git history? I wish more repos could be like this!
  - adrianmonk 6y ago
    > Why not check the git history? Because that is effort every person who uses the file has to do over and over again, whereas maintaining the file is effort that has to be done once by one person.
    - chris_wot 6y ago
      It sounds like you want a manual. Personal preference I guess. The maintainers seem to have decided to keep it in the history. It's not like this was ever meant for anything other than git itself.
      - astrobe_ 6y ago
        If all you have is Git everything looks like a commit.

deleted 6y ago

[deleted]

deleted 6y ago

[deleted]

dev_tty01 6y ago

It would be even nicer if it redefined the call to a safe version and then generated a warning message informing the programmer of the substitution.

pjc50 6y ago
You can't do that because the semantics are different in most cases.