From: Dmitry Ivankov Date: Thu, 21 Jun 2012 06:39:34 GMT Subject: Re: [PATCH] fast-import: disallow empty branches as parents Message-ID: In-Reply-To: <20120621035753.GA3842@burratino> On Thu, Jun 21, 2012 at 9:57 AM, Jonathan Nieder wrote: > Hi Dmitry, > > Dmitry Ivankov wrote: > >> Empty branches (either new or reset-ed) have null_sha1 in fast-import >> internals. These null_sha1 heads can slip to the real commit objects. > [... nice explanation snipped ...] > > Very nice, thanks much for this. > > Would it be possible to split this into multiple independent fixes? > See [*] below for one way. > >> --- a/fast-import.c >> +++ b/fast-import.c >> @@ -2540,7 +2540,8 @@ static void file_change_deleteall(struct branch *b) >>       b->num_notes = 0; >>  } >> >> -static void parse_from_commit(struct branch *b, char *buf, unsigned long size) >> +static void parse_from_commit(struct branch *b, unsigned char *sha1, >> +                                             char *buf, unsigned long size) >>  { > > What is happening here?  The new argument doesn't seem to be used. Ow, sorry. In fact it is here for die() messages. A bit like spaghetti code, maybe die()-s can be moved to the caller. > > [...] >> @@ -2551,29 +2552,31 @@ static void parse_from_commit(struct branch *b, char *buf, unsigned long size) >>               b->branch_tree.versions[1].sha1); >>  } >> >> -static void parse_from_existing(struct branch *b) >> +static void parse_from_existing(struct branch *b, unsigned char *sha1) >>  { >> -     if (is_null_sha1(b->sha1)) { >> +     if (is_null_sha1(sha1)) { >>               hashclr(b->branch_tree.versions[0].sha1); >>               hashclr(b->branch_tree.versions[1].sha1); >>       } else { >>               unsigned long size; >>               char *buf; >> >> -             buf = read_object_with_reference(b->sha1, >> -                     commit_type, &size, b->sha1); >> +             buf = read_object_with_reference(sha1, >> +                     commit_type, &size, sha1); > > This seems to be about delaying the effect of "from" so it doesn't > interfere with a "merge" command referring to the same commit. Kind of, parse_from_*() arrange b->sha1 and b->branch_tree to correspond the first parent of a new tip. With this patch b->sha1 is left alone to avoid the interference. Oh, yes, luckily parse_merge() doesn't need b->branch_tree-s. > > [...] >> -static int parse_from(struct branch *b) >> +static int parse_from(struct branch *b, unsigned char *sha1out) >>  { >>       const char *from; >>       struct branch *s; >> >> -     if (prefixcmp(command_buf.buf, "from ")) >> +     if (prefixcmp(command_buf.buf, "from ")) { >> +             hashclr(sha1out); >>               return 0; >> +     } > > This code path handles the case where there is no "from" after a > "reset" or "commit" command.  We clear sha1out to make the calling > convention simple --- sha1out is always written to, and the caller > does not have to worry about initializing it in advance. > > I guess this is part of the change that delays the effect of "from"? > > [...] >> @@ -2586,7 +2589,10 @@ static int parse_from(struct branch *b) >>               die("Can't create a branch from itself: %s", b->name); >>       else if (s) { >>               unsigned char *t = s->branch_tree.versions[1].sha1; >> +             if (is_null_sha1(s->sha1)) >> +                     die("Can't create a branch from an empty branch:" >> +                             " %s from %s", b->name, s->name); > > This seems to be about protecting against "from" with an unborn > branch. > >> -             hashcpy(b->sha1, s->sha1); >> +             hashcpy(sha1out, s->sha1); > > Delaying the effect of "from", maybe. > > [...] >> @@ -2594,16 +2600,16 @@ static int parse_from(struct branch *b) >>               struct object_entry *oe = find_mark(idnum); >>               if (oe->type != OBJ_COMMIT) >>                       die("Mark :%" PRIuMAX " not a commit", idnum); >> -             hashcpy(b->sha1, oe->idx.sha1); >> +             hashcpy(sha1out, oe->idx.sha1); > > Delaying "from" effect? > >>               if (oe->pack_id != MAX_PACK_ID) { >>                       unsigned long size; >>                       char *buf = gfi_unpack_entry(oe, &size); >> -                     parse_from_commit(b, buf, size); >> +                     parse_from_commit(b, sha1out, buf, size); > > Likewise? > >>                       free(buf); >>               } else >> -                     parse_from_existing(b); >> +                     parse_from_existing(b, sha1out); >> -     } else if (!get_sha1(from, b->sha1)) >> +     } else if (!get_sha1(from, sha1out)) >> -             parse_from_existing(b); >> +             parse_from_existing(b, sha1out); > > Likewise? > > [...] >>       else >>               die("Invalid ref name or SHA1 expression: %s", from); >> >> @@ -2622,9 +2628,11 @@ static struct hash_list *parse_merge(unsigned int *count) >>               from = strchr(command_buf.buf, ' ') + 1; >>               n = xmalloc(sizeof(*n)); >>               s = lookup_branch(from); >> -             if (s) >> +             if (s) { >> +                     if (is_null_sha1(s->sha1)) >> +                             die("Can't merge empty branch: %s", s->name); >>                       hashcpy(n->sha1, s->sha1); >> -             else if (*from == ':') { >> +             } else if (*from == ':') { > > Protecting against "merge" with unknown branch. > > [...] >> @@ -2656,6 +2664,7 @@ static void parse_new_commit(void) >>  { >>       static struct strbuf msg = STRBUF_INIT; >>       struct branch *b; >> +     unsigned char sha1[20]; >>       char *sp; >>       char *author = NULL; >>       char *committer = NULL; >> @@ -2683,7 +2692,7 @@ static void parse_new_commit(void) >>               die("Expected committer but didn't get one"); >>       parse_data(&msg, 0, NULL); >>       read_next_command(); >> -     parse_from(b); >> +     parse_from(b, sha1); > > Delayed "from" effect? > > [...] >> @@ -2730,7 +2739,9 @@ static void parse_new_commit(void) >>       strbuf_reset(&new_data); >>       strbuf_addf(&new_data, "tree %s\n", >>               sha1_to_hex(b->branch_tree.versions[1].sha1)); >> -     if (!is_null_sha1(b->sha1)) >> +     if (!is_null_sha1(sha1)) >> +             strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(sha1)); >> +     else if (!is_null_sha1(b->sha1)) >>               strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(b->sha1)); > > Delaying "from"? > > [...] >> @@ -2855,7 +2866,7 @@ static void parse_reset_branch(void) >>       else >>               b = new_branch(sp); >>       read_next_command(); >> -     parse_from(b); >> +     parse_from(b, b->sha1); > > Likewise? > > [*] > So it looks like this would be easier to read as three patches: > >  1. protecting against "from" of unborn branch >  2. protecting against "merge" of unborn branch >  3. delaying the effect of "from" to avoid it confusingly changing >    the effect of a "merge" in the same commit Thanks, will look into it. Without (3) we'll still have inputs asking for (2) accepted, like: commit new_tip ... from old_tip merge new_tip new_tip won't end up with null_sha1 parent written, so should be ok to postpone this case to (3). > > (1) and (2) would be no-brainers, while (3) seems more subtle --- > maybe it should be documented to help importers for other version > control systems to know to make the same change? Will add a note on this. > > [...] >> --- a/t/t9300-fast-import.sh >> +++ b/t/t9300-fast-import.sh >> @@ -2895,6 +2895,54 @@ test_expect_success 'S: merge with garbage after mark must fail' ' >>       test_i18ngrep "after mark" err >>  ' >> >> +test_expect_success 'S: empty branch as merge parent must fail' ' >> +     test_must_fail git fast-import <<-EOF 2>err && >> +     commit refs/heads/chicken >> +     committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE >> +     data <> +     I am the chicken. >> +     COMMIT >> +     merge refs/heads/chicken >> +     EOF >> +     cat err && >> +     test_must_fail git rev-parse --verify refs/heads/chicken^ > > There would be no "chicken" branch after this import at all, right? Oh, yes, these rev-parse are all to illustrate the reason why import must fail. With this one we can add a positive test where chicken is a born branch and import succeeds. > > [...] >> +test_expect_success 'S: empty branch as merge parent must fail (2)' ' >> +     test_must_fail git fast-import <<-EOF 2>err && >> +     commit refs/heads/egg1 >> +     committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE >> +     data <> +     I am the egg N1. >> +     COMMIT >> + >> +     commit refs/heads/egg2 >> +     committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE >> +     data <> +     I am the egg N2. >> +     COMMIT >> +     from refs/heads/egg1 >> +     merge refs/heads/egg2 >> +     EOF >> +     cat err && >> +     test_must_fail git rev-parse --verify refs/heads/egg2^2 > > Likewise for egg2. Adding a positive one here too looks a good idea. > > [...] >> +test_expect_success 'S: empty branch as a parent must fail ' ' >> +     test_must_fail git fast-import <<-EOF 2>err && >> +     reset refs/heads/egg3 >> + >> +     commit refs/heads/egg4 >> +     committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE >> +     data <> +     I am the egg N4. >> +     COMMIT >> +     from refs/heads/egg3 >> +     EOF >> +     cat err && >> +     test_must_fail git rev-parse --verify refs/heads/egg4^2 > > Likewise for egg4. There is a copy-paste typo "egg4^2" instead of "egg4". Will just drop rev-parse here as there should be a positive merge test somewhere already. > > If this were split up as described above, I imagine it would be much > easier to read and most of it would move into the "obviously good" > category (I'm still uncertain about some details of the "delayed from" > implementation and haven't checked carefully whether it misses any > spots).  What do you think? Thanks, good idea indeed. Will rearrange and resend. > > Thanks for a pleasant read, and hope that helps, > Jonathan