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

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

[Stefan Beller]

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 <stefanbeller@googlemail.com>
---
 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

[PATCH 2/4] commit: Fix a memory leak in determine_author_info

[Stefan Beller]

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 <stefanbeller@googlemail.com>
---
 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

Re: [PATCH 2/4] commit: Fix a memory leak in determine_author_info

[Jonathan Nieder]

Stefan Beller wrote:

> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>

Thanks.  That was quick. :)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[PATCH 3/4] diff-no-index: Remove unused variable.

[Stefan Beller]

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 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

Re: [PATCH 3/4] diff-no-index: Remove unused variable.

[Jonathan Nieder]

Stefan Beller wrote:

> [Subject: diff-no-index: Remove unused variable.]
[...]
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
[...]
> -		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

[PATCH 0/2] git diff -q option removal

[Stefan Beller]

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

[PATCH 1/2] diff --no-index: remove nonfunctional "-q" handling

[Stefan Beller]

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 <stefanbeller@googlemail.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
---
 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

[PATCH 2/2] diff: Remove -q to stay silent on missing files.

[Stefan Beller]

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 <stefanbeller@googlemail.com>
Proposed-by: <Jonathan Nieder> <jrnieder@gmail.com>
---
 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

Re: [PATCH 0/2] git diff -q option removal

[Junio C Hamano]

Stefan Beller <stefanbeller@googlemail.com> writes:

> 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?

Builtins link all sorts of stuff from outside, e.g. diff.c and
diffcore-*.c at the toplevel.  I do not see diff_no_index.c is any
different, so I am probably not understanding your question.

> 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'

Hmm, do you really get that error message?  I think you would get

    fatal: ambiguous argument 'no_file': unknown revision or path not in the working tree.

> 	echo $?
> 	1

The command line parsing infrastructure has changed vastly since
"show-diff" days (see below for a history lesson); I think your
"Before" should read more like this

	git diff -q -- actual_file no_file

and it should not show removal of no_file in its output.  E.g. in
git.git

	$ git reset --hard
        $ rm COPYING
        $ git diff -q -- COPYING

should show nothing.

I personally think "-q" no longer makes sense in today's codebase,
but I am not convinced that removal of '-q' from the proper "git
diff-files" and the "git diff --no-index" (aka "I am too lazy to
teach our diff enhancement to other people's diff implementations,
so let's throw in a "files do not have to be tracked in Git
repository at all" mode") is the right direction to go.

The "-q" option is a remnant from the "show-diff" command, the
precursor of today's "git diff-files" (back then, we didn't even
have "git" potty.  The user literally typed "show-diff", not "git
show-diff").

ca2a0798 ([PATCH] Add "-q" option to show-diff.c, 2005-04-15) added
that option.  Back then, we did not have pathspec matching, and we
iterated over command line arguments, and required all of them exist
as filesystem entities.  "-q" was a way to defeat that "you name a
file, it must exist in the working tree" safety, and also at the
same time not give output for such a file that was removed from the
working tree.

These days, the former "safety" is enforced by the generalized
revision parser ("is it a path or is it a rev?") code and the "--"
delimiter on the command line is the way to defeat it.  The latter
is done by giving a filtering specification that lack D to the
"--diff-filter".

If we wanted to make "-q" follow the spirit of its original addition
to "show-diff" again, we could internally add a diff-filter when the
"-q" option is parsed.

"git diff -q ..." is "git diff --diff-filter=ACMRTUB ...", and "git
diff -q --diff-filter=AD" is "git diff --diff-filter=A".  That would
let us remove the special case for SILENT_ON_REMOVED, and also make
"-q" work across various commands in the "diff" family.  It might
even make it work for "diff --no-index", but I didn't bother to
check.

> 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(-)

Re: [PATCH 0/2] git diff -q option removal

[Junio C Hamano]

On Wed, Jul 17, 2013 at 10:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> If we wanted to make "-q" follow the spirit of its original addition
> to "show-diff" again, we could internally add a diff-filter when the
> "-q" option is parsed.

