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

Re: [PATCH v7 13/16] remote-svn: add incremental import

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 28, 2012, 17:54 UTC
Message-ID
<7v1uiq3nea.fsf@alter.siamese.dyndns.org>
In-Reply-To
<1346143790-23491-14-git-send-email-florian.achleitner.2.6.31@gmail.com>
Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:
Show 59 quoted lines
> Search for a note attached to the ref to update and read it's
> 'Revision-number:'-line. Start import from the next svn revision.
>
> If there is no next revision in the svn repo, svnrdump terminates with
> a message on stderr an non-zero return value. This looks a little
> weird, but there is no other way to know whether there is a new
> revision in the svn repo.
>
> On the start of an incremental import, the parent of the first commit
> in the fast-import stream is set to the branch name to update. All
> following commits specify their parent by a mark number. Previous mark
> files are currently not reused.
>
> Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  contrib/svn-fe/svn-fe.c |    3 ++-
>  remote-testsvn.c        |   67 ++++++++++++++++++++++++++++++++++++++++++++---
>  test-svn-fe.c           |    2 +-
>  vcs-svn/fast_export.c   |   10 +++++--
>  vcs-svn/fast_export.h   |    6 ++---
>  vcs-svn/svndump.c       |   10 +++----
>  vcs-svn/svndump.h       |    2 +-
>  7 files changed, 84 insertions(+), 16 deletions(-)
>
> diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
> index c796cc0..f363505 100644
> --- a/contrib/svn-fe/svn-fe.c
> +++ b/contrib/svn-fe/svn-fe.c
> @@ -10,7 +10,8 @@ int main(int argc, char **argv)
>  {
>  	if (svndump_init(NULL))
>  		return 1;
> -	svndump_read((argc > 1) ? argv[1] : NULL, "refs/heads/master");
> +	svndump_read((argc > 1) ? argv[1] : NULL, "refs/heads/master",
> +			"refs/notes/svn/revs");
>  	svndump_deinit();
>  	svndump_reset();
>  	return 0;
> diff --git a/remote-testsvn.c b/remote-testsvn.c
> index b6e7968..e90d221 100644
> --- a/remote-testsvn.c
> +++ b/remote-testsvn.c
> @@ -12,7 +12,8 @@ static const char *url;
>  static int dump_from_file;
>  static const char *private_ref;
>  static const char *remote_ref = "refs/heads/master";
> -static const char *marksfilename;
> +static const char *marksfilename, *notes_ref;
> +struct rev_note { unsigned int rev_nr; };
>  
>  static int cmd_capabilities(const char *line);
>  static int cmd_import(const char *line);
> @@ -47,14 +48,70 @@ static void terminate_batch(void)
>  	fflush(stdout);
>  }
>  
> +/* NOTE: 'ref' refers to a git reference, while 'rev' refers to a svn revision. */
> +static char *read_ref_note(const unsigned char sha1[20]) {
Style:
	static char *read_ref_note(const unsigned char sha1[20])
        {
Show 14 quoted lines
> +	const unsigned char *note_sha1;
> +	char *msg = NULL;
> +	unsigned long msglen;
> +	enum object_type type;
> +	init_notes(NULL, notes_ref, NULL, 0);
> +	if(	(note_sha1 = get_note(NULL, sha1)) == NULL ||
> +			!(msg = read_sha1_file(note_sha1, &type, &msglen)) ||
> +			!msglen || type != OBJ_BLOB) {
> +		free(msg);
> +		return NULL;
> +	}
> +	free_notes(NULL);
> +	return msg;
> +}
Style:
	if (!(note_sha1 = get_note(NULL, sha1)) ||
	    !(msg = read_sha1_file(note_sha1, &type, &msglen)) ||
	    !msglen ||
            type != OBJ_BLOB) {
		...
But a bigger question is if any of these cases is a non-error.

It may be perfectly normal so get_note() that returns NULL may be a normal condition, but is it something you want to silently ignore if read_sha1_file() did not give you anything when called with a note_sha1 that ought to be valid? How about the case where you got msg but msglen is zero? Is it an error? Is it normal and the caller wants to see a note that happens to be an empty string? How about the case where the note returned was ot a blob? Is it something you want to silently ignore, or is it an error?

Having multiple assingments inside a conditional, and chaining them together with "||", lets you code lazily, and the resulting code like the above _appear_ concise, but in order to prepare the code to answer these questions sensibly, it is often a good habit to avoid the appearance of conciseness that hides the lack of thought (e.g. the code is hiding the reason why it does not call free_notes() when you do have note_sah1 but found a note that is of undesired object type, and the reader cannot tell if it is done deliberately).

It is far more preferrable to see this written like:
	if (!(note_sha1 = get_note(NULL, sha1)))
        	return NULL; /* no notes - nothing to return */
	msg = read_sha1_file(note_sha1, &type, &msglen);
	if (!msg) {
		error("cannot read notes for ...");
        } else if (type != OBJ_BLOB) {
		free(msg); /* something we cannot use */
		msg = NULL;
	} ... you may have more else if clauses here ...
        free_notes(NULL);
        return msg;

Also, I am not sure if you want to silently ignore OBJ_BLOB here. If I understand correctly, you are not reading from a random notes tree, but from a notes tree that was populated by an earlier incarnation of your process, no? If asking for a note in that notes tree yields a note that you do not recognize, shouldn't you treat it as an error and raise a big red flag? The same discussion goes for ignoring an empty msg. If you never produce an empty msg, and if nobody else is supposed to add random stuff to that notes tree, shouldn't you treat it as an indication that something fishy is going on if you read an empty msg?

> +static int parse_rev_note(const char *msg, struct rev_note *res) {
Style.
	static int parse_rev_note(const char *msg, struct rev_note *res)
	{
> +	const char *key, *value, *end;
> +	size_t len;
> +	while(*msg) {
Style.
        while (*msg) {
Show 5 quoted lines
> +		end = strchr(msg, '\n');
> +		len = end ? end - msg : strlen(msg);
> +
> +		key = "Revision-number: ";
> +		if(!prefixcmp(msg, key)) {
Style.
		if (!prefixcmp(msg, key)) {
> +			long i;
> +			value = msg + strlen(key);
> +			i = atol(value);
> +			if(i < 0 || i > UINT32_MAX)
Style.
		if (i < 0 || ...)

More importantly, if you are parsing text that is supposed to be a format known to you and not human generated, you should avoid using atoi & atol when parsing numbers; use strtol or strtoul instead, as they allow you much better error handling.

> +				return 1;

Is it signaling an error to the caller? The usual convention is to use negative value for such a purpose.

Show 19 quoted lines
> +			res->rev_nr = i;
> +		}
> +		msg += len + 1;
> +	}
> +	return 0;
> +}
> +
>  static int cmd_import(const char *line)
>  {
>  	int code;
>  	int dumpin_fd;
> -	unsigned int startrev = 0;
> +	char *note_msg;
> +	unsigned char head_sha1[20];
> +	unsigned int startrev;
>  	struct argv_array svndump_argv = ARGV_ARRAY_INIT;
>  	struct child_process svndump_proc;
>  
> +	if(read_ref(private_ref, head_sha1))
Style.
Show 8 quoted lines
> +		startrev = 0;
> +	else {
> +		note_msg = read_ref_note(head_sha1);
> +		if(note_msg == NULL) {
> +			warning("No note found for %s.", private_ref);
> +			startrev = 0;
> +		}
> +		else {
Style.
Show 103 quoted lines
> +			struct rev_note note = { 0 };
> +			parse_rev_note(note_msg, &note);
> +			startrev = note.rev_nr + 1;
> +			free(note_msg);
> +		}
> +	}
> +
>  	if (dump_from_file) {
>  		dumpin_fd = open(url, O_RDONLY);
>  		if(dumpin_fd < 0) {
> @@ -80,7 +137,7 @@ static int cmd_import(const char *line)
>  			"feature export-marks=%s\n", marksfilename, marksfilename);
>  
>  	svndump_init_fd(dumpin_fd, STDIN_FILENO);
> -	svndump_read(url, private_ref);
> +	svndump_read(url, private_ref, notes_ref);
>  	svndump_deinit();
>  	svndump_reset();
>  
> @@ -177,6 +234,9 @@ int main(int argc, const char **argv)
>  	strbuf_addf(&buf, "refs/svn/%s/master", remote->name);
>  	private_ref = strbuf_detach(&buf, NULL);
>  
> +	strbuf_addf(&buf, "refs/notes/%s/revs", remote->name);
> +	notes_ref = strbuf_detach(&buf, NULL);
> +
>  	strbuf_addf(&buf, "%s/info/fast-import/remote-svn/%s.marks",
>  		get_git_dir(), remote->name);
>  	marksfilename = strbuf_detach(&buf, NULL);
> @@ -196,6 +256,7 @@ int main(int argc, const char **argv)
>  	strbuf_release(&buf);
>  	free((void*)url);
>  	free((void*)private_ref);
> +	free((void*)notes_ref);
>  	free((void*)marksfilename);
>  	return 0;
>  }
> diff --git a/test-svn-fe.c b/test-svn-fe.c
> index cb0d80f..0f2d9a4 100644
> --- a/test-svn-fe.c
> +++ b/test-svn-fe.c
> @@ -40,7 +40,7 @@ int main(int argc, char *argv[])
>  	if (argc == 2) {
>  		if (svndump_init(argv[1]))
>  			return 1;
> -		svndump_read(NULL, "refs/heads/master");
> +		svndump_read(NULL, "refs/heads/master", "refs/notes/svn/revs");
>  		svndump_deinit();
>  		svndump_reset();
>  		return 0;
> diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
> index df51c59..f2b23c8 100644
> --- a/vcs-svn/fast_export.c
> +++ b/vcs-svn/fast_export.c
> @@ -68,13 +68,19 @@ void fast_export_modify(const char *path, uint32_t mode, const char *dataref)
>  }
>  
>  void fast_export_begin_note(uint32_t revision, const char *author,
> -		const char *log, unsigned long timestamp)
> +		const char *log, unsigned long timestamp, const char *note_ref)
>  {
> +	static int firstnote = 1;
>  	size_t loglen = strlen(log);
> -	printf("commit refs/notes/svn/revs\n");
> +	printf("commit %s\n", note_ref);
>  	printf("committer %s <%s@%s> %ld +0000\n", author, author, "local", timestamp);
>  	printf("data %"PRIuMAX"\n", (uintmax_t)loglen);
>  	fwrite(log, loglen, 1, stdout);
> +	if (firstnote) {
> +		if (revision > 1)
> +			printf("from %s^0", note_ref);
> +		firstnote = 0;
> +	}
>  	fputc('\n', stdout);
>  }
>  
> diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
> index c2f6f11..c8b5adb 100644
> --- a/vcs-svn/fast_export.h
> +++ b/vcs-svn/fast_export.h
> @@ -11,10 +11,10 @@ void fast_export_delete(const char *path);
>  void fast_export_modify(const char *path, uint32_t mode, const char *dataref);
>  void fast_export_note(const char *committish, const char *dataref);
>  void fast_export_begin_note(uint32_t revision, const char *author,
> -		const char *log, unsigned long timestamp);
> +		const char *log, unsigned long timestamp, const char *note_ref);
>  void fast_export_begin_commit(uint32_t revision, const char *author,
> -			const struct strbuf *log, const char *uuid,
> -			const char *url, unsigned long timestamp, const char *local_ref);
> +			const struct strbuf *log, const char *uuid,const char *url,
> +			unsigned long timestamp, const char *local_ref);
>  void fast_export_end_commit(uint32_t revision);
>  void fast_export_data(uint32_t mode, off_t len, struct line_buffer *input);
>  void fast_export_buf_to_data(const struct strbuf *data);
> diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
> index cd65b51..31d1d83 100644
> --- a/vcs-svn/svndump.c
> +++ b/vcs-svn/svndump.c
> @@ -309,20 +309,20 @@ static void begin_revision(const char *remote_ref)
>  		rev_ctx.timestamp, remote_ref);
>  }
>  
> -static void end_revision()

7e11902 (vcs-svn: add a comment before each commit, 2011-01-04) added this as

	static void end_revision(void)

but it degenerated to pre-ANSI definition at "[PATCH 8/16] Enable fetching to private refs", which needs to be fixed by losing that hunk.

Show 49 quoted lines
> +static void end_revision(const char *note_ref)
>  {
>  	struct strbuf mark = STRBUF_INIT;
>  	if (rev_ctx.revision) {
>  		fast_export_end_commit(rev_ctx.revision);
>  		fast_export_begin_note(rev_ctx.revision, "remote-svn",
> -				"Note created by remote-svn.", rev_ctx.timestamp);
> +				"Note created by remote-svn.", rev_ctx.timestamp, note_ref);
>  		strbuf_addf(&mark, ":%"PRIu32, rev_ctx.revision);
>  		fast_export_note(mark.buf, "inline");
>  		fast_export_buf_to_data(&rev_ctx.note);
>  	}
>  }
>  
> -void svndump_read(const char *url, const char *local_ref)
> +void svndump_read(const char *url, const char *local_ref, const char *notes_ref)
>  {
>  	char *val;
>  	char *t;
> @@ -363,7 +363,7 @@ void svndump_read(const char *url, const char *local_ref)
>  			if (active_ctx == REV_CTX)
>  				begin_revision(local_ref);
>  			if (active_ctx != DUMP_CTX)
> -				end_revision();
> +				end_revision(notes_ref);
>  			active_ctx = REV_CTX;
>  			reset_rev_ctx(atoi(val));
>  			strbuf_addf(&rev_ctx.note, "%s\n", t);
> @@ -479,7 +479,7 @@ void svndump_read(const char *url, const char *local_ref)
>  	if (active_ctx == REV_CTX)
>  		begin_revision(local_ref);
>  	if (active_ctx != DUMP_CTX)
> -		end_revision();
> +		end_revision(notes_ref);
>  }
>  
>  static void init(int report_fd)
> diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h
> index febeecb..b8eb129 100644
> --- a/vcs-svn/svndump.h
> +++ b/vcs-svn/svndump.h
> @@ -3,7 +3,7 @@
>  
>  int svndump_init(const char *filename);
>  int svndump_init_fd(int in_fd, int back_fd);
> -void svndump_read(const char *url, const char *local_ref);
> +void svndump_read(const char *url, const char *local_ref, const char *notes_ref);
>  void svndump_deinit(void);
>  void svndump_reset(void);
Previous: Junio C HamanoNext: Junio C Hamano
Message 20 of 26 in “GSOC remote-svn”
  1. 00/16 GSOC remote-svnFlorian Achleitner, Aug 28, 2012
  2. 01/16 Implement a remote helper for svn in CFlorian Achleitner, Aug 28, 2012
  3. 02/16 Add git-remote-testsvn to MakefileFlorian Achleitner, Aug 28, 2012
  4. 03/16 Add svndump_init_fd to allow reading dumps from arbitrary FDsFlorian Achleitner, Aug 28, 2012
  5. 04/16 Add argv_array_detach and argv_array_free_detachedFlorian Achleitner, Aug 28, 2012
  6. 05/16 Connect fast-import to the remote-helper via pipe, adding 'bidi-import' capabilityFlorian Achleitner, Aug 28, 2012
  7. 06/16 Add documentation for the 'bidi-import' capability of remote-helpersFlorian Achleitner, Aug 28, 2012
  8. 07/16 When debug==1, start fast-import with "--stats" instead of "--quiet"Florian Achleitner, Aug 28, 2012
  9. 08/16 remote-svn, vcs-svn: Enable fetching to private refsFlorian Achleitner, Aug 28, 2012
  10. 09/16 Allow reading svn dumps from files via file:// urlsFlorian Achleitner, Aug 28, 2012
  11. 10/16 vcs-svn: add fast_export_note to create notesFlorian Achleitner, Aug 28, 2012
  12. 11/16 Create a note for every imported commit containing svn metadataFlorian Achleitner, Aug 28, 2012
  13. 12/16 remote-svn: Activate import/export-marks for fast-importFlorian Achleitner, Aug 28, 2012
  14. 13/16 remote-svn: add incremental importFlorian Achleitner, Aug 28, 2012
  15. 14/16 Add a svnrdump-simulator replaying a dump file for testingFlorian Achleitner, Aug 28, 2012
  16. 15/16 remote-svn: add marks-file regenerationFlorian Achleitner, Aug 28, 2012
  17. 16/16 Add a test script for remote-svnFlorian Achleitner, Aug 28, 2012
  18. Junio C HamanoAug 28, 2012
  19. Junio C HamanoAug 28, 2012
  20. Junio C HamanoAug 28, 2012
  21. Junio C HamanoAug 28, 2012
  22. Junio C HamanoAug 28, 2012
  23. Junio C HamanoAug 28, 2012
  24. Junio C HamanoAug 28, 2012
  25. Junio C HamanoAug 28, 2012
  26. Florian AchleitnerAug 28, 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.