Re: [PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits
Johannes Schindelinwrites: >> > Given that the test_expect_* functions evaluate the code, it makes me >> > wonder whether those `return` statements really return appropriately, or >> > one call level too low. >> >> The test_expect functions eval the actual snippets inside a dummy >> function. This is intentional exactly to allow them to call "return" at >> will. > > Tricksy. And a bit unintuitive for script kings such as myself, but okay. Exactly. The hidden assumption on the way how "return" interacts with the way we use "eval" is the reason why I said "Ugly? Yes, Correct? Questionable" in the first place.
Re: [PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits
On Wed, Nov 1, 2017 at 3:39 PM, Johannes Schindelinwrote: >> not ok 1 - witty title >> >> That is all we want to care about here? > > We care about the loop body being executed successfully *each time*. A > better counter example: Good point. I'll use return in that case. Thanks! Stefan
Re: [PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits
Hi Stefan, On Wed, 1 Nov 2017, Stefan Beller wrote: > We do not care about the internal state, aborting early, we rather > care only if the whole loop body was executed. Running the test > > test_expect_success 'witty title' ' > for a in 1 2 3; do echo $a && false; done && echo done > ' > > not ok 1 - witty title > > That is all we want to care about here? We care about the loop body being executed successfully *each time*. A better counter example: $ for a in 1 2 3; do echo $a && test 2 != $a || false; done && echo done 1 2 3 done (even if the second of three runs failed) Ciao, Dscho
Re: [PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits
Hi, On Wed, 1 Nov 2017, Jeff King wrote: > On Wed, Nov 01, 2017 at 10:36:02PM +0100, Johannes Schindelin wrote: > > > On Wed, 1 Nov 2017, Junio C Hamano wrote: > > > > > On Wed, Nov 1, 2017 at 9:26 PM, Johannes Schindelin > > >wrote: > > > > > > > > ... > > > > ( > > > > for x in four three > > > > do > > > > git rm $x && > > > > git commit -m "remote $x" || > > > > exit > > > > done > > > > ) && > > > > test 0 -eq $? && > > > > ... > > > > > > > > Ugly? Yes. Correct? Also yes. > > > > > > I think returning non-zero with "return" is how other tests avoid an > > > extra level of subshell. > > > Ugly? Yes. Correct? Questionable but it seems to work for those who > > > wrote them ;-) > > > > Given that the test_expect_* functions evaluate the code, it makes me > > wonder whether those `return` statements really return appropriately, or > > one call level too low. > > The test_expect functions eval the actual snippets inside a dummy > function. This is intentional exactly to allow them to call "return" at > will. Tricksy. And a bit unintuitive for script kings such as myself, but okay. Ciao, Dscho
Re: [PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits
>> I think I'll go without the extra subshell and with s/return 1/false/ as >> the exact value doesn't matter. > > You mean > > for ... > do > xyz || > false > done Yes, I do. > ? That does not work. Try this: > > for a in 1 2 3; do echo $a && false; done && echo done ... && echo $? 1 > While it does not print `done`, it will print all of 1, 2 and 3. We do not care about the internal state, aborting early, we rather care only if the whole loop body was executed. Running the test test_expect_success 'witty title' ' for a in 1 2 3; do echo $a && false; done && echo done ' not ok 1 - witty title That is all we want to care about here? Otherwise I may still have a misunderstanding. Thanks, Stefan
Re: [PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits
Hi Stefan, On Wed, 1 Nov 2017, Stefan Beller wrote: > On Wed, Nov 1, 2017 at 5:37 AM, Junio C Hamanowrote: > > On Wed, Nov 1, 2017 at 9:26 PM, Johannes Schindelin > > wrote: > >> > >> ... > >> ( > >> for x in four three > >> do > >> git rm $x && > >> git commit -m "remote $x" || > >> exit > >> done > >> ) && > >> test 0 -eq $? && > >> ... > >> > >> Ugly? Yes. Correct? Also yes. > > > > I think returning non-zero with "return" is how other tests avoid an > > extra level of subshell. > > Ugly? Yes. Correct? Questionable but it seems to work for those who > > wrote them ;-) > > I think I'll go without the extra subshell and with s/return 1/false/ as > the exact value doesn't matter. You mean for ... do xyz || false done ? That does not work. Try this: for a in 1 2 3; do echo $a && false; done && echo done While it does not print `done`, it will print all of 1, 2 and 3. Ciao, Dscho
Re: [PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits
On Wed, Nov 01, 2017 at 10:36:02PM +0100, Johannes Schindelin wrote: > Hi Junio, > > On Wed, 1 Nov 2017, Junio C Hamano wrote: > > > On Wed, Nov 1, 2017 at 9:26 PM, Johannes Schindelin > >wrote: > > > > > > ... > > > ( > > > for x in four three > > > do > > > git rm $x && > > > git commit -m "remote $x" || > > > exit > > > done > > > ) && > > > test 0 -eq $? && > > > ... > > > > > > Ugly? Yes. Correct? Also yes. > > > > I think returning non-zero with "return" is how other tests avoid an > > extra level of subshell. > > Ugly? Yes. Correct? Questionable but it seems to work for those who > > wrote them ;-) > > Given that the test_expect_* functions evaluate the code, it makes me > wonder whether those `return` statements really return appropriately, or > one call level too low. The test_expect functions eval the actual snippets inside a dummy function. This is intentional exactly to allow them to call "return" at will. -Peff
Re: [PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits
Hi Junio, On Wed, 1 Nov 2017, Junio C Hamano wrote: > On Wed, Nov 1, 2017 at 9:26 PM, Johannes Schindelin >wrote: > > > > ... > > ( > > for x in four three > > do > > git rm $x && > > git commit -m "remote $x" || > > exit > > done > > ) && > > test 0 -eq $? && > > ... > > > > Ugly? Yes. Correct? Also yes. > > I think returning non-zero with "return" is how other tests avoid an > extra level of subshell. > Ugly? Yes. Correct? Questionable but it seems to work for those who > wrote them ;-) Given that the test_expect_* functions evaluate the code, it makes me wonder whether those `return` statements really return appropriately, or one call level too low. Ciao, Dscho
Re: [PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits
On Wed, Nov 1, 2017 at 5:37 AM, Junio C Hamanowrote: > On Wed, Nov 1, 2017 at 9:26 PM, Johannes Schindelin > wrote: >> >> ... >> ( >> for x in four three >> do >> git rm $x && >> git commit -m "remote $x" || >> exit >> done >> ) && >> test 0 -eq $? && >> ... >> >> Ugly? Yes. Correct? Also yes. > > I think returning non-zero with "return" is how other tests avoid an > extra level of subshell. > Ugly? Yes. Correct? Questionable but it seems to work for those who > wrote them ;-) I think I'll go without the extra subshell and with s/return 1/false/ as the exact value doesn't matter. Thanks for hinting at correct implementations
Re: [PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits
On Wed, Nov 1, 2017 at 9:26 PM, Johannes Schindelinwrote: > > ... > ( > for x in four three > do > git rm $x && > git commit -m "remote $x" || > exit > done > ) && > test 0 -eq $? && > ... > > Ugly? Yes. Correct? Also yes. I think returning non-zero with "return" is how other tests avoid an extra level of subshell. Ugly? Yes. Correct? Questionable but it seems to work for those who wrote them ;-)
Re: [PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits
Hi, On Wed, 1 Nov 2017, Junio C Hamano wrote: > Stefan Bellerwrites: > > > diff --git a/t/t6100-rev-list-in-order.sh b/t/t6100-rev-list-in-order.sh > > new file mode 100755 > > index 00..651666979b > > --- /dev/null > > +++ b/t/t6100-rev-list-in-order.sh > > @@ -0,0 +1,46 @@ > > +#!/bin/sh > > + > > +test_description='miscellaneous rev-list tests' > > + > > +. ./test-lib.sh > > + > > + > > +test_expect_success 'setup' ' > > + for x in one two three four > > + do > > + echo $x >$x && > > + git add $x && > > + git commit -m "add file $x" > > + done && > > + for x in four three > > + do > > + git rm $x && > > + git commit -m "remove $x" > > + done && > > When "git commit -m 'remove four'" fails, this loop would still > continue, so the &&-chain in "done &&" would still be rendered > ineffetive. ... so the proper fix is to append an "|| break", as in: ... for x in four three do git rm $x && git commit -m "remote $x" || break done && ... But that is still incorrect, as the "break" statement will succeed, breaking (pun intended) the && chain in a different way. The canonical way to do it is to wrap the loop in a subshell, *and also* test $? (because `(exit 1) && echo something` still prints `something`): ... ( for x in four three do git rm $x && git commit -m "remote $x" || exit done ) && test 0 -eq $? && ... Ugly? Yes. Correct? Also yes. Ciao, Dscho
Re: [PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits
Stefan Bellerwrites: > diff --git a/t/t6100-rev-list-in-order.sh b/t/t6100-rev-list-in-order.sh > new file mode 100755 > index 00..651666979b > --- /dev/null > +++ b/t/t6100-rev-list-in-order.sh > @@ -0,0 +1,46 @@ > +#!/bin/sh > + > +test_description='miscellaneous rev-list tests' > + > +. ./test-lib.sh > + > + > +test_expect_success 'setup' ' > + for x in one two three four > + do > + echo $x >$x && > + git add $x && > + git commit -m "add file $x" > + done && > + for x in four three > + do > + git rm $x && > + git commit -m "remove $x" > + done && When "git commit -m 'remove four'" fails, this loop would still continue, so the &&-chain in "done &&" would still be rendered ineffetive. > + git rev-list --in-commit-order --objects HEAD >actual.raw && > + cut -c 1-40 >actual + > + git cat-file --batch-check="%(objectname)" >expect.raw <<-\EOF && > + HEAD^{commit} > + HEAD^{tree} > + HEAD^{tree}:one > + HEAD^{tree}:two > + HEAD~1^{commit} > + HEAD~1^{tree} > + HEAD~1^{tree}:three > + HEAD~2^{commit} > + HEAD~2^{tree} > + HEAD~2^{tree}:four > + HEAD~3^{commit} > + # HEAD~3^{tree} skipped > + HEAD~4^{commit} > + # HEAD~4^{tree} skipped > + HEAD~5^{commit} > + HEAD~5^{tree} > + EOF > + grep -v "#" >expect + > + test_cmp expect actual > +' > + > +test_done
[PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits
The functionality to list tree objects in the order they were seen while traversing the commits will be used in the next commit, where we teach `git describe` to describe not only commits, but trees and blobs, too. Signed-off-by: Stefan Beller--- Documentation/rev-list-options.txt | 5 + list-objects.c | 2 ++ revision.c | 2 ++ revision.h | 3 ++- t/t6100-rev-list-in-order.sh | 46 ++ 5 files changed, 57 insertions(+), 1 deletion(-) create mode 100755 t/t6100-rev-list-in-order.sh diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index 13501e1556..9066e0c777 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -686,6 +686,11 @@ ifdef::git-rev-list[] all object IDs which I need to download if I have the commit object _bar_ but not _foo_''. +--in-commit-order:: + Print tree and blob ids in order of the commits. The tree + and blob ids are printed after they are first referenced + by a commit. + --objects-edge:: Similar to `--objects`, but also print the IDs of excluded commits prefixed with a ``-'' character. This is used by diff --git a/list-objects.c b/list-objects.c index ef9dbe8f92..03438e5a8b 100644 --- a/list-objects.c +++ b/list-objects.c @@ -239,6 +239,8 @@ void traverse_commit_list(struct rev_info *revs, if (commit->tree) add_pending_tree(revs, commit->tree); show_commit(commit, data); + if (revs->tree_blobs_in_commit_order) + traverse_trees_and_blobs(revs, , show_object, data); } traverse_trees_and_blobs(revs, , show_object, data); diff --git a/revision.c b/revision.c index d167223e69..9329d4ebbf 100644 --- a/revision.c +++ b/revision.c @@ -1845,6 +1845,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg revs->dense = 0; } else if (!strcmp(arg, "--show-all")) { revs->show_all = 1; + } else if (!strcmp(arg, "--in-commit-order")) { + revs->tree_blobs_in_commit_order = 1; } else if (!strcmp(arg, "--remove-empty")) { revs->remove_empty_trees = 1; } else if (!strcmp(arg, "--merges")) { diff --git a/revision.h b/revision.h index 54761200ad..86985d68aa 100644 --- a/revision.h +++ b/revision.h @@ -121,7 +121,8 @@ struct rev_info { bisect:1, ancestry_path:1, first_parent_only:1, - line_level_traverse:1; + line_level_traverse:1, + tree_blobs_in_commit_order:1; /* Diff flags */ unsigned intdiff:1, diff --git a/t/t6100-rev-list-in-order.sh b/t/t6100-rev-list-in-order.sh new file mode 100755 index 00..651666979b --- /dev/null +++ b/t/t6100-rev-list-in-order.sh @@ -0,0 +1,46 @@ +#!/bin/sh + +test_description='miscellaneous rev-list tests' + +. ./test-lib.sh + + +test_expect_success 'setup' ' + for x in one two three four + do + echo $x >$x && + git add $x && + git commit -m "add file $x" + done && + for x in four three + do + git rm $x && + git commit -m "remove $x" + done && + git rev-list --in-commit-order --objects HEAD >actual.raw && + cut -c 1-40 >actual expect.raw <<-\EOF && + HEAD^{commit} + HEAD^{tree} + HEAD^{tree}:one + HEAD^{tree}:two + HEAD~1^{commit} + HEAD~1^{tree} + HEAD~1^{tree}:three + HEAD~2^{commit} + HEAD~2^{tree} + HEAD~2^{tree}:four + HEAD~3^{commit} + # HEAD~3^{tree} skipped + HEAD~4^{commit} + # HEAD~4^{tree} skipped + HEAD~5^{commit} + HEAD~5^{tree} + EOF + grep -v "#" >expect