From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Sun, 30 May 2010 15:58:56 GMT
Subject: Re: [PATCH 6/7] Add SVN revision parser and exporter
Message-ID: <AANLkTimPkAWa8YEro50gWgeaPSmgPCRXppHZNZuCb_hj@mail.gmail.com>
In-Reply-To: <20100529140645.GA8338@progeny.tock>

Hi Jonathan,

Jonathan Nieder wrote:
> Wait, where are SVN revisions being parsed?  It seems that the repo
> module maintains the exporter's state and provides a facility to
> to call the fast_export module to write it out.

Right. Sorry about the bad commit message.

>  repo_add(path, mode, blob_mark) is used to add a new file to
>  the current commit.
>
>  repo_modify is used to add a replacement for an existing file;
>  it is implemented exactly the same way, but a check could be
>  added later to distinguish the two cases.
>
>  repo_copy copies a blob from a previous revision to the current
>  commit.
>
>  repo_replace modifies the content of a file from the current
>  commit, if and only if it already exists.
>
>  repo_delete removes a file or directory from the current commit.
>
>  repo_commit calls out to fast_export to write the current commit
>  to the fast-import stream in stdout.
>
>  repo_diff is used by the fast_export module to write the changes
>  for a commit.
>
>  repo_reset erases the exporter's state, so valgrind can be happy.

Thanks for the nice descriptions. Will be useful while preparing
Documentation/technical.

> Mode must be 100644, 100755, 120000, or 160000.

Okay, do we put in an assert() for sanity?

> Style: we don’t tend to use typedef to hide underlying struct definitions
> (see Documentation/CodingStyle from linux-2.6.git, chapter 5, for some
> explanation about why).

Fixed.

> Maybe some local variables would make this more readable:

Fixed.

> Is this for adding new entries to an existing directory?  It is
> getting late, so I did not look it over carefully.

For David.

> If a file "foo" exists and I ask for "foo/bar", this will return
> the entry for foo.  Is that appropriate?

For David.

> When would it be NULL?  Is an empty path allowed?

For David.

> Can this overflow the path stack?

Actually, this has already been changed- see the latest commits to see
how path is allocated.

> Might be easier to read the other way around:

Fixed.

> I did not carefully trace the cases where repo_clone_dir may reuse a
> dir.  I would be happier if someone else does.

For David.

-- Ram