@matheusbsbs / matheus-bagliano / commits / 37b9dcabfc

Fork 0

shallow.c: use '{commit,rollback}_shallow_file'

In bd0b42aed3 (fetch-pack: do not take shallow lock unnecessarily, 2019-01-10), the author noted that 'is_repository_shallow' produces visible side-effect(s) by setting 'is_shallow' and 'shallow_stat'. This is a problem for e.g., fetching with '--update-shallow' in a shallow repository with 'fetch.writeCommitGraph' enabled, since the update to '.git/shallow' will cause Git to think that the repository isn't shallow when it is, thereby circumventing the commit-graph compatibility check. This causes problems in shallow repositories with at least shallow refs that have at least one ancestor (since the client won't have those objects, and therefore can't take the reachability closure over commits when writing a commit-graph). Address this by introducing thin wrappers over 'commit_lock_file' and 'rollback_lock_file' for use specifically when the lock is held over '.git/shallow'. These wrappers (appropriately called 'commit_shallow_file' and 'rollback_shallow_file') call into their respective functions in 'lockfile.h', but additionally reset validity checks used by the shallow machinery. Replace each instance of 'commit_lock_file' and 'rollback_lock_file' with 'commit_shallow_file' and 'rollback_shallow_file' when the lock being held is over the '.git/shallow' file. As a result, 'prune_shallow' can now only be called once (since 'check_shallow_file_for_update' will die after calling 'reset_repository_shallow'). But, this is OK since we only call 'prune_shallow' at most once per process. Helped-by: Jonathan Tan <jonathantanmy@google.com> Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Taylor Blau committed Apr 22, 2020 at 18:25 UTC 37b9dcabfc48b0cbce638140279878dac37aec73

5 files changed +59 -16

builtin/receive-pack.c

+2 -2

@@ -872,12 +872,12 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)

872 opt.env = tmp_objdir_env(tmp_objdir);

873 setup_alternate_shallow(&shallow_lock, &opt.shallow_file, &extra);

874 if (check_connected(command_singleton_iterator, cmd, &opt)) {

875 - rollback_lock_file(&shallow_lock);

875 + rollback_shallow_file(the_repository, &shallow_lock);

876 oid_array_clear(&extra);

877 return -1;

878 }

879

880 - commit_lock_file(&shallow_lock);

880 + commit_shallow_file(the_repository, &shallow_lock);

881

882 /*

883 * Make sure setup_alternate_shallow() for the next ref does

commit.h

@@ -249,6 +249,8 @@ struct oid_array;

249 struct ref;

250 int register_shallow(struct repository *r, const struct object_id *oid);

251 int unregister_shallow(const struct object_id *oid);

252 +int commit_shallow_file(struct repository *r, struct lock_file *lk);

253 +void rollback_shallow_file(struct repository *r, struct lock_file *lk);

254 int for_each_commit_graft(each_commit_graft_fn, void *);

255 int is_repository_shallow(struct repository *r);

256 struct commit_list *get_shallow_commits(struct object_array *heads,

fetch-pack.c

+5 -5

@@ -1629,9 +1629,9 @@ static void update_shallow(struct fetch_pack_args *args,

1629 if (args->deepen && alternate_shallow_file) {

1630 if (*alternate_shallow_file == '\0') { /* --unshallow */

1631 unlink_or_warn(git_path_shallow(the_repository));

1632 - rollback_lock_file(&shallow_lock);

1632 + rollback_shallow_file(the_repository, &shallow_lock);

1633 } else

1634 - commit_lock_file(&shallow_lock);

1634 + commit_shallow_file(the_repository, &shallow_lock);

1635 alternate_shallow_file = NULL;

1636 return;

1637 }

@@ -1655,7 +1655,7 @@ static void update_shallow(struct fetch_pack_args *args,

1655 setup_alternate_shallow(&shallow_lock,

1656 &alternate_shallow_file,

1657 &extra);

1658 - commit_lock_file(&shallow_lock);

1658 + commit_shallow_file(the_repository, &shallow_lock);

1659 alternate_shallow_file = NULL;

1660 }

1661 oid_array_clear(&extra);

