| Message ID | 20190930211018.23633-3-newren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fast export/import: handle nested tags, improve incremental exports | expand |
Am 30.09.19 um 23:10 schrieb Elijah Newren: > If our input stream includes a tag which is later deleted, we were not > properly deleting it. We did have a step which would delete it, but we > left a tag in the tag list noting that it needed to be updated, and the > updating of annotated tags occurred AFTER ref deletion. So, when we > record that a tag needs to be deleted, also remove it from the list of > annotated tags to update. > > While this has likely been something that has not happened in practice, > it will come up more in order to support nested tags. For nested tags, > we either need to give temporary names to the intermediate tags and then > delete them, or else we need to use the final name for the intermediate > tags. If we use the final name for the intermediate tags, then in order > to keep the sanity check that someone doesn't try to update the same tag > twice, we need to delete the ref after creating the intermediate tag. > So, either way nested tags imply the need to delete temporary inner tag > references. > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > fast-import.c | 29 +++++++++++++++++++++++++++++ > t/t9300-fast-import.sh | 13 +++++++++++++ > 2 files changed, 42 insertions(+) > > diff --git a/fast-import.c b/fast-import.c > index b44d6a467e..546da3a938 100644 > --- a/fast-import.c > +++ b/fast-import.c > @@ -2793,6 +2793,35 @@ static void parse_reset_branch(const char *arg) > b = new_branch(arg); > read_next_command(); > parse_from(b); > + if (b->delete && !strncmp(b->name, "refs/tags/", 10)) { b->name is a NUL-terminated string; starts_with() could be used to avoid the magic number 10. > + /* > + * Elsewhere, we call dump_branches() before dump_tags(), > + * and dump_branches() will handle ref deletions first, so > + * in order to make sure the deletion actually takes effect, > + * we need to remove the tag from our list of tags to update. > + * > + * NEEDSWORK: replace list of tags with hashmap for faster > + * deletion? > + */ > + struct strbuf tag_name = STRBUF_INIT; This adds a small memory leak. > + struct tag *t, *prev = NULL; > + for (t = first_tag; t; t = t->next_tag) { > + strbuf_reset(&tag_name); > + strbuf_addf(&tag_name, "refs/tags/%s", t->name); > + if (!strcmp(b->name, tag_name.buf)) So the strbuf is used to prefix t->name with "refs/tags/", which we know b->name starts with, and to compare the result with b->name. Removing the "refs/tags/" prefix from b->name using skip_prefix() and comparing the result with t->name would be easier. > + break; > + prev = t; > + } > + if (t) { > + if (prev) > + prev->next_tag = t->next_tag; > + else > + first_tag = t->next_tag; > + if (!t->next_tag) > + last_tag = prev; > + /* There is no mem_pool_free(t) function to call. */ > + } > + } > if (command_buf.len > 0) > unread_command_buf = 1; > } Here's a squashable patch for that: --- fast-import.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/fast-import.c b/fast-import.c index 70cd3f0ff4..a109591406 100644 --- a/fast-import.c +++ b/fast-import.c @@ -2779,6 +2779,7 @@ static void parse_new_tag(const char *arg) static void parse_reset_branch(const char *arg) { struct branch *b; + const char *tag_name; b = lookup_branch(arg); if (b) { @@ -2794,7 +2795,7 @@ static void parse_reset_branch(const char *arg) b = new_branch(arg); read_next_command(); parse_from(b); - if (b->delete && !strncmp(b->name, "refs/tags/", 10)) { + if (b->delete && skip_prefix(b->name, "refs/tags/", &tag_name)) { /* * Elsewhere, we call dump_branches() before dump_tags(), * and dump_branches() will handle ref deletions first, so @@ -2804,12 +2805,9 @@ static void parse_reset_branch(const char *arg) * NEEDSWORK: replace list of tags with hashmap for faster * deletion? */ - struct strbuf tag_name = STRBUF_INIT; struct tag *t, *prev = NULL; for (t = first_tag; t; t = t->next_tag) { - strbuf_reset(&tag_name); - strbuf_addf(&tag_name, "refs/tags/%s", t->name); - if (!strcmp(b->name, tag_name.buf)) + if (!strcmp(t->name, tag_name)) break; prev = t; } -- 2.23.0
diff --git a/fast-import.c b/fast-import.c index b44d6a467e..546da3a938 100644 --- a/fast-import.c +++ b/fast-import.c @@ -2793,6 +2793,35 @@ static void parse_reset_branch(const char *arg) b = new_branch(arg); read_next_command(); parse_from(b); + if (b->delete && !strncmp(b->name, "refs/tags/", 10)) { + /* + * Elsewhere, we call dump_branches() before dump_tags(), + * and dump_branches() will handle ref deletions first, so + * in order to make sure the deletion actually takes effect, + * we need to remove the tag from our list of tags to update. + * + * NEEDSWORK: replace list of tags with hashmap for faster + * deletion? + */ + struct strbuf tag_name = STRBUF_INIT; + struct tag *t, *prev = NULL; + for (t = first_tag; t; t = t->next_tag) { + strbuf_reset(&tag_name); + strbuf_addf(&tag_name, "refs/tags/%s", t->name); + if (!strcmp(b->name, tag_name.buf)) + break; + prev = t; + } + if (t) { + if (prev) + prev->next_tag = t->next_tag; + else + first_tag = t->next_tag; + if (!t->next_tag) + last_tag = prev; + /* There is no mem_pool_free(t) function to call. */ + } + } if (command_buf.len > 0) unread_command_buf = 1; } diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index 141b7fa35e..74bc41333b 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -85,6 +85,15 @@ test_expect_success 'A: create pack from stdin' ' An annotated tag that annotates a blob. EOF + tag to-be-deleted + from :3 + data <<EOF + Another annotated tag that annotates a blob. + EOF + + reset refs/tags/to-be-deleted + from 0000000000000000000000000000000000000000 + INPUT_END git fast-import --export-marks=marks.out <input && git whatchanged master @@ -157,6 +166,10 @@ test_expect_success 'A: verify tag/series-A-blob' ' test_cmp expect actual ' +test_expect_success 'A: verify tag deletion is successful' ' + test_must_fail git rev-parse --verify refs/tags/to-be-deleted +' + test_expect_success 'A: verify marks output' ' cat >expect <<-EOF && :2 $(git rev-parse --verify master:file2)
If our input stream includes a tag which is later deleted, we were not properly deleting it. We did have a step which would delete it, but we left a tag in the tag list noting that it needed to be updated, and the updating of annotated tags occurred AFTER ref deletion. So, when we record that a tag needs to be deleted, also remove it from the list of annotated tags to update. While this has likely been something that has not happened in practice, it will come up more in order to support nested tags. For nested tags, we either need to give temporary names to the intermediate tags and then delete them, or else we need to use the final name for the intermediate tags. If we use the final name for the intermediate tags, then in order to keep the sanity check that someone doesn't try to update the same tag twice, we need to delete the ref after creating the intermediate tag. So, either way nested tags imply the need to delete temporary inner tag references. Signed-off-by: Elijah Newren <newren@gmail.com> --- fast-import.c | 29 +++++++++++++++++++++++++++++ t/t9300-fast-import.sh | 13 +++++++++++++ 2 files changed, 42 insertions(+)