[PATCH] Allow git-diff exit with codes similar to diff(1)

[PATCH] Allow git-diff exit with codes similar to diff(1)

[Alex Riesen]

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

This introduces a new command-line option: --exit-code. The diff
programs will return 1 for differences, return 0 for equality, and
something else for errors.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Still working on my e-mail problems. Sorry for duplications.

As promised on irc. I'm somewhat confused about diff_tree: it used to
unconditionally return 0, yet every caller of it saves and passes the
value!

 Documentation/diff-options.txt |    5 ++++
 builtin-diff-tree.c            |   12 +++++----
 builtin-diff.c                 |    5 ++-
 diff-lib.c                     |   12 +++++++--
 diff.c                         |    2 +
 diff.h                         |    3 +-
 t/t4017-diff-retval.sh         |   51 ++++++++++++++++++++++++++++++++++++++++
 tree-diff.c                    |   11 +++++++-
 8 files changed, 88 insertions(+), 13 deletions(-)
 create mode 100755 t/t4017-diff-retval.sh

[-- Attachment #2: 0001-Allow-git-diff-exit-with-codes-similar-to-diff-1.patch --]
[-- Type: text/x-patch, Size: 8599 bytes --]

From 2309f82f88274a0ddac0d5b06c5df580f5cb7d81 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Tue, 13 Mar 2007 18:06:08 +0100
Subject: [PATCH] Allow git-diff exit with codes similar to diff(1)

This introduces a new command-line option: --exit-code. The diff
programs will return 1 for differences, return 0 for equality, and
something else for errors.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 Documentation/diff-options.txt |    5 ++++
 builtin-diff-tree.c            |   12 +++++----
 builtin-diff.c                 |    5 ++-
 diff-lib.c                     |   12 +++++++--
 diff.c                         |    2 +
 diff.h                         |    3 +-
 t/t4017-diff-retval.sh         |   51 ++++++++++++++++++++++++++++++++++++++++
 tree-diff.c                    |   11 +++++++-
 8 files changed, 88 insertions(+), 13 deletions(-)
 create mode 100755 t/t4017-diff-retval.sh

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index d8696b7..77a3f78 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -159,5 +159,10 @@
 -w::
 	Shorthand for "--ignore-all-space".
 
+--exit-code::
+	Make the program exit with codes similar to diff(1).
+	That is, it exits with 1 if there were differences and
+	0 means no differences.
+
 For more detailed explanation on these common options, see also
 link:diffcore.html[diffcore documentation].
diff --git a/builtin-diff-tree.c b/builtin-diff-tree.c
index 24cb2d7..d293a5e 100644
--- a/builtin-diff-tree.c
+++ b/builtin-diff-tree.c
@@ -61,6 +61,7 @@ COMMON_DIFF_OPTIONS_HELP;
 
 int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 {
+	int result = 0;
 	int nr_sha1;
 	char line[1000];
 	struct object *tree1, *tree2;
@@ -110,7 +111,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 			tree2 = tree1;
 			tree1 = tmp;
 		}
-		diff_tree_sha1(tree1->sha1,
+		result = diff_tree_sha1(tree1->sha1,
 			       tree2->sha1,
 			       "", &opt->diffopt);
 		log_tree_diff_flush(opt);
@@ -118,7 +119,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 	}
 
 	if (!read_stdin)
-		return 0;
+		return opt->diffopt.diff_exit_code ? result: 0;
 
 	if (opt->diffopt.detect_rename)
 		opt->diffopt.setup |= (DIFF_SETUP_USE_SIZE_CACHE |
@@ -130,8 +131,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
 			fputs(line, stdout);
 			fflush(stdout);
 		}
-		else
-			diff_tree_stdin(line);
+		else if (diff_tree_stdin(line)) {
+			result = 1;
+		}
 	}
-	return 0;
+	return opt->diffopt.diff_exit_code ? result: 0;
 }
