diff mbox series

Re* [PATCH v2] fast-import: disallow "." and ".." path components

Message ID xmqq1pywvmhq.fsf_-_@gitster.g (mailing list archive)
State New
Headers show
Series Re* [PATCH v2] fast-import: disallow "." and ".." path components | expand

Commit Message

Junio C Hamano Nov. 27, 2024, 1:23 p.m. UTC 
"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

>> +	if (is_dot_or_dotdot(e->name->str_dat))
>> +		die("path %s contains invalid component", p);
>
> Nit: single-quoting the path seems more common:
>
>     $ git grep "\"path '%s'" ':!po/' | wc -l
>     17
>     $ git grep "\"path %s" ':!po/' | wc -l
>     4

Ah, I missed that one.  Thanks for catching.

We probably should write it down.

--- >8 ---
[PATCH] CodingGuidelines: a handful of error message guidelines

It is more efficient to have something in the coding guidelines
document to point at, when we want to review and comment on a new
message in the codebase to make sure it "fits" in the set of
existing messages.

Let's write down established best practice we are aware of.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * I am writing what I think is the established practice from
   memory; clarifications, corrections, and additions are all
   welcome.

 Documentation/CodingGuidelines | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Eric Sunshine Nov. 27, 2024, 7:41 p.m. UTC | #1 
On Wed, Nov 27, 2024 at 8:23 AM Junio C Hamano <gitster@pobox.com> wrote:
> [PATCH] CodingGuidelines: a handful of error message guidelines
>
> It is more efficient to have something in the coding guidelines
> document to point at, when we want to review and comment on a new
> message in the codebase to make sure it "fits" in the set of
> existing messages.
>
> Let's write down established best practice we are aware of.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
> @@ -703,16 +703,22 @@ Program Output
>  Error Messages
>
> - - Do not end error messages with a full stop.
> + - Do not end a single-sentence error message with a full stop.
>
>   - Do not capitalize the first word, only because it is the first word
> -   in the message ("unable to open %s", not "Unable to open %s").  But
> +   in the message ("unable to open '%s'", not "Unable to open '%s'").  But
>     "SHA-3 not supported" is fine, because the reason the first word is
>     capitalized is not because it is at the beginning of the sentence,
>     but because the word would be spelled in capital letters even when
>     it appeared in the middle of the sentence.
>
> - - Say what the error is first ("cannot open %s", not "%s: cannot open")
> + - Say what the error is first ("cannot open '%s'", not "%s: cannot open").
> +
> + - Enclose the subject of an error inside a pair of single quotes,
> +   e.g. `die(_("unable to open '%s'"), path)`.

These changes all seem fine.

> + - Unless there is a compelling reason not to, error messages should
> +   be marked for `_("translation")`.

We might want to spell this out more fully, such as stating that
messages from porcelain commands should be marked for translation, but
messages in plumbing should not. Also, perhaps mention explicitly that
BUG("message") should not be marked for translation since they are
intended to be read by Git developers, not by end-users.
diff mbox series

Patch

diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
index 87904791cb..0444391983 100644
--- c/Documentation/CodingGuidelines
+++ w/Documentation/CodingGuidelines
@@ -703,16 +703,22 @@  Program Output
 
 Error Messages
 
- - Do not end error messages with a full stop.
+ - Do not end a single-sentence error message with a full stop.
 
  - Do not capitalize the first word, only because it is the first word
-   in the message ("unable to open %s", not "Unable to open %s").  But
+   in the message ("unable to open '%s'", not "Unable to open '%s'").  But
    "SHA-3 not supported" is fine, because the reason the first word is
    capitalized is not because it is at the beginning of the sentence,
    but because the word would be spelled in capital letters even when
    it appeared in the middle of the sentence.
 
- - Say what the error is first ("cannot open %s", not "%s: cannot open")
+ - Say what the error is first ("cannot open '%s'", not "%s: cannot open").
+
+ - Enclose the subject of an error inside a pair of single quotes,
+   e.g. `die(_("unable to open '%s'"), path)`.
+
+ - Unless there is a compelling reason not to, error messages should
+   be marked for `_("translation")`.
 
 
 Externally Visible Names