[PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635

[PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635

[Naomi Ibe]

From: Naomi <naomi.ibeh69@gmail.com>

Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
---
 builtin/add.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index c27254a5cd..5126d2ede3 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -182,7 +182,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 
 	if (repo_read_index(the_repository) < 0)
-		die(_("Could not read the index"));
+		die(_("could not read the index"));
 
 	repo_init_revisions(the_repository, &rev, prefix);
 	rev.diffopt.context = 7;
@@ -200,15 +200,15 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 		die(_("editing patch failed"));
 
 	if (stat(file, &st))
-		die_errno(_("Could not stat '%s'"), file);
+		die_errno(_("could not stat '%s'"), file);
 	if (!st.st_size)
-		die(_("Empty patch. Aborted."));
+		die(_("empty patch. aborted"));
 
 	child.git_cmd = 1;
 	strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
 		     NULL);
 	if (run_command(&child))
-		die(_("Could not apply '%s'"), file);
+		die(_("could not apply '%s'"), file);
 
 	unlink(file);
 	free(file);
@@ -568,7 +568,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 finish:
 	if (write_locked_index(&the_index, &lock_file,
 			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
-		die(_("Unable to write new index file"));
+		die(_("unable to write new index file"));
 
 	dir_clear(&dir);
 	clear_pathspec(&pathspec);
-- 
2.36.1.windows.1

[PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635

[Naomi Ibe]

From: Naomi <naomi.ibeh69@gmail.com>

Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
---
 builtin/add.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index c27254a5cd..5126d2ede3 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -182,7 +182,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 
 	if (repo_read_index(the_repository) < 0)
-		die(_("Could not read the index"));
+		die(_("could not read the index"));
 
 	repo_init_revisions(the_repository, &rev, prefix);
 	rev.diffopt.context = 7;
@@ -200,15 +200,15 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
 		die(_("editing patch failed"));
 
 	if (stat(file, &st))
-		die_errno(_("Could not stat '%s'"), file);
+		die_errno(_("could not stat '%s'"), file);
 	if (!st.st_size)
-		die(_("Empty patch. Aborted."));
+		die(_("empty patch. aborted"));
 
 	child.git_cmd = 1;
 	strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
 		     NULL);
 	if (run_command(&child))
-		die(_("Could not apply '%s'"), file);
+		die(_("could not apply '%s'"), file);
 
 	unlink(file);
 	free(file);
@@ -568,7 +568,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 finish:
 	if (write_locked_index(&the_index, &lock_file,
 			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
-		die(_("Unable to write new index file"));
+		die(_("unable to write new index file"));
 
 	dir_clear(&dir);
 	clear_pathspec(&pathspec);
-- 
2.36.1.windows.1

Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635

[Christian Couder]

On Mon, Oct 9, 2023 at 3:17 AM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
>
> From: Naomi <naomi.ibeh69@gmail.com>

First the subject should start, after "[PATCH 1/1][Outreachy]", with
the area of the code you are changing followed by ":", so here "add:"
(no need for ".c").

Also even if the subject gives a lot of information already, it's
better to use the body of the commit message to give a bit more
context and details. For example here either the subject or the body
of the commit message should say which specific guideline(s) the patch
is enforcing.

> Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
> ---
>  builtin/add.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>

Otherwise the patch looks good to me.

Thanks!

Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635

[Naomi Ibe]

Thank you very much for the feedback Christian, I'd definitely keep
this in mind the next time I'm creating/sending a patch.
You also said asides working on my commit message, the patch looks
good, but as I'm applying to Outreachy, I need to send in my proof of
contribution as a link of a task I worked on.
Would sending a link of the mailing list , with the url of this patch
I just worked on suffice?Something like this :
https://public-inbox.org/git/20231009011652.1791-1-naomi.ibeh69@gmail.com/T/#u
I ask because since I used patches to send to Git , I have no pull
request link to submit.

On Mon, Oct 9, 2023 at 8:28 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Mon, Oct 9, 2023 at 3:17 AM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
> >
> > From: Naomi <naomi.ibeh69@gmail.com>
>
> First the subject should start, after "[PATCH 1/1][Outreachy]", with
> the area of the code you are changing followed by ":", so here "add:"
> (no need for ".c").
>
> Also even if the subject gives a lot of information already, it's
> better to use the body of the commit message to give a bit more
> context and details. For example here either the subject or the body
> of the commit message should say which specific guideline(s) the patch
> is enforcing.
>
> > Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
> > ---
> >  builtin/add.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
>
> Otherwise the patch looks good to me.
>
> Thanks!

Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635

[Christian Couder]

On Mon, Oct 9, 2023 at 9:57 AM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
>
> Thank you very much for the feedback Christian, I'd definitely keep
> this in mind the next time I'm creating/sending a patch.

Great!

> You also said asides working on my commit message, the patch looks
> good,

Yeah, but first others might find some issues in the patch itself,
also, as I say elsewhere, I think it's worth it for you to send a
version 2 of the patch that fixes the commit message issues I pointed
out.

> but as I'm applying to Outreachy, I need to send in my proof of
> contribution as a link of a task I worked on.
> Would sending a link of the mailing list , with the url of this patch
> I just worked on suffice?Something like this :
> https://public-inbox.org/git/20231009011652.1791-1-naomi.ibeh69@gmail.com/T/#u
> I ask because since I used patches to send to Git , I have no pull
> request link to submit.

Yeah, links to the emails you sent in a mailing list archive (Public
Inbox or lore.kernel.org/git) are fine.

Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635

[Junio C Hamano]

Naomi Ibe <naomi.ibeh69@gmail.com> writes:

> Subject: Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using d

Subject: [OUTREACHY][PATCH 1/1] builtin/add.c: clean up die() messages

> From: Naomi <naomi.ibeh69@gmail.com>

The name and address on this line come from your commit object,
which in turn would have came from your configuration.  You have

    [user]
	name = Naomi
	email = naomi.ibeh69@gmail.com

somewhere in your configuration file, perhaps in $HOME/.gitconfig or
somewhere.  When contributiong to this project, you want that "name"
line to also include your family name, as it should match what you
write on your Signed-off-by: line.  A focused way to do so without
affecting your author identity for other projects is to add

    [user]
	name = Naomi Ibe

in .git/config of the repository that you use to contribute to this
project, e.g.,

    $ cd ... to the working tree of your clone of git you work in ...
    $ git config user.name "Naomi Ibe"

The space above your Sign off is to fill the details you omitted on
the title of the message (which would say something about "conform
to guidelines" or "clean up" without mentioning what rule the
original violates), making the whole thing something like:

    builtin/add.c: clean up die() messages

    As described in the CodingGuidelines document, a single line
    message given to die() and its friends should not capitalize its
    first word, and should not add full-stop at the end.

    Signed-off-by: ...

> Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
> ---
>  builtin/add.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Thanks.  Otherwise the patch looks good.

>
> diff --git a/builtin/add.c b/builtin/add.c
> index c27254a5cd..5126d2ede3 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -182,7 +182,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
>  	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
>  
>  	if (repo_read_index(the_repository) < 0)
> -		die(_("Could not read the index"));
> +		die(_("could not read the index"));
>  
>  	repo_init_revisions(the_repository, &rev, prefix);
>  	rev.diffopt.context = 7;
> @@ -200,15 +200,15 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
>  		die(_("editing patch failed"));
>  
>  	if (stat(file, &st))
> -		die_errno(_("Could not stat '%s'"), file);
> +		die_errno(_("could not stat '%s'"), file);
>  	if (!st.st_size)
> -		die(_("Empty patch. Aborted."));
> +		die(_("empty patch. aborted"));
>  
>  	child.git_cmd = 1;
>  	strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
>  		     NULL);
>  	if (run_command(&child))
> -		die(_("Could not apply '%s'"), file);
> +		die(_("could not apply '%s'"), file);
>  
>  	unlink(file);
>  	free(file);
> @@ -568,7 +568,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
>  finish:
>  	if (write_locked_index(&the_index, &lock_file,
>  			       COMMIT_LOCK | SKIP_IF_UNCHANGED))
> -		die(_("Unable to write new index file"));
> +		die(_("unable to write new index file"));
>  
>  	dir_clear(&dir);
>  	clear_pathspec(&pathspec);

Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635

[Naomi Ibe]

Thank you very much! I'd definitely make those changes on my next patch.
Should I begin work on version 2 or should I still wait for additional
input on the version 1?

On Mon, Oct 9, 2023 at 7:49 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Naomi Ibe <naomi.ibeh69@gmail.com> writes:
>
> > Subject: Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using d
>
> Subject: [OUTREACHY][PATCH 1/1] builtin/add.c: clean up die() messages
>
> > From: Naomi <naomi.ibeh69@gmail.com>
>
> The name and address on this line come from your commit object,
> which in turn would have came from your configuration.  You have
>
>     [user]
>         name = Naomi
>         email = naomi.ibeh69@gmail.com
>
> somewhere in your configuration file, perhaps in $HOME/.gitconfig or
> somewhere.  When contributiong to this project, you want that "name"
> line to also include your family name, as it should match what you
> write on your Signed-off-by: line.  A focused way to do so without
> affecting your author identity for other projects is to add
>
>     [user]
>         name = Naomi Ibe
>
> in .git/config of the repository that you use to contribute to this
> project, e.g.,
>
>     $ cd ... to the working tree of your clone of git you work in ...
>     $ git config user.name "Naomi Ibe"
>
> The space above your Sign off is to fill the details you omitted on
> the title of the message (which would say something about "conform
> to guidelines" or "clean up" without mentioning what rule the
> original violates), making the whole thing something like:
>
>     builtin/add.c: clean up die() messages
>
>     As described in the CodingGuidelines document, a single line
>     message given to die() and its friends should not capitalize its
>     first word, and should not add full-stop at the end.
>
>     Signed-off-by: ...
>
> > Signed-off-by: Naomi Ibe <naomi.ibeh69@gmail.com>
> > ---
> >  builtin/add.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
>
> Thanks.  Otherwise the patch looks good.
>
> >
> > diff --git a/builtin/add.c b/builtin/add.c
> > index c27254a5cd..5126d2ede3 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -182,7 +182,7 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
> >       git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
> >
> >       if (repo_read_index(the_repository) < 0)
> > -             die(_("Could not read the index"));
> > +             die(_("could not read the index"));
> >
> >       repo_init_revisions(the_repository, &rev, prefix);
> >       rev.diffopt.context = 7;
> > @@ -200,15 +200,15 @@ static int edit_patch(int argc, const char **argv, const char *prefix)
> >               die(_("editing patch failed"));
> >
> >       if (stat(file, &st))
> > -             die_errno(_("Could not stat '%s'"), file);
> > +             die_errno(_("could not stat '%s'"), file);
> >       if (!st.st_size)
> > -             die(_("Empty patch. Aborted."));
> > +             die(_("empty patch. aborted"));
> >
> >       child.git_cmd = 1;
> >       strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
> >                    NULL);
> >       if (run_command(&child))
> > -             die(_("Could not apply '%s'"), file);
> > +             die(_("could not apply '%s'"), file);
> >
> >       unlink(file);
> >       free(file);
> > @@ -568,7 +568,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> >  finish:
> >       if (write_locked_index(&the_index, &lock_file,
> >                              COMMIT_LOCK | SKIP_IF_UNCHANGED))
> > -             die(_("Unable to write new index file"));
> > +             die(_("unable to write new index file"));
> >
> >       dir_clear(&dir);
> >       clear_pathspec(&pathspec);

Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635

[Christian Couder]

On Tue, Oct 10, 2023 at 5:20 PM Naomi Ibe <naomi.ibeh69@gmail.com> wrote:
>
> Thank you very much! I'd definitely make those changes on my next patch.
> Should I begin work on version 2 or should I still wait for additional
> input on the version 1?

I think you can work on version 2. Also please reply inline instead of
top-posting (see https://en.wikipedia.org/wiki/Posting_style).

Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635

[Junio C Hamano]

Naomi Ibe <naomi.ibeh69@gmail.com> writes:

> Thank you very much! I'd definitely make those changes on my next patch.

[administrivia] do not top post.

> Should I begin work on version 2 or should I still wait for additional
> input on the version 1?

There is no "rule", but based on observations on how people behave,
e.g.

 * for a small patch like this that can be given a good review in 10
   minutes or so, those who do not do so within the first 3 days
   will probably not do so.

 * once a reasonably thorough review is given, those who haven't
   responded to the patch and do not have much else to say are
   unlikely to respond.

 * on the other hand, after such a review is given, those who do not
   agree with the review tend to respond rather quickly, to get
   their voice in before it becomes too late.

I would say it would be good to start working on it right away and
use a couple of days reviewing it yourself before posting it.

Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635

[Naomi Ibe]

Okay then, I'll work with these guidelines in mind. Thank you very much

On Tue, Oct 10, 2023 at 11:24 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Naomi Ibe <naomi.ibeh69@gmail.com> writes:
>
> > Thank you very much! I'd definitely make those changes on my next patch.
>
> [administrivia] do not top post.
>
> > Should I begin work on version 2 or should I still wait for additional
> > input on the version 1?
>
> There is no "rule", but based on observations on how people behave,
> e.g.
>
>  * for a small patch like this that can be given a good review in 10
>    minutes or so, those who do not do so within the first 3 days
>    will probably not do so.
>
>  * once a reasonably thorough review is given, those who haven't
>    responded to the patch and do not have much else to say are
>    unlikely to respond.
>
>  * on the other hand, after such a review is given, those who do not
>    agree with the review tend to respond rather quickly, to get
>    their voice in before it becomes too late.
>
> I would say it would be good to start working on it right away and
> use a couple of days reviewing it yourself before posting it.

Re: [PATCH 1/1] [OUTREACHY] Fixed add.c file to conform to guidelines when using die() listed in issue #635

[Dragan Simic]

On 2023-10-11 07:50, Naomi Ibe wrote:
> Okay then, I'll work with these guidelines in mind. Thank you very much

If I may notice, avoiding top posting has already been suggested twice, 
IIRC.

I think that various webmails have made top posting popular, and 
actually forced people a bit to do that through the way their user 
interfaces were laid out, which made inline replies kind of a dying art, 
which is really sad to see.  It is so much more useful when replies are 
posted inline, i.e. right below the contents they refer to.


> On Tue, Oct 10, 2023 at 11:24 PM Junio C Hamano <gitster@pobox.com> 
> wrote:
>> 
>> Naomi Ibe <naomi.ibeh69@gmail.com> writes:
>> 
>> > Thank you very much! I'd definitely make those changes on my next patch.
>> 
>> [administrivia] do not top post.
>> 
>> > Should I begin work on version 2 or should I still wait for additional
>> > input on the version 1?
>> 
>> There is no "rule", but based on observations on how people behave,
>> e.g.
>> 
>>  * for a small patch like this that can be given a good review in 10
>>    minutes or so, those who do not do so within the first 3 days
>>    will probably not do so.
>> 
>>  * once a reasonably thorough review is given, those who haven't
>>    responded to the patch and do not have much else to say are
>>    unlikely to respond.
>> 
>>  * on the other hand, after such a review is given, those who do not
>>    agree with the review tend to respond rather quickly, to get
>>    their voice in before it becomes too late.
>> 
>> I would say it would be good to start working on it right away and
>> use a couple of days reviewing it yourself before posting it.