diff --git a/builtin-diff.c b/builtin-diff.c
index 4efbb82..5e6265f 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -130,6 +130,7 @@ static int builtin_diff_tree(struct rev_info *revs,
 {
 	const unsigned char *(sha1[2]);
 	int swap = 0;
+	int result = 0;
 
 	if (argc > 1)
 		usage(builtin_diff_usage);
@@ -141,9 +142,9 @@ static int builtin_diff_tree(struct rev_info *revs,
 		swap = 1;
 	sha1[swap] = ent[0].item->sha1;
 	sha1[1-swap] = ent[1].item->sha1;
-	diff_tree_sha1(sha1[0], sha1[1], "", &revs->diffopt);
+	result = diff_tree_sha1(sha1[0], sha1[1], "", &revs->diffopt);
 	log_tree_diff_flush(revs);
-	return 0;
+	return result;
 }
 
 static int builtin_diff_combined(struct rev_info *revs,
diff --git a/diff-lib.c b/diff-lib.c
index 6abb981..f943b6f 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -170,8 +170,10 @@ static int handle_diff_files_args(struct rev_info *revs,
 		else if (!strcmp(argv[1], "--theirs"))
 			revs->max_count = 3;
 		else if (!strcmp(argv[1], "-n") ||
-				!strcmp(argv[1], "--no-index"))
+				!strcmp(argv[1], "--no-index")) {
 			revs->max_count = -2;
+			revs->diffopt.diff_exit_code = 1;
+		}
 		else if (!strcmp(argv[1], "-q"))
 			*silent = 1;
 		else
@@ -237,6 +239,7 @@ int setup_diff_no_index(struct rev_info *revs,
 			break;
 		} else if (i < argc - 3 && !strcmp(argv[i], "--no-index")) {
 			i = argc - 3;
+			revs->diffopt.diff_exit_code = 1;
 			break;
 		}
 	if (argc != i + 2 || (!is_outside_repo(argv[i + 1], nongit, prefix) &&
@@ -309,6 +312,7 @@ int run_diff_files_cmd(struct rev_info *revs, int argc, const char **argv)
 
 int run_diff_files(struct rev_info *revs, int silent_on_removed)
 {
+	int result = 0;
 	int entries, i;
 	int diff_unmerged_stage = revs->max_count;
 
@@ -433,8 +437,9 @@ int run_diff_files(struct rev_info *revs, int silent_on_removed)
 
 	}
 	diffcore_std(&revs->diffopt);
+	result = revs->diffopt.diff_exit_code && diff_queued_diff.nr ? 1: 0;
 	diff_flush(&revs->diffopt);
-	return 0;
+	return result;
 }
 
 /*
@@ -664,9 +669,10 @@ int run_diff_index(struct rev_info *revs, int cached)
 		return error("bad tree object %s", tree_name);
 	if (read_tree(tree, 1, revs->prune_data))
 		return error("unable to read tree object %s", tree_name);
-	ret = diff_cache(revs, active_cache, active_nr, revs->prune_data,
+	diff_cache(revs, active_cache, active_nr, revs->prune_data,
 			 cached, match_missing);
 	diffcore_std(&revs->diffopt);
+	ret = revs->diffopt.diff_exit_code && diff_queued_diff.nr ? 1: 0;
 	diff_flush(&revs->diffopt);
 	return ret;
 }
diff --git a/diff.c b/diff.c
index 954ca83..e2f91ed 100644
--- a/diff.c
+++ b/diff.c
@@ -2134,6 +2134,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 		options->color_diff = options->color_diff_words = 1;
 	else if (!strcmp(arg, "--no-renames"))
 		options->detect_rename = 0;
+	else if (!strcmp(arg, "--exit-code"))
+		options->diff_exit_code = 1;
 	else
 		return 0;
 	return 1;
diff --git a/diff.h b/diff.h
index 4b435e8..cea61b5 100644
--- a/diff.h
+++ b/diff.h
@@ -56,7 +56,8 @@ struct diff_options {
 		 silent_on_remove:1,
 		 find_copies_harder:1,
 		 color_diff:1,
-		 color_diff_words:1;
+		 color_diff_words:1,
+		 diff_exit_code:1;
 	int context;
 	int break_opt;
 	int detect_rename;
diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh
new file mode 100755
index 0000000..81a9393
--- /dev/null
+++ b/t/t4017-diff-retval.sh
@@ -0,0 +1,51 @@
+#!/bin/sh
+
+test_description='Return value of diffs'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo 1 >a &&
+	git add . &&
+	git commit -m first &&
+	echo 2 >b &&
+	git add . &&
+	git commit -a -m second
+'
+
+test_expect_failure 'git diff-tree HEAD^ HEAD' '
+	git diff-tree --exit-code HEAD^ HEAD
+'
+test_expect_failure 'echo HEAD | git diff-tree --stdin' '
+	echo $(git rev-parse HEAD) | git diff-tree --exit-code --stdin
+'
+test_expect_success 'git diff-tree HEAD HEAD' '
+	git diff-tree --exit-code HEAD HEAD
+'
+
+test_expect_success 'git diff-files' '
+	git diff-files --exit-code
+'
+
+test_expect_success 'git diff-index --cached HEAD' '
+	git diff-index --exit-code --cached HEAD
+'
+test_expect_failure 'git diff-index --cached HEAD^' '
+	git diff-index --exit-code --cached HEAD^
+'
+test_expect_failure 'git diff-index --cached HEAD^' '
+	echo 3 >c &&
+	git add . &&
+	git diff-index --exit-code --cached HEAD^
+'
+git commit -m 'third' >/dev/null
+test_expect_failure 'git diff-files' '
+	echo 3 >>c &&
+	git diff-files --exit-code
+'
+test_expect_failure 'git diff-index --cached HEAD' '
+	git update-index c &&
+	git diff-index --exit-code --cached HEAD
+'
+
+test_done
diff --git a/tree-diff.c b/tree-diff.c
index c827582..47c516f 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -160,6 +160,8 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree
 
 int diff_tree(struct tree_desc *t1, struct tree_desc *t2, const char *base, struct diff_options *opt)
 {
+	int result = 0;
+
 	while (t1->size | t2->size) {
 		if (opt->nr_paths && t1->size && !interesting(t1, base, opt)) {
 			update_tree_entry(t1);
@@ -170,29 +172,34 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2, const char *base, stru
 			continue;
 		}
 		if (!t1->size) {
+			result = 1;
 			show_entry(opt, "+", t2, base);
 			update_tree_entry(t2);
 			continue;
 		}
 		if (!t2->size) {
+			result = 1;
 			show_entry(opt, "-", t1, base);
 			update_tree_entry(t1);
 			continue;
 		}
 		switch (compare_tree_entry(t1, t2, base, opt)) {
 		case -1:
+			result = 1;
 			update_tree_entry(t1);
 			continue;
 		case 0:
 			update_tree_entry(t1);
-			/* Fallthrough */
+			update_tree_entry(t2);
+			continue;
 		case 1:
+			result = 1;
 			update_tree_entry(t2);
 			continue;
 		}
 		die("git-diff-tree: internal error");
 	}
-	return 0;
+	return opt->diff_exit_code ? result: 0;
 }
 
 int diff_tree_sha1(const unsigned char *old, const unsigned char *new, const char *base, struct diff_options *opt)
-- 
1.5.0.3.545.g5d3ba

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Johannes Schindelin]

Hi,

On Wed, 14 Mar 2007, Alex Riesen wrote:

> This introduces a new command-line option: --exit-code. The diff 
> programs will return 1 for differences, return 0 for equality, and 
> something else for errors.

I am not convinced that this exit code (even if expected by experience) 
makes sense. A diff should _only_ fail if anything goes _wrong_, not if 
the files are _different. That is what cmp is for, no?

Ciao,
Dscho

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Linus Torvalds]

On Wed, 14 Mar 2007, Johannes Schindelin wrote:
> 
> I am not convinced that this exit code (even if expected by experience) 
> makes sense. A diff should _only_ fail if anything goes _wrong_, not if 
> the files are _different. That is what cmp is for, no?