Having said all that, I do not mean to advocate to retain "-q". Most
likely nobody uses it, and "-q" is grossly misnamed ("why is it so
special to be "quiet" only for removals?"). As long as we mention its
removal in the release notes (and possibly tell those miniscule
minority that wants to ignore deleted files to use --diff-filter
instead), we should be OK.

Independently, we might want to enhance --diff-filter parser to make
it easier to say "I want everything but D", perhaps use lowercase
letter to subtract from what have been specified so far (or if there
is no uppercase letter, start from "everything"), so that we can say
--diff-filter=d
or something.

Re: [PATCH 0/2] git diff -q option removal

[Stefan Beller]

[-- Attachment #1: Type: text/plain, Size: 2956 bytes --]

On 07/17/2013 07:04 PM, Junio C Hamano wrote:

> Builtins link all sorts of stuff from outside, e.g. diff.c and
> diffcore-*.c at the toplevel.  I do not see diff_no_index.c is any
> different, so I am probably not understanding your question.

Thanks for the explanation. I am not yet very used to gits code structure. So I sometimes think of 'how would I structure things', so I
was confused of things in builtin using some parts outside of it.
Maybe that folder raised to much expectations for me to be 'the real'
core, whereas the files outside, i.e. those files in the top level
directory, are just there for other things or scripts.
This understanding of the structure seems obviously wrong now.

Thanks for clarification.

> 
> Hmm, do you really get that error message?  I think you would get
> 
>     fatal: ambiguous argument 'no_file': unknown revision or path not in the working tree.
> 
>> 	echo $?
>> 	1

Ok here we go (using current origin/master 9c3c367):

	cd $(mktemp -d)
	echo "test" > actual_file
	git diff actual_file no_file
	# error: Could not access 'no_file'
	echo $?
	1
	
	## I get the same message as well, if I'm using -- or not.
	## also the -q doesn't make a change

	git init 
	git diff -q -- actual_file no_file
	echo $?
	# 0
	git diff  -- actual_file no_file
	echo $?
	# 0
	git diff  actual_file no_file
	# fatal: no_file: no such path in the working tree.
	# Use 'git <command> -- <path>...' to specify paths that do not exist locally.
	echo $?
	# 128
	git diff -q  actual_file no_file
	# fatal: no_file: no such path in the working tree.
	# Use 'git <command> -- <path>...' to specify paths that do not exist locally.
	echo $?
	128

So apparently git diff behaves differently if not in a repo, which is what I tested.

> 
> The command line parsing infrastructure has changed vastly since
> "show-diff" days (see below for a history lesson);

A very interesting read, much appreciated. :)

> 
> If we wanted to make "-q" follow the spirit of its original addition
> to "show-diff" again, we could internally add a diff-filter when the
> "-q" option is parsed.
> 

I'm rather new to the project, so my opinion may not have much weight,
I'll state it anyway:
Keeping backwards compatibility is really hard, because you need the 
knowledge of such history lessons as read above, to understand what should
happen, like having an intuitive feeling about such parameters. Hence
maintaining/evolving the project will become harder and harder 
(specially for newcomers). So having one and only one way to achieve the desired
output, which fits into the greater structure as it's the case with --diff-filter
is easier to remember for both the user and developers.

Hence I think keeping the -q option would only make sense for the plumber 
layer, because there the explicit promise was given to not change stuff
every other release. 

Stefan




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

[PATCH 0/6] Deprecating "diff-files -q"

[Junio C Hamano]

The "-q" option given to "git diff-files" is a remnant of the
"show-diff" command, the precursor of today's "git diff-files" (back
then, we didn't even have "git" potty.  The user literally typed
"show-diff", not "git show-diff").

ca2a0798 ([PATCH] Add "-q" option to show-diff.c, 2005-04-15) added
that option.  Back then, we did not have pathspec matching, and we
iterated over command line arguments, and required all of them exist
as filesystem entities.  "-q" was a way to defeat that "you name a
file, it must exist in the working tree" safety, and also at the
same time not give output for such a file that was removed from the
working tree.

These days, the command line parsing infrastructure has changed
vastly since "show-diff" days, and the former "safety" is enforced
by the generalized revision parser ("is it a path or is it a rev?")
code and the "--" delimiter on the command line is the way to defeat
it.  The latter is done by giving a filtering specification that
lack D to the "--diff-filter", e.g. "--diff-filter=ACMRTUB".

This is however a bit cumbersome to type.  This miniseries updates
the diff-filter mechanism to let you say --diff-filter=d (lowercase)
to express that you are interested in the changes in general, but
not the changes in the 'D' class (i.e. deletion).

The last step tweaks the command line parser of "git diff-files"
(and "git diff" without any object on the command line, which goes
to the same codepath) and "git diff --no-index" to notice "-q", warn
and then turn it into "--diff-filter=d".  We should remove the
entire thing at a major version bump, like Git 2.0.


