[PATCH] Enable git rev-list to parse --quiet

[PATCH] Enable git rev-list to parse --quiet

[Nick Andrew]

Enable git rev-list to parse --quiet

git rev-list never sees the --quiet option because --quiet is
also an option for diff-files.

Example:

$ ./git rev-list --quiet ^HEAD~2 HEAD
1e102bf7c83281944ffd9202a7d35c514e4a5644
3bf0dd1f4e75ee1591169b687ce04dff00ae2e3e
$ echo $?
0

The fix scans the argument list to detect --quiet before passing it
to setup_revisions(). It also arranges to count the number of commits
or objects (whether sent to STDOUT or not) so --quiet can return an
appropriate exit code (1 if there were commits/objects, 0 otherwise).

After fix:

$ ./git rev-list --quiet ^HEAD~2 HEAD
$ echo $?
1
---

 builtin-rev-list.c |   28 ++++++++++++++++++++++++----
 1 files changed, 24 insertions(+), 4 deletions(-)


diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 8e1720c..e2e5e13 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -52,6 +52,11 @@ static const char rev_list_usage[] =
 
 static struct rev_info revs;
 
+/* Count of number of commits or objects noticed (even if not output).
+ * Used by --quiet option to set an appropriate exit status.
+ */
+static int seen_count;
+
 static int bisect_list;
 static int show_timestamp;
 static int hdr_termination;
@@ -167,12 +172,14 @@ static void finish_commit(struct commit *commit)
 	}
 	free(commit->buffer);
 	commit->buffer = NULL;
+	seen_count++;
 }
 
 static void finish_object(struct object_array_entry *p)
 {
 	if (p->item->type == OBJ_BLOB && !has_sha1_file(p->item->sha1))
 		die("missing blob object '%s'", sha1_to_hex(p->item->sha1));
+	seen_count++;
 }
 
 static void show_object(struct object_array_entry *p)
@@ -588,6 +595,17 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	init_revisions(&revs, prefix);
 	revs.abbrev = 0;
 	revs.commit_format = CMIT_FMT_UNSPECIFIED;
+
+	/* Parse options which are also recognised by git-diff-files */
+	for (i = 1 ; i < argc; i++) {
+		const char *arg = argv[i];
+
+		if (!strcmp(arg, "--quiet")) {
+			quiet = 1;
+			continue;
+		}
+	}
+
 	argc = setup_revisions(argc, argv, &revs, NULL);
 
 	for (i = 1 ; i < argc; i++) {
@@ -621,10 +639,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 			read_revisions_from_stdin(&revs);
 			continue;
 		}
-		if (!strcmp(arg, "--quiet")) {
-			quiet = 1;
-			continue;
-		}
 		usage(rev_list_usage);
 
 	}
@@ -700,9 +714,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	seen_count = 0;
+
 	traverse_commit_list(&revs,
 		quiet ? finish_commit : show_commit,
 		quiet ? finish_object : show_object);
 
+	if (quiet) {
+		return seen_count ? 1 : 0;
+	}
+
 	return 0;
 }

Re: [PATCH] Enable git rev-list to parse --quiet

[Junio C Hamano]

Nick Andrew <nick@nick-andrew.net> writes:

> Enable git rev-list to parse --quiet
>
> git rev-list never sees the --quiet option because --quiet is
> also an option for diff-files.
>
> Example:
>
> $ ./git rev-list --quiet ^HEAD~2 HEAD
> 1e102bf7c83281944ffd9202a7d35c514e4a5644
> 3bf0dd1f4e75ee1591169b687ce04dff00ae2e3e
> $ echo $?
> 0
>
> The fix scans the argument list to detect --quiet before passing it
> to setup_revisions(). It also arranges to count the number of commits
> or objects (whether sent to STDOUT or not) so --quiet can return an
> appropriate exit code (1 if there were commits/objects, 0 otherwise).
>
> After fix:

Thanks for noticing, but this replaces one breakage with another.

Your new behaviour is a new "tell me if it is an empty set" option, and it
means quite different thing from what --quiet does.

The --quiet option is designed primarily for sanity checking after a
failed fetch by commit walkers.  Here is how it works (well, at least how
it is supposed to work).

Imagine you have this history:

	---o---o---X

and the other side has this history:

	---o---o---X---A---B---C

And you run fetch over a dumb protocol; the commit walker fetches C,
discovers you do not have its parent B and tries to fetch it, and you
somehow kill that process.  Your repository will have:

	---o---o---X           C

Now, we do not mark C with our refs, so we do not say "Ok we have
everything leading up to C" when you re-run the same commit walker.
Instead, we'll let the walker walk again starting from C.  So we will
never in corrupt state.