Well, "cmp" doesn't do a sane job for recursive diffs.

And Alex is certainly correct in stating that people historically expect 
and use the exit status of a regular "diff". I don't think there is any 
real down-side to making "git diff" have the same semantics, especially 
since the current exit-status isn't exactly useful.

			Linus

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Wed, 14 Mar 2007, Alex Riesen wrote:
>
>> This introduces a new command-line option: --exit-code. The diff 
>> programs will return 1 for differences, return 0 for equality, and 
>> something else for errors.
>
> I am not convinced that this exit code (even if expected by experience) 
> makes sense. A diff should _only_ fail if anything goes _wrong_, not if 
> the files are _different. That is what cmp is for, no?

Not really, if you are used to use diff as a replacement for
cmp.

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Linus Torvalds]

On Wed, 14 Mar 2007, Alex Riesen wrote:
>
> This introduces a new command-line option: --exit-code. The diff
> programs will return 1 for differences, return 0 for equality, and
> something else for errors.

I don't think you should need a new command-line option.

Is there any reason to not just do this unconditionally?

> As promised on irc. I'm somewhat confused about diff_tree: it used to
> unconditionally return 0, yet every caller of it saves and passes the
> value!

I think we just never implemented the error codes, but they were always 
meant to be there.

I also thought I did some early-out logic (for the revision list pruning 
thing), where the "show_entry()" routine could return a negative value to 
say "Ok, no need to do anything more" but apparently I never added that..

[ I have this very distinct memory of working on it, but either I was 
  dreaming or I never got it working.. ]

			Linus

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Johannes Schindelin]

Hi,

On Tue, 13 Mar 2007, Linus Torvalds wrote:

> On Wed, 14 Mar 2007, Alex Riesen wrote:
> >
> > This introduces a new command-line option: --exit-code. The diff 
> > programs will return 1 for differences, return 0 for equality, and 
> > something else for errors.
> 
> I don't think you should need a new command-line option.
> 
> Is there any reason to not just do this unconditionally?

So, big master to hom everybody bows, how to return the correct value when 
executing a pager? Because this _has_ to be done if we go that way.

Ciao,
Dscho

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Linus Torvalds]

On Wed, 14 Mar 2007, Johannes Schindelin wrote:
> 
> So, big master to hom everybody bows, how to return the correct value when 
> executing a pager? Because this _has_ to be done if we go that way.

Why? If you execute the pager, nobody cares about the error value anyway. 

I don't see why you would mix in a pager here. If you do

	diff -u file1 file2 | less -S

the return value of the pipe will not only generally be totally 
uninteresting and never used, but it will be the return value of "less" 
anyway. Which is what we'd get quite naturally.

The return value of "git diff" really only is sensibly and meaningful when 
you use it in a script *without* a pager (whether built-in or not).

		Linus

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Johannes Schindelin]

Hi,

On Tue, 13 Mar 2007, Linus Torvalds wrote:

> On Wed, 14 Mar 2007, Johannes Schindelin wrote:
> > 
> > So, big master to hom everybody bows, how to return the correct value 
> > when executing a pager? Because this _has_ to be done if we go that 
> > way.
> 
> Why? If you execute the pager, nobody cares about the error value 
> anyway.
> 
> I don't see why you would mix in a pager here. If you do
> 
> 	diff -u file1 file2 | less -S
> 
> the return value of the pipe will not only generally be totally 
> uninteresting and never used, but it will be the return value of "less" 
> anyway. Which is what we'd get quite naturally.

The thing is, most people do not realize that

	git diff file1 file2

_will_ execute a pager. As foreground process. And the return value is 
that of the pager.

Ciao,
Dscho

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Alex Riesen]

On 3/14/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > > So, big master to hom everybody bows, how to return the correct value
> > > when executing a pager? Because this _has_ to be done if we go that
> > > way.
> >
> > Why? If you execute the pager, nobody cares about the error value
> > anyway.
> >
> > I don't see why you would mix in a pager here. If you do
> >
> >       diff -u file1 file2 | less -S
> >
> > the return value of the pipe will not only generally be totally
> > uninteresting and never used, but it will be the return value of "less"
> > anyway. Which is what we'd get quite naturally.
>
> The thing is, most people do not realize that
>
>         git diff file1 file2
>
> _will_ execute a pager. As foreground process. And the return value is
> that of the pager.

In this example this is obviously (sometimes it is obscurely) interactive.
The return code is seldom expected.

More sneaky case could be this:

  git diff file1 file2 > tmp && do_something
  rm -f tmp

But we have isatty in setup_pager, so this works properly.

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Johannes Schindelin]

Hi,

On Wed, 14 Mar 2007, Alex Riesen wrote:

> On 3/14/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > > > So, big master to hom everybody bows, how to return the correct value
> > > > when executing a pager? Because this _has_ to be done if we go that
> > > > way.
> > >
> > > Why? If you execute the pager, nobody cares about the error value
> > > anyway.
> > >
> > > I don't see why you would mix in a pager here. If you do
> > >
> > >       diff -u file1 file2 | less -S
> > >
> > > the return value of the pipe will not only generally be totally
> > > uninteresting and never used, but it will be the return value of "less"
> > > anyway. Which is what we'd get quite naturally.
> > 
> > The thing is, most people do not realize that
> > 
> >         git diff file1 file2
> > 
> > _will_ execute a pager. As foreground process. And the return value is
> > that of the pager.
> 
> In this example this is obviously (sometimes it is obscurely) interactive.
> The return code is seldom expected.
> 
> More sneaky case could be this:
> 
>  git diff file1 file2 > tmp && do_something
>  rm -f tmp
> 
> But we have isatty in setup_pager, so this works properly.

The problem is test cases. I think that we pipe the output of the test 
case _anyway_, so the isatty() call is helping us there.

If we did not (this applies to test cases _outside_ of Git, too), then a 
simple

	git diff bla || exit