This is still a bit rough, without any documentation updates nor
tests.


Junio C Hamano (6):
  diff: pass the whole diff_options to diffcore_apply_filter()
  diff: factor out match_filter()
  diff: preparse --diff-filter string argument
  diff: reject unknown change class given to --diff-filter
  diff: allow lowercase letter to specify what change class to exclude
  diff: deprecate -q option to diff-files

 diff-lib.c      |   8 ++--
 diff-no-index.c |   7 +++-
 diff.c          | 125 ++++++++++++++++++++++++++++++++++++++++++++++----------
 diff.h          |   7 +++-
 4 files changed, 118 insertions(+), 29 deletions(-)

-- 
1.8.3.3-962-gf04df43

[PATCH 1/6] diff: pass the whole diff_options to diffcore_apply_filter()

[Junio C Hamano]

The --diff-filter=<arg> option given by the user is kept as a
string, and passed to the underlying diffcore_apply_filter()
function as a string for each resulting path we run number of
strchr() to see if each class of change among ACDMRTXUB is meant to
be given.

Change the function signature to pass the whole diff_options, so
that we can pre-parse this string in the next patch.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 649ec86..41c64f2 100644
--- a/diff.c
+++ b/diff.c
@@ -4509,11 +4509,13 @@ free_queue:
 	}
 }
 
-static void diffcore_apply_filter(const char *filter)
+static void diffcore_apply_filter(struct diff_options *options)
 {
 	int i;
 	struct diff_queue_struct *q = &diff_queued_diff;
 	struct diff_queue_struct outq;
+	const char *filter = options->filter;
+
 	DIFF_QUEUE_CLEAR(&outq);
 
 	if (!filter)
@@ -4661,7 +4663,7 @@ void diffcore_std(struct diff_options *options)
 	if (!options->found_follow)
 		/* See try_to_follow_renames() in tree-diff.c */
 		diff_resolve_rename_copy();
-	diffcore_apply_filter(options->filter);
+	diffcore_apply_filter(options);
 
 	if (diff_queued_diff.nr && !DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))
 		DIFF_OPT_SET(options, HAS_CHANGES);
-- 
1.8.3.3-962-gf04df43

[PATCH 2/6] diff: factor out match_filter()

[Junio C Hamano]

diffcore_apply_filter() checks if a filepair matches the filter
given with the "--diff-filter" option for each input filepairs with
a fairly complex expression in two places.

Create a helper function and call it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/diff.c b/diff.c
index 41c64f2..0220c19 100644
--- a/diff.c
+++ b/diff.c
@@ -4509,6 +4509,17 @@ free_queue:
 	}
 }
 
