{"thread":{"id":"22671","subject":"[PATCH 1/2] bugfix: segfault on git diff --output=/bad/path","startedAt":"2010-02-16T04:10:45Z","lastAt":"2010-02-16T06:55:21Z","messageCount":5,"participants":["Larry D'Anna","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"134695","messageId":"1266293446-8092-1-git-send-email-larry@elder-gods.org","threadId":"22671","inReplyTo":null,"subject":"[PATCH 1/2] bugfix: segfault on git diff --output=/bad/path","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-16T04:10:45Z","receivedAt":"2010-02-16T04:10:45Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"The return value from fopen wasn't being checked.\n\nSigned-off-by: Larry D'Anna \n---\n diff.c | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 381cc8d..68def6c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2893,6 +2893,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\t;\n \telse if (!prefixcmp(arg, \"--output=\")) {\n \t\toptions->file = fopen(arg + strlen(\"--output=\"), \"w\");\n+\t\tif (!options->file)\n+\t\t\tdie_errno(\"Could not open '%s'\", arg + strlen(\"--output=\"));\n \t\toptions->close_file = 1;\n \t} else\n \t\treturn 0;\n-- \n1.7.0.rc2.40.g7d8aa\n"},{"id":"134696","messageId":"1266293446-8092-2-git-send-email-larry@elder-gods.org","threadId":"22671","inReplyTo":"1266293446-8092-1-git-send-email-larry@elder-gods.org","subject":"[PATCH 2/2] bugfix: git diff --quiet -w never returns with exit status 1","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-16T04:10:46Z","receivedAt":"2010-02-16T04:10:46Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"The problem: -w causes the flag DIFF_FROM_CONTENTS to be set, which causes\ndiff_flush to set the flag HAS_CHANGES based on options->found_changes, which is\nset by diff_flush_patch (if there were any changes). However, --quiet causes\ndiff_flush to never call diff_flush_patch, so options->found_changes is always 0.\n\nThe solution: In this situation, call diff_flush_patch with options->file set to\n/dev/null.\n\nRationale: diff_flush_patch expects to write its output to options->file.\nAdding a \"silence\" flag to diff_flush_patch and everything it calls would be\nmore invasive.\n\nSigned-off-by: Larry D'Anna \n---\n diff.c | 20 ++++++++++++++++++++\n 1 files changed, 20 insertions(+), 0 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 68def6c..ff00816 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3522,6 +3522,26 @@ void diff_flush(struct diff_options *options)\n \t\tseparator++;\n \t}\n \n+\tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n+\t DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&\n+\t DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {\n+\t\t/* run diff_flush_patch for the exit status */\n+\t\t/* setting options->file to /dev/null should be safe, becaue we\n+\t\t aren't supposed to produce any output anyways */\n+\t\tstatic FILE *devnull = NULL;\n+\t\tif(!devnull) {\n+\t\t\tdevnull = fopen(\"/dev/null\", \"w\");\n+\t\t\tif (!devnull)\n+\t\t\t\tdie_errno(\"Could not open /dev/null\");\n+\t\t}\n+\t\toptions->file = devnull;\n+\t\tfor (i = 0; i < q->nr; i++) {\n+\t\t\tstruct diff_filepair *p = q->queue[i];\n+\t\t\tif (check_pair_status(p))\n+\t\t\t\tdiff_flush_patch(p, options);\n+\t\t}\n+\t}\n+\n \tif (output_format & DIFF_FORMAT_PATCH) {\n \t\tif (separator) {\n \t\t\tputc(options->line_termination, options->file);\n-- \n1.7.0.rc2.40.g7d8aa\n"},{"id":"134699","messageId":"7v3a11ivmz.fsf@alter.siamese.dyndns.org","threadId":"22671","inReplyTo":"1266293446-8092-2-git-send-email-larry@elder-gods.org","subject":"Re: [PATCH 2/2] bugfix: git diff --quiet -w never returns with exit status 1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-16T05:42:44Z","receivedAt":"2010-02-16T05:42:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Larry D'Anna writes:\n\n> Rationale: diff_flush_patch expects to write its output to options->file.\n> Adding a \"silence\" flag to diff_flush_patch and everything it calls would be\n> more invasive.\n\nI would agree that the logic to redirect the output to nowhere may be the\neasiest way out, but because the reason anybody sane would want to give -q\nis to say \"I don't care what the actual changes are, but I want to know if\nthere is any real quick\" (otherwise the call would be \"diff -w >/dev/null\"),\nshouldn't we at least be exiting the loop early when we see any difference?\n\n> Signed-off-by: Larry D'Anna \n> ---\n> diff.c | 20 ++++++++++++++++++++\n> 1 files changed, 20 insertions(+), 0 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 68def6c..ff00816 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3522,6 +3522,26 @@ void diff_flush(struct diff_options *options)\n> \t\tseparator++;\n> \t}\n> \n> +\tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n> +\t DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&\n> +\t DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {\n> +\t\t/* run diff_flush_patch for the exit status */\n> +\t\t/* setting options->file to /dev/null should be safe, becaue we\n> +\t\t aren't supposed to produce any output anyways */\n\nStyle?\n\n> +\t\tstatic FILE *devnull = NULL;\n\nWould this cause one file descriptor to leak? Do we care?\n\n> +\t\tif(!devnull) {\n\nStyle?\tif (!devnull)\n\n> +\t\t\tdevnull = fopen(\"/dev/null\", \"w\");\n> +\t\t\tif (!devnull)\n> +\t\t\t\tdie_errno(\"Could not open /dev/null\");\n> +\t\t}\n> +\t\toptions->file = devnull;\n\nWould this cause the original \"options->file\" leak? Do we care?\n\n> +\t\tfor (i = 0; i < q->nr; i++) {\n> +\t\t\tstruct diff_filepair *p = q->queue[i];\n> +\t\t\tif (check_pair_status(p))\n> +\t\t\t\tdiff_flush_patch(p, options);\n> +\t\t}\n> +\t}\n> +\n> \tif (output_format & DIFF_FORMAT_PATCH) {\n> \t\tif (separator) {\n> \t\t\tputc(options->line_termination, options->file);\n> -- \n> 1.7.0.rc2.40.g7d8aa\n"},{"id":"134704","messageId":"20100216064539.GA18741@cthulhu","threadId":"22671","inReplyTo":"7v3a11ivmz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] bugfix: git diff --quiet -w never returns with exit status 1","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-16T06:45:39Z","receivedAt":"2010-02-16T06:45:39Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"* Junio C Hamano (gitster@pobox.com) [100216 00:42]:\n> Larry D'Anna writes:\n> \n> > Rationale: diff_flush_patch expects to write its output to options->file.\n> > Adding a \"silence\" flag to diff_flush_patch and everything it calls would be\n> > more invasive.\n> \n> I would agree that the logic to redirect the output to nowhere may be the\n> easiest way out, but because the reason anybody sane would want to give -q\n> is to say \"I don't care what the actual changes are, but I want to know if\n> there is any real quick\" (otherwise the call would be \"diff -w >/dev/null\"),\n> shouldn't we at least be exiting the loop early when we see any difference?\n> \n> > Signed-off-by: Larry D'Anna \n> > ---\n> > diff.c | 20 ++++++++++++++++++++\n> > 1 files changed, 20 insertions(+), 0 deletions(-)\n> >\n> > diff --git a/diff.c b/diff.c\n> > index 68def6c..ff00816 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -3522,6 +3522,26 @@ void diff_flush(struct diff_options *options)\n> > \t\tseparator++;\n> > \t}\n> > \n> > +\tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n> > +\t DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&\n> > +\t DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {\n> > +\t\t/* run diff_flush_patch for the exit status */\n> > +\t\t/* setting options->file to /dev/null should be safe, becaue we\n> > +\t\t aren't supposed to produce any output anyways */\n> \n> Style?\n> \n> > +\t\tstatic FILE *devnull = NULL;\n> \n> Would this cause one file descriptor to leak? Do we care?\n\nOriginally I thought it would be best to just let one leak, because I didn't\nknow how much longer it would need to stick around. I didn't notice it's being\nclosed anyway a few lines down.\n\n\n> > +\t\tif(!devnull) {\n> \n> Style?\tif (!devnull)\n> \n> > +\t\t\tdevnull = fopen(\"/dev/null\", \"w\");\n> > +\t\t\tif (!devnull)\n> > +\t\t\t\tdie_errno(\"Could not open /dev/null\");\n> > +\t\t}\n> > +\t\toptions->file = devnull;\n> \n> Would this cause the original \"options->file\" leak? Do we care?\n\noops.\n\n> > +\t\tfor (i = 0; i < q->nr; i++) {\n> > +\t\t\tstruct diff_filepair *p = q->queue[i];\n> > +\t\t\tif (check_pair_status(p))\n> > +\t\t\t\tdiff_flush_patch(p, options);\n> > +\t\t}\n> > +\t}\n> > +\n> > \tif (output_format & DIFF_FORMAT_PATCH) {\n> > \t\tif (separator) {\n> > \t\t\tputc(options->line_termination, options->file);\n> > -- \n> > 1.7.0.rc2.40.g7d8aa\n> \n"},{"id":"134707","messageId":"1266303321-28337-1-git-send-email-larry@elder-gods.org","threadId":"22671","inReplyTo":"20100216064539.GA18741@cthulhu","subject":"[PATCH 2/2] bugfix: git diff --quiet -w never returns with exit status 1","fromName":"Larry D'Anna","fromEmail":"larry@elder-gods.org","sentAt":"2010-02-16T06:55:21Z","receivedAt":"2010-02-16T06:55:21Z","isPatch":true,"sender":{"key":"larry@elder-gods.org","avatar":"https://avatars.githubusercontent.com/u/3013304?v=4"},"body":"The problem: -w causes the flag DIFF_FROM_CONTENTS to be set, which causes\ndiff_flush to set the flag HAS_CHANGES based on options->found_changes, which is\nset by diff_flush_patch (if there were any changes). However, --quiet causes\ndiff_flush to never call diff_flush_patch, so options->found_changes is always 0.\n\nThe solution: In this situation, call diff_flush_patch with options->file set to\n/dev/null.\n\nRationale: diff_flush_patch expects to write its output to options->file.\nAdding a \"silence\" flag to diff_flush_patch and everything it calls would be\nmore invasive.\n\nSigned-off-by: Larry D'Anna \n---\n diff.c | 23 +++++++++++++++++++++++\n 1 files changed, 23 insertions(+), 0 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 68def6c..2984c41 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3522,6 +3522,29 @@ void diff_flush(struct diff_options *options)\n \t\tseparator++;\n \t}\n \n+\tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n+\t DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&\n+\t DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {\n+\t\t/*\n+\t\t * run diff_flush_patch for the exit status.\n+\t\t * setting options->file to /dev/null should be safe, becaue we\n+\t\t * aren't supposed to produce any output anyways\n+\t\t */\n+\t\tif (options->close_file)\n+\t\t\tfclose(options->file);\n+\t\toptions->file = fopen(\"/dev/null\", \"w\");\n+\t\tif (!options->file)\n+\t\t\tdie_errno(\"Could not open /dev/null\");\n+\t\toptions->close_file = 1;\n+\t\tfor (i = 0; i < q->nr; i++) {\n+\t\t\tstruct diff_filepair *p = q->queue[i];\n+\t\t\tif (check_pair_status(p))\n+\t\t\t\tdiff_flush_patch(p, options);\n+\t\t\tif (options->found_changes)\n+\t\t\t\tbreak;\n+\t\t}\n+\t}\n+\n \tif (output_format & DIFF_FORMAT_PATCH) {\n \t\tif (separator) {\n \t\t\tputc(options->line_termination, options->file);\n-- \n1.7.0.rc2.40.g7d8aa\n"}]}