would not work as expected. Even worse, as long as it is piped somewhere, 
even cat, it works. But once you no longer pipe it (to get the nice pager, 
for example), it stops working.

Ciao,
Dscho

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Alex Riesen]

On 3/14/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > But we have isatty in setup_pager, so this works properly.
>
> The problem is test cases. I think that we pipe the output of the test
> case _anyway_, so the isatty() call is helping us there.
>
> If we did not (this applies to test cases _outside_ of Git, too), then a
> simple
>
>         git diff bla || exit
>
> would not work as expected. Even worse, as long as it is piped somewhere,
> even cat, it works. But once you no longer pipe it (to get the nice pager,
> for example), it stops working.

We have "PAGER=cat" in test-lib.sh which just disables pager,
so gits tests do not have the problem (maybe it was disabled just
because of this?). And if someone has own tests _with_ pager active
the one better be aware of what the one doing.

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Johannes Schindelin]

Hi,

On Wed, 14 Mar 2007, Alex Riesen wrote:

> We have "PAGER=cat" in test-lib.sh which just disables pager, so gits 
> tests do not have the problem (maybe it was disabled just because of 
> this?).

I don't know if it was disabled just because of this, but I guess that it 
was to prevent interactive tests from stopping with the output of _every_ 
_single_ test until the user quits the pager.

> And if someone has own tests _with_ pager active the one better be aware 
> of what the one doing.

No. This is wrong. The whole purpose of the patch you sent is to meet 
expectations. What you just wrote flies in the face of that.

Ciao,
Dscho

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Simon 'corecode' Schubert]

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

Alex Riesen wrote:
> We have "PAGER=cat" in test-lib.sh which just disables pager,
> so gits tests do not have the problem (maybe it was disabled just
> because of this?). And if someone has own tests _with_ pager active
> the one better be aware of what the one doing.

Does PAGER=cat really disable the pager?  seems to me that it simply passes the data through cat.  That's not disabling it, but it is instead rendering the pager transparent.  Not for the exit code though.

cheers
  simon

-- 
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low €€€ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \


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

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Alex Riesen]

On 3/15/07, Simon 'corecode' Schubert <corecode@fs.ei.tum.de> wrote:
> Alex Riesen wrote:
> > We have "PAGER=cat" in test-lib.sh which just disables pager,
> > so gits tests do not have the problem (maybe it was disabled just
> > because of this?). And if someone has own tests _with_ pager active
> > the one better be aware of what the one doing.
>
> Does PAGER=cat really disable the pager?  seems to me that it simply
> passes the data through cat.

Yes:
pager.c:

	else if (!*pager || !strcmp(pager, "cat"))
		return;

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Wed, 14 Mar 2007, Alex Riesen wrote:
>>
>> This introduces a new command-line option: --exit-code. The diff
>> programs will return 1 for differences, return 0 for equality, and
>> something else for errors.
>
> I don't think you should need a new command-line option.
>
> Is there any reason to not just do this unconditionally?

Exiting with 0 for no-change, 1 for has-change and other value
for error is something that falls into the

    "I wish if we did it from day one, but now many people's
    scripts depend on the behaviour, and heck we ourselves say
    that the right way to see if there is difference is to check
    if the output is an empty string (look at a few scripts of
    our own), so it would be a huge backward compatibility
    hassle"

category.

e.g.

git-am.sh:	files=$(git-diff-index --cached --name-only HEAD) || exit
git-am.sh:      changed="$(git-diff-index --cached --name-only HEAD)"
t/t0020-crlf.sh:	differs=`git diff-index --cached HEAD` &&
t/t0020-crlf.sh:	differs=`git diff-index --cached HEAD` &&
t/t0020-crlf.sh:	differs=`git diff-index --cached HEAD` &&
t/t0020-crlf.sh:	differs=`git diff-index --cached HEAD` &&

And I expect people's scripts are modelled after how git's
scripts do things --- if they were half competent, they'd know
that would be the way to make sure their scripts would be
compatible with future plumbing changes.

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Alex Riesen]

On 3/14/07, Junio C Hamano <junkio@cox.net> wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
> > On Wed, 14 Mar 2007, Alex Riesen wrote:
> >>
> >> This introduces a new command-line option: --exit-code. The diff
> >> programs will return 1 for differences, return 0 for equality, and
> >> something else for errors.
> >
> > I don't think you should need a new command-line option.
> >
> > Is there any reason to not just do this unconditionally?
>
> Exiting with 0 for no-change, 1 for has-change and other value
> for error is something that falls into the
>
>     "I wish if we did it from day one, but now many people's
>     scripts depend on the behaviour, and heck we ourselves say
>     that the right way to see if there is difference is to check
>     if the output is an empty string (look at a few scripts of
>     our own), so it would be a huge backward compatibility
>     hassle"
>
> category.
>
> e.g.
>
> git-am.sh:      files=$(git-diff-index --cached --name-only HEAD) || exit
> git-am.sh:      changed="$(git-diff-index --cached --name-only HEAD)"

Isn't this crazy? Get the information and never really use it?
The second one does not check for errors, BTW. Will
break if someone sends enough files in one commit to go past
local limit on shell variable size (or the local limit on process size,
which is much worse situation: everything gets swapped out).

> And I expect people's scripts are modelled after how git's
> scripts do things --- if they were half competent, they'd know
> that would be the way to make sure their scripts would be
> compatible with future plumbing changes.

That is expert level. The half competents are used to diff(1).

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> On 3/14/07, Junio C Hamano <junkio@cox.net> wrote:
> ...
>> Exiting with 0 for no-change, 1 for has-change and other value
>> for error is something that falls into the
>>
>>     "I wish if we did it from day one, but now many people's
>>     scripts depend on the behaviour, and heck we ourselves say
>>     that the right way to see if there is difference is to check
>>     if the output is an empty string (look at a few scripts of
>>     our own), so it would be a huge backward compatibility
>>     hassle"
>>
>> category.
>>
>> e.g.
>>
>> git-am.sh:      files=$(git-diff-index --cached --name-only HEAD) || exit
>> git-am.sh:      changed="$(git-diff-index --cached --name-only HEAD)"
>
> Isn't this crazy? Get the information and never really use it?