+static int match_filter(const struct diff_options *options, const struct diff_filepair *p)
+{
+	return (((p->status == DIFF_STATUS_MODIFIED) &&
+		 ((p->score &&
+		   strchr(options->filter, DIFF_STATUS_FILTER_BROKEN)) ||
+		  (!p->score &&
+		   strchr(options->filter, DIFF_STATUS_MODIFIED)))) ||
+		((p->status != DIFF_STATUS_MODIFIED) &&
+		 strchr(options->filter, p->status)));
+}
+
 static void diffcore_apply_filter(struct diff_options *options)
 {
 	int i;
@@ -4524,14 +4535,7 @@ static void diffcore_apply_filter(struct diff_options *options)
 	if (strchr(filter, DIFF_STATUS_FILTER_AON)) {
 		int found;
 		for (i = found = 0; !found && i < q->nr; i++) {
-			struct diff_filepair *p = q->queue[i];
-			if (((p->status == DIFF_STATUS_MODIFIED) &&
-			     ((p->score &&
-			       strchr(filter, DIFF_STATUS_FILTER_BROKEN)) ||
-			      (!p->score &&
-			       strchr(filter, DIFF_STATUS_MODIFIED)))) ||
-			    ((p->status != DIFF_STATUS_MODIFIED) &&
-			     strchr(filter, p->status)))
+			if (match_filter(options, q->queue[i]))
 				found++;
 		}
 		if (found)
@@ -4549,14 +4553,7 @@ static void diffcore_apply_filter(struct diff_options *options)
 		/* Only the matching ones */
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
-
-			if (((p->status == DIFF_STATUS_MODIFIED) &&
-			     ((p->score &&
-			       strchr(filter, DIFF_STATUS_FILTER_BROKEN)) ||
-			      (!p->score &&
-			       strchr(filter, DIFF_STATUS_MODIFIED)))) ||
-			    ((p->status != DIFF_STATUS_MODIFIED) &&
-			     strchr(filter, p->status)))
+			if (match_filter(options, p))
 				diff_q(&outq, p);
 			else
 				diff_free_filepair(p);
-- 
1.8.3.3-962-gf04df43

[PATCH 3/6] diff: preparse --diff-filter string argument

[Junio C Hamano]

Instead of running strchr() on the list of status characters over
and over again, parse the --diff-filter option into bitfields and
use the bits to see if the change to the filepair matches the status
requested.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------
 diff.h |  5 ++++-
 2 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/diff.c b/diff.c
index 0220c19..03f10e6 100644
--- a/diff.c
+++ b/diff.c
@@ -3496,6 +3496,53 @@ static int parse_submodule_opt(struct diff_options *options, const char *value)
 	return 1;
 }
 
+static const char diff_status_letters[] = {
+	DIFF_STATUS_ADDED,
+	DIFF_STATUS_COPIED,
+	DIFF_STATUS_DELETED,
+	DIFF_STATUS_MODIFIED,
+	DIFF_STATUS_RENAMED,
+	DIFF_STATUS_TYPE_CHANGED,
+	DIFF_STATUS_UNKNOWN,
+	DIFF_STATUS_UNMERGED,
+	DIFF_STATUS_FILTER_AON,
+	DIFF_STATUS_FILTER_BROKEN,
+	'\0',
+};
+
+static unsigned int filter_bit['Z' + 1];
+
+static void prepare_filter_bits(void)
+{
+	int i;
+
+	if (!filter_bit[DIFF_STATUS_ADDED]) {
+		for (i = 0; diff_status_letters[i]; i++)
+			filter_bit[(int) diff_status_letters[i]] = (1 << i);
+	}
+}
+
+static unsigned filter_bit_tst(char status, const struct diff_options *opt)
+{
+	return opt->filter & filter_bit[(int) status];
+}
+
+static int parse_diff_filter_opt(const char *optarg, struct diff_options *opt)
+{
+	int i, optch;
+
+	prepare_filter_bits();
+	for (i = 0; (optch = optarg[i]) != '\0'; i++) {
+		unsigned int bit;
+
+		bit = (0 <= optch && optch <= 'Z') ? filter_bit[optch] : 0;
+		if (!bit)
+			continue; /* ignore unknown ones, like we always have */
+		opt->filter |= bit;
+	}
+	return 0;
+}
+
 int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 {
 	const char *arg = av[0];
@@ -3717,7 +3764,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		return argcount;
 	}
 	else if ((argcount = parse_long_opt("diff-filter", av, &optarg))) {
-		options->filter = optarg;
+		int offending = parse_diff_filter_opt(optarg, options);
+		if (offending)
+			die("unknown change class '%c' in --diff-filter=%s",
+			    offending, optarg);
 		return argcount;
 	}
 	else if (!strcmp(arg, "--abbrev"))
@@ -4513,11 +4563,11 @@ static int match_filter(const struct diff_options *options, const struct diff_fi
 {
 	return (((p->status == DIFF_STATUS_MODIFIED) &&
 		 ((p->score &&
-		   strchr(options->filter, DIFF_STATUS_FILTER_BROKEN)) ||
+		   filter_bit_tst(DIFF_STATUS_FILTER_BROKEN, options)) ||
 		  (!p->score &&
-		   strchr(options->filter, DIFF_STATUS_MODIFIED)))) ||
+		   filter_bit_tst(DIFF_STATUS_MODIFIED, options)))) ||
 		((p->status != DIFF_STATUS_MODIFIED) &&
-		 strchr(options->filter, p->status)));
+		 filter_bit_tst(p->status, options)));
 }
 
 static void diffcore_apply_filter(struct diff_options *options)
@@ -4525,14 +4575,13 @@ static void diffcore_apply_filter(struct diff_options *options)
 	int i;
 	struct diff_queue_struct *q = &diff_queued_diff;
 	struct diff_queue_struct outq;
-	const char *filter = options->filter;
 
 	DIFF_QUEUE_CLEAR(&outq);
 
-	if (!filter)
+	if (!options->filter)
 		return;
 
