git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] fast-import: disallow empty branches as parents

From
Dmitry Ivankov <divanorama@gmail.com>
Date
Jun 21, 2012, 06:39 UTC
Message-ID
<CA+gfSn9+Wg9oA7q4=nWdNXuG98n-p8VHYaN_9nVeGUk8iHpeSA@mail.gmail.com>
In-Reply-To
<20120621035753.GA3842@burratino>
On Thu, Jun 21, 2012 at 9:57 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 25 quoted lines
> 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.

Show 24 quoted lines
>
> [...]
>> @@ -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.

Show 133 quoted lines
>
> [...]
>> -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.
Show 21 quoted lines
>
> [...]
>> --- 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 <<COMMIT
>> +     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.

Show 22 quoted lines
>
> [...]
>> +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 <<COMMIT
>> +     I am the egg N1.
>> +     COMMIT
>> +
>> +     commit refs/heads/egg2
>> +     committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
>> +     data <<COMMIT
>> +     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.
Show 17 quoted lines
>
> [...]
>> +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 <<COMMIT
>> +     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.

Show 6 quoted lines
>
> 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
Previous: Jonathan NiederNext: Dmitry Ivankov
Message 4 of 5 in “fast-import: disallow empty branches as parents”
  1. fast-import: disallow empty branches as parentsDmitry Ivankov, Jun 20, 2012
  2. fast-import: disallow empty branches as parentsDmitry Ivankov, Jun 20, 2012
  3. Jonathan NiederJun 21, 2012
  4. Dmitry IvankovJun 21, 2012
  5. Dmitry IvankovJun 25, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.