@@ -1693,7 +1693,7 @@ static void update_shallow(struct fetch_pack_args *args,

1693 setup_alternate_shallow(&shallow_lock,

1694 &alternate_shallow_file,

1695 &extra);

1696 - commit_lock_file(&shallow_lock);

1696 + commit_shallow_file(the_repository, &shallow_lock);

1697 oid_array_clear(&extra);

1698 oid_array_clear(&ref);

1699 alternate_shallow_file = NULL;

@@ -1785,7 +1785,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,

1785 error(_("remote did not send all necessary objects"));

1786 free_refs(ref_cpy);

1787 ref_cpy = NULL;

1788 - rollback_lock_file(&shallow_lock);

1788 + rollback_shallow_file(the_repository, &shallow_lock);

1789 goto cleanup;

1790 }

1791 args->connectivity_checked = 1;

shallow.c

+21 -9

@@ -40,13 +40,6 @@ int register_shallow(struct repository *r, const struct object_id *oid)

41 int is_repository_shallow(struct repository *r)

42 {

43 - /*

44 - * NEEDSWORK: This function updates

45 - * r->parsed_objects->{is_shallow,shallow_stat} as a side effect but

46 - * there is no corresponding function to clear them when the shallow

47 - * file is updated.

48 - */

49 -

43 FILE *fp;

44 char buf[1024];

45 const char *path = r->parsed_objects->alternate_shallow_file;

@@ -79,6 +72,25 @@ int is_repository_shallow(struct repository *r)

72 return r->parsed_objects->is_shallow;

73 }

75 +static void reset_repository_shallow(struct repository *r)

76 +{

77 + r->parsed_objects->is_shallow = -1;

78 + stat_validity_clear(r->parsed_objects->shallow_stat);

79 +}

80 +

81 +int commit_shallow_file(struct repository *r, struct lock_file *lk)

82 +{

83 + int res = commit_lock_file(lk);

84 + reset_repository_shallow(r);

85 + return res;

86 +}

87 +

88 +void rollback_shallow_file(struct repository *r, struct lock_file *lk)

89 +{

90 + rollback_lock_file(lk);

91 + reset_repository_shallow(r);

92 +}

93 +

94 /*

95 * TODO: use "int" elemtype instead of "int *" when/if commit-slab

96 * supports a "valid" flag.

@@ -410,10 +422,10 @@ void prune_shallow(unsigned options)

422 if (write_in_full(fd, sb.buf, sb.len) < 0)

423 die_errno("failed to write to %s",

424 get_lock_file_path(&shallow_lock));

413 - commit_lock_file(&shallow_lock);

425 + commit_shallow_file(the_repository, &shallow_lock);

426 } else {

427 unlink(git_path_shallow(the_repository));

416 - rollback_lock_file(&shallow_lock);

428 + rollback_shallow_file(the_repository, &shallow_lock);

429 }

430 strbuf_release(&sb);

431 }

t/t5537-fetch-shallow.sh

+29

@@ -144,6 +144,35 @@ test_expect_success 'fetch --update-shallow' '

144 )

145 '

146

147 +test_expect_success 'fetch --update-shallow (with fetch.writeCommitGraph)' '

148 + (

149 + cd shallow &&

150 + git checkout master &&

151 + commit 8 &&

152 + git tag -m foo heavy-tag-for-graph HEAD^ &&

153 + git tag light-tag-for-graph HEAD^:tracked

154 + ) &&

155 + test_config -C notshallow fetch.writeCommitGraph true &&

156 + (

157 + cd notshallow &&

158 + git fetch --update-shallow ../shallow/.git refs/heads/*:refs/remotes/shallow/* &&

159 + git fsck &&

160 + git for-each-ref --sort=refname --format="%(refname)" >actual.refs &&

161 + cat <<-EOF >expect.refs &&

162 + refs/remotes/shallow/master

163 + refs/remotes/shallow/no-shallow

164 + refs/tags/heavy-tag

165 + refs/tags/heavy-tag-for-graph

166 + refs/tags/light-tag

167 + refs/tags/light-tag-for-graph

168 + EOF

169 + test_cmp expect.refs actual.refs &&

170 + git log --format=%s shallow/master >actual &&

171 + test_write_lines 8 7 6 5 4 3 >expect &&

172 + test_cmp expect actual

173 + )

174 +'

175 +

176 test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' '

177 cp -R .git read-only.git &&

178 test_when_finished "find read-only.git -type d -print | xargs chmod +w" &&