But you might want to see if your repository has this kind of failure.
For that, you can run rev-list starting from C and X --- it will fail
after it finds out that C's parent is B and tries to read it.  And you
will learn the failure with the exit code from the command.  --quiet was
about squelching the output of "I've seen C", "I've seen X", as the only
thing you care about in that mode of usage is if the history is well
connected which is reported by the exit code.

-- >8 --
Subject: [PATCH] rev-list: honor --quiet option

Nick Andrew noticed that rev-list lets the --quiet option to be parsed by
underlying diff_options parser but did not pick up the result.  This
resulted in --quiet option to become effectively a no-op.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-rev-list.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 8e1720c..507201e 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -589,7 +589,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	revs.abbrev = 0;
 	revs.commit_format = CMIT_FMT_UNSPECIFIED;
 	argc = setup_revisions(argc, argv, &revs, NULL);
-
+	quiet = DIFF_OPT_TST(&revs.diffopt, QUIET);
 	for (i = 1 ; i < argc; i++) {
 		const char *arg = argv[i];
 
@@ -621,10 +621,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 			read_revisions_from_stdin(&revs);
 			continue;
 		}
-		if (!strcmp(arg, "--quiet")) {
-			quiet = 1;
-			continue;
-		}
 		usage(rev_list_usage);
 
 	}
-- 
1.5.6.3.573.gd2d2

Re: [PATCH] Enable git rev-list to parse --quiet

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Nick Andrew <nick@nick-andrew.net> writes:
> ...
>> After fix:
>
> Thanks for noticing, but this replaces one breakage with another.
>
> Your new behaviour is a new "tell me if it is an empty set" option, and it
> means quite different thing from what --quiet does.

And here is how I would do it if I were interested in such a feature.

-- >8 --
rev-list --check-empty

This new option squelches the output entirely and signals if the specified
set is empty by its exit status.  E.g.

    $ git rev-list --check-empty HEAD..HEAD

will exit with a non-zero status, and

    $ git rev-list --check-empty HEAD^..HEAD

will exit with zero status.

---
 builtin-rev-list.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 507201e..4f9cce9 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -52,6 +52,7 @@ static const char rev_list_usage[] =
 
 static struct rev_info revs;
 
+static int check_empty;
 static int bisect_list;
 static int show_timestamp;
 static int hdr_termination;
