{"thread":{"id":"2226","subject":"git-rev-list: make --dense the default (and introduce \"--sparse\")","startedAt":"2005-10-25T22:24:55Z","lastAt":"2005-10-26T20:03:53Z","messageCount":5,"participants":["Linus Torvalds","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"10618","messageId":"Pine.LNX.4.64.0510251459070.10477@g5.osdl.org","threadId":"2226","inReplyTo":null,"subject":"git-rev-list: make --dense the default (and introduce \"--sparse\")","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-10-25T22:24:55Z","receivedAt":"2005-10-25T22:24:55Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nThis actually does three things:\n\n - make \"--dense\" the default for git-rev-list. Since dense is a no-op if \n   no filenames are given, this doesn't actually change any historical \n   behaviour, but it's logically the right default (if we want to prune on \n   filenames, do it fully. The sparse \"merge-only\" thing may be useful, \n   but it's not what you'd normally expect)\n\n - make \"git-rev-parse\" show the default revision control before it shows \n   any pathnames.\n\n   This was a real bug, but nobody would ever have noticed, because \n   the default thing tends to only make sense for git-rev-list, and \n   git-rev-list didn't use to take pathnames.\n\n - it changes \"git-rev-list\" to match the other commands that take a mix \n   of revisions and filenames - it no longer requires the \"--\" before \n   filenames (although you still need to do it if a filename could be \n   confused with a revision name, eg \"gitk\" in the git archive)\n\nThis all just makes for much more pleasant and obvous usage. Just doing a\n\n\tgitk t/\n\ndoes the obvious thing: it will show the history as it concerns the \"t/\" \nsubdirectory.\n\nSigned-off-by: Linus Torvalds <torvalds@osdl.org>\n---\n\nNOTE! None of this will change anything that used to work before. But \nthings that used to make git-rev-list die with a usage message will now \npossibly do things. In particular, it may do something that you don't \nexpect.\n\nFor example, let's say that you mis-type a revision name, and do\n\n\tgitk v0.99.88\n\nwith one \"8\" too much. It used to cause gitk to die on you, echoing the \nusage string from git-rev-list. Now it will cause gitk to start up \nhappily, and say \"No commits selected\", because there is no _file_ called \n\"v0.99.88\" anywhere, so git-rev-list will end up with empty output.\n\nSo it does change behaviour, even if it doesn't change anything that was \n_successful_ before.\n\nFor the same reasons, a\n\n\tgit-rev-list $(git-rev-parse --default HEAD v0.99.88)\n\nwill now give an empty list rather than an error usage string.\n\n(But doing\n\n\tgit-rev-list v0.99.88\n\nwill still cause an error, since that doesn't have any revision at all!)\n\nComments? It really makes usage more \"obvious\", I think.\n\n\ndiff --git a/rev-list.c b/rev-list.c\nindex edf3b37..ac7a47f 100644\n--- a/rev-list.c\n+++ b/rev-list.c\n@@ -28,7 +28,7 @@ static const char rev_list_usage[] =\n \t\t      \"  --merge-order [ --show-breaks ]\\n\"\n \t\t      \"  --topo-order\";\n \n-static int dense = 0;\n+static int dense = 1;\n static int unpacked = 0;\n static int bisect_list = 0;\n static int tag_objects = 0;\n@@ -619,7 +619,7 @@ static struct commit *get_commit_referen\n \tstruct object *object;\n \n \tif (get_sha1(name, sha1))\n-\t\tusage(rev_list_usage);\n+\t\treturn NULL;\n \tobject = parse_object(sha1);\n \tif (!object)\n \t\tdie(\"bad object %s\", name);\n@@ -793,12 +793,12 @@ int main(int argc, const char **argv)\n \t\t\tdense = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"--sparse\")) {\n+\t\t\tdense = 0;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--\")) {\n-\t\t\tpaths = get_pathspec(prefix, argv + i + 1);\n-\t\t\tif (paths) {\n-\t\t\t\tlimited = 1;\n-\t\t\t\tdiff_tree_setup_paths(paths);\n-\t\t\t}\n+\t\t\ti++;\n \t\t\tbreak;\n \t\t}\n \n@@ -830,9 +830,20 @@ int main(int argc, const char **argv)\n \t\t\tlimited = 1;\n \t\t}\n \t\tcommit = get_commit_reference(arg, flags);\n+\t\tif (!commit)\n+\t\t\tbreak;\n \t\thandle_one_commit(commit, &list);\n \t}\n \n+\tif (!list)\n+\t\tusage(rev_list_usage);\n+\n+\tpaths = get_pathspec(prefix, argv + i);\n+\tif (paths) {\n+\t\tlimited = 1;\n+\t\tdiff_tree_setup_paths(paths);\n+\t}\n+\n \tsave_commit_buffer = verbose_header;\n \ttrack_object_refs = 0;\n \ndiff --git a/rev-parse.c b/rev-parse.c\nindex 243f89f..adfc68c 100644\n--- a/rev-parse.c\n+++ b/rev-parse.c\n@@ -153,6 +153,7 @@ static void show_datestring(const char *\n \n static void show_file(const char *arg)\n {\n+\tshow_default();\n \tif ((filter & (DO_NONFLAGS|DO_NOREV)) == (DO_NONFLAGS|DO_NOREV))\n \t\tshow(arg);\n }\n@@ -174,7 +175,6 @@ int main(int argc, char **argv)\n \t\tif (*arg == '-') {\n \t\t\tif (!strcmp(arg, \"--\")) {\n \t\t\t\tas_is = 1;\n-\t\t\t\tshow_default();\n \t\t\t\t/* Pass on the \"--\" if we show anything but files.. */\n \t\t\t\tif (filter & (DO_FLAGS | DO_REVS))\n \t\t\t\t\tshow_file(arg);\n"},{"id":"10619","messageId":"Pine.LNX.4.64.0510251525540.10477@g5.osdl.org","threadId":"2226","inReplyTo":"Pine.LNX.4.64.0510251459070.10477@g5.osdl.org","subject":"Re: git-rev-list: make --dense the default (and introduce \"--sparse\")","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-10-25T22:29:42Z","receivedAt":"2005-10-25T22:29:42Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 25 Oct 2005, Linus Torvalds wrote:\n> \n> This actually does three things:\n> \n>  - make \"--dense\" the default for git-rev-list. Since dense is a no-op if \n>    no filenames are given, this doesn't actually change any historical \n>    behaviour, but it's logically the right default (if we want to prune on \n>    filenames, do it fully. The sparse \"merge-only\" thing may be useful, \n>    but it's not what you'd normally expect)\n> \n>  - make \"git-rev-parse\" show the default revision control before it shows \n>    any pathnames.\n\nBtw, if it wasn't obvious, this has some subtle (and very nice) side \neffects.\n\nFor example, try this:\n\n\tgit log git-fetch.sh\n\nand notice how \"git log\" automagically became able to give per-file logs. \nWith _zero_ changes to git-log.sh itself ;)\n\nAnd same old rev-confusion-avoidance:\n\n\tgit log -- gitk\n\nto make clear that you want to get the log of \"gitk the file\", rather than \n\"gitk the branch\".\n\nMagic. \n\n\t\tLinus\n"},{"id":"10652","messageId":"7vwtk08wnx.fsf@assigned-by-dhcp.cox.net","threadId":"2226","inReplyTo":"Pine.LNX.4.64.0510251525540.10477@g5.osdl.org","subject":"Re: git-rev-list: make --dense the default (and introduce \"--sparse\")","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-10-26T09:17:22Z","receivedAt":"2005-10-26T09:17:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> On Tue, 25 Oct 2005, Linus Torvalds wrote:\n>> \n>> This actually does three things:\n>> \n>>  - make \"--dense\" the default for git-rev-list...\n\nHeads up.\n\nI have not looked closely into what exactly, but the fourth\nthing this does might be to break git-send-pack.\n\nI usually use the tip of \"pu\" myself, but for tonight, I am\nexcluding the fetch-pack/upload-pack changes from Johannes when\nbuilding git for my own use, and using somewhere in the middle\nof \"pu\" branch.  With this \"--dense default\" patch,\ngit-send-pack seems to send too few objects.  With this patch\nreverted, git-send-pack seems to work again.\n\n +  [build] Revert \"git-rev-list: make --dense the default (and introduce \"--sparse\")\"\n ++ [pu^] Merge branch 'js-fat'\n ++ [pu^^2] Test in git-init-db if the filemode can be trusted\n ++ [pu~2] Merge branches 'cache-pack', 'lazy-subdir' and 'lt-dense'\n ++ [pu~2^4] git-rev-list: make --dense the default (and introduce \"--sparse\")\n ++ [pu~2^3] Create object subdirectories on demand (phase II)\n ++ [pu~2^2] Allow caching of generated pack for full cloning.\n+++ [master] upload-pack: tighten request validation.\n\nI'll take a look at the issue in the morning unless somebody\nelse beats me to it.\n"},{"id":"10664","messageId":"Pine.LNX.4.64.0510260757350.10477@g5.osdl.org","threadId":"2226","inReplyTo":"7vwtk08wnx.fsf@assigned-by-dhcp.cox.net","subject":"Re: git-rev-list: make --dense the default (and introduce \"--sparse\")","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2005-10-26T15:18:13Z","receivedAt":"2005-10-26T15:18:13Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 26 Oct 2005, Junio C Hamano wrote:\n> \n> I have not looked closely into what exactly, but the fourth\n> thing this does might be to break git-send-pack.\n\nAck. And I see why.\n\nWhat happens is that the new logic decides that if it can't look up a \ncommit reference (ie \"get_commit_reference()\" returns NULL), the thing \nmust be a pathname.\n\nFair enough.\n\nBut wrong.\n\nThe thing is, it may be a perfectly fine ref that _isn't_ a commit. In \ngit, you have a tag that points to your PGP key, and in the kernel, I have \na tag that points to a tree (and a direct ref that points to that tree \ntoo, for that matter). \n\nSo the rule is (as for all the other programs that mix revs and pathnames) \nnot that we only accept commit references, but _any_ valid object ref.\n\nIf the object then isn't a commit ref, git-rev-list will either ignore it, \nor add it to the list of non-commit objects (if using \"--objects\").\n\nThe solution is to move the \"get_sha1()\" out of get_commit_reference(), \nand into the callers. In fact, we already _have_ the SHA1 in the case of \nthe handle_all() loop, since for_each_ref() will have done it for us, so \nthis is the correct thing to do anyway. \n\nThis patch (on top of the original one) does exactly that.\n\n\t\tLinus\n\n----\ndiff --git a/rev-list.c b/rev-list.c\nindex ac7a47f..2b82b8a 100644\n--- a/rev-list.c\n+++ b/rev-list.c\n@@ -613,13 +613,10 @@ static void add_pending_object(struct ob\n \tadd_object(obj, &pending_objects, name);\n }\n \n-static struct commit *get_commit_reference(const char *name, unsigned int flags)\n+static struct commit *get_commit_reference(const char *name, const unsigned char *sha1, unsigned int flags)\n {\n-\tunsigned char sha1[20];\n \tstruct object *object;\n \n-\tif (get_sha1(name, sha1))\n-\t\treturn NULL;\n \tobject = parse_object(sha1);\n \tif (!object)\n \t\tdie(\"bad object %s\", name);\n@@ -697,7 +694,7 @@ static struct commit_list **global_lst;\n \n static int include_one_commit(const char *path, const unsigned char *sha1)\n {\n-\tstruct commit *com = get_commit_reference(path, 0);\n+\tstruct commit *com = get_commit_reference(path, sha1, 0);\n \thandle_one_commit(com, global_lst);\n \treturn 0;\n }\n@@ -720,6 +717,7 @@ int main(int argc, const char **argv)\n \t\tconst char *arg = argv[i];\n \t\tchar *dotdot;\n \t\tstruct commit *commit;\n+\t\tunsigned char sha1[20];\n \n \t\tif (!strncmp(arg, \"--max-count=\", 12)) {\n \t\t\tmax_count = atoi(arg + 12);\n@@ -808,15 +806,19 @@ int main(int argc, const char **argv)\n \t\tflags = 0;\n \t\tdotdot = strstr(arg, \"..\");\n \t\tif (dotdot) {\n+\t\t\tunsigned char from_sha1[20];\n \t\t\tchar *next = dotdot + 2;\n-\t\t\tstruct commit *exclude = NULL;\n-\t\t\tstruct commit *include = NULL;\n \t\t\t*dotdot = 0;\n \t\t\tif (!*next)\n \t\t\t\tnext = \"HEAD\";\n-\t\t\texclude = get_commit_reference(arg, UNINTERESTING);\n-\t\t\tinclude = get_commit_reference(next, 0);\n-\t\t\tif (exclude && include) {\n+\t\t\tif (!get_sha1(arg, from_sha1) && !get_sha1(next, sha1)) {\n+\t\t\t\tstruct commit *exclude;\n+\t\t\t\tstruct commit *include;\n+\t\t\t\t\n+\t\t\t\texclude = get_commit_reference(arg, from_sha1, UNINTERESTING);\n+\t\t\t\tinclude = get_commit_reference(next, sha1, 0);\n+\t\t\t\tif (!exclude || !include)\n+\t\t\t\t\tdie(\"Invalid revision range %s..%s\", arg, next);\n \t\t\t\tlimited = 1;\n \t\t\t\thandle_one_commit(exclude, &list);\n \t\t\t\thandle_one_commit(include, &list);\n@@ -829,9 +831,9 @@ int main(int argc, const char **argv)\n \t\t\targ++;\n \t\t\tlimited = 1;\n \t\t}\n-\t\tcommit = get_commit_reference(arg, flags);\n-\t\tif (!commit)\n+\t\tif (get_sha1(arg, sha1) < 0)\n \t\t\tbreak;\n+\t\tcommit = get_commit_reference(arg, sha1, flags);\n \t\thandle_one_commit(commit, &list);\n \t}\n \n"},{"id":"10680","messageId":"7vvezkcafq.fsf@assigned-by-dhcp.cox.net","threadId":"2226","inReplyTo":"Pine.LNX.4.64.0510260757350.10477@g5.osdl.org","subject":"Re: git-rev-list: make --dense the default (and introduce \"--sparse\")","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2005-10-26T20:03:53Z","receivedAt":"2005-10-26T20:03:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> On Wed, 26 Oct 2005, Junio C Hamano wrote:\n>> \n>> I have not looked closely into what exactly, but the fourth\n>> thing this does might be to break git-send-pack.\n>\n> Ack. And I see why.\n>\n>\n> This patch (on top of the original one) does exactly that.\n\nThanks, this fixes it.\n"}]}