{"thread":{"id":"34436","subject":"[PATCH 1/4] daemon.c:handle: Remove unneeded check for null pointer.","startedAt":"2013-07-14T21:35:46Z","lastAt":"2013-07-19T21:01:06Z","messageCount":25,"participants":["Stefan Beller","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"223353","messageId":"1373837749-14402-1-git-send-email-stefanbeller@googlemail.com","threadId":"34436","inReplyTo":null,"subject":"[PATCH 1/4] daemon.c:handle: Remove unneeded check for null pointer.","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-14T21:35:46Z","receivedAt":"2013-07-14T21:35:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"addr doesn't need to be checked at that line as it it already accessed\n7 lines before in the if (addr->sa_family).\n\nSigned-off-by: Stefan Beller \n---\n daemon.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 6aeddcb..5e48c1e 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -754,19 +754,19 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n \t}\n \n \tif (addr->sa_family == AF_INET) {\n \t\tstruct sockaddr_in *sin_addr = (void *) addr;\n \t\tinet_ntop(addr->sa_family, &sin_addr->sin_addr, addrbuf + 12,\n \t\t sizeof(addrbuf) - 12);\n \t\tsnprintf(portbuf, sizeof(portbuf), \"REMOTE_PORT=%d\",\n \t\t ntohs(sin_addr->sin_port));\n #ifndef NO_IPV6\n-\t} else if (addr && addr->sa_family == AF_INET6) {\n+\t} else if (addr->sa_family == AF_INET6) {\n \t\tstruct sockaddr_in6 *sin6_addr = (void *) addr;\n \n \t\tchar *buf = addrbuf + 12;\n \t\t*buf++ = '['; *buf = '\\0'; /* stpcpy() is cool */\n \t\tinet_ntop(AF_INET6, &sin6_addr->sin6_addr, buf,\n \t\t sizeof(addrbuf) - 13);\n \t\tstrcat(buf, \"]\");\n \n \t\tsnprintf(portbuf, sizeof(portbuf), \"REMOTE_PORT=%d\",\n-- \n1.8.3.2.806.gdee5b9b\n"},{"id":"223354","messageId":"1373837749-14402-2-git-send-email-stefanbeller@googlemail.com","threadId":"34436","inReplyTo":"1373837749-14402-1-git-send-email-stefanbeller@googlemail.com","subject":"[PATCH 2/4] commit: Fix a memory leak in determine_author_info","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-14T21:35:47Z","receivedAt":"2013-07-14T21:35:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The date variable is assigned new memory via xmemdupz and 2 lines later\nit is assigned new memory again via xmalloc, but the first assignment\nis never freed nor used.\n\nSigned-off-by: Stefan Beller \n---\n builtin/commit.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 790e5ab..00da83c 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -528,19 +528,18 @@ static void determine_author_info(struct strbuf *author_ident)\n \n \t\tif (lb == a + strlen(\"\\nauthor \"))\n \t\t\t/* \\nauthor */\n \t\t\tname = xcalloc(1, 1);\n \t\telse\n \t\t\tname = xmemdupz(a + strlen(\"\\nauthor \"),\n \t\t\t\t\t(lb - strlen(\" \") -\n \t\t\t\t\t (a + strlen(\"\\nauthor \"))));\n \t\temail = xmemdupz(lb + strlen(\"<\"), rb - (lb + strlen(\"<\")));\n-\t\tdate = xmemdupz(rb + strlen(\"> \"), eol - (rb + strlen(\"> \")));\n \t\tlen = eol - (rb + strlen(\"> \"));\n \t\tdate = xmalloc(len + 2);\n \t\t*date = '@';\n \t\tmemcpy(date + 1, rb + strlen(\"> \"), len);\n \t\tdate[len + 1] = '\\0';\n \t}\n \n \tif (force_author) {\n \t\tconst char *lb = strstr(force_author, \" <\");\n-- \n1.8.3.2.806.gdee5b9b\n"},{"id":"223355","messageId":"1373837749-14402-3-git-send-email-stefanbeller@googlemail.com","threadId":"34436","inReplyTo":"1373837749-14402-1-git-send-email-stefanbeller@googlemail.com","subject":"[PATCH 3/4] diff-no-index: Remove unused variable.","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-14T21:35:48Z","receivedAt":"2013-07-14T21:35:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n diff-no-index.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex e66fdf3..842add4 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -181,19 +181,18 @@ static int queue_diff(struct diff_options *o,\n \t}\n }\n \n void diff_no_index(struct rev_info *revs,\n \t\t int argc, const char **argv,\n \t\t int nongit, const char *prefix)\n {\n \tint i, prefixlen;\n \tint no_index = 0;\n-\tunsigned options = 0;\n \tconst char *paths[2];\n \n \t/* Were we asked to do --no-index explicitly? */\n \tfor (i = 1; i < argc; i++) {\n \t\tif (!strcmp(argv[i], \"--\")) {\n \t\t\ti++;\n \t\t\tbreak;\n \t\t}\n \t\tif (!strcmp(argv[i], \"--no-index\"))\n@@ -218,22 +217,20 @@ void diff_no_index(struct rev_info *revs,\n \tif (argc != i + 2)\n \t\tusagef(\"git diff %s \",\n \t\t no_index ? \"--no-index\" : \"[--no-index]\");\n \n \tdiff_setup(&revs->diffopt);\n \tfor (i = 1; i < argc - 2; ) {\n \t\tint j;\n \t\tif (!strcmp(argv[i], \"--no-index\"))\n \t\t\ti++;\n-\t\telse if (!strcmp(argv[i], \"-q\")) {\n-\t\t\toptions |= DIFF_SILENT_ON_REMOVED;\n+\t\telse if (!strcmp(argv[i], \"-q\"))\n \t\t\ti++;\n-\t\t}\n \t\telse if (!strcmp(argv[i], \"--\"))\n \t\t\ti++;\n \t\telse {\n \t\t\tj = diff_opt_parse(&revs->diffopt, argv + i, argc - i);\n \t\t\tif (!j)\n \t\t\t\tdie(\"invalid diff option/value: %s\", argv[i]);\n \t\t\ti += j;\n \t\t}\n \t}\n-- \n1.8.3.2.806.gdee5b9b\n"},{"id":"223356","messageId":"1373837749-14402-4-git-send-email-stefanbeller@googlemail.com","threadId":"34436","inReplyTo":"1373837749-14402-1-git-send-email-stefanbeller@googlemail.com","subject":"[PATCH 4/4] diff.c: Do not initialize a variable, which gets reassigned anyway.","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-14T21:35:49Z","receivedAt":"2013-07-14T21:35:49Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n diff.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex e53ddad..24382d7 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1677,21 +1677,19 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n \t\t}\n \n \t\t/*\n \t\t * scale the add/delete\n \t\t */\n \t\tadd = added;\n \t\tdel = deleted;\n \n \t\tif (graph_width <= max_change) {\n-\t\t\tint total = add + del;\n-\n-\t\t\ttotal = scale_linear(add + del, graph_width, max_change);\n+\t\t\tint total = scale_linear(add + del, graph_width, max_change);\n \t\t\tif (total < 2 && add && del)\n \t\t\t\t/* width >= 2 due to the sanity check */\n \t\t\t\ttotal = 2;\n \t\t\tif (add < del) {\n \t\t\t\tadd = scale_linear(add, graph_width, max_change);\n \t\t\t\tdel = total - add;\n \t\t\t} else {\n \t\t\t\tdel = scale_linear(del, graph_width, max_change);\n \t\t\t\tadd = total - del;\n-- \n1.8.3.2.806.gdee5b9b\n"},{"id":"223357","messageId":"20130714214812.GA13444@google.com","threadId":"34436","inReplyTo":"1373837749-14402-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH 1/4] daemon.c:handle: Remove unneeded check for null pointer.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-14T21:48:12Z","receivedAt":"2013-07-14T21:48:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> addr doesn't need to be checked at that line as it it already accessed\n> 7 lines before in the if (addr->sa_family).\n\nGood catch. This asymmetry has been present since the lines were first\nintroduced (all guarded by \"if (addr)\") in v1.4.1-rc1~3^2~4 (Log peer\naddress when git-daemon called from inetd, 2006-06-20).\n\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -754,19 +754,19 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)\n> \t}\n> \n> \tif (addr->sa_family == AF_INET) {\n> \t\tstruct sockaddr_in *sin_addr = (void *) addr;\n> \t\tinet_ntop(addr->sa_family, &sin_addr->sin_addr, addrbuf + 12,\n> \t\t sizeof(addrbuf) - 12);\n> \t\tsnprintf(portbuf, sizeof(portbuf), \"REMOTE_PORT=%d\",\n> \t\t ntohs(sin_addr->sin_port));\n> #ifndef NO_IPV6\n> -\t} else if (addr && addr->sa_family == AF_INET6) {\n> +\t} else if (addr->sa_family == AF_INET6) {\n\nAt this point 'addr' is &ss.sa from service_loop, so it really cannot\nbe NULL.\n\nSo fwiw, I like this patch.\n"},{"id":"223358","messageId":"20130714214932.GB13444@google.com","threadId":"34436","inReplyTo":"1373837749-14402-2-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH 2/4] commit: Fix a memory leak in determine_author_info","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-14T21:49:32Z","receivedAt":"2013-07-14T21:49:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Signed-off-by: Stefan Beller \n\nThanks. That was quick. :)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"223360","messageId":"20130714220739.GC13444@google.com","threadId":"34436","inReplyTo":"1373837749-14402-3-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH 3/4] diff-no-index: Remove unused variable.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-14T22:07:39Z","receivedAt":"2013-07-14T22:07:39Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> [Subject: diff-no-index: Remove unused variable.]\n[...]\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n[...]\n> -\t\telse if (!strcmp(argv[i], \"-q\")) {\n> +\t\telse if (!strcmp(argv[i], \"-q\"))\n> -\t\t\toptions |= DIFF_SILENT_ON_REMOVED;\n> \t\t\ti++;\n> -\t\t}\n\nThis feature was obviously never tested with --no-index, so I agree it\nmakes sense to remove it. Probably the commit message and a comment\nshould say so, though. E.g.:\n\n\tdiff --no-index: remove nonfunctional \"-q\" handling\n\n\tBefore v1.5.6-rc1~41^2~2, the option parsing for diff --no-index\n\tand \"git diff-files\" shared code. In \"git diff-files\", \"-q\" means\n\tto be silent about removed files. In \"git diff --no-index\", in\n\tvarious versions it has been an error, an infinite loop, or a no-op.\n\n\tSimplify the code to clarify that it is now a no-op, continuing to\n\taccept and ignore the -q option in \"git diff --no-index\" to avoid\n\tbreaking scripts.\n\nI wouldn't mind removing support for \"-q\" altogether, by the way (as a\nseparate change).\n\nHope that helps,\nJonathan\n"},{"id":"223361","messageId":"20130714221848.GE13444@google.com","threadId":"34436","inReplyTo":"1373837749-14402-4-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH 4/4] diff.c: Do not initialize a variable, which gets reassigned anyway.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-14T22:18:48Z","receivedAt":"2013-07-14T22:18:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Signed-off-by: Stefan Beller \n> ---\n> diff.c | 4 +---\n> 1 file changed, 1 insertion(+), 3 deletions(-)\n[...]\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1677,21 +1677,19 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)\n> \t\t}\n> \n> \t\t/*\n> \t\t * scale the add/delete\n> \t\t */\n> \t\tadd = added;\n> \t\tdel = deleted;\n> \n> \t\tif (graph_width <= max_change) {\n> -\t\t\tint total = add + del;\n> -\n> -\t\t\ttotal = scale_linear(add + del, graph_width, max_change);\n> +\t\t\tint total = scale_linear(add + del, graph_width, max_change);\n\nYeah, we should have caught this in review.\n\nThanks for reporting.\nReviewed-by: Jonathan Nieder \n"},{"id":"223520","messageId":"1373970487-32595-1-git-send-email-stefanbeller@googlemail.com","threadId":"34436","inReplyTo":"20130714220739.GC13444@google.com","subject":"[PATCH 0/2] git diff -q option removal","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-16T10:28:05Z","receivedAt":"2013-07-16T10:28:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 07/15/2013 12:07 AM, Jonathan Nieder wrote:\n> This feature was obviously never tested with --no-index, so I agree it\n> makes sense to remove it. Probably the commit message and a comment\n> should say so, though. E.g.:\n>\n> diff --no-index: remove nonfunctional \"-q\" handling\n>\n> Before v1.5.6-rc1~41^2~2, the option parsing for diff --no-index\n> and \"git diff-files\" shared code. In \"git diff-files\", \"-q\" means\n> to be silent about removed files. In \"git diff --no-index\", in\n> various versions it has been an error, an infinite loop, or a no-op.\n> \n> Simplify the code to clarify that it is now a no-op, continuing to\n> accept and ignore the -q option in \"git diff --no-index\" to avoid\n> breaking scripts.\n>\n> I wouldn't mind removing support for \"-q\" altogether, by the way (as a\n> separate change).\n>\n> Hope that helps,\n> Jonathan\n>\n\nI am resending the commit with a different wording, thanks to Jonathan. \n\nHowever I tried to remove support for -q in a separate commit, and\nI have some questions about the structure of the files.\n(I am sure it's documented, but I cannot find it, so please hint me \nwhere to read.)\n\nThe changes in the following patch are in diff_no_index.c, but the\ndiff_no_index(...) is called from cmd_diff, which is in builtin/diff.c\nThat cmd_diff is actually called from git.c having the\n{ \"diff\", cmd_diff }, entry in handle_internal_command.\n\nMy question now is this: Why is the builtin/diff.c relying on stuff\noutside of builtin/ ? Wouldn't it be better to move all these files\n(such as diff_no_index.c) into the builtin folder as well?\n\nRegarding the removal of the -q option, I tried it in the second patch.\nIs it as easy as that, or am I missing the point?\n\nThe first patch doesn't change the behavior, so I'd assume it's safe to \napply it to origin/sb/misc-fixes, whereas the second patch will make \ngit diff complain about the -q option, so I'd assume it would wait for the\nnext major release?\n\nBefore:\n\ttouch actual_file\n\tgit diff -q actual_file no_file\n\terror: Could not access 'no_file'\n\techo $?\n\t1\n\nAfter:\n\ttouch actual_file\n\tgit diff -q actual_file no_file\n\tfatal: invalid diff option/value: -q\n\techo $?\n\t128\n\nThanks,\nStefan\n\nStefan Beller (2):\n diff --no-index: remove nonfunctional \"-q\" handling\n git diff: Remove -q option to stay silent on missing files.\n\n Documentation/git-diff-files.txt | 6 +-----\n diff-no-index.c | 5 -----\n 2 files changed, 1 insertion(+), 10 deletions(-)\n\n-- \n1.8.2.3.10.g2733812\n"},{"id":"223521","messageId":"1373970487-32595-2-git-send-email-stefanbeller@googlemail.com","threadId":"34436","inReplyTo":"1373970487-32595-1-git-send-email-stefanbeller@googlemail.com","subject":"[PATCH 1/2] diff --no-index: remove nonfunctional \"-q\" handling","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-16T10:28:06Z","receivedAt":"2013-07-16T10:28:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Before v1.5.6-rc1~41^2~2, the option parsing for diff --no-index\nand \"git diff-files\" shared code. In \"git diff-files\", \"-q\" means\nto be silent about removed files. In \"git diff --no-index\", in\nvarious versions it has been an error, an infinite loop, or a no-op.\n\nSimplify the code to clarify that it is now a no-op, continuing to\naccept and ignore the -q option in \"git diff --no-index\" to avoid\nbreaking scripts.\n\nSigned-off-by: Stefan Beller \nHelped-by: Jonathan Nieder \n---\n diff-no-index.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 74da659..419cd78 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -187,7 +187,6 @@ void diff_no_index(struct rev_info *revs,\n {\n \tint i, prefixlen;\n \tint no_index = 0;\n-\tunsigned options = 0;\n \tconst char *paths[2];\n \n \t/* Were we asked to do --no-index explicitly? */\n@@ -224,10 +223,8 @@ void diff_no_index(struct rev_info *revs,\n \t\tint j;\n \t\tif (!strcmp(argv[i], \"--no-index\"))\n \t\t\ti++;\n-\t\telse if (!strcmp(argv[i], \"-q\")) {\n-\t\t\toptions |= DIFF_SILENT_ON_REMOVED;\n+\t\telse if (!strcmp(argv[i], \"-q\"))\n \t\t\ti++;\n-\t\t}\n \t\telse if (!strcmp(argv[i], \"--\"))\n \t\t\ti++;\n \t\telse {\n-- \n1.8.2.3.10.g2733812\n"},{"id":"223522","messageId":"1373970487-32595-3-git-send-email-stefanbeller@googlemail.com","threadId":"34436","inReplyTo":"1373970487-32595-1-git-send-email-stefanbeller@googlemail.com","subject":"[PATCH 2/2] diff: Remove -q to stay silent on missing files.","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-16T10:28:07Z","receivedAt":"2013-07-16T10:28:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This feature was not tested in the test suite, hence we'd remove it for\nnow. It doesn't seem to be often used anyway.\nA google search for \"git diff -q\" (match string exactly) only returned\nhttp://stackoverflow.com/questions/11021287/git-detect-if-there-are-untracked-files-quickly\nwhere \"git diff -q\" was quoted for its exit code behavior regarding files\nbeing found or not.\n\nSigned-off-by: Stefan Beller \nProposed-by: \n---\n Documentation/git-diff-files.txt | 6 +-----\n diff-no-index.c | 2 --\n 2 files changed, 1 insertion(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-diff-files.txt b/Documentation/git-diff-files.txt\nindex 906774f..d118cfb 100644\n--- a/Documentation/git-diff-files.txt\n+++ b/Documentation/git-diff-files.txt\n@@ -9,7 +9,7 @@ git-diff-files - Compares files in the working tree and the index\n SYNOPSIS\n --------\n [verse]\n-'git diff-files' [-q] [-0|-1|-2|-3|-c|--cc] [] [...]\n+'git diff-files' [-0|-1|-2|-3|-c|--cc] [] [...]\n \n DESCRIPTION\n -----------\n@@ -41,10 +41,6 @@ omit diff output for unmerged entries and just show \"Unmerged\".\n \tdiff, similar to the way 'diff-tree' shows a merge\n \tcommit with these flags.\n \n--q::\n-\tRemain silent even on nonexistent files\n-\n-\n include::diff-format.txt[]\n \n GIT\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 419cd78..98a9cf1 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -223,8 +223,6 @@ void diff_no_index(struct rev_info *revs,\n \t\tint j;\n \t\tif (!strcmp(argv[i], \"--no-index\"))\n \t\t\ti++;\n-\t\telse if (!strcmp(argv[i], \"-q\"))\n-\t\t\ti++;\n \t\telse if (!strcmp(argv[i], \"--\"))\n \t\t\ti++;\n \t\telse {\n-- \n1.8.2.3.10.g2733812\n"},{"id":"223593","messageId":"7vvc496ruf.fsf@alter.siamese.dyndns.org","threadId":"34436","inReplyTo":"1373970487-32595-1-git-send-email-stefanbeller@googlemail.com","subject":"Re: [PATCH 0/2] git diff -q option removal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-17T17:04:08Z","receivedAt":"2013-07-17T17:04:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> The changes in the following patch are in diff_no_index.c, but the\n> diff_no_index(...) is called from cmd_diff, which is in builtin/diff.c\n> That cmd_diff is actually called from git.c having the\n> { \"diff\", cmd_diff }, entry in handle_internal_command.\n>\n> My question now is this: Why is the builtin/diff.c relying on stuff\n> outside of builtin/ ? Wouldn't it be better to move all these files\n> (such as diff_no_index.c) into the builtin folder as well?\n\nBuiltins link all sorts of stuff from outside, e.g. diff.c and\ndiffcore-*.c at the toplevel. I do not see diff_no_index.c is any\ndifferent, so I am probably not understanding your question.\n\n> Regarding the removal of the -q option, I tried it in the second patch.\n> Is it as easy as that, or am I missing the point?\n> \n> The first patch doesn't change the behavior, so I'd assume it's safe to \n> apply it to origin/sb/misc-fixes, whereas the second patch will make \n> git diff complain about the -q option, so I'd assume it would wait for the\n> next major release?\n>\n> Before:\n> \ttouch actual_file\n> \tgit diff -q actual_file no_file\n> \terror: Could not access 'no_file'\n\nHmm, do you really get that error message? I think you would get\n\n fatal: ambiguous argument 'no_file': unknown revision or path not in the working tree.\n\n> \techo $?\n> \t1\n\nThe command line parsing infrastructure has changed vastly since\n\"show-diff\" days (see below for a history lesson); I think your\n\"Before\" should read more like this\n\n\tgit diff -q -- actual_file no_file\n\nand it should not show removal of no_file in its output. E.g. in\ngit.git\n\n\t$ git reset --hard\n $ rm COPYING\n $ git diff -q -- COPYING\n\nshould show nothing.\n\nI personally think \"-q\" no longer makes sense in today's codebase,\nbut I am not convinced that removal of '-q' from the proper \"git\ndiff-files\" and the \"git diff --no-index\" (aka \"I am too lazy to\nteach our diff enhancement to other people's diff implementations,\nso let's throw in a \"files do not have to be tracked in Git\nrepository at all\" mode\") is the right direction to go.\n\nThe \"-q\" option is a remnant from the \"show-diff\" command, the\nprecursor of today's \"git diff-files\" (back then, we didn't even\nhave \"git\" potty. The user literally typed \"show-diff\", not \"git\nshow-diff\").\n\nca2a0798 ([PATCH] Add \"-q\" option to show-diff.c, 2005-04-15) added\nthat option. Back then, we did not have pathspec matching, and we\niterated over command line arguments, and required all of them exist\nas filesystem entities. \"-q\" was a way to defeat that \"you name a\nfile, it must exist in the working tree\" safety, and also at the\nsame time not give output for such a file that was removed from the\nworking tree.\n\nThese days, the former \"safety\" is enforced by the generalized\nrevision parser (\"is it a path or is it a rev?\") code and the \"--\"\ndelimiter on the command line is the way to defeat it. The latter\nis done by giving a filtering specification that lack D to the\n\"--diff-filter\".\n\nIf we wanted to make \"-q\" follow the spirit of its original addition\nto \"show-diff\" again, we could internally add a diff-filter when the\n\"-q\" option is parsed.\n\n\"git diff -q ...\" is \"git diff --diff-filter=ACMRTUB ...\", and \"git\ndiff -q --diff-filter=AD\" is \"git diff --diff-filter=A\". That would\nlet us remove the special case for SILENT_ON_REMOVED, and also make\n\"-q\" work across various commands in the \"diff\" family. It might\neven make it work for \"diff --no-index\", but I didn't bother to\ncheck.\n\n> After:\n> \ttouch actual_file\n> \tgit diff -q actual_file no_file\n> \tfatal: invalid diff option/value: -q\n> \techo $?\n> \t128\n>\n> Thanks,\n> Stefan\n>\n> Stefan Beller (2):\n> diff --no-index: remove nonfunctional \"-q\" handling\n> git diff: Remove -q option to stay silent on missing files.\n>\n> Documentation/git-diff-files.txt | 6 +-----\n> diff-no-index.c | 5 -----\n> 2 files changed, 1 insertion(+), 10 deletions(-)\n"},{"id":"223599","messageId":"CAPc5daVaOuGD73a=jK7RhOpBnBcUPmL5XnuoXLWYqMs=AoDk0A@mail.gmail.com","threadId":"34436","inReplyTo":"7vvc496ruf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/2] git diff -q option removal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-17T18:06:38Z","receivedAt":"2013-07-17T18:06:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Wed, Jul 17, 2013 at 10:04 AM, Junio C Hamano wrote:\n> If we wanted to make \"-q\" follow the spirit of its original addition\n> to \"show-diff\" again, we could internally add a diff-filter when the\n> \"-q\" option is parsed.\n\nHaving said all that, I do not mean to advocate to retain \"-q\". Most\nlikely nobody uses it, and \"-q\" is grossly misnamed (\"why is it so\nspecial to be \"quiet\" only for removals?\"). As long as we mention its\nremoval in the release notes (and possibly tell those miniscule\nminority that wants to ignore deleted files to use --diff-filter\ninstead), we should be OK.\n\nIndependently, we might want to enhance --diff-filter parser to make\nit easier to say \"I want everything but D\", perhaps use lowercase\nletter to subtract from what have been specified so far (or if there\nis no uppercase letter, start from \"everything\"), so that we can say\n--diff-filter=d\nor something.\n"},{"id":"223605","messageId":"51E6F8F3.6020404@googlemail.com","threadId":"34436","inReplyTo":"7vvc496ruf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/2] git diff -q option removal","fromName":"Stefan Beller","fromEmail":"stefanbeller@googlemail.com","sentAt":"2013-07-17T20:05:07Z","receivedAt":"2013-07-17T20:05:07Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On 07/17/2013 07:04 PM, Junio C Hamano wrote:\n\n> Builtins link all sorts of stuff from outside, e.g. diff.c and\n> diffcore-*.c at the toplevel. I do not see diff_no_index.c is any\n> different, so I am probably not understanding your question.\n\nThanks 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\nwas confused of things in builtin using some parts outside of it.\nMaybe that folder raised to much expectations for me to be 'the real'\ncore, whereas the files outside, i.e. those files in the top level\ndirectory, are just there for other things or scripts.\nThis understanding of the structure seems obviously wrong now.\n\nThanks for clarification.\n\n> \n> Hmm, do you really get that error message? I think you would get\n> \n> fatal: ambiguous argument 'no_file': unknown revision or path not in the working tree.\n> \n>> \techo $?\n>> \t1\n\nOk here we go (using current origin/master 9c3c367):\n\n\tcd $(mktemp -d)\n\techo \"test\" > actual_file\n\tgit diff actual_file no_file\n\t# error: Could not access 'no_file'\n\techo $?\n\t1\n\t\n\t## I get the same message as well, if I'm using -- or not.\n\t## also the -q doesn't make a change\n\n\tgit init \n\tgit diff -q -- actual_file no_file\n\techo $?\n\t# 0\n\tgit diff -- actual_file no_file\n\techo $?\n\t# 0\n\tgit diff actual_file no_file\n\t# fatal: no_file: no such path in the working tree.\n\t# Use 'git -- ...' to specify paths that do not exist locally.\n\techo $?\n\t# 128\n\tgit diff -q actual_file no_file\n\t# fatal: no_file: no such path in the working tree.\n\t# Use 'git -- ...' to specify paths that do not exist locally.\n\techo $?\n\t128\n\nSo apparently git diff behaves differently if not in a repo, which is what I tested.\n\n> \n> The command line parsing infrastructure has changed vastly since\n> \"show-diff\" days (see below for a history lesson);\n\nA very interesting read, much appreciated. :)\n\n> \n> If we wanted to make \"-q\" follow the spirit of its original addition\n> to \"show-diff\" again, we could internally add a diff-filter when the\n> \"-q\" option is parsed.\n> \n\nI'm rather new to the project, so my opinion may not have much weight,\nI'll state it anyway:\nKeeping backwards compatibility is really hard, because you need the \nknowledge of such history lessons as read above, to understand what should\nhappen, like having an intuitive feeling about such parameters. Hence\nmaintaining/evolving the project will become harder and harder \n(specially for newcomers). So having one and only one way to achieve the desired\noutput, which fits into the greater structure as it's the case with --diff-filter\nis easier to remember for both the user and developers.\n\nHence I think keeping the -q option would only make sense for the plumber \nlayer, because there the explicit promise was given to not change stuff\nevery other release. \n\nStefan\n\n\n\n"},{"id":"223624","messageId":"1374107406-14357-1-git-send-email-gitster@pobox.com","threadId":"34436","inReplyTo":"7vvc496ruf.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/6] Deprecating \"diff-files -q\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-18T00:30:00Z","receivedAt":"2013-07-18T00:30:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The \"-q\" option given to \"git diff-files\" is a remnant of the\n\"show-diff\" command, the precursor of today's \"git diff-files\" (back\nthen, we didn't even have \"git\" potty. The user literally typed\n\"show-diff\", not \"git show-diff\").\n\nca2a0798 ([PATCH] Add \"-q\" option to show-diff.c, 2005-04-15) added\nthat option. Back then, we did not have pathspec matching, and we\niterated over command line arguments, and required all of them exist\nas filesystem entities. \"-q\" was a way to defeat that \"you name a\nfile, it must exist in the working tree\" safety, and also at the\nsame time not give output for such a file that was removed from the\nworking tree.\n\nThese days, the command line parsing infrastructure has changed\nvastly since \"show-diff\" days, and the former \"safety\" is enforced\nby the generalized revision parser (\"is it a path or is it a rev?\")\ncode and the \"--\" delimiter on the command line is the way to defeat\nit. The latter is done by giving a filtering specification that\nlack D to the \"--diff-filter\", e.g. \"--diff-filter=ACMRTUB\".\n\nThis is however a bit cumbersome to type. This miniseries updates\nthe diff-filter mechanism to let you say --diff-filter=d (lowercase)\nto express that you are interested in the changes in general, but\nnot the changes in the 'D' class (i.e. deletion).\n\nThe last step tweaks the command line parser of \"git diff-files\"\n(and \"git diff\" without any object on the command line, which goes\nto the same codepath) and \"git diff --no-index\" to notice \"-q\", warn\nand then turn it into \"--diff-filter=d\". We should remove the\nentire thing at a major version bump, like Git 2.0.\n\n\nThis is still a bit rough, without any documentation updates nor\ntests.\n\n\nJunio C Hamano (6):\n diff: pass the whole diff_options to diffcore_apply_filter()\n diff: factor out match_filter()\n diff: preparse --diff-filter string argument\n diff: reject unknown change class given to --diff-filter\n diff: allow lowercase letter to specify what change class to exclude\n diff: deprecate -q option to diff-files\n\n diff-lib.c | 8 ++--\n diff-no-index.c | 7 +++-\n diff.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++----------\n diff.h | 7 +++-\n 4 files changed, 118 insertions(+), 29 deletions(-)\n\n-- \n1.8.3.3-962-gf04df43\n"},{"id":"223623","messageId":"1374107406-14357-2-git-send-email-gitster@pobox.com","threadId":"34436","inReplyTo":"1374107406-14357-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/6] diff: pass the whole diff_options to diffcore_apply_filter()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-18T00:30:01Z","receivedAt":"2013-07-18T00:30:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The --diff-filter= option given by the user is kept as a\nstring, and passed to the underlying diffcore_apply_filter()\nfunction as a string for each resulting path we run number of\nstrchr() to see if each class of change among ACDMRTXUB is meant to\nbe given.\n\nChange the function signature to pass the whole diff_options, so\nthat we can pre-parse this string in the next patch.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 649ec86..41c64f2 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4509,11 +4509,13 @@ free_queue:\n \t}\n }\n \n-static void diffcore_apply_filter(const char *filter)\n+static void diffcore_apply_filter(struct diff_options *options)\n {\n \tint i;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tstruct diff_queue_struct outq;\n+\tconst char *filter = options->filter;\n+\n \tDIFF_QUEUE_CLEAR(&outq);\n \n \tif (!filter)\n@@ -4661,7 +4663,7 @@ void diffcore_std(struct diff_options *options)\n \tif (!options->found_follow)\n \t\t/* See try_to_follow_renames() in tree-diff.c */\n \t\tdiff_resolve_rename_copy();\n-\tdiffcore_apply_filter(options->filter);\n+\tdiffcore_apply_filter(options);\n \n \tif (diff_queued_diff.nr && !DIFF_OPT_TST(options, DIFF_FROM_CONTENTS))\n \t\tDIFF_OPT_SET(options, HAS_CHANGES);\n-- \n1.8.3.3-962-gf04df43\n"},{"id":"223626","messageId":"1374107406-14357-3-git-send-email-gitster@pobox.com","threadId":"34436","inReplyTo":"1374107406-14357-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/6] diff: factor out match_filter()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-18T00:30:02Z","receivedAt":"2013-07-18T00:30:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"diffcore_apply_filter() checks if a filepair matches the filter\ngiven with the \"--diff-filter\" option for each input filepairs with\na fairly complex expression in two places.\n\nCreate a helper function and call it.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 29 +++++++++++++----------------\n 1 file changed, 13 insertions(+), 16 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 41c64f2..0220c19 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4509,6 +4509,17 @@ free_queue:\n \t}\n }\n \n+static int match_filter(const struct diff_options *options, const struct diff_filepair *p)\n+{\n+\treturn (((p->status == DIFF_STATUS_MODIFIED) &&\n+\t\t ((p->score &&\n+\t\t strchr(options->filter, DIFF_STATUS_FILTER_BROKEN)) ||\n+\t\t (!p->score &&\n+\t\t strchr(options->filter, DIFF_STATUS_MODIFIED)))) ||\n+\t\t((p->status != DIFF_STATUS_MODIFIED) &&\n+\t\t strchr(options->filter, p->status)));\n+}\n+\n static void diffcore_apply_filter(struct diff_options *options)\n {\n \tint i;\n@@ -4524,14 +4535,7 @@ static void diffcore_apply_filter(struct diff_options *options)\n \tif (strchr(filter, DIFF_STATUS_FILTER_AON)) {\n \t\tint found;\n \t\tfor (i = found = 0; !found && i < q->nr; i++) {\n-\t\t\tstruct diff_filepair *p = q->queue[i];\n-\t\t\tif (((p->status == DIFF_STATUS_MODIFIED) &&\n-\t\t\t ((p->score &&\n-\t\t\t strchr(filter, DIFF_STATUS_FILTER_BROKEN)) ||\n-\t\t\t (!p->score &&\n-\t\t\t strchr(filter, DIFF_STATUS_MODIFIED)))) ||\n-\t\t\t ((p->status != DIFF_STATUS_MODIFIED) &&\n-\t\t\t strchr(filter, p->status)))\n+\t\t\tif (match_filter(options, q->queue[i]))\n \t\t\t\tfound++;\n \t\t}\n \t\tif (found)\n@@ -4549,14 +4553,7 @@ static void diffcore_apply_filter(struct diff_options *options)\n \t\t/* Only the matching ones */\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n-\n-\t\t\tif (((p->status == DIFF_STATUS_MODIFIED) &&\n-\t\t\t ((p->score &&\n-\t\t\t strchr(filter, DIFF_STATUS_FILTER_BROKEN)) ||\n-\t\t\t (!p->score &&\n-\t\t\t strchr(filter, DIFF_STATUS_MODIFIED)))) ||\n-\t\t\t ((p->status != DIFF_STATUS_MODIFIED) &&\n-\t\t\t strchr(filter, p->status)))\n+\t\t\tif (match_filter(options, p))\n \t\t\t\tdiff_q(&outq, p);\n \t\t\telse\n \t\t\t\tdiff_free_filepair(p);\n-- \n1.8.3.3-962-gf04df43\n"},{"id":"223628","messageId":"1374107406-14357-4-git-send-email-gitster@pobox.com","threadId":"34436","inReplyTo":"1374107406-14357-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/6] diff: preparse --diff-filter string argument","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-18T00:30:03Z","receivedAt":"2013-07-18T00:30:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Instead of running strchr() on the list of status characters over\nand over again, parse the --diff-filter option into bitfields and\nuse the bits to see if the change to the filepair matches the status\nrequested.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------\n diff.h | 5 ++++-\n 2 files changed, 60 insertions(+), 8 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 0220c19..03f10e6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3496,6 +3496,53 @@ static int parse_submodule_opt(struct diff_options *options, const char *value)\n \treturn 1;\n }\n \n+static const char diff_status_letters[] = {\n+\tDIFF_STATUS_ADDED,\n+\tDIFF_STATUS_COPIED,\n+\tDIFF_STATUS_DELETED,\n+\tDIFF_STATUS_MODIFIED,\n+\tDIFF_STATUS_RENAMED,\n+\tDIFF_STATUS_TYPE_CHANGED,\n+\tDIFF_STATUS_UNKNOWN,\n+\tDIFF_STATUS_UNMERGED,\n+\tDIFF_STATUS_FILTER_AON,\n+\tDIFF_STATUS_FILTER_BROKEN,\n+\t'\\0',\n+};\n+\n+static unsigned int filter_bit['Z' + 1];\n+\n+static void prepare_filter_bits(void)\n+{\n+\tint i;\n+\n+\tif (!filter_bit[DIFF_STATUS_ADDED]) {\n+\t\tfor (i = 0; diff_status_letters[i]; i++)\n+\t\t\tfilter_bit[(int) diff_status_letters[i]] = (1 << i);\n+\t}\n+}\n+\n+static unsigned filter_bit_tst(char status, const struct diff_options *opt)\n+{\n+\treturn opt->filter & filter_bit[(int) status];\n+}\n+\n+static int parse_diff_filter_opt(const char *optarg, struct diff_options *opt)\n+{\n+\tint i, optch;\n+\n+\tprepare_filter_bits();\n+\tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n+\t\tunsigned int bit;\n+\n+\t\tbit = (0 <= optch && optch <= 'Z') ? filter_bit[optch] : 0;\n+\t\tif (!bit)\n+\t\t\tcontinue; /* ignore unknown ones, like we always have */\n+\t\topt->filter |= bit;\n+\t}\n+\treturn 0;\n+}\n+\n int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n {\n \tconst char *arg = av[0];\n@@ -3717,7 +3764,10 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\treturn argcount;\n \t}\n \telse if ((argcount = parse_long_opt(\"diff-filter\", av, &optarg))) {\n-\t\toptions->filter = optarg;\n+\t\tint offending = parse_diff_filter_opt(optarg, options);\n+\t\tif (offending)\n+\t\t\tdie(\"unknown change class '%c' in --diff-filter=%s\",\n+\t\t\t offending, optarg);\n \t\treturn argcount;\n \t}\n \telse if (!strcmp(arg, \"--abbrev\"))\n@@ -4513,11 +4563,11 @@ static int match_filter(const struct diff_options *options, const struct diff_fi\n {\n \treturn (((p->status == DIFF_STATUS_MODIFIED) &&\n \t\t ((p->score &&\n-\t\t strchr(options->filter, DIFF_STATUS_FILTER_BROKEN)) ||\n+\t\t filter_bit_tst(DIFF_STATUS_FILTER_BROKEN, options)) ||\n \t\t (!p->score &&\n-\t\t strchr(options->filter, DIFF_STATUS_MODIFIED)))) ||\n+\t\t filter_bit_tst(DIFF_STATUS_MODIFIED, options)))) ||\n \t\t((p->status != DIFF_STATUS_MODIFIED) &&\n-\t\t strchr(options->filter, p->status)));\n+\t\t filter_bit_tst(p->status, options)));\n }\n \n static void diffcore_apply_filter(struct diff_options *options)\n@@ -4525,14 +4575,13 @@ static void diffcore_apply_filter(struct diff_options *options)\n \tint i;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n \tstruct diff_queue_struct outq;\n-\tconst char *filter = options->filter;\n \n \tDIFF_QUEUE_CLEAR(&outq);\n \n-\tif (!filter)\n+\tif (!options->filter)\n \t\treturn;\n \n-\tif (strchr(filter, DIFF_STATUS_FILTER_AON)) {\n+\tif (filter_bit_tst(DIFF_STATUS_FILTER_AON, options)) {\n \t\tint found;\n \t\tfor (i = found = 0; !found && i < q->nr; i++) {\n \t\t\tif (match_filter(options, q->queue[i]))\ndiff --git a/diff.h b/diff.h\nindex 78b4091..a367207 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -103,12 +103,15 @@ enum diff_words_type {\n };\n \n struct diff_options {\n-\tconst char *filter;\n \tconst char *orderfile;\n \tconst char *pickaxe;\n \tconst char *single_follow;\n \tconst char *a_prefix, *b_prefix;\n \tunsigned flags;\n+\n+\t/* diff-filter bits */\n+\tunsigned int filter;\n+\n \tint use_color;\n \tint context;\n \tint interhunkcontext;\n-- \n1.8.3.3-962-gf04df43\n"},{"id":"223627","messageId":"1374107406-14357-5-git-send-email-gitster@pobox.com","threadId":"34436","inReplyTo":"1374107406-14357-1-git-send-email-gitster@pobox.com","subject":"[PATCH 4/6] diff: reject unknown change class given to --diff-filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-18T00:30:04Z","receivedAt":"2013-07-18T00:30:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We used to accept \"git diff --diff-filter=Q\" (note that there is no\nsuch change class 'Q') silently and showed no output (because there\nis no such change class 'Q').\n\nError out when such an input is given.\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 03f10e6..3d37b56 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3537,7 +3537,7 @@ static int parse_diff_filter_opt(const char *optarg, struct diff_options *opt)\n \n \t\tbit = (0 <= optch && optch <= 'Z') ? filter_bit[optch] : 0;\n \t\tif (!bit)\n-\t\t\tcontinue; /* ignore unknown ones, like we always have */\n+\t\t\treturn optarg[i];\n \t\topt->filter |= bit;\n \t}\n \treturn 0;\n-- \n1.8.3.3-962-gf04df43\n"},{"id":"223629","messageId":"1374107406-14357-6-git-send-email-gitster@pobox.com","threadId":"34436","inReplyTo":"1374107406-14357-1-git-send-email-gitster@pobox.com","subject":"[PATCH 5/6] diff: allow lowercase letter to specify what change class to exclude","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-18T00:30:05Z","receivedAt":"2013-07-18T00:30:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"In order to express \"we do not care about deletions\", we had to say\n\"--diff-filter=ACMRTXUB\", giving all the possible change class\nexcept for the one we do not want, \"D\".\n\nThis is cumbersome. As all the change classes are in uppercase,\nallow their lowercase counterpart to selectively exclude the class\nfrom the output. When such a negated change class is in the input,\nstart the filter option with the full bits set.\n\nThis would allow us to express the old \"show-diff -q\" with\n\"git diff-files --diff-filter=d\".\n\nSigned-off-by: Junio C Hamano \n---\n diff.c | 29 ++++++++++++++++++++++++++++-\n 1 file changed, 28 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 3d37b56..2d0b5e3 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3532,13 +3532,40 @@ static int parse_diff_filter_opt(const char *optarg, struct diff_options *opt)\n \tint i, optch;\n \n \tprepare_filter_bits();\n+\n+\t/*\n+\t * If there is a negation e.g. 'd' in the input, and we haven't\n+\t * initialized the filter field with another --diff-filter, start\n+\t * from full set of bits, except for AON.\n+\t */\n+\tif (!opt->filter) {\n+\t\tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n+\t\t\tif (optch < 'a' || 'z' < optch)\n+\t\t\t\tcontinue;\n+\t\t\topt->filter = (1 << (ARRAY_SIZE(diff_status_letters) - 1)) - 1;\n+\t\t\topt->filter &= ~filter_bit[DIFF_STATUS_FILTER_AON];\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n \tfor (i = 0; (optch = optarg[i]) != '\\0'; i++) {\n \t\tunsigned int bit;\n+\t\tint negate;\n+\n+\t\tif ('a' <= optch && optch <= 'z') {\n+\t\t\tnegate = 1;\n+\t\t\toptch = toupper(optch);\n+\t\t} else {\n+\t\t\tnegate = 0;\n+\t\t}\n \n \t\tbit = (0 <= optch && optch <= 'Z') ? filter_bit[optch] : 0;\n \t\tif (!bit)\n \t\t\treturn optarg[i];\n-\t\topt->filter |= bit;\n+\t\tif (negate)\n+\t\t\topt->filter &= ~bit;\n+\t\telse\n+\t\t\topt->filter |= bit;\n \t}\n \treturn 0;\n }\n-- \n1.8.3.3-962-gf04df43\n"},{"id":"223625","messageId":"1374107406-14357-7-git-send-email-gitster@pobox.com","threadId":"34436","inReplyTo":"1374107406-14357-1-git-send-email-gitster@pobox.com","subject":"[PATCH 6/6] diff: deprecate -q option to diff-files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-18T00:30:06Z","receivedAt":"2013-07-18T00:30:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This reimplements the ancient \"-q\" option to \"git diff-files\" that\nwas inherited from \"show-diff -q\" in terms of \"--diff-filter=d\", and\nissue a warning against the use of the former.\n\nIncidentally this also tentatively fix \"git diff --no-index\" to\nhonor \"-q\" and hide deletions; the use will get the same warning.\n\nWe should remove the support for \"-q\" in Git 2.0.\n\nSigned-off-by: Junio C Hamano \n---\n diff-lib.c | 8 +++-----\n diff-no-index.c | 7 +++++--\n diff.c | 8 ++++++++\n diff.h | 2 ++\n 4 files changed, 18 insertions(+), 7 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f35de0f..4634b29 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -86,10 +86,12 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n {\n \tint entries, i;\n \tint diff_unmerged_stage = revs->max_count;\n-\tint silent_on_removed = option & DIFF_SILENT_ON_REMOVED;\n \tunsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)\n \t\t\t ? CE_MATCH_RACY_IS_DIRTY : 0);\n \n+\tif (option & DIFF_SILENT_ON_REMOVED)\n+\t\thandle_deprecated_show_diff_q(&revs->diffopt);\n+\n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n \n \tif (diff_unmerged_stage < 0)\n@@ -136,8 +138,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\t\tperror(ce->name);\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n-\t\t\t\tif (silent_on_removed)\n-\t\t\t\t\tcontinue;\n \t\t\t\twt_mode = 0;\n \t\t\t}\n \t\t\tdpath->mode = wt_mode;\n@@ -203,8 +203,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\tperror(ce->name);\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (silent_on_removed)\n-\t\t\t\tcontinue;\n \t\t\tdiff_addremove(&revs->diffopt, '-', ce->ce_mode,\n \t\t\t\t ce->sha1, !is_null_sha1(ce->sha1),\n \t\t\t\t ce->name, 0);\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 74da659..a788a5f 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -187,7 +187,7 @@ void diff_no_index(struct rev_info *revs,\n {\n \tint i, prefixlen;\n \tint no_index = 0;\n-\tunsigned options = 0;\n+\tunsigned deprecated_show_diff_q_option_used = 0;\n \tconst char *paths[2];\n \n \t/* Were we asked to do --no-index explicitly? */\n@@ -225,7 +225,7 @@ void diff_no_index(struct rev_info *revs,\n \t\tif (!strcmp(argv[i], \"--no-index\"))\n \t\t\ti++;\n \t\telse if (!strcmp(argv[i], \"-q\")) {\n-\t\t\toptions |= DIFF_SILENT_ON_REMOVED;\n+\t\t\tdeprecated_show_diff_q_option_used = 1;\n \t\t\ti++;\n \t\t}\n \t\telse if (!strcmp(argv[i], \"--\"))\n@@ -260,6 +260,9 @@ void diff_no_index(struct rev_info *revs,\n \trevs->max_count = -2;\n \tdiff_setup_done(&revs->diffopt);\n \n+\tif (deprecated_show_diff_q_option_used)\n+\t\thandle_deprecated_show_diff_q(&revs->diffopt);\n+\n \tsetup_diff_pager(&revs->diffopt);\n \tDIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);\n \ndiff --git a/diff.c b/diff.c\nindex 2d0b5e3..78819ba 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3570,6 +3570,14 @@ static int parse_diff_filter_opt(const char *optarg, struct diff_options *opt)\n \treturn 0;\n }\n \n+/* Used only by \"diff-files\" and \"diff --no-index\" */\n+void handle_deprecated_show_diff_q(struct diff_options *opt)\n+{\n+\twarning(\"'diff -q' and 'diff-files -q' are deprecated.\");\n+\twarning(\"Use 'diff --diff-filter=d' instead to ignore deleted filepairs.\");\n+\tparse_diff_filter_opt(\"d\", opt);\n+}\n+\n int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n {\n \tconst char *arg = av[0];\ndiff --git a/diff.h b/diff.h\nindex a367207..5237d63 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -341,6 +341,8 @@ extern int parse_rename_score(const char **cp_p);\n \n extern long parse_algorithm_value(const char *value);\n \n+extern void handle_deprecated_show_diff_q(struct diff_options *);\n+\n extern int print_stat_summary(FILE *fp, int files,\n \t\t\t int insertions, int deletions);\n extern void setup_diff_pager(struct diff_options *);\n-- \n1.8.3.3-962-gf04df43\n"},{"id":"223731","messageId":"7vvc47z14q.fsf_-_@alter.siamese.dyndns.org","threadId":"34436","inReplyTo":"1374107406-14357-7-git-send-email-gitster@pobox.com","subject":"[PATCH 7/6] diff: remove \"diff-files -q\" at Git 2.0 version boundary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-19T03:20:37Z","receivedAt":"2013-07-19T03:20:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This was inherited from \"show-diff -q\" that was invented to tell\ncomparison between the index and the working tree to ignore only\nremovals in 2005.\n\nThese days, it is spelled as \"--diff-filter=d\".\n\nSigned-off-by: Junio C Hamano \n---\n\n * And this is the endgame.\n\n diff-lib.c | 3 ---\n diff-no-index.c | 8 --------\n diff.c | 8 --------\n diff.h | 2 --\n 4 files changed, 21 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 4634b29..872643f 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -89,9 +89,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \tunsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)\n \t\t\t ? CE_MATCH_RACY_IS_DIRTY : 0);\n \n-\tif (option & DIFF_SILENT_ON_REMOVED)\n-\t\thandle_deprecated_show_diff_q(&revs->diffopt);\n-\n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"i/\", \"w/\");\n \n \tif (diff_unmerged_stage < 0)\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex a788a5f..98a9cf1 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -187,7 +187,6 @@ void diff_no_index(struct rev_info *revs,\n {\n \tint i, prefixlen;\n \tint no_index = 0;\n-\tunsigned deprecated_show_diff_q_option_used = 0;\n \tconst char *paths[2];\n \n \t/* Were we asked to do --no-index explicitly? */\n@@ -224,10 +223,6 @@ void diff_no_index(struct rev_info *revs,\n \t\tint j;\n \t\tif (!strcmp(argv[i], \"--no-index\"))\n \t\t\ti++;\n-\t\telse if (!strcmp(argv[i], \"-q\")) {\n-\t\t\tdeprecated_show_diff_q_option_used = 1;\n-\t\t\ti++;\n-\t\t}\n \t\telse if (!strcmp(argv[i], \"--\"))\n \t\t\ti++;\n \t\telse {\n@@ -260,9 +255,6 @@ void diff_no_index(struct rev_info *revs,\n \trevs->max_count = -2;\n \tdiff_setup_done(&revs->diffopt);\n \n-\tif (deprecated_show_diff_q_option_used)\n-\t\thandle_deprecated_show_diff_q(&revs->diffopt);\n-\n \tsetup_diff_pager(&revs->diffopt);\n \tDIFF_OPT_SET(&revs->diffopt, EXIT_WITH_STATUS);\n \ndiff --git a/diff.c b/diff.c\nindex 78819ba..2d0b5e3 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3570,14 +3570,6 @@ static int parse_diff_filter_opt(const char *optarg, struct diff_options *opt)\n \treturn 0;\n }\n \n-/* Used only by \"diff-files\" and \"diff --no-index\" */\n-void handle_deprecated_show_diff_q(struct diff_options *opt)\n-{\n-\twarning(\"'diff -q' and 'diff-files -q' are deprecated.\");\n-\twarning(\"Use 'diff --diff-filter=d' instead to ignore deleted filepairs.\");\n-\tparse_diff_filter_opt(\"d\", opt);\n-}\n-\n int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n {\n \tconst char *arg = av[0];\ndiff --git a/diff.h b/diff.h\nindex 5237d63..a367207 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -341,8 +341,6 @@ extern int parse_rename_score(const char **cp_p);\n \n extern long parse_algorithm_value(const char *value);\n \n-extern void handle_deprecated_show_diff_q(struct diff_options *);\n-\n extern int print_stat_summary(FILE *fp, int files,\n \t\t\t int insertions, int deletions);\n extern void setup_diff_pager(struct diff_options *);\n-- \n1.8.3.3-1001-gcfc78f3\n"},{"id":"223733","messageId":"20130719033126.GA27853@google.com","threadId":"34436","inReplyTo":"1374107406-14357-7-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 6/6] diff: deprecate -q option to diff-files","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-19T03:31:26Z","receivedAt":"2013-07-19T03:31:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> We should remove the support for \"-q\" in Git 2.0.\n\nNooooo. I hope you are teasing.\n\nI don't mind seeing support for \"-q\" dropped, but I really don't think\nit's worth delaying git 2.0 for that. Would s/in Git 2.0/in some\nfuture release/ be ok?\n\nThe patch text itself looks good.\n\nThanks,\nJonathan\n"},{"id":"223738","messageId":"7vip07ypu7.fsf@alter.siamese.dyndns.org","threadId":"34436","inReplyTo":"20130719033126.GA27853@google.com","subject":"Re: [PATCH 6/6] diff: deprecate -q option to diff-files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-19T07:24:32Z","receivedAt":"2013-07-19T07:24:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Junio C Hamano wrote:\n>\n>> We should remove the support for \"-q\" in Git 2.0.\n>\n> Nooooo. I hope you are teasing.\n>\n> I don't mind seeing support for \"-q\" dropped, but I really don't think\n> it's worth delaying git 2.0 for that. Would s/in Git 2.0/in some\n> future release/ be ok?\n\nI do not think keeping the support for \"-q\" in is any huge burden.\nWe do not have to remove it, forever, for that matter.\n\nThe option is so obscure that (1) I would suspect that only a few\nvery old scripts would use it, (2) even the users who use such a\nscript every day would not know what the script is doing with the\noption, and (3) we would not know if such scripts exist until a long\ntime passes after we include patches up to this step in a released\nversion.\n\nThat is why the patch 7/6 rolls the removal into a version bump at\nwhich we promised to have a bunch of backward incompatible changes\nto people.\n"},{"id":"223805","messageId":"20130719210106.GZ14690@google.com","threadId":"34436","inReplyTo":"7vip07ypu7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/6] diff: deprecate -q option to diff-files","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-07-19T21:01:06Z","receivedAt":"2013-07-19T21:01:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> I don't mind seeing support for \"-q\" dropped, but I really don't think\n>> it's worth delaying git 2.0 for that. Would s/in Git 2.0/in some\n>> future release/ be ok?\n>\n> I do not think keeping the support for \"-q\" in is any huge burden.\n> We do not have to remove it, forever, for that matter.\n\nI agree with the above, which is why I don't want a promise to remove\nthe \"-q\" option to cause Git 2.0 to be delayed. It would be better to\nschedule it for Git 3.0, or for another unspecified future git\nrelease.\n\nI thought the 2.0 boundary was a time for changes that everyone\nalready knew we should make, where we had been waiting for a good\nmoment to change behavior while giving people adequate warning to\navoid disrupting them too much. We have a good collection of those\nfor 2.0, and the next batch can wait until 3.0.\n\nSorry for the lack of clarity,\nJonathan\n"}]}