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

Re: [PATCH 2/5] vcs-svn: Start working on the dumpfile producer

From
Ramkumar Ramachandra <artagnon@gmail.com>
Date
Jan 22, 2011, 09:45 UTC
Message-ID
<20110122094556.GB7827@kytes>
In-Reply-To
<7v39olok4l.fsf@alter.siamese.dyndns.org>
Hi Junio,
Junio C Hamano writes:
Show 9 quoted lines
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
> > Start off with some broad design sketches. Compile succeeds, but
> > parser is incorrect. Include a Makefile rule to build it into
> > vcs-svn/lib.a.
> >
> > Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> 
> I initially thought I should refrain from doing a usual picky review for
> contrib/ material, but realized this does not go to contrib/, so...

Thanks for the review. A new iteration including tests is almost ready- your review came in at the perfect time :)

Show 10 quoted lines
> > diff --git a/vcs-svn/dump_export.c b/vcs-svn/dump_export.c
> > new file mode 100644
> > index 0000000..04ede06
> > --- /dev/null
> > +++ b/vcs-svn/dump_export.c
> > ...
> > +void dump_export_begin_rev(int revision, const char *revprops,
> > +			int prop_len) {
> 
> Style. "{" at the beginning of the next line (same everywhere).
Fixed.
Show 28 quoted lines
> > +void dump_export_node(const char *path, enum node_kind kind,
> > +		enum node_action action, unsigned long text_len,
> > +		unsigned long copyfrom_rev, const char *copyfrom_path) {
> > +	printf("Node-path: %s\n", path);
> > +	printf("Node-kind: ");
> > +	switch (action) {
> > +	case NODE_KIND_NORMAL:
> > +		printf("file\n");
> > +		break;
> > +	case NODE_KIND_EXECUTABLE:
> > +		printf("file\n");
> > +		break;
> > +	case NODE_KIND_SYMLINK:
> > +		printf("file\n");
> > +		break;
> > +	case NODE_KIND_GITLINK:
> > +		printf("file\n");
> > +		break;
> > +	case NODE_KIND_SUBDIR:
> > +		die("Unsupported: subdirectory");
> > +	default:
> > +		break;
> > +	}
> 
> Hmph, is it expected that the repertoire of node-kind stay the same, or
> will it be extended over time?  It might make sense to change the above
> switch a more table-driven one.  The same for node-action that follows
> this part of the code.

I needed more than a lookup table, because I wanted the flexibility to execute arbitrary statements in each case. It's extended in the latest version (will come up on the list soon).

Show 15 quoted lines
> > diff --git a/vcs-svn/svnload.c b/vcs-svn/svnload.c
> > new file mode 100644
> > index 0000000..7043ae7
> > --- /dev/null
> > +++ b/vcs-svn/svnload.c
> > @@ -0,0 +1,294 @@
> > ...
> > +static struct strbuf blobs[100];
> > +	
> 
> Is there a rationale for "100", or is it just a random number that can be
> tweakable at the source level?  Would we want to have a symbolic constant
> for it?
> 
> The "blank" line has a trailing whitespace.

Fixed. I used this to illustrate the problem with persisting blobs- in this version, I persist 100 blobs; this is totally arbitrary and inextensible. Since the latest version leverages the --inline-blobs feature of fast export, this is totally unecessary.

Show 12 quoted lines
> > +static struct {
> > +	unsigned long prop_len, text_len, copyfrom_rev, mark;
> > +	int text_delta, prop_delta; /* Boolean */
> > ...
> > +static void reset_node_ctx(void)
> > ...
> > +	node_ctx.text_delta = -1;
> > +	node_ctx.prop_delta = -1;
> 
> Does your "Boolean" type take values 0 or -1?  Or is it perhaps a tristate
> false=0, true=1, unknown=-1?  If so, the comment on the type above needs
> to be fixed.
Fixed. Thanks for pointing this out.
Show 8 quoted lines
> > +	strbuf_reset(&node_ctx.copyfrom_path);
> > +	strbuf_reset(&node_ctx.path);
> > +}
> > +
> > +static void populate_props(struct strbuf *props, const char *author,
> > +			const char *log, const char *date) {
> 
> Style on "{" (look everywhere).
Fixed.
> > +	strbuf_reset(props);	
> 
> Trailing whitespace.
Fixed.
> > ...
> > +	t ++;
> 
> Unwanted SP before "++" (look everywhere).
Fixed.
> > +	tm_time = time_to_tm(strtoul(t, NULL, 10), atoi(tz_off));
> 
> Has your caller already verified tz_off is a reasonable integer?
Fixed.
Show 7 quoted lines
> > ...
> > +			if (!memcmp(t, "D", 1)) {
> > +				node_ctx.action = NODE_ACTION_DELETE;
> > +			}
> > +			else if (!memcmp(t, "C", 1)) {
> 
> Style: "} else if (...) {".
Fixed.
Show 8 quoted lines
> > ...
> > +					node_ctx.kind = NODE_KIND_GITLINK;
> > +				else if (!memcmp(val, "040000", 6))
> 
> Is <mode> guaranteed to be a 6-digit octal, padded with 0 to the left?
> 
> The manual page of git-fast-import seems to hint that is the case, but I
> am double checking, as the leading zero is an error in the tree object.

The documentation and commit messages seem to hint that this is the case. I'm digging to see if there's anything wrong with this.

Show 7 quoted lines
> > +					node_ctx.kind = NODE_KIND_SUBDIR;
> > +				else {
> > +					if (!memcmp(val, "755", 3))
> > +						node_ctx.kind = NODE_KIND_EXECUTABLE;
> > +					else if(!memcmp(val, "644", 3))
> 
> Style; missing SP after "if/switch" (look everywhere).
Fixed.
Show 12 quoted lines
> > +						node_ctx.kind = NODE_KIND_NORMAL;
> > +					else
> > +						die("Unrecognized mode: %s", val);
> > +					mode_incr = 4;
> > +				}
> > +				val += mode_incr;
> 
> Hmm, this whole thing is ugly.
> 
> Perhaps a table-lookup, or at least a helper function that translates
> "val" to node-kind (while advancing val as the side effect) may make it
> easier to read?
Fixed. I used a helper function.
Show 5 quoted lines
> > ...
> > +					to_dump = &blobs[strtoul(val + 1, NULL, 10)];
> 
> Has anybody so far verified the decimal number at (val+1) is a reasonable
> value, or am I seeing a future CVE here?
Fixed.
> Do we care what follows the len of digits on this line?  Does a line in G-F-I
> stream allow trailing garbage (this question is not limited to this part
> of the code)?

Hm, I haven't thought about this hard enough. Currently, both svn-fi and svn-fe ignore any trailing garbage/ commands they don't understand. Are there some issues that we haven't anticipated?

Show 7 quoted lines
> > +int svnload_init(const char *filename)
> > ...
> > +void svnload_deinit(void)
>
> Whoever needs to add an element to rev_ctx must consistently touch reset,
> init and deinit.  Would it help him/her if you moved the code so that
> these functions are close together from the beginning?
Fixed.
-- Ram
Previous: Junio C HamanoNext: Ramkumar Ramachandra
Message 5 of 18 in “Towards a Git-to-SVN bridge”
  1. 0/5 Towards a Git-to-SVN bridgeRamkumar Ramachandra, Jan 19, 2011
  2. 1/5 date: Expose the time_to_tm functionRamkumar Ramachandra, Jan 19, 2011
  3. 2/5 vcs-svn: Start working on the dumpfile producerRamkumar Ramachandra, Jan 19, 2011
  4. Junio C HamanoJan 22, 2011
  5. Ramkumar RamachandraJan 22, 2011
  6. 3/5 Build an svn-fi target in contrib/svn-feRamkumar Ramachandra, Jan 19, 2011
  7. 4/5 fast-export: Introduce --inline-blobsRamkumar Ramachandra, Jan 19, 2011
  8. Junio C HamanoJan 19, 2011
  9. Jonathan NiederJan 19, 2011
  10. Ramkumar RamachandraJan 20, 2011
  11. Jonathan NiederJan 20, 2011
  12. Ramkumar RamachandraJan 20, 2011
  13. Drew NorthupJan 20, 2011
  14. Ramkumar RamachandraJan 22, 2011
  15. Jonathan NiederJan 22, 2011
  16. Jonathan NiederJan 20, 2011
  17. Junio C HamanoJan 22, 2011
  18. 5/5 vcs-svn: Add dir_cache for svnloadRamkumar Ramachandra, Jan 19, 2011

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.