Simple dead assignment

PATCH In review Chris Wilson 5 participants 10 messages Jun 15, 2016

Share & tools

Chris Wilson

author

2016-06-15 22:51 UTC #0 lore raw

Hi Folks,

Sentry picked up this dead assignment commited yesterday in ba67aaf. I've provided a patch to remove it. It might also be a good idea to ask the author if that value was supposed to be used for something in particular before pulling it out.

Thanks,
Chris

Signature · 3 lines

Chris Wilson
http://vigilantsw.com/
Vigilant Software, LLC

sh-i18n--envsubst.c +0 -4

index 7125093..5829463 100644
@@ -67,9 +67,6 @@ static void subst_from_stdin (void);
 int
 main (int argc, char *argv[])
 {
-  /* Default values for command line options.  */
-  unsigned short int show_variables = 0;
-
   switch (argc)
 	{
 	case 1:

@@ -88,7 +85,6 @@ main (int argc, char *argv[])
 	  /* git sh-i18n--envsubst --variables '$foo and $bar' */
 	  if (strcmp(argv[1], "--variables"))
 		error ("first argument must be --variables when two are given");
-	  show_variables = 1;
       print_variables (argv[2]);
 	  break;
 	default:

Chris Wilson

author

2016-06-15 22:51 UTC #1 lore raw

PATCH 1/1 Simple dead assignment

Quoted 4 lines · Chris Wilson

Sentry picked up this dead assignment commited yesterday in ba67aaf. I've provided a patch to remove it. It might also be a good idea to ask the author if that value was supposed to be used for something in particular before pulling it out.

Oops, I see others putting the patches inline. Here you go.

Chris

Signature · 3 lines

Chris Wilson
http://vigilantsw.com/
Vigilant Software, LLC

sh-i18n--envsubst.c +0 -4

index 7125093..5829463 100644
@@ -67,9 +67,6 @@ static void subst_from_stdin (void);
 int
 main (int argc, char *argv[])
 {
-  /* Default values for command line options.  */
-  unsigned short int show_variables = 0;
-
   switch (argc)
 	{
 	case 1:

@@ -88,7 +85,6 @@ main (int argc, char *argv[])
 	  /* git sh-i18n--envsubst --variables '$foo and $bar' */
 	  if (strcmp(argv[1], "--variables"))
 		error ("first argument must be --variables when two are given");
-	  show_variables = 1;
       print_variables (argv[2]);
 	  break;
 	default:

Matthieu Moy

2016-06-15 22:51 UTC #2 lore raw

Quoted 1 line · Chris Wilson

Oops, I see others putting the patches inline. Here you go.

Please, read Documentation/SubmittingPatches. Especially read about signed-off-by and the way patches should be formatted (git send-email would help).

Signature · 2 lines

Matthieu Moy
http://www-verimag.imag.fr/~moy/

Chris Wilson

author

2016-06-15 22:51 UTC #3 lore raw

PATCH 1/1 Remove a dead assignment

Quoted 7 lines · Matthieu Moy

Quoted 1 line · Chris Wilson
Oops, I see others putting the patches inline. Here you go.
Please, read Documentation/SubmittingPatches. Especially read about signed-off-by and the way patches should be formatted (git send-email would help).

Thanks, trying this again. Like I said before, the author should investigate if this variable should have been used before removing it.

Signed-off-by Chris Wilson <[email protected]>

---
 sh-i18n--envsubst.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

sh-i18n--envsubst.c +0 -4

index 7125093..5829463 100644
@@ -67,9 +67,6 @@ static void subst_from_stdin (void);
 int
 main (int argc, char *argv[])
 {
-  /* Default values for command line options.  */
-  unsigned short int show_variables = 0;
-
   switch (argc)
        {
        case 1:

@@ -88,7 +85,6 @@ main (int argc, char *argv[])
          /* git sh-i18n--envsubst --variables '$foo and $bar' */
          if (strcmp(argv[1], "--variables"))
                error ("first argument must be --variables when two are given");
-         show_variables = 1;
       print_variables (argv[2]);
          break;
        default:

--
1.7.5.2.354.g19aea

Michael Schubert

2016-06-15 22:51 UTC #4 lore raw

Re: [PATCH] Remove a dead assignment

There already is a patch on its way:

http://article.gmane.org/gmane.comp.version-control.git/174378

Chris Wilson

author

2016-06-15 22:51 UTC #5 lore raw

PATCH 1/1 Remove a dead assignment

The assignment to fmt is dead and is also useless.

Signed-off-by Chris Wilson <[email protected]>

---

Quoted 3 lines · Michael Schubert

There already is a patch on its way:
http://article.gmane.org/gmane.comp.version-control.git/174378

Thanks! Well, I wasn't going to report this dead assignment since it wasn't done recently, but now I want to figure out how to properly submit a patch. :) Am I there yet? and thanks for the help.

Thanks,
CHris

 pretty.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

pretty.c +0 -1

index dff5c8d..5667c7f 100644
@@ -1082,7 +1082,6 @@ void userformat_find_requirements(const char *fmt, struct userformat_
        if (!fmt) {
                if (!user_format)
                        return;
-               fmt = user_format;
        }
        strbuf_expand(&dummy, user_format, userformat_want_item, w);
        strbuf_release(&dummy);

Signature · 1 line

1.7.5.2.354.g19aea

Junio C Hamano

2016-06-15 22:51 UTC #6 lore raw

Re: [PATCH] Remove a dead assignment

Quoted 3 lines · Chris Wilson

Thanks! Well, I wasn't going to report this dead assignment since it wasn't done recently, but now I want to figure out how to properly submit a patch. :) Am I there yet? and thanks for the help.

The compiler does not understand the meaning of the code, so after seeing such a "set but unused" statement, you should wonder why such a seemingly useless statement is there, before sending a mechanical patch to remove it without thinking things through.

Quoted 8 lines

 pretty.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

pretty.c +0 -0

index dff5c8d..5667c7f 100644

pretty.c void userformat_find_requirements(const char *fmt, struct userformat_ · line 1087

Quoted 7 lines

pretty.c +0 -1

@@ -1082,7 +1082,6 @@ void userformat_find_requirements(const char *fmt, struct userformat_
        if (!fmt) {
                if (!user_format)
                        return;
-               fmt = user_format;
        }
        strbuf_expand(&dummy, user_format, userformat_want_item, w);
        strbuf_release(&dummy);

The if statement says "we might be passing NULL in fmt and in that case please fall back to user_format" to human readers, but the compiler is too stupid to infer such an intention, so you have to help it with your brain. I have to wonder if the strbuf_expand() should be passing fmt instead of user_format. "git blame -L1082,+7 pretty.c" points at 5b16360 (pretty:
Initialize notes if %N is used, 2010-04-13).

The only callsite that is introduced by that patch passes NULL to fmt, so a better fix might be to do something like this instead.

 builtin/log.c |    3 +--
 commit.h      |    2 +-
 pretty.c      |   10 ++++------
 3 files changed, 6 insertions(+), 9 deletions(-)

builtin/log.c +1 -2

index f621990..9e05d46 100644
@@ -110,8 +110,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 	if (argc > 1)
 		die("unrecognized argument: %s", argv[1]);
 
-	memset(&w, 0, sizeof(w));
-	userformat_find_requirements(NULL, &w);
+	userformat_find_requirements(&w);
 
 	if (!rev->show_notes_given && (!rev->pretty_given || w.notes))
 		rev->show_notes = 1;

commit.h +1 -1

index 3114bd1..b652c22 100644
@@ -92,7 +92,7 @@ extern char *reencode_commit_message(const struct commit *commit,
 extern void get_commit_format(const char *arg, struct rev_info *);
 extern const char *format_subject(struct strbuf *sb, const char *msg,
 				  const char *line_separator);
-extern void userformat_find_requirements(const char *fmt, struct userformat_want *w);
+extern void userformat_find_requirements(struct userformat_want *w);
 extern void format_commit_message(const struct commit *commit,
 				  const char *format, struct strbuf *sb,
 				  const struct pretty_print_context *context);

pretty.c +4 -6

index dff5c8d..ca24925 100644
@@ -1075,15 +1075,13 @@ static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,
 	return 0;
 }
 
-void userformat_find_requirements(const char *fmt, struct userformat_want *w)
+void userformat_find_requirements(struct userformat_want *w)
 {
 	struct strbuf dummy = STRBUF_INIT;
 
-	if (!fmt) {
-		if (!user_format)
-			return;
-		fmt = user_format;
-	}
+	memset(w, 0, sizeof(*w));
+	if (!user_format)
+		return;
 	strbuf_expand(&dummy, user_format, userformat_want_item, w);
 	strbuf_release(&dummy);
 }

Junio C Hamano

2016-06-15 22:51 UTC #7 lore raw

Re: [PATCH] Remove a dead assignment

Quoted 9 lines · Junio C Hamano

The if statement says "we might be passing NULL in fmt and in that case please fall back to user_format" to human readers, but the compiler is too stupid to infer such an intention, so you have to help it with your brain. I have to wonder if the strbuf_expand() should be passing fmt instead of user_format. "git blame -L1082,+7 pretty.c" points at 5b16360 (pretty:
Initialize notes if %N is used, 2010-04-13).
The only callsite that is introduced by that patch passes NULL to fmt, so a better fix might be to do something like this instead.

If somebody cares about the reusability of the code for other callsites added in the future, we could do this instead.

I think this is what Johannes wanted to do from the beginning, and is a better fix than my previous one to remove the fmt parameter altogether.

-- >8 --
Subject: userformat_find_requirements(): find requirement for the correct format

This function was introduced in 5b16360 (pretty: Initialize notes if %N is used, 2010-04-13) to check what kind of information the "log --format=..." user format string wants. The function can be passed a NULL instead of a format string to ask it to check user_format variable kept by an earlier call to save_user_format().

But it unconditionally checked user_format and not the string it was given. The only caller introduced by the change passes NULL, which kept the bug unnoticed, until a new GCC noticed that there is an assignment to fmt that is never used.

Noticed-by Chris Wilson's compiler

Signed-off-by Junio C Hamano <[email protected]>

---
 pretty.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

pretty.c +1 -1

index dff5c8d..52174fd 100644
@@ -1084,7 +1084,7 @@ void userformat_find_requirements(const char *fmt, struct userformat_want *w)
 			return;
 		fmt = user_format;
 	}
-	strbuf_expand(&dummy, user_format, userformat_want_item, w);
+	strbuf_expand(&dummy, fmt, userformat_want_item, w);
 	strbuf_release(&dummy);
 }

Jeff King

Acked-by

2016-06-15 22:51 UTC #8 lore raw

Re: [PATCH] Remove a dead assignment

Quoted 9 lines · Junio C Hamano

Quoted 7 lines · Junio C Hamano
The if statement says "we might be passing NULL in fmt and in that case please fall back to user_format" to human readers, but the compiler is too stupid to infer such an intention, so you have to help it with your brain. I have to wonder if the strbuf_expand() should be passing fmt instead of user_format. "git blame -L1082,+7 pretty.c" points at 5b16360 (pretty:
Initialize notes if %N is used, 2010-04-13).

Quoted 8 lines

Quoted 2 lines
The only callsite that is introduced by that patch passes NULL to fmt, so a better fix might be to do something like this instead.
If somebody cares about the reusability of the code for other callsites added in the future, we could do this instead.
I think this is what Johannes wanted to do from the beginning, and is a better fix than my previous one to remove the fmt parameter altogether.

Actually, I am to blame for this interface, see:

http://article.gmane.org/gmane.comp.version-control.git/144650

and my response:

http://article.gmane.org/gmane.comp.version-control.git/144715

The resulting patch definitely has a bug (albeit one that could not be triggered by current users), and should have had this:

pretty.c void userformat_find_requirements(const char *fmt, struct userformat_want *w) · line 1087

Quoted 2 lines

pretty.c +1 -1

-	strbuf_expand(&dummy, user_format, userformat_want_item, w);
+	strbuf_expand(&dummy, fmt, userformat_want_item, w);

all along.

I'm also OK with just tightening the interface to always use user_format, as no callers who wanted the extra parameter have come up in the past year.

Quoted 8 lines

Subject: userformat_find_requirements(): find requirement for the correct format
This function was introduced in 5b16360 (pretty: Initialize notes if %N is used, 2010-04-13) to check what kind of information the "log --format=..." user format string wants. The function can be passed a NULL instead of a format string to ask it to check user_format variable kept by an earlier call to save_user_format().

Quoted 4 lines

But it unconditionally checked user_format and not the string it was given. The only caller introduced by the change passes NULL, which kept the bug unnoticed, until a new GCC noticed that there is an assignment to fmt that is never used.

Acked-by Jeff King <[email protected]>

Quoted 1 line

Noticed-by: Chris Wilson's compiler

Heh.

-Peff

Matthieu Moy

2016-06-15 22:51 UTC #9 lore raw

Re: [PATCH] Remove a dead assignment

In reply to Chris Wilson

Quoted 10 lines · Chris Wilson

Quoted 7 lines · Matthieu Moy
Quoted 1 line · Chris Wilson
Oops, I see others putting the patches inline. Here you go.
Please, read Documentation/SubmittingPatches. Especially read about signed-off-by and the way patches should be formatted (git send-email would help).
Thanks, trying this again.

Still not right ;-). The text here (above ---) will become the commit message when applied, and you don't want the commit message to mention this discussion. This discussion could have been added below ...

Quoted 5 lines

Like I said before, the author should investigate if this variable should have been used before removing it.
Signed-off-by Chris Wilson <[email protected]>
---

=> this is the place for informal discussions.

Quoted 2 lines

 sh-i18n--envsubst.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

If you don't follow this, then the maintainer has to do this manually, and we all prefer his time to be invested in better activities ;-).

Signature · 2 lines

Matthieu Moy
http://www-verimag.imag.fr/~moy/