-	if (strchr(filter, DIFF_STATUS_FILTER_AON)) {
+	if (filter_bit_tst(DIFF_STATUS_FILTER_AON, options)) {
 		int found;
 		for (i = found = 0; !found && i < q->nr; i++) {
 			if (match_filter(options, q->queue[i]))
diff --git a/diff.h b/diff.h
index 78b4091..a367207 100644
--- a/diff.h
+++ b/diff.h
@@ -103,12 +103,15 @@ enum diff_words_type {
 };
 
 struct diff_options {
-	const char *filter;
 	const char *orderfile;
 	const char *pickaxe;
 	const char *single_follow;
 	const char *a_prefix, *b_prefix;
 	unsigned flags;
+
+	/* diff-filter bits */
+	unsigned int filter;
+
 	int use_color;
 	int context;
 	int interhunkcontext;
-- 
1.8.3.3-962-gf04df43

[PATCH 4/6] diff: reject unknown change class given to --diff-filter

[Junio C Hamano]

We used to accept "git diff --diff-filter=Q" (note that there is no
such change class 'Q') silently and showed no output (because there
is no such change class 'Q').

Error out when such an input is given.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 03f10e6..3d37b56 100644
--- a/diff.c
+++ b/diff.c
@@ -3537,7 +3537,7 @@ static int parse_diff_filter_opt(const char *optarg, struct diff_options *opt)
 
 		bit = (0 <= optch && optch <= 'Z') ? filter_bit[optch] : 0;
 		if (!bit)
-			continue; /* ignore unknown ones, like we always have */
+			return optarg[i];
 		opt->filter |= bit;
 	}
 	return 0;
-- 
1.8.3.3-962-gf04df43

[PATCH 5/6] diff: allow lowercase letter to specify what change class to exclude

[Junio C Hamano]

In order to express "we do not care about deletions", we had to say
"--diff-filter=ACMRTXUB", giving all the possible change class
except for the one we do not want, "D".

This is cumbersome.  As all the change classes are in uppercase,
allow their lowercase counterpart to selectively exclude the class
from the output.  When such a negated change class is in the input,
start the filter option with the full bits set.

This would allow us to express the old "show-diff -q" with
"git diff-files --diff-filter=d".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 3d37b56..2d0b5e3 100644
--- a/diff.c
+++ b/diff.c
@@ -3532,13 +3532,40 @@ static int parse_diff_filter_opt(const char *optarg, struct diff_options *opt)
 	int i, optch;
 
 	prepare_filter_bits();
+
+	/*
+	 * If there is a negation e.g. 'd' in the input, and we haven't
+	 * initialized the filter field with another --diff-filter, start
+	 * from full set of bits, except for AON.
+	 */
+	if (!opt->filter) {
+		for (i = 0; (optch = optarg[i]) != '\0'; i++) {
+			if (optch < 'a' || 'z' < optch)
+				continue;
+			opt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;
+			opt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];
+			break;
+		}
+	}
+
 	for (i = 0; (optch = optarg[i]) != '\0'; i++) {
 		unsigned int bit;
+		int negate;
+
+		if ('a' <= optch && optch <= 'z') {
+			negate = 1;
+			optch = toupper(optch);
+		} else {
+			negate = 0;
+		}
 
 		bit = (0 <= optch && optch <= 'Z') ? filter_bit[optch] : 0;
 		if (!bit)
 			return optarg[i];
-		opt->filter |= bit;
+		if (negate)
+			opt->filter &= ~bit;
+		else
+			opt->filter |= bit;
 	}
 	return 0;
 }
-- 
1.8.3.3-962-gf04df43

[PATCH 6/6] diff: deprecate -q option to diff-files

[Junio C Hamano]

This reimplements the ancient "-q" option to "git diff-files" that
was inherited from "show-diff -q" in terms of "--diff-filter=d", and
issue a warning against the use of the former.

Incidentally this also tentatively fix "git diff --no-index" to
honor "-q" and hide deletions; the use will get the same warning.

We should remove the support for "-q" in Git 2.0.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff-lib.c      | 8 +++-----
 diff-no-index.c | 7 +++++--
 diff.c          | 8 ++++++++
 diff.h          | 2 ++
 4 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index f35de0f..4634b29 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -86,10 +86,12 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 {
 	int entries, i;
 	int diff_unmerged_stage = revs->max_count;
-	int silent_on_removed = option & DIFF_SILENT_ON_REMOVED;
 	unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
 			      ? CE_MATCH_RACY_IS_DIRTY : 0);
 
+	if (option & DIFF_SILENT_ON_REMOVED)
+		handle_deprecated_show_diff_q(&revs->diffopt);
+
 	diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
 
 	if (diff_unmerged_stage < 0)
@@ -136,8 +138,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 					perror(ce->name);
 					continue;
 				}
