[PATCH 1/4] daemon.c:handle: Remove unneeded check for null pointer.

addr doesn't need to be checked at that line as it it already accessed
7 lines before in the if (addr->sa_family).

Signed-off-by: Stefan Beller <redacted>
---
 daemon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/daemon.c b/daemon.c
index 6aeddcb..5e48c1e 100644
--- a/daemon.c
+++ b/daemon.c

@@ -754,19 +754,19 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
 	}
 
 	if (addr->sa_family == AF_INET) {
 		struct sockaddr_in *sin_addr = (void *) addr;
 		inet_ntop(addr->sa_family, &sin_addr->sin_addr, addrbuf + 12,
 		    sizeof(addrbuf) - 12);
 		snprintf(portbuf, sizeof(portbuf), "REMOTE_PORT=%d",
 		    ntohs(sin_addr->sin_port));
 #ifndef NO_IPV6
-	} else if (addr && addr->sa_family == AF_INET6) {
+	} else if (addr->sa_family == AF_INET6) {
 		struct sockaddr_in6 *sin6_addr = (void *) addr;
 
 		char *buf = addrbuf + 12;
 		*buf++ = '['; *buf = '\0'; /* stpcpy() is cool */
 		inet_ntop(AF_INET6, &sin6_addr->sin6_addr, buf,
 		    sizeof(addrbuf) - 13);
 		strcat(buf, "]");
 
 		snprintf(portbuf, sizeof(portbuf), "REMOTE_PORT=%d",

-- 
1.8.3.2.806.gdee5b9b

The date variable is assigned new memory via xmemdupz and 2 lines later
it is assigned new memory again via xmalloc, but the first assignment
is never freed nor used.

Signed-off-by: Stefan Beller <redacted>
---
 builtin/commit.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 790e5ab..00da83c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -528,19 +528,18 @@ static void determine_author_info(struct strbuf *author_ident)
 
 		if (lb == a + strlen("\nauthor "))
 			/* \nauthor <foo@example.com> */
 			name = xcalloc(1, 1);
 		else
 			name = xmemdupz(a + strlen("\nauthor "),
 					(lb - strlen(" ") -
 					 (a + strlen("\nauthor "))));
 		email = xmemdupz(lb + strlen("<"), rb - (lb + strlen("<")));
-		date = xmemdupz(rb + strlen("> "), eol - (rb + strlen("> ")));
 		len = eol - (rb + strlen("> "));
 		date = xmalloc(len + 2);
 		*date = '@';
 		memcpy(date + 1, rb + strlen("> "), len);
 		date[len + 1] = '\0';
 	}
 
 	if (force_author) {
 		const char *lb = strstr(force_author, " <");

-- 
1.8.3.2.806.gdee5b9b

Signed-off-by: Stefan Beller <redacted>
---
 diff-no-index.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index e66fdf3..842add4 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c

@@ -181,19 +181,18 @@ static int queue_diff(struct diff_options *o,
 	}
 }
 
 void diff_no_index(struct rev_info *revs,
 		   int argc, const char **argv,
 		   int nongit, const char *prefix)
 {
 	int i, prefixlen;
 	int no_index = 0;
-	unsigned options = 0;
 	const char *paths[2];
 
 	/* Were we asked to do --no-index explicitly? */
 	for (i = 1; i < argc; i++) {
 		if (!strcmp(argv[i], "--")) {
 			i++;
 			break;
 		}
 		if (!strcmp(argv[i], "--no-index"))

@@ -218,22 +217,20 @@ void diff_no_index(struct rev_info *revs,
 	if (argc != i + 2)
 		usagef("git diff %s <path> <path>",
 		       no_index ? "--no-index" : "[--no-index]");
 
 	diff_setup(&revs->diffopt);
 	for (i = 1; i < argc - 2; ) {
 		int j;
 		if (!strcmp(argv[i], "--no-index"))
 			i++;
-		else if (!strcmp(argv[i], "-q")) {
-			options |= DIFF_SILENT_ON_REMOVED;
+		else if (!strcmp(argv[i], "-q"))
 			i++;
-		}
 		else if (!strcmp(argv[i], "--"))
 			i++;
 		else {
 			j = diff_opt_parse(&revs->diffopt, argv + i, argc - i);
 			if (!j)
 				die("invalid diff option/value: %s", argv[i]);
 			i += j;
 		}
 	}

-- 
1.8.3.2.806.gdee5b9b

Signed-off-by: Stefan Beller <redacted>
---
 diff.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index e53ddad..24382d7 100644
--- a/diff.c
+++ b/diff.c

@@ -1677,21 +1677,19 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 		}
 
 		/*
 		 * scale the add/delete
 		 */
 		add = added;
 		del = deleted;
 
 		if (graph_width <= max_change) {
-			int total = add + del;
-
-			total = scale_linear(add + del, graph_width, max_change);
+			int total = scale_linear(add + del, graph_width, max_change);
 			if (total < 2 && add && del)
 				/* width >= 2 due to the sanity check */
 				total = 2;
 			if (add < del) {
 				add = scale_linear(add, graph_width, max_change);
 				del = total - add;
 			} else {
 				del = scale_linear(del, graph_width, max_change);
 				add = total - del;

-- 
1.8.3.2.806.gdee5b9b

Hi,

Stefan Beller wrote:

addr doesn't need to be checked at that line as it it already accessed
7 lines before in the if (addr->sa_family).

Good catch.  This asymmetry has been present since the lines were first
introduced (all guarded by "if (addr)") in v1.4.1-rc1~3^2~4 (Log peer
address when git-daemon called from inetd, 2006-06-20).

At this point 'addr' is &ss.sa from service_loop, so it really cannot
be NULL.

So fwiw, I like this patch.

Stefan Beller wrote:

Signed-off-by: Stefan Beller <redacted>

Thanks.  That was quick. :)

Reviewed-by: Jonathan Nieder <redacted>

Stefan Beller wrote:

[Subject: diff-no-index: Remove unused variable.]

[...]

[...]

-		else if (!strcmp(argv[i], "-q")) {
+		else if (!strcmp(argv[i], "-q"))
-			options |= DIFF_SILENT_ON_REMOVED;
 			i++;
-		}

This feature was obviously never tested with --no-index, so I agree it
makes sense to remove it.  Probably the commit message and a comment
should say so, though.  E.g.:

	diff --no-index: remove nonfunctional "-q" handling

	Before v1.5.6-rc1~41^2~2, the option parsing for diff --no-index
	and "git diff-files" shared code.  In "git diff-files", "-q" means
	to be silent about removed files.  In "git diff --no-index", in
	various versions it has been an error, an infinite loop, or a no-op.

	Simplify the code to clarify that it is now a no-op, continuing to
	accept and ignore the -q option in "git diff --no-index" to avoid
	breaking scripts.

I wouldn't mind removing support for "-q" altogether, by the way (as a
separate change).

Hope that helps,
Jonathan

Stefan Beller wrote:

Signed-off-by: Stefan Beller <redacted>
---
 diff.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

[...]

Yeah, we should have caught this in review.

Thanks for reporting.
Reviewed-by: Jonathan Nieder <redacted>

On 07/15/2013 12:07 AM, Jonathan Nieder wrote:

This feature was obviously never tested with --no-index, so I agree it
makes sense to remove it.  Probably the commit message and a comment
should say so, though.  E.g.:

      diff --no-index: remove nonfunctional "-q" handling

      Before v1.5.6-rc1~41^2~2, the option parsing for diff --no-index
      and "git diff-files" shared code.  In "git diff-files", "-q" means
      to be silent about removed files.  In "git diff --no-index", in
      various versions it has been an error, an infinite loop, or a no-op.

      Simplify the code to clarify that it is now a no-op, continuing to
      accept and ignore the -q option in "git diff --no-index" to avoid
      breaking scripts.

I wouldn't mind removing support for "-q" altogether, by the way (as a
separate change).

Hope that helps,
Jonathan

I am resending the commit with a different wording, thanks to Jonathan.   

However I tried to remove support for -q in a separate commit, and
I have some questions about the structure of the files.
(I am sure it's documented, but I cannot find it, so please hint me 
where to read.)

The changes in the following patch are in diff_no_index.c, but the
diff_no_index(...) is called from cmd_diff, which is in builtin/diff.c
That cmd_diff is actually called from git.c having the
{ "diff", cmd_diff }, entry in handle_internal_command.

My question now is this: Why is the builtin/diff.c relying on stuff
outside of builtin/ ? Wouldn't it be better to move all these files
(such as diff_no_index.c) into the builtin folder as well?

Regarding the removal of the -q option, I tried it in the second patch.
Is it as easy as that, or am I missing the point?

The first patch doesn't change the behavior, so I'd assume it's safe to 
apply it to origin/sb/misc-fixes, whereas the second patch will make 
git diff complain about the -q option, so I'd assume it would wait for the
next major release?

Before:
	touch actual_file
	git diff -q  actual_file no_file
	error: Could not access 'no_file'
	echo $?
	1

After:
	touch actual_file
	git diff -q  actual_file no_file
	fatal: invalid diff option/value: -q
	echo $?
	128

Thanks,
Stefan

Stefan Beller (2):
  diff --no-index: remove nonfunctional "-q" handling
  git diff: Remove -q option to stay silent on missing files.

 Documentation/git-diff-files.txt | 6 +-----
 diff-no-index.c                  | 5 -----
 2 files changed, 1 insertion(+), 10 deletions(-)

-- 
1.8.2.3.10.g2733812

Before v1.5.6-rc1~41^2~2, the option parsing for diff --no-index
and "git diff-files" shared code.  In "git diff-files", "-q" means
to be silent about removed files.  In "git diff --no-index", in
various versions it has been an error, an infinite loop, or a no-op.

Simplify the code to clarify that it is now a no-op, continuing to
accept and ignore the -q option in "git diff --no-index" to avoid
breaking scripts.

Signed-off-by: Stefan Beller <redacted>
Helped-by: Jonathan Nieder [off-list ref]
---
 diff-no-index.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index 74da659..419cd78 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c

@@ -187,7 +187,6 @@ void diff_no_index(struct rev_info *revs,
 {
 	int i, prefixlen;
 	int no_index = 0;
-	unsigned options = 0;
 	const char *paths[2];
 
 	/* Were we asked to do --no-index explicitly? */

@@ -224,10 +223,8 @@ void diff_no_index(struct rev_info *revs,
 		int j;
 		if (!strcmp(argv[i], "--no-index"))
 			i++;
-		else if (!strcmp(argv[i], "-q")) {
-			options |= DIFF_SILENT_ON_REMOVED;
+		else if (!strcmp(argv[i], "-q"))
 			i++;
-		}
 		else if (!strcmp(argv[i], "--"))
 			i++;
 		else {

-- 
1.8.2.3.10.g2733812

This feature was not tested in the test suite, hence we'd remove it for
now. It doesn't seem to be often used anyway.
A google search for "git diff -q" (match string exactly) only returned
http://stackoverflow.com/questions/11021287/git-detect-if-there-are-untracked-files-quickly
where "git diff -q" was quoted for its exit code behavior regarding files
being found or not.

Signed-off-by: Stefan Beller <redacted>
Proposed-by: <Jonathan Nieder> [off-list ref]
---
 Documentation/git-diff-files.txt | 6 +-----
 diff-no-index.c                  | 2 --
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/Documentation/git-diff-files.txt b/Documentation/git-diff-files.txt
index 906774f..d118cfb 100644
--- a/Documentation/git-diff-files.txt
+++ b/Documentation/git-diff-files.txt

@@ -9,7 +9,7 @@ git-diff-files - Compares files in the working tree and the index
 SYNOPSIS
 --------
 [verse]
-'git diff-files' [-q] [-0|-1|-2|-3|-c|--cc] [<common diff options>] [<path>...]
+'git diff-files' [-0|-1|-2|-3|-c|--cc] [<common diff options>] [<path>...]
 
 DESCRIPTION
 -----------

@@ -41,10 +41,6 @@ omit diff output for unmerged entries and just show "Unmerged".
 	diff, similar to the way 'diff-tree' shows a merge
 	commit with these flags.
 
--q::
-	Remain silent even on nonexistent files
-
-
 include::diff-format.txt[]
 
 GIT

diff --git a/diff-no-index.c b/diff-no-index.c
index 419cd78..98a9cf1 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c

@@ -223,8 +223,6 @@ void diff_no_index(struct rev_info *revs,
 		int j;
 		if (!strcmp(argv[i], "--no-index"))
 			i++;
-		else if (!strcmp(argv[i], "-q"))
-			i++;
 		else if (!strcmp(argv[i], "--"))
 			i++;
 		else {

-- 
1.8.2.3.10.g2733812