The information is used.

The first one says "see if we have already difference stashed in
the index for our own committing --- by the way, if there is an
error, do not do any further harm but error out".  If we do not
get an error, that line is followed by this:

	files=$(git-diff-index --cached --name-only HEAD) || exit
        if [ "$files" ]
        then
        	echo "Dirty index: ..." >&2
                exit 1
	fi

I've been telling you since the #git session that I know that is
*different* from how "diff" works, and I think everybody agrees
if we were doing git from scratch today we would have done exit
status with 0/1/other to signal no-change, have-diff and error.

But the established way for scripts that use plumbing is

 - to check error with $? (or ... || )
 - to check modified-or-not with output

and people who have been learning from the scripts (we used to
have lot more scripts) would have picked up that pattern.
That's why I already told you that --exit-status is the right
thing to do if we were doing it from scratch, but is a wrong
thing to do at this point.  Maybe in a release as big as 1.5.0
that we pre-announce a lot of interface changes.

In short, Linus is right in that the current exit code is not
useful to see what the end users are interested in (and they are
not in the business of debugging git, and diff would error out
only when the repository has problems, perhaps a corrupt object
or something like that).  But being not useful and being
currently not relied upon are two different things.

And I am being conservative, especially after a big release.

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> I've been telling you since the #git session that I know that is
> *different* from how "diff" works, and I think everybody agrees
> if we were doing git from scratch today we would have done exit
> status with 0/1/other to signal no-change, have-diff and error.
>
> But the established way for scripts that use plumbing is
>
>  - to check error with $? (or ... || )
>  - to check modified-or-not with output
>
> and people who have been learning from the scripts (we used to
> have lot more scripts) would have picked up that pattern.
> That's why I already told you that --exit-status is the right
> thing to do if we were doing it from scratch, but is a wrong
> thing to do at this point.

Correction.

s/--exit-status is/doing --exit-status without such an explicit option is/.

> Maybe in a release as big as 1.5.0
> that we pre-announce a lot of interface changes.
>
> In short, Linus is right in that the current exit code is not
> useful to see what the end users are interested in (and they are
> not in the business of debugging git, and diff would error out
> only when the repository has problems, perhaps a corrupt object
> or something like that).  But being not useful and being
> currently not relied upon are two different things.
>
> And I am being conservative, especially after a big release.

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Alex Riesen]

On 3/14/07, Junio C Hamano <junkio@cox.net> wrote:
> and people who have been learning from the scripts (we used to
> have lot more scripts) would have picked up that pattern.
> That's why I already told you that --exit-status is the right
> thing to do if we were doing it from scratch, but is a wrong
> thing to do at this point.  Maybe in a release as big as 1.5.0
> that we pre-announce a lot of interface changes.

This is something I don't understand: why is a new option
--exit-code wrong? Noone uses it yet and the default exit code
is at it was before (modulo the bug with diff-tree). I am *not*
proposing to change default exit codes (well, I suggest that).
So what's the problem?

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> On 3/14/07, Junio C Hamano <junkio@cox.net> wrote:
>> and people who have been learning from the scripts (we used to
>> have lot more scripts) would have picked up that pattern.
>> That's why I already told you that --exit-status is the right
>> thing to do if we were doing it from scratch, but is a wrong
>> thing to do at this point.  Maybe in a release as big as 1.5.0
>> that we pre-announce a lot of interface changes.
>
> This is something I don't understand: why is a new option
> --exit-code wrong?

Sorry, what I said was very confused (I sent out a
correction/clarification didn't I?).

In the ideal world we would sensibly make our diff to report
with its exit status.  Unfortunately we cannot for the above
backward compatibility worries, so a new option is an ugly
but necessary workaround.

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Alex Riesen]

On 3/14/07, Junio C Hamano <junkio@cox.net> wrote:
> In the ideal world we would sensibly make our diff to report
> with its exit status.  Unfortunately we cannot for the above
> backward compatibility worries, so a new option is an ugly
> but necessary workaround.

