[PATCH 1/2] bugfix: segfault on git diff --output=/bad/path

[PATCH 1/2] bugfix: segfault on git diff --output=/bad/path

[Larry D'Anna]

The return value from fopen wasn't being checked.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 diff.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/diff.c b/diff.c
index 381cc8d..68def6c 100644
--- a/diff.c
+++ b/diff.c
@@ -2893,6 +2893,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		;
 	else if (!prefixcmp(arg, "--output=")) {
 		options->file = fopen(arg + strlen("--output="), "w");
+		if (!options->file)
+			die_errno("Could not open '%s'", arg + strlen("--output="));
 		options->close_file = 1;
 	} else
 		return 0;
-- 
1.7.0.rc2.40.g7d8aa

[PATCH 2/2] bugfix: git diff --quiet -w never returns with exit status 1

[Larry D'Anna]

The problem: -w causes the flag DIFF_FROM_CONTENTS to be set, which causes
diff_flush to set the flag HAS_CHANGES based on options->found_changes, which is
set by diff_flush_patch (if there were any changes).  However, --quiet causes
diff_flush to never call diff_flush_patch, so options->found_changes is always 0.

The solution: In this situation, call diff_flush_patch with options->file set to
/dev/null.

Rationale: diff_flush_patch expects to write its output to options->file.
Adding a "silence" flag to diff_flush_patch and everything it calls would be
more invasive.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 diff.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/diff.c b/diff.c
index 68def6c..ff00816 100644
--- a/diff.c
+++ b/diff.c
@@ -3522,6 +3522,26 @@ void diff_flush(struct diff_options *options)
 		separator++;
 	}
 
+	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
+	    DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&
+	    DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {
+		/* run diff_flush_patch for the exit status */
+		/* setting options->file to /dev/null should be safe, becaue we
+		   aren't supposed to produce any output anyways */
+		static FILE *devnull = NULL;
+		if(!devnull) {
+			devnull = fopen("/dev/null", "w");
+			if (!devnull)
+				die_errno("Could not open /dev/null");
+		}
+		options->file = devnull;
+		for (i = 0; i < q->nr; i++) {
+			struct diff_filepair *p = q->queue[i];
+			if (check_pair_status(p))
+				diff_flush_patch(p, options);
+		}
+	}
+
 	if (output_format & DIFF_FORMAT_PATCH) {
 		if (separator) {
 			putc(options->line_termination, options->file);
-- 
1.7.0.rc2.40.g7d8aa

Re: [PATCH 2/2] bugfix: git diff --quiet -w never returns with exit status 1

[Junio C Hamano]

Larry D'Anna <larry@elder-gods.org> writes:

> Rationale: diff_flush_patch expects to write its output to options->file.
> Adding a "silence" flag to diff_flush_patch and everything it calls would be
> more invasive.

I would agree that the logic to redirect the output to nowhere may be the
easiest way out, but because the reason anybody sane would want to give -q
is to say "I don't care what the actual changes are, but I want to know if
there is any real quick" (otherwise the call would be "diff -w >/dev/null"),
shouldn't we at least be exiting the loop early when we see any difference?

> Signed-off-by: Larry D'Anna <larry@elder-gods.org>
> ---
>  diff.c |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 68def6c..ff00816 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3522,6 +3522,26 @@ void diff_flush(struct diff_options *options)
>  		separator++;
>  	}
>  
> +	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
> +	    DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&
> +	    DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {
> +		/* run diff_flush_patch for the exit status */
> +		/* setting options->file to /dev/null should be safe, becaue we
> +		   aren't supposed to produce any output anyways */

Style?

> +		static FILE *devnull = NULL;

Would this cause one file descriptor to leak?  Do we care?

> +		if(!devnull) {

Style?	if (!devnull)

> +			devnull = fopen("/dev/null", "w");
> +			if (!devnull)
> +				die_errno("Could not open /dev/null");
> +		}
> +		options->file = devnull;

Would this cause the original "options->file" leak?  Do we care?

> +		for (i = 0; i < q->nr; i++) {
> +			struct diff_filepair *p = q->queue[i];
> +			if (check_pair_status(p))
> +				diff_flush_patch(p, options);
> +		}
> +	}
> +
>  	if (output_format & DIFF_FORMAT_PATCH) {
>  		if (separator) {
>  			putc(options->line_termination, options->file);
> -- 
> 1.7.0.rc2.40.g7d8aa

Re: [PATCH 2/2] bugfix: git diff --quiet -w never returns with exit status 1

[Larry D'Anna]

* Junio C Hamano (gitster@pobox.com) [100216 00:42]:
> Larry D'Anna <larry@elder-gods.org> writes:
> 
> > Rationale: diff_flush_patch expects to write its output to options->file.
> > Adding a "silence" flag to diff_flush_patch and everything it calls would be
> > more invasive.
> 
> I would agree that the logic to redirect the output to nowhere may be the
> easiest way out, but because the reason anybody sane would want to give -q
> is to say "I don't care what the actual changes are, but I want to know if
> there is any real quick" (otherwise the call would be "diff -w >/dev/null"),
> shouldn't we at least be exiting the loop early when we see any difference?
> 
> > Signed-off-by: Larry D'Anna <larry@elder-gods.org>
> > ---
> >  diff.c |   20 ++++++++++++++++++++
> >  1 files changed, 20 insertions(+), 0 deletions(-)
> >
> > diff --git a/diff.c b/diff.c
> > index 68def6c..ff00816 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -3522,6 +3522,26 @@ void diff_flush(struct diff_options *options)
> >  		separator++;
> >  	}
> >  
> > +	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
> > +	    DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&
> > +	    DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {
> > +		/* run diff_flush_patch for the exit status */
> > +		/* setting options->file to /dev/null should be safe, becaue we
> > +		   aren't supposed to produce any output anyways */
> 
> Style?
> 
> > +		static FILE *devnull = NULL;
> 
> Would this cause one file descriptor to leak?  Do we care?

Originally I thought it would be best to just let one leak, because I didn't
know how much longer it would need to stick around.  I didn't notice it's being
closed anyway a few lines down.


> > +		if(!devnull) {
> 
> Style?	if (!devnull)
> 
> > +			devnull = fopen("/dev/null", "w");
> > +			if (!devnull)
> > +				die_errno("Could not open /dev/null");
> > +		}
> > +		options->file = devnull;
> 
> Would this cause the original "options->file" leak?  Do we care?

oops.

> > +		for (i = 0; i < q->nr; i++) {
> > +			struct diff_filepair *p = q->queue[i];
> > +			if (check_pair_status(p))
> > +				diff_flush_patch(p, options);
> > +		}
> > +	}
> > +
> >  	if (output_format & DIFF_FORMAT_PATCH) {
> >  		if (separator) {
> >  			putc(options->line_termination, options->file);
> > -- 
> > 1.7.0.rc2.40.g7d8aa
> 

[PATCH 2/2] bugfix: git diff --quiet -w never returns with exit status 1

[Larry D'Anna]

The problem: -w causes the flag DIFF_FROM_CONTENTS to be set, which causes
diff_flush to set the flag HAS_CHANGES based on options->found_changes, which is
set by diff_flush_patch (if there were any changes).  However, --quiet causes
diff_flush to never call diff_flush_patch, so options->found_changes is always 0.

The solution: In this situation, call diff_flush_patch with options->file set to
/dev/null.

Rationale: diff_flush_patch expects to write its output to options->file.
Adding a "silence" flag to diff_flush_patch and everything it calls would be
more invasive.

Signed-off-by: Larry D'Anna <larry@elder-gods.org>
---
 diff.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/diff.c b/diff.c
index 68def6c..2984c41 100644
--- a/diff.c
+++ b/diff.c
@@ -3522,6 +3522,29 @@ void diff_flush(struct diff_options *options)
 		separator++;
 	}
 
+	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
+	    DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&
+	    DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {
+		/*
+		 * run diff_flush_patch for the exit status.
+		 * setting options->file to /dev/null should be safe, becaue we
+		 * aren't supposed to produce any output anyways
+		 */
+		if (options->close_file)
+			fclose(options->file);
+		options->file = fopen("/dev/null", "w");
+		if (!options->file)
+			die_errno("Could not open /dev/null");
+		options->close_file = 1;
+		for (i = 0; i < q->nr; i++) {
+			struct diff_filepair *p = q->queue[i];
+			if (check_pair_status(p))
+				diff_flush_patch(p, options);
+			if (options->found_changes)
+				break;
+		}
+	}
+
 	if (output_format & DIFF_FORMAT_PATCH) {
 		if (separator) {
 			putc(options->line_termination, options->file);
-- 
1.7.0.rc2.40.g7d8aa