{"thread":{"id":"14520","subject":"[PATCH] Enable git rev-list to parse --quiet","startedAt":"2008-07-18T04:05:00Z","lastAt":"2008-07-20T18:31:38Z","messageCount":8,"participants":["Nick Andrew","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"83818","messageId":"20080718040459.13073.76896.stgit@marcab.local.tull.net","threadId":"14520","inReplyTo":null,"subject":"[PATCH] Enable git rev-list to parse --quiet","fromName":"Nick Andrew","fromEmail":"nick@nick-andrew.net","sentAt":"2008-07-18T04:05:00Z","receivedAt":"2008-07-18T04:05:00Z","isPatch":true,"sender":{"key":"nick@nick-andrew.net","avatar":"https://gravatar.com/avatar/85f25a67ca6eaa4016ed374f6d07f3cd853c886aeb7e1507eb7dbc47b00082fe?d=mp&s=160"},"body":"Enable git rev-list to parse --quiet\n\ngit rev-list never sees the --quiet option because --quiet is\nalso an option for diff-files.\n\nExample:\n\n$ ./git rev-list --quiet ^HEAD~2 HEAD\n1e102bf7c83281944ffd9202a7d35c514e4a5644\n3bf0dd1f4e75ee1591169b687ce04dff00ae2e3e\n$ echo $?\n0\n\nThe fix scans the argument list to detect --quiet before passing it\nto setup_revisions(). It also arranges to count the number of commits\nor objects (whether sent to STDOUT or not) so --quiet can return an\nappropriate exit code (1 if there were commits/objects, 0 otherwise).\n\nAfter fix:\n\n$ ./git rev-list --quiet ^HEAD~2 HEAD\n$ echo $?\n1\n---\n\n builtin-rev-list.c | 28 ++++++++++++++++++++++++----\n 1 files changed, 24 insertions(+), 4 deletions(-)\n\n\ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex 8e1720c..e2e5e13 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -52,6 +52,11 @@ static const char rev_list_usage[] =\n \n static struct rev_info revs;\n \n+/* Count of number of commits or objects noticed (even if not output).\n+ * Used by --quiet option to set an appropriate exit status.\n+ */\n+static int seen_count;\n+\n static int bisect_list;\n static int show_timestamp;\n static int hdr_termination;\n@@ -167,12 +172,14 @@ static void finish_commit(struct commit *commit)\n \t}\n \tfree(commit->buffer);\n \tcommit->buffer = NULL;\n+\tseen_count++;\n }\n \n static void finish_object(struct object_array_entry *p)\n {\n \tif (p->item->type == OBJ_BLOB && !has_sha1_file(p->item->sha1))\n \t\tdie(\"missing blob object '%s'\", sha1_to_hex(p->item->sha1));\n+\tseen_count++;\n }\n \n static void show_object(struct object_array_entry *p)\n@@ -588,6 +595,17 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tinit_revisions(&revs, prefix);\n \trevs.abbrev = 0;\n \trevs.commit_format = CMIT_FMT_UNSPECIFIED;\n+\n+\t/* Parse options which are also recognised by git-diff-files */\n+\tfor (i = 1 ; i < argc; i++) {\n+\t\tconst char *arg = argv[i];\n+\n+\t\tif (!strcmp(arg, \"--quiet\")) {\n+\t\t\tquiet = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t}\n+\n \targc = setup_revisions(argc, argv, &revs, NULL);\n \n \tfor (i = 1 ; i < argc; i++) {\n@@ -621,10 +639,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tread_revisions_from_stdin(&revs);\n \t\t\tcontinue;\n \t\t}\n-\t\tif (!strcmp(arg, \"--quiet\")) {\n-\t\t\tquiet = 1;\n-\t\t\tcontinue;\n-\t\t}\n \t\tusage(rev_list_usage);\n \n \t}\n@@ -700,9 +714,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tseen_count = 0;\n+\n \ttraverse_commit_list(&revs,\n \t\tquiet ? finish_commit : show_commit,\n \t\tquiet ? finish_object : show_object);\n \n+\tif (quiet) {\n+\t\treturn seen_count ? 1 : 0;\n+\t}\n+\n \treturn 0;\n }\n"},{"id":"83821","messageId":"7v8wvzeojm.fsf@gitster.siamese.dyndns.org","threadId":"14520","inReplyTo":"20080718040459.13073.76896.stgit@marcab.local.tull.net","subject":"Re: [PATCH] Enable git rev-list to parse --quiet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-18T05:42:21Z","receivedAt":"2008-07-18T05:42:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nick Andrew writes:\n\n> Enable git rev-list to parse --quiet\n>\n> git rev-list never sees the --quiet option because --quiet is\n> also an option for diff-files.\n>\n> Example:\n>\n> $ ./git rev-list --quiet ^HEAD~2 HEAD\n> 1e102bf7c83281944ffd9202a7d35c514e4a5644\n> 3bf0dd1f4e75ee1591169b687ce04dff00ae2e3e\n> $ echo $?\n> 0\n>\n> The fix scans the argument list to detect --quiet before passing it\n> to setup_revisions(). It also arranges to count the number of commits\n> or objects (whether sent to STDOUT or not) so --quiet can return an\n> appropriate exit code (1 if there were commits/objects, 0 otherwise).\n>\n> After fix:\n\nThanks for noticing, but this replaces one breakage with another.\n\nYour new behaviour is a new \"tell me if it is an empty set\" option, and it\nmeans quite different thing from what --quiet does.\n\nThe --quiet option is designed primarily for sanity checking after a\nfailed fetch by commit walkers. Here is how it works (well, at least how\nit is supposed to work).\n\nImagine you have this history:\n\n\t---o---o---X\n\nand the other side has this history:\n\n\t---o---o---X---A---B---C\n\nAnd you run fetch over a dumb protocol; the commit walker fetches C,\ndiscovers you do not have its parent B and tries to fetch it, and you\nsomehow kill that process. Your repository will have:\n\n\t---o---o---X C\n\nNow, we do not mark C with our refs, so we do not say \"Ok we have\neverything leading up to C\" when you re-run the same commit walker.\nInstead, we'll let the walker walk again starting from C. So we will\nnever in corrupt state.\n\nBut you might want to see if your repository has this kind of failure.\nFor that, you can run rev-list starting from C and X --- it will fail\nafter it finds out that C's parent is B and tries to read it. And you\nwill learn the failure with the exit code from the command. --quiet was\nabout squelching the output of \"I've seen C\", \"I've seen X\", as the only\nthing you care about in that mode of usage is if the history is well\nconnected which is reported by the exit code.\n\n-- >8 --\nSubject: [PATCH] rev-list: honor --quiet option\n\nNick Andrew noticed that rev-list lets the --quiet option to be parsed by\nunderlying diff_options parser but did not pick up the result. This\nresulted in --quiet option to become effectively a no-op.\n\nSigned-off-by: Junio C Hamano \n---\n builtin-rev-list.c | 6 +-----\n 1 files changed, 1 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex 8e1720c..507201e 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -589,7 +589,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \trevs.abbrev = 0;\n \trevs.commit_format = CMIT_FMT_UNSPECIFIED;\n \targc = setup_revisions(argc, argv, &revs, NULL);\n-\n+\tquiet = DIFF_OPT_TST(&revs.diffopt, QUIET);\n \tfor (i = 1 ; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \n@@ -621,10 +621,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tread_revisions_from_stdin(&revs);\n \t\t\tcontinue;\n \t\t}\n-\t\tif (!strcmp(arg, \"--quiet\")) {\n-\t\t\tquiet = 1;\n-\t\t\tcontinue;\n-\t\t}\n \t\tusage(rev_list_usage);\n \n \t}\n-- \n1.5.6.3.573.gd2d2\n"},{"id":"83830","messageId":"7vy73zd8ok.fsf@gitster.siamese.dyndns.org","threadId":"14520","inReplyTo":"7v8wvzeojm.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Enable git rev-list to parse --quiet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-18T06:10:19Z","receivedAt":"2008-07-18T06:10:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Nick Andrew writes:\n> ...\n>> After fix:\n>\n> Thanks for noticing, but this replaces one breakage with another.\n>\n> Your new behaviour is a new \"tell me if it is an empty set\" option, and it\n> means quite different thing from what --quiet does.\n\nAnd here is how I would do it if I were interested in such a feature.\n\n-- >8 --\nrev-list --check-empty\n\nThis new option squelches the output entirely and signals if the specified\nset is empty by its exit status. E.g.\n\n $ git rev-list --check-empty HEAD..HEAD\n\nwill exit with a non-zero status, and\n\n $ git rev-list --check-empty HEAD^..HEAD\n\nwill exit with zero status.\n\n---\n builtin-rev-list.c | 15 ++++++++++++++-\n 1 files changed, 14 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-rev-list.c b/builtin-rev-list.c\nindex 507201e..4f9cce9 100644\n--- a/builtin-rev-list.c\n+++ b/builtin-rev-list.c\n@@ -52,6 +52,7 @@ static const char rev_list_usage[] =\n \n static struct rev_info revs;\n \n+static int check_empty;\n static int bisect_list;\n static int show_timestamp;\n static int hdr_termination;\n@@ -161,6 +162,8 @@ static void show_commit(struct commit *commit)\n \n static void finish_commit(struct commit *commit)\n {\n+\tif (check_empty)\n+\t\texit(0);\n \tif (commit->parents) {\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n@@ -171,6 +174,8 @@ static void finish_commit(struct commit *commit)\n \n static void finish_object(struct object_array_entry *p)\n {\n+\tif (check_empty)\n+\t\texit(0);\n \tif (p->item->type == OBJ_BLOB && !has_sha1_file(p->item->sha1))\n \t\tdie(\"missing blob object '%s'\", sha1_to_hex(p->item->sha1));\n }\n@@ -593,6 +598,10 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tfor (i = 1 ; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \n+\t\tif (!strcmp(arg, \"--check-empty\")) {\n+\t\t\tcheck_empty = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--header\")) {\n \t\t\trevs.verbose_header = 1;\n \t\t\tcontinue;\n@@ -650,6 +659,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \n \tif (prepare_revision_walk(&revs))\n \t\tdie(\"revision walk setup failed\");\n+\tif (check_empty)\n+\t\tquiet = 1;\n+\n \tif (revs.tree_objects)\n \t\tmark_edges_uninteresting(revs.commits, &revs, show_edge);\n \n@@ -699,6 +711,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \ttraverse_commit_list(&revs,\n \t\tquiet ? finish_commit : show_commit,\n \t\tquiet ? finish_object : show_object);\n-\n+\tif (check_empty)\n+\t\texit(1);\n \treturn 0;\n }\n"},{"id":"83854","messageId":"20080718092001.GD16102@mail.local.tull.net","threadId":"14520","inReplyTo":"7v8wvzeojm.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Enable git rev-list to parse --quiet","fromName":"Nick Andrew","fromEmail":"nick@nick-andrew.net","sentAt":"2008-07-18T09:20:01Z","receivedAt":"2008-07-18T09:20:01Z","isPatch":true,"sender":{"key":"nick@nick-andrew.net","avatar":"https://gravatar.com/avatar/85f25a67ca6eaa4016ed374f6d07f3cd853c886aeb7e1507eb7dbc47b00082fe?d=mp&s=160"},"body":"On Thu, Jul 17, 2008 at 10:42:21PM -0700, Junio C Hamano wrote:\n> Thanks for noticing, but this replaces one breakage with another.\n> \n> Your new behaviour is a new \"tell me if it is an empty set\" option, and it\n> means quite different thing from what --quiet does.\n\nFair enough. Yes, I want to find out if it is an empty set. The\nmanpage does say \"fully connected\" which I interpreted to mean\nthat one set of commits is a subset of the other..\n\nI want to automatically (e.g. in crontab) update a git repo to the latest\nHEAD from a remote branch ... but with the possibility that the local\nrepo has local changes, and I want no chance of merge failure. In other\nwords, \"git fetch remote; git merge origin/master\" and only do the\nmerge if it's a fast-forward. If there are any local commits, or local\nuncommitted changes, then leave the local working tree alone.\n\nSo my idea was to use \"git rev-list --quiet master ^origin/master\"\nand check the exit code; if zero do \"git merge origin/master\". Without\na working \"--quiet\" nor exit code I can pipe the output to \"wc -l\"\nbut is there a more efficient/reliable way to implement the requirement?\n\nNick.\n"},{"id":"83864","messageId":"alpine.DEB.1.00.0807181246590.3932@eeepc-johanness","threadId":"14520","inReplyTo":"20080718092001.GD16102@mail.local.tull.net","subject":"Re: [PATCH] Enable git rev-list to parse --quiet","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-07-18T10:50:11Z","receivedAt":"2008-07-18T10:50:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 18 Jul 2008, Nick Andrew wrote:\n\n> I want to automatically (e.g. in crontab) update a git repo to the \n> latest HEAD from a remote branch ... but with the possibility that the \n> local repo has local changes, and I want no chance of merge failure. In \n> other words, \"git fetch remote; git merge origin/master\" and only do the \n> merge if it's a fast-forward. If there are any local commits, or local \n> uncommitted changes, then leave the local working tree alone.\n> \n> So my idea was to use \"git rev-list --quiet master ^origin/master\" and \n> check the exit code; if zero do \"git merge origin/master\". Without a \n> working \"--quiet\" nor exit code I can pipe the output to \"wc -l\" but is \n> there a more efficient/reliable way to implement the requirement?\n\nYes. Check if \"$(git rev-parse master)\" is different from \"$(git rev-parse \norigin/master)\" (to avoid unnecessary merging), and then that \"$(git \nmerge-base master origin/master)\" is equal to \"$(git rev-parse master)\".\n\nNote: this is plumbing, meant for scripting (which is exactly your \nscenario). Do not teach this to new Git users.\n\nCiao,\nDscho\n"},{"id":"84008","messageId":"7vwsjhc7kj.fsf@gitster.siamese.dyndns.org","threadId":"14520","inReplyTo":"20080718092001.GD16102@mail.local.tull.net","subject":"Re: [PATCH] Enable git rev-list to parse --quiet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-20T07:56:28Z","receivedAt":"2008-07-20T07:56:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nick Andrew writes:\n\n> ...Without\n> a working \"--quiet\" nor exit code I can pipe the output to \"wc -l\"\n> but is there a more efficient/reliable way to implement the requirement?\n\nDid you read the whole thread before asking the above question?\n\nIOW, does this answer the above question?\n\n http://mid.gmane.org/7vy73zd8ok.fsf@gitster.siamese.dyndns.org\n"},{"id":"84024","messageId":"20080720120437.GC15586@mail.local.tull.net","threadId":"14520","inReplyTo":"7vwsjhc7kj.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Enable git rev-list to parse --quiet","fromName":"Nick Andrew","fromEmail":"nick@nick-andrew.net","sentAt":"2008-07-20T12:04:37Z","receivedAt":"2008-07-20T12:04:37Z","isPatch":true,"sender":{"key":"nick@nick-andrew.net","avatar":"https://gravatar.com/avatar/85f25a67ca6eaa4016ed374f6d07f3cd853c886aeb7e1507eb7dbc47b00082fe?d=mp&s=160"},"body":"On Sun, Jul 20, 2008 at 12:56:28AM -0700, Junio C Hamano wrote:\n> Nick Andrew writes:\n> \n> > ...Without\n> > a working \"--quiet\" nor exit code I can pipe the output to \"wc -l\"\n> > but is there a more efficient/reliable way to implement the requirement?\n> \n> Did you read the whole thread before asking the above question?\n\nI took your answer to mean that I shouldn't be using git-rev-list\nfor this purpose, so I asked whether there's a better way to do\nit. Johannes Schindelin gave a good answer to that.\n\n> IOW, does this answer the above question?\n> \n> http://mid.gmane.org/7vy73zd8ok.fsf@gitster.siamese.dyndns.org\n\nI'm not happy with that patch due to this:\n\n static void finish_commit(struct commit *commit)\n {\n+ if (check_empty)\n+ exit(0);\n\nExiting a process from within a callback function seems to me to violate\nthe principle of least surprise. If the return code should be zero then\nthe cmd_rev_list function should return zero, and run_command will\nreturn zero and handle_internal_command will exit zero. There must be\na better way to avoid redundant processing for the empty set case.\n\nNick.\n"},{"id":"84046","messageId":"7vwsjg76gl.fsf@gitster.siamese.dyndns.org","threadId":"14520","inReplyTo":"20080720120437.GC15586@mail.local.tull.net","subject":"Re: [PATCH] Enable git rev-list to parse --quiet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-20T18:31:38Z","receivedAt":"2008-07-20T18:31:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nick Andrew writes:\n\n> Exiting a process from within a callback function seems to me to violate\n> the principle of least surprise.\n\nHuh? Who is surprised?\n\nI do not know who taught you that \"do not exit in a callback\" dogma, but I\nsuspect it was misrepresented when it was taught to you.\n\nA library that calls your function back could be structured this way:\n\n\tlib() {\n \tperform some set-up that affects external world;\n call your callback function;\n clean-up the effect of previous set-up action;\n\t}\n\nand exiting from your callback function is not a good idea as it prevents\nthe library from doing the necessary clean-up in such a case.\n\nBut that is true just in a(n extremely) general case. Your generalization\nis not particularly useful, methinks, and use of exit(0) in the patch is\nvery well justified (rather, I do not think they even need justifying).\n\n - The callback you are looking at is not a general purpose callback for\n other program's use, but written for a specific use of rev-list;\n\n - The purpose of that exit(0) is to signal \"there is something\" as\n quickly as possible, which was what you wanted out of rev-list;\n\n - Revision traversal is a read-only operation and we know that there is\n no externally visible set-up done in the function you are calling to\n get your callback called, that needs cleaning up later --- this is not\n expected to change, as there are longstanding existing callback\n functions supplied to traverse_commit_list() that die() upon seeing\n errors already.\n"}]}