right

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> diff --git a/builtin-diff.c b/builtin-diff.c
> index 4efbb82..5e6265f 100644
> --- a/builtin-diff.c
> +++ b/builtin-diff.c
> @@ -130,6 +130,7 @@ static int builtin_diff_tree(struct rev_info *revs,
>  {
>  	const unsigned char *(sha1[2]);
>  	int swap = 0;
> +	int result = 0;
>  
>  	if (argc > 1)
>  		usage(builtin_diff_usage);
> @@ -141,9 +142,9 @@ static int builtin_diff_tree(struct rev_info *revs,
>  		swap = 1;
>  	sha1[swap] = ent[0].item->sha1;
>  	sha1[1-swap] = ent[1].item->sha1;
> -	diff_tree_sha1(sha1[0], sha1[1], "", &revs->diffopt);
> +	result = diff_tree_sha1(sha1[0], sha1[1], "", &revs->diffopt);
>  	log_tree_diff_flush(revs);
> -	return 0;
> +	return result;
>  }
>  
>  static int builtin_diff_combined(struct rev_info *revs,

The change to diff-tree side is completely borked.  (1) You did
not notice compare_tree_entry() in tree-diff.c returns 0 only to
signal that it has dealt with an entry from both sides (so the
caller can do update_tree_entry() on both), and the return value
does not mean they are the same.  (2) You are checking if there
are differences at wrong level, before letting diffcore_std() to
process the queue.  Because of the bug (1) I cannot test that
but after you fix (1) you would notice that it would not work if
you say "-Spickaxe"; your changes to diff-files and diff-index
are correct on this regard.

A slight tangent, but what Linus recalled he thought he did but
he didn't is related to the parts you touched in diff-tree
above.  Because of the interaction with diffcore, these changes
should not be used for the purpose of -exit-code, but catching
the tree level change in the above places and leaving early
would be the right thing to do for comparing the whole tree for
the purpose of simplifying the parents.  Tomorrow will be my git
day so I might whip up a patch to speed that up.

> diff --git a/diff-lib.c b/diff-lib.c
> index 6abb981..f943b6f 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -433,8 +437,9 @@ int run_diff_files(struct rev_info *revs, int silent_on_removed)
>  
>  	}
>  	diffcore_std(&revs->diffopt);
> +	result = revs->diffopt.diff_exit_code && diff_queued_diff.nr ? 1: 0;
>  	diff_flush(&revs->diffopt);
> -	return 0;
> +	return result;
>  }
>  
>  /*
> @@ -664,9 +669,10 @@ int run_diff_index(struct rev_info *revs, int cached)
>  		return error("bad tree object %s", tree_name);
>  	if (read_tree(tree, 1, revs->prune_data))
>  		return error("unable to read tree object %s", tree_name);
> -	ret = diff_cache(revs, active_cache, active_nr, revs->prune_data,
> +	diff_cache(revs, active_cache, active_nr, revs->prune_data,
>  			 cached, match_missing);
>  	diffcore_std(&revs->diffopt);
> +	ret = revs->diffopt.diff_exit_code && diff_queued_diff.nr ? 1: 0;
>  	diff_flush(&revs->diffopt);
>  	return ret;
>  }

This side looks correct, as you are counting queued_diff.nr after
letting diffcore_std() to filter the results.

> +test_expect_failure 'git diff-tree HEAD^ HEAD' '
> +	git diff-tree --exit-code HEAD^ HEAD
> +'
> ...
> +test_expect_failure 'git diff-index --cached HEAD' '
> +	git update-index c &&
> +	git diff-index --exit-code --cached HEAD
> +'

In general, expect_failure should not be used for complex cases
like this.  The first one I quoted is fine, but the latter one
is not.  update-index may fail (perhaps somebody screwed up
while updating read-cache.c or sha1_file.c) and the whole test
would say "happy that the command chain as a whole failed".

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Alex Riesen]

On 3/14/07, Junio C Hamano <junkio@cox.net> wrote:
> "Alex Riesen" <raa.lkml@gmail.com> writes:
>
> > diff --git a/builtin-diff.c b/builtin-diff.c
> > index 4efbb82..5e6265f 100644
> > --- a/builtin-diff.c
> > +++ b/builtin-diff.c
> > @@ -130,6 +130,7 @@ static int builtin_diff_tree(struct rev_info *revs,
> >  {
> >       const unsigned char *(sha1[2]);
> >       int swap = 0;
> > +     int result = 0;
> >
> >       if (argc > 1)
> >               usage(builtin_diff_usage);
> > @@ -141,9 +142,9 @@ static int builtin_diff_tree(struct rev_info *revs,
> >               swap = 1;
> >       sha1[swap] = ent[0].item->sha1;
> >       sha1[1-swap] = ent[1].item->sha1;
> > -     diff_tree_sha1(sha1[0], sha1[1], "", &revs->diffopt);
> > +     result = diff_tree_sha1(sha1[0], sha1[1], "", &revs->diffopt);
> >       log_tree_diff_flush(revs);
> > -     return 0;
> > +     return result;
> >  }
> >
> >  static int builtin_diff_combined(struct rev_info *revs,
>
> The change to diff-tree side is completely borked.  (1) You did
> not notice compare_tree_entry() in tree-diff.c returns 0 only to
> signal that it has dealt with an entry from both sides (so the
> caller can do update_tree_entry() on both), and the return value
> does not mean they are the same.  (2) You are checking if there
> are differences at wrong level, before letting diffcore_std() to
> process the queue.  Because of the bug (1) I cannot test that
> but after you fix (1) you would notice that it would not work if
> you say "-Spickaxe"; your changes to diff-files and diff-index
> are correct on this regard.

Challenging... Now, if someone just told me where to look for
differences in diff-tree case...

> A slight tangent, but what Linus recalled he thought he did but
> he didn't is related to the parts you touched in diff-tree
> above.  Because of the interaction with diffcore, these changes
> should not be used for the purpose of -exit-code, but catching
> the tree level change in the above places and leaving early
> would be the right thing to do for comparing the whole tree for
> the purpose of simplifying the parents.  Tomorrow will be my git
> day so I might whip up a patch to speed that up.

Can it eventually be wired to "-s" (DIFF_FORMAT_NO_OUTPUT)?

> >       diffcore_std(&revs->diffopt);
> > +     ret = revs->diffopt.diff_exit_code && diff_queued_diff.nr ? 1: 0;
> >       diff_flush(&revs->diffopt);
> >       return ret;
> >  }
>
> This side looks correct, as you are counting queued_diff.nr after
> letting diffcore_std() to filter the results.
>

And it will continue to work if the diffing is left early because of
no output needed. Err, will it?

> > +test_expect_failure 'git diff-index --cached HEAD' '
> > +     git update-index c &&
> > +     git diff-index --exit-code --cached HEAD
> > +'
>
> In general, expect_failure should not be used for complex cases
> like this.  The first one I quoted is fine, but the latter one
> is not.  update-index may fail (perhaps somebody screwed up
> while updating read-cache.c or sha1_file.c) and the whole test
> would say "happy that the command chain as a whole failed".

Right. Fixed.

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> Challenging... Now, if someone just told me where to look for
> differences in diff-tree case...

As the resident git-diff expert I might hack on this myself as
the --quiet options are useful, and --exit-code comes almost
free when you properly do --quiet that implies --quick.

>> A slight tangent, but what Linus recalled he thought he did but
>> he didn't is related to the parts you touched in diff-tree
>> above.  Because of the interaction with diffcore, these changes
>> should not be used for the purpose of -exit-code, but catching
>> the tree level change in the above places and leaving early
>> would be the right thing to do for comparing the whole tree for
>> the purpose of simplifying the parents.  Tomorrow will be my git
>> day so I might whip up a patch to speed that up.
>
> Can it eventually be wired to "-s" (DIFF_FORMAT_NO_OUTPUT)?

I do not think so.  When we run diff internally to pick the set
of paths (i.e. run diffcore and check contents of diff_queue,
just like you did for diff-index/diff-files), we internally use
NO_OUTPUT.  See merge-recursive.c for an example.

>> >       diffcore_std(&revs->diffopt);
>> > +     ret = revs->diffopt.diff_exit_code && diff_queued_diff.nr ? 1: 0;
>> >       diff_flush(&revs->diffopt);
>> >       return ret;
>> >  }
>>
>> This side looks correct, as you are counting queued_diff.nr after
>> letting diffcore_std() to filter the results.
>
> And it will continue to work if the diffing is left early because of
> no output needed. Err, will it?

To implement --quick correctly, you need to know when it is safe
to leave early.  Presence of -S (pickaxe) would most likely mean
you shouldn't leave early.

[PATCH] Allow git-diff exit with codes similar to diff(1)

[Alex Riesen]

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

This introduces a new command-line option: --exit-code. The diff
programs will return 1 for differences, return 0 for equality, and
something else for errors.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
Updated the patch:

- added more tests: path limiter and -S
- moved exit code into diff_options and used diffcore_std* for its evaluation

On 3/14/07, Junio C Hamano <junkio@cox.net> wrote:
> > Challenging... Now, if someone just told me where to look for
> > differences in diff-tree case...
>
> As the resident git-diff expert I might hack on this myself as
> the --quiet options are useful, and --exit-code comes almost
> free when you properly do --quiet that implies --quick.

Thanks! That would _most_ welcome :)

> > Can it eventually be wired to "-s" (DIFF_FORMAT_NO_OUTPUT)?
>
> I do not think so.  When we run diff internally to pick the set
> of paths (i.e. run diffcore and check contents of diff_queue,
> just like you did for diff-index/diff-files), we internally use
> NO_OUTPUT.  See merge-recursive.c for an example.

Right, is not that simple as it looks...

> >> >       diffcore_std(&revs->diffopt);
> >> > +     ret = revs->diffopt.diff_exit_code && diff_queued_diff.nr ? 1: 0;
> >> >       diff_flush(&revs->diffopt);
> >> >       return ret;
> >> >  }
> >>
> >> This side looks correct, as you are counting queued_diff.nr after
> >> letting diffcore_std() to filter the results.
> >
> > And it will continue to work if the diffing is left early because of
> > no output needed. Err, will it?
>
> To implement --quick correctly, you need to know when it is safe
> to leave early.  Presence of -S (pickaxe) would most likely mean
> you shouldn't leave early.

Thanks, that got me thinking. Moved all exit code evaluation
into diffcore_std, added a field for the code to diff_options,
and use it if called with --exit-code.

 Documentation/diff-options.txt |    5 +++
 builtin-diff-files.c           |    4 ++-
 builtin-diff-index.c           |    4 ++-
 builtin-diff-tree.c            |    4 +-
 builtin-diff.c                 |   19 +++++++-----
 diff-lib.c                     |    5 ++-
 diff.c                         |    6 ++++
 diff.h                         |    5 ++-
 t/t4017-diff-retval.sh         |   64 ++++++++++++++++++++++++++++++++++++++++
 9 files changed, 102 insertions(+), 14 deletions(-)
 create mode 100755 t/t4017-diff-retval.sh

[-- Attachment #2: 0001-Allow-git-diff-exit-with-codes-similar-to-diff-1.patch --]
[-- Type: application/xxxxx, Size: 9788 bytes --]

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

>> To implement --quick correctly, you need to know when it is safe
>> to leave early.  Presence of -S (pickaxe) would most likely mean
>> you shouldn't leave early.
>
> Thanks, that got me thinking. Moved all exit code evaluation
> into diffcore_std, added a field for the code to diff_options,
> and use it if called with --exit-code.

Certainly --quick would be "interesting" and useful to add on
top of your patch.

> diff --git a/builtin-diff-files.c b/builtin-diff-files.c
> index aec8338..2525ae6 100644
> --- a/builtin-diff-files.c
> +++ b/builtin-diff-files.c
> @@ -17,6 +17,7 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
>  {
>  	struct rev_info rev;
>  	int nongit = 0;
> +	int result;
>  
>  	prefix = setup_git_directory_gently(&nongit);
>  	init_revisions(&rev, prefix);
> @@ -29,5 +30,6 @@ int cmd_diff_files(int argc, const char **argv, const char *prefix)
>  		argc = setup_revisions(argc, argv, &rev, NULL);
>  	if (!rev.diffopt.output_format)
>  		rev.diffopt.output_format = DIFF_FORMAT_RAW;
> -	return run_diff_files_cmd(&rev, argc, argv);
> +	result = run_diff_files_cmd(&rev, argc, argv);
> +	return rev.diffopt.diff_exit_code ? rev.diffopt.exit_code: result;
>  }

Yuck.  Let's call the former "exit_with_status" (meaning, the
caller instructed us to do that) and the latter "has_changes".

> +test_expect_failure 'git diff-index --cached HEAD^' '
> +	echo text >>b &&
> +	echo 3 >c &&
> +	git add . &&
> +	git diff-index --exit-code --cached HEAD^
> +'

Please:

        test_expect_success '...
                echo ... &&
                git add . &&
                ! git diff-index ...
        '

I recall somebody on the list had a buggy shell that cannot
handle "a && ! b" and tweaked a few tests to work around it.
In that case...

	echo ... &&
        git add . &&
	{
        	git diff-index ...; test $? != 0
	}

> +test_expect_failure 'git diff-files' '
> +	echo 3 >>c &&
> +	git diff-files --exit-code
> +'

Likewise

> +git update-index c || error "update-index failed"

Please do not have command outside test_expect without good
reason.

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Alex Riesen]

On 3/14/07, Junio C Hamano <junkio@cox.net> wrote:
> > -     return run_diff_files_cmd(&rev, argc, argv);
> > +     result = run_diff_files_cmd(&rev, argc, argv);
> > +     return rev.diffopt.diff_exit_code ? rev.diffopt.exit_code: result;
> >  }
>
> Yuck.  Let's call the former "exit_with_status" (meaning, the
> caller instructed us to do that) and the latter "has_changes".

I like "exit_with_status". But has_changes looks confusing
good near return value of run_diff_files_cmd, which "has"
nothing. Or do you mean to highlight this "difference"?

> > +test_expect_failure 'git diff-index --cached HEAD^' '
> > +     echo text >>b &&
> > +     echo 3 >c &&
> > +     git add . &&
> > +     git diff-index --exit-code --cached HEAD^
> > +'
>
> Please:
>
>         test_expect_success '...
>                 echo ... &&
>                 git add . &&
>                 ! git diff-index ...
>         '
>
> I recall somebody on the list had a buggy shell that cannot
> handle "a && ! b" and tweaked a few tests to work around it.
> In that case...
>
>         echo ... &&
>         git add . &&
>         {
>                 git diff-index ...; test $? != 0
>         }

Confused. What's wrong with test_expect_failure which
does not need any of the both ugly constructions?
Hmm... Do you mean: "we expect successful
operation with exit code 1"? I like this, easier to follow.
Will do.

> > +test_expect_failure 'git diff-files' '
> > +     echo 3 >>c &&
> > +     git diff-files --exit-code
> > +'
>
> Likewise

Yep, I'll change this too.

> > +git update-index c || error "update-index failed"
>
> Please do not have command outside test_expect without good
> reason.

Well, it does not actually belongs to the test logic
(as well as the echo's, one may notice).

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

>>         echo ... &&
>>         git add . &&
>>         {
>>                 git diff-index ...; test $? != 0
>>         }
>
> Confused. What's wrong with test_expect_failure which
> does not need any of the both ugly constructions?
> Hmm... Do you mean: "we expect successful
> operation with exit code 1"?

No.  Think what happens if you broke "git add".

[PATCH] Allow git-diff exit with codes similar to diff(1)

[Alex Riesen]

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

This introduces a new command-line option: --exit-code. The diff
programs will return 1 for differences, return 0 for equality, and
something else for errors.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---

Updated patch:
- implemented exit_with_status+has_changes
- fixed test WRT intermediate failures

On 3/14/07, Junio C Hamano <junkio@cox.net> wrote:
> "Alex Riesen" <raa.lkml@gmail.com> writes:
>
> >>         echo ... &&
> >>         git add . &&
> >>         {
> >>                 git diff-index ...; test $? != 0
> >>         }
> >
> > Confused. What's wrong with test_expect_failure which
> > does not need any of the both ugly constructions?
> > Hmm... Do you mean: "we expect successful
> > operation with exit code 1"?
>
> No.  Think what happens if you broke "git add".
>

Right. Figured that out myself while changing the test.
Removed all test_expect_failure anyway: not that I
actually expect a failure, just exit code. Made the test
stricter, too: it must be either 0 or 1. Anything else
considered test failure.

 Documentation/diff-options.txt |    5 +++
 builtin-diff-files.c           |    4 ++-
 builtin-diff-index.c           |    4 ++-
 builtin-diff-tree.c            |    5 ++-
 builtin-diff.c                 |   19 ++++++----
 diff-lib.c                     |    5 ++-
 diff.c                         |    6 +++
 diff.h                         |    5 ++-
 t/t4017-diff-retval.sh         |   79 ++++++++++++++++++++++++++++++++++++++++
 9 files changed, 118 insertions(+), 14 deletions(-)
 create mode 100755 t/t4017-diff-retval.sh

[-- Attachment #2: 0001-Allow-git-diff-exit-with-codes-similar-to-diff-1.patch --]
[-- Type: application/xxxxx, Size: 9982 bytes --]

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> Updated patch:
> - implemented exit_with_status+has_changes
> - fixed test WRT intermediate failures

Thanks.

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Junio C Hamano]

"Alex Riesen" <raa.lkml@gmail.com> writes:

> On 3/14/07, Junio C Hamano <junkio@cox.net> wrote:
>> > -     return run_diff_files_cmd(&rev, argc, argv);
>> > +     result = run_diff_files_cmd(&rev, argc, argv);
>> > +     return rev.diffopt.diff_exit_code ? rev.diffopt.exit_code: result;
>> >  }
>>
>> Yuck.  Let's call the former "exit_with_status" (meaning, the
>> caller instructed us to do that) and the latter "has_changes".
>
> I like "exit_with_status". But has_changes looks confusing
> good near return value of run_diff_files_cmd, which "has"
> nothing. Or do you mean to highlight this "difference"?

Maybe 'found_changes' would be a better name, then?

	if (exit_with_status)
        	return !!found_changes;
	else
        	return result;

Re: [PATCH] Allow git-diff exit with codes similar to diff(1)

[Alex Riesen]

On 3/14/07, Junio C Hamano <junkio@cox.net> wrote:
> >> > -     return run_diff_files_cmd(&rev, argc, argv);
> >> > +     result = run_diff_files_cmd(&rev, argc, argv);
> >> > +     return rev.diffopt.diff_exit_code ? rev.diffopt.exit_code: result;
> >> >  }
> >>
> >> Yuck.  Let's call the former "exit_with_status" (meaning, the
> >> caller instructed us to do that) and the latter "has_changes".
> >
> > I like "exit_with_status". But has_changes looks confusing
> > good near return value of run_diff_files_cmd, which "has"
> > nothing. Or do you mean to highlight this "difference"?
>
> Maybe 'found_changes' would be a better name, then?
>
>         if (exit_with_status)
>                 return !!found_changes;
>         else
>                 return result;

Nah... has_changes has almost the same meaning,
and I'm weary regarding making it a changing counter
(which one can assume it to be when seeing !!found_changes).
And I already sent the patch off.