git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Thomas Gummerer <t.gummerer@gmail.com>
To: sushmaunnibhavi <sushmaunnibhavi425@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code
Date: Sun, 10 Mar 2019 18:26:04 +0000	[thread overview]
Message-ID: <20190310182604.GG31533@hank.intra.tgummerer.com> (raw)
In-Reply-To: <20190310143126.GA13588@hacker-queen>

> Subject: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code

Commit messages (and titles) should always be in the imperative mood.
The title in particular should be a short description of what the
patch is doing, and should give meaningful information to people
reading the output of 'git log --oneline'.  Also we usually prefix the
title with the area of the code we're touching.

The title above is very generic, and thus not very useful to future
readers.  Something like

    compat/regex: remove unnecessary if

would give future readers a bit more context.

On 03/10, sushmaunnibhavi wrote:
> From 5a6c233c6bf0a35aca000b32b9e936a34532900a Mon Sep 17 00:00:00 2001
> From: sushmaunnibhavi <sushmaunnibhavi@gmail.com>
> Date: Sun, 10 Mar 2019 19:37:33 +0530
> Subject: [GSOC][PATCH] Fixed an issue which contained extra unnecessary code

The four lines above don't need to be included here, as they are
already defined by the email headers.

The one line that may be useful here is "From: sushmaunnibhavi
<sushmaunnibhavi@gmail.com>".  That should be your full name, and
match the name in the Signed-off-by line.  Usually people just set
that up in the "From" header in the email, but if you are unable to
configure your mailer like that, including a "From: Sushma Unnibhavi
<sushmaunnibhavi425@gmail.com>" before the rest of the commit message
also works.

> Signed-off-by: Sushma Unnibhavi <sushmaunnibhavi425@gmail.com>
> ---
> Since '\n' and '\0' are char_len==1,it is not necessary to check if the char_len<=1.

This explanation of why this is a good patch should be included in the
commit message before your Signed-off-by.  If we want to apply this
change (which I don't think we want for the reasons stated below), I
think this needs a bit of a deeper analysis in the commit message, to
convince readers this is the correct thing to do.

Also please wrap commit messages at 72 characters.

>  compat/regex/regexec.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/compat/regex/regexec.c b/compat/regex/regexec.c

This file in compat/ was directly imported from upstream gawk.  We
generally don't patch this type of imported file, to make updates from
upstream easier, unless there is an actual fix from upstream that
needs to be fixed that's not going to be fixed upstream.

As this change at best removes a redundant 'if' (I can't comment on the
correctness, as I'm not familar with this code), so it is not worth
changing this file in our codebase.

> index 1b5d89fd5e..df62597531 100644
> --- a/compat/regex/regexec.c
> +++ b/compat/regex/regexec.c
> @@ -3799,11 +3799,6 @@ check_node_accept_bytes (const re_dfa_t *dfa, int node_idx,
>    char_len = re_string_char_size_at (input, str_idx);
>    if (node->type == OP_PERIOD)
>      {
> -      if (char_len <= 1)
> -	return 0;
> -      /* FIXME: I don't think this if is needed, as both '\n'
> -	 and '\0' are char_len == 1.  */
> -      /* '.' accepts any one character except the following two cases.  */
>        if ((!(dfa->syntax & RE_DOT_NEWLINE) &&
>  	   re_string_byte_at (input, str_idx) == '\n') ||
>  	  ((dfa->syntax & RE_DOT_NOT_NULL) &&
> -- 
> 2.17.1
> 

      parent reply	other threads:[~2019-03-10 18:26 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-10 14:31 [GSOC][PATCH] Fixed an issue which contained extra unnecessary code sushmaunnibhavi
2019-03-10 18:10 ` Christian Couder
2019-03-10 18:26 ` Thomas Gummerer [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190310182604.GG31533@hank.intra.tgummerer.com \
    --to=t.gummerer@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=sushmaunnibhavi425@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).