@@ -161,6 +162,8 @@ static void show_commit(struct commit *commit)
 
 static void finish_commit(struct commit *commit)
 {
+	if (check_empty)
+		exit(0);
 	if (commit->parents) {
 		free_commit_list(commit->parents);
 		commit->parents = NULL;
@@ -171,6 +174,8 @@ static void finish_commit(struct commit *commit)
 
 static void finish_object(struct object_array_entry *p)
 {
+	if (check_empty)
+		exit(0);
 	if (p->item->type == OBJ_BLOB && !has_sha1_file(p->item->sha1))
 		die("missing blob object '%s'", sha1_to_hex(p->item->sha1));
 }
@@ -593,6 +598,10 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	for (i = 1 ; i < argc; i++) {
 		const char *arg = argv[i];
 
+		if (!strcmp(arg, "--check-empty")) {
+			check_empty = 1;
+			continue;
+		}
 		if (!strcmp(arg, "--header")) {
 			revs.verbose_header = 1;
 			continue;
@@ -650,6 +659,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 
 	if (prepare_revision_walk(&revs))
 		die("revision walk setup failed");
+	if (check_empty)
+		quiet = 1;
+
 	if (revs.tree_objects)
 		mark_edges_uninteresting(revs.commits, &revs, show_edge);
 
@@ -699,6 +711,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	traverse_commit_list(&revs,
 		quiet ? finish_commit : show_commit,
 		quiet ? finish_object : show_object);
-
+	if (check_empty)
+		exit(1);
 	return 0;
 }

Re: [PATCH] Enable git rev-list to parse --quiet

[Nick Andrew]

On Thu, Jul 17, 2008 at 10:42:21PM -0700, Junio C Hamano wrote:
> Thanks for noticing, but this replaces one breakage with another.
> 
> Your new behaviour is a new "tell me if it is an empty set" option, and it
> means quite different thing from what --quiet does.

Fair enough. Yes, I want to find out if it is an empty set. The
manpage does say "fully connected" which I interpreted to mean
that one set of commits is a subset of the other..

I want to automatically (e.g. in crontab) update a git repo to the latest
HEAD from a remote branch ... but with the possibility that the local
repo has local changes, and I want no chance of merge failure. In other
words, "git fetch remote; git merge origin/master" and only do the
merge if it's a fast-forward. If there are any local commits, or local
uncommitted changes, then leave the local working tree alone.

So my idea was to use "git rev-list --quiet master ^origin/master"
and check the exit code; if zero do "git merge origin/master". Without
a working "--quiet" nor exit code I can pipe the output to "wc -l"
but is there a more efficient/reliable way to implement the requirement?

Nick.

Re: [PATCH] Enable git rev-list to parse --quiet

[Johannes Schindelin]

Hi,

On Fri, 18 Jul 2008, Nick Andrew wrote:

> I want to automatically (e.g. in crontab) update a git repo to the 
> latest HEAD from a remote branch ... but with the possibility that the 
> local repo has local changes, and I want no chance of merge failure. In 
> other words, "git fetch remote; git merge origin/master" and only do the 
> merge if it's a fast-forward. If there are any local commits, or local 
> uncommitted changes, then leave the local working tree alone.
> 
> So my idea was to use "git rev-list --quiet master ^origin/master" and 
> check the exit code; if zero do "git merge origin/master". Without a 
> working "--quiet" nor exit code I can pipe the output to "wc -l" but is 
> there a more efficient/reliable way to implement the requirement?

Yes. Check if "$(git rev-parse master)" is different from "$(git rev-parse 
origin/master)" (to avoid unnecessary merging), and then that "$(git 
merge-base master origin/master)" is equal to "$(git rev-parse master)".

Note: this is plumbing, meant for scripting (which is exactly your 
scenario).  Do not teach this to new Git users.

Ciao,
Dscho

Re: [PATCH] Enable git rev-list to parse --quiet

[Junio C Hamano]

Nick Andrew <nick@nick-andrew.net> writes:

> ...Without
> a working "--quiet" nor exit code I can pipe the output to "wc -l"
> but is there a more efficient/reliable way to implement the requirement?

Did you read the whole thread before asking the above question?

IOW, does this answer the above question?

    http://mid.gmane.org/7vy73zd8ok.fsf@gitster.siamese.dyndns.org

Re: [PATCH] Enable git rev-list to parse --quiet

[Nick Andrew]

On Sun, Jul 20, 2008 at 12:56:28AM -0700, Junio C Hamano wrote:
> Nick Andrew <nick@nick-andrew.net> writes:
> 
> > ...Without
> > a working "--quiet" nor exit code I can pipe the output to "wc -l"
> > but is there a more efficient/reliable way to implement the requirement?
> 
> Did you read the whole thread before asking the above question?

I took your answer to mean that I shouldn't be using git-rev-list
for this purpose, so I asked whether there's a better way to do
it. Johannes Schindelin gave a good answer to that.

> IOW, does this answer the above question?
> 
>     http://mid.gmane.org/7vy73zd8ok.fsf@gitster.siamese.dyndns.org

I'm not happy with that patch due to this:

 static void finish_commit(struct commit *commit)
 {
+       if (check_empty)
+               exit(0);

Exiting a process from within a callback function seems to me to violate
the principle of least surprise. If the return code should be zero then
the cmd_rev_list function should return zero, and run_command will
return zero and handle_internal_command will exit zero. There must be
a better way to avoid redundant processing for the empty set case.

Nick.

Re: [PATCH] Enable git rev-list to parse --quiet

[Junio C Hamano]

Nick Andrew <nick@nick-andrew.net> writes:

> Exiting a process from within a callback function seems to me to violate
> the principle of least surprise.

Huh?  Who is surprised?

I do not know who taught you that "do not exit in a callback" dogma, but I
suspect it was misrepresented when it was taught to you.

A library that calls your function back could be structured this way:

	lib() {
        	perform some set-up that affects external world;
                call your callback function;
                clean-up the effect of previous set-up action;
	}

and exiting from your callback function is not a good idea as it prevents
the library from doing the necessary clean-up in such a case.

But that is true just in a(n extremely) general case.  Your generalization
is not particularly useful, methinks, and use of exit(0) in the patch is
very well justified (rather, I do not think they even need justifying).

 - The callback you are looking at is not a general purpose callback for
   other program's use, but written for a specific use of rev-list;

 - The purpose of that exit(0) is to signal "there is something" as
   quickly as possible, which was what you wanted out of rev-list;

 - Revision traversal is a read-only operation and we know that there is
   no externally visible set-up done in the function you are calling to
   get your callback called, that needs cleaning up later --- this is not
   expected to change, as there are longstanding existing callback
   functions supplied to traverse_commit_list() that die() upon seeing
   errors already.