{"thread":{"id":"32933","subject":"[BUG] git-check-ignore: Segmentation fault","startedAt":"2013-02-19T05:24:24Z","lastAt":"2013-02-22T17:23:53Z","messageCount":27,"participants":["Zoltan Klinger","Adam Spiers","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"209766","messageId":"CAKJhZwR7AT6VfCZYwaTvWYyjtYWg+RxBSmB5NaJY0LrqMUnD6A@mail.gmail.com","threadId":"32933","inReplyTo":null,"subject":"[BUG] git-check-ignore: Segmentation fault","fromName":"Zoltan Klinger","fromEmail":"zoltan.klinger@gmail.com","sentAt":"2013-02-19T05:24:24Z","receivedAt":"2013-02-19T05:24:24Z","isPatch":false,"sender":{"key":"zoltan.klinger@gmail.com","avatar":"https://avatars.githubusercontent.com/u/95923?v=4"},"body":"Hi there,\n\nThe new git-check-ignore command seg faults when\n (1) it is called with single dot path name at $GIT_DIR level _AND_\n (2) and .gitignore has at least one directory pattern.\n\nGit version: 1.8.2.rc0.16.g20a599e\n\nReproduce the bug:\n $ git --version\n git version 1.8.2.rc0.16.g20a599e\n $ mkdir test\n $ cd test\n $ git init\n $ git check-ignore . # All good, no errors here\n $ echo \"dirpattern/\" > .gitignore\n $ git check-ignore .\n Segmentation fault (core dumped)\n\nThe segmentation fault is actually caused by hash_name(const char\n*name, int namelen) function in name-hash.c when the 'name' argument\nis an empty stringi and namelen is 0.\n\nThe empty string comes from a call to the prefix_path(prefix, len,\npath) function in setup.c. In this instance arguments 'prefix' is\nNULL, 'len' is 0 and 'path' is \".\" .\n\nCheers,\nZoltan\n"},{"id":"209820","messageId":"CAOkDyE_96Ef5CjoxNk3mbsNi+ZAuv6XeHcO7r8RQ-Of5ELsuKw@mail.gmail.com","threadId":"32933","inReplyTo":"CAKJhZwR7AT6VfCZYwaTvWYyjtYWg+RxBSmB5NaJY0LrqMUnD6A@mail.gmail.com","subject":"Re: [BUG] git-check-ignore: Segmentation fault","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2013-02-19T13:40:17Z","receivedAt":"2013-02-19T13:40:17Z","isPatch":false,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"On Tue, Feb 19, 2013 at 5:24 AM, Zoltan Klinger\n wrote:\n> Hi there,\n>\n> The new git-check-ignore command seg faults when\n> (1) it is called with single dot path name at $GIT_DIR level _AND_\n> (2) and .gitignore has at least one directory pattern.\n>\n> Git version: 1.8.2.rc0.16.g20a599e\n>\n> Reproduce the bug:\n> $ git --version\n> git version 1.8.2.rc0.16.g20a599e\n> $ mkdir test\n> $ cd test\n> $ git init\n> $ git check-ignore . # All good, no errors here\n> $ echo \"dirpattern/\" > .gitignore\n> $ git check-ignore .\n> Segmentation fault (core dumped)\n>\n> The segmentation fault is actually caused by hash_name(const char\n> *name, int namelen) function in name-hash.c when the 'name' argument\n> is an empty stringi and namelen is 0.\n>\n> The empty string comes from a call to the prefix_path(prefix, len,\n> path) function in setup.c. In this instance arguments 'prefix' is\n> NULL, 'len' is 0 and 'path' is \".\" .\n\nGood catch! Thanks for the very helpful bug report. I can reproduce\nthis, and have a fix - see follow-up mail to follow shortly.\n"},{"id":"209822","messageId":"1361282783-1413-1-git-send-email-git@adamspiers.org","threadId":"32933","inReplyTo":"CAOkDyE_96Ef5CjoxNk3mbsNi+ZAuv6XeHcO7r8RQ-Of5ELsuKw@mail.gmail.com","subject":"[PATCH 1/2] t0008: document test_expect_success_multi","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2013-02-19T14:06:22Z","receivedAt":"2013-02-19T14:06:22Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"test_expect_success_multi() helper function warrants some explanation,\nsince at first sight it may seem like generic test framework plumbing,\nbut is in fact specific to testing check-ignore, and allows more\nthorough testing of the various output formats without significantly\nincrease the size of t0008.\n\nSigned-off-by: Adam Spiers \n---\n t/t0008-ignores.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex d7df719..ebe7c70 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -75,6 +75,16 @@ test_check_ignore () {\n \tstderr_empty_on_success \"$expect_code\"\n }\n \n+# Runs the same code with 3 different levels of output verbosity,\n+# expecting success each time. Takes advantage of the fact that\n+# check-ignore --verbose output is the same as normal output except\n+# for the extra first column.\n+#\n+# Arguments:\n+# - (optional) prereqs for this test, e.g. 'SYMLINKS'\n+# - test name\n+# - output to expect from -v / --verbose mode\n+# - code to run (should invoke test_check_ignore)\n test_expect_success_multi () {\n \tprereq=\n \tif test $# -eq 4\n-- \n1.8.1.291.g0730ed6\n"},{"id":"209823","messageId":"1361282783-1413-2-git-send-email-git@adamspiers.org","threadId":"32933","inReplyTo":"1361282783-1413-1-git-send-email-git@adamspiers.org","subject":"[PATCH 2/2] check-ignore.c: fix segfault with '.' argument from repo root","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2013-02-19T14:06:23Z","receivedAt":"2013-02-19T14:06:23Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"Fix a corner case where check-ignore would segfault when run with the\n'.' argument from the top level of a repository, due to prefix_path()\nconverting '.' into the empty string. It doesn't make much sense to\ncall check-ignore from the top level with '.' as a parameter, since\nthe top-level directory would never typically be ignored, but of\ncourse it should not segfault in this case.\n\nSigned-off-by: Adam Spiers \n---\n builtin/check-ignore.c | 2 +-\n t/t0008-ignores.sh | 5 +++++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\nindex 709535c..b0dd7c2 100644\n--- a/builtin/check-ignore.c\n+++ b/builtin/check-ignore.c\n@@ -89,7 +89,7 @@ static int check_ignore(const char *prefix, const char **pathspec)\n \t\t\t\t\t? strlen(prefix) : 0, path);\n \t\tfull_path = check_path_for_gitlink(full_path);\n \t\tdie_if_path_beyond_symlink(full_path, prefix);\n-\t\tif (!seen[i] && path[0]) {\n+\t\tif (!seen[i] && full_path[0]) {\n \t\t\texclude = last_exclude_matching_path(&check, full_path,\n \t\t\t\t\t\t\t -1, &dtype);\n \t\t\tif (exclude) {\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex ebe7c70..9c1bde1 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -138,6 +138,7 @@ test_expect_success 'setup' '\n \tcat <<-\\EOF >.gitignore &&\n \t\tone\n \t\tignored-*\n+\t\ttop-level-dir/\n \tEOF\n \tfor dir in . a\n \tdo\n@@ -177,6 +178,10 @@ test_expect_success 'setup' '\n #\n # test invalid inputs\n \n+test_expect_success_multi '. corner-case' '' '\n+\ttest_check_ignore . 1\n+'\n+\n test_expect_success_multi 'empty command line' '' '\n \ttest_check_ignore \"\" 128 &&\n \tstderr_contains \"fatal: no path specified\"\n-- \n1.8.1.291.g0730ed6\n"},{"id":"209842","messageId":"7v621o6w83.fsf@alter.siamese.dyndns.org","threadId":"32933","inReplyTo":"1361282783-1413-1-git-send-email-git@adamspiers.org","subject":"Re: [PATCH 1/2] t0008: document test_expect_success_multi","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-19T17:37:32Z","receivedAt":"2013-02-19T17:37:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Spiers writes:\n\n> test_expect_success_multi() helper function warrants some explanation,\n> since at first sight it may seem like generic test framework plumbing,\n> but is in fact specific to testing check-ignore, and allows more\n> thorough testing of the various output formats without significantly\n> increase the size of t0008.\n>\n> Signed-off-by: Adam Spiers \n> ---\n\nGood. I vaguely recall saying why I hate these mini-frameworks\ninvented in individual tests, but with comments like this, they\nbecome much more palatable.\n\nThanks.\n\n> t/t0008-ignores.sh | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n>\n> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\n> index d7df719..ebe7c70 100755\n> --- a/t/t0008-ignores.sh\n> +++ b/t/t0008-ignores.sh\n> @@ -75,6 +75,16 @@ test_check_ignore () {\n> \tstderr_empty_on_success \"$expect_code\"\n> }\n> \n> +# Runs the same code with 3 different levels of output verbosity,\n> +# expecting success each time. Takes advantage of the fact that\n> +# check-ignore --verbose output is the same as normal output except\n> +# for the extra first column.\n> +#\n> +# Arguments:\n> +# - (optional) prereqs for this test, e.g. 'SYMLINKS'\n> +# - test name\n> +# - output to expect from -v / --verbose mode\n> +# - code to run (should invoke test_check_ignore)\n> test_expect_success_multi () {\n> \tprereq=\n> \tif test $# -eq 4\n"},{"id":"209843","messageId":"7v1ucc6vgd.fsf@alter.siamese.dyndns.org","threadId":"32933","inReplyTo":"1361282783-1413-2-git-send-email-git@adamspiers.org","subject":"Re: [PATCH 2/2] check-ignore.c: fix segfault with '.' argument from repo root","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-19T17:54:10Z","receivedAt":"2013-02-19T17:54:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Spiers writes:\n\n> Fix a corner case where check-ignore would segfault when run with the\n> '.' argument from the top level of a repository, due to prefix_path()\n> converting '.' into the empty string. It doesn't make much sense to\n> call check-ignore from the top level with '.' as a parameter, since\n> the top-level directory would never typically be ignored, but of\n> course it should not segfault in this case.\n>\n> Signed-off-by: Adam Spiers \n> ---\n\nPlease step back a bit and explain why the original had check for\npath[0] in the first place?\n\nIf the answer is \"the code wanted to special case the question 'is\nthe top-level excluded?', but used a wrong variable to implement the\ncheck, and this patch is a fix to that\", then the proposed commit\nlog message looks incomplete. The cause of the segv is not that\nprefix_path() returns an empty string, but because the function\ncalled inside the \"if\" block was written without expecting to be fed\nthe path that refers to the top-level of the working tree, no?\n\nWhile this change certainly will prevent the \"check the top-level\"\nrequest to last-exclude-matching-path, I have to wonder if it is a\ngood idea to force the caller of the l-e-m-p function to even care.\n\nIn other words, would it be a cleaner approach to fix the l-e-m-p\nfunction so that the caller can ask \"check the top-level\" and give a\nsensible answer (perhaps the answer may be \"nothing matches\"), and\nremove the \"&& path[0]\" (or \"&& full_path[0]\") special case from\nthis call site?\n\nThe last sentence \"It doesn't make much sense...\" in the proposed\nlog message would become a good justification for such a special\ncase at the beginning of l-e-m-p function, I would think.\n\n> builtin/check-ignore.c | 2 +-\n> t/t0008-ignores.sh | 5 +++++\n> 2 files changed, 6 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\n> index 709535c..b0dd7c2 100644\n> --- a/builtin/check-ignore.c\n> +++ b/builtin/check-ignore.c\n> @@ -89,7 +89,7 @@ static int check_ignore(const char *prefix, const char **pathspec)\n> \t\t\t\t\t? strlen(prefix) : 0, path);\n> \t\tfull_path = check_path_for_gitlink(full_path);\n> \t\tdie_if_path_beyond_symlink(full_path, prefix);\n> -\t\tif (!seen[i] && path[0]) {\n> +\t\tif (!seen[i] && full_path[0]) {\n> \t\t\texclude = last_exclude_matching_path(&check, full_path,\n> \t\t\t\t\t\t\t -1, &dtype);\n> \t\t\tif (exclude) {\n> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\n> index ebe7c70..9c1bde1 100755\n> --- a/t/t0008-ignores.sh\n> +++ b/t/t0008-ignores.sh\n> @@ -138,6 +138,7 @@ test_expect_success 'setup' '\n> \tcat <<-\\EOF >.gitignore &&\n> \t\tone\n> \t\tignored-*\n> +\t\ttop-level-dir/\n> \tEOF\n> \tfor dir in . a\n> \tdo\n> @@ -177,6 +178,10 @@ test_expect_success 'setup' '\n> #\n> # test invalid inputs\n> \n> +test_expect_success_multi '. corner-case' '' '\n> +\ttest_check_ignore . 1\n> +'\n> +\n> test_expect_success_multi 'empty command line' '' '\n> \ttest_check_ignore \"\" 128 &&\n> \tstderr_contains \"fatal: no path specified\"\n"},{"id":"209852","messageId":"CAOkDyE9VVuFn6B=Fe4XHxGCEW0MFgndx1X0+9hO36Soxb37YQw@mail.gmail.com","threadId":"32933","inReplyTo":"7v1ucc6vgd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] check-ignore.c: fix segfault with '.' argument from repo root","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2013-02-19T19:07:13Z","receivedAt":"2013-02-19T19:07:13Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"On Tue, Feb 19, 2013 at 5:54 PM, Junio C Hamano wrote:\n> Adam Spiers writes:\n>\n>> Fix a corner case where check-ignore would segfault when run with the\n>> '.' argument from the top level of a repository, due to prefix_path()\n>> converting '.' into the empty string. It doesn't make much sense to\n>> call check-ignore from the top level with '.' as a parameter, since\n>> the top-level directory would never typically be ignored, but of\n>> course it should not segfault in this case.\n>>\n>> Signed-off-by: Adam Spiers \n>> ---\n>\n> Please step back a bit and explain why the original had check for\n> path[0] in the first place?\n\nI can't remember to be honest.\n\n> If the answer is \"the code wanted to special case the question 'is\n> the top-level excluded?',\n\nYes, I think that's the most likely explanation. Maybe it got missed\nin a variable renaming refactoring.\n\n> but used a wrong variable to implement the\n> check, and this patch is a fix to that\", then the proposed commit\n> log message looks incomplete. The cause of the segv is not that\n> prefix_path() returns an empty string, but because the function\n> called inside the \"if\" block was written without expecting to be fed\n> the path that refers to the top-level of the working tree, no?\n>\n> While this change certainly will prevent the \"check the top-level\"\n> request to last-exclude-matching-path, I have to wonder if it is a\n> good idea to force the caller of the l-e-m-p function to even care.\n>\n> In other words, would it be a cleaner approach to fix the l-e-m-p\n> function so that the caller can ask \"check the top-level\" and give a\n> sensible answer (perhaps the answer may be \"nothing matches\"), and\n> remove the \"&& path[0]\" (or \"&& full_path[0]\") special case from\n> this call site?\n\nYes, that did cross my mind. I also wondered whether hash_name()\nshould do stricter input validation, but I guess that could have an\nimpact on performance.\n\n> The last sentence \"It doesn't make much sense...\" in the proposed\n> log message would become a good justification for such a special\n> case at the beginning of l-e-m-p function, I would think.\n\nFair enough. I'll reply to this with a new version.[0]\n\n[0] I wish there was a clean way to include the new version inline,\n but as I've noted before, there doesn't seem to be:\n\n http://article.gmane.org/gmane.comp.version-control.git/146110\n"},{"id":"209856","messageId":"1361301696-11307-1-git-send-email-git@adamspiers.org","threadId":"32933","inReplyTo":"CAOkDyE9VVuFn6B=Fe4XHxGCEW0MFgndx1X0+9hO36Soxb37YQw@mail.gmail.com","subject":"[PATCH v2 2/2] check-ignore.c, dir.c: fix segfault with '.' argument from repo root","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2013-02-19T19:21:36Z","receivedAt":"2013-02-19T19:21:36Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"Fix a corner case where check-ignore would segfault when run with the\n'.' argument from the top level of a repository, due to prefix_path()\nconverting '.' into the empty string. It doesn't make much sense to\ncall check-ignore from the top level with '.' as a parameter, since\nthe top-level directory would never typically be ignored, but of\ncourse it should not segfault in this case. The existing code\nattempted to check for this case but failed due to using the wrong\nvariable. Instead we move the check to last_exclude_matching_path(),\nin case other callers present or future have a similar issue.\n\nSigned-off-by: Adam Spiers \n---\n builtin/check-ignore.c | 2 +-\n dir.c | 8 ++++++++\n t/t0008-ignores.sh | 5 +++++\n 3 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\nindex 709535c..0240f99 100644\n--- a/builtin/check-ignore.c\n+++ b/builtin/check-ignore.c\n@@ -89,7 +89,7 @@ static int check_ignore(const char *prefix, const char **pathspec)\n \t\t\t\t\t? strlen(prefix) : 0, path);\n \t\tfull_path = check_path_for_gitlink(full_path);\n \t\tdie_if_path_beyond_symlink(full_path, prefix);\n-\t\tif (!seen[i] && path[0]) {\n+\t\tif (!seen[i]) {\n \t\t\texclude = last_exclude_matching_path(&check, full_path,\n \t\t\t\t\t\t\t -1, &dtype);\n \t\t\tif (exclude) {\ndiff --git a/dir.c b/dir.c\nindex 57394e4..1ae0b90 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -828,6 +828,14 @@ struct exclude *last_exclude_matching_path(struct path_exclude_check *check,\n \tstruct exclude *exclude;\n \n \t/*\n+\t * name could be the empty string, e.g. if check-ignore was\n+\t * invoked from the top level with '.', prefix_path() will\n+\t * convert it into \"\".\n+\t */\n+\tif (!*name)\n+\t\treturn NULL;\n+\n+\t/*\n \t * we allow the caller to pass namelen as an optimization; it\n \t * must match the length of the name, as we eventually call\n \t * is_excluded() on the whole name string.\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex ebe7c70..9c1bde1 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -138,6 +138,7 @@ test_expect_success 'setup' '\n \tcat <<-\\EOF >.gitignore &&\n \t\tone\n \t\tignored-*\n+\t\ttop-level-dir/\n \tEOF\n \tfor dir in . a\n \tdo\n@@ -177,6 +178,10 @@ test_expect_success 'setup' '\n #\n # test invalid inputs\n \n+test_expect_success_multi '. corner-case' '' '\n+\ttest_check_ignore . 1\n+'\n+\n test_expect_success_multi 'empty command line' '' '\n \ttest_check_ignore \"\" 128 &&\n \tstderr_contains \"fatal: no path specified\"\n-- \n1.8.2.rc0.18.g543d1e4\n"},{"id":"209858","messageId":"7v1ucc5b7n.fsf_-_@alter.siamese.dyndns.org","threadId":"32933","inReplyTo":"CAOkDyE9VVuFn6B=Fe4XHxGCEW0MFgndx1X0+9hO36Soxb37YQw@mail.gmail.com","subject":"Re* [PATCH 2/2] check-ignore.c: fix segfault with '.' argument from repo root","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-19T19:56:44Z","receivedAt":"2013-02-19T19:56:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Spiers writes:\n\n> Fair enough. I'll reply to this with a new version.[0]\n>\n> [0] I wish there was a clean way to include the new version inline,\n> but as I've noted before, there doesn't seem to be:\n>\n> http://article.gmane.org/gmane.comp.version-control.git/146110\n\nI find it easier to later find the patch if you made it a separate\nfollow-up like you did, but you can do it this way if you really\nwant to, using a scissors line, like so. Please do not try to be\ncreative and change the shape of scissors just for the sake of\nchaning it.\n\n-- >8 --\nSubject: name-hash: allow hashing an empty string\n\nUsually we do not pass an empty string to the function hash_name()\nbecause we almost always ask for hash values for a path that is a\ncandidate to be added to the index. However, check-ignore (and most\nlikely check-attr, but I didn't check) apparently has a callchain\nto ask the hash value for an empty path when it was given a \".\" from\nthe top-level directory to ask \"Is the path . excluded by default?\"\n\nMake sure that hash_name() does not overrun the end of the given\npathname even when it is empty.\n\nAlso remove a sweep-the-issue-under-the-rug conditional in\ncheck-ignore that avoided to pass an empty string to the callchain.\n\nSigned-off-by: Adam Spiers \n---\n builtin/check-ignore.c | 2 +-\n name-hash.c | 4 ++--\n t/t0008-ignores.sh | 5 +++++\n 3 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\nindex 709535c..0240f99 100644\n--- a/builtin/check-ignore.c\n+++ b/builtin/check-ignore.c\n@@ -89,7 +89,7 @@ static int check_ignore(const char *prefix, const char **pathspec)\n \t\t\t\t\t? strlen(prefix) : 0, path);\n \t\tfull_path = check_path_for_gitlink(full_path);\n \t\tdie_if_path_beyond_symlink(full_path, prefix);\n-\t\tif (!seen[i] && path[0]) {\n+\t\tif (!seen[i]) {\n \t\t\texclude = last_exclude_matching_path(&check, full_path,\n \t\t\t\t\t\t\t -1, &dtype);\n \t\t\tif (exclude) {\ndiff --git a/name-hash.c b/name-hash.c\nindex d8d25c2..942c459 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -24,11 +24,11 @@ static unsigned int hash_name(const char *name, int namelen)\n {\n \tunsigned int hash = 0x123;\n \n-\tdo {\n+\twhile (namelen--) {\n \t\tunsigned char c = *name++;\n \t\tc = icase_hash(c);\n \t\thash = hash*101 + c;\n-\t} while (--namelen);\n+\t}\n \treturn hash;\n }\n \ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex ebe7c70..9c1bde1 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -138,6 +138,7 @@ test_expect_success 'setup' '\n \tcat <<-\\EOF >.gitignore &&\n \t\tone\n \t\tignored-*\n+\t\ttop-level-dir/\n \tEOF\n \tfor dir in . a\n \tdo\n@@ -177,6 +178,10 @@ test_expect_success 'setup' '\n #\n # test invalid inputs\n \n+test_expect_success_multi '. corner-case' '' '\n+\ttest_check_ignore . 1\n+'\n+\n test_expect_success_multi 'empty command line' '' '\n \ttest_check_ignore \"\" 128 &&\n \tstderr_contains \"fatal: no path specified\"\n"},{"id":"209859","messageId":"7vzjz03wid.fsf@alter.siamese.dyndns.org","threadId":"32933","inReplyTo":"1361301696-11307-1-git-send-email-git@adamspiers.org","subject":"Re: [PATCH v2 2/2] check-ignore.c, dir.c: fix segfault with '.' argument from repo root","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-19T19:59:38Z","receivedAt":"2013-02-19T19:59:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Spiers writes:\n\n> Fix a corner case where check-ignore would segfault when run with the\n> '.' argument from the top level of a repository, due to prefix_path()\n> converting '.' into the empty string.\n\nThe description does not match what I understand is happening from\nthe original report, though. The above is more like this, no?\n\n When check-ignore is run with the '.' argument from the top level of\n a repository, it fed an empty string to hash_name() in name-hash.c\n and caused a segfault, as the function kept reading forever past the\n end of the string.\n\nA point to note is that it is not cleaer why it is a corner case to\nask about a pathspec \".\". It is a valid question \"Is the whole tree\nignored by default?\", isn't it?\n\n> It doesn't make much sense to\n> call check-ignore from the top level with '.' as a parameter, since\n> the top-level directory would never typically be ignored,\n\nAnd this sounds like a really bad excuse. If it were \"it does not\nmake *any* sense ... because the top level is *never* ignored\", then\nthe patch is a perfectly fine optimization that happens to work\naround the problem, but the use of \"much\" and \"typically\" is a sure\nsign that the design of the fix is iffy. It also shows that the\npatch is not a fix, but is sweeping the problem under the rug, if\nthere were a valid use case to set the top level to be ignored.\n\nI wonder what would happen if we removed that \"&& path[0]\" check\nfrom the caller, not add the \"assume the top is never ignored\"\nworkaround, and do something like this to the location that causes\nsegv, so that it can give an answer when asked to hash an empty\nstring?\n\nDoes the callchain that goes down to this function have other places\nthat assume they will never see an empty string, like this function\ndoes, which I _think_ is the real issue?\n\n name-hash.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/name-hash.c b/name-hash.c\nindex d8d25c2..942c459 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -24,11 +24,11 @@ static unsigned int hash_name(const char *name, int namelen)\n {\n \tunsigned int hash = 0x123;\n \n-\tdo {\n+\twhile (namelen--) {\n \t\tunsigned char c = *name++;\n \t\tc = icase_hash(c);\n \t\thash = hash*101 + c;\n-\t} while (--namelen);\n+\t}\n \treturn hash;\n }\n \n"},{"id":"209867","messageId":"7vfw0s3qsq.fsf@alter.siamese.dyndns.org","threadId":"32933","inReplyTo":"7vzjz03wid.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] check-ignore.c, dir.c: fix segfault with '.' argument from repo root","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-19T22:03:01Z","receivedAt":"2013-02-19T22:03:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> And this sounds like a really bad excuse. If it were \"it does not\n> make *any* sense ... because the top level is *never* ignored\", then\n> the patch is a perfectly fine optimization that happens to work\n> around the problem, but the use of \"much\" and \"typically\" is a sure\n> sign that the design of the fix is iffy. It also shows that the\n> patch is not a fix, but is sweeping the problem under the rug, if\n> there were a valid use case to set the top level to be ignored.\n>\n> I wonder what would happen if we removed that \"&& path[0]\" check\n> from the caller, not add the \"assume the top is never ignored\"\n> workaround, and do something like this to the location that causes\n> segv, so that it can give an answer when asked to hash an empty\n> string?\n>\n> Does the callchain that goes down to this function have other places\n> that assume they will never see an empty string, like this function\n> does, which I _think_ is the real issue?\n\nI started to suspect that may be the right approach. Why not do this?\n\n-- >8 --\nFrom: Junio C Hamano \nDate: Tue, 19 Feb 2013 11:56:44 -0800\nSubject: [PATCH] name-hash: allow hashing an empty string\n\nUsually we do not pass an empty string to the function hash_name()\nbecause we almost always ask for hash values for a path that is a\ncandidate to be added to the index. However, check-ignore (and most\nlikely check-attr, but I didn't check) apparently has a callchain\nto ask the hash value for an empty path when it was given a \".\" from\nthe top-level directory to ask \"Is the path . excluded by default?\"\n\nMake sure that hash_name() does not overrun the end of the given\npathname even when it is empty.\n\nRemove a sweep-the-issue-under-the-rug conditional in check-ignore\nthat avoided to pass an empty string to the callchain while at it.\nIt is a valid question to ask for check-ignore if the top-level is\nset to be ignored by default, even though the answer is most likely\nno, if only because there is currently no way to specify such an\nentry in the .gitignore file. But it is an unusual thing to ask and\nit is not worth optimizing for it by special casing at the top level\nof the call chain.\n\nSigned-off-by: Adam Spiers \nSigned-off-by: Junio C Hamano \n---\n builtin/check-ignore.c | 2 +-\n name-hash.c | 4 ++--\n t/t0008-ignores.sh | 5 +++++\n 3 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\nindex 709535c..0240f99 100644\n--- a/builtin/check-ignore.c\n+++ b/builtin/check-ignore.c\n@@ -89,7 +89,7 @@ static int check_ignore(const char *prefix, const char **pathspec)\n \t\t\t\t\t? strlen(prefix) : 0, path);\n \t\tfull_path = check_path_for_gitlink(full_path);\n \t\tdie_if_path_beyond_symlink(full_path, prefix);\n-\t\tif (!seen[i] && path[0]) {\n+\t\tif (!seen[i]) {\n \t\t\texclude = last_exclude_matching_path(&check, full_path,\n \t\t\t\t\t\t\t -1, &dtype);\n \t\t\tif (exclude) {\ndiff --git a/name-hash.c b/name-hash.c\nindex d8d25c2..942c459 100644\n--- a/name-hash.c\n+++ b/name-hash.c\n@@ -24,11 +24,11 @@ static unsigned int hash_name(const char *name, int namelen)\n {\n \tunsigned int hash = 0x123;\n \n-\tdo {\n+\twhile (namelen--) {\n \t\tunsigned char c = *name++;\n \t\tc = icase_hash(c);\n \t\thash = hash*101 + c;\n-\t} while (--namelen);\n+\t}\n \treturn hash;\n }\n \ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex ebe7c70..9c1bde1 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -138,6 +138,7 @@ test_expect_success 'setup' '\n \tcat <<-\\EOF >.gitignore &&\n \t\tone\n \t\tignored-*\n+\t\ttop-level-dir/\n \tEOF\n \tfor dir in . a\n \tdo\n@@ -177,6 +178,10 @@ test_expect_success 'setup' '\n #\n # test invalid inputs\n \n+test_expect_success_multi '. corner-case' '' '\n+\ttest_check_ignore . 1\n+'\n+\n test_expect_success_multi 'empty command line' '' '\n \ttest_check_ignore \"\" 128 &&\n \tstderr_contains \"fatal: no path specified\"\n-- \n1.8.2.rc0.89.g6e4b41d\n"},{"id":"209874","messageId":"20130220013035.GA7860@pacific.linksys.moosehall","threadId":"32933","inReplyTo":"7vzjz03wid.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] check-ignore.c, dir.c: fix segfault with '.' argument from repo root","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2013-02-20T01:30:35Z","receivedAt":"2013-02-20T01:30:35Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"On Tue, Feb 19, 2013 at 7:59 PM, Junio C Hamano wrote:\n> Adam Spiers writes:\n>\n>> Fix a corner case where check-ignore would segfault when run with the\n>> '.' argument from the top level of a repository, due to prefix_path()\n>> converting '.' into the empty string.\n>\n> The description does not match what I understand is happening from\n> the original report, though.\n\nWhy not?\n\n> The above is more like this, no?\n>\n> When check-ignore is run with the '.' argument from the top level of\n> a repository, it fed an empty string to hash_name() in name-hash.c\n> and caused a segfault, as the function kept reading forever past the\n> end of the string.\n\nThe only difference I can see between the two is that yours has\nchanged the phrase order and gives a bit more information. I don't\nsee any disagreement between them though.\n\n> A point to note is that it is not cleaer why it is a corner case to\n> ask about a pathspec \".\". It is a valid question \"Is the whole tree\n> ignored by default?\", isn't it?\n\nIt's probably valid, but I'm not the best judge of that. It doesn't\nmake much sense to me why anyone would do that, and it seems very\nunlikely it would be a common use case, therefore it's a corner case.\nThe next sentence then qualifies that:\n\n>> It doesn't make much sense to\n>> call check-ignore from the top level with '.' as a parameter, since\n>> the top-level directory would never typically be ignored,\n>\n> And this sounds like a really bad excuse.\n\nAn excuse for what? The final part of that sentence which you trimmed\nmade it clear that it was not an excuse for the segfault. Your choice\nof wording here sounds more like a personal attack than a technical\ndiscussion - presumably unintentional, so I'll choose not to take\noffense.\n\n> If it were \"it does not\n> make *any* sense ... because the top level is *never* ignored\", then\n> the patch is a perfectly fine optimization that happens to work\n> around the problem, but the use of \"much\" and \"typically\" is a sure\n> sign that the design of the fix is iffy. It also shows that the\n> patch is not a fix, but is sweeping the problem under the rug, if\n> there were a valid use case to set the top level to be ignored.\n\nThere *may* be a valid use case to set the top level to be ignored.\n*I* can't think of one personally, therefore it doesn't make sense to\nme to do so, but my intention was to avoid imposing my own personal\njudgement on everyone else by preventing people from doing that.\nHowever, in that case I now realise that check-ignore should report a\nmatch, and my proposed patches do not do that. Perhaps that is what\nyou meant by \"sweeping the problem under the rug\"?\n\n> I wonder what would happen if we removed that \"&& path[0]\" check\n> from the caller, not add the \"assume the top is never ignored\"\n> workaround, and do something like this to the location that causes\n> segv, so that it can give an answer when asked to hash an empty\n> string?\n>\n> name-hash.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/name-hash.c b/name-hash.c\n> index d8d25c2..942c459 100644\n> --- a/name-hash.c\n> +++ b/name-hash.c\n> @@ -24,11 +24,11 @@ static unsigned int hash_name(const char *name, int namelen)\n> {\n> unsigned int hash = 0x123;\n>\n> - do {\n> + while (namelen--) {\n> unsigned char c = *name++;\n> c = icase_hash(c);\n> hash = hash*101 + c;\n> - } while (--namelen);\n> + }\n> return hash;\n> }\n\nYep, that makes sense - that's what I meant when I said I was\nwondering \"whether hash_name() should do stricter input validation\".\n\n> Does the callchain that goes down to this function have other places\n> that assume they will never see an empty string, like this function\n> does, which I _think_ is the real issue?\n\nGood question. In the absence of a proper audit for similar issues,\nit definitely makes sense to defensively program hash_name() (and any\nother low-level functions which accept path strings).\n"},{"id":"209876","messageId":"20130220015723.GB7860@pacific.linksys.moosehall","threadId":"32933","inReplyTo":"7vfw0s3qsq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] check-ignore.c, dir.c: fix segfault with '.' argument from repo root","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2013-02-20T01:57:24Z","receivedAt":"2013-02-20T01:57:24Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"On Tue, Feb 19, 2013 at 02:03:01PM -0800, Junio C Hamano wrote:\n> I started to suspect that may be the right approach. Why not do this?\n> \n> -- >8 --\n> From: Junio C Hamano \n> Date: Tue, 19 Feb 2013 11:56:44 -0800\n> Subject: [PATCH] name-hash: allow hashing an empty string\n> \n> Usually we do not pass an empty string to the function hash_name()\n> because we almost always ask for hash values for a path that is a\n> candidate to be added to the index. However, check-ignore (and most\n> likely check-attr, but I didn't check) apparently has a callchain\n> to ask the hash value for an empty path when it was given a \".\" from\n> the top-level directory to ask \"Is the path . excluded by default?\"\n\nAccording to a single gdb run, 'git check-attr -a .' does not hit\nhash_name() for me. However that naive experiment doesn't rule out\nthe possibility of it happening if the right attributes are set, but I\ndon't know enough about that code to comment.\n\n> Make sure that hash_name() does not overrun the end of the given\n> pathname even when it is empty.\n> \n> Remove a sweep-the-issue-under-the-rug conditional in check-ignore\n> that avoided to pass an empty string to the callchain while at it.\n> It is a valid question to ask for check-ignore if the top-level is\n> set to be ignored by default, even though the answer is most likely\n\nHmm, I see very little difference between the use of \"most likely\" and\nthe use of the words \"much\" and \"typically\" which you previously\nconsidered \"a sure sign that the design of the fix is iffy\".\n\n> no, if only because there is currently no way to specify such an\n> entry in the .gitignore file. But it is an unusual thing to ask and\n> it is not worth optimizing for it by special casing at the top level\n> of the call chain.\n\nAlthough I agree with your proposed patch's sentiment of avoiding\nsweeping this corner case under the rug, 'check-ignore .' still\nwouldn't match anything if for example './' was a supported mechanism\nfor ignoring the top level. So, modulo the obvious advantages that\ndefensive coding in hash_name() bring, I'm struggling to see a\nsignificant difference between any of the three patches proposed so\nfar. All of them fix the segfault, and all would require the same\namount of extra work in order to support matching against './'.\n\nBut I don't have any strong objections either, so you have my approval\nfor whichever patch you prefer to take. Thanks.\n"},{"id":"209877","messageId":"20130220020046.GC7860@pacific.linksys.moosehall","threadId":"32933","inReplyTo":"7v1ucc5b7n.fsf_-_@alter.siamese.dyndns.org","subject":"Re: Re* [PATCH 2/2] check-ignore.c: fix segfault with '.' argument from repo root","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2013-02-20T02:00:46Z","receivedAt":"2013-02-20T02:00:46Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"On Tue, Feb 19, 2013 at 11:56:44AM -0800, Junio C Hamano wrote:\n> Adam Spiers writes:\n> \n> > Fair enough. I'll reply to this with a new version.[0]\n> >\n> > [0] I wish there was a clean way to include the new version inline,\n> > but as I've noted before, there doesn't seem to be:\n> >\n> > http://article.gmane.org/gmane.comp.version-control.git/146110\n> \n> I find it easier to later find the patch if you made it a separate\n> follow-up like you did, but you can do it this way if you really\n> want to, using a scissors line, like so. Please do not try to be\n> creative and change the shape of scissors just for the sake of\n> chaning it.\n\n[snipped]\n\nOK, thanks for the information. IMHO it would be nice if 'git\nformat-patch' and 'git am' supported this style of inline patch\ninclusion, but maybe there are good reasons to discourage it?\n"},{"id":"209878","messageId":"7vtxp73dms.fsf@alter.siamese.dyndns.org","threadId":"32933","inReplyTo":"20130220015723.GB7860@pacific.linksys.moosehall","subject":"Re: [PATCH v2 2/2] check-ignore.c, dir.c: fix segfault with '.' argument from repo root","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-20T02:47:23Z","receivedAt":"2013-02-20T02:47:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Spiers writes:\n\n>> Remove a sweep-the-issue-under-the-rug conditional in check-ignore\n>> that avoided to pass an empty string to the callchain while at it.\n>> It is a valid question to ask for check-ignore if the top-level is\n>> set to be ignored by default, even though the answer is most likely\n>> no, if only because there is currently no way to specify such an\n>\n> Hmm, I see very little difference between the use of \"most likely\" and\n> the use of the words \"much\" and \"typically\" which you previously\n> considered \"a sure sign that the design of the fix is iffy\".\n\nYour patch were \"The reason why feeding empty string upsets\nhash_name() were not investigated; by punting the '.' as input, and\nignoring the possibility that such a question might make sense, I\ncan work around the segfault. I do not even question if hash_name()\nthat misbehaves on an empty string is a bug. Just make sure we do\nnot tickle the function with a problematic input\".\n\nThe patch you are responding to declares that hash_name() should\nwork sensibly on an empty string, and that is the _only_ necessary\nchange for the fix. We could keep \"&& path[0]\", but even without\nit, by fixing the hash_name(), we will no longer segfault.\n\nMy \"most likely\" is about \"the special case '&& path[0]' produces\ncorrect result, and it is likely to stay so in the near future until\nwe update .gitignore format to allow users to say 'ignore the top by\ndefault', which is not likely to happen soon\". It is not about the\nnature of the fix at all.\n\nStill do not see the difference?\n\nThe removal of the \"&& path[0]\" is about allowing such a question\nwhose likeliness may be remote. In the current .gitignore format,\nyou may not be able to say \"ignore the whole thing by default\", so\nin that sense, the answer to the question this part of the code is\nasking when given \".\" may always be \"no\". Keeping the \"&& path[0]\"\nwill optimize for that case.\n\nAnd \"unusual thing to ask\" below is to judge if answering such a\nquestion is worth optimizing for (the verdict is \"no, it is not a\ncommon thing to do\").\n\n>> entry in the .gitignore file. But it is an unusual thing to ask and\n>> it is not worth optimizing for it by special casing at the top level\n>> of the call chain.\n>\n> Although I agree with your proposed patch's sentiment of avoiding\n> sweeping this corner case under the rug, 'check-ignore .' still\n> wouldn't match anything if for example './' was a supported mechanism\n> for ignoring the top level.\n\nIt indicates that there may be more bugs (that may not result in\nsegv) somewhere in check-ignore codepath, if (1)\n\n\techo ./ >.gitignore\n \nwere to say \"ignore everything in the tree by default.\", and (2) the\nreal ignore check does work that way, but (3)\n\n\tgit check-ignore .\n\nsays \"we do not ignore that one\". Such a bug may come from some\ncode that is not prepared to see an empty pathname that refers to\nthe top-level in the codepath, which was why I originally asked \n\n Does the callchain that goes down to this function have other places\n that assume they will never see an empty string, like this function\n does, which I _think_ is the real issue?\n\nin one of the previous messages.\n\nBut is \n\n\techo ./ >.gitignore\n\na way to say \"ignore everything in the tree by default\" in the first\nplace? I think historically that has never been the case, I recall\nthat the list had discussions on that topic in the past (it may be\nbefore you appeared here), and I do not recall we reached a concensus\nthat we should make it mean that nor applied a patch to do so.\n"},{"id":"209879","messageId":"7vppzv3dd8.fsf@alter.siamese.dyndns.org","threadId":"32933","inReplyTo":"20130220020046.GC7860@pacific.linksys.moosehall","subject":"Re: Re* [PATCH 2/2] check-ignore.c: fix segfault with '.' argument from repo root","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-20T02:53:07Z","receivedAt":"2013-02-20T02:53:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Spiers writes:\n\n> OK, thanks for the information. IMHO it would be nice if 'git\n> format-patch' and 'git am' supported this style of inline patch\n> inclusion, but maybe there are good reasons to discourage it?\n\n\"git am --scissors\" is a way to process such e-mail where the patch\nsubmitter continues discussion in the top part of a message,\nconcludes the message with:\n\n\tA patch to do so is attached.\n\t-- >8 --\n\nand then tells the MUA to read in an output from format-patch into\nthe e-mail buffer. You still need to strip out unneeded headers\nlike the \"From \", \"From: \" and \"Date: \" lines when you add the\nscissors anyway, and this is applicable only for a single-patch\nseries, so the \"feature\" does not fit well as a format-patch option.\n"},{"id":"209906","messageId":"20130220104720.GD7860@pacific.linksys.moosehall","threadId":"32933","inReplyTo":"7vppzv3dd8.fsf@alter.siamese.dyndns.org","subject":"Re: Re* [PATCH 2/2] check-ignore.c: fix segfault with '.' argument from repo root","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2013-02-20T10:47:20Z","receivedAt":"2013-02-20T10:47:20Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"On Tue, Feb 19, 2013 at 06:53:07PM -0800, Junio C Hamano wrote:\n> Adam Spiers writes:\n> \n> > OK, thanks for the information. IMHO it would be nice if 'git\n> > format-patch' and 'git am' supported this style of inline patch\n> > inclusion, but maybe there are good reasons to discourage it?\n> \n> \"git am --scissors\" is a way to process such e-mail where the patch\n> submitter continues discussion in the top part of a message,\n> concludes the message with:\n> \n> \tA patch to do so is attached.\n> \t-- >8 --\n> \n> and then tells the MUA to read in an output from format-patch into\n> the e-mail buffer.\n\nAh, nice! I didn't know about that.\n\n> You still need to strip out unneeded headers\n> like the \"From \", \"From: \" and \"Date: \" lines when you add the\n> scissors anyway, and this is applicable only for a single-patch\n> series, so the \"feature\" does not fit well as a format-patch option.\n\nRather than requiring the user to manually strip out unneeded headers,\nwouldn't it be friendlier and less error-prone to add a new --inline\noption to format-patch which omitted them in the first place? It\nshould be easy to make it bail with an error when multiple revisions\nare requested.\n"},{"id":"209911","messageId":"20130220124311.GE7860@pacific.linksys.moosehall","threadId":"32933","inReplyTo":"7vtxp73dms.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] check-ignore.c, dir.c: fix segfault with '.' argument from repo root","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2013-02-20T12:43:11Z","receivedAt":"2013-02-20T12:43:11Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"On Tue, Feb 19, 2013 at 06:47:23PM -0800, Junio C Hamano wrote:\n> Adam Spiers writes:\n> >> Remove a sweep-the-issue-under-the-rug conditional in check-ignore\n> >> that avoided to pass an empty string to the callchain while at it.\n> >> It is a valid question to ask for check-ignore if the top-level is\n> >> set to be ignored by default, even though the answer is most likely\n> >> no, if only because there is currently no way to specify such an\n> >\n> > Hmm, I see very little difference between the use of \"most likely\" and\n> > the use of the words \"much\" and \"typically\" which you previously\n> > considered \"a sure sign that the design of the fix is iffy\".\n> \n> Your patch were \"The reason why feeding empty string upsets\n ^^^^^^^^^^\n\"patches were\", or \"patch was\"? It's not clear which patch(es) you're\nreferring to.\n\n> hash_name() were not investigated; by punting the '.' as input, and\n> ignoring the possibility that such a question might make sense, I\n> can work around the segfault.\n\nI don't see how explicitly referring to the possibility can be counted\nas ignoring it.\n\n> I do not even question if hash_name()\n> that misbehaves on an empty string is a bug. Just make sure we do\n> not tickle the function with a problematic input\".\n\nPresumably the \"I\" here refers to anthropomorphized commit message\nrather than to me personally, since I did question hash_name()'s\nbehaviour several times already.\n\n> The patch you are responding to declares that hash_name() should\n> work sensibly on an empty string, and that is the _only_ necessary\n> change for the fix. We could keep \"&& path[0]\", but even without\n> it, by fixing the hash_name(), we will no longer segfault.\n\nYes, and as already stated, I agree that is a good thing.\n\n> My \"most likely\" is about \"the special case '&& path[0]' produces\n> correct result,\n\nSorry, I can't understand this. You are paraphrasing something and\nplacing it inside \"\" quotes, but I can't find the corresponding\nsource. I presumed it refers to this extract of your proposed patch's\ncommit message:\n\n \"Remove a sweep-the-issue-under-the-rug conditional in check-ignore\n that avoided to pass an empty string to the callchain while at it.\n It is a valid question to ask for check-ignore if the top-level is\n set to be ignored by default, even though the answer is most\n likely no\"\n\nbut I can't reconcile this extract with the paraphrase \"the special\ncase '&& path[0]' produces correct result\".\n\n> and it is likely to stay so in the near future until\n> we update .gitignore format to allow users to say 'ignore the top by\n> default', which is not likely to happen soon\". It is not about the\n> nature of the fix at all.\n>\n> Still do not see the difference?\n\nI think I *might* be beginning to see you were getting at, although my\nunderstanding is still clouded by the ambiguities detailed above. Is\nyour point that the use of words like 'much' and 'typically' are a\n\"sure sign\" of \"iffy design\" _when_used_to_talk_about_fixes_ but not\nnecessarily in other contexts? If so then it makes a bit more sense\nto me, even though I tend to disagree with such broadly sweeping\ngeneralizations, especially when the qualifying context is missing.\n\nThat aside, your idea of looking out for \"bad smells\" not only in code\nbut also in the spoken language contained by commit messages and\ndesign discussions is an interesting one. I will try to bear that\ntechnique in mind more consciously in the future, and see how well it\nserves me.\n\n> The removal of the \"&& path[0]\" is about allowing such a question\n> whose likeliness may be remote. In the current .gitignore format,\n> you may not be able to say \"ignore the whole thing by default\", so\n> in that sense, the answer to the question this part of the code is\n> asking when given \".\" may always be \"no\". Keeping the \"&& path[0]\"\n> will optimize for that case.\n> \n> And \"unusual thing to ask\" below is to judge if answering such a\n> question is worth optimizing for (the verdict is \"no, it is not a\n> common thing to do\").\n\nYes, I understand and agree with these paragraphs.\n\n> >> entry in the .gitignore file. But it is an unusual thing to ask and\n> >> it is not worth optimizing for it by special casing at the top level\n> >> of the call chain.\n> >\n> > Although I agree with your proposed patch's sentiment of avoiding\n> > sweeping this corner case under the rug, 'check-ignore .' still\n> > wouldn't match anything if for example './' was a supported mechanism\n> > for ignoring the top level.\n> \n> It indicates that there may be more bugs (that may not result in\n> segv) somewhere in check-ignore codepath, if (1)\n> \n> \techo ./ >.gitignore\n>\n> were to say \"ignore everything in the tree by default.\", and (2) the\n> real ignore check does work that way, but (3)\n> \n> \tgit check-ignore .\n> \n> says \"we do not ignore that one\".\n\nYes, I think we are saying exactly the same thing here, although if\n\"It indicates that [...]\" refers to your proposed patch's commit\nmessage then I don't think it indicates the possibility of these bugs\nin the most obvious or explicit way.\n\n> Such a bug may come from some\n> code that is not prepared to see an empty pathname that refers to\n> the top-level in the codepath, which was why I originally asked \n> \n> Does the callchain that goes down to this function have other places\n> that assume they will never see an empty string, like this function\n> does, which I _think_ is the real issue?\n> \n> in one of the previous messages.\n\nAgreed.\n\n> But is \n> \n> \techo ./ >.gitignore\n> \n> a way to say \"ignore everything in the tree by default\" in the first\n> place? I think historically that has never been the case, I recall\n> that the list had discussions on that topic in the past (it may be\n> before you appeared here), and I do not recall we reached a concensus\n> that we should make it mean that nor applied a patch to do so.\n\nGood question. './' and '/' both seem like reasonable values here,\nbut if noone screamed loud enough for a decision and implementation\nyet, it reinforces my description of it as a corner case, and suggests\nthat we probably shouldn't spend much more time on this thread of\ndiminishing returns ;-)\n"},{"id":"209987","messageId":"7vehg9v2xj.fsf@alter.siamese.dyndns.org","threadId":"32933","inReplyTo":"20130220104720.GD7860@pacific.linksys.moosehall","subject":"Re: Re* [PATCH 2/2] check-ignore.c: fix segfault with '.' argument from repo root","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-21T20:15:36Z","receivedAt":"2013-02-21T20:15:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Spiers writes:\n\n> On Tue, Feb 19, 2013 at 06:53:07PM -0800, Junio C Hamano wrote:\n>> Adam Spiers writes:\n>> \n>> > OK, thanks for the information. IMHO it would be nice if 'git\n>> > format-patch' and 'git am' supported this style of inline patch\n>> > inclusion, but maybe there are good reasons to discourage it?\n>> \n>> \"git am --scissors\" is a way to process such e-mail where the patch\n>> submitter continues discussion in the top part of a message,\n>> concludes the message with:\n>> \n>> \tA patch to do so is attached.\n>> \t-- >8 --\n>> \n>> and then tells the MUA to read in an output from format-patch into\n>> the e-mail buffer.\n>\n> Ah, nice! I didn't know about that.\n>\n>> You still need to strip out unneeded headers\n>> like the \"From \", \"From: \" and \"Date: \" lines when you add the\n>> scissors anyway, and this is applicable only for a single-patch\n>> series, so the \"feature\" does not fit well as a format-patch option.\n>\n> Rather than requiring the user to manually strip out unneeded headers,\n> wouldn't it be friendlier and less error-prone to add a new --inline\n> option to format-patch which omitted them in the first place? It\n> should be easy to make it bail with an error when multiple revisions\n> are requested.\n\nPerhaps.\n"},{"id":"209988","messageId":"7va9qxv2v3.fsf_-_@alter.siamese.dyndns.org","threadId":"32933","inReplyTo":"7vehg9v2xj.fsf@alter.siamese.dyndns.org","subject":"[PATCH 1/2] format-patch: rename \"no_inline\" field","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-21T20:17:04Z","receivedAt":"2013-02-21T20:17:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The name of the fields invites a misunderstanding that setting it to\nfalse, saying \"No, I will not to tell you not to inline\", make the\npatch inlined in the body of the message, but that is not what it\ndoes. The result is still a MIME attachment as long as\nmime_boundary is set. This field only controls if the content\ndisposition of a MIME attachment is set to \"attachment\" or \"inline\".\n\nRename it to clarify what it is used for. Besides, a toggle whose\nname is \"no_frotz\" is asking for a double-negation. Calling it\n\"disposition-attachment\" allows us to naturally read setting the\nfield to true as \"Yes, I want to set content-disposition to\n'attachment'\".\n\nSigned-off-by: Junio C Hamano \n---\n\n * This is a general \"clean-up\" patch that does not add any feature\n nor fixes any bug.\n\n builtin/log.c | 6 +++---\n log-tree.c | 2 +-\n revision.h | 2 +-\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 8f0b2e8..30265d8 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -983,7 +983,7 @@ static int attach_callback(const struct option *opt, const char *arg, int unset)\n \t\trev->mime_boundary = arg;\n \telse\n \t\trev->mime_boundary = git_version_string;\n-\trev->no_inline = unset ? 0 : 1;\n+\trev->disposition_attachment = unset ? 0 : 1;\n \treturn 0;\n }\n \n@@ -996,7 +996,7 @@ static int inline_callback(const struct option *opt, const char *arg, int unset)\n \t\trev->mime_boundary = arg;\n \telse\n \t\trev->mime_boundary = git_version_string;\n-\trev->no_inline = 0;\n+\trev->disposition_attachment = 0;\n \treturn 0;\n }\n \n@@ -1172,7 +1172,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \n \tif (default_attach) {\n \t\trev.mime_boundary = default_attach;\n-\t\trev.no_inline = 1;\n+\t\trev.disposition_attachment = 1;\n \t}\n \n \t/*\ndiff --git a/log-tree.c b/log-tree.c\nindex 5dc45c4..34ec20d 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -408,7 +408,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t \" filename=\\\"%s\\\"\\n\\n\",\n \t\t\t mime_boundary_leader, opt->mime_boundary,\n \t\t\t filename.buf,\n-\t\t\t opt->no_inline ? \"attachment\" : \"inline\",\n+\t\t\t opt->disposition_attachment ? \"attachment\" : \"inline\",\n \t\t\t filename.buf);\n \t\topt->diffopt.stat_sep = buffer;\n \t\tstrbuf_release(&filename);\ndiff --git a/revision.h b/revision.h\nindex 5da09ee..90813dd 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -142,7 +142,7 @@ struct rev_info {\n \tconst char\t*extra_headers;\n \tconst char\t*log_reencode;\n \tconst char\t*subject_prefix;\n-\tint\t\tno_inline;\n+\tint\t\tdisposition_attachment;\n \tint\t\tshow_log_size;\n \tstruct string_list *mailmap;\n \n-- \n1.8.2.rc0.129.gcce6fe7\n"},{"id":"209992","messageId":"7v4nh5v2fl.fsf_-_@alter.siamese.dyndns.org","threadId":"32933","inReplyTo":"7vehg9v2xj.fsf@alter.siamese.dyndns.org","subject":"[PATCH 2/2] format-patch: --inline-single","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-21T20:26:22Z","receivedAt":"2013-02-21T20:26:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Some people may find it convenient to append a simple patch at the\nbottom of a discussion e-mail separated by a \"scissors\" mark, ready\nto be applied with \"git am -c\". Introduce \"--inline-single\" option\nto format-patch to do so. A typical usage example might be to start\n'F'ollow-up to a discussion, write your message, conclude with \"a\npatch to do so may look like this.\", and \n\n \\C-u M-! git format-patch --inline-single -1 HEAD \n\nif you are an Emacs user. Users of other MUA's may want to consult\ntheir manuals to find equivalent command to append output from an\nexternal command to the message being composed.\n\nIt does not make any sense to use this mode when formatting multiple\npatches, or to combine this with options such as --attach, --inline,\nand --cover-letter, so some of such uses are forbidden. There may\nbe more insane combination the check in this patch may not even\nbother to reject. Caveat emptor.\n\nSigned-off-by: Junio C Hamano \n---\n\n * I did this as a lunch-time hack, but I'll leave it to interested\n readers as an exercise to find corner case \"bugs\", e.g. some\n insane combinations of options may not be diagnosed as usage\n errors, and to update the tests and documentation.\n\n Personally, \"git format-patch --stdout -1 HEAD\" with manual\n editing is more flexible, so I am not interested in spending\n cycles to polish this further myself.\n\n The preliminary patch 1/2 I sent earlier is worth doing, though.\n\n builtin/log.c | 32 ++++++++++++++++++++++++++++++++\n commit.h | 1 +\n log-tree.c | 7 ++++++-\n pretty.c | 27 ++++++++++++++++++++++++++-\n revision.h | 1 +\n 5 files changed, 66 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 30265d8..5ad0837 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1000,6 +1000,19 @@ static int inline_callback(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int inline_single_callback(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct rev_info *rev = (struct rev_info *)opt->value;\n+\trev->mime_boundary = NULL;\n+\trev->inline_single = 1;\n+\n+\t/* defeat configured format.attach, format.thread, etc. */\n+\tfree(default_attach);\n+\tdefault_attach = NULL;\n+\tthread = 0;\n+\treturn 0;\n+}\n+\n static int header_callback(const struct option *opt, const char *arg, int unset)\n {\n \tif (unset) {\n@@ -1149,6 +1162,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t PARSE_OPT_OPTARG, thread_callback },\n \t\tOPT_STRING(0, \"signature\", &signature, N_(\"signature\"),\n \t\t\t N_(\"add a signature\")),\n+\t\t{ OPTION_CALLBACK, 0, \"inline-single\", &rev, NULL,\n+\t\t N_(\"single patch appendable to the end of an e-mail body\"),\n+\t\t PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n+\t\t inline_single_callback },\n \t\tOPT_BOOLEAN(0, \"quiet\", &quiet,\n \t\t\t N_(\"don't print the patch filenames\")),\n \t\tOPT_END()\n@@ -1185,6 +1202,17 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |\n \t\t\t PARSE_OPT_KEEP_DASHDASH);\n \n+\t/* Set defaults and check incompatible options */\n+\tif (rev.inline_single) {\n+\t\tuse_stdout = 1;\n+\t\tif (cover_letter)\n+\t\t\tdie(_(\"inline-single and cover-letter are incompatible.\"));\n+\t\tif (thread)\n+\t\t\tdie(_(\"inline-single and thread are incompatible.\"));\n+\t\tif (output_directory)\n+\t\t\tdie(_(\"inline-single and output-directory are incompatible.\"));\n+\t}\n+\n \tif (0 < reroll_count) {\n \t\tstruct strbuf sprefix = STRBUF_INIT;\n \t\tstrbuf_addf(&sprefix, \"%s v%d\",\n@@ -1373,6 +1401,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tlist[nr - 1] = commit;\n \t}\n \ttotal = nr;\n+\n+\tif (rev.inline_single && total != 1)\n+\t\tdie(_(\"inline-single is only for a single commit\"));\n+\n \tif (!keep_subject && auto_number && total > 1)\n \t\tnumbered = 1;\n \tif (numbered)\ndiff --git a/commit.h b/commit.h\nindex 4138bb4..f3d9959 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -85,6 +85,7 @@ struct pretty_print_context {\n \tint preserve_subject;\n \tenum date_mode date_mode;\n \tunsigned date_mode_explicit:1;\n+\tunsigned inline_single:1;\n \tint need_8bit_cte;\n \tchar *notes_message;\n \tstruct reflog_walk_info *reflog_info;\ndiff --git a/log-tree.c b/log-tree.c\nindex 34ec20d..15c9749 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -358,7 +358,11 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\tsubject = \"Subject: \";\n \t}\n \n-\tprintf(\"From %s Mon Sep 17 00:00:00 2001\\n\", name);\n+\tif (opt->inline_single)\n+\t\tprintf(\"-- >8 --\\n\");\n+\telse\n+\t\tprintf(\"From %s Mon Sep 17 00:00:00 2001\\n\", name);\n+\n \tgraph_show_oneline(opt->graph);\n \tif (opt->message_id) {\n \t\tprintf(\"Message-Id: <%s>\\n\", opt->message_id);\n@@ -683,6 +687,7 @@ void show_log(struct rev_info *opt)\n \tctx.fmt = opt->commit_format;\n \tctx.mailmap = opt->mailmap;\n \tctx.color = opt->diffopt.use_color;\n+\tctx.inline_single = opt->inline_single;\n \tpretty_print_commit(&ctx, commit, &msgbuf);\n \n \tif (opt->add_signoff)\ndiff --git a/pretty.c b/pretty.c\nindex eae57ad..363b3d9 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -383,6 +383,29 @@ static void add_rfc2047(struct strbuf *sb, const char *line, int len,\n \tstrbuf_addstr(sb, \"?=\");\n }\n \n+static int is_current_user(const struct pretty_print_context *pp,\n+\t\t\t const char *email, size_t emaillen,\n+\t\t\t const char *name, size_t namelen)\n+{\n+\tconst char *me = git_committer_info(0);\n+\tconst char *myname, *mymail;\n+\tsize_t mynamelen, mymaillen;\n+\tstruct ident_split ident;\n+\n+\tif (split_ident_line(&ident, me, strlen(me)))\n+\t\treturn 0; /* play safe, as we do not know */\n+\tmymail = ident.mail_begin;\n+\tmymaillen = ident.mail_end - ident.mail_begin;\n+\tmyname = ident.name_begin;\n+\tmynamelen = ident.name_end - ident.name_begin;\n+\tif (pp->mailmap)\n+\t\tmap_user(pp->mailmap, &mymail, &mymaillen, &myname, &mynamelen);\n+\treturn (mymaillen == emaillen &&\n+\t\tmynamelen == namelen &&\n+\t\t!memcmp(mymail, email, emaillen) &&\n+\t\t!memcmp(myname, name, namelen));\n+}\n+\n void pp_user_info(const struct pretty_print_context *pp,\n \t\t const char *what, struct strbuf *sb,\n \t\t const char *line, const char *encoding)\n@@ -412,7 +435,6 @@ void pp_user_info(const struct pretty_print_context *pp,\n \tif (split_ident_line(&ident, line, linelen))\n \t\treturn;\n \n-\n \tmailbuf = ident.mail_begin;\n \tmaillen = ident.mail_end - ident.mail_begin;\n \tnamebuf = ident.name_begin;\n@@ -421,6 +443,9 @@ void pp_user_info(const struct pretty_print_context *pp,\n \tif (pp->mailmap)\n \t\tmap_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);\n \n+\tif (pp->inline_single && is_current_user(pp, mailbuf, maillen, namebuf, namelen))\n+\t\treturn;\n+\n \tstrbuf_init(&mail, 0);\n \tstrbuf_init(&name, 0);\n \ndiff --git a/revision.h b/revision.h\nindex 90813dd..0c6d955 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -143,6 +143,7 @@ struct rev_info {\n \tconst char\t*log_reencode;\n \tconst char\t*subject_prefix;\n \tint\t\tdisposition_attachment;\n+\tint\t\tinline_single;\n \tint\t\tshow_log_size;\n \tstruct string_list *mailmap;\n \n-- \n1.8.2.rc0.129.gcce6fe7\n"},{"id":"210001","messageId":"20130221231328.GA19808@sigill.intra.peff.net","threadId":"32933","inReplyTo":"7v4nh5v2fl.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] format-patch: --inline-single","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-21T23:13:28Z","receivedAt":"2013-02-21T23:13:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 21, 2013 at 12:26:22PM -0800, Junio C Hamano wrote:\n\n> Some people may find it convenient to append a simple patch at the\n> bottom of a discussion e-mail separated by a \"scissors\" mark, ready\n> to be applied with \"git am -c\". Introduce \"--inline-single\" option\n> to format-patch to do so. A typical usage example might be to start\n> 'F'ollow-up to a discussion, write your message, conclude with \"a\n> patch to do so may look like this.\", and\n> \n> \\C-u M-! git format-patch --inline-single -1 HEAD \n> \n> if you are an Emacs user. Users of other MUA's may want to consult\n> their manuals to find equivalent command to append output from an\n> external command to the message being composed.\n\nInteresting. I usually just do this by hand, but this could save a few\nkeystrokes in my workflow.\n\n> +static int is_current_user(const struct pretty_print_context *pp,\n> +\t\t\t const char *email, size_t emaillen,\n> +\t\t\t const char *name, size_t namelen)\n> +{\n> +\tconst char *me = git_committer_info(0);\n> +\tconst char *myname, *mymail;\n> +\tsize_t mynamelen, mymaillen;\n> +\tstruct ident_split ident;\n> +\n> +\tif (split_ident_line(&ident, me, strlen(me)))\n> +\t\treturn 0; /* play safe, as we do not know */\n> +\tmymail = ident.mail_begin;\n> +\tmymaillen = ident.mail_end - ident.mail_begin;\n> +\tmyname = ident.name_begin;\n> +\tmynamelen = ident.name_end - ident.name_begin;\n> +\tif (pp->mailmap)\n> +\t\tmap_user(pp->mailmap, &mymail, &mymaillen, &myname, &mynamelen);\n> +\treturn (mymaillen == emaillen &&\n> +\t\tmynamelen == namelen &&\n> +\t\t!memcmp(mymail, email, emaillen) &&\n> +\t\t!memcmp(myname, name, namelen));\n> +}\n\nNice, I'm glad you handled this case properly. I've wondered if we\nshould have an option to do a similar test when writing out the \"real\"\nmessage format. I.e., to put the extra \"From\" line in the body of the\nmessage when !is_current_user(). Traditionally we have just said \"that\nis the responsibility of the MUA you use\", and let send-email handle it.\nBut it means people who do not use send-email have to reimplement the\nfeature themselves.\n\n> @@ -421,6 +443,9 @@ void pp_user_info(const struct pretty_print_context *pp,\n> \tif (pp->mailmap)\n> \t\tmap_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);\n> \n> +\tif (pp->inline_single && is_current_user(pp, mailbuf, maillen, namebuf, namelen))\n> +\t\treturn;\n> +\n> \tstrbuf_init(&mail, 0);\n> \tstrbuf_init(&name, 0);\n\nThis makes sense to suppress the user line when it is not necessary. But\nwe should probably always be suppressing the Date line, as it is almost\nalways useless.\n\nI also wonder if we should suppress the subject-prefix in such a case,\nas it is not adding anything (it is not the subject of the email, so it\ndoes not need to grab attention there, and it will not make it into the\nfinal commit). On the other hand, having tried it, the \"Subject:\" looks\na little lonely without it. Perhaps the [PATCH] is still necessary to\ngrab attention after the scissors line. I dunno.\n\nPatch for both below if you want to pick up either suggestion.\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 15c9749..8994354 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -348,7 +348,8 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,\n \t\t\t digits_in_number(opt->total),\n \t\t\t opt->nr, opt->total);\n \t\tsubject = buffer;\n-\t} else if (opt->total == 0 && opt->subject_prefix && *opt->subject_prefix) {\n+\t} else if (opt->total == 0 && !opt->inline_single &&\n+\t\t opt->subject_prefix && *opt->subject_prefix) {\n \t\tstatic char buffer[256];\n \t\tsnprintf(buffer, sizeof(buffer),\n \t\t\t \"Subject: [%s] \",\ndiff --git a/pretty.c b/pretty.c\nindex 363b3d9..1a7352c 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -490,7 +490,8 @@ void pp_user_info(const struct pretty_print_context *pp,\n \t\tstrbuf_addf(sb, \"Date: %s\\n\", show_date(time, tz, pp->date_mode));\n \t\tbreak;\n \tcase CMIT_FMT_EMAIL:\n-\t\tstrbuf_addf(sb, \"Date: %s\\n\", show_date(time, tz, DATE_RFC2822));\n+\t\tif (!pp->inline_single)\n+\t\t\tstrbuf_addf(sb, \"Date: %s\\n\", show_date(time, tz, DATE_RFC2822));\n \t\tbreak;\n \tcase CMIT_FMT_FULLER:\n \t\tstrbuf_addf(sb, \"%sDate: %s\\n\", what, show_date(time, tz, pp->date_mode));\n"},{"id":"210008","messageId":"7v7gm1teuf.fsf@alter.siamese.dyndns.org","threadId":"32933","inReplyTo":"20130221231328.GA19808@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] format-patch: --inline-single","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-21T23:41:12Z","receivedAt":"2013-02-21T23:41:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> @@ -421,6 +443,9 @@ void pp_user_info(const struct pretty_print_context *pp,\n>> \tif (pp->mailmap)\n>> \t\tmap_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);\n>> \n>> +\tif (pp->inline_single && is_current_user(pp, mailbuf, maillen, namebuf, namelen))\n>> +\t\treturn;\n>> +\n>> \tstrbuf_init(&mail, 0);\n>> \tstrbuf_init(&name, 0);\n>\n> This makes sense to suppress the user line when it is not necessary. But\n> we should probably always be suppressing the Date line, as it is almost\n> always useless.\n\nWhen I (figuratively) am sending my patch in a discussion, saying\n\"You could do it this way\", on the other hand, I agree that the date\nis uninteresting.\n\nI however think I would prefer to keep the Date: line when I am\nrelaying somebody else's work during a discussion. It is more like\n\"Yeah, Peff already did that with this commit; here it is for\nreference\". The fact that I have _your_ patch makes it more \"done\",\nthan the case I send out my own patch.\n\nBesides, removing an extra line in the MUA editor is far easier than\nhaving to type what the tool \"helpfully\" omitted, guided by an \"it\nis almost always useless\" that is not backed by the user preference.\nI'd rather err on the side of giving extra than omitting too much.\n\n> I also wonder if we should suppress the subject-prefix in such a case,\n> as it is not adding anything (it is not the subject of the email, so it\n> does not need to grab attention there, and it will not make it into the\n> final commit).\n\nIf the user does not want to waste too much space in the message,\nnot passing the --subject-prefix=foo from the command line, or\nediting it out in the editor buffer if for some reason the user ran\nthe command with the option, are both easy things to do. I do not\nthink extra lines to excise subject prefix is not worth it, and who\nknows what the user's preferences are.\n\nBut there is something more important.\n\nWe should make sure that we disable MIMEy stuff (i.e. MIME-Version,\nC-T-E: 8bit/quoted-printable, Content-type, etc.) when producing the\noutput to be appended to the body, which should be just a straight\n8-bit text. I do not think the posted patch tries to do anything to\nthat effect.\n"},{"id":"210009","messageId":"7v38wptekq.fsf@alter.siamese.dyndns.org","threadId":"32933","inReplyTo":"7v7gm1teuf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] format-patch: --inline-single","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-21T23:47:01Z","receivedAt":"2013-02-21T23:47:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>>> @@ -421,6 +443,9 @@ void pp_user_info(const struct pretty_print_context *pp,\n>>> \tif (pp->mailmap)\n>>> \t\tmap_user(pp->mailmap, &mailbuf, &maillen, &namebuf, &namelen);\n>>> \n>>> +\tif (pp->inline_single && is_current_user(pp, mailbuf, maillen, namebuf, namelen))\n>>> +\t\treturn;\n>>> +\n>>> \tstrbuf_init(&mail, 0);\n>>> \tstrbuf_init(&name, 0);\n>>\n>> This makes sense to suppress the user line when it is not necessary. But\n>> we should probably always be suppressing the Date line, as it is almost\n>> always useless.\n>\n> When I (figuratively) am sending my patch in a discussion, saying\n> \"You could do it this way\", on the other hand, I agree that the date\n> is uninteresting.\n\nJust in case somebody is wondering, please s/, on the other hand//;\nabove. I swapped the paragraphs after I wrote them X-<.\n"},{"id":"210030","messageId":"20130222153209.GF7860@pacific.linksys.moosehall","threadId":"32933","inReplyTo":"20130221231328.GA19808@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] format-patch: --inline-single","fromName":"Adam Spiers","fromEmail":"git@adamspiers.org","sentAt":"2013-02-22T15:32:09Z","receivedAt":"2013-02-22T15:32:09Z","isPatch":true,"sender":{"key":"git@adamspiers.org","avatar":"https://avatars.githubusercontent.com/u/100738?v=4"},"body":"On Thu, Feb 21, 2013 at 06:13:28PM -0500, Jeff King wrote:\n> On Thu, Feb 21, 2013 at 12:26:22PM -0800, Junio C Hamano wrote:\n> \n> > Some people may find it convenient to append a simple patch at the\n> > bottom of a discussion e-mail separated by a \"scissors\" mark, ready\n> > to be applied with \"git am -c\". Introduce \"--inline-single\" option\n> > to format-patch to do so. A typical usage example might be to start\n> > 'F'ollow-up to a discussion, write your message, conclude with \"a\n> > patch to do so may look like this.\", and\n> > \n> > \\C-u M-! git format-patch --inline-single -1 HEAD \n> > \n> > if you are an Emacs user. Users of other MUA's may want to consult\n> > their manuals to find equivalent command to append output from an\n> > external command to the message being composed.\n> \n> Interesting. I usually just do this by hand, but this could save a few\n> keystrokes in my workflow.\n\nSame here. This is great; thanks a lot both for working on it!\n"},{"id":"210032","messageId":"7vmwuws3bo.fsf@alter.siamese.dyndns.org","threadId":"32933","inReplyTo":"20130221231328.GA19808@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] format-patch: --inline-single","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-22T16:47:39Z","receivedAt":"2013-02-22T16:47:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> ... ...\n>> +}\n>\n> Nice, I'm glad you handled this case properly. I've wondered if we\n> should have an option to do a similar test when writing out the \"real\"\n> message format. I.e., to put the extra \"From\" line in the body of the\n> message when !is_current_user(). Traditionally we have just said \"that\n> is the responsibility of the MUA you use\", and let send-email handle it.\n> But it means people who do not use send-email have to reimplement the\n> feature themselves.\n\nI am not sure if I follow. Do you mean that you have to remove\nfewer lines if you omit Date/From when it is from you in the first\nplace? People who do not use send-email (like me) slurp the output\n0001-have-gostak-distim-doshes.patch into their MUA editor, tell the\nMUA to use the contents on the Subject: line as the subject, and\nremove what is redundant, including the Subject. Because the output\ncannot be used as-is anyway, I do not think it is such a big deal.\n\nAnd those who have a custom mechanism to stuff our output in their\nMUA's outbox, similar to what imap-send does, would already have to\nhave a trivial parser to read the first part of our output up to the\nfirst blank line (i.e. parsing out the header part) and formatting\nthe information it finds into a form that is understood by their\nMUA. Omitting From: or Date: lines would not help those people who\nalready have established the procedure to handle the \"Oh, this one\nis from me\" case, or to send the output always with the Sender: and\nkeeping the From: intact. So,...\n\n \n"},{"id":"210038","messageId":"20130222172353.GA17475@sigill.intra.peff.net","threadId":"32933","inReplyTo":"7vmwuws3bo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] format-patch: --inline-single","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-22T17:23:53Z","receivedAt":"2013-02-22T17:23:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 22, 2013 at 08:47:39AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> ... ...\n> >> +}\n> >\n> > Nice, I'm glad you handled this case properly. I've wondered if we\n> > should have an option to do a similar test when writing out the \"real\"\n> > message format. I.e., to put the extra \"From\" line in the body of the\n> > message when !is_current_user(). Traditionally we have just said \"that\n> > is the responsibility of the MUA you use\", and let send-email handle it.\n> > But it means people who do not use send-email have to reimplement the\n> > feature themselves.\n> \n> I am not sure if I follow. Do you mean that you have to remove\n> fewer lines if you omit Date/From when it is from you in the first\n> place?\n\nSorry, I think I confused you by going off on a tangent. The rest of my\nemail was about dropping unnecessary lines from the inline view. But\nhere I was talking about another possible use of the \"is user the\nauthor\" function. For the existing view, we show:\n\n From: A U Thor \n Date: ...\n Subject: [PATCH] whatever\n\n body\n\nand if committer != author, we expect the MUA to convert that to:\n\n From: C O Mitter \n Date: ...\n Subject: [PATCH] whatever\n\n From: A U Thor \n\n body\n\nThat logic happens in git-send-email right now, but given that your\npatch adds the \"are we the author?\" function, it would be trivial to add\na \"--sender-is-committer\" option to format-patch to have it do it\nautomatically. That saves the MUA from having to worry about it.\n\n> People who do not use send-email (like me) slurp the output\n> 0001-have-gostak-distim-doshes.patch into their MUA editor, tell the\n> MUA to use the contents on the Subject: line as the subject, and\n> remove what is redundant, including the Subject. Because the output\n> cannot be used as-is anyway, I do not think it is such a big deal.\n\nThat is one way to do it. Another way is to hand the output of\nformat-patch to your MUA as a template, making it a starting point for a\nmessage we are about to send. No manual editing is necessary in that\ncase, unless the \"From\" header does not match the sender identity.\n\n> And those who have a custom mechanism to stuff our output in their\n> MUA's outbox, similar to what imap-send does, would already have to\n> have a trivial parser to read the first part of our output up to the\n> first blank line (i.e. parsing out the header part) and formatting\n> the information it finds into a form that is understood by their\n> MUA.\n\nNot necessarily. The existing format is an rfc822 message, which mailers\nunderstand already. It's perfectly cromulent to do:\n\n git format-patch --stdout \"$@\" >mbox &&\n mutt -f mbox\n\nand use mutt's \"resend-message\" as a starting point for sending each\nmessage. No editing is necessary except for adding recipients (which you\ncan also do on the command-line to format-patch).\n\n> Omitting From: or Date: lines would not help those people who\n> already have established the procedure to handle the \"Oh, this one\n> is from me\" case, or to send the output always with the Sender: and\n> keeping the From: intact. So,...\n\nRight, my point was to help people who _should_ have implemented the\n\"oh, this one is from me\" case, but were too lazy to do so (and it's\nactually a little tricky to get right, because you might have to adjust\nthe mime headers to account for encoded author names).\n\n-Peff\n"}]}