[PATCH 3/3] t0030: reformat style
Signed-off-by: Stefan Beller --- t/t0030-stripspace.sh | 525 -- 1 file changed, 254 insertions(+), 271 deletions(-) diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh index 5ce47e8af51..c2281a39b58 100755 --- a/t/t0030-stripspace.sh +++ b/t/t0030-stripspace.sh @@ -12,379 +12,362 @@ s40='' sss="$s40$s40$s40$s40$s40$s40$s40$s40$s40$s40" # 400 ttt="$t40$t40$t40$t40$t40$t40$t40$t40$t40$t40" # 400 -test_expect_success \ -'long lines without spaces should be unchanged' ' -echo "$ttt" >expect && -git stripspace actual && -test_cmp expect actual && - -echo "$ttt$ttt" >expect && -git stripspace actual && -test_cmp expect actual && - -echo "$ttt$ttt$ttt" >expect && -git stripspace actual && -test_cmp expect actual && - -echo "$ttt$ttt$ttt$ttt" >expect && -git stripspace actual && -test_cmp expect actual +test_expect_success 'long lines without spaces should be unchanged' ' + echo "$ttt" >expect && + git stripspace actual && + test_cmp expect actual && + + echo "$ttt$ttt" >expect && + git stripspace actual && + test_cmp expect actual && + + echo "$ttt$ttt$ttt" >expect && + git stripspace actual && + test_cmp expect actual && + + echo "$ttt$ttt$ttt$ttt" >expect && + git stripspace actual && + test_cmp expect actual ' -test_expect_success \ -'lines with spaces at the beginning should be unchanged' ' -echo "$sss$ttt" >expect && -git stripspace actual && -test_cmp expect actual && +test_expect_success 'lines with spaces at the beginning should be unchanged' ' + echo "$sss$ttt" >expect && + git stripspace actual && + test_cmp expect actual && -echo "$sss$sss$ttt" >expect && -git stripspace actual && -test_cmp expect actual && + echo "$sss$sss$ttt" >expect && + git stripspace actual && + test_cmp expect actual && -echo "$sss$sss$sss$ttt" >expect && -git stripspace actual && -test_cmp expect actual + echo "$sss$sss$sss$ttt" >expect && + git stripspace actual && + test_cmp expect actual ' -test_expect_success \ -'lines with intermediate spaces should be unchanged' ' -echo "$ttt$sss$ttt" >expect && -git stripspace actual && -test_cmp expect actual && +test_expect_success 'lines with intermediate spaces should be unchanged' ' + echo "$ttt$sss$ttt" >expect && + git stripspace actual && + test_cmp expect actual && -echo "$ttt$sss$sss$ttt" >expect && -git stripspace actual && -test_cmp expect actual + echo "$ttt$sss$sss$ttt" >expect && + git stripspace actual && + test_cmp expect actual ' -test_expect_success \ -'consecutive blank lines should be unified' ' -printf "$ttt\n\n$ttt\n" > expect && -printf "$ttt\n\n\n\n\n$ttt\n" | git stripspace >actual && -test_cmp expect actual && +test_expect_success 'consecutive blank lines should be unified' ' + printf "$ttt\n\n$ttt\n" > expect && + printf "$ttt\n\n\n\n\n$ttt\n" | git stripspace >actual && + test_cmp expect actual && -printf "$ttt$ttt\n\n$ttt\n" > expect && -printf "$ttt$ttt\n\n\n\n\n$ttt\n" | git stripspace >actual && -test_cmp expect actual && + printf "$ttt$ttt\n\n$ttt\n" > expect && + printf "$ttt$ttt\n\n\n\n\n$ttt\n" | git stripspace >actual && + test_cmp expect actual && -printf "$ttt$ttt$ttt\n\n$ttt\n" > expect && -printf "$ttt$ttt$ttt\n\n\n\n\n$ttt\n" | git stripspace >actual && -test_cmp expect actual && + printf "$ttt$ttt$ttt\n\n$ttt\n" > expect && + printf "$ttt$ttt$ttt\n\n\n\n\n$ttt\n" | git stripspace >actual && + test_cmp expect actual && -printf "$ttt\n\n$ttt\n" > expect && -printf "$ttt\n\n\n\n\n$ttt\n" | git stripspace >actual && -test_cmp expect actual && + printf "$ttt\n\n$ttt\n" > expect && + printf "$ttt\n\n\n\n\n$ttt\n" | git stripspace >actual && + test_cmp expect actual && -printf "$ttt\n\n$ttt$ttt\n" > expect && -printf "$ttt\n\n\n\n\n$ttt$ttt\n" | git stripspace >actual && -test_cmp expect actual && + printf "$ttt\n\n$ttt$ttt\n" > expect && + printf "$ttt\n\n\n\n\n$ttt$ttt\n" | git stripspace >actual && + test_cmp expect actual && -printf "$ttt\n\n$ttt$ttt$ttt\n" > expect && -printf "$ttt\n\n\n\n\n$ttt$ttt$ttt\n" | git stripspace >actual && -test_cmp expect actual && + printf "$ttt\n\n$ttt$ttt$ttt\n" > expect && + printf "$ttt\n\n\n\n\n$ttt$ttt$ttt\n" | git stripspace >actual && + test_cmp expect actual && -printf "$ttt\n\n$ttt\n" > expect && -printf "$ttt\n\t\n \n\n \t\t\n$ttt\n" | git stripspace >actual && -test_cmp expect actual && + printf "$ttt\n\n$ttt\n" > expect && + printf "$ttt\n\t\n \n\n \t\t\n$ttt\n" | git stripspace >actual && +
[PATCH 2/3] t7004: reformat style
Signed-off-by: Stefan Beller --- t/t7004-tag.sh | 149 +++-- 1 file changed, 56 insertions(+), 93 deletions(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 0b01862c23a..03a96b7f79e 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -144,26 +144,25 @@ test_expect_success 'listing a tag using a matching pattern should succeed' \ test_expect_success 'listing a tag with --ignore-case' \ 'test $(git tag -l --ignore-case MYTAG) = mytag' -test_expect_success \ - 'listing a tag using a matching pattern should output that tag' \ - 'test $(git tag -l mytag) = mytag' +test_expect_success 'listing a tag using a matching pattern should output that tag' ' + test $(git tag -l mytag) = mytag +' -test_expect_success \ - 'listing tags using a non-matching pattern should succeed' \ - 'git tag -l xxx' +test_expect_success 'listing tags using a non-matching pattern should succeed' ' + git tag -l xxx +' -test_expect_success \ - 'listing tags using a non-matching pattern should output nothing' \ - 'test $(git tag -l xxx | wc -l) -eq 0' +test_expect_success 'listing tags using a non-matching pattern should output nothing' ' + test $(git tag -l xxx | wc -l) -eq 0 +' # special cases for creating tags: -test_expect_success \ - 'trying to create a tag with the name of one existing should fail' \ - 'test_must_fail git tag mytag' +test_expect_success 'trying to create a tag with the name of one existing should fail' ' + test_must_fail git tag mytag +' -test_expect_success \ - 'trying to create a tag with a non-valid name should fail' ' +test_expect_success 'trying to create a tag with a non-valid name should fail' ' test $(git tag -l | wc -l) -eq 1 && test_must_fail git tag "" && test_must_fail git tag .othertag && @@ -201,15 +200,13 @@ cat >expect < actual && test_cmp expect actual && git tag -d && git tag -l > actual && test_cmp expect actual ' -test_expect_success \ - 'deleting two existing tags in one command should succeed' ' +test_expect_success 'deleting two existing tags in one command should succeed' ' tag_exists mytag && tag_exists myhead && git tag -d mytag myhead && @@ -217,15 +214,13 @@ test_expect_success \ ! tag_exists myhead ' -test_expect_success \ - 'creating a tag with the name of another deleted one should succeed' ' +test_expect_success 'creating a tag with the name of another deleted one should succeed' ' ! tag_exists mytag && git tag mytag && tag_exists mytag ' -test_expect_success \ - 'trying to delete two tags, existing and not, should fail in the 2nd' ' +test_expect_success 'trying to delete two tags, existing and not, should fail in the 2nd' ' tag_exists mytag && ! tag_exists myhead && test_must_fail git tag -d mytag anothertag && @@ -270,8 +265,7 @@ a1 aa1 cba EOF -test_expect_success \ - 'listing tags with substring as pattern must print those matching' ' +test_expect_success 'listing tags with substring as pattern must print those matching' ' rm *a* && git tag -l "*a*" > current && test_cmp expect current @@ -281,8 +275,7 @@ cat >expect < actual && test_cmp expect actual ' @@ -291,8 +284,7 @@ cat >expect < actual && test_cmp expect actual ' @@ -300,8 +292,7 @@ test_expect_success \ cat >expect < actual && test_cmp expect actual ' @@ -309,8 +300,7 @@ test_expect_success \ cat >expect < actual && test_cmp expect actual ' @@ -319,14 +309,12 @@ cat >expect < actual && test_cmp expect actual ' -test_expect_success \ - 'listing tags using v.* should print nothing because none have v.' ' +test_expect_success 'listing tags using v.* should print nothing because none have v.' ' git tag -l "v.*" > actual && test_must_be_empty actual ' @@ -337,8 +325,7 @@ v1.0 v1.0.1 v1.1.3 EOF -test_expect_success \ - 'listing tags using v* should print only those having v' ' +test_expect_success 'listing tags using v* should print only those having v' ' git tag -l "v*" > actual && test_cmp expect actual ' @@ -404,8 +391,7 @@ EOF # creating and verifying lightweight tags: -test_expect_success \ - 'a non-annotated tag created without parameters should point to HEAD' ' +test_expect_success 'a non-annotated tag created without parameters should point to HEAD' ' git tag non-annotated-tag && test $(git cat-file -t non-annotated-tag) = commit && test $(git rev-parse non-annotated-tag) = $(git rev-parse HEAD) @@ -414,13 +400,13 @@ test_expect_success \ test_expect_success 'trying to verify an unknown tag should fail' \ 'test_must_fail git tag -v unknown-tag' -test_expect_success \ - 'trying to verify a non-annotated and non-signed tag should fail'
[PATCH 0/3] bring some tests to newer style.
The old formatting style is a real hindrance of getting people up to speed
contributing as they use existing code as an example and follow that style.
So let's get rid of the old style and reformat it in our current style.
This was reported off list by Derrick and Jeff as both of them followed
outdated formatting style for their first patches only to have a long
discussion on the mailing list on style.
The tests changed are not modified in origin/master..origin/pu,
and they were changed using a hacky script:
--8<--
#!/usr/bin/python
import sys
def applyformat(fname):
with open(fname, 'r') as f:
lines = f.readlines()
state = "lookingforstart"
outlines = []
for line in lines:
if state == "lookingforstart":
if line == "test_expect_success \\\n":
print "AHA!"
state = "firstlinefound"
else:
outlines += [line]
elif state == "firstlinefound":
l = line.strip()
must_strip = False
if l.endswith("\\"):
l = l[:-1]
must_strip=True
if l.endswith("'"):
l = l[:-1].strip()
if l.startswith("'"):
line = "test_expect_success " + l + "
'\n"
outlines += [line]
state = "re-indent-until-done"
else:
print "what?"
exit(1)
elif state == "re-indent-until-done":
l = line.strip()
if must_strip:
if l.startswith("'"):
l = l[1:]
must_strip = False
else:
print "what 1?"
exit(1)
if l.endswith("'"):
l = l[:-1]
state = "lookingforstart"
if len(l):
line = "" + l + "\n"
outlines += [line]
elif state == "lookingforstart":
# skip an empty line before test is done
pass
else:
outlines += ["\n"]
if state == "lookingforstart":
outlines += ["'\n"]
else:
print "what?"
exit(1)
with open(fname, 'w') as f:
f.write(''.join(outlines))
for n in sys.argv[1:]:
print n
applyformat(n)
--8<--
Thanks,
Stefan
Stefan Beller (3):
t7001: reformat to newer style
t7004: reformat style
t0030: reformat style
t/t0030-stripspace.sh | 525 --
t/t7001-mv.sh | 268 ++---
t/t7004-tag.sh| 149 +---
3 files changed, 444 insertions(+), 498 deletions(-)
--
2.19.0.444.g18242da7ef-goog
[PATCH 1/3] t7001: reformat to newer style
The old formatting style is a real hindrance of getting people up to speed contributing as they use existing code as an example and follow that style. So let's get rid of the old style and reformat it in our current style. Reported-by: Derrick Stolee Reported-by: Jeff Hostetler Signed-off-by: Stefan Beller --- t/t7001-mv.sh | 268 +- 1 file changed, 134 insertions(+), 134 deletions(-) diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh index 36b50d0b4c1..2251d24735c 100755 --- a/t/t7001-mv.sh +++ b/t/t7001-mv.sh @@ -3,74 +3,74 @@ test_description='git mv in subdirs' . ./test-lib.sh -test_expect_success \ -'prepare reference tree' \ -'mkdir path0 path1 && - cp "$TEST_DIRECTORY"/../COPYING path0/COPYING && - git add path0/COPYING && - git commit -m add -a' +test_expect_success 'prepare reference tree' ' + mkdir path0 path1 && + cp "$TEST_DIRECTORY"/../COPYING path0/COPYING && + git add path0/COPYING && + git commit -m add -a +' -test_expect_success \ -'moving the file out of subdirectory' \ -'cd path0 && git mv COPYING ../path1/COPYING' +test_expect_success 'moving the file out of subdirectory' ' + cd path0 && git mv COPYING ../path1/COPYING +' # in path0 currently -test_expect_success \ -'commiting the change' \ -'cd .. && git commit -m move-out -a' +test_expect_success 'commiting the change' ' + cd .. && git commit -m move-out -a +' -test_expect_success \ -'checking the commit' \ -'git diff-tree -r -M --name-status HEAD^ HEAD >actual && -grep "^R100..*path0/COPYING..*path1/COPYING" actual' +test_expect_success 'checking the commit' ' + git diff-tree -r -M --name-status HEAD^ HEAD >actual && + grep "^R100..*path0/COPYING..*path1/COPYING" actual +' -test_expect_success \ -'moving the file back into subdirectory' \ -'cd path0 && git mv ../path1/COPYING COPYING' +test_expect_success 'moving the file back into subdirectory' ' + cd path0 && git mv ../path1/COPYING COPYING +' # in path0 currently -test_expect_success \ -'commiting the change' \ -'cd .. && git commit -m move-in -a' - -test_expect_success \ -'checking the commit' \ -'git diff-tree -r -M --name-status HEAD^ HEAD >actual && -grep "^R100..*path1/COPYING..*path0/COPYING" actual' - -test_expect_success \ -'mv --dry-run does not move file' \ -'git mv -n path0/COPYING MOVED && - test -f path0/COPYING && - test ! -f MOVED' - -test_expect_success \ -'checking -k on non-existing file' \ -'git mv -k idontexist path0' - -test_expect_success \ -'checking -k on untracked file' \ -'touch untracked1 && - git mv -k untracked1 path0 && - test -f untracked1 && - test ! -f path0/untracked1' - -test_expect_success \ -'checking -k on multiple untracked files' \ -'touch untracked2 && - git mv -k untracked1 untracked2 path0 && - test -f untracked1 && - test -f untracked2 && - test ! -f path0/untracked1 && - test ! -f path0/untracked2' - -test_expect_success \ -'checking -f on untracked file with existing target' \ -'touch path0/untracked1 && - test_must_fail git mv -f untracked1 path0 && - test ! -f .git/index.lock && - test -f untracked1 && - test -f path0/untracked1' +test_expect_success 'commiting the change' ' + cd .. && git commit -m move-in -a +' + +test_expect_success 'checking the commit' ' + git diff-tree -r -M --name-status HEAD^ HEAD >actual && + grep "^R100..*path1/COPYING..*path0/COPYING" actual +' + +test_expect_success 'mv --dry-run does not move file' ' + git mv -n path0/COPYING MOVED && + test -f path0/COPYING && + test ! -f MOVED +' + +test_expect_success 'checking -k on non-existing file' ' + git mv -k idontexist path0 +' + +test_expect_success 'checking -k on untracked file' ' + touch untracked1 && + git mv -k untracked1 path0 && + test -f untracked1 && + test ! -f path0/untracked1 +' + +test_expect_success 'checking -k on multiple untracked files' ' + touch untracked2 && + git mv -k untracked1 untracked2 path0 && + test -f untracked1 && + test -f untracked2 && + test ! -f path0/untracked1 && + test ! -f path0/untracked2 +' + +test_expect_success 'checking -f on untracked file with existing target' ' + touch path0/untracked1 && + test_must_fail git mv -f untracked1 path0 && + test ! -f .git/index.lock && + test -f untracked1 && + test -f path0/untracked1 +' # clean up the mess in case bad things happen rm -f idontexist untracked1 untracked2 \ @@ -78,79 +78,79 @@ rm -f idontexist untracked1 untracked2 \ .git/index.lock rmdir path1 -test_expect_success \ -'moving to absent target with trailing slash' \ -'test_must_fail git mv path0/COPYING no-such-dir/ && - test_must_fail git mv path0/COPYING no-such-dir// && -
Re: [PATCH v3 2/2] commit-reach: fix memory and flag leaks
On Fri, Sep 21, 2018 at 08:05:27AM -0700, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee > > The can_all_from_reach_with_flag() method uses 'assign_flag' as a > value we can use to mark objects temporarily during our commit walk. > The intent is that these flags are removed from all objects before > returning. However, this is not the case. > > The 'from' array could also contain objects that are not commits, and > we mark those objects with 'assign_flag'. Add a loop to the 'cleanup' > section that removes these markers. > > Also, we forgot to free() the memory for 'list', so add that to the > 'cleanup' section. Urgh, ignore most of my response to patch 1, then. I saw there was a patch 2, but thought it was just handling the free(). The flag-clearing here makes perfect sense. -Peff
Re: [PATCH v3 1/2] commit-reach: properly peel tags
On Fri, Sep 21, 2018 at 08:05:26AM -0700, Derrick Stolee via GitGitGadget wrote:
> diff --git a/commit-reach.c b/commit-reach.c
> index 86715c103c..e748414d04 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -544,20 +544,42 @@ int can_all_from_reach_with_flag(struct object_array
> *from,
> {
> struct commit **list = NULL;
> int i;
> + int nr_commits;
> int result = 1;
>
> ALLOC_ARRAY(list, from->nr);
> + nr_commits = 0;
> for (i = 0; i < from->nr; i++) {
> - list[i] = (struct commit *)from->objects[i].item;
> + struct object *from_one = from->objects[i].item;
>
> - if (parse_commit(list[i]) ||
> - list[i]->generation < min_generation)
> - return 0;
> + if (!from_one || from_one->flags & assign_flag)
> + continue;
> +
> + from_one = deref_tag(the_repository, from_one,
> + "a from object", 0);
> + if (!from_one || from_one->type != OBJ_COMMIT) {
> + /* no way to tell if this is reachable by
> + * looking at the ancestry chain alone, so
> + * leave a note to ourselves not to worry about
> + * this object anymore.
> + */
A minor nit, but the original comment you restored here has a style
violation. Might be worth fixing up (but certainly not worth a re-roll
on its own).
> + from->objects[i].item->flags |= assign_flag;
OK, so here we mark the original tag with a flag...
> +
> + list[nr_commits] = (struct commit *)from_one;
> + if (parse_commit(list[nr_commits]) ||
> + list[nr_commits]->generation < min_generation) {
> + result = 0;
> + goto cleanup;
> + }
And we jump to the cleanup here, but...
> @@ -600,7 +622,7 @@ int can_all_from_reach_with_flag(struct object_array
> *from,
> }
>
> cleanup:
> - for (i = 0; i < from->nr; i++) {
> + for (i = 0; i < nr_commits; i++) {
> clear_commit_marks(list[i], RESULT);
> clear_commit_marks(list[i], assign_flag);
This walks over the items in the list array, which don't include the tag
we marked. Did we decide that's OK? I think it's actually how the
original code behaved, too, but it seems funny. At the least we might
want to call it out in the commit message.
Should we also be walking from->objects and clearing the flags from
there (maybe not RESULT, but assign_flag)? It's not clear to me if it's
unintentional for those to be marked after the function leaves or not.
Also, a minor aside, but I think it would be slightly more efficient for
those final two lines to do:
clear_commit_marks(list[i], RESULT | assign_flag);
Of course, that's totally orthogonal to this patch, but it may make you
feel better to offset the other round of clearing I'm suggesting. ;)
> diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
> index eb21103998..08d2ea68e8 100644
> --- a/t/helper/test-reach.c
> +++ b/t/helper/test-reach.c
These bits all looked good to me.
-Peff
Re: Segfault in master due to 4fbcca4eff
On Fri, Sep 21 2018, Jeff King wrote: > On Sat, Sep 22, 2018 at 01:37:17AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> > Thanks, both of you ;-). I was aware of the issue and proposed fix >> > but forgot about it when merging things down to 'master'. Sorry >> > about that. >> >> Just a follow-up question, in your merge commit you just pushed to >> "next" you say: >> >> Recent update broke the reachability algorithm when tags pointing >> at objects that are not commit were involved, which has been fixed. >> >> And in Derrick's commit message it says: >> >> [...]but incorrectly assumed that all objects provided were commits[...] >> >> I just wanted to double check (without having the time to dig myself at >> this point) whether this bug was understood & tested for, or whether the >> case I had was just /also/ fixed for unexpected reasons. >> >> I.e. in my upthread test case I have two annotated tags pointing at >> commits, whereas the merge to "next" says "when tags pointing at objects >> that are not commit were involved", which I I assume means say annotated >> tags pointing at blobs..., but that's not what I had. >> >> Wasn't this just a bug fix that had nothing to do with tags not pointing >> to commits, but just ones where we had the simple case of tags pointing >> to commits, but they just weren't peeled? >> >> I'm hoping for a "Junio skimmed the fix and wrote a merge message that >> wasn't quite accurate" here, but maybe that's not the case and something >> might be missing (e.g. missing test code). > > I think it's a combination of the merge message being slightly > inaccurate, and you slightly misreading it. :) > > I think by "tags pointing", Junio meant "tag refs". Which of course, > often point at tag objects, but can also point at trees, etc. > > But the problem is not limited to tag refs. I think it's a problem with > any "want" that is a non-commit. So really any ref pointing to a > non-commit is a problem. But of course tags are the likely way for that > to happen, since refs/heads is generally limited to commits. > > So in short, yeah, the bug was triggered by fetching any annotated tag. Thanks for clearing that up.
Re: Segfault in master due to 4fbcca4eff
On Sat, Sep 22, 2018 at 01:37:17AM +0200, Ævar Arnfjörð Bjarmason wrote: > > Thanks, both of you ;-). I was aware of the issue and proposed fix > > but forgot about it when merging things down to 'master'. Sorry > > about that. > > Just a follow-up question, in your merge commit you just pushed to > "next" you say: > > Recent update broke the reachability algorithm when tags pointing > at objects that are not commit were involved, which has been fixed. > > And in Derrick's commit message it says: > > [...]but incorrectly assumed that all objects provided were commits[...] > > I just wanted to double check (without having the time to dig myself at > this point) whether this bug was understood & tested for, or whether the > case I had was just /also/ fixed for unexpected reasons. > > I.e. in my upthread test case I have two annotated tags pointing at > commits, whereas the merge to "next" says "when tags pointing at objects > that are not commit were involved", which I I assume means say annotated > tags pointing at blobs..., but that's not what I had. > > Wasn't this just a bug fix that had nothing to do with tags not pointing > to commits, but just ones where we had the simple case of tags pointing > to commits, but they just weren't peeled? > > I'm hoping for a "Junio skimmed the fix and wrote a merge message that > wasn't quite accurate" here, but maybe that's not the case and something > might be missing (e.g. missing test code). I think it's a combination of the merge message being slightly inaccurate, and you slightly misreading it. :) I think by "tags pointing", Junio meant "tag refs". Which of course, often point at tag objects, but can also point at trees, etc. But the problem is not limited to tag refs. I think it's a problem with any "want" that is a non-commit. So really any ref pointing to a non-commit is a problem. But of course tags are the likely way for that to happen, since refs/heads is generally limited to commits. So in short, yeah, the bug was triggered by fetching any annotated tag. -Peff
Re: Very simple popen() code request, ground-shaking functionality openned by it
On Sat, Sep 22, 2018 at 01:30:36AM +0200, Ævar Arnfjörð Bjarmason wrote: > >> This will allow users to free their creativity and provide probably > >> dozens of custom Git progress bars. > > > > I don't personally feel that the existing progress bar is that bad, but > > if anybody wants to pursue this, I think the most sensible path is: > > I don't think it's bad either, but one thing that's really neat about > Sebastian's suggestion is that it's using some UTF-8 terminal ASCII art > to render an actual progress bar. Yeah. I don't care myself, but I'm not opposed to somebody trying to spruce up the in-code bar, as long we can still handle the lowest common denominator cleanly (and remember that includes passing progress bars back over the remote sideband). > > 1. Add a trace_key for sending machine-readable progress output to a > > descriptor or file. E.g., via setting GIT_TRACE_PROGRESS=2 in the > > environment. > > > > 2. Teach the trace code to open a command for piping, so that you > > could do something like GIT_TRACE_PROGRESS='|mygauge'. > > > > That would make your use case work, and I think many other use cases > > would benefit from both of those features independently. > > Yup, that's all sensible, and would be great both for this and other > stuff if we wanted true extensibility for this sort of thing. > > I'll just add that a 3rd thing that would also make sense would be to > add a feature to configure the value of these GIT_TRACE_*=* variables > via the .gitconfig, that's been suggested before (too lazy to dig up a > ML archive reference), and would make this as easy to configure as > Sebastian's suggestion. Heh, I almost included that in my original mail, but didn't want to get into the tangle of secondary concerns. But since you mention it... :) One thing to watch out is that (2) and (3) combined may mean executing arbitrary code specified in the .git/config of an untrusted repository. This is already the case for many commands, but we've specifically tried to avoid it with git-upload-pack, making it safe to "git fetch" out of an untrusted repository. It's almost certain that you'd be able to trigger trace code from it. There are a number of solutions floating around. We already have some upload-pack config which is smart enough to realize when its source is in-repo and handle it appropriately, and we've talked on the list about having some "I don't trust this repo" environment variable that would make Git operate in a more restricted way. I don't think we need to hash out the solution here, but I just want to mention that it's a thing that would have to dealt with before adding those two features. (Actually, I guess you could argue that even reading existing trace specs out of config is potentially dangerous, since you can append to arbitrary files). -Peff
Re: Segfault in master due to 4fbcca4eff
On Fri, Sep 21 2018, Junio C Hamano wrote: > Derrick Stolee writes: > >> On 9/21/2018 10:40 AM, Ævar Arnfjörð Bjarmason wrote: >>> On Fri, Sep 21 2018, Derrick Stolee wrote: >>> This error was reported by Peff [1] and fixed in [2], but as stated [3] I was waiting for more review before sending a v3. I'll send that v3 shortly, responding to the feedback so far. -Stolee [1] https://public-inbox.org/git/0bf9103c-9377-506b-7ad7-e5273d8e9...@gmail.com/T/#u [2] https://public-inbox.org/git/pull.39.git.gitgitgad...@gmail.com/ [3] https://public-inbox.org/git/8d6061de-1654-577c-40c6-211dbd03a...@gmail.com/ >>> Thanks and sorry for the duplicate report. I can confirm that applying >>> the v2 of that fixes the segfault for the test case I posted. >> >> Thanks for the report! You are a good dogfooder. > > Thanks, both of you ;-). I was aware of the issue and proposed fix > but forgot about it when merging things down to 'master'. Sorry > about that. Just a follow-up question, in your merge commit you just pushed to "next" you say: Recent update broke the reachability algorithm when tags pointing at objects that are not commit were involved, which has been fixed. And in Derrick's commit message it says: [...]but incorrectly assumed that all objects provided were commits[...] I just wanted to double check (without having the time to dig myself at this point) whether this bug was understood & tested for, or whether the case I had was just /also/ fixed for unexpected reasons. I.e. in my upthread test case I have two annotated tags pointing at commits, whereas the merge to "next" says "when tags pointing at objects that are not commit were involved", which I I assume means say annotated tags pointing at blobs..., but that's not what I had. Wasn't this just a bug fix that had nothing to do with tags not pointing to commits, but just ones where we had the simple case of tags pointing to commits, but they just weren't peeled? I'm hoping for a "Junio skimmed the fix and wrote a merge message that wasn't quite accurate" here, but maybe that's not the case and something might be missing (e.g. missing test code).
Re: Very simple popen() code request, ground-shaking functionality openned by it
On Fri, Sep 21 2018, Jeff King wrote:
> On Fri, Sep 21, 2018 at 09:34:14AM -0400, Sebastian Gniazdowski wrote:
>
>> Git default progress indicator for clone is very unattractive, IMO. It
>> does its job in providing all the operation details very well, but I
>> bet most of users strongly dream about a gauge box!
>>
>> Have a look at my gauge box constructed as git-stderr pipe script:
>> https://asciinema.org/a/202401
>>
>> The main point of my feature request is: git can add
>> core.progress_pipe option, where e.g. `/usr/local/bin/mygauge' will be
>> set (a script like the one in the asciinema), and then simply do the
>> ground-school-known `popen("/usr/local/bin/mygauge","r+")', and write
>> **unchanged current-progress data** to the pipe, then read from the
>> pipe and forward to `stderr', where the progress normally lands in.
>>
>> This will allow users to free their creativity and provide probably
>> dozens of custom Git progress bars.
>
> I don't personally feel that the existing progress bar is that bad, but
> if anybody wants to pursue this, I think the most sensible path is:
I don't think it's bad either, but one thing that's really neat about
Sebastian's suggestion is that it's using some UTF-8 terminal ASCII art
to render an actual progress bar.
It would be very cool if we had some "we can use non-ASCII for such art"
core.* option, similar to how we pushed the boundaries with what was
acceptable with pagers and having colors on by default (and perhaps we
could even turn such a non-ASCII mode on by default eventually...).
Duy's
https://public-inbox.org/git/20180920161928.ga13...@duynguyen.home/ is
another recent thing that reminded me of this, i.e. that suggested
"\\|/-" spinner could be made much neater with non-ASCII.
> 1. Add a trace_key for sending machine-readable progress output to a
> descriptor or file. E.g., via setting GIT_TRACE_PROGRESS=2 in the
> environment.
>
> 2. Teach the trace code to open a command for piping, so that you
> could do something like GIT_TRACE_PROGRESS='|mygauge'.
>
> That would make your use case work, and I think many other use cases
> would benefit from both of those features independently.
Yup, that's all sensible, and would be great both for this and other
stuff if we wanted true extensibility for this sort of thing.
I'll just add that a 3rd thing that would also make sense would be to
add a feature to configure the value of these GIT_TRACE_*=* variables
via the .gitconfig, that's been suggested before (too lazy to dig up a
ML archive reference), and would make this as easy to configure as
Sebastian's suggestion.
[PATCH] receive-pack: update comment with check_everything_connected
That function is now called "check_connected()", but we forgot to update this comment in 7043c7071c (check_everything_connected: use a struct with named options, 2016-07-15). Signed-off-by: Jeff King --- Just a minor annoyance I happened to notice while discussing in another thread. I notice both of us still called it check-everything-connected in our emails; old habits die hard, I suppose. ;) builtin/receive-pack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index a3bb13af10..3b7432c8e4 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1834,7 +1834,7 @@ static void prepare_shallow_update(struct command *commands, /* * keep hooks happy by forcing a temporary shallow file via * env variable because we can't add --shallow-file to every -* command. check_everything_connected() will be done with +* command. check_connected() will be done with * true .git/shallow though. */ setenv(GIT_SHALLOW_FILE_ENVIRONMENT, alt_shallow_file, 1); -- 2.19.0.762.gbd1f43ccbf
Re: credential..helper with partial url path
On Fri, Sep 21, 2018 at 04:56:20PM +, Zych, David M wrote:
> Suppose I need to use different credential.helper values for different
> repositories on the same HTTPS host. Ideally I would like to be able to
> write this logic using a partial URL path prefix, for example in
> ~/.gitconfig
>
> [credential "https://example.com/prefix1/foo.git;]
> helper = !ZZZ
> [credential "https://example.com/prefix1/;]
> helper = !YYY
> [credential "https://example.com/;]
> useHttpPath = true
> helper = !XXX
> [...]
> $ git config --get-urlmatch credential https://example.com/prefix1/bar.git
> credential.helper !YYY
> credential.usehttppath true
>
> Is this discrepancy intended?
Sort of. The matching done by the credential code predates the config
code learning about url matching, so it uses a much more basic system.
It walks through the config in order for a particular request, throwing
away any entries whose subsections don't match, and then applying (in
the order it finds them) any entries which do. The matching for paths is
done using the whole path, not a prefix match.
There's something else going on, too: credential.helper is a
multi-valued variable, so it's going to try each matching helper in
turn. Whereas "git config --get" (and "--get-urlmatch") assume you're
looking for a single value, and use the last-one-wins rule that most
variables use.
Normally you'd want to use "--get-all" for that, though I don't know if
there's a way to combine it with url matching.
So what you're seeing is the code working as designed, but I agree the
result kind of sucks. I wouldn't be sad to see the credential code moved
over to use the same url-matching as http.* uses. It would technically
be backwards-incompatible in a few cases, but I think the new behavior
would almost always be what the person intended in the first place.
With the current code, you'd have to teach your helper to be more clever
about matching the path. E.g., by wrapping your existing helper with
something like:
-- >8 --
#!/usr/bin/perl
my %input = map { /(.*?)=(.*)/ } ;
my $helper =
$input{path} =~ m{^prefix1/foo\.git} ? 'ZZZ' :
$input{path} =~ m{^prefix1/} ? 'YYY' :
'XXX';
my $pid = open(my $out, '|-', $helper, @ARGV);
print $out "$_=$input{$_}\n" for keys(%input);
close($out);
waitpid $pid, 0;
-- >8 --
I know that's pretty nasty for your simple use case, but I think it's
the best you can do with the current system.
> If indeed the current behavior of git-credential is as intended, I think
> it would be helpful for that manpage to explicitly mention it (i.e. that
> you may specify a URL path component but that it must match exactly).
> Right now the only example given is one in which 'the "pattern" URL does
> not care about the path component at all.'
Yes, I think this could be more clear in the "credential contexts"
section of gitcredentials(7). Do you want to try to make a patch?
-Peff
Re: [PATCH] Add an EditorConfig file
On Thu, Sep 20, 2018 at 07:08:35AM -0700, Junio C Hamano wrote: > My comment was that it would be confusing if they gave contradicting > suggestions to the end user. After letting EditorConfig to enforce > one style while typing and saving, if "make style" suggests to > format it differently, it would not be a great user experience. Ah, okay. I understand your concern better now. > The ideal response would have been "Oh, of course EditorConfig folks > already thought about that, which is a natural thing to wish for, > and they have a tool to generate clang-format configuration from the > section for C language in any EditorConfig file---here is a link. > After all, tools like clang-format look like just another editor to > them ;-)". No, I don't think there's such a tool. clang-format wants a lot of format settings that aren't in EditorConfig, and EditorConfig wants to address things that are not C source files. > But that is a response in a dream-world. If there is no such tool, > I am perfectly OK if the plan is to manually keep them (loosely) in > sync. I do not think it is good use of our time to try to come up > with such a tool (unless somebody is really interested in doing it, > that is). Would it be helpful if I sent a script that ran during CI to ensure they stayed in sync for the couple places where they overlap? I'm happy to do so if you think it would be useful. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
[PATCH 5/8] submodule.c: do not copy around submodule list
'calculate_changed_submodule_paths' uses a local list to compute the
changed submodules, and then produces the result by copying appropriate
items into the result list.
Instead use the result list directly and prune items afterwards
using string_list_remove_empty_items.
By doing so we'll have access to the util pointer for longer that
contains the commits that we need to fetch, which will be
useful in a later patch.
Signed-off-by: Stefan Beller
---
submodule.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/submodule.c b/submodule.c
index 7b4d136849b..209680ec076 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1129,8 +1129,7 @@ static void calculate_changed_submodule_paths(
struct submodule_parallel_fetch *spf)
{
struct argv_array argv = ARGV_ARRAY_INIT;
- struct string_list changed_submodules = STRING_LIST_INIT_DUP;
- const struct string_list_item *name;
+ struct string_list_item *name;
/* No need to check if there are no submodules configured */
if (!submodule_from_path(the_repository, NULL, NULL))
@@ -1147,9 +1146,9 @@ static void calculate_changed_submodule_paths(
* Collect all submodules (whether checked out or not) for which new
* commits have been recorded upstream in "changed_submodule_names".
*/
- collect_changed_submodules(_submodules, );
+ collect_changed_submodules(>changed_submodule_names, );
- for_each_string_list_item(name, _submodules) {
+ for_each_string_list_item(name, >changed_submodule_names) {
struct oid_array *commits = name->util;
const struct submodule *submodule;
const char *path = NULL;
@@ -1163,12 +1162,14 @@ static void calculate_changed_submodule_paths(
if (!path)
continue;
- if (!submodule_has_commits(path, commits))
- string_list_append(>changed_submodule_names,
- name->string);
+ if (submodule_has_commits(path, commits)) {
+ oid_array_clear(commits);
+ *name->string = '\0';
+ }
}
- free_submodules_oids(_submodules);
+ string_list_remove_empty_items(>changed_submodule_names, 1);
+
argv_array_clear();
oid_array_clear(_tips_before_fetch);
oid_array_clear(_tips_after_fetch);
@@ -1364,7 +1365,7 @@ int fetch_populated_submodules(struct repository *r,
argv_array_clear();
out:
- string_list_clear(_submodule_names, 1);
+ free_submodules_oids(_submodule_names);
return spf.result;
}
--
2.19.0.444.g18242da7ef-goog
[PATCH 6/8] submodule: fetch in submodules git directory instead of in worktree
This patch started as a refactoring to make 'get_next_submodule' more
readable, but upon doing so, I realized that "git fetch" of the submodule
actually doesn't need to be run in the submodules worktree. So let's run
it in its git dir instead.
That should pave the way towards fetching submodules that are currently
not checked out.
This patch leaks the cp->dir in get_next_submodule, as any further
callback in run_processes_parallel doesn't have access to the child
process any more. In an early iteration of this patch, the function
get_submodule_repo_for directly returned the string containing the
git directory, which would be a better design choice for this patch.
However the next patch both fixes the memory leak of cp->dir and also has
a use case for using the full repository handle of the submodule, so
it makes sense to introduce the get_submodule_repo_for here already.
Signed-off-by: Stefan Beller
---
submodule.c | 47 +++--
t/t5526-fetch-submodules.sh | 7 +-
2 files changed, 40 insertions(+), 14 deletions(-)
diff --git a/submodule.c b/submodule.c
index 209680ec076..8829e8d4eff 100644
--- a/submodule.c
+++ b/submodule.c
@@ -482,6 +482,12 @@ void prepare_submodule_repo_env(struct argv_array *out)
DEFAULT_GIT_DIR_ENVIRONMENT);
}
+static void prepare_submodule_repo_env_in_gitdir(struct argv_array *out)
+{
+ prepare_submodule_repo_env_no_git_dir(out);
+ argv_array_pushf(out, "%s=.", GIT_DIR_ENVIRONMENT);
+}
+
/* Helper function to display the submodule header line prior to the full
* summary output. If it can locate the submodule objects directory it will
* attempt to lookup both the left and right commits and put them into the
@@ -1228,6 +1234,25 @@ static int get_fetch_recurse_config(const struct
submodule *submodule,
return spf->default_option;
}
+static struct repository *get_submodule_repo_for(struct repository *r,
+const char *path)
+{
+ struct repository *ret = xmalloc(sizeof(*ret));
+
+ if (repo_submodule_init(ret, r, path)) {
+ /* no entry in .gitmodules? */
+ struct strbuf gitdir = STRBUF_INIT;
+ strbuf_repo_worktree_path(, r, "%s/.git", path);
+ if (repo_init(ret, gitdir.buf, NULL)) {
+ strbuf_release();
+ return NULL;
+ }
+ strbuf_release();
+ }
+
+ return ret;
+}
+
static int get_next_submodule(struct child_process *cp,
struct strbuf *err, void *data, void **task_cb)
{
@@ -1235,12 +1260,11 @@ static int get_next_submodule(struct child_process *cp,
struct submodule_parallel_fetch *spf = data;
for (; spf->count < spf->r->index->cache_nr; spf->count++) {
- struct strbuf submodule_path = STRBUF_INIT;
- struct strbuf submodule_git_dir = STRBUF_INIT;
struct strbuf submodule_prefix = STRBUF_INIT;
const struct cache_entry *ce = spf->r->index->cache[spf->count];
- const char *git_dir, *default_argv;
+ const char *default_argv;
const struct submodule *submodule;
+ struct repository *repo;
struct submodule default_submodule = SUBMODULE_INIT;
if (!S_ISGITLINK(ce->ce_mode))
@@ -1275,16 +1299,12 @@ static int get_next_submodule(struct child_process *cp,
continue;
}
- strbuf_repo_worktree_path(_path, spf->r, "%s",
ce->name);
- strbuf_addf(_git_dir, "%s/.git", submodule_path.buf);
strbuf_addf(_prefix, "%s%s/", spf->prefix, ce->name);
- git_dir = read_gitfile(submodule_git_dir.buf);
- if (!git_dir)
- git_dir = submodule_git_dir.buf;
- if (is_directory(git_dir)) {
+ repo = get_submodule_repo_for(spf->r, ce->name);
+ if (repo) {
child_process_init(cp);
- cp->dir = strbuf_detach(_path, NULL);
- prepare_submodule_repo_env(>env_array);
+ prepare_submodule_repo_env_in_gitdir(>env_array);
+ cp->dir = xstrdup(repo->gitdir);
cp->git_cmd = 1;
if (!spf->quiet)
strbuf_addf(err, "Fetching submodule %s%s\n",
@@ -1294,10 +1314,11 @@ static int get_next_submodule(struct child_process *cp,
argv_array_push(>args, default_argv);
argv_array_push(>args, "--submodule-prefix");
argv_array_push(>args, submodule_prefix.buf);
+
+ repo_clear(repo);
+ free(repo);
ret = 1;
}
- strbuf_release(_path);
[PATCH 7/8] fetch: retry fetching submodules if needed objects were not fetched
Currently when git-fetch is asked to recurse into submodules, it dispatches
a plain "git-fetch -C " (with some submodule related options
such as prefix and recusing strategy, but) without any information of the
remote or the tip that should be fetched.
This works surprisingly well in some workflows (such as using submodules
as a third party library), while not so well in other scenarios, such
as in a Gerrit topic-based workflow, that can tie together changes
(potentially across repositories) on the server side. One of the parts
of such a Gerrit workflow is to download a change when wanting to examine
it, and you'd want to have its submodule changes that are in the same
topic downloaded as well. However these submodule changes reside in their
own repository in their own ref (refs/changes/).
Retry fetching a submodule by object name if the object id that the
superproject points to, cannot be found.
This retrying does not happen when the "git fetch" done at the
superproject is not storing the fetched results in remote
tracking branches (i.e. instead just recording them to
FETCH_HEAD) in this step. A later patch will fix this.
builtin/fetch used to only inspect submodules when they were fetched
"on-demand", as in either on/off case it was clear whether the submodule
needs to be fetched. However to know whether we need to try fetching the
object ids, we need to identify the object names, which is done in this
function check_for_new_submodule_commits(), so we'll also run that code
in case the submodule recursion is set to "on".
Signed-off-by: Stefan Beller
---
builtin/fetch.c | 9 +-
submodule.c | 182 ++--
t/t5526-fetch-submodules.sh | 16
3 files changed, 174 insertions(+), 33 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 0696abfc2a1..e3b03ad3bd3 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -707,8 +707,7 @@ static int update_local_ref(struct ref *ref,
what = _("[new ref]");
}
- if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
- (recurse_submodules != RECURSE_SUBMODULES_ON))
+ if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref(msg, ref, 0);
format_display(display, r ? '!' : '*', what,
@@ -723,8 +722,7 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(, >object.oid,
DEFAULT_ABBREV);
strbuf_addstr(, "..");
strbuf_add_unique_abbrev(, >new_oid,
DEFAULT_ABBREV);
- if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
- (recurse_submodules != RECURSE_SUBMODULES_ON))
+ if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref("fast-forward", ref, 1);
format_display(display, r ? '!' : ' ', quickref.buf,
@@ -738,8 +736,7 @@ static int update_local_ref(struct ref *ref,
strbuf_add_unique_abbrev(, >object.oid,
DEFAULT_ABBREV);
strbuf_addstr(, "...");
strbuf_add_unique_abbrev(, >new_oid,
DEFAULT_ABBREV);
- if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
- (recurse_submodules != RECURSE_SUBMODULES_ON))
+ if (recurse_submodules != RECURSE_SUBMODULES_OFF)
check_for_new_submodule_commits(>new_oid);
r = s_update_ref("forced-update", ref, 1);
format_display(display, r ? '!' : '+', quickref.buf,
diff --git a/submodule.c b/submodule.c
index 8829e8d4eff..ba98a94d4db 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1128,8 +1128,12 @@ struct submodule_parallel_fetch {
int result;
struct string_list changed_submodule_names;
+ struct get_next_submodule_task **retry;
+ int retry_nr, retry_alloc;
};
-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0,
STRING_LIST_INIT_DUP }
+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0, \
+ STRING_LIST_INIT_DUP, \
+ NULL, 0, 0}
static void calculate_changed_submodule_paths(
struct submodule_parallel_fetch *spf)
@@ -1234,6 +1238,56 @@ static int get_fetch_recurse_config(const struct
submodule *submodule,
return spf->default_option;
}
+struct get_next_submodule_task {
+ struct repository *repo;
+ const struct submodule *sub;
+ unsigned free_sub : 1; /* Do we need to free the submodule? */
+ struct oid_array *commits;
+};
+
+static const struct submodule *get_default_submodule(const char *path)
+{
+ struct submodule *ret = NULL;
+ const char *name = default_name_or_path(path);
+
+ if (!name)
+ return NULL;
+
+ ret = xmalloc(sizeof(*ret));
+
[PATCH 8/8] builtin/fetch: check for submodule updates for non branch fetches
Gerrit, the code review tool, has a different workflow than our mailing
list based approach. Usually users upload changes to a Gerrit server and
continuous integration and testing happens by bots. Sometimes however a
user wants to checkout a change locally and look at it locally. For this
use case, Gerrit offers a command line snippet to copy and paste to your
terminal, which looks like
git fetch https:///gerrit refs/changes/ &&
git checkout FETCH_HEAD
For Gerrit changes that contain changing submodule gitlinks, it would be
easy to extend both the fetch and checkout with the '--recurse-submodules'
flag, such that this command line snippet would produce the state of a
change locally.
However the functionality added in the previous patch, which would
ensure that we fetch the objects in the submodule that the gitlink pointed
at, only works for remote tracking branches so far, not for FETCH_HEAD.
Make sure that fetching a superproject to its FETCH_HEAD, also respects
the existence checks for objects in the submodule recursion.
Signed-off-by: Stefan Beller
---
builtin/fetch.c | 5 -
t/t5526-fetch-submodules.sh | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index e3b03ad3bd3..f2d9e548bf0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -894,11 +894,14 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
rc |= update_local_ref(ref, what, rm, ,
summary_width);
free(ref);
- } else
+ } else {
+ check_for_new_submodule_commits(>old_oid);
format_display(, '*',
*kind ? kind : "branch", NULL,
*what ? what : "HEAD",
"FETCH_HEAD", summary_width);
+ }
+
if (note.len) {
if (verbosity >= 0 && !shown_url) {
fprintf(stderr, _("From %.*s\n"),
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index af12c50e7dd..a509eabb044 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -615,7 +615,7 @@ test_expect_success "fetch new commits on-demand when they
are not reachable" '
git update-ref refs/changes/2 $D &&
(
cd downstream &&
- git fetch --recurse-submodules --recurse-submodules-default
on-demand origin refs/changes/2:refs/heads/my_branch &&
+ git fetch --recurse-submodules origin refs/changes/2 &&
git -C submodule cat-file -t $C &&
git checkout --recurse-submodules FETCH_HEAD
)
--
2.19.0.444.g18242da7ef-goog
[PATCH 4/8] submodule: move global changed_submodule_names into fetch submodule struct
The `changed_submodule_names` are only used for fetching, so let's make it
part of the struct that is passed around for fetching submodules.
Signed-off-by: Stefan Beller
---
submodule.c | 42 +++---
1 file changed, 23 insertions(+), 19 deletions(-)
diff --git a/submodule.c b/submodule.c
index 2f5c19d0aac..7b4d136849b 100644
--- a/submodule.c
+++ b/submodule.c
@@ -25,7 +25,7 @@
#include "commit-reach.h"
static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
-static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
+
static int initialized_fetch_ref_tips;
static struct oid_array ref_tips_before_fetch;
static struct oid_array ref_tips_after_fetch;
@@ -,7 +,22 @@ void check_for_new_submodule_commits(struct object_id
*oid)
oid_array_append(_tips_after_fetch, oid);
}
-static void calculate_changed_submodule_paths(void)
+struct submodule_parallel_fetch {
+ int count;
+ struct argv_array args;
+ struct repository *r;
+ const char *prefix;
+ int command_line_option;
+ int default_option;
+ int quiet;
+ int result;
+
+ struct string_list changed_submodule_names;
+};
+#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0,
STRING_LIST_INIT_DUP }
+
+static void calculate_changed_submodule_paths(
+ struct submodule_parallel_fetch *spf)
{
struct argv_array argv = ARGV_ARRAY_INIT;
struct string_list changed_submodules = STRING_LIST_INIT_DUP;
@@ -1149,7 +1164,8 @@ static void calculate_changed_submodule_paths(void)
continue;
if (!submodule_has_commits(path, commits))
- string_list_append(_submodule_names,
name->string);
+ string_list_append(>changed_submodule_names,
+ name->string);
}
free_submodules_oids(_submodules);
@@ -1186,18 +1202,6 @@ int submodule_touches_in_range(struct object_id
*excl_oid,
return ret;
}
-struct submodule_parallel_fetch {
- int count;
- struct argv_array args;
- struct repository *r;
- const char *prefix;
- int command_line_option;
- int default_option;
- int quiet;
- int result;
-};
-#define SPF_INIT {0, ARGV_ARRAY_INIT, NULL, NULL, 0, 0, 0, 0}
-
static int get_fetch_recurse_config(const struct submodule *submodule,
struct submodule_parallel_fetch *spf)
{
@@ -1258,7 +1262,7 @@ static int get_next_submodule(struct child_process *cp,
case RECURSE_SUBMODULES_ON_DEMAND:
if (!submodule ||
!string_list_lookup(
- _submodule_names,
+ >changed_submodule_names,
submodule->name))
continue;
default_argv = "on-demand";
@@ -1350,8 +1354,8 @@ int fetch_populated_submodules(struct repository *r,
argv_array_push(, "--recurse-submodules-default");
/* default value, "--submodule-prefix" and its value are added later */
- calculate_changed_submodule_paths();
- string_list_sort(_submodule_names);
+ calculate_changed_submodule_paths();
+ string_list_sort(_submodule_names);
run_processes_parallel(max_parallel_jobs,
get_next_submodule,
fetch_start_failure,
@@ -1360,7 +1364,7 @@ int fetch_populated_submodules(struct repository *r,
argv_array_clear();
out:
- string_list_clear(_submodule_names, 1);
+ string_list_clear(_submodule_names, 1);
return spf.result;
}
--
2.19.0.444.g18242da7ef-goog
[PATCH 3/8] submodule.c: sort changed_submodule_names before searching it
We can string_list_insert() to maintain sorted-ness of the list as we find new items, or we can string_list_append() to build an unsorted list and sort it at the end just once. To pick which one is more appropriate, we notice the fact that we discover new items more or less in the already sorted order. That makes "append then sort" more appropriate. Signed-off-by: Stefan Beller --- submodule.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/submodule.c b/submodule.c index 67469a8f513..2f5c19d0aac 100644 --- a/submodule.c +++ b/submodule.c @@ -1257,7 +1257,7 @@ static int get_next_submodule(struct child_process *cp, case RECURSE_SUBMODULES_DEFAULT: case RECURSE_SUBMODULES_ON_DEMAND: if (!submodule || - !unsorted_string_list_lookup( + !string_list_lookup( _submodule_names, submodule->name)) continue; @@ -1351,6 +1351,7 @@ int fetch_populated_submodules(struct repository *r, /* default value, "--submodule-prefix" and its value are added later */ calculate_changed_submodule_paths(); + string_list_sort(_submodule_names); run_processes_parallel(max_parallel_jobs, get_next_submodule, fetch_start_failure, -- 2.19.0.444.g18242da7ef-goog
[PATCH 1/8] sha1-array: provide oid_array_filter
Helped-by: Junio C Hamano
Signed-off-by: Stefan Beller
---
sha1-array.c | 17 +
sha1-array.h | 9 +
2 files changed, 26 insertions(+)
diff --git a/sha1-array.c b/sha1-array.c
index b94e0ec0f5e..d922e94e3fc 100644
--- a/sha1-array.c
+++ b/sha1-array.c
@@ -77,3 +77,20 @@ int oid_array_for_each_unique(struct oid_array *array,
}
return 0;
}
+
+void oid_array_filter(struct oid_array *array,
+ for_each_oid_fn want,
+ void *cb_data)
+{
+ unsigned nr = array->nr, src, dst;
+ struct object_id *oids = array->oid;
+
+ for (src = dst = 0; src < nr; src++) {
+ if (want([src], cb_data)) {
+ if (src != dst)
+ oidcpy([dst], [src]);
+ dst++;
+ }
+ }
+ array->nr = dst;
+}
diff --git a/sha1-array.h b/sha1-array.h
index 232bf950172..275e5b02f5e 100644
--- a/sha1-array.h
+++ b/sha1-array.h
@@ -23,4 +23,13 @@ int oid_array_for_each_unique(struct oid_array *array,
for_each_oid_fn fn,
void *data);
+/*
+ * Apply want to each entry in array, retaining only the entries for
+ * which the function returns true. Preserve the order of the entries
+ * that are retained.
+ */
+void oid_array_filter(struct oid_array *array,
+ for_each_oid_fn want,
+ void *cbdata);
+
#endif /* SHA1_ARRAY_H */
--
2.19.0.444.g18242da7ef-goog
[PATCH 2/8] submodule.c: fix indentation
The submodule subsystem is really bad at staying within 80 characters.
Fix it while we are here.
Signed-off-by: Stefan Beller
---
submodule.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/submodule.c b/submodule.c
index ed05339b588..67469a8f513 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1245,7 +1245,8 @@ static int get_next_submodule(struct child_process *cp,
if (!submodule) {
const char *name = default_name_or_path(ce->name);
if (name) {
- default_submodule.path = default_submodule.name
= name;
+ default_submodule.path = name;
+ default_submodule.name = name;
submodule = _submodule;
}
}
@@ -1255,8 +1256,10 @@ static int get_next_submodule(struct child_process *cp,
default:
case RECURSE_SUBMODULES_DEFAULT:
case RECURSE_SUBMODULES_ON_DEMAND:
- if (!submodule ||
!unsorted_string_list_lookup(_submodule_names,
-submodule->name))
+ if (!submodule ||
+ !unsorted_string_list_lookup(
+ _submodule_names,
+ submodule->name))
continue;
default_argv = "on-demand";
break;
--
2.19.0.444.g18242da7ef-goog
[PATCHv3 0/8] fetch: make sure submodule oids are fetched
v3:
* I discovered some issues with v2 after sending,
which is why I rewrote the later patches completely
and now we pass around a "task" struct that contains everything to know
about the things to work on and what needs free()ing afterwards.
* as it is no longer string list based, this drops adding string_list_{pop,
last}
v2:
* extended commit messages,
* plugged a memory leak
* rewrote the patch "sha1-array: provide oid_array_filter" to be much more like
object_array_fiter
* fixed a typo pointed out by Ramsay.
The range diff is below.
Thanks,
Stefan
v1:
Currently when git-fetch is asked to recurse into submodules, it dispatches
a plain "git-fetch -C " (and some submodule related options
such as prefix and recusing strategy, but) without any information of the
remote or the tip that should be fetched.
This works surprisingly well in some workflows, not so well in others,
which this series aims to fix.
The first patches provide new basic functionality and do some refactoring;
the interesting part is in the two last patches.
This was discussed in
https://public-inbox.org/git/20180808221752.195419-1-sbel...@google.com/
and I think I addressed all feedback so far.
Stefan Beller (8):
sha1-array: provide oid_array_filter
submodule.c: fix indentation
submodule.c: sort changed_submodule_names before searching it
submodule: move global changed_submodule_names into fetch submodule
struct
submodule.c: do not copy around submodule list
submodule: fetch in submodules git directory instead of in worktree
fetch: retry fetching submodules if needed objects were not fetched
builtin/fetch: check for submodule updates for non branch fetches
builtin/fetch.c | 14 +-
sha1-array.c| 17 +++
sha1-array.h| 9 ++
submodule.c | 268
t/t5526-fetch-submodules.sh | 23 +++-
5 files changed, 268 insertions(+), 63 deletions(-)
--
2.19.0.444.g18242da7ef-goog
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 03:23:40PM -0700, Junio C Hamano wrote: > >> > +git config receive.advertisealternates true && > >> > >> Hmph. Do we have code to support this configuration variable? > > > > Sorry, I should have caught that. Our existing solution is to disable > > alternates in the advertisement entirely (since the optimization > > backfires for us). So this line is a leftover from testing it against > > our fork, and should be dropped. > > > > If anybody is interested, we can share those patches, though they're > > unsurprisingly trivial. > > Heh, I guessed correctly what is going on ;-) > > Even though there may not be much interest in the "all-or-none" > boolean configuration, in order to upstream this custom thing, it > may be the cleanest to upstream that all-or-none thing as well. > Otherwise, you'd need to keep a patch to this test script that is > private for your "all-or-none" feature. That's your maintenance > burden so it ultimately is your call ;-) Easy one-liners in test scripts are the least of my ongoing maintenance burden. ;) I think in this case, though, the line is not even necessary, as our patches leave the default as "true" (which is certainly what we would want upstream, as well, for compatibility). -Peff
Re: Very simple popen() code request, ground-shaking functionality openned by it
On Fri, Sep 21, 2018 at 09:34:14AM -0400, Sebastian Gniazdowski wrote:
> Git default progress indicator for clone is very unattractive, IMO. It
> does its job in providing all the operation details very well, but I
> bet most of users strongly dream about a gauge box!
>
> Have a look at my gauge box constructed as git-stderr pipe script:
> https://asciinema.org/a/202401
>
> The main point of my feature request is: git can add
> core.progress_pipe option, where e.g. `/usr/local/bin/mygauge' will be
> set (a script like the one in the asciinema), and then simply do the
> ground-school-known `popen("/usr/local/bin/mygauge","r+")', and write
> **unchanged current-progress data** to the pipe, then read from the
> pipe and forward to `stderr', where the progress normally lands in.
>
> This will allow users to free their creativity and provide probably
> dozens of custom Git progress bars.
I don't personally feel that the existing progress bar is that bad, but
if anybody wants to pursue this, I think the most sensible path is:
1. Add a trace_key for sending machine-readable progress output to a
descriptor or file. E.g., via setting GIT_TRACE_PROGRESS=2 in the
environment.
2. Teach the trace code to open a command for piping, so that you
could do something like GIT_TRACE_PROGRESS='|mygauge'.
That would make your use case work, and I think many other use cases
would benefit from both of those features independently.
-Peff
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
Jeff King writes: > On Fri, Sep 21, 2018 at 02:09:08PM -0700, Junio C Hamano wrote: > >> > +test_expect_success 'setup' ' >> > + test_commit one && >> > + git update-ref refs/heads/a HEAD && >> > + test_commit two && >> > + git update-ref refs/heads/b HEAD && >> > + test_commit three && >> > + git update-ref refs/heads/c HEAD && >> > + git clone --bare . fork && >> > + git clone fork pusher && >> > + ( >> > + cd fork && >> > + git config receive.advertisealternates true && >> >> Hmph. Do we have code to support this configuration variable? > > Sorry, I should have caught that. Our existing solution is to disable > alternates in the advertisement entirely (since the optimization > backfires for us). So this line is a leftover from testing it against > our fork, and should be dropped. > > If anybody is interested, we can share those patches, though they're > unsurprisingly trivial. Heh, I guessed correctly what is going on ;-) Even though there may not be much interest in the "all-or-none" boolean configuration, in order to upstream this custom thing, it may be the cleanest to upstream that all-or-none thing as well. Otherwise, you'd need to keep a patch to this test script that is private for your "all-or-none" feature. That's your maintenance burden so it ultimately is your call ;-) > Also, useless-use-of-cat in the original, which could be: > > git update-ref --stdin <<-\EOF Yup. Thanks.
Re: [PATCH v2 3/3] transport.c: introduce core.alternateRefsPrefixes
On Fri, Sep 21, 2018 at 3:18 PM Jeff King wrote: > I agree that core.* is kind of a kitchen sink, but I'm not sure that's > all that bad. Is "here is how Git finds refs in an alternate" any more This touches both "refs" and "alternates", which are Git concepts whereas ssh is not. > or less core than "here is how Git invokes ssh"? Arguably core.sshCommand should be deprecated and re-introduced as transport."ssh".command. :-P
Re: [PATCH] Add an EditorConfig file
On Thu, Sep 20, 2018 at 10:35:04PM -0400, Jeff King wrote: > On Thu, Sep 20, 2018 at 10:26:47PM -0400, Eric Sunshine wrote: > > > On Wed, Sep 19, 2018 at 8:00 PM brian m. carlson > > wrote: > > > (I am having trouble getting make style to work, though, because it > > > seems to invoke clang-format as a git subcommand, and I don't think that > > > works. I may send a patch.) > > > > You're probably missing this piece: > > https://llvm.org/svn/llvm-project/cfe/trunk/tools/clang-format/git-clang-format I am indeed missing that. I looked for it in git.git, but didn't think to look for it as part of clang-format. > I've been confused by this, too. In Debian they ship it with the version > number. E.g., /usr/bin/git-clang-format-7, which is a symlink to > /usr/lib/llvm-7/bin/git-clang-format. I filed a bug report asking Debian to include a symlink in the default package. > We might want a Makefile variable to let you override this path (I think > I just ended up putting another symlink in ~/local/bin). I may send a patch to do that. -- brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH v2 3/3] transport.c: introduce core.alternateRefsPrefixes
On Fri, Sep 21, 2018 at 03:06:43PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > There's no extension necessary; these should already affect upload-pack > > as well. I agree transport.* would cover both upload-pack and > > receive-pack. If we extend it to check_everything_connected(), would it > > make sense as part of transport.*, too? > > > > I dunno. I guess I could see an argument either way. > > Sorry but I do not quite follow. Are you saying that something that > covers check-everything-connected would the result be too wide to > fit inside transport.*? or something that does not cover > check-everything-connected falls short of transport.*? Or something > else? Either way, core.* is way too wide for what this hook does, I > would think. I was suggesting that check_everything_connected() is not strictly transport-related, so would be inappropriate for transport.*, and we'd need a more generic name. And my "either way" was that I could see an argument that it _is_ transport related, since we only call it now when receiving a pack. But that doesn't have to be the case, and certainly implementing it with "rev-list --alternate-refs" muddies that considerably. I agree that core.* is kind of a kitchen sink, but I'm not sure that's all that bad. Is "here is how Git finds refs in an alternate" any more or less core than "here is how Git invokes ssh"? -Peff
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 02:09:08PM -0700, Junio C Hamano wrote: > > +test_expect_success 'setup' ' > > + test_commit one && > > + git update-ref refs/heads/a HEAD && > > + test_commit two && > > + git update-ref refs/heads/b HEAD && > > + test_commit three && > > + git update-ref refs/heads/c HEAD && > > + git clone --bare . fork && > > + git clone fork pusher && > > + ( > > + cd fork && > > + git config receive.advertisealternates true && > > Hmph. Do we have code to support this configuration variable? Sorry, I should have caught that. Our existing solution is to disable alternates in the advertisement entirely (since the optimization backfires for us). So this line is a leftover from testing it against our fork, and should be dropped. If anybody is interested, we can share those patches, though they're unsurprisingly trivial. I suspect we may end up discarding them if this custom-command thing works, but it's possible we'll still need to be able to shut them off completely for some truly pathological cases. > > + cat <<-EOF | git update-ref --stdin && > > Style: writing "<<-\EOF" instead would allow readers' eyes to > coast over without having to look for $variable_references in > the here-doc. Also, useless-use-of-cat in the original, which could be: git update-ref --stdin <<-\EOF > [...] Yeah, I second all the other bits you mentioned. -Peff
[PATCH] string-list: add string_list_{pop, last} functions
Add a few functions to allow a string-list to be used as a stack:
- string_list_last() lets a caller peek the string_list_item at the
end of the string list. The caller needs to be aware that it is
borrowing a pointer, which can become invalid if/when the
string_list is resized.
- string_list_pop() removes the string_list_item at the end of
the string list.
- there is no string_list_push(); string_list_append() can be used
in its place.
You can use them in this pattern:
while (list.nr) {
struct string_list_item *item = string_list_last();
work_on(item);
string_list_pop(, free_util);
}
Helped-by: Junio C Hamano
Signed-off-by: Stefan Beller
---
I will be resending this series shortly, but it will not contain this patch,
as I will have no need for these any longer.
For the record, this is the final state of this patch.
It may be useful on its own.
Thanks,
Stefan
string-list.c | 14 ++
string-list.h | 15 +++
2 files changed, 29 insertions(+)
diff --git a/string-list.c b/string-list.c
index 771c4550980..04db2b537c0 100644
--- a/string-list.c
+++ b/string-list.c
@@ -80,6 +80,20 @@ void string_list_remove(struct string_list *list, const char
*string,
}
}
+void string_list_pop(struct string_list *list, int free_util)
+{
+ if (list->nr == 0)
+ BUG("tried to remove an item from empty string list");
+
+ if (list->strdup_strings)
+ free(list->items[list->nr - 1].string);
+
+ if (free_util)
+ free(list->items[list->nr - 1].util);
+
+ list->nr--;
+}
+
int string_list_has_string(const struct string_list *list, const char *string)
{
int exact_match;
diff --git a/string-list.h b/string-list.h
index ff8f6094a33..15f2936e337 100644
--- a/string-list.h
+++ b/string-list.h
@@ -191,6 +191,21 @@ extern void string_list_remove(struct string_list *list,
const char *string,
*/
struct string_list_item *string_list_lookup(struct string_list *list, const
char *string);
+/**
+ * Removes the last item from the list.
+ * The caller must ensure that the list is not empty.
+ */
+void string_list_pop(struct string_list *list, int free_util);
+
+/*
+ * Returns the last item of the list. As it returns the raw access, do not
+ * modify the list while holding onto the returned pointer.
+ */
+static inline struct string_list_item *string_list_last(struct string_list
*list)
+{
+ return >items[list->nr - 1];
+}
+
/*
* Remove all but the first of consecutive entries with the same
* string value. If free_util is true, call free() on the util
--
2.19.0.444.g18242da7ef-goog
Re: [PATCH v2 3/3] transport.c: introduce core.alternateRefsPrefixes
Jeff King writes: > There's no extension necessary; these should already affect upload-pack > as well. I agree transport.* would cover both upload-pack and > receive-pack. If we extend it to check_everything_connected(), would it > make sense as part of transport.*, too? > > I dunno. I guess I could see an argument either way. Sorry but I do not quite follow. Are you saying that something that covers check-everything-connected would the result be too wide to fit inside transport.*? or something that does not cover check-everything-connected falls short of transport.*? Or something else? Either way, core.* is way too wide for what this hook does, I would think.
Re: [PATCH v2 1/2] commit-graph write: add progress output
Junio C Hamano writes: >> The above prototype change seems to have created a semantic conflict >> with ds/commit-graph-tests (859fdc "commit-graph: define >> GIT_TEST_COMMIT_GRAPH") because when GIT_TEST_COMMIT_GRAPH is set, we >> call write_commit_graph_reachable() but the final parameter was >> resolved to be "1" instead of "0". > > Hmph. That's unfortunate. > > Perhaps one of the topics should have yielded and waited until the > other one passes through. Nah, I see where things went wrong. I'll queue a single-liner "mismerge fix" to 'next', and then correct the seed for the evil merge kept in merge-fix/ab/commit-graph-progress, and rebuild 'pu'. Things will straighten out by themselves after that happens. Thanks for noticing.
Re: [PATCH v2 1/2] commit-graph write: add progress output
Derrick Stolee writes:
> On 9/7/2018 2:29 PM, Ævar Arnfjörð Bjarmason wrote:
>> -void write_commit_graph_reachable(const char *obj_dir, int append);
>> +void write_commit_graph_reachable(const char *obj_dir, int append,
>> + int report_progress);
>> void write_commit_graph(const char *obj_dir,
>> struct string_list *pack_indexes,
>> struct string_list *commit_hex,
>> -int append);
>> +int append, int report_progress);
>> int verify_commit_graph(struct repository *r, struct
>> commit_graph *g);
>>
>
> Junio,
>
> The above prototype change seems to have created a semantic conflict
> with ds/commit-graph-tests (859fdc "commit-graph: define
> GIT_TEST_COMMIT_GRAPH") because when GIT_TEST_COMMIT_GRAPH is set, we
> call write_commit_graph_reachable() but the final parameter was
> resolved to be "1" instead of "0".
Hmph. That's unfortunate.
Perhaps one of the topics should have yielded and waited until the
other one passes through.
As 859fdc0c ("commit-graph: define GIT_TEST_COMMIT_GRAPH",
2018-08-29) already is in 'master', the other "progress" topic
probably should be corrected to match. The easiest and cleanest
would be to eject the ab/commit-graph-progress topic out of 'next'
and have it rerolled on top of 'master', as we are going to rewind
the tip of 'next' anyway.
While we are at it, I suspect that a saner evolution of the API into
the function would not append more parameters to the call, but would
make the "do we append?" bit into a flag word "unsigned flags" with
two bits, and such a clean-up can be done as a preliminary change.
> This causes t3420-rebase-autostash.sh to fail, as that test watches
> the full output of the rebase command, including commit runs. The
> following patch fixes the problem, but could probably be squashed into
> a merge or other commit.
>
> Thanks,
>
> -Stolee
>
> -->8--
>
> From: Derrick Stolee
> Date: Fri, 21 Sep 2018 19:57:36 +
> Subject: [PATCH] commit: quietly write commit-graph in tests
>
> The GIT_TEST_COMMIT_GRAPH environment variable causes git-commit to
> write a commit-graph file on every execution. Recently, we added
> progress output when writing the commit-graph. This conflicts with
> some expected output during some tests, so avoid writing progress
> if writing a commit-graph this way.
>
> Signed-off-by: Derrick Stolee
> ---
> builtin/commit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 2a49ab4917..764664d832 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1660,7 +1660,7 @@ int cmd_commit(int argc, const char **argv,
> const char *prefix)
> "not exceeded, and then \"git reset HEAD\" to
> recover."));
>
> if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0))
> - write_commit_graph_reachable(get_object_directory(), 0, 1);
> + write_commit_graph_reachable(get_object_directory(), 0, 0);
>
> rerere(0);
> run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
> --
> 2.19.0
Re: [PATCH v2 3/3] transport.c: introduce core.alternateRefsPrefixes
On Fri, Sep 21, 2018 at 02:14:17PM -0700, Junio C Hamano wrote: > Taylor Blau writes: > > > +core.alternateRefsPrefixes:: > > + When listing references from an alternate, list only references that > > begin > > + with the given prefix. Prefixes match as if they were given as > > arguments to > > + linkgit:git-for-each-ref[1]. To list multiple prefixes, separate them > > with > > + whitespace. If `core.alternateRefsCommand` is set, setting > > + `core.alternateRefsPrefixes` has no effect. > > We do not allow anything elaborate like "refs/tags/release-*" but we > still allow "refs/tags/" and "refs/heads/" by listing them together, > and because these are only prefixes, whitespace is a reasonable list > separator as they cannot appear anywhere in a refname. OK. > > Why is this "core"? I thought this was more about receive-pack; > even if this is going to be extended to upload-pack's negotiation, > "core" is way too wide a hierarchy. We have "transport.*" for > things like this, no? There's no extension necessary; these should already affect upload-pack as well. I agree transport.* would cover both upload-pack and receive-pack. If we extend it to check_everything_connected(), would it make sense as part of transport.*, too? I dunno. I guess I could see an argument either way. If we do add "rev-list --alternate-refs", that pushes it even further away from transport.*, though. -Peff
Re: [PATCH v3 0/7] Use generation numbers for --topo-order
"Derrick Stolee via GitGitGadget" writes: > Changes in V3: I added a new patch that updates the tab-alignment for flags > in revision.h before adding new ones (Thanks, Ævar!). This is most unwelcome while other topics are in flight that caused unnecessary conflict. It would have been very welcomed if the codebase was dormant, though. I'll live, and there is no need to resend, but this change may not appear in today's pushout (I'll have to push out the result of integration before I saw this new reroll with all the other topics).
Re: [PATCH v2 3/3] transport.c: introduce core.alternateRefsPrefixes
Taylor Blau writes:
> +core.alternateRefsPrefixes::
> + When listing references from an alternate, list only references that
> begin
> + with the given prefix. Prefixes match as if they were given as
> arguments to
> + linkgit:git-for-each-ref[1]. To list multiple prefixes, separate them
> with
> + whitespace. If `core.alternateRefsCommand` is set, setting
> + `core.alternateRefsPrefixes` has no effect.
We do not allow anything elaborate like "refs/tags/release-*" but we
still allow "refs/tags/" and "refs/heads/" by listing them together,
and because these are only prefixes, whitespace is a reasonable list
separator as they cannot appear anywhere in a refname. OK.
Why is this "core"? I thought this was more about receive-pack;
even if this is going to be extended to upload-pack's negotiation,
"core" is way too wide a hierarchy. We have "transport.*" for
things like this, no?
The exact same comment applies to 2/3, of course.
> diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> index 2f21f1cb8f..b656c9b30c 100755
> --- a/t/t5410-receive-pack.sh
> +++ b/t/t5410-receive-pack.sh
> @@ -51,4 +51,12 @@ test_expect_success 'with core.alternateRefsCommand' '
> test_cmp expect actual.haves
> '
>
> +test_expect_success 'with core.alternateRefsPrefixes' '
> + test_config -C fork core.alternateRefsPrefixes "refs/tags" &&
> + expect_haves one three two >expect &&
> + printf "" | git receive-pack fork >actual &&
> + extract_haves actual.haves &&
> + test_cmp expect actual.haves
> +'
> +
> test_done
> diff --git a/transport.c b/transport.c
> index e7d2cdf00b..9323e5c3cd 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1341,6 +1341,11 @@ static void fill_alternate_refs_command(struct
> child_process *cmd,
> argv_array_pushf(>args, "--git-dir=%s", repo_path);
> argv_array_push(>args, "for-each-ref");
> argv_array_push(>args, "--format=%(objectname)
> %(refname)");
> +
> + if (!git_config_get_value("core.alternateRefsPrefixes",
> )) {
> + argv_array_push(>args, "--");
> + argv_array_split(>args, value);
> + }
> }
>
> cmd->env = local_repo_env;
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 2:47 PM Taylor Blau wrote:
> When in a repository containing one or more alternates, Git would
> sometimes like to list references from its alternates. For example, 'git
> receive-pack' list the objects pointed to by alternate references as
> special ".have" references.
> [...]
> Signed-off-by: Taylor Blau
> ---
> diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> @@ -0,0 +1,54 @@
> +expect_haves () {
> + printf "%s .have\n" $(git rev-parse $@) >expect
> +}
> +
> +test_expect_success 'with core.alternateRefsCommand' '
> + [...]
> + expect_haves a c >expect &&
This is not great. Both the caller of expect_haves() and
expect_haves() itself redirect to a file named "expect". This works,
but only by accident.
Better would be to make expect_haves() simply a generator to stdout
and let the caller redirect to the file rather than hardcoding the
filename in the function itself (much as extract_haves() takes it its
input on stdin rather than hardcoding a filename). If you take this
approach, then you'd probably want to rename the function, as well;
perhaps call it emit_haves() or something.
> + printf "" | git receive-pack fork >actual &&
> + extract_haves actual.haves &&
> + test_cmp expect actual.haves
> +'
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
Taylor Blau writes:
> +core.alternateRefsCommand::
> + When listing references from an alternate (e.g., in the case of
> ".have"), use
It is not clear how (e.g.,...) connects to what is said in the
sentence. "When advertising tips of available history from an
alternate, use ..." without saying ".have" may be less cryptic.
I dunno.
> + the shell to execute the specified command instead of
> + linkgit:git-for-each-ref[1]. The first argument is the path of the
> alternate.
"The path" meaning the absolute path? Relative to the original
object store? Something else?
> + Output must be of the form: `%(objectname) SPC %(refname)`.
> ++
> +This is useful when a repository only wishes to advertise some of its
> +alternate's references as ".have"'s. For example, to only advertise branch
> +heads, configure `core.alternateRefsCommand` to the path of a script which
> runs
> +`git --git-dir="$1" for-each-ref refs/heads`.
> +
> core.bare::
> If true this repository is assumed to be 'bare' and has no
> working directory associated with it. If this is the case a
> diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> new file mode 100755
> index 00..2f21f1cb8f
> --- /dev/null
> +++ b/t/t5410-receive-pack.sh
> @@ -0,0 +1,54 @@
> +#!/bin/sh
> +
> +test_description='git receive-pack test'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> + test_commit one &&
> + git update-ref refs/heads/a HEAD &&
> + test_commit two &&
> + git update-ref refs/heads/b HEAD &&
> + test_commit three &&
> + git update-ref refs/heads/c HEAD &&
> + git clone --bare . fork &&
> + git clone fork pusher &&
> + (
> + cd fork &&
> + git config receive.advertisealternates true &&
Hmph. Do we have code to support this configuration variable?
> + cat <<-EOF | git update-ref --stdin &&
Style: writing "<<-\EOF" instead would allow readers' eyes to
coast over without having to look for $variable_references in
the here-doc.
> + delete refs/heads/a
> + delete refs/heads/b
> + delete refs/heads/c
> + delete refs/heads/master
> + delete refs/tags/one
> + delete refs/tags/two
> + delete refs/tags/three
So, the original created one/two/three/a/b/c/master, fork is a bare
clone of it and has all these things, and then you deleted all of
these? What does fork have after this is done? HEAD that is
dangling?
> + EOF
> + echo "../../.git/objects" >objects/info/alternates
When viewed from fork/objects, ../../.git is the GIT_DIR of the
primary test repository, so that is where we borrow objects from.
If we pruned the objects from fork's object store before this echo,
we would have an almost empty repository that borrows from its
alternates everything, which may make a more realistic sample case,
but because you are only focusing on the ref advertisement, it does
not matter that your fork is full of duplicate objects that are
available from the alternates.
> +expect_haves () {
> + printf "%s .have\n" $(git rev-parse $@) >expect
Quote $@ inside dq pair, like $(git rev-parse "$@").
> +extract_haves () {
> + depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
> +}
Don't pipe grep into sed, especially when both the pattern to filter
and the operation to perform are simple.
I am not sure what you are trying to achive with 'g' in
s/pattern$//g; The anchor at the rightmost end of the pattern makes
sure that the pattern matches only once per line at the end anyway,
so "do this howmanyever times as we have match on each line" would
not make any difference, no?
> diff --git a/transport.c b/transport.c
> index 24ae3f375d..e7d2cdf00b 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1328,10 +1328,21 @@ char *transport_anonymize_url(const char *url)
> static void fill_alternate_refs_command(struct child_process *cmd,
> const char *repo_path)
> {
> - cmd->git_cmd = 1;
> - argv_array_pushf(>args, "--git-dir=%s", repo_path);
> - argv_array_push(>args, "for-each-ref");
> - argv_array_push(>args, "--format=%(objectname) %(refname)");
> + const char *value;
> +
> + if (!git_config_get_value("core.alternateRefsCommand", )) {
> + cmd->use_shell = 1;
> +
> + argv_array_push(>args, value);
> + argv_array_push(>args, repo_path);
> + } else {
> + cmd->git_cmd = 1;
> +
> + argv_array_pushf(>args, "--git-dir=%s", repo_path);
> + argv_array_push(>args, "for-each-ref");
> + argv_array_push(>args, "--format=%(objectname)
> %(refname)");
> + }
> +
> cmd->env = local_repo_env;
> cmd->out = -1;
> }
Re: [PATCH v3 1/5] CodingGuidelines: add shell piping guidelines
On Thu, Sep 20, 2018 at 7:06 PM Eric Sunshine wrote: > > On Thu, Sep 20, 2018 at 9:43 PM Matthew DeVore wrote: > > Add two guidelines: > > Probably s/two/three/ or s/two/several/ since the patch now adds three > guidelines. > > > - pipe characters should appear at the end of lines, and not cause > >indentation > > The "not cause indentation" bit is outdated since the added guideline > no longer says this. > > > - pipes should be avoided when they swallow exit codes that can > >potentially fail > > And: > > - $(git ...) should be avoided ... Thank you for pointing this out - I obviously forgot to update the commit message. Here is the revised message: CodingGuidelines: add shell piping guidelines Add the following guidelines: - pipe characters should appear at the end of lines, not the beginning, and the \ line continuation character should be omitted - pipes and $(git ...) should be avoided when they swallow exit codes of processes that can potentially fail
[PATCH v9 5/8] revision: mark non-user-given objects instead
Currently, list-objects.c incorrectly treats all root trees of commits
as USER_GIVEN. Also, it would be easier to mark objects that are
non-user-given instead of user-given, since the places in the code
where we access an object through a reference are more obvious than
the places where we access an object that was given by the user.
Resolve these two problems by introducing a flag NOT_USER_GIVEN that
marks blobs and trees that are non-user-given, replacing USER_GIVEN.
(Only blobs and trees are marked because this mark is only used when
filtering objects, and filtering of other types of objects is not
supported yet.)
This fixes a bug in that git rev-list behaved differently from git
pack-objects. pack-objects would *not* filter objects given explicitly
on the command line and rev-list would filter. This was because the two
commands used a different function to add objects to the rev_info
struct. This seems to have been an oversight, and pack-objects has the
correct behavior, so I added a test to make sure that rev-list now
behaves properly.
Signed-off-by: Matthew DeVore
---
list-objects.c | 31 +
revision.c | 1 -
revision.h | 11 --
t/t6112-rev-list-filters-objects.sh | 12 +++
4 files changed, 39 insertions(+), 16 deletions(-)
diff --git a/list-objects.c b/list-objects.c
index 243192af5..7a1a0929d 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -53,7 +53,7 @@ static void process_blob(struct traversal_context *ctx,
pathlen = path->len;
strbuf_addstr(path, name);
- if (!(obj->flags & USER_GIVEN) && ctx->filter_fn)
+ if ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn)
r = ctx->filter_fn(LOFS_BLOB, obj,
path->buf, >buf[pathlen],
ctx->filter_data);
@@ -120,17 +120,19 @@ static void process_tree_contents(struct
traversal_context *ctx,
continue;
}
- if (S_ISDIR(entry.mode))
- process_tree(ctx,
-lookup_tree(the_repository, entry.oid),
-base, entry.path);
+ if (S_ISDIR(entry.mode)) {
+ struct tree *t = lookup_tree(the_repository, entry.oid);
+ t->object.flags |= NOT_USER_GIVEN;
+ process_tree(ctx, t, base, entry.path);
+ }
else if (S_ISGITLINK(entry.mode))
process_gitlink(ctx, entry.oid->hash,
base, entry.path);
- else
- process_blob(ctx,
-lookup_blob(the_repository, entry.oid),
-base, entry.path);
+ else {
+ struct blob *b = lookup_blob(the_repository, entry.oid);
+ b->object.flags |= NOT_USER_GIVEN;
+ process_blob(ctx, b, base, entry.path);
+ }
}
}
@@ -171,7 +173,7 @@ static void process_tree(struct traversal_context *ctx,
}
strbuf_addstr(base, name);
- if (!(obj->flags & USER_GIVEN) && ctx->filter_fn)
+ if ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn)
r = ctx->filter_fn(LOFS_BEGIN_TREE, obj,
base->buf, >buf[baselen],
ctx->filter_data);
@@ -185,7 +187,7 @@ static void process_tree(struct traversal_context *ctx,
if (!failed_parse)
process_tree_contents(ctx, tree, base);
- if (!(obj->flags & USER_GIVEN) && ctx->filter_fn) {
+ if ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn) {
r = ctx->filter_fn(LOFS_END_TREE, obj,
base->buf, >buf[baselen],
ctx->filter_data);
@@ -301,8 +303,11 @@ static void do_traverse(struct traversal_context *ctx)
* an uninteresting boundary commit may not have its tree
* parsed yet, but we are not going to show them anyway
*/
- if (get_commit_tree(commit))
- add_pending_tree(ctx->revs, get_commit_tree(commit));
+ if (get_commit_tree(commit)) {
+ struct tree *tree = get_commit_tree(commit);
+ tree->object.flags |= NOT_USER_GIVEN;
+ add_pending_tree(ctx->revs, tree);
+ }
ctx->show_commit(commit, ctx->show_data);
if (ctx->revs->tree_blobs_in_commit_order)
diff --git a/revision.c b/revision.c
index de4dce600..72d48a17f 100644
--- a/revision.c
+++ b/revision.c
@@ -175,7 +175,6 @@ static void add_pending_object_with_path(struct rev_info
*revs,
[PATCH v9 6/8] list-objects-filter: use BUG rather than die
In some cases in this file, BUG makes more sense than die. In such
cases, a we get there from a coding error rather than a user error.
'return' has been removed following some instances of BUG since BUG does
not return.
Signed-off-by: Matthew DeVore
---
list-objects-filter.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/list-objects-filter.c b/list-objects-filter.c
index a0ba78b20..5f8b1a002 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -44,8 +44,7 @@ static enum list_objects_filter_result filter_blobs_none(
switch (filter_situation) {
default:
- die("unknown filter_situation");
- return LOFR_ZERO;
+ BUG("unknown filter_situation: %d", filter_situation);
case LOFS_BEGIN_TREE:
assert(obj->type == OBJ_TREE);
@@ -102,8 +101,7 @@ static enum list_objects_filter_result filter_blobs_limit(
switch (filter_situation) {
default:
- die("unknown filter_situation");
- return LOFR_ZERO;
+ BUG("unknown filter_situation: %d", filter_situation);
case LOFS_BEGIN_TREE:
assert(obj->type == OBJ_TREE);
@@ -208,8 +206,7 @@ static enum list_objects_filter_result filter_sparse(
switch (filter_situation) {
default:
- die("unknown filter_situation");
- return LOFR_ZERO;
+ BUG("unknown filter_situation: %d", filter_situation);
case LOFS_BEGIN_TREE:
assert(obj->type == OBJ_TREE);
@@ -389,7 +386,7 @@ void *list_objects_filter__init(
assert((sizeof(s_filters) / sizeof(s_filters[0])) == LOFC__COUNT);
if (filter_options->choice >= LOFC__COUNT)
- die("invalid list-objects filter choice: %d",
+ BUG("invalid list-objects filter choice: %d",
filter_options->choice);
init_fn = s_filters[filter_options->choice];
--
2.19.0.444.g18242da7ef-goog
[PATCH v9 7/8] list-objects-filter-options: do not over-strbuf_init
The function gently_parse_list_objects_filter is either called with
errbuf=STRBUF_INIT or errbuf=NULL, but that function calls strbuf_init
when errbuf is not NULL. strbuf_init is only necessary if errbuf
contains garbage, and risks a memory leak if errbuf already has a
non-STRBUF_INIT state. It should be the caller's responsibility to make
sure errbuf is not garbage, since garbage content is easily avoidable
with STRBUF_INIT.
Signed-off-by: Matthew DeVore
---
list-objects-filter-options.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index c0e2bd6a0..d259bdb2c 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -30,7 +30,6 @@ static int gently_parse_list_objects_filter(
if (filter_options->choice) {
if (errbuf) {
- strbuf_init(errbuf, 0);
strbuf_addstr(
errbuf,
_("multiple filter-specs cannot be combined"));
@@ -71,10 +70,9 @@ static int gently_parse_list_objects_filter(
return 0;
}
- if (errbuf) {
- strbuf_init(errbuf, 0);
+ if (errbuf)
strbuf_addf(errbuf, "invalid filter-spec '%s'", arg);
- }
+
memset(filter_options, 0, sizeof(*filter_options));
return 1;
}
--
2.19.0.444.g18242da7ef-goog
[PATCH v9 8/8] list-objects-filter: implement filter tree:0
Teach list-objects the "tree:0" filter which allows for filtering
out all tree and blob objects (unless other objects are explicitly
specified by the user). The purpose of this patch is to allow smaller
partial clones.
The name of this filter - tree:0 - does not explicitly specify that
it also filters out all blobs, but this should not cause much confusion
because blobs are not at all useful without the trees that refer to
them.
I also considered only:commits as a name, but this is inaccurate because
it suggests that annotated tags are omitted, but actually they are
included.
The name "tree:0" allows later filtering based on depth, i.e. "tree:1"
would filter out all but the root tree and blobs. In order to avoid
confusion between 0 and capital O, the documentation was worded in a
somewhat round-about way that also hints at this future improvement to
the feature.
Signed-off-by: Matthew DeVore
---
Documentation/rev-list-options.txt | 5 +++
list-objects-filter-options.c | 13 +++
list-objects-filter-options.h | 1 +
list-objects-filter.c | 49 ++
t/t5317-pack-objects-filter-objects.sh | 28 +++
t/t5616-partial-clone.sh | 41 +
t/t6112-rev-list-filters-objects.sh| 13 +++
7 files changed, 150 insertions(+)
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index 7b273635d..5f1672913 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -731,6 +731,11 @@ the requested refs.
+
The form '--filter=sparse:path=' similarly uses a sparse-checkout
specification contained in .
++
+The form '--filter=tree:' omits all blobs and trees whose depth
+from the root tree is >= (minimum depth if an object is located
+at multiple depths in the commits traversed). Currently, only =0
+is supported, which omits all blobs and trees.
--no-filter::
Turn off any previous `--filter=` argument.
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index d259bdb2c..e8da2e858 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -49,6 +49,19 @@ static int gently_parse_list_objects_filter(
return 0;
}
+ } else if (skip_prefix(arg, "tree:", )) {
+ unsigned long depth;
+ if (!git_parse_ulong(v0, ) || depth != 0) {
+ if (errbuf) {
+ strbuf_addstr(
+ errbuf,
+ _("only 'tree:0' is supported"));
+ }
+ return 1;
+ }
+ filter_options->choice = LOFC_TREE_NONE;
+ return 0;
+
} else if (skip_prefix(arg, "sparse:oid=", )) {
struct object_context oc;
struct object_id sparse_oid;
diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
index a61f8..af64e5c66 100644
--- a/list-objects-filter-options.h
+++ b/list-objects-filter-options.h
@@ -10,6 +10,7 @@ enum list_objects_filter_choice {
LOFC_DISABLED = 0,
LOFC_BLOB_NONE,
LOFC_BLOB_LIMIT,
+ LOFC_TREE_NONE,
LOFC_SPARSE_OID,
LOFC_SPARSE_PATH,
LOFC__COUNT /* must be last */
diff --git a/list-objects-filter.c b/list-objects-filter.c
index 5f8b1a002..09b2b05d5 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -79,6 +79,54 @@ static void *filter_blobs_none__init(
return d;
}
+/*
+ * A filter for list-objects to omit ALL trees and blobs from the traversal.
+ * Can OPTIONALLY collect a list of the omitted OIDs.
+ */
+struct filter_trees_none_data {
+ struct oidset *omits;
+};
+
+static enum list_objects_filter_result filter_trees_none(
+ enum list_objects_filter_situation filter_situation,
+ struct object *obj,
+ const char *pathname,
+ const char *filename,
+ void *filter_data_)
+{
+ struct filter_trees_none_data *filter_data = filter_data_;
+
+ switch (filter_situation) {
+ default:
+ BUG("unknown filter_situation: %d", filter_situation);
+
+ case LOFS_BEGIN_TREE:
+ case LOFS_BLOB:
+ if (filter_data->omits)
+ oidset_insert(filter_data->omits, >oid);
+ return LOFR_MARK_SEEN; /* but not LOFR_DO_SHOW (hard omit) */
+
+ case LOFS_END_TREE:
+ assert(obj->type == OBJ_TREE);
+ return LOFR_ZERO;
+
+ }
+}
+
+static void* filter_trees_none__init(
+ struct oidset *omitted,
+ struct list_objects_filter_options *filter_options,
+ filter_object_fn *filter_fn,
+ filter_free_fn *filter_free_fn)
+{
+ struct filter_trees_none_data *d = xcalloc(1, sizeof(*d));
+ d->omits = omitted;
+
+ *filter_fn = filter_trees_none;
+
[PATCH v9 4/8] rev-list: handle missing tree objects properly
Previously, we assumed only blob objects could be missing. This patch
makes rev-list handle missing trees like missing blobs. The --missing=*
and --exclude-promisor-objects flags now work for trees as they already
do for blobs. This is demonstrated in t6112.
Signed-off-by: Matthew DeVore
---
builtin/rev-list.c | 11 ---
list-objects.c | 11 +--
revision.h | 15 +
t/t0410-partial-clone.sh | 45 ++
t/t5317-pack-objects-filter-objects.sh | 13
t/t6112-rev-list-filters-objects.sh| 17 ++
6 files changed, 105 insertions(+), 7 deletions(-)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 5b07f3f4a..49d6deed7 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -6,6 +6,7 @@
#include "list-objects.h"
#include "list-objects-filter.h"
#include "list-objects-filter-options.h"
+#include "object.h"
#include "object-store.h"
#include "pack.h"
#include "pack-bitmap.h"
@@ -209,7 +210,8 @@ static inline void finish_object__ma(struct object *obj)
*/
switch (arg_missing_action) {
case MA_ERROR:
- die("missing blob object '%s'", oid_to_hex(>oid));
+ die("missing %s object '%s'",
+ type_name(obj->type), oid_to_hex(>oid));
return;
case MA_ALLOW_ANY:
@@ -222,8 +224,8 @@ static inline void finish_object__ma(struct object *obj)
case MA_ALLOW_PROMISOR:
if (is_promisor_object(>oid))
return;
- die("unexpected missing blob object '%s'",
- oid_to_hex(>oid));
+ die("unexpected missing %s object '%s'",
+ type_name(obj->type), oid_to_hex(>oid));
return;
default:
@@ -235,7 +237,7 @@ static inline void finish_object__ma(struct object *obj)
static int finish_object(struct object *obj, const char *name, void *cb_data)
{
struct rev_list_info *info = cb_data;
- if (obj->type == OBJ_BLOB && !has_object_file(>oid)) {
+ if (!has_object_file(>oid)) {
finish_object__ma(obj);
return 1;
}
@@ -373,6 +375,7 @@ int cmd_rev_list(int argc, const char **argv, const char
*prefix)
init_revisions(, prefix);
revs.abbrev = DEFAULT_ABBREV;
revs.commit_format = CMIT_FMT_UNSPECIFIED;
+ revs.do_not_die_on_missing_tree = 1;
/*
* Scan the argument list before invoking setup_revisions(), so that we
diff --git a/list-objects.c b/list-objects.c
index f9b51db7a..243192af5 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -143,6 +143,7 @@ static void process_tree(struct traversal_context *ctx,
struct rev_info *revs = ctx->revs;
int baselen = base->len;
enum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW;
+ int failed_parse;
if (!revs->tree_objects)
return;
@@ -150,7 +151,9 @@ static void process_tree(struct traversal_context *ctx,
die("bad tree object");
if (obj->flags & (UNINTERESTING | SEEN))
return;
- if (parse_tree_gently(tree, 1) < 0) {
+
+ failed_parse = parse_tree_gently(tree, 1);
+ if (failed_parse) {
if (revs->ignore_missing_links)
return;
@@ -163,7 +166,8 @@ static void process_tree(struct traversal_context *ctx,
is_promisor_object(>oid))
return;
- die("bad tree object %s", oid_to_hex(>oid));
+ if (!revs->do_not_die_on_missing_tree)
+ die("bad tree object %s", oid_to_hex(>oid));
}
strbuf_addstr(base, name);
@@ -178,7 +182,8 @@ static void process_tree(struct traversal_context *ctx,
if (base->len)
strbuf_addch(base, '/');
- process_tree_contents(ctx, tree, base);
+ if (!failed_parse)
+ process_tree_contents(ctx, tree, base);
if (!(obj->flags & USER_GIVEN) && ctx->filter_fn) {
r = ctx->filter_fn(LOFS_END_TREE, obj,
diff --git a/revision.h b/revision.h
index 007278cc1..5910613cb 100644
--- a/revision.h
+++ b/revision.h
@@ -126,6 +126,21 @@ struct rev_info {
line_level_traverse:1,
tree_blobs_in_commit_order:1,
+ /*
+* Blobs are shown without regard for their existence.
+* But not so for trees: unless exclude_promisor_objects
+* is set and the tree in question is a promisor object;
+* OR ignore_missing_links is set, the revision walker
+* dies with a "bad tree object HASH" message when
+* encountering a missing tree. For callers that can
+* handle missing trees
[PATCH v9 2/8] list-objects: refactor to process_tree_contents
This will be used in a follow-up patch to reduce indentation needed when
invoking the logic conditionally. i.e. rather than:
if (foo) {
while (...) {
/* this is very indented */
}
}
we will have:
if (foo)
process_tree_contents(...);
Signed-off-by: Matthew DeVore
---
list-objects.c | 68 ++
1 file changed, 41 insertions(+), 27 deletions(-)
diff --git a/list-objects.c b/list-objects.c
index 584518a3f..ccc529e5e 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -94,6 +94,46 @@ static void process_gitlink(struct traversal_context *ctx,
/* Nothing to do */
}
+static void process_tree(struct traversal_context *ctx,
+struct tree *tree,
+struct strbuf *base,
+const char *name);
+
+static void process_tree_contents(struct traversal_context *ctx,
+ struct tree *tree,
+ struct strbuf *base)
+{
+ struct tree_desc desc;
+ struct name_entry entry;
+ enum interesting match = ctx->revs->diffopt.pathspec.nr == 0 ?
+ all_entries_interesting : entry_not_interesting;
+
+ init_tree_desc(, tree->buffer, tree->size);
+
+ while (tree_entry(, )) {
+ if (match != all_entries_interesting) {
+ match = tree_entry_interesting(, base, 0,
+
>revs->diffopt.pathspec);
+ if (match == all_entries_not_interesting)
+ break;
+ if (match == entry_not_interesting)
+ continue;
+ }
+
+ if (S_ISDIR(entry.mode))
+ process_tree(ctx,
+lookup_tree(the_repository, entry.oid),
+base, entry.path);
+ else if (S_ISGITLINK(entry.mode))
+ process_gitlink(ctx, entry.oid->hash,
+ base, entry.path);
+ else
+ process_blob(ctx,
+lookup_blob(the_repository, entry.oid),
+base, entry.path);
+ }
+}
+
static void process_tree(struct traversal_context *ctx,
struct tree *tree,
struct strbuf *base,
@@ -101,10 +141,6 @@ static void process_tree(struct traversal_context *ctx,
{
struct object *obj = >object;
struct rev_info *revs = ctx->revs;
- struct tree_desc desc;
- struct name_entry entry;
- enum interesting match = revs->diffopt.pathspec.nr == 0 ?
- all_entries_interesting: entry_not_interesting;
int baselen = base->len;
enum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW;
int gently = revs->ignore_missing_links ||
@@ -144,29 +180,7 @@ static void process_tree(struct traversal_context *ctx,
if (base->len)
strbuf_addch(base, '/');
- init_tree_desc(, tree->buffer, tree->size);
-
- while (tree_entry(, )) {
- if (match != all_entries_interesting) {
- match = tree_entry_interesting(, base, 0,
- >diffopt.pathspec);
- if (match == all_entries_not_interesting)
- break;
- if (match == entry_not_interesting)
- continue;
- }
-
- if (S_ISDIR(entry.mode))
- process_tree(ctx,
-lookup_tree(the_repository, entry.oid),
-base, entry.path);
- else if (S_ISGITLINK(entry.mode))
- process_gitlink(ctx, entry.oid->hash, base, entry.path);
- else
- process_blob(ctx,
-lookup_blob(the_repository, entry.oid),
-base, entry.path);
- }
+ process_tree_contents(ctx, tree, base);
if (!(obj->flags & USER_GIVEN) && ctx->filter_fn) {
r = ctx->filter_fn(LOFS_END_TREE, obj,
--
2.19.0.444.g18242da7ef-goog
[PATCH v9 3/8] list-objects: always parse trees gently
If parsing fails when revs->ignore_missing_links and
revs->exclude_promisor_objects are both false, we print the OID anyway
in the die("bad tree object...") call, so any message printed by
parse_tree_gently() is superfluous.
Signed-off-by: Matthew DeVore
---
list-objects.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/list-objects.c b/list-objects.c
index ccc529e5e..f9b51db7a 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -143,8 +143,6 @@ static void process_tree(struct traversal_context *ctx,
struct rev_info *revs = ctx->revs;
int baselen = base->len;
enum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW;
- int gently = revs->ignore_missing_links ||
-revs->exclude_promisor_objects;
if (!revs->tree_objects)
return;
@@ -152,7 +150,7 @@ static void process_tree(struct traversal_context *ctx,
die("bad tree object");
if (obj->flags & (UNINTERESTING | SEEN))
return;
- if (parse_tree_gently(tree, gently) < 0) {
+ if (parse_tree_gently(tree, 1) < 0) {
if (revs->ignore_missing_links)
return;
--
2.19.0.444.g18242da7ef-goog
[PATCH v9 0/8] filter: support for excluding all trees and blobs
Since v8, I cleaned up the test scripts in the following ways: - correct order of expect/actual arguments to test_cmp - correct pipe placement - put flags before positional arguments Also, removed some junk in the commit message of the 5th patch. Thank you, Matthew DeVore (8): list-objects: store common func args in struct list-objects: refactor to process_tree_contents list-objects: always parse trees gently rev-list: handle missing tree objects properly revision: mark non-user-given objects instead list-objects-filter: use BUG rather than die list-objects-filter-options: do not over-strbuf_init list-objects-filter: implement filter tree:0 Documentation/rev-list-options.txt | 5 + builtin/rev-list.c | 11 +- list-objects-filter-options.c | 19 +- list-objects-filter-options.h | 1 + list-objects-filter.c | 60 ++- list-objects.c | 232 + revision.c | 1 - revision.h | 26 ++- t/t0410-partial-clone.sh | 45 + t/t5317-pack-objects-filter-objects.sh | 41 + t/t5616-partial-clone.sh | 41 + t/t6112-rev-list-filters-objects.sh| 42 + 12 files changed, 396 insertions(+), 128 deletions(-) -- 2.19.0.444.g18242da7ef-goog
[PATCH v9 1/8] list-objects: store common func args in struct
This will make utility functions easier to create, as done by the next
patch.
Signed-off-by: Matthew DeVore
---
list-objects.c | 158 +++--
1 file changed, 74 insertions(+), 84 deletions(-)
diff --git a/list-objects.c b/list-objects.c
index c99c47ac1..584518a3f 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -12,20 +12,25 @@
#include "packfile.h"
#include "object-store.h"
-static void process_blob(struct rev_info *revs,
+struct traversal_context {
+ struct rev_info *revs;
+ show_object_fn show_object;
+ show_commit_fn show_commit;
+ void *show_data;
+ filter_object_fn filter_fn;
+ void *filter_data;
+};
+
+static void process_blob(struct traversal_context *ctx,
struct blob *blob,
-show_object_fn show,
struct strbuf *path,
-const char *name,
-void *cb_data,
-filter_object_fn filter_fn,
-void *filter_data)
+const char *name)
{
struct object *obj = >object;
size_t pathlen;
enum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW;
- if (!revs->blob_objects)
+ if (!ctx->revs->blob_objects)
return;
if (!obj)
die("bad blob object");
@@ -41,21 +46,21 @@ static void process_blob(struct rev_info *revs,
* may cause the actual filter to report an incomplete list
* of missing objects.
*/
- if (revs->exclude_promisor_objects &&
+ if (ctx->revs->exclude_promisor_objects &&
!has_object_file(>oid) &&
is_promisor_object(>oid))
return;
pathlen = path->len;
strbuf_addstr(path, name);
- if (!(obj->flags & USER_GIVEN) && filter_fn)
- r = filter_fn(LOFS_BLOB, obj,
- path->buf, >buf[pathlen],
- filter_data);
+ if (!(obj->flags & USER_GIVEN) && ctx->filter_fn)
+ r = ctx->filter_fn(LOFS_BLOB, obj,
+ path->buf, >buf[pathlen],
+ ctx->filter_data);
if (r & LOFR_MARK_SEEN)
obj->flags |= SEEN;
if (r & LOFR_DO_SHOW)
- show(obj, path->buf, cb_data);
+ ctx->show_object(obj, path->buf, ctx->show_data);
strbuf_setlen(path, pathlen);
}
@@ -81,26 +86,21 @@ static void process_blob(struct rev_info *revs,
* the link, and how to do it. Whether it necessarily makes
* any sense what-so-ever to ever do that is another issue.
*/
-static void process_gitlink(struct rev_info *revs,
+static void process_gitlink(struct traversal_context *ctx,
const unsigned char *sha1,
- show_object_fn show,
struct strbuf *path,
- const char *name,
- void *cb_data)
+ const char *name)
{
/* Nothing to do */
}
-static void process_tree(struct rev_info *revs,
+static void process_tree(struct traversal_context *ctx,
struct tree *tree,
-show_object_fn show,
struct strbuf *base,
-const char *name,
-void *cb_data,
-filter_object_fn filter_fn,
-void *filter_data)
+const char *name)
{
struct object *obj = >object;
+ struct rev_info *revs = ctx->revs;
struct tree_desc desc;
struct name_entry entry;
enum interesting match = revs->diffopt.pathspec.nr == 0 ?
@@ -133,14 +133,14 @@ static void process_tree(struct rev_info *revs,
}
strbuf_addstr(base, name);
- if (!(obj->flags & USER_GIVEN) && filter_fn)
- r = filter_fn(LOFS_BEGIN_TREE, obj,
- base->buf, >buf[baselen],
- filter_data);
+ if (!(obj->flags & USER_GIVEN) && ctx->filter_fn)
+ r = ctx->filter_fn(LOFS_BEGIN_TREE, obj,
+ base->buf, >buf[baselen],
+ ctx->filter_data);
if (r & LOFR_MARK_SEEN)
obj->flags |= SEEN;
if (r & LOFR_DO_SHOW)
- show(obj, base->buf, cb_data);
+ ctx->show_object(obj, base->buf, ctx->show_data);
if (base->len)
strbuf_addch(base, '/');
@@ -157,29 +157,25 @@ static void process_tree(struct rev_info *revs,
}
if (S_ISDIR(entry.mode))
- process_tree(revs,
+ process_tree(ctx,
lookup_tree(the_repository, entry.oid),
-
Re: [PATCH v2 1/2] connected: document connectivity in partial clones
Jonathan Tan writes:
> In acb0c57260 ("fetch: support filters", 2017-12-08), check_connected()
> was extended to allow objects to either be promised to be available (if
> the repository is a partial clone) or to be present; previously, this
> function required the latter. However, this change was not reflected in
> the documentation of that function. Update the documentation
> accordingly.
>
> Signed-off-by: Jonathan Tan
> ---
> connected.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Very much makes sense. I think this is sufficient clarification to
allay your earlier worry of having to have a huge in-code comment to
prevent the "must exist" loop from getting removed.
> diff --git a/connected.h b/connected.h
> index e4c961817..8d5a6b3ad 100644
> --- a/connected.h
> +++ b/connected.h
> @@ -51,9 +51,9 @@ struct check_connected_options {
> #define CHECK_CONNECTED_INIT { 0 }
>
> /*
> - * Make sure that our object store has all the commits necessary to
> - * connect the ancestry chain to some of our existing refs, and all
> - * the trees and blobs that these commits use.
> + * Make sure that all given objects and all objects reachable from them
> + * either exist in our object store or (if the repository is a partial
> + * clone) are promised to be available.
> *
> * Return 0 if Ok, non zero otherwise (i.e. some missing objects)
> *
Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 2:47 PM Taylor Blau wrote:
> When in a repository containing one or more alternates, Git would
> sometimes like to list references from its alternates. For example, 'git
> receive-pack' list the objects pointed to by alternate references as
> special ".have" references.
> [...]
> Signed-off-by: Taylor Blau
> ---
> diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> @@ -0,0 +1,54 @@
> +expect_haves () {
> + printf "%s .have\n" $(git rev-parse $@) >expect
> +}
Magic quoting behavior only kicks in when $@ is itself quoted, so this
should be:
printf "%s .have\n" $(git rev-parse "$@") >expect
However, as it's unlikely that you need magic quoting in this case,
you might get by with plain $* (unquoted).
Re: [PATCH v2 1/2] commit-graph write: add progress output
On 9/7/2018 2:29 PM, Ævar Arnfjörð Bjarmason wrote: -void write_commit_graph_reachable(const char *obj_dir, int append); +void write_commit_graph_reachable(const char *obj_dir, int append, + int report_progress); void write_commit_graph(const char *obj_dir, struct string_list *pack_indexes, struct string_list *commit_hex, - int append); + int append, int report_progress); int verify_commit_graph(struct repository *r, struct commit_graph *g); Junio, The above prototype change seems to have created a semantic conflict with ds/commit-graph-tests (859fdc "commit-graph: define GIT_TEST_COMMIT_GRAPH") because when GIT_TEST_COMMIT_GRAPH is set, we call write_commit_graph_reachable() but the final parameter was resolved to be "1" instead of "0". This causes t3420-rebase-autostash.sh to fail, as that test watches the full output of the rebase command, including commit runs. The following patch fixes the problem, but could probably be squashed into a merge or other commit. Thanks, -Stolee -->8-- From: Derrick Stolee Date: Fri, 21 Sep 2018 19:57:36 + Subject: [PATCH] commit: quietly write commit-graph in tests The GIT_TEST_COMMIT_GRAPH environment variable causes git-commit to write a commit-graph file on every execution. Recently, we added progress output when writing the commit-graph. This conflicts with some expected output during some tests, so avoid writing progress if writing a commit-graph this way. Signed-off-by: Derrick Stolee --- builtin/commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/commit.c b/builtin/commit.c index 2a49ab4917..764664d832 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1660,7 +1660,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) "not exceeded, and then \"git reset HEAD\" to recover.")); if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0)) - write_commit_graph_reachable(get_object_directory(), 0, 1); + write_commit_graph_reachable(get_object_directory(), 0, 0); rerere(0); run_command_v_opt(argv_gc_auto, RUN_GIT_CMD); -- 2.19.0
Re: [PATCH 2/3] transport.c: introduce core.alternateRefsCommand
Taylor Blau writes:
> In fact, I think that we can go even further: since we don't need to
> catch the beginning '^.*' (without -o), we can instead:
>
> extract_haves () {
> depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
> }
Do not pipe grep into sed, unless you have an overly elaborate set
of patterns to filter with, e.g. something along the lines of...
sed -ne '/\.have/s/...//p'
Re: [PATCH] fetch: Ensure that fetch.recurseSubmodules overrides submodule.recurse.
On Fri, Sep 21, 2018 at 12:00 PM Marc Branchaud wrote:
>
> Also document this fact.
>
> Signed-off-by: Marc Branchaud
> ---
>
> I ran into this bug when I had both fetch.recurseSubmodules=on-demand and
> submodule.recurse=true, and submodule.recurse was set *after*
> fetch.recurseSubmodules in my config.
>
> The fix ensures that fetch.recurseSubmodules always overrides
> submodule.recurse. If neither is set then fetch still behaves as if
> fetch.recurseSubmodules=on-demand (the documented default).
At least the second paragraph is valuable information in the commit
message, so maybe add it there? I am not sure if the first paragraph is
a good part for the commit message, but maybe helps for writing a test?
> + reference. This option overrides the more general submodule.recurse
> + option, for the `fetch` command.
>
> fetch.fsckObjects::
> If it is set to true, git-fetch-pack will check all fetched
> @@ -3465,7 +3466,8 @@ submodule.active::
> submodule.recurse::
> Specifies if commands recurse into submodules by default. This
> applies to all commands that have a `--recurse-submodules` option,
> - except `clone`.
> + except `clone`. Also, the `fetch` command's behaviour can be
> specified
> + independently with the fetch.recurseSubmodules option.
There is also push.recurseSubmodules, which should behave similarly?
The series that introduced submodule.recurse ends with 58f4203e7db
(builtin/fetch.c: respect 'submodule.recurse' option, 2017-05-31)
(sb/submodule-blanket-recursive)
seems to have overlooked this only for fetch/push, as the other
commands (checkout, read-tree, reset, grep) do not have their
own specific setting to recurse.
> @@ -88,6 +90,7 @@ static int git_fetch_config(const char *k, const char *v,
> void *cb)
> max_children = parse_submodule_fetchjobs(k, v);
> return 0;
> } else if (!strcmp(k, "fetch.recursesubmodules")) {
> + recurse_submodules_set_explicitly = 1;
the command line option also overried explicitely, but that
is ensured via the program flow (parse_config happens after
git_config to overlay options, which itself was pre-seeded
with fetch_config_from_gitmodules).
I briefly wondered if this overlaying approach would be better
(i.e. first do git_config with more generic option, and then
again with the more detailed option) as it would save one
global variable, but the downsides are terrible (way more
work to do, more code and such), so I think having a global
makes sense and gets the job done.
Ideally instead of a global we'd have this flag stored in
the repository struct, as eventually in the long run,
fetch_populated_submodules could happen in-process
instead of spawning fetch processes for each submodule
(and their nested submodules which may be configured
differently). But for now the global will do.
Thanks!
Stefan
[PATCH] fetch: Ensure that fetch.recurseSubmodules overrides submodule.recurse.
Also document this fact.
Signed-off-by: Marc Branchaud
---
I ran into this bug when I had both fetch.recurseSubmodules=on-demand and
submodule.recurse=true, and submodule.recurse was set *after*
fetch.recurseSubmodules in my config.
The fix ensures that fetch.recurseSubmodules always overrides
submodule.recurse. If neither is set then fetch still behaves as if
fetch.recurseSubmodules=on-demand (the documented default).
I'm not sure if this is the most elegant implementation, but it gets the job
done.
M.
Documentation/config.txt | 6 --
builtin/fetch.c | 5 -
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index eb66a11975..67b0adc1d4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1514,7 +1514,8 @@ fetch.recurseSubmodules::
recurse at all when set to false. When set to 'on-demand' (the default
value), fetch and pull will only recurse into a populated submodule
when its superproject retrieves a commit that updates the submodule's
- reference.
+ reference. This option overrides the more general submodule.recurse
+ option, for the `fetch` command.
fetch.fsckObjects::
If it is set to true, git-fetch-pack will check all fetched
@@ -3465,7 +3466,8 @@ submodule.active::
submodule.recurse::
Specifies if commands recurse into submodules by default. This
applies to all commands that have a `--recurse-submodules` option,
- except `clone`.
+ except `clone`. Also, the `fetch` command's behaviour can be specified
+ independently with the fetch.recurseSubmodules option.
Defaults to false.
submodule.fetchJobs::
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 61bec5d213..08b8bf2741 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -60,6 +60,7 @@ static struct transport *gsecondary;
static const char *submodule_prefix = "";
static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
static int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;
+static int recurse_submodules_set_explicitly = 0;
static int shown_url = 0;
static struct refspec refmap = REFSPEC_INIT_FETCH;
static struct list_objects_filter_options filter_options;
@@ -78,7 +79,8 @@ static int git_fetch_config(const char *k, const char *v,
void *cb)
return 0;
}
- if (!strcmp(k, "submodule.recurse")) {
+ if (!strcmp(k, "submodule.recurse") &&
+ !recurse_submodules_set_explicitly) {
int r = git_config_bool(k, v) ?
RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
recurse_submodules = r;
@@ -88,6 +90,7 @@ static int git_fetch_config(const char *k, const char *v,
void *cb)
max_children = parse_submodule_fetchjobs(k, v);
return 0;
} else if (!strcmp(k, "fetch.recursesubmodules")) {
+ recurse_submodules_set_explicitly = 1;
recurse_submodules = parse_fetch_recurse_submodules_arg(k, v);
return 0;
}
--
2.19.0.1.g5109f9487a
[PATCH v2 1/3] transport.c: extract 'fill_alternate_refs_command'
To list alternate references, 'read_alternate_refs' creates a child
process running 'git for-each-ref' in the alternate's Git directory.
Prepare to run other commands besides 'git for-each-ref' by introducing
and moving the relevant code from 'read_alternate_refs' to
'fill_alternate_refs_command'.
Signed-off-by: Taylor Blau
---
transport.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/transport.c b/transport.c
index 1c76d64aba..24ae3f375d 100644
--- a/transport.c
+++ b/transport.c
@@ -1325,6 +1325,17 @@ char *transport_anonymize_url(const char *url)
return xstrdup(url);
}
+static void fill_alternate_refs_command(struct child_process *cmd,
+ const char *repo_path)
+{
+ cmd->git_cmd = 1;
+ argv_array_pushf(>args, "--git-dir=%s", repo_path);
+ argv_array_push(>args, "for-each-ref");
+ argv_array_push(>args, "--format=%(objectname) %(refname)");
+ cmd->env = local_repo_env;
+ cmd->out = -1;
+}
+
static void read_alternate_refs(const char *path,
alternate_ref_fn *cb,
void *data)
@@ -1333,12 +1344,7 @@ static void read_alternate_refs(const char *path,
struct strbuf line = STRBUF_INIT;
FILE *fh;
- cmd.git_cmd = 1;
- argv_array_pushf(, "--git-dir=%s", path);
- argv_array_push(, "for-each-ref");
- argv_array_push(, "--format=%(objectname) %(refname)");
- cmd.env = local_repo_env;
- cmd.out = -1;
+ fill_alternate_refs_command(, path);
if (start_command())
return;
--
2.19.0.221.g150f307af
[PATCH v2 3/3] transport.c: introduce core.alternateRefsPrefixes
The recently-introduced "core.alternateRefsCommand" allows callers to
specify with high flexibility the tips that they wish to advertise from
alternates. This flexibility comes at the cost of some inconvenience
when the caller only wishes to limit the advertisement to one or more
prefixes.
For example, to advertise only tags, a caller using
'core.alternateRefsCommand' would have to do:
$ git config core.alternateRefsCommand ' \
git -C "$1" for-each-ref refs/tags \
--format="%(objectname) %(refname)" \
'
The above is cumbersome to write, so let's introduce a
"core.alternateRefsPrefixes" to address this common case. Instead, the
caller can run:
$ git config core.alternateRefsPrefixes 'refs/tags'
Which will behave identically to the longer example using
"core.alternateRefsCommand".
Since the value of "core.alternateRefsPrefixes" is appended to 'git
for-each-ref' and then executed, include a "--" before taking the
configured value to avoid misinterpreting arguments as flags to 'git
for-each-ref'.
In the case that the caller wishes to specify multiple prefixes, they
may separate them by whitespace. If "core.alternateRefsCommand" is set,
it will take precedence over "core.alternateRefsPrefixes".
Signed-off-by: Taylor Blau
---
Documentation/config.txt | 7 +++
t/t5410-receive-pack.sh | 8
transport.c | 5 +
3 files changed, 20 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 526557e494..7df6c22925 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -627,6 +627,13 @@ alternate's references as ".have"'s. For example, to only
advertise branch
heads, configure `core.alternateRefsCommand` to the path of a script which runs
`git --git-dir="$1" for-each-ref refs/heads`.
+core.alternateRefsPrefixes::
+ When listing references from an alternate, list only references that
begin
+ with the given prefix. Prefixes match as if they were given as
arguments to
+ linkgit:git-for-each-ref[1]. To list multiple prefixes, separate them
with
+ whitespace. If `core.alternateRefsCommand` is set, setting
+ `core.alternateRefsPrefixes` has no effect.
+
core.bare::
If true this repository is assumed to be 'bare' and has no
working directory associated with it. If this is the case a
diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
index 2f21f1cb8f..b656c9b30c 100755
--- a/t/t5410-receive-pack.sh
+++ b/t/t5410-receive-pack.sh
@@ -51,4 +51,12 @@ test_expect_success 'with core.alternateRefsCommand' '
test_cmp expect actual.haves
'
+test_expect_success 'with core.alternateRefsPrefixes' '
+ test_config -C fork core.alternateRefsPrefixes "refs/tags" &&
+ expect_haves one three two >expect &&
+ printf "" | git receive-pack fork >actual &&
+ extract_haves actual.haves &&
+ test_cmp expect actual.haves
+'
+
test_done
diff --git a/transport.c b/transport.c
index e7d2cdf00b..9323e5c3cd 100644
--- a/transport.c
+++ b/transport.c
@@ -1341,6 +1341,11 @@ static void fill_alternate_refs_command(struct
child_process *cmd,
argv_array_pushf(>args, "--git-dir=%s", repo_path);
argv_array_push(>args, "for-each-ref");
argv_array_push(>args, "--format=%(objectname)
%(refname)");
+
+ if (!git_config_get_value("core.alternateRefsPrefixes",
)) {
+ argv_array_push(>args, "--");
+ argv_array_split(>args, value);
+ }
}
cmd->env = local_repo_env;
--
2.19.0.221.g150f307af
[PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand
When in a repository containing one or more alternates, Git would
sometimes like to list references from its alternates. For example, 'git
receive-pack' list the objects pointed to by alternate references as
special ".have" references.
Listing ".have" references is designed to make pushing changes from
upstream to a fork a lightweight operation, by advertising to the pusher
that the fork already has the objects (via its alternate). Thus, the
client can avoid sending them.
However, when the alternate has a pathologically large number of
references, the initial advertisement is too expensive. In fact, it can
dominate any such optimization where the pusher avoids sending certain
objects.
Introduce "core.alternateRefsCommand" in order to provide a facility to
limit or filter alternate references. This can be used, for example, to
filter out "uninteresting" references from the initial advertisement in
the above scenario.
Let the repository that has alternates configure this command to avoid
trusting the alternate to provide us a safe command to run in the shell.
To behave differently on each alternate (e.g., only list tags from
alternate A, only heads from B) provide the path of the alternate as the
first argument.
Signed-off-by: Taylor Blau
---
Documentation/config.txt | 11
t/t5410-receive-pack.sh | 54
transport.c | 19 +++---
3 files changed, 80 insertions(+), 4 deletions(-)
create mode 100755 t/t5410-receive-pack.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 112041f407..526557e494 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -616,6 +616,17 @@ core.preferSymlinkRefs::
This is sometimes needed to work with old scripts that
expect HEAD to be a symbolic link.
+core.alternateRefsCommand::
+ When listing references from an alternate (e.g., in the case of
".have"), use
+ the shell to execute the specified command instead of
+ linkgit:git-for-each-ref[1]. The first argument is the path of the
alternate.
+ Output must be of the form: `%(objectname) SPC %(refname)`.
++
+This is useful when a repository only wishes to advertise some of its
+alternate's references as ".have"'s. For example, to only advertise branch
+heads, configure `core.alternateRefsCommand` to the path of a script which runs
+`git --git-dir="$1" for-each-ref refs/heads`.
+
core.bare::
If true this repository is assumed to be 'bare' and has no
working directory associated with it. If this is the case a
diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
new file mode 100755
index 00..2f21f1cb8f
--- /dev/null
+++ b/t/t5410-receive-pack.sh
@@ -0,0 +1,54 @@
+#!/bin/sh
+
+test_description='git receive-pack test'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit one &&
+ git update-ref refs/heads/a HEAD &&
+ test_commit two &&
+ git update-ref refs/heads/b HEAD &&
+ test_commit three &&
+ git update-ref refs/heads/c HEAD &&
+ git clone --bare . fork &&
+ git clone fork pusher &&
+ (
+ cd fork &&
+ git config receive.advertisealternates true &&
+ cat <<-EOF | git update-ref --stdin &&
+ delete refs/heads/a
+ delete refs/heads/b
+ delete refs/heads/c
+ delete refs/heads/master
+ delete refs/tags/one
+ delete refs/tags/two
+ delete refs/tags/three
+ EOF
+ echo "../../.git/objects" >objects/info/alternates
+ )
+'
+
+expect_haves () {
+ printf "%s .have\n" $(git rev-parse $@) >expect
+}
+
+extract_haves () {
+ depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
+}
+
+test_expect_success 'with core.alternateRefsCommand' '
+ write_script fork/alternate-refs <<-\EOF &&
+ git --git-dir="$1" for-each-ref \
+ --format="%(objectname) %(refname)" \
+ refs/heads/a \
+ refs/heads/c
+ EOF
+ test_config -C fork core.alternateRefsCommand alternate-refs &&
+ expect_haves a c >expect &&
+ printf "" | git receive-pack fork >actual &&
+ extract_haves actual.haves &&
+ test_cmp expect actual.haves
+'
+
+test_done
diff --git a/transport.c b/transport.c
index 24ae3f375d..e7d2cdf00b 100644
--- a/transport.c
+++ b/transport.c
@@ -1328,10 +1328,21 @@ char *transport_anonymize_url(const char *url)
static void fill_alternate_refs_command(struct child_process *cmd,
const char *repo_path)
{
- cmd->git_cmd = 1;
- argv_array_pushf(>args, "--git-dir=%s", repo_path);
- argv_array_push(>args, "for-each-ref");
- argv_array_push(>args, "--format=%(objectname) %(refname)");
+ const char *value;
+
+ if
[PATCH v2 0/3] Filter alternate references
Hi,
Attached is the second re-roll of my series to teach
"core.alternateRefsCommand" and "core.alternateRefsPrefixes".
I have included a range-diff below (which I have taught my scripts to do
by default now), but will summarize the changes as usual:
* Clean up t5410 according to Peff's suggestions in [1]:
* Simplify many `git update-ref -d`'s into one `git update-ref
--stdin`.
* Use `echo >`, instead of `printf >` to write an alternate
repository.
* Avoid placing Git on the left-hand side of a pipe.
* Use 'write_script', instead of embedding the same code in a
lengthy 'test_config'.
* Add a motivating example in Documentation/config.txt, per Peff's
suggestion in [1].
* Use `printf "%s .have\n"` with many arguments instead of another
`cat <<-EOF` block and extract it into `expect_haves`, per [2].
* Do not use `grep -o` in `extract_haves`, thus making it portable.
Per [3].
[1]: https://public-inbox.org/git/20180920193751.gc29...@sigill.intra.peff.net/
[2]:
https://public-inbox.org/git/capig+ct7wtybcqz75wsjmbqiui383yrkqohqblasqkoagvt...@mail.gmail.com/
[3]: https://public-inbox.org/git/xmqqlg7ux0st@gitster-ct.c.googlers.com/
Taylor Blau (3):
transport.c: extract 'fill_alternate_refs_command'
transport.c: introduce core.alternateRefsCommand
transport.c: introduce core.alternateRefsPrefixes
Documentation/config.txt | 18
t/t5410-receive-pack.sh | 62
transport.c | 34 ++
3 files changed, 108 insertions(+), 6 deletions(-)
create mode 100755 t/t5410-receive-pack.sh
Range-diff against v1:
1: 6e3a58afe7 = 1: 6e3a58afe7 transport.c: extract
'fill_alternate_refs_command'
2: 4c4900722c ! 2: 9797f52551 transport.c: introduce core.alternateRefsCommand
@@ -42,6 +42,11 @@
+ the shell to execute the specified command instead of
+ linkgit:git-for-each-ref[1]. The first argument is the path of the
alternate.
+ Output must be of the form: `%(objectname) SPC %(refname)`.
+++
++This is useful when a repository only wishes to advertise some of its
++alternate's references as ".have"'s. For example, to only advertise branch
++heads, configure `core.alternateRefsCommand` to the path of a script
which runs
++`git --git-dir="$1" for-each-ref refs/heads`.
+
core.bare::
If true this repository is assumed to be 'bare' and has no
@@ -70,32 +75,39 @@
+ (
+ cd fork &&
+ git config receive.advertisealternates true &&
-+ git update-ref -d refs/heads/a &&
-+ git update-ref -d refs/heads/b &&
-+ git update-ref -d refs/heads/c &&
-+ git update-ref -d refs/heads/master &&
-+ git update-ref -d refs/tags/one &&
-+ git update-ref -d refs/tags/two &&
-+ git update-ref -d refs/tags/three &&
-+ printf "../../.git/objects" >objects/info/alternates
++ cat <<-EOF | git update-ref --stdin &&
++ delete refs/heads/a
++ delete refs/heads/b
++ delete refs/heads/c
++ delete refs/heads/master
++ delete refs/tags/one
++ delete refs/tags/two
++ delete refs/tags/three
++ EOF
++ echo "../../.git/objects" >objects/info/alternates
+ )
+'
+
++expect_haves () {
++ printf "%s .have\n" $(git rev-parse $@) >expect
++}
++
+extract_haves () {
-+ depacketize - | grep -o '^.* \.have'
++ depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
+}
+
+test_expect_success 'with core.alternateRefsCommand' '
-+ test_config -C fork core.alternateRefsCommand \
-+ "git --git-dir=\"\$1\" for-each-ref \
-+ --format=\"%(objectname) %(refname)\" \
-+ refs/heads/a refs/heads/c;:" &&
-+ cat >expect <<-EOF &&
-+ $(git rev-parse a) .have
-+ $(git rev-parse c) .have
++ write_script fork/alternate-refs <<-\EOF &&
++ git --git-dir="$1" for-each-ref \
++ --format="%(objectname) %(refname)" \
++ refs/heads/a \
++ refs/heads/c
+ EOF
-+ printf "" | git receive-pack fork | extract_haves >actual &&
-+ test_cmp expect actual
++ test_config -C fork core.alternateRefsCommand alternate-refs &&
++ expect_haves a c >expect &&
++ printf "" | git receive-pack fork >actual &&
++ extract_haves actual.haves &&
++ test_cmp expect actual.haves
+'
+
+test_done
3: 3639e90588 ! 3: 6e8f65a16d transport.c: introduce
core.alternateRefsPrefixes
@@ -40,13 +40,14 @@
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@
- linkgit:git-for-each-ref[1]. The first argument is the path of the
alternate.
- Output must be of
[PATCH v2 0/2] Check presence of targets when fetching to partial clone
New in v2: - added patch to clarify in documentation what check_connected() does - renamed quickfetch() to check_exist_and_connected() to better reflect what it does - also updated its documentation; I avoided usage of "wanted objects" and used "fetch targets" instead to clarify that I'm only talking about the direct targets, not all objects referenced by those targets - in check_exist_and_connected() (previously quickfetch()), check existence directly regardless of whether the repository is a partial clone or not This should resolve most of Junio's comments in [1], except that I chose not to modify or rename check_connected(). In this current world, it is true that we usually require existence of ref targets, but that might not be so in the future (having said that, I don't know of any schedules for this future). Also, check_connected() is used in a few places, so such a change would cause some churn in the codebase. So I left this function alone. [1] https://public-inbox.org/git/xmqqy3bvycie@gitster-ct.c.googlers.com/ Jonathan Tan (2): connected: document connectivity in partial clones fetch: in partial clone, check presence of targets builtin/fetch.c | 15 +-- connected.h | 6 +++--- t/t5616-partial-clone.sh | 17 + 3 files changed, 33 insertions(+), 5 deletions(-) -- 2.19.0.444.g18242da7ef-goog
[PATCH v2 2/2] fetch: in partial clone, check presence of targets
When fetching an object that is known as a promisor object to the local
repository, the connectivity check in quickfetch() in builtin/fetch.c
succeeds, causing object transfer to be bypassed. However, this should
not happen if that object is merely promised and not actually present.
Because this happens, when a user invokes "git fetch origin " on
the command-line, the object may not actually be fetched even
though the command returns an exit code of 0. This is a similar issue
(but with a different cause) to the one fixed by a0c9016abd
("upload-pack: send refs' objects despite "filter"", 2018-07-09).
Therefore, update quickfetch() to also directly check for the presence
of all objects to be fetched. Its documentation and name are also
updated to better reflect what it does.
Signed-off-by: Jonathan Tan
---
builtin/fetch.c | 15 +--
t/t5616-partial-clone.sh | 17 +
2 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 61bec5d21..b9e74c129 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -924,10 +924,11 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
* everything we are going to fetch already exists and is connected
* locally.
*/
-static int quickfetch(struct ref *ref_map)
+static int check_exist_and_connected(struct ref *ref_map)
{
struct ref *rm = ref_map;
struct check_connected_options opt = CHECK_CONNECTED_INIT;
+ struct ref *r;
/*
* If we are deepening a shallow clone we already have these
@@ -938,13 +939,23 @@ static int quickfetch(struct ref *ref_map)
*/
if (deepen)
return -1;
+
+ /*
+* check_connected() allows objects to merely be promised, but
+* we need all direct targets to exist.
+*/
+ for (r = rm; r; r = r->next) {
+ if (!has_object_file(>old_oid))
+ return -1;
+ }
+
opt.quiet = 1;
return check_connected(iterate_ref_map, , );
}
static int fetch_refs(struct transport *transport, struct ref *ref_map)
{
- int ret = quickfetch(ref_map);
+ int ret = check_exist_and_connected(ref_map);
if (ret)
ret = transport_fetch_refs(transport, ref_map);
if (!ret)
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index bbbe7537d..359d27d02 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -170,6 +170,23 @@ test_expect_success 'partial clone fetches blobs pointed
to by refs even if norm
git -C dst fsck
'
+test_expect_success 'fetch what is specified on CLI even if already promised' '
+ rm -rf src dst.git &&
+ git init src &&
+ test_commit -C src foo &&
+ test_config -C src uploadpack.allowfilter 1 &&
+ test_config -C src uploadpack.allowanysha1inwant 1 &&
+
+ git hash-object --stdin blob &&
+
+ git clone --bare --filter=blob:none "file://$(pwd)/src" dst.git &&
+ git -C dst.git rev-list --objects --quiet --missing=print HEAD
>missing_before &&
+ grep "?$(cat blob)" missing_before &&
+ git -C dst.git fetch origin $(cat blob) &&
+ git -C dst.git rev-list --objects --quiet --missing=print HEAD
>missing_after &&
+ ! grep "?$(cat blob)" missing_after
+'
+
. "$TEST_DIRECTORY"/lib-httpd.sh
start_httpd
--
2.19.0.444.g18242da7ef-goog
[PATCH v2 1/2] connected: document connectivity in partial clones
In acb0c57260 ("fetch: support filters", 2017-12-08), check_connected()
was extended to allow objects to either be promised to be available (if
the repository is a partial clone) or to be present; previously, this
function required the latter. However, this change was not reflected in
the documentation of that function. Update the documentation
accordingly.
Signed-off-by: Jonathan Tan
---
connected.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/connected.h b/connected.h
index e4c961817..8d5a6b3ad 100644
--- a/connected.h
+++ b/connected.h
@@ -51,9 +51,9 @@ struct check_connected_options {
#define CHECK_CONNECTED_INIT { 0 }
/*
- * Make sure that our object store has all the commits necessary to
- * connect the ancestry chain to some of our existing refs, and all
- * the trees and blobs that these commits use.
+ * Make sure that all given objects and all objects reachable from them
+ * either exist in our object store or (if the repository is a partial
+ * clone) are promised to be available.
*
* Return 0 if Ok, non zero otherwise (i.e. some missing objects)
*
--
2.19.0.444.g18242da7ef-goog
Re: [PATCH 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 01:48:25PM -0400, Taylor Blau wrote:
> On Fri, Sep 21, 2018 at 09:39:14AM -0700, Junio C Hamano wrote:
> > Taylor Blau writes:
> >
> > > +extract_haves () {
> > > + depacketize - | grep -o '^.* \.have'
> >
> > Not portable, isn't it?
> >
> > cf. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html
>
> Good catch. Definitely not portable, per the link that you shared above.
>
> Since 'depacketize()' will give us a "\0", we can pull it and anything
> after it out with 'sed', instead. Any lines that don't contain a "\0"
> only contain an OID and the literal, ".have", and are fine as-is.
>
> Something like this:
>
> extract_haves () {
> depacketize - | grep '^.* \.have' | sed -e 's/\\0.*$//g'
> }
>
> Harder to read--at least for me--but infinitely more portable.
In fact, I think that we can go even further: since we don't need to
catch the beginning '^.*' (without -o), we can instead:
extract_haves () {
depacketize - | grep '\.have' | sed -e 's/\\0.*$//g'
}
Thanks,
Taylor
Re: [PATCH 3/3] transport.c: introduce core.alternateRefsPrefixes
On Fri, Sep 21, 2018 at 09:45:11AM -0700, Junio C Hamano wrote:
> Taylor Blau writes:
>
> > ...' block with your suggestion above. It's tempting to introduce it as:
> >
> > expect_haves() {
> > printf "%s .have\n" $(git rev-parse -- $@)
> > }
> >
> > And call it as:
> >
> > expect_haves one three two >expect
> >
> > But I'm not sure whether I think that this is better or worse than
> > writing it twice inline.
>
> If the expected pattern is expected to stay to be just a sequence of
> " .have" and nothing else for the foreseeable future, I think
> it is a good idea to introduce such a helper function. Spelling it
> out at the use site, e.g.
>
> printf "%s .have\n" $(git rev-parse a b c) >expect
>
> will become cumbersome once the set of objects you need to show
> starts growing.
That's a good reason, and I hadn't thought of it.
> expect_haves a b c >expect
>
> would be shorter, of course. And as long as we expect to have ONLY
> " .have" lines and nothing else, there is no downside that the
> details of the format is hidden away inside the helper.
Yeah, I don't expect this to to change much at all, so I think that
'expect_haves()' is good.
Thanks,
Taylor
Re: [PATCH 2/3] transport.c: introduce core.alternateRefsCommand
On Fri, Sep 21, 2018 at 09:39:14AM -0700, Junio C Hamano wrote:
> Taylor Blau writes:
>
> > +extract_haves () {
> > + depacketize - | grep -o '^.* \.have'
>
> Not portable, isn't it?
>
> cf. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html
Good catch. Definitely not portable, per the link that you shared above.
Since 'depacketize()' will give us a "\0", we can pull it and anything
after it out with 'sed', instead. Any lines that don't contain a "\0"
only contain an OID and the literal, ".have", and are fine as-is.
Something like this:
extract_haves () {
depacketize - | grep '^.* \.have' | sed -e 's/\\0.*$//g'
}
Harder to read--at least for me--but infinitely more portable.
I'll wait until a little later today, and then send you v2. Thanks for
reviewing :-).
Thanks,
Taylor
Re: [PATCH 9/9] commit-reach.h: add missing declarations (hdr-check)
On 9/20/2018 11:35 AM, Ramsay Jones wrote:
On 20/09/18 00:38, Derrick Stolee wrote:
On 9/18/2018 8:15 PM, Ramsay Jones wrote:
Signed-off-by: Ramsay Jones
---
commit-reach.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/commit-reach.h b/commit-reach.h
index 7d313e2975..f41d8f6ba3 100644
--- a/commit-reach.h
+++ b/commit-reach.h
@@ -1,12 +1,13 @@
#ifndef __COMMIT_REACH_H__
#define __COMMIT_REACH_H__
+#include "commit.h"
#include "commit-slab.h"
-struct commit;
struct commit_list;
-struct contains_cache;
Interesting that you needed all of commit.h here, and these 'struct commit' and
'struct contains_cache' were not enough. Was there a reason you needed the
entire header file instead of just adding a missing struct declaration?
Actually, the original version of this patch didn't add that
include! I changed my mind just before sending this series
out, since I felt the original patch was conflating two issues.
The reason for the #include of 'commit.h' in this patch, but
not with my original patch, has to to with the inline functions
used in the commit-slab implementation. My original patch used
the 'commit-slab-{decl,impl}.h' header files to sidestep the
need for the definition of 'struct commit'.
I have included an 'RFC on-top' patch below to show you what I
had in mind. NOTE: I had not finished writing the commit message
and you could argue that the 'implement' macro should go in the
ref-filter.c file, since that is were the contains_cache is
'defined and initialised'. I had not intended to send this to
the list ... :-D
Note that, if you compile with optimizations disabled, then
run this script:
$ cat -n dup-static.sh
1 #!/bin/sh
2
3 nm $1 | grep ' t ' | cut -d' ' -f3 | sort | uniq -c |
4 sort -rn | grep -v ' 1'
$
$ ./dup-static.sh git | grep contains
24 init_contains_cache_with_stride
24 init_contains_cache
24 contains_cache_peek
24 contains_cache_at_peek
24 contains_cache_at
24 clear_contains_cache
$
you will find 24 copies of the commit-slab routines for the
contains_cache. Of course, when you enable optimizations again,
these duplicate static functions (mostly) disappear. (Two static
functions remain, the rest are actually inlined at -O2).
However, if you apply the patch below, you end up with all of
the functions in the contains_cache commit-slab implementation
as external functions. Some of those functions are never called,
so it's not necessarily a win ... ;-)
Thanks for the explanation! I prefer your #include "commit.h" above to
the alternative.
Thanks,
-Stolee
Re: [PATCH] fetch-object.h: add missing declaration (hdr-check)
On 9/21/2018 1:05 PM, Junio C Hamano wrote: Ramsay Jones writes: BTW, I notice that patch #9 (commit-reach.h: add missing declarations (hdr-check)) didn't make it onto 'pu' - was there something else I needed to do? (I am still in two minds about sending an RFC patch on-top of patch #9). I refrained from queuing it as I did not sense a clear resolution of the discussion. I found it a sign that you may want to update the log message to explain "instead of adding a few forward decls, include the whole commit.h because..." that you had to explain why the patch did what it did to Derrick in a follow-up message. Also my fault for not saying "That's a good reason, thanks for explaining!" (I'll add that to the necessary thread.) -Stolee
[PATCH v3 5/7] commit/revisions: bookkeeping before refactoring
From: Derrick Stolee
There are a few things that need to move around a little before
making a big refactoring in the topo-order logic:
1. We need access to record_author_date() and
compare_commits_by_author_date() in revision.c. These are used
currently by sort_in_topological_order() in commit.c.
2. Moving these methods to commit.h requires adding the author_slab
definition to commit.h.
3. The add_parents_to_list() method in revision.c performs logic
around the UNINTERESTING flag and other special cases depending
on the struct rev_info. Allow this method to ignore a NULL 'list'
parameter, as we will not be populating the list for our walk.
Signed-off-by: Derrick Stolee
---
commit.c | 11 ---
commit.h | 8
revision.c | 6 --
3 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/commit.c b/commit.c
index d0f199e122..f68e04b2f1 100644
--- a/commit.c
+++ b/commit.c
@@ -655,11 +655,8 @@ struct commit *pop_commit(struct commit_list **stack)
/* count number of children that have not been emitted */
define_commit_slab(indegree_slab, int);
-/* record author-date for each commit object */
-define_commit_slab(author_date_slab, timestamp_t);
-
-static void record_author_date(struct author_date_slab *author_date,
- struct commit *commit)
+void record_author_date(struct author_date_slab *author_date,
+ struct commit *commit)
{
const char *buffer = get_commit_buffer(commit, NULL);
struct ident_split ident;
@@ -684,8 +681,8 @@ fail_exit:
unuse_commit_buffer(commit, buffer);
}
-static int compare_commits_by_author_date(const void *a_, const void *b_,
- void *cb_data)
+int compare_commits_by_author_date(const void *a_, const void *b_,
+ void *cb_data)
{
const struct commit *a = a_, *b = b_;
struct author_date_slab *author_date = cb_data;
diff --git a/commit.h b/commit.h
index 2b1a734388..ff0eb5f8ef 100644
--- a/commit.h
+++ b/commit.h
@@ -8,6 +8,7 @@
#include "gpg-interface.h"
#include "string-list.h"
#include "pretty.h"
+#include "commit-slab.h"
#define COMMIT_NOT_FROM_GRAPH 0x
#define GENERATION_NUMBER_INFINITY 0x
@@ -328,6 +329,13 @@ extern int remove_signature(struct strbuf *buf);
*/
extern int check_commit_signature(const struct commit *commit, struct
signature_check *sigc);
+/* record author-date for each commit object */
+define_commit_slab(author_date_slab, timestamp_t);
+
+void record_author_date(struct author_date_slab *author_date,
+ struct commit *commit);
+
+int compare_commits_by_author_date(const void *a_, const void *b_, void
*unused);
int compare_commits_by_commit_date(const void *a_, const void *b_, void
*unused);
int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_,
void *unused);
diff --git a/revision.c b/revision.c
index 2dcde8a8ac..92012d5f45 100644
--- a/revision.c
+++ b/revision.c
@@ -808,7 +808,8 @@ static int add_parents_to_list(struct rev_info *revs,
struct commit *commit,
if (p->object.flags & SEEN)
continue;
p->object.flags |= SEEN;
- commit_list_insert_by_date_cached(p, list, cached_base,
cache_ptr);
+ if (list)
+ commit_list_insert_by_date_cached(p, list,
cached_base, cache_ptr);
}
return 0;
}
@@ -847,7 +848,8 @@ static int add_parents_to_list(struct rev_info *revs,
struct commit *commit,
p->object.flags |= left_flag;
if (!(p->object.flags & SEEN)) {
p->object.flags |= SEEN;
- commit_list_insert_by_date_cached(p, list, cached_base,
cache_ptr);
+ if (list)
+ commit_list_insert_by_date_cached(p, list,
cached_base, cache_ptr);
}
if (revs->first_parent_only)
break;
--
gitgitgadget
[PATCH v3 7/7] revision.c: refactor basic topo-order logic
From: Derrick Stolee When running a command like 'git rev-list --topo-order HEAD', Git performed the following steps: 1. Run limit_list(), which parses all reachable commits, adds them to a linked list, and distributes UNINTERESTING flags. If all unprocessed commits are UNINTERESTING, then it may terminate without walking all reachable commits. This does not occur if we do not specify UNINTERESTING commits. 2. Run sort_in_topological_order(), which is an implementation of Kahn's algorithm. It first iterates through the entire set of important commits and computes the in-degree of each (plus one, as we use 'zero' as a special value here). Then, we walk the commits in priority order, adding them to the priority queue if and only if their in-degree is one. As we remove commits from this priority queue, we decrement the in-degree of their parents. 3. While we are peeling commits for output, get_revision_1() uses pop_commit on the full list of commits computed by sort_in_topological_order(). In the new algorithm, these three steps correspond to three different commit walks. We run these walks simultaneously, and advance each only as far as necessary to satisfy the requirements of the 'higher order' walk. We know when we can pause each walk by using generation numbers from the commit- graph feature. Recall that the generation number of a commit satisfies: * If the commit has at least one parent, then the generation number is one more than the maximum generation number among its parents. * If the commit has no parent, then the generation number is one. There are two special generation numbers: * GENERATION_NUMBER_INFINITY: this value is 0x and indicates that the commit is not stored in the commit-graph and the generation number was not previously calculated. * GENERATION_NUMBER_ZERO: this value (0) is a special indicator to say that the commit-graph was generated by a version of Git that does not compute generation numbers (such as v2.18.0). Since we use generation_numbers_enabled() before using the new algorithm, we do not need to worry about GENERATION_NUMBER_ZERO. However, the existence of GENERATION_NUMBER_INFINITY implies the following weaker statement than the usual we expect from generation numbers: If A and B are commits with generation numbers gen(A) and gen(B) and gen(A) < gen(B), then A cannot reach B. Thus, we will walk in each of our stages until the "maximum unexpanded generation number" is strictly lower than the generation number of a commit we are about to use. The walks are as follows: 1. EXPLORE: using the explore_queue priority queue (ordered by maximizing the generation number), parse each reachable commit until all commits in the queue have generation number strictly lower than needed. During this walk, update the UNINTERESTING flags as necessary. 2. INDEGREE: using the indegree_queue priority queue (ordered by maximizing the generation number), add one to the in- degree of each parent for each commit that is walked. Since we walk in order of decreasing generation number, we know that discovering an in-degree value of 0 means the value for that commit was not initialized, so should be initialized to two. (Recall that in-degree value "1" is what we use to say a commit is ready for output.) As we iterate the parents of a commit during this walk, ensure the EXPLORE walk has walked beyond their generation numbers. 3. TOPO: using the topo_queue priority queue (ordered based on the sort_order given, which could be commit-date, author- date, or typical topo-order which treats the queue as a LIFO stack), remove a commit from the queue and decrement the in-degree of each parent. If a parent has an in-degree of one, then we add it to the topo_queue. Before we decrement the in-degree, however, ensure the INDEGREE walk has walked beyond that generation number. The implementations of these walks are in the following methods: * explore_walk_step and explore_to_depth * indegree_walk_step and compute_indegrees_to_depth * next_topo_commit and expand_topo_walk These methods have some patterns that may seem strange at first, but they are probably carry-overs from their equivalents in limit_list and sort_in_topological_order. One thing that is missing from this implementation is a proper way to stop walking when the entire queue is UNINTERESTING, so this implementation is not enabled by comparisions, such as in 'git rev-list --topo-order A..B'. This can be updated in the future. In my local testing, I used the following Git commands on the Linux repository in three modes: HEAD~1 with no commit-graph, HEAD~1 with a commit-graph, and HEAD with a commit-graph. This allows comparing the benefits we get from parsing commits from the commit-graph and then again the benefits we get by restricting the set of commits we walk. Test: git rev-list --topo-order -100 HEAD
[PATCH v3 3/7] test-reach: add rev-list tests
From: Derrick Stolee The rev-list command is critical to Git's functionality. Ensure it works in the three commit-graph environments constructed in t6600-test-reach.sh. Here are a few important types of rev-list operations: * Basic: git rev-list --topo-order HEAD * Range: git rev-list --topo-order compare..HEAD * Ancestry: git rev-list --topo-order --ancestry-path compare..HEAD * Symmetric Difference: git rev-list --topo-order compare...HEAD Signed-off-by: Derrick Stolee --- t/t6600-test-reach.sh | 84 +++ 1 file changed, 84 insertions(+) diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh index 9d65b8b946..288f703b7b 100755 --- a/t/t6600-test-reach.sh +++ b/t/t6600-test-reach.sh @@ -243,4 +243,88 @@ test_expect_success 'commit_contains:miss' ' test_three_modes commit_contains --tag ' +test_expect_success 'rev-list: basic topo-order' ' + git rev-parse \ + commit-6-6 commit-5-6 commit-4-6 commit-3-6 commit-2-6 commit-1-6 \ + commit-6-5 commit-5-5 commit-4-5 commit-3-5 commit-2-5 commit-1-5 \ + commit-6-4 commit-5-4 commit-4-4 commit-3-4 commit-2-4 commit-1-4 \ + commit-6-3 commit-5-3 commit-4-3 commit-3-3 commit-2-3 commit-1-3 \ + commit-6-2 commit-5-2 commit-4-2 commit-3-2 commit-2-2 commit-1-2 \ + commit-6-1 commit-5-1 commit-4-1 commit-3-1 commit-2-1 commit-1-1 \ + >expect && + run_three_modes git rev-list --topo-order commit-6-6 +' + +test_expect_success 'rev-list: first-parent topo-order' ' + git rev-parse \ + commit-6-6 \ + commit-6-5 \ + commit-6-4 \ + commit-6-3 \ + commit-6-2 \ + commit-6-1 commit-5-1 commit-4-1 commit-3-1 commit-2-1 commit-1-1 \ + >expect && + run_three_modes git rev-list --first-parent --topo-order commit-6-6 +' + +test_expect_success 'rev-list: range topo-order' ' + git rev-parse \ + commit-6-6 commit-5-6 commit-4-6 commit-3-6 commit-2-6 commit-1-6 \ + commit-6-5 commit-5-5 commit-4-5 commit-3-5 commit-2-5 commit-1-5 \ + commit-6-4 commit-5-4 commit-4-4 commit-3-4 commit-2-4 commit-1-4 \ + commit-6-3 commit-5-3 commit-4-3 \ + commit-6-2 commit-5-2 commit-4-2 \ + commit-6-1 commit-5-1 commit-4-1 \ + >expect && + run_three_modes git rev-list --topo-order commit-3-3..commit-6-6 +' + +test_expect_success 'rev-list: range topo-order' ' + git rev-parse \ + commit-6-6 commit-5-6 commit-4-6 \ + commit-6-5 commit-5-5 commit-4-5 \ + commit-6-4 commit-5-4 commit-4-4 \ + commit-6-3 commit-5-3 commit-4-3 \ + commit-6-2 commit-5-2 commit-4-2 \ + commit-6-1 commit-5-1 commit-4-1 \ + >expect && + run_three_modes git rev-list --topo-order commit-3-8..commit-6-6 +' + +test_expect_success 'rev-list: first-parent range topo-order' ' + git rev-parse \ + commit-6-6 \ + commit-6-5 \ + commit-6-4 \ + commit-6-3 \ + commit-6-2 \ + commit-6-1 commit-5-1 commit-4-1 \ + >expect && + run_three_modes git rev-list --first-parent --topo-order commit-3-8..commit-6-6 +' + +test_expect_success 'rev-list: ancestry-path topo-order' ' + git rev-parse \ + commit-6-6 commit-5-6 commit-4-6 commit-3-6 \ + commit-6-5 commit-5-5 commit-4-5 commit-3-5 \ + commit-6-4 commit-5-4 commit-4-4 commit-3-4 \ + commit-6-3 commit-5-3 commit-4-3 \ + >expect && + run_three_modes git rev-list --topo-order --ancestry-path commit-3-3..commit-6-6 +' + +test_expect_success 'rev-list: symmetric difference topo-order' ' + git rev-parse \ + commit-6-6 commit-5-6 commit-4-6 \ + commit-6-5 commit-5-5 commit-4-5 \ + commit-6-4 commit-5-4 commit-4-4 \ + commit-6-3 commit-5-3 commit-4-3 \ + commit-6-2 commit-5-2 commit-4-2 \ + commit-6-1 commit-5-1 commit-4-1 \ + commit-3-8 commit-2-8 commit-1-8 \ + commit-3-7 commit-2-7 commit-1-7 \ + >expect && + run_three_modes git rev-list --topo-order commit-3-8...commit-6-6 +' + test_done -- gitgitgadget
[PATCH v3 6/7] revision.h: add whitespace in flag definitions
From: Derrick Stolee In anticipation of adding longer flag names in the next change, add an extra tab to each flag definition in revision.h. Signed-off-by: Derrick Stolee --- revision.h | 28 ++-- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/revision.h b/revision.h index fd4154ff75..e7bd059d80 100644 --- a/revision.h +++ b/revision.h @@ -10,20 +10,20 @@ #include "commit-slab-decl.h" /* Remember to update object flag allocation in object.h */ -#define SEEN (1u<<0) -#define UNINTERESTING (1u<<1) -#define TREESAME (1u<<2) -#define SHOWN (1u<<3) -#define TMP_MARK (1u<<4) /* for isolated cases; clean after use */ -#define BOUNDARY (1u<<5) -#define CHILD_SHOWN(1u<<6) -#define ADDED (1u<<7) /* Parents already parsed and added? */ -#define SYMMETRIC_LEFT (1u<<8) -#define PATCHSAME (1u<<9) -#define BOTTOM (1u<<10) -#define USER_GIVEN (1u<<25) /* given directly by the user */ -#define TRACK_LINEAR (1u<<26) -#define ALL_REV_FLAGS (((1u<<11)-1) | USER_GIVEN | TRACK_LINEAR) +#define SEEN (1u<<0) +#define UNINTERESTING (1u<<1) +#define TREESAME (1u<<2) +#define SHOWN (1u<<3) +#define TMP_MARK (1u<<4) /* for isolated cases; clean after use */ +#define BOUNDARY (1u<<5) +#define CHILD_SHOWN(1u<<6) +#define ADDED (1u<<7) /* Parents already parsed and added? */ +#define SYMMETRIC_LEFT (1u<<8) +#define PATCHSAME (1u<<9) +#define BOTTOM (1u<<10) +#define USER_GIVEN (1u<<25) /* given directly by the user */ +#define TRACK_LINEAR (1u<<26) +#define ALL_REV_FLAGS (((1u<<11)-1) | USER_GIVEN | TRACK_LINEAR) #define DECORATE_SHORT_REFS1 #define DECORATE_FULL_REFS 2 -- gitgitgadget
[PATCH v3 4/7] revision.c: begin refactoring --topo-order logic
From: Derrick Stolee
When running 'git rev-list --topo-order' and its kin, the topo_order
setting in struct rev_info implies the limited setting. This means
that the following things happen during prepare_revision_walk():
* revs->limited implies we run limit_list() to walk the entire
reachable set. There are some short-cuts here, such as if we
perform a range query like 'git rev-list COMPARE..HEAD' and we
can stop limit_list() when all queued commits are uninteresting.
* revs->topo_order implies we run sort_in_topological_order(). See
the implementation of that method in commit.c. It implies that
the full set of commits to order is in the given commit_list.
These two methods imply that a 'git rev-list --topo-order HEAD'
command must walk the entire reachable set of commits _twice_ before
returning a single result.
If we have a commit-graph file with generation numbers computed, then
there is a better way. This patch introduces some necessary logic
redirection when we are in this situation.
In v2.18.0, the commit-graph file contains zero-valued bytes in the
positions where the generation number is stored in v2.19.0 and later.
Thus, we use generation_numbers_enabled() to check if the commit-graph
is available and has non-zero generation numbers.
When setting revs->limited only because revs->topo_order is true,
only do so if generation numbers are not available. There is no
reason to use the new logic as it will behave similarly when all
generation numbers are INFINITY or ZERO.
In prepare_revision_walk(), if we have revs->topo_order but not
revs->limited, then we trigger the new logic. It breaks the logic
into three pieces, to fit with the existing framework:
1. init_topo_walk() fills a new struct topo_walk_info in the rev_info
struct. We use the presence of this struct as a signal to use the
new methods during our walk. In this patch, this method simply
calls limit_list() and sort_in_topological_order(). In the future,
this method will set up a new data structure to perform that logic
in-line.
2. next_topo_commit() provides get_revision_1() with the next topo-
ordered commit in the list. Currently, this simply pops the commit
from revs->commits.
3. expand_topo_walk() provides get_revision_1() with a way to signal
walking beyond the latest commit. Currently, this calls
add_parents_to_list() exactly like the old logic.
While this commit presents method redirection for performing the
exact same logic as before, it allows the next commit to focus only
on the new logic.
Signed-off-by: Derrick Stolee
---
revision.c | 42 ++
revision.h | 4
2 files changed, 42 insertions(+), 4 deletions(-)
diff --git a/revision.c b/revision.c
index e18bd530e4..2dcde8a8ac 100644
--- a/revision.c
+++ b/revision.c
@@ -25,6 +25,7 @@
#include "worktree.h"
#include "argv-array.h"
#include "commit-reach.h"
+#include "commit-graph.h"
volatile show_early_output_fn_t show_early_output;
@@ -2454,7 +2455,7 @@ int setup_revisions(int argc, const char **argv, struct
rev_info *revs, struct s
if (revs->diffopt.objfind)
revs->simplify_history = 0;
- if (revs->topo_order)
+ if (revs->topo_order && !generation_numbers_enabled(the_repository))
revs->limited = 1;
if (revs->prune_data.nr) {
@@ -2892,6 +2893,33 @@ static int mark_uninteresting(const struct object_id
*oid,
return 0;
}
+struct topo_walk_info {};
+
+static void init_topo_walk(struct rev_info *revs)
+{
+ struct topo_walk_info *info;
+ revs->topo_walk_info = xmalloc(sizeof(struct topo_walk_info));
+ info = revs->topo_walk_info;
+ memset(info, 0, sizeof(struct topo_walk_info));
+
+ limit_list(revs);
+ sort_in_topological_order(>commits, revs->sort_order);
+}
+
+static struct commit *next_topo_commit(struct rev_info *revs)
+{
+ return pop_commit(>commits);
+}
+
+static void expand_topo_walk(struct rev_info *revs, struct commit *commit)
+{
+ if (add_parents_to_list(revs, commit, >commits, NULL) < 0) {
+ if (!revs->ignore_missing_links)
+ die("Failed to traverse parents of commit %s",
+ oid_to_hex(>object.oid));
+ }
+}
+
int prepare_revision_walk(struct rev_info *revs)
{
int i;
@@ -2928,11 +2956,13 @@ int prepare_revision_walk(struct rev_info *revs)
commit_list_sort_by_date(>commits);
if (revs->no_walk)
return 0;
- if (revs->limited)
+ if (revs->limited) {
if (limit_list(revs) < 0)
return -1;
- if (revs->topo_order)
- sort_in_topological_order(>commits, revs->sort_order);
+ if (revs->topo_order)
+ sort_in_topological_order(>commits,
revs->sort_order);
+ } else if (revs->topo_order)
+ init_topo_walk(revs);
if
[PATCH v3 1/7] prio-queue: add 'peek' operation
From: Derrick Stolee
When consuming a priority queue, it can be convenient to inspect
the next object that will be dequeued without actually dequeueing
it. Our existing library did not have such a 'peek' operation, so
add it as prio_queue_peek().
Add a reference-level comparison in t/helper/test-prio-queue.c
so this method is exercised by t0009-prio-queue.sh.
Signed-off-by: Derrick Stolee
---
prio-queue.c | 9 +
prio-queue.h | 6 ++
t/helper/test-prio-queue.c | 10 +++---
3 files changed, 22 insertions(+), 3 deletions(-)
diff --git a/prio-queue.c b/prio-queue.c
index a078451872..d3f488cb05 100644
--- a/prio-queue.c
+++ b/prio-queue.c
@@ -85,3 +85,12 @@ void *prio_queue_get(struct prio_queue *queue)
}
return result;
}
+
+void *prio_queue_peek(struct prio_queue *queue)
+{
+ if (!queue->nr)
+ return NULL;
+ if (!queue->compare)
+ return queue->array[queue->nr - 1].data;
+ return queue->array[0].data;
+}
diff --git a/prio-queue.h b/prio-queue.h
index d030ec9dd6..682e51867a 100644
--- a/prio-queue.h
+++ b/prio-queue.h
@@ -46,6 +46,12 @@ extern void prio_queue_put(struct prio_queue *, void *thing);
*/
extern void *prio_queue_get(struct prio_queue *);
+/*
+ * Gain access to the "thing" that would be returned by
+ * prio_queue_get, but do not remove it from the queue.
+ */
+extern void *prio_queue_peek(struct prio_queue *);
+
extern void clear_prio_queue(struct prio_queue *);
/* Reverse the LIFO elements */
diff --git a/t/helper/test-prio-queue.c b/t/helper/test-prio-queue.c
index 9807b649b1..e817bbf464 100644
--- a/t/helper/test-prio-queue.c
+++ b/t/helper/test-prio-queue.c
@@ -22,9 +22,13 @@ int cmd__prio_queue(int argc, const char **argv)
struct prio_queue pq = { intcmp };
while (*++argv) {
- if (!strcmp(*argv, "get"))
- show(prio_queue_get());
- else if (!strcmp(*argv, "dump")) {
+ if (!strcmp(*argv, "get")) {
+ void *peek = prio_queue_peek();
+ void *get = prio_queue_get();
+ if (peek != get)
+ BUG("peek and get results do not match");
+ show(get);
+ } else if (!strcmp(*argv, "dump")) {
int *v;
while ((v = prio_queue_get()))
show(v);
--
gitgitgadget
[PATCH v3 2/7] test-reach: add run_three_modes method
From: Derrick Stolee
The 'test_three_modes' method assumes we are using the 'test-tool
reach' command for our test. However, we may want to use the data
shape of our commit graph and the three modes (no commit-graph,
full commit-graph, partial commit-graph) for other git commands.
Split test_three_modes to be a simple translation on a more general
run_three_modes method that executes the given command and tests
the actual output to the expected output.
Signed-off-by: Derrick Stolee
---
t/t6600-test-reach.sh | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
index d139a00d1d..9d65b8b946 100755
--- a/t/t6600-test-reach.sh
+++ b/t/t6600-test-reach.sh
@@ -53,18 +53,22 @@ test_expect_success 'setup' '
git config core.commitGraph true
'
-test_three_modes () {
+run_three_modes () {
test_when_finished rm -rf .git/objects/info/commit-graph &&
- test-tool reach $1 actual &&
+ "$@" actual &&
test_cmp expect actual &&
cp commit-graph-full .git/objects/info/commit-graph &&
- test-tool reach $1 actual &&
+ "$@" actual &&
test_cmp expect actual &&
cp commit-graph-half .git/objects/info/commit-graph &&
- test-tool reach $1 actual &&
+ "$@" actual &&
test_cmp expect actual
}
+test_three_modes () {
+ run_three_modes test-tool reach "$@"
+}
+
test_expect_success 'ref_newer:miss' '
cat >input <<-\EOF &&
A:commit-5-7
--
gitgitgadget
[PATCH v3 0/7] Use generation numbers for --topo-order
This patch series performs a decently-sized refactoring of the revision-walk machinery. Well, "refactoring" is probably the wrong word, as I don't actually remove the old code. Instead, when we see certain options in the 'rev_info' struct, we redirect the commit-walk logic to a new set of methods that distribute the workload differently. By using generation numbers in the commit-graph, we can significantly improve 'git log --graph' commands (and the underlying 'git rev-list --topo-order'). On the Linux repository, I got the following performance results when comparing to the previous version with or without a commit-graph: Test: git rev-list --topo-order -100 HEAD HEAD~1, no commit-graph: 6.80 s HEAD~1, w/ commit-graph: 0.77 s HEAD, w/ commit-graph: 0.02 s Test: git rev-list --topo-order -100 HEAD -- tools HEAD~1, no commit-graph: 9.63 s HEAD~1, w/ commit-graph: 6.06 s HEAD, w/ commit-graph: 0.06 s If you want to read this series but are unfamiliar with the commit-graph and generation numbers, then I recommend reading Documentation/technical/commit-graph.txt or a blob post [1] I wrote on the subject. In particular, the three-part walk described in "revision.c: refactor basic topo-order logic" is present (but underexplained) as an animated PNG [2]. Since revision.c is an incredibly important (and old) portion of the codebase -- and because there are so many orthogonal options in 'struct rev_info' -- I consider this submission to be "RFC quality". That is, I am not confident that I am not missing anything, or that my solution is the best it can be. I did merge this branch with ds/commit-graph-with-grafts and the "DO-NOT-MERGE: write and read commit-graph always" commit that computes a commit-graph with every 'git commit' command. The test suite passed with that change, available on GitHub [3]. To ensure that I cover at least the case I think are interesting, I added tests to t6600-test-reach.sh to verify the walks report the correct results for the three cases there (no commit-graph, full commit-graph, and a partial commit-graph so the walk starts at GENERATION_NUMBER_INFINITY). One notable case that is not included in this series is the case of a history comparison such as 'git rev-list --topo-order A..B'. The existing code in limit_list() has ways to cut the walk short when all pending commits are UNINTERESTING. Since this code depends on commit_list instead of the prio_queue we are using here, I chose to leave it untouched for now. We can revisit it in a separate series later. Since handle_commit() turns on revs->limited when a commit is UNINTERESTING, we do not hit the new code in this case. Removing this 'revs->limited = 1;' line yields correct results, but the performance is worse. This series was based on ds/reachable, but is now based on 'master' to not conflict with 182070 "commit: use timestamp_t for author_date_slab". There is a small conflict with md/filter-trees, because it renamed a flag in revisions.h in the line before I add new flags. Hopefully this conflict is not too difficult to resolve. Changes in V3: I added a new patch that updates the tab-alignment for flags in revision.h before adding new ones (Thanks, Ævar!). Also, I squashed the recommended changes to run_three_modes and test_three_modes from Szeder and Junio. Thanks! Thanks, -Stolee [1] https://blogs.msdn.microsoft.com/devops/2018/07/09/supercharging-the-git-commit-graph-iii-generations/ Supercharging the Git Commit Graph III: Generations and Graph Algorithms [2] https://msdnshared.blob.core.windows.net/media/2018/06/commit-graph-topo-order-b-a.png Animation showing three-part walk [3] https://github.com/derrickstolee/git/tree/topo-order/testA branch containing this series along with commits to compute commit-graph in entire test suite. Cc: avarab@gmail.comCc: szeder@gmail.com Derrick Stolee (7): prio-queue: add 'peek' operation test-reach: add run_three_modes method test-reach: add rev-list tests revision.c: begin refactoring --topo-order logic commit/revisions: bookkeeping before refactoring revision.h: add whitespace in flag definitions revision.c: refactor basic topo-order logic commit.c | 11 +- commit.h | 8 ++ object.h | 4 +- prio-queue.c | 9 ++ prio-queue.h | 6 + revision.c | 232 - revision.h | 34 +++--- t/helper/test-prio-queue.c | 10 +- t/t6600-test-reach.sh | 96 ++- 9 files changed, 374 insertions(+), 36 deletions(-) base-commit: 2d3b1c576c85b7f5db1f418907af00ab88e0c303 Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-25%2Fderrickstolee%2Ftopo-order%2Fprogress-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-25/derrickstolee/topo-order/progress-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/25 Range-diff vs v2: 1: cc1ec4c270 = 1: cc1ec4c270
Re: What's cooking in git.git (Sep 2018, #04; Thu, 20)
Johannes Sixt writes:
> Am 21.09.18 um 07:22 schrieb Junio C Hamano:
>> The tip of 'next' hasn't been rewound yet. The three GSoC "rewrite
>> in C" topics are still unclassified in this "What's cooking" report,
>> but I am hoping that we can have them in 'next' sooner rather than
>> later. I got an impression that Dscho wanted a chance for the final
>> clean-up on some of them, so I am not doing anything hasty yet at
>> this moment, though.
>
> While playing around with those topics in my own build on Windows, I
> noticed a small glitch in your merge commits.
>
> When I compile 59085279e6, which is today's jch~11, I see
>
> CC builtin/rebase.o
> builtin/rebase.c: In function 'can_fast_forward':
> builtin/rebase.c:443:2: warning: implicit declaration of function
> 'get_merge_bases' [-Wimplicit-function-declaration]
> merge_bases = get_merge_bases(onto, head);
> ^
> builtin/rebase.c:443:14: warning: assignment makes pointer from integer
> without a cast [enabled by default]
> merge_bases = get_merge_bases(onto, head);
> ^
>
> I notice that you fixed it in the next merge, jch~10 aka d311e29abe,
> by adding
>
> #include "commit-reach.h"
>
> in builtin/rebase.c; this line is obviously required one merge
> commit earlier, jch~11.
Thanks. Near the problematic merges are
41e89b1c02 Merge branch 'pk/rebase-in-c-6-final' into jch
a9794eb0fe Merge branch 'js/rebase-in-c-5.5-work-with-rebase-i-in-c' into
jch
d348159563 Merge branch 'pk/rebase-in-c-5-test' into jch
d311e29abe Merge branch 'pk/rebase-in-c-4-opts' into jch
59085279e6 Merge branch 'pk/rebase-in-c-3-acts' into jch
88091f8941 Merge branch 'pk/rebase-in-c-2-basic' into jch
38a693a042 Merge branch 'ps/stash-in-c' into jch
488f36e338 Merge branch 'ag/rebase-i-in-c' into jch
Actually 88091f8941 is already broken. The merge-fix must go there.
Thanks for letting me know (even though it is very unlikely that
2-basic would graduate without any of these other topics---in a
sense there is not much point for these patches to be spread across
this many topics).
commit a581ba92f4f4e112a7d7e0c84c0ced1af271b7dc
Author: Junio C Hamano
Date: Fri Sep 21 10:14:43 2018 -0700
merge-fix/pk/rebase-in-c-2-basic
diff --git a/builtin/rebase.c b/builtin/rebase.c
index e817956d96..71367c8530 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -21,6 +21,7 @@
#include "diff.h"
#include "wt-status.h"
#include "revision.h"
+#include "commit-reach.h"
static char const * const builtin_rebase_usage[] = {
N_("git rebase [-i] [options] [--exec ] [--onto ] "
Re: What's cooking in git.git (Sep 2018, #04; Thu, 20)
Am 21.09.18 um 07:22 schrieb Junio C Hamano: > The tip of 'next' hasn't been rewound yet. The three GSoC "rewrite > in C" topics are still unclassified in this "What's cooking" report, > but I am hoping that we can have them in 'next' sooner rather than > later. I got an impression that Dscho wanted a chance for the final > clean-up on some of them, so I am not doing anything hasty yet at > this moment, though. While playing around with those topics in my own build on Windows, I noticed a small glitch in your merge commits. When I compile 59085279e6, which is today's jch~11, I see CC builtin/rebase.o builtin/rebase.c: In function 'can_fast_forward': builtin/rebase.c:443:2: warning: implicit declaration of function 'get_merge_bases' [-Wimplicit-function-declaration] merge_bases = get_merge_bases(onto, head); ^ builtin/rebase.c:443:14: warning: assignment makes pointer from integer without a cast [enabled by default] merge_bases = get_merge_bases(onto, head); ^ I notice that you fixed it in the next merge, jch~10 aka d311e29abe, by adding #include "commit-reach.h" in builtin/rebase.c; this line is obviously required one merge commit earlier, jch~11. -- Hannes
Re: [PATCH] fetch-object.h: add missing declaration (hdr-check)
Ramsay Jones writes: > BTW, I notice that patch #9 (commit-reach.h: add missing declarations > (hdr-check)) didn't make it onto 'pu' - was there something else I > needed to do? (I am still in two minds about sending an RFC patch > on-top of patch #9). I refrained from queuing it as I did not sense a clear resolution of the discussion. I found it a sign that you may want to update the log message to explain "instead of adding a few forward decls, include the whole commit.h because..." that you had to explain why the patch did what it did to Derrick in a follow-up message.
credential..helper with partial url path
Suppose I need to use different credential.helper values for different repositories on the same HTTPS host. Ideally I would like to be able to write this logic using a partial URL path prefix, for example in ~/.gitconfig [credential "https://example.com/prefix1/foo.git;] helper = !ZZZ [credential "https://example.com/prefix1/;] helper = !YYY [credential "https://example.com/;] useHttpPath = true helper = !XXX For prefix1/foo.git (exact path match) this tries ZZZ first as desired: $ git credential fill protocol=https host=example.com path=prefix1/foo.git ZZZ get: ZZZ: command not found XXX get: XXX: command not found Username for 'https://example.com/prefix1/foo.git': ^C However, prefix1/bar.git does not try YYY: $ git credential fill protocol=https host=example.com path=prefix1/bar.git XXX get: XXX: command not found Username for 'https://example.com/prefix1/bar.git': ^C even though (AFAICT) the general behavior of git-config seems to imply that it would do so: $ git config --get-urlmatch credential https://example.com/prefix1/bar.git credential.helper !YYY credential.usehttppath true Is this discrepancy intended? The documentation in `man gitcredentials` (https://git-scm.com/docs/gitcredentials) points out quite explicitly that my patterns will not match foo.example.com nor plain http, but both of those rules are consistent with what git-config reports: $ git config --get-urlmatch credential https://foo.example.com/ credential.helper osxkeychain $ git config --get-urlmatch credential http://example.com/ credential.helper osxkeychain If indeed the current behavior of git-credential is as intended, I think it would be helpful for that manpage to explicitly mention it (i.e. that you may specify a URL path component but that it must match exactly). Right now the only example given is one in which 'the "pattern" URL does not care about the path component at all.' I'm testing against $ git --version git version 2.19.0 installed via homebrew on Mac OS X. Thanks, David -- David Zych Lead Network Service Engineer University of Illinois Technology Services
Re: [PATCH 2/3] git-column.1: clarify initial description, provide examples
Thank you Junio, maybe I will have another chance to get practice sending a v2 patch. On Fri, Sep 21, 2018 at 09:32:00AM -0700, Junio C Hamano wrote: > frede...@ofb.net writes: > > > On Thu, Sep 20, 2018 at 06:23:03PM +0200, Duy Nguyen wrote: > >> On Wed, Sep 19, 2018 at 03:59:58PM -0700, Junio C Hamano wrote: > >> > > @@ -23,7 +26,7 @@ OPTIONS > >> > > > >> > > --mode=:: > >> > >Specify layout mode. See configuration variable column.ui for > >> > > option > >> > > - syntax. > >> > > + syntax (in git-config(1)). > >> > >> I think we usually link to other commands with "linkgit", like > >> linkgit:git-config[1] > >> > >> Other than that, the rest looks good. > > > > Thank you, then do I edit the patch and resubmit as PATCH v2 with the > > message ID and all that? > > > > Frederick > > If this is the only change in the whole 3 patches, then I can just > squash in the following to save one round-trip. > > Documentation/git-column.txt | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt > index 5bbb51068e..763afabb6d 100644 > --- a/Documentation/git-column.txt > +++ b/Documentation/git-column.txt > @@ -26,7 +26,7 @@ OPTIONS > > --mode=:: > Specify layout mode. See configuration variable column.ui for option > - syntax (in git-config(1)). > + syntax in linkgit:git-config[1]. > > --raw-mode=:: > Same as --mode but take mode encoded as a number. This is mainly used >
Re: [PATCH] fetch-object.h: add missing declaration (hdr-check)
On 21/09/18 17:21, Junio C Hamano wrote: > Ramsay Jones writes: > >> Signed-off-by: Ramsay Jones >> --- >> >> Hi Junio, >> >> This is the patch I needed for the current 'next' branch to get >> a clean 'hdr-check' > > Which means that this is a fix on top of jt/lazy-object-fetch-fix > topic, I think. > > Will apply there. Yes, indeed. Sorry, I should have added that information, rather than forcing you to look it up! (Similar comment on the userdiff.h patch as well) :( BTW, I notice that patch #9 (commit-reach.h: add missing declarations (hdr-check)) didn't make it onto 'pu' - was there something else I needed to do? (I am still in two minds about sending an RFC patch on-top of patch #9). Thanks! ATB, Ramsay Jones
Re: [PATCH v5 17/23] userdiff.c: remove implicit dependency on the_index
Nguyễn Thái Ngọc Duy writes:
> diff --git a/userdiff.h b/userdiff.h
> index 2ef0ce5452..dad3fc03c1 100644
> --- a/userdiff.h
> +++ b/userdiff.h
> @@ -21,7 +21,8 @@ struct userdiff_driver {
>
> int userdiff_config(const char *k, const char *v);
> struct userdiff_driver *userdiff_find_by_name(const char *name);
> -struct userdiff_driver *userdiff_find_by_path(const char *path);
> +struct userdiff_driver *userdiff_find_by_path(struct index_state *istate,
> + const char *path);
>
> /*
> * Initialize any textconv-related fields in the driver and return it, or
> NULL
This needs fix described in
https://public-inbox.org/git/c46ca4a9-6822-436c-8e84-95b977527...@ramsayjones.plus.com/
I can squash it in.
Re: [PATCH 3/3] transport.c: introduce core.alternateRefsPrefixes
Taylor Blau writes:
> ...' block with your suggestion above. It's tempting to introduce it as:
>
> expect_haves() {
> printf "%s .have\n" $(git rev-parse -- $@)
> }
>
> And call it as:
>
> expect_haves one three two >expect
>
> But I'm not sure whether I think that this is better or worse than
> writing it twice inline.
If the expected pattern is expected to stay to be just a sequence of
" .have" and nothing else for the foreseeable future, I think
it is a good idea to introduce such a helper function. Spelling it
out at the use site, e.g.
printf "%s .have\n" $(git rev-parse a b c) >expect
will become cumbersome once the set of objects you need to show
starts growing.
expect_haves a b c >expect
would be shorter, of course. And as long as we expect to have ONLY
" .have" lines and nothing else, there is no downside that the
details of the format is hidden away inside the helper.
Re: [PATCH 3/3] transport.c: introduce core.alternateRefsPrefixes
Eric Sunshine writes: > On Thu, Sep 20, 2018 at 2:04 PM Taylor Blau wrote: >> The recently-introduced "core.alternateRefsCommand" allows callers to >> specify with high flexibility the tips that they wish to advertise from >> alternates. This flexibility comes at the cost of some inconvenience >> when the caller only wishes to limit the advertisement to one or more >> prefixes. >> [...] >> Signed-off-by: Taylor Blau >> --- >> diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh >> @@ -44,4 +44,15 @@ test_expect_success 'with core.alternateRefsCommand' ' >> +test_expect_success 'with core.alternateRefsPrefixes' ' >> + test_config -C fork core.alternateRefsPrefixes "refs/tags" && >> + cat >expect <<-EOF && >> + $(git rev-parse one) .have >> + $(git rev-parse three) .have >> + $(git rev-parse two) .have >> + EOF > > It's probably a matter of taste as to which is more readable, but this > entire "cat < > printf "%s .have\n" $(git rev-parse one three two) >expect && > > Same comment applies to previous patch, as well. If the expected pattern is expected to stay to be just a sequence of " .have" and nothing else for the foreseeable future, I think it is a good idea. > >> + printf "" | git receive-pack fork | extract_haves >actual && >> + test_cmp expect actual >> +'
Re: [PATCH 2/3] transport.c: introduce core.alternateRefsCommand
Taylor Blau writes:
> +extract_haves () {
> + depacketize - | grep -o '^.* \.have'
Not portable, isn't it?
cf. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/grep.html
Re: [PATCH 2/3] git-column.1: clarify initial description, provide examples
frede...@ofb.net writes: > On Thu, Sep 20, 2018 at 06:23:03PM +0200, Duy Nguyen wrote: >> On Wed, Sep 19, 2018 at 03:59:58PM -0700, Junio C Hamano wrote: >> > > @@ -23,7 +26,7 @@ OPTIONS >> > > >> > > --mode=:: >> > > Specify layout mode. See configuration variable column.ui for >> > > option >> > > -syntax. >> > > +syntax (in git-config(1)). >> >> I think we usually link to other commands with "linkgit", like >> linkgit:git-config[1] >> >> Other than that, the rest looks good. > > Thank you, then do I edit the patch and resubmit as PATCH v2 with the > message ID and all that? > > Frederick If this is the only change in the whole 3 patches, then I can just squash in the following to save one round-trip. Documentation/git-column.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/git-column.txt b/Documentation/git-column.txt index 5bbb51068e..763afabb6d 100644 --- a/Documentation/git-column.txt +++ b/Documentation/git-column.txt @@ -26,7 +26,7 @@ OPTIONS --mode=:: Specify layout mode. See configuration variable column.ui for option - syntax (in git-config(1)). + syntax in linkgit:git-config[1]. --raw-mode=:: Same as --mode but take mode encoded as a number. This is mainly used
Re: [PATCH] userdiff.h: add missing declaration (hdr-check)
On Fri, Sep 21, 2018 at 6:24 PM Junio C Hamano wrote:
>
> Ramsay Jones writes:
>
> > Signed-off-by: Ramsay Jones
> > ---
> >
> > Hi Junio,
> >
> > ... and this is the patch I needed for the current 'pu' branch.
>
> Which in turn means that this is to fix 5b338d60 ("userdiff.c:
> remove implicit dependency on the_index", 2018-09-15) and should be
> rolled into nd/the_index topic.
I guess I should send v6 then?
--
Duy
Re: [PATCH] userdiff.h: add missing declaration (hdr-check)
Ramsay Jones writes:
> Signed-off-by: Ramsay Jones
> ---
>
> Hi Junio,
>
> ... and this is the patch I needed for the current 'pu' branch.
Which in turn means that this is to fix 5b338d60 ("userdiff.c:
remove implicit dependency on the_index", 2018-09-15) and should be
rolled into nd/the_index topic.
>
> ATB,
> Ramsay Jones
>
> userdiff.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/userdiff.h b/userdiff.h
> index dad3fc03c1..b072bfe89a 100644
> --- a/userdiff.h
> +++ b/userdiff.h
> @@ -3,6 +3,8 @@
>
> #include "notes-cache.h"
>
> +struct index_state;
> +
> struct userdiff_funcname {
> const char *pattern;
> int cflags;
Re: [PATCH] fetch-object.h: add missing declaration (hdr-check)
Ramsay Jones writes: > Signed-off-by: Ramsay Jones > --- > > Hi Junio, > > This is the patch I needed for the current 'next' branch to get > a clean 'hdr-check' Which means that this is a fix on top of jt/lazy-object-fetch-fix topic, I think. Will apply there. Thanks.
Re: [PATCH v5 9/9] submodule: support reading .gitmodules when it's not in the working tree
Antonio Ospite writes: > Protecting the problematic submodules function could work for now, but > I'd like to have more comments, my proposal is: > > diff --git a/builtin/grep.c b/builtin/grep.c > index 601f801158..52b45de749 100644 > --- a/builtin/grep.c > +++ b/builtin/grep.c > @@ -427,6 +427,11 @@ static int grep_submodule(struct grep_opt *opt, struct > repository *superproject, > if (repo_submodule_init(, superproject, path)) > return 0; > > + grep_read_lock(); > + /* > +* NEEDSWORK: repo_read_gitmodules accesses the object store which is > +* global, thus it needs to be protected. > +*/ > repo_read_gitmodules(); > > /* > @@ -439,7 +444,6 @@ static int grep_submodule(struct grep_opt *opt, struct > repository *superproject, > * store is no longer global and instead is a member of the repository > * object. > */ > - grep_read_lock(); > add_to_alternates_memory(submodule.objects->objectdir); > grep_read_unlock(); I think this is in line with how the grep codepath protects itself when doing anything that accesses the object store. Thanks.
Re: git check-ignore ignores negated entries
On Thu, Sep 20, 2018 at 7:25 PM David Alphus wrote: > In looking through check-ignore.c, it appears that we check that > last_exclude_matching() returns an exclude object. It should be noted > that we do not consider that exclude struct can be set with the > EXC_FLAG_NEGATIVE flag. This flag says that the final matching rule > should not be ignored. > check_ignore needs to consider this condition in order to work properly. I haven't checked the code yet. But since you already did, patches are welcome ;-) -- Duy
Re: [RFC PATCH v4 1/3] Add support for nested aliases
Tim Schumacher writes: > it is located at the top of the while() loop. Giving an example is nice, but > wouldn't > it be better to say something like the following? > > /* >* Check if av[0] is a command before seeing if it is an >* alias to avoid taking over existing commands >*/ If we have more concrete and constructive things to explain why we choose to forbid it, that may be worth saying, but I agree that it does not add much value to this comment to declare that an attempt to take over existing commands is "insane". Thanks.
[PATCH v5 21/23] ws.c: remove implicit dependency on the_index
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: Junio C Hamano
---
apply.c | 8 +---
cache.h | 2 +-
diff.c | 6 +++---
ws.c| 5 ++---
4 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/apply.c b/apply.c
index 571b89c2e0..fdae1d423b 100644
--- a/apply.c
+++ b/apply.c
@@ -2131,10 +2131,12 @@ static int parse_chunk(struct apply_state *state, char
*buffer, unsigned long si
if (!use_patch(state, patch))
patch->ws_rule = 0;
+ else if (patch->new_name)
+ patch->ws_rule = whitespace_rule(state->repo->index,
+patch->new_name);
else
- patch->ws_rule = whitespace_rule(patch->new_name
-? patch->new_name
-: patch->old_name);
+ patch->ws_rule = whitespace_rule(state->repo->index,
+patch->old_name);
patchsize = parse_single_patch(state,
buffer + offset + hdrsize,
diff --git a/cache.h b/cache.h
index 094652a503..eb0f7d5470 100644
--- a/cache.h
+++ b/cache.h
@@ -1694,7 +1694,7 @@ void shift_tree_by(const struct object_id *, const struct
object_id *, struct ob
/* All WS_* -- when extended, adapt diff.c emit_symbol */
#define WS_RULE_MASK 0
extern unsigned whitespace_rule_cfg;
-extern unsigned whitespace_rule(const char *);
+extern unsigned whitespace_rule(struct index_state *, const char *);
extern unsigned parse_whitespace_rule(const char *);
extern unsigned ws_check(const char *line, int len, unsigned ws_rule);
extern void ws_check_emit(const char *line, int len, unsigned ws_rule, FILE
*stream, const char *set, const char *reset, const char *ws);
diff --git a/diff.c b/diff.c
index 5256b9eabc..c5b5e7ac41 100644
--- a/diff.c
+++ b/diff.c
@@ -1705,7 +1705,7 @@ static void emit_rewrite_diff(const char *name_a,
memset(, 0, sizeof(ecbdata));
ecbdata.color_diff = want_color(o->use_color);
- ecbdata.ws_rule = whitespace_rule(name_b);
+ ecbdata.ws_rule = whitespace_rule(o->repo->index, name_b);
ecbdata.opt = o;
if (ecbdata.ws_rule & WS_BLANK_AT_EOF) {
mmfile_t mf1, mf2;
@@ -3480,7 +3480,7 @@ static void builtin_diff(const char *name_a,
lbl[0] = NULL;
ecbdata.label_path = lbl;
ecbdata.color_diff = want_color(o->use_color);
- ecbdata.ws_rule = whitespace_rule(name_b);
+ ecbdata.ws_rule = whitespace_rule(o->repo->index, name_b);
if (ecbdata.ws_rule & WS_BLANK_AT_EOF)
check_blank_at_eof(, , );
ecbdata.opt = o;
@@ -3640,7 +3640,7 @@ static void builtin_checkdiff(const char *name_a, const
char *name_b,
data.filename = name_b ? name_b : name_a;
data.lineno = 0;
data.o = o;
- data.ws_rule = whitespace_rule(attr_path);
+ data.ws_rule = whitespace_rule(o->repo->index, attr_path);
data.conflict_marker_size = ll_merge_marker_size(o->repo->index,
attr_path);
if (fill_mmfile(o->repo, , one) < 0 ||
diff --git a/ws.c b/ws.c
index 5b67b426e7..55349b4c5d 100644
--- a/ws.c
+++ b/ws.c
@@ -3,7 +3,6 @@
*
* Copyright (c) 2007 Junio C Hamano
*/
-
#include "cache.h"
#include "attr.h"
@@ -71,14 +70,14 @@ unsigned parse_whitespace_rule(const char *string)
return rule;
}
-unsigned whitespace_rule(const char *pathname)
+unsigned whitespace_rule(struct index_state *istate, const char *pathname)
{
static struct attr_check *attr_whitespace_rule;
if (!attr_whitespace_rule)
attr_whitespace_rule = attr_check_initl("whitespace", NULL);
- if (!git_check_attr(_index, pathname, attr_whitespace_rule)) {
+ if (!git_check_attr(istate, pathname, attr_whitespace_rule)) {
const char *value;
value = attr_whitespace_rule->items[0].value;
--
2.19.0.640.gcd3aa10a8a
[PATCH v5 22/23] revision.c: remove implicit dependency on the_index
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: Junio C Hamano
---
.../technical/api-revision-walking.txt| 4 +--
bisect.c | 4 +--
builtin/add.c | 4 +--
builtin/am.c | 6 ++--
builtin/blame.c | 2 +-
builtin/checkout.c| 4 +--
builtin/commit.c | 2 +-
builtin/describe.c| 4 +--
builtin/diff-files.c | 2 +-
builtin/diff-index.c | 2 +-
builtin/diff-tree.c | 2 +-
builtin/diff.c| 2 +-
builtin/fast-export.c | 2 +-
builtin/fmt-merge-msg.c | 2 +-
builtin/log.c | 16 +-
builtin/merge.c | 4 +--
builtin/pack-objects.c| 2 +-
builtin/prune.c | 2 +-
builtin/reflog.c | 2 +-
builtin/rev-list.c| 2 +-
builtin/revert.c | 2 +-
builtin/shortlog.c| 2 +-
builtin/submodule--helper.c | 2 +-
bundle.c | 4 +--
diff-lib.c| 4 +--
http-push.c | 2 +-
merge-recursive.c | 2 +-
pack-bitmap-write.c | 2 +-
ref-filter.c | 2 +-
remote.c | 2 +-
revision.c| 32 +++
revision.h| 13 ++--
sequencer.c | 8 ++---
shallow.c | 2 +-
submodule.c | 6 ++--
t/helper/test-revision-walking.c | 2 +-
wt-status.c | 10 +++---
37 files changed, 89 insertions(+), 78 deletions(-)
diff --git a/Documentation/technical/api-revision-walking.txt
b/Documentation/technical/api-revision-walking.txt
index 55b878ade8..03f9ea6ac4 100644
--- a/Documentation/technical/api-revision-walking.txt
+++ b/Documentation/technical/api-revision-walking.txt
@@ -15,9 +15,9 @@ revision list.
Functions
-
-`init_revisions`::
+`repo_init_revisions`::
- Initialize a rev_info structure with default values. The second
+ Initialize a rev_info structure with default values. The third
parameter may be NULL or can be prefix path, and then the `.prefix`
variable will be set to it. This is typically the first function you
want to call when you want to deal with a revision list. After calling
diff --git a/bisect.c b/bisect.c
index e1275ba79e..6ae5e5b49e 100644
--- a/bisect.c
+++ b/bisect.c
@@ -632,7 +632,7 @@ static void bisect_rev_setup(struct rev_info *revs, const
char *prefix,
struct argv_array rev_argv = ARGV_ARRAY_INIT;
int i;
- init_revisions(revs, prefix);
+ repo_init_revisions(the_repository, revs, prefix);
revs->abbrev = 0;
revs->commit_format = CMIT_FMT_UNSPECIFIED;
@@ -889,7 +889,7 @@ static void show_diff_tree(const char *prefix, struct
commit *commit)
struct rev_info opt;
/* diff-tree init */
- init_revisions(, prefix);
+ repo_init_revisions(the_repository, , prefix);
git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
opt.abbrev = 0;
opt.diff = 1;
diff --git a/builtin/add.c b/builtin/add.c
index 9916498a29..f94b614c1c 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -110,7 +110,7 @@ int add_files_to_cache(const char *prefix,
memset(, 0, sizeof(data));
data.flags = flags;
- init_revisions(, prefix);
+ repo_init_revisions(the_repository, , prefix);
setup_revisions(0, NULL, , NULL);
if (pathspec)
copy_pathspec(_data, pathspec);
@@ -232,7 +232,7 @@ static int edit_patch(int argc, const char **argv, const
char *prefix)
if (read_cache() < 0)
die(_("Could not read the index"));
- init_revisions(, prefix);
+ repo_init_revisions(the_repository, , prefix);
rev.diffopt.context = 7;
argc = setup_revisions(argc, argv, , NULL);
diff --git a/builtin/am.c b/builtin/am.c
index 4c7a5bc576..601570dbef 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1372,7 +1372,7 @@ static void write_commit_patch(const struct am_state
*state, struct commit *comm
FILE *fp;
fp = xfopen(am_path(state, "patch"), "w");
- init_revisions(_info, NULL);
+ repo_init_revisions(the_repository, _info, NULL);
rev_info.diff = 1;
rev_info.abbrev = 0;
[PATCH v5 23/23] revision.c: reduce implicit dependency the_repository
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: Junio C Hamano
---
list-objects.c | 8 +---
revision.c | 44 +++-
revision.h | 2 +-
3 files changed, 29 insertions(+), 25 deletions(-)
diff --git a/list-objects.c b/list-objects.c
index c99c47ac18..0c2989d5ca 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -196,7 +196,7 @@ static void mark_edge_parents_uninteresting(struct commit
*commit,
struct commit *parent = parents->item;
if (!(parent->object.flags & UNINTERESTING))
continue;
- mark_tree_uninteresting(get_commit_tree(parent));
+ mark_tree_uninteresting(revs->repo, get_commit_tree(parent));
if (revs->edge_hint && !(parent->object.flags & SHOWN)) {
parent->object.flags |= SHOWN;
show_edge(parent);
@@ -213,7 +213,8 @@ void mark_edges_uninteresting(struct rev_info *revs,
show_edge_fn show_edge)
struct commit *commit = list->item;
if (commit->object.flags & UNINTERESTING) {
- mark_tree_uninteresting(get_commit_tree(commit));
+ mark_tree_uninteresting(revs->repo,
+ get_commit_tree(commit));
if (revs->edge_hint_aggressive &&
!(commit->object.flags & SHOWN)) {
commit->object.flags |= SHOWN;
show_edge(commit);
@@ -228,7 +229,8 @@ void mark_edges_uninteresting(struct rev_info *revs,
show_edge_fn show_edge)
struct commit *commit = (struct commit *)obj;
if (obj->type != OBJ_COMMIT || !(obj->flags &
UNINTERESTING))
continue;
- mark_tree_uninteresting(get_commit_tree(commit));
+ mark_tree_uninteresting(revs->repo,
+ get_commit_tree(commit));
if (!(obj->flags & SHOWN)) {
obj->flags |= SHOWN;
show_edge(commit);
diff --git a/revision.c b/revision.c
index f8a801d5f6..28366eaccf 100644
--- a/revision.c
+++ b/revision.c
@@ -51,7 +51,8 @@ static void mark_blob_uninteresting(struct blob *blob)
blob->object.flags |= UNINTERESTING;
}
-static void mark_tree_contents_uninteresting(struct tree *tree)
+static void mark_tree_contents_uninteresting(struct repository *r,
+struct tree *tree)
{
struct tree_desc desc;
struct name_entry entry;
@@ -63,10 +64,10 @@ static void mark_tree_contents_uninteresting(struct tree
*tree)
while (tree_entry(, )) {
switch (object_type(entry.mode)) {
case OBJ_TREE:
- mark_tree_uninteresting(lookup_tree(the_repository,
entry.oid));
+ mark_tree_uninteresting(r, lookup_tree(r, entry.oid));
break;
case OBJ_BLOB:
- mark_blob_uninteresting(lookup_blob(the_repository,
entry.oid));
+ mark_blob_uninteresting(lookup_blob(r, entry.oid));
break;
default:
/* Subproject commit - not in this repository */
@@ -81,7 +82,7 @@ static void mark_tree_contents_uninteresting(struct tree
*tree)
free_tree_buffer(tree);
}
-void mark_tree_uninteresting(struct tree *tree)
+void mark_tree_uninteresting(struct repository *r, struct tree *tree)
{
struct object *obj;
@@ -92,7 +93,7 @@ void mark_tree_uninteresting(struct tree *tree)
if (obj->flags & UNINTERESTING)
return;
obj->flags |= UNINTERESTING;
- mark_tree_contents_uninteresting(tree);
+ mark_tree_contents_uninteresting(r, tree);
}
struct commit_stack {
@@ -198,7 +199,7 @@ void add_head_to_pending(struct rev_info *revs)
struct object *obj;
if (get_oid("HEAD", ))
return;
- obj = parse_object(the_repository, );
+ obj = parse_object(revs->repo, );
if (!obj)
return;
add_pending_object(revs, obj, "HEAD");
@@ -210,7 +211,7 @@ static struct object *get_reference(struct rev_info *revs,
const char *name,
{
struct object *object;
- object = parse_object(the_repository, oid);
+ object = parse_object(revs->repo, oid);
if (!object) {
if (revs->ignore_missing)
return object;
@@ -247,7 +248,7 @@ static struct commit *handle_commit(struct rev_info *revs,
add_pending_object(revs, object, tag->tag);
if (!tag->tagged)
die("bad tag");
- object = parse_object(the_repository, >tagged->oid);
+ object =
[PATCH v5 17/23] userdiff.c: remove implicit dependency on the_index
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: Junio C Hamano
---
archive-zip.c | 14 +-
builtin/grep.c | 3 ++-
combine-diff.c | 2 +-
diff.c | 40 +++-
diff.h | 3 ++-
diffcore-pickaxe.c | 4 ++--
grep.c | 21 -
grep.h | 3 ++-
line-range.c | 2 +-
userdiff.c | 5 +++--
userdiff.h | 3 ++-
11 files changed, 59 insertions(+), 41 deletions(-)
diff --git a/archive-zip.c b/archive-zip.c
index 5a62351ab1..155ee4a779 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -264,9 +264,10 @@ static int has_only_ascii(const char *s)
}
}
-static int entry_is_binary(const char *path, const void *buffer, size_t size)
+static int entry_is_binary(struct index_state *istate, const char *path,
+ const void *buffer, size_t size)
{
- struct userdiff_driver *driver = userdiff_find_by_path(path);
+ struct userdiff_driver *driver = userdiff_find_by_path(istate, path);
if (!driver)
driver = userdiff_find_by_name("default");
if (driver->binary != -1)
@@ -352,7 +353,8 @@ static int write_zip_entry(struct archiver_args *args,
return error(_("cannot read %s"),
oid_to_hex(oid));
crc = crc32(crc, buffer, size);
- is_binary = entry_is_binary(path_without_prefix,
+ is_binary = entry_is_binary(args->repo->index,
+ path_without_prefix,
buffer, size);
out = buffer;
}
@@ -428,7 +430,8 @@ static int write_zip_entry(struct archiver_args *args,
break;
crc = crc32(crc, buf, readlen);
if (is_binary == -1)
- is_binary = entry_is_binary(path_without_prefix,
+ is_binary = entry_is_binary(args->repo->index,
+ path_without_prefix,
buf, readlen);
write_or_die(1, buf, readlen);
}
@@ -460,7 +463,8 @@ static int write_zip_entry(struct archiver_args *args,
break;
crc = crc32(crc, buf, readlen);
if (is_binary == -1)
- is_binary = entry_is_binary(path_without_prefix,
+ is_binary = entry_is_binary(args->repo->index,
+ path_without_prefix,
buf, readlen);
zstream.next_in = buf;
diff --git a/builtin/grep.c b/builtin/grep.c
index 0667ffde84..0c3527242e 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -103,7 +103,8 @@ static void add_work(struct grep_opt *opt, const struct
grep_source *gs)
todo[todo_end].source = *gs;
if (opt->binary != GREP_BINARY_TEXT)
- grep_source_load_driver([todo_end].source);
+ grep_source_load_driver([todo_end].source,
+ opt->repo->index);
todo[todo_end].done = 0;
strbuf_reset([todo_end].out);
todo_end = (todo_end + 1) % ARRAY_SIZE(todo);
diff --git a/combine-diff.c b/combine-diff.c
index 9b43e557a1..41ab5b01de 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -987,7 +987,7 @@ static void show_patch_diff(struct combine_diff_path *elem,
int num_parent,
const char *line_prefix = diff_line_prefix(opt);
context = opt->context;
- userdiff = userdiff_find_by_path(elem->path);
+ userdiff = userdiff_find_by_path(opt->repo->index, elem->path);
if (!userdiff)
userdiff = userdiff_find_by_name("default");
if (opt->flags.allow_textconv)
diff --git a/diff.c b/diff.c
index 140d0e86df..5256b9eabc 100644
--- a/diff.c
+++ b/diff.c
@@ -2093,23 +2093,25 @@ static void diff_words_flush(struct emit_callback
*ecbdata)
}
}
-static void diff_filespec_load_driver(struct diff_filespec *one)
+static void diff_filespec_load_driver(struct diff_filespec *one,
+ struct index_state *istate)
{
/* Use already-loaded driver */
if (one->driver)
return;
if (S_ISREG(one->mode))
- one->driver = userdiff_find_by_path(one->path);
+ one->driver = userdiff_find_by_path(istate, one->path);
/* Fallback to default settings */
if (!one->driver)
one->driver = userdiff_find_by_name("default");
}
-static const char *userdiff_word_regex(struct diff_filespec *one)
[PATCH v5 19/23] submodule.c: remove implicit dependency on the_index
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: Junio C Hamano
---
builtin/pull.c | 2 +-
submodule.c| 28 +---
submodule.h| 9 ++---
transport.c| 9 ++---
4 files changed, 30 insertions(+), 18 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index 33b7100837..9c455984d1 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -944,7 +944,7 @@ int cmd_pull(int argc, const char **argv, const char
*prefix)
int ret = 0;
if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
- submodule_touches_in_range(_fork_point, _head))
+ submodule_touches_in_range(_index, _fork_point,
_head))
die(_("cannot rebase with locally recorded submodule
modifications"));
if (!autostash) {
struct commit_list *list = NULL;
diff --git a/submodule.c b/submodule.c
index a2b266fbfa..58427b5dfc 100644
--- a/submodule.c
+++ b/submodule.c
@@ -766,7 +766,8 @@ static void collect_changed_submodules_cb(struct
diff_queue_struct *q,
* have a corresponding 'struct oid_array' (in the 'util' field) which lists
* what the submodule pointers were updated to during the change.
*/
-static void collect_changed_submodules(struct string_list *changed,
+static void collect_changed_submodules(struct index_state *istate,
+ struct string_list *changed,
struct argv_array *argv)
{
struct rev_info rev;
@@ -930,8 +931,10 @@ static int submodule_needs_pushing(const char *path,
struct oid_array *commits)
return 0;
}
-int find_unpushed_submodules(struct oid_array *commits,
- const char *remotes_name, struct string_list *needs_pushing)
+int find_unpushed_submodules(struct index_state *istate,
+struct oid_array *commits,
+const char *remotes_name,
+struct string_list *needs_pushing)
{
struct string_list submodules = STRING_LIST_INIT_DUP;
struct string_list_item *name;
@@ -943,7 +946,7 @@ int find_unpushed_submodules(struct oid_array *commits,
argv_array_push(, "--not");
argv_array_pushf(, "--remotes=%s", remotes_name);
- collect_changed_submodules(, );
+ collect_changed_submodules(istate, , );
for_each_string_list_item(name, ) {
struct oid_array *commits = name->util;
@@ -1044,7 +1047,8 @@ static void submodule_push_check(const char *path, const
char *head,
die("process for submodule '%s' failed", path);
}
-int push_unpushed_submodules(struct oid_array *commits,
+int push_unpushed_submodules(struct index_state *istate,
+struct oid_array *commits,
const struct remote *remote,
const struct refspec *rs,
const struct string_list *push_options,
@@ -1053,7 +1057,8 @@ int push_unpushed_submodules(struct oid_array *commits,
int i, ret = 1;
struct string_list needs_pushing = STRING_LIST_INIT_DUP;
- if (!find_unpushed_submodules(commits, remote->name, _pushing))
+ if (!find_unpushed_submodules(istate, commits,
+ remote->name, _pushing))
return 1;
/*
@@ -1110,7 +1115,7 @@ void check_for_new_submodule_commits(struct object_id
*oid)
oid_array_append(_tips_after_fetch, oid);
}
-static void calculate_changed_submodule_paths(void)
+static void calculate_changed_submodule_paths(struct index_state *istate)
{
struct argv_array argv = ARGV_ARRAY_INIT;
struct string_list changed_submodules = STRING_LIST_INIT_DUP;
@@ -1131,7 +1136,7 @@ static void calculate_changed_submodule_paths(void)
* Collect all submodules (whether checked out or not) for which new
* commits have been recorded upstream in "changed_submodule_names".
*/
- collect_changed_submodules(_submodules, );
+ collect_changed_submodules(istate, _submodules, );
for_each_string_list_item(name, _submodules) {
struct oid_array *commits = name->util;
@@ -1158,7 +1163,8 @@ static void calculate_changed_submodule_paths(void)
initialized_fetch_ref_tips = 0;
}
-int submodule_touches_in_range(struct object_id *excl_oid,
+int submodule_touches_in_range(struct index_state *istate,
+ struct object_id *excl_oid,
struct object_id *incl_oid)
{
struct string_list subs = STRING_LIST_INIT_DUP;
@@ -1176,7 +1182,7 @@ int submodule_touches_in_range(struct object_id *excl_oid,
argv_array_push(, oid_to_hex(excl_oid));
}
- collect_changed_submodules(, );
+ collect_changed_submodules(istate, ,
[PATCH v5 15/23] sha1-file.c: remove implicit dependency on the_index
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: Junio C Hamano
---
builtin/difftool.c | 2 +-
builtin/hash-object.c | 2 +-
builtin/replace.c | 2 +-
builtin/update-index.c | 2 +-
cache.h| 4 ++--
diff.c | 20 -
notes-merge.c | 2 +-
read-cache.c | 25 -
sha1-file.c| 50 --
9 files changed, 61 insertions(+), 48 deletions(-)
diff --git a/builtin/difftool.c b/builtin/difftool.c
index cdd585ca76..e7023e3adf 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -112,7 +112,7 @@ static int use_wt_file(const char *workdir, const char
*name,
int fd = open(buf.buf, O_RDONLY);
if (fd >= 0 &&
- !index_fd(_oid, fd, , OBJ_BLOB, name, 0)) {
+ !index_fd(_index, _oid, fd, , OBJ_BLOB, name, 0))
{
if (is_null_oid(oid)) {
oidcpy(oid, _oid);
use = 1;
diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index 9ada4f4dfd..d6f06ea32f 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c
@@ -40,7 +40,7 @@ static void hash_fd(int fd, const char *type, const char
*path, unsigned flags,
if (fstat(fd, ) < 0 ||
(literally
? hash_literally(, fd, type, flags)
-: index_fd(, fd, , type_from_string(type), path, flags)))
+: index_fd(_index, , fd, , type_from_string(type),
path, flags)))
die((flags & HASH_WRITE_OBJECT)
? "Unable to add %s to database"
: "Unable to hash %s", path);
diff --git a/builtin/replace.c b/builtin/replace.c
index 4f05791f3e..e0b16ad44b 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -295,7 +295,7 @@ static int import_object(struct object_id *oid, enum
object_type type,
close(fd);
return -1;
}
- if (index_fd(oid, fd, , type, NULL, flags) < 0)
+ if (index_fd(_index, oid, fd, , type, NULL, flags) < 0)
return error(_("unable to write object to database"));
/* index_fd close()s fd for us */
}
diff --git a/builtin/update-index.c b/builtin/update-index.c
index fe84003b4f..3086212fdb 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -282,7 +282,7 @@ static int add_one_path(const struct cache_entry *old,
const char *path, int len
fill_stat_cache_info(ce, st);
ce->ce_mode = ce_mode_from_stat(old, st->st_mode);
- if (index_path(>oid, path, st,
+ if (index_path(_index, >oid, path, st,
info_only ? 0 : HASH_WRITE_OBJECT)) {
discard_cache_entry(ce);
return -1;
diff --git a/cache.h b/cache.h
index 49fe83331c..094652a503 100644
--- a/cache.h
+++ b/cache.h
@@ -787,8 +787,8 @@ extern int ie_modified(struct index_state *, const struct
cache_entry *, struct
#define HASH_WRITE_OBJECT 1
#define HASH_FORMAT_CHECK 2
#define HASH_RENORMALIZE 4
-extern int index_fd(struct object_id *oid, int fd, struct stat *st, enum
object_type type, const char *path, unsigned flags);
-extern int index_path(struct object_id *oid, const char *path, struct stat
*st, unsigned flags);
+extern int index_fd(struct index_state *istate, struct object_id *oid, int fd,
struct stat *st, enum object_type type, const char *path, unsigned flags);
+extern int index_path(struct index_state *istate, struct object_id *oid, const
char *path, struct stat *st, unsigned flags);
/*
* Record to sd the data from st that we use to check whether a file
diff --git a/diff.c b/diff.c
index a06c3b070c..140d0e86df 100644
--- a/diff.c
+++ b/diff.c
@@ -4252,7 +4252,7 @@ static void run_diff_cmd(const char *pgm,
fprintf(o->file, "* Unmerged path %s\n", name);
}
-static void diff_fill_oid_info(struct diff_filespec *one)
+static void diff_fill_oid_info(struct diff_filespec *one, struct index_state
*istate)
{
if (DIFF_FILE_VALID(one)) {
if (!one->oid_valid) {
@@ -4263,7 +4263,7 @@ static void diff_fill_oid_info(struct diff_filespec *one)
}
if (lstat(one->path, ) < 0)
die_errno("stat '%s'", one->path);
- if (index_path(>oid, one->path, , 0))
+ if (index_path(istate, >oid, one->path, , 0))
die("cannot hash %s", one->path);
}
}
@@ -4311,8 +4311,8 @@ static void run_diff(struct diff_filepair *p, struct
diff_options *o)
return;
}
- diff_fill_oid_info(one);
- diff_fill_oid_info(two);
+ diff_fill_oid_info(one, o->repo->index);
+ diff_fill_oid_info(two, o->repo->index);
if (!pgm &&
[PATCH v5 18/23] line-range.c: remove implicit dependency on the_index
Signed-off-by: Nguyễn Thái Ngọc Duy
Signed-off-by: Junio C Hamano
---
builtin/blame.c | 2 +-
line-log.c | 4 ++--
line-range.c| 22 ++
line-range.h| 6 --
4 files changed, 21 insertions(+), 13 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index c2da673ac8..97632828db 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1001,7 +1001,7 @@ int cmd_blame(int argc, const char **argv, const char
*prefix)
long bottom, top;
if (parse_range_arg(range_list.items[range_i].string,
nth_line_cb, , lno, anchor,
- , , sb.path))
+ , , sb.path, _index))
usage(blame_usage);
if ((!lno && (top || bottom)) || lno < bottom)
die(Q_("file %s has only %lu line",
diff --git a/line-log.c b/line-log.c
index 35adf199a5..d1d429d738 100644
--- a/line-log.c
+++ b/line-log.c
@@ -574,7 +574,7 @@ parse_lines(struct repository *r, struct commit *commit,
long begin = 0, end = 0;
long anchor;
- name_part = skip_range_arg(item->string);
+ name_part = skip_range_arg(item->string, r->index);
if (!name_part || *name_part != ':' || !name_part[1])
die("-L argument not 'start,end:file' or
':funcname:file': %s",
item->string);
@@ -599,7 +599,7 @@ parse_lines(struct repository *r, struct commit *commit,
if (parse_range_arg(range_part, nth_line, _data,
lines, anchor, , ,
- full_name))
+ full_name, r->index))
die("malformed -L argument '%s'", range_part);
if ((!lines && (begin || end)) || lines < begin)
die("file %s has only %lu lines", name_part, lines);
diff --git a/line-range.c b/line-range.c
index 7fa0d8bba5..9b50583dc0 100644
--- a/line-range.c
+++ b/line-range.c
@@ -163,9 +163,10 @@ static const char
*find_funcname_matching_regexp(xdemitconf_t *xecfg, const char
}
}
-static const char *parse_range_funcname(const char *arg, nth_line_fn_t
nth_line_cb,
- void *cb_data, long lines, long anchor,
long *begin, long *end,
- const char *path)
+static const char *parse_range_funcname(
+ const char *arg, nth_line_fn_t nth_line_cb,
+ void *cb_data, long lines, long anchor, long *begin, long *end,
+ const char *path, struct index_state *istate)
{
char *pattern;
const char *term;
@@ -198,7 +199,7 @@ static const char *parse_range_funcname(const char *arg,
nth_line_fn_t nth_line_
anchor--; /* input is in human terms */
start = nth_line_cb(cb_data, anchor);
- drv = userdiff_find_by_path(_index, path);
+ drv = userdiff_find_by_path(istate, path);
if (drv && drv->funcname.pattern) {
const struct userdiff_funcname *pe = >funcname;
xecfg = xcalloc(1, sizeof(*xecfg));
@@ -244,7 +245,8 @@ static const char *parse_range_funcname(const char *arg,
nth_line_fn_t nth_line_
int parse_range_arg(const char *arg, nth_line_fn_t nth_line_cb,
void *cb_data, long lines, long anchor,
- long *begin, long *end, const char *path)
+ long *begin, long *end,
+ const char *path, struct index_state *istate)
{
*begin = *end = 0;
@@ -254,7 +256,9 @@ int parse_range_arg(const char *arg, nth_line_fn_t
nth_line_cb,
anchor = lines + 1;
if (*arg == ':' || (*arg == '^' && *(arg + 1) == ':')) {
- arg = parse_range_funcname(arg, nth_line_cb, cb_data, lines,
anchor, begin, end, path);
+ arg = parse_range_funcname(arg, nth_line_cb, cb_data,
+ lines, anchor, begin, end,
+ path, istate);
if (!arg || *arg)
return -1;
return 0;
@@ -275,10 +279,12 @@ int parse_range_arg(const char *arg, nth_line_fn_t
nth_line_cb,
return 0;
}
-const char *skip_range_arg(const char *arg)
+const char *skip_range_arg(const char *arg, struct index_state *istate)
{
if (*arg == ':' || (*arg == '^' && *(arg + 1) == ':'))
- return parse_range_funcname(arg, NULL, NULL, 0, 0, NULL, NULL,
NULL);
+ return parse_range_funcname(arg, NULL, NULL,
+ 0, 0, NULL, NULL,
+ NULL, istate);
arg = parse_loc(arg, NULL, NULL, 0, -1, NULL);
diff --git a/line-range.h b/line-range.h
index d3c54e45aa..e69bf7c017 100644
--- a/line-range.h
+++ b/line-range.h
@@ -1,6 +1,8 @@
[PATCH v5 20/23] tree-diff.c: remove implicit dependency on the_index
Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano --- tree-diff.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tree-diff.c b/tree-diff.c index 57a15f51f0..16b28ff6d6 100644 --- a/tree-diff.c +++ b/tree-diff.c @@ -605,7 +605,7 @@ static void try_to_follow_renames(const struct object_id *old_oid, choice = q->queue[0]; q->nr = 0; - repo_diff_setup(the_repository, _opts); + repo_diff_setup(opt->repo, _opts); diff_opts.flags.recursive = 1; diff_opts.flags.find_copies_harder = 1; diff_opts.output_format = DIFF_FORMAT_NO_OUTPUT; -- 2.19.0.640.gcd3aa10a8a