-				if (silent_on_removed)
-					continue;
 				wt_mode = 0;
 			}
 			dpath->mode = wt_mode;
@@ -203,8 +203,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 				perror(ce->name);
 				continue;
 			}
-			if (silent_on_removed)
-				continue;
 			diff_addremove(&revs->diffopt, '-', ce->ce_mode,
 				       ce->sha1, !is_null_sha1(ce->sha1),
 				       ce->name, 0);
diff --git a/diff-no-index.c b/diff-no-index.c
index 74da659..a788a5f 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -187,7 +187,7 @@ void diff_no_index(struct rev_info *revs,
 {
 	int i, prefixlen;
 	int no_index = 0;
-	unsigned options = 0;
+	unsigned deprecated_show_diff_q_option_used = 0;
 	const char *paths[2];
 
 	/* Were we asked to do --no-index explicitly? */
@@ -225,7 +225,7 @@ void diff_no_index(struct rev_info *revs,
 		if (!strcmp(argv[i], "--no-index"))
 			i++;
 		else if (!strcmp(argv[i], "-q")) {
-			options |= DIFF_SILENT_ON_REMOVED;
+			deprecated_show_diff_q_option_used = 1;
 			i++;
 		}
 		else if (!strcmp(argv[i], "--"))
@@ -260,6 +260,9 @@ void diff_no_index(struct rev_info *revs,
 	revs->max_count = -2;
 	diff_setup_done(&revs->diffopt);
 
+	if (deprecated_show_diff_q_option_used)
+		handle_deprecated_show_diff_q(&revs->diffopt);
+
 	setup_diff_pager(&revs->diffopt);
 	DIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);
 
diff --git a/diff.c b/diff.c
index 2d0b5e3..78819ba 100644
--- a/diff.c
+++ b/diff.c
@@ -3570,6 +3570,14 @@ static int parse_diff_filter_opt(const char *optarg, struct diff_options *opt)
 	return 0;
 }
 
+/* Used only by "diff-files" and "diff --no-index" */
+void handle_deprecated_show_diff_q(struct diff_options *opt)
+{
+	warning("'diff -q' and 'diff-files -q' are deprecated.");
+	warning("Use 'diff --diff-filter=d' instead to ignore deleted filepairs.");
+	parse_diff_filter_opt("d", opt);
+}
+
 int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 {
 	const char *arg = av[0];
diff --git a/diff.h b/diff.h
index a367207..5237d63 100644
--- a/diff.h
+++ b/diff.h
@@ -341,6 +341,8 @@ extern int parse_rename_score(const char **cp_p);
 
 extern long parse_algorithm_value(const char *value);
 
+extern void handle_deprecated_show_diff_q(struct diff_options *);
+
 extern int print_stat_summary(FILE *fp, int files,
 			      int insertions, int deletions);
 extern void setup_diff_pager(struct diff_options *);
-- 
1.8.3.3-962-gf04df43

[PATCH 7/6] diff: remove "diff-files -q" at Git 2.0 version boundary

[Junio C Hamano]

This was inherited from "show-diff -q" that was invented to tell
comparison between the index and the working tree to ignore only
removals in 2005.

These days, it is spelled as "--diff-filter=d".

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

 * And this is the endgame.

 diff-lib.c      | 3 ---
 diff-no-index.c | 8 --------
 diff.c          | 8 --------
 diff.h          | 2 --
 4 files changed, 21 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 4634b29..872643f 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -89,9 +89,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 	unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
 			      ? CE_MATCH_RACY_IS_DIRTY : 0);
 
-	if (option & DIFF_SILENT_ON_REMOVED)
-		handle_deprecated_show_diff_q(&revs->diffopt);
-
 	diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
 
 	if (diff_unmerged_stage < 0)
diff --git a/diff-no-index.c b/diff-no-index.c
index a788a5f..98a9cf1 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 deprecated_show_diff_q_option_used = 0;
 	const char *paths[2];
 
 	/* Were we asked to do --no-index explicitly? */
@@ -224,10 +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")) {
-			deprecated_show_diff_q_option_used = 1;
-			i++;
-		}
 		else if (!strcmp(argv[i], "--"))
 			i++;
 		else {
@@ -260,9 +255,6 @@ void diff_no_index(struct rev_info *revs,
 	revs->max_count = -2;
 	diff_setup_done(&revs->diffopt);
 
-	if (deprecated_show_diff_q_option_used)
-		handle_deprecated_show_diff_q(&revs->diffopt);
-
 	setup_diff_pager(&revs->diffopt);
 	DIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);
 
diff --git a/diff.c b/diff.c
index 78819ba..2d0b5e3 100644
--- a/diff.c
+++ b/diff.c
@@ -3570,14 +3570,6 @@ static int parse_diff_filter_opt(const char *optarg, struct diff_options *opt)
 	return 0;
 }
 
-/* Used only by "diff-files" and "diff --no-index" */
-void handle_deprecated_show_diff_q(struct diff_options *opt)
-{
-	warning("'diff -q' and 'diff-files -q' are deprecated.");
-	warning("Use 'diff --diff-filter=d' instead to ignore deleted filepairs.");
-	parse_diff_filter_opt("d", opt);
-}
-
 int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 {
 	const char *arg = av[0];
diff --git a/diff.h b/diff.h
index 5237d63..a367207 100644
--- a/diff.h
+++ b/diff.h
@@ -341,8 +341,6 @@ extern int parse_rename_score(const char **cp_p);
 
 extern long parse_algorithm_value(const char *value);
 
-extern void handle_deprecated_show_diff_q(struct diff_options *);
-
 extern int print_stat_summary(FILE *fp, int files,
 			      int insertions, int deletions);
 extern void setup_diff_pager(struct diff_options *);
-- 
1.8.3.3-1001-gcfc78f3

Re: [PATCH 6/6] diff: deprecate -q option to diff-files

[Jonathan Nieder]

Junio C Hamano wrote:

> We should remove the support for "-q" in Git 2.0.

Nooooo.  I hope you are teasing.

I don't mind seeing support for "-q" dropped, but I really don't think
it's worth delaying git 2.0 for that.  Would s/in Git 2.0/in some
future release/ be ok?

The patch text itself looks good.

Thanks,
Jonathan

Re: [PATCH 6/6] diff: deprecate -q option to diff-files

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>> We should remove the support for "-q" in Git 2.0.
>
> Nooooo.  I hope you are teasing.
>
> I don't mind seeing support for "-q" dropped, but I really don't think
> it's worth delaying git 2.0 for that.  Would s/in Git 2.0/in some
> future release/ be ok?

I do not think keeping the support for "-q" in is any huge burden.
We do not have to remove it, forever, for that matter.

The option is so obscure that (1) I would suspect that only a few
very old scripts would use it, (2) even the users who use such a
script every day would not know what the script is doing with the
option, and (3) we would not know if such scripts exist until a long
time passes after we include patches up to this step in a released
version.

That is why the patch 7/6 rolls the removal into a version bump at
which we promised to have a bunch of backward incompatible changes
to people.

Re: [PATCH 6/6] diff: deprecate -q option to diff-files

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> I don't mind seeing support for "-q" dropped, but I really don't think
>> it's worth delaying git 2.0 for that.  Would s/in Git 2.0/in some
>> future release/ be ok?
>
> I do not think keeping the support for "-q" in is any huge burden.
> We do not have to remove it, forever, for that matter.

I agree with the above, which is why I don't want a promise to remove
the "-q" option to cause Git 2.0 to be delayed.  It would be better to
schedule it for Git 3.0, or for another unspecified future git
release.

I thought the 2.0 boundary was a time for changes that everyone
already knew we should make, where we had been waiting for a good
moment to change behavior while giving people adequate warning to
avoid disrupting them too much.  We have a good collection of those
for 2.0, and the next batch can wait until 3.0.

Sorry for the lack of clarity,
Jonathan

[PATCH 4/4] diff.c: Do not initialize a variable, which gets reassigned anyway.

[Stefan Beller]

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 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

Re: [PATCH 4/4] diff.c: Do not initialize a variable, which gets reassigned anyway.

[Jonathan Nieder]

Stefan Beller wrote:

> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
> ---
>  diff.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
[...]
> --- 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);

Yeah, we should have caught this in review.

Thanks for reporting.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

[Jonathan Nieder]

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).

> --- 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) {

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

So fwiw, I like this patch.