# git-rev-list: make --dense the default (and introduce "--sparse") 5 messages from 2005-10-25 to 2005-10-26. Participants: Linus Torvalds, Junio C Hamano. Thread: https://gitlist.dev/t/2226 ## Linus Torvalds, 2005-10-25 22:24 Subject: git-rev-list: make --dense the default (and introduce "--sparse") Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0510251459070.10477%40g5.osdl.org ``` This actually does three things: - make "--dense" the default for git-rev-list. Since dense is a no-op if no filenames are given, this doesn't actually change any historical behaviour, but it's logically the right default (if we want to prune on filenames, do it fully. The sparse "merge-only" thing may be useful, but it's not what you'd normally expect) - make "git-rev-parse" show the default revision control before it shows any pathnames. This was a real bug, but nobody would ever have noticed, because the default thing tends to only make sense for git-rev-list, and git-rev-list didn't use to take pathnames. - it changes "git-rev-list" to match the other commands that take a mix of revisions and filenames - it no longer requires the "--" before filenames (although you still need to do it if a filename could be confused with a revision name, eg "gitk" in the git archive) This all just makes for much more pleasant and obvous usage. Just doing a gitk t/ does the obvious thing: it will show the history as it concerns the "t/" subdirectory. Signed-off-by: Linus Torvalds --- NOTE! None of this will change anything that used to work before. But things that used to make git-rev-list die with a usage message will now possibly do things. In particular, it may do something that you don't expect. For example, let's say that you mis-type a revision name, and do gitk v0.99.88 with one "8" too much. It used to cause gitk to die on you, echoing the usage string from git-rev-list. Now it will cause gitk to start up happily, and say "No commits selected", because there is no _file_ called "v0.99.88" anywhere, so git-rev-list will end up with empty output. So it does change behaviour, even if it doesn't change anything that was _successful_ before. For the same reasons, a git-rev-list $(git-rev-parse --default HEAD v0.99.88) will now give an empty list rather than an error usage string. (But doing git-rev-list v0.99.88 will still cause an error, since that doesn't have any revision at all!) Comments? It really makes usage more "obvious", I think. diff --git a/rev-list.c b/rev-list.c index edf3b37..ac7a47f 100644 --- a/rev-list.c +++ b/rev-list.c @@ -28,7 +28,7 @@ static const char rev_list_usage[] = " --merge-order [ --show-breaks ]\n" " --topo-order"; -static int dense = 0; +static int dense = 1; static int unpacked = 0; static int bisect_list = 0; static int tag_objects = 0; @@ -619,7 +619,7 @@ static struct commit *get_commit_referen struct object *object; if (get_sha1(name, sha1)) - usage(rev_list_usage); + return NULL; object = parse_object(sha1); if (!object) die("bad object %s", name); @@ -793,12 +793,12 @@ int main(int argc, const char **argv) dense = 1; continue; } + if (!strcmp(arg, "--sparse")) { + dense = 0; + continue; + } if (!strcmp(arg, "--")) { - paths = get_pathspec(prefix, argv + i + 1); - if (paths) { - limited = 1; - diff_tree_setup_paths(paths); - } + i++; break; } @@ -830,9 +830,20 @@ int main(int argc, const char **argv) limited = 1; } commit = get_commit_reference(arg, flags); + if (!commit) + break; handle_one_commit(commit, &list); } + if (!list) + usage(rev_list_usage); + + paths = get_pathspec(prefix, argv + i); + if (paths) { + limited = 1; + diff_tree_setup_paths(paths); + } + save_commit_buffer = verbose_header; track_object_refs = 0; diff --git a/rev-parse.c b/rev-parse.c index 243f89f..adfc68c 100644 --- a/rev-parse.c +++ b/rev-parse.c @@ -153,6 +153,7 @@ static void show_datestring(const char * static void show_file(const char *arg) { + show_default(); if ((filter & (DO_NONFLAGS|DO_NOREV)) == (DO_NONFLAGS|DO_NOREV)) show(arg); } @@ -174,7 +175,6 @@ int main(int argc, char **argv) if (*arg == '-') { if (!strcmp(arg, "--")) { as_is = 1; - show_default(); /* Pass on the "--" if we show anything but files.. */ if (filter & (DO_FLAGS | DO_REVS)) show_file(arg); ``` ## Linus Torvalds, 2005-10-25 22:29 Subject: Re: git-rev-list: make --dense the default (and introduce "--sparse") Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0510251525540.10477%40g5.osdl.org In-Reply-To: ``` On Tue, 25 Oct 2005, Linus Torvalds wrote: > > This actually does three things: > > - make "--dense" the default for git-rev-list. Since dense is a no-op if > no filenames are given, this doesn't actually change any historical > behaviour, but it's logically the right default (if we want to prune on > filenames, do it fully. The sparse "merge-only" thing may be useful, > but it's not what you'd normally expect) > > - make "git-rev-parse" show the default revision control before it shows > any pathnames. Btw, if it wasn't obvious, this has some subtle (and very nice) side effects. For example, try this: git log git-fetch.sh and notice how "git log" automagically became able to give per-file logs. With _zero_ changes to git-log.sh itself ;) And same old rev-confusion-avoidance: git log -- gitk to make clear that you want to get the log of "gitk the file", rather than "gitk the branch". Magic. Linus ``` ## Junio C Hamano, 2005-10-26 09:17 Subject: Re: git-rev-list: make --dense the default (and introduce "--sparse") Message-ID: <7vwtk08wnx.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vwtk08wnx.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Linus Torvalds writes: > On Tue, 25 Oct 2005, Linus Torvalds wrote: >> >> This actually does three things: >> >> - make "--dense" the default for git-rev-list... Heads up. I have not looked closely into what exactly, but the fourth thing this does might be to break git-send-pack. I usually use the tip of "pu" myself, but for tonight, I am excluding the fetch-pack/upload-pack changes from Johannes when building git for my own use, and using somewhere in the middle of "pu" branch. With this "--dense default" patch, git-send-pack seems to send too few objects. With this patch reverted, git-send-pack seems to work again. + [build] Revert "git-rev-list: make --dense the default (and introduce "--sparse")" ++ [pu^] Merge branch 'js-fat' ++ [pu^^2] Test in git-init-db if the filemode can be trusted ++ [pu~2] Merge branches 'cache-pack', 'lazy-subdir' and 'lt-dense' ++ [pu~2^4] git-rev-list: make --dense the default (and introduce "--sparse") ++ [pu~2^3] Create object subdirectories on demand (phase II) ++ [pu~2^2] Allow caching of generated pack for full cloning. +++ [master] upload-pack: tighten request validation. I'll take a look at the issue in the morning unless somebody else beats me to it. ``` ## Linus Torvalds, 2005-10-26 15:18 Subject: Re: git-rev-list: make --dense the default (and introduce "--sparse") Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0510260757350.10477%40g5.osdl.org In-Reply-To: <7vwtk08wnx.fsf@assigned-by-dhcp.cox.net> ``` On Wed, 26 Oct 2005, Junio C Hamano wrote: > > I have not looked closely into what exactly, but the fourth > thing this does might be to break git-send-pack. Ack. And I see why. What happens is that the new logic decides that if it can't look up a commit reference (ie "get_commit_reference()" returns NULL), the thing must be a pathname. Fair enough. But wrong. The thing is, it may be a perfectly fine ref that _isn't_ a commit. In git, you have a tag that points to your PGP key, and in the kernel, I have a tag that points to a tree (and a direct ref that points to that tree too, for that matter). So the rule is (as for all the other programs that mix revs and pathnames) not that we only accept commit references, but _any_ valid object ref. If the object then isn't a commit ref, git-rev-list will either ignore it, or add it to the list of non-commit objects (if using "--objects"). The solution is to move the "get_sha1()" out of get_commit_reference(), and into the callers. In fact, we already _have_ the SHA1 in the case of the handle_all() loop, since for_each_ref() will have done it for us, so this is the correct thing to do anyway. This patch (on top of the original one) does exactly that. Linus ---- diff --git a/rev-list.c b/rev-list.c index ac7a47f..2b82b8a 100644 --- a/rev-list.c +++ b/rev-list.c @@ -613,13 +613,10 @@ static void add_pending_object(struct ob add_object(obj, &pending_objects, name); } -static struct commit *get_commit_reference(const char *name, unsigned int flags) +static struct commit *get_commit_reference(const char *name, const unsigned char *sha1, unsigned int flags) { - unsigned char sha1[20]; struct object *object; - if (get_sha1(name, sha1)) - return NULL; object = parse_object(sha1); if (!object) die("bad object %s", name); @@ -697,7 +694,7 @@ static struct commit_list **global_lst; static int include_one_commit(const char *path, const unsigned char *sha1) { - struct commit *com = get_commit_reference(path, 0); + struct commit *com = get_commit_reference(path, sha1, 0); handle_one_commit(com, global_lst); return 0; } @@ -720,6 +717,7 @@ int main(int argc, const char **argv) const char *arg = argv[i]; char *dotdot; struct commit *commit; + unsigned char sha1[20]; if (!strncmp(arg, "--max-count=", 12)) { max_count = atoi(arg + 12); @@ -808,15 +806,19 @@ int main(int argc, const char **argv) flags = 0; dotdot = strstr(arg, ".."); if (dotdot) { + unsigned char from_sha1[20]; char *next = dotdot + 2; - struct commit *exclude = NULL; - struct commit *include = NULL; *dotdot = 0; if (!*next) next = "HEAD"; - exclude = get_commit_reference(arg, UNINTERESTING); - include = get_commit_reference(next, 0); - if (exclude && include) { + if (!get_sha1(arg, from_sha1) && !get_sha1(next, sha1)) { + struct commit *exclude; + struct commit *include; + + exclude = get_commit_reference(arg, from_sha1, UNINTERESTING); + include = get_commit_reference(next, sha1, 0); + if (!exclude || !include) + die("Invalid revision range %s..%s", arg, next); limited = 1; handle_one_commit(exclude, &list); handle_one_commit(include, &list); @@ -829,9 +831,9 @@ int main(int argc, const char **argv) arg++; limited = 1; } - commit = get_commit_reference(arg, flags); - if (!commit) + if (get_sha1(arg, sha1) < 0) break; + commit = get_commit_reference(arg, sha1, flags); handle_one_commit(commit, &list); } ``` ## Junio C Hamano, 2005-10-26 20:03 Subject: Re: git-rev-list: make --dense the default (and introduce "--sparse") Message-ID: <7vvezkcafq.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vvezkcafq.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Linus Torvalds writes: > On Wed, 26 Oct 2005, Junio C Hamano wrote: >> >> I have not looked closely into what exactly, but the fourth >> thing this does might be to break git-send-pack. > > Ack. And I see why. > > > This patch (on top of the original one) does exactly that. Thanks, this fixes it. ```