[PATCH] builtin-fast-export: Add importing and exporting of revision marks

[PATCH] builtin-fast-export: Add importing and exporting of revision marks

[Pieter de Bie]

This adds the --import-marks and --export-marks to fast-export. These import
and export the marks used to for all revisions exported in a similar fashion
to what fast-import does. The format is the same as fast-import, so you can
create a bidirectional importer / exporter by using the same marks file on
both sides.
---

I used this to create a bidirectional import/export script between Git and
Bazaar. As both sides can now both import and export marks, keeping two
repositories in sync is just a matter of keeping the marks files up to date.

This is my first c code that's more than one line, so please don't be too
harsh ;)

 builtin-fast-export.c |   71 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 71 insertions(+), 0 deletions(-)

diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index 8218199..1d5c83d 100755
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -375,30 +375,98 @@ static void handle_tags_and_duplicates(struct path_list *extra_refs)
 	}
 }
 
+static void export_marks(char * file)
+{
+	unsigned int i;
+	uintmax_t mark;
+	struct object_decoration *deco = idnums.hash;
+	FILE *f;
+
+	f = fopen(file, "w");
+	if (!f)
+		error("Unable to open marks file %s for writing", file);
+
+	for (i = 0; i < idnums.size; ++i) {
+		deco++;
+		if (deco && deco->base && deco->base->type == 1) {
+			mark = (uint32_t *) deco-> decoration - (uint32_t *)NULL;
+			fprintf(f, ":%" PRIuMAX " %s\n", mark, sha1_to_hex(deco->base->sha1));
+		}
+	}
+	if (ferror(f) || fclose(f)) {
+		error("Unable to write marks file %s.", file);
+	}
+}
+
+static void import_marks(char * input_file)
+{
+	char line[512];
+	FILE *f = fopen(input_file, "r");
+	if (!f)
+		die("cannot read %s: %s", input_file, strerror(errno));
+
+	while (fgets(line, sizeof(line), f)) {
+		uintmax_t mark;
+		char *end;
+		unsigned char sha1[20];
+		struct object *object;
+
+		end = strchr(line, '\n');
+		if (line[0] != ':' || !end)
+			die("corrupt mark line: %s", line);
+		*end = 0;
+		mark = strtoumax(line + 1, &end, 10);
+		if (!mark || end == line + 1
+			|| *end != ' ' || get_sha1(end + 1, sha1))
+			die("corrupt mark line: %s", line);
+
+		object = parse_object(sha1);
+		if (!object)
+			die ("Could not read blob %s", sha1_to_hex(sha1));
+
+		if (object->flags & SHOWN)
+			error("Object %s was already has a mark", sha1);
+
+		add_decoration(&idnums, object, ((uint32_t *)NULL) + mark);
+		if (last_idnum < mark)
+			last_idnum = mark;
+
+		object->flags |= SHOWN;
+	}
+	fclose(f);
+}
+
 int cmd_fast_export(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
 	struct object_array commits = { 0, 0, NULL };
 	struct path_list extra_refs = { NULL, 0, 0, 0 };
 	struct commit *commit;
+	char *export_filename, *import_filename;
 	struct option options[] = {
 		OPT_INTEGER(0, "progress", &progress,
 			    "show progress after <n> objects"),
 		OPT_CALLBACK(0, "signed-tags", &signed_tag_mode, "mode",
 			     "select handling of signed tags",
 			     parse_opt_signed_tag_mode),
+		OPT_STRING(0, "export-marks", &export_filename, "FILE", "Dump marks to this file"),
+		OPT_STRING(0, "import-marks", &import_filename, "FILE", "Import marks from this file"),
 		OPT_END()
 	};
 
 	/* we handle encodings */
 	git_config(git_default_config, NULL);
 
+
 	init_revisions(&revs, prefix);
 	argc = setup_revisions(argc, argv, &revs, NULL);
 	argc = parse_options(argc, argv, options, fast_export_usage, 0);
 	if (argc > 1)
 		usage_with_options (fast_export_usage, options);
 
+	if (import_filename)
+		import_marks(import_filename);
+
 	get_tags_and_duplicates(&revs.pending, &extra_refs);
 
 	if (prepare_revision_walk(&revs))
@@ -421,5 +489,8 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 
 	handle_tags_and_duplicates(&extra_refs);
 
+	if (export_filename)
+		export_marks(export_filename);
+
 	return 0;
 }
-- 
1.5.6.rc0.165.ge08d6b.dirty

Re: [PATCH] builtin-fast-export: Add importing and exporting of revision marks

[Johannes Schindelin]

Hi,

On Wed, 4 Jun 2008, Pieter de Bie wrote:

> +static void export_marks(char * file)

Extra space after star.

> +{
> +	unsigned int i;
> +	uintmax_t mark;
> +	struct object_decoration *deco = idnums.hash;
> +	FILE *f;
> +
> +	f = fopen(file, "w");
> +	if (!f)
> +		error("Unable to open marks file %s for writing", file);
> +
> +	for (i = 0; i < idnums.size; ++i) {
> +		deco++;
> +		if (deco && deco->base && deco->base->type == 1) {
> +			mark = (uint32_t *) deco-> decoration - (uint32_t *)NULL;

Why do you use uint32_t here, when you use uintmax_t to declare "mark"?

Also, there is an extra space after the closing paren.

Is "- (uint32_t *)NULL" needed?

> +			fprintf(f, ":%" PRIuMAX " %s\n", mark, sha1_to_hex(deco->base->sha1));

Too long line.

If you already only use uint32_t, I think you do not need the (ugly) 
PRIuMAX.

> +static void import_marks(char * input_file)
> +{
> +	char line[512];
> +	FILE *f = fopen(input_file, "r");
> +	if (!f)
> +		die("cannot read %s: %s", input_file, strerror(errno));
> +
> +	while (fgets(line, sizeof(line), f)) {
> +		uintmax_t mark;
> +		char *end;
> +		unsigned char sha1[20];
> +		struct object *object;
> +
> +		end = strchr(line, '\n');
> +		if (line[0] != ':' || !end)
> +			die("corrupt mark line: %s", line);
> +		*end = 0;
> +		mark = strtoumax(line + 1, &end, 10);
> +		if (!mark || end == line + 1
> +			|| *end != ' ' || get_sha1(end + 1, sha1))
> +			die("corrupt mark line: %s", line);

You do a bit too much with "end" for my liking.  Better use two variables, 
and spare the reader a (brief) "Huh?" moment.

> +		object = parse_object(sha1);
> +		if (!object)
> +			die ("Could not read blob %s", sha1_to_hex(sha1));
> +
> +		if (object->flags & SHOWN)
> +			error("Object %s was already has a mark", sha1);

s/was //

> +		add_decoration(&idnums, object, ((uint32_t *)NULL) + mark);

Better write (void *)mark.

> +	char *export_filename, *import_filename;
>  	struct option options[] = {
>  		OPT_INTEGER(0, "progress", &progress,
>  			    "show progress after <n> objects"),
>  		OPT_CALLBACK(0, "signed-tags", &signed_tag_mode, "mode",
>  			     "select handling of signed tags",
>  			     parse_opt_signed_tag_mode),
> +		OPT_STRING(0, "export-marks", &export_filename, "FILE", "Dump marks to this file"),
> +		OPT_STRING(0, "import-marks", &import_filename, "FILE", "Import marks from this file"),

Two long lines.

>  		OPT_END()
>  	};
>  
>  	/* we handle encodings */
>  	git_config(git_default_config, NULL);
>  
> +
>  	init_revisions(&revs, prefix);

Unnecessary change.

Other than that: ACK.

Thanks,
Dscho

Re: [PATCH] builtin-fast-export: Add importing and exporting of revision marks

[Pieter de Bie]

On 5 jun 2008, at 02:00, Johannes Schindelin wrote:

> Hi,
>
> On Wed, 4 Jun 2008, Pieter de Bie wrote:
>
>> +{
>> +	unsigned int i;
>> +	uintmax_t mark;
>> +	struct object_decoration *deco = idnums.hash;
>> +	FILE *f;
>> +
>> +	f = fopen(file, "w");
>> +	if (!f)
>> +		error("Unable to open marks file %s for writing", file);
>> +
>> +	for (i = 0; i < idnums.size; ++i) {
>> +		deco++;
>> +		if (deco && deco->base && deco->base->type == 1) {
>> +			mark = (uint32_t *) deco-> decoration - (uint32_t *)NULL;
>
> Why do you use uint32_t here, when you use uintmax_t to declare  
> "mark"?
>
> Also, there is an extra space after the closing paren.
>
> Is "- (uint32_t *)NULL" needed?

I changed the uintmax_t to to a uint32_t. If I remove the "- (uint32_t  
*)NULL",
it won't return the same marks. The same is done in get_object_mark().

>> +static void import_marks(char * input_file)
>> +{
>> +	char line[512];
>> +	FILE *f = fopen(input_file, "r");
>> +	if (!f)
>> +		die("cannot read %s: %s", input_file, strerror(errno));
>> +
>> +	while (fgets(line, sizeof(line), f)) {
>> +		uintmax_t mark;
>> +		char *end;
>> +		unsigned char sha1[20];
>> +		struct object *object;
>> +
>> +		end = strchr(line, '\n');
>> +		if (line[0] != ':' || !end)
>> +			die("corrupt mark line: %s", line);
>> +		*end = 0;
>> +		mark = strtoumax(line + 1, &end, 10);
>> +		if (!mark || end == line + 1
>> +			|| *end != ' ' || get_sha1(end + 1, sha1))
>> +			die("corrupt mark line: %s", line);
>
> You do a bit too much with "end" for my liking.  Better use two  
> variables,
> and spare the reader a (brief) "Huh?" moment.

Right. I copied this code from fast-export.c. I changed it to two  
variables now.

>> +		add_decoration(&idnums, object, ((uint32_t *)NULL) + mark);
>
> Better write (void *)mark.

That won't return the same result, as pointer addition goes with 4  
bytes. The
same thing is done in mark_object().

I will send an updated patch.

- Pieter

[PATCH v2] builtin-fast-export: Add importing and exporting of revision marks

[Pieter de Bie]

This adds the --import-marks and --export-marks to fast-export. These import
and export the marks used to for all revisions exported in a similar fashion
to what fast-import does. The format is the same as fast-import, so you can
create a bidirectional importer / exporter by using the same marks file on
both sides.

Signed-off-by: Pieter de Bie <pdebie@ai.rug.nl>
---
 builtin-fast-export.c |   74 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 74 insertions(+), 0 deletions(-)

diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index 1dfc01e..8aed868 100755
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -352,18 +352,86 @@ static void handle_tags_and_duplicates(struct path_list *extra_refs)
 	}
 }
 
+static void export_marks(char *file)
+{
+	unsigned int i;
+	uint32_t mark;
+	struct object_decoration *deco = idnums.hash;
+	FILE *f;
+
+	f = fopen(file, "w");
+	if (!f)
+		error("Unable to open marks file %s for writing", file);
+
+	for (i = 0; i < idnums.size; ++i) {
+		deco++;
+		if (deco && deco->base && deco->base->type == 1) {
+			mark = (uint32_t *)deco->decoration - (uint32_t *)NULL;
+			fprintf(f, ":%u %s\n", mark,
+				sha1_to_hex(deco->base->sha1));
+		}
+	}
+
+	if (ferror(f) || fclose(f))
+		error("Unable to write marks file %s.", file);
+}
+
+static void import_marks(char * input_file)
+{
+	char line[512];
+	FILE *f = fopen(input_file, "r");
+	if (!f)
+		die("cannot read %s: %s", input_file, strerror(errno));
+
+	while (fgets(line, sizeof(line), f)) {
+		uint32_t mark;
+		char *line_end, *mark_end;
+		unsigned char sha1[20];
+		struct object *object;
+
+		line_end = strchr(line, '\n');
+		if (line[0] != ':' || !line_end)
+			die("corrupt mark line: %s", line);
+		*line_end = 0;
+
+		mark = strtoumax(line + 1, &mark_end, 10);
+		if (!mark || mark_end == line + 1
+			|| *mark_end != ' ' || get_sha1(mark_end + 1, sha1))
+			die("corrupt mark line: %s", line);
+
+		object = parse_object(sha1);
+		if (!object)
+			die ("Could not read blob %s", sha1_to_hex(sha1));
+
+		if (object->flags & SHOWN)
+			error("Object %s already has a mark", sha1);
+
+		add_decoration(&idnums, object, ((uint32_t *)NULL) + mark);
+		if (last_idnum < mark)
+			last_idnum = mark;
+
+		object->flags |= SHOWN;
+	}
+	fclose(f);
+}
+
 int cmd_fast_export(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
 	struct object_array commits = { 0, 0, NULL };
 	struct path_list extra_refs = { NULL, 0, 0, 0 };
 	struct commit *commit;
+	char *export_filename, *import_filename;
 	struct option options[] = {
 		OPT_INTEGER(0, "progress", &progress,
 			    "show progress after <n> objects"),
 		OPT_CALLBACK(0, "signed-tags", &signed_tag_mode, "mode",
 			     "select handling of signed tags",
 			     parse_opt_signed_tag_mode),
+		OPT_STRING(0, "export-marks", &export_filename, "FILE",
+			   "Dump marks to this file"),
+		OPT_STRING(0, "import-marks", &import_filename, "FILE",
+			   "Import marks from this file"),
 		OPT_END()
 	};
 
@@ -376,6 +444,9 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	if (argc > 1)
 		usage_with_options (fast_export_usage, options);
 
+	if (import_filename)
+		import_marks(import_filename);
+
 	get_tags_and_duplicates(&revs.pending, &extra_refs);
 
 	if (prepare_revision_walk(&revs))
@@ -398,5 +469,8 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 
 	handle_tags_and_duplicates(&extra_refs);
 
+	if (export_filename)
+		export_marks(export_filename);
+
 	return 0;
 }
-- 
1.5.6.rc0.165.ge08d6b.dirty

Re: [PATCH] builtin-fast-export: Add importing and exporting of revision marks

[Johannes Schindelin]

Hi,

On Thu, 5 Jun 2008, Pieter de Bie wrote:

> On 5 jun 2008, at 02:00, Johannes Schindelin wrote:
> 
> >Is "- (uint32_t *)NULL" needed?
> 
> I changed the uintmax_t to to a uint32_t. If I remove the "- (uint32_t
> *)NULL", it won't return the same marks. The same is done in 
>  get_object_mark().

Ah, I missed that again.  I think I had exactly the same issue (of not 
understanding) with another patch for the same area of the code.

Maybe it would be worth having two functions to describe what is done 
there, for documentation purposes?

> I will send an updated patch.

Thanks,
Dscho

Re: [PATCH v2] builtin-fast-export: Add importing and exporting of revision marks

[Johannes Schindelin]

Hi,

On Thu, 5 Jun 2008, Pieter de Bie wrote:

> This adds the --import-marks and --export-marks to fast-export. These 
> import and export the marks used to for all revisions exported in a 
> similar fashion to what fast-import does. The format is the same as 
> fast-import, so you can create a bidirectional importer / exporter by 
> using the same marks file on both sides.

Nicely done.  As I said, I would like the pointer magic to be wrapped in a 
function so that this programmer does not get confused by it again, but 
it's not that important.  IOW only do it if you agree strongly.

Other than that, ACK.

Ciao,
Dscho

Re: [PATCH v2] builtin-fast-export: Add importing and exporting of revision marks

[Junio C Hamano]

Pieter de Bie <pdebie@ai.rug.nl> writes:

> +static void export_marks(char *file)
> +{
> +	unsigned int i;
> +	uint32_t mark;
> +	struct object_decoration *deco = idnums.hash;
> +	FILE *f;
> +
> +	f = fopen(file, "w");
> +	if (!f)
> +		error("Unable to open marks file %s for writing", file);
> +
> +	for (i = 0; i < idnums.size; ++i) {
> +		deco++;
> ...
> +			mark = (uint32_t *)deco->decoration - (uint32_t *)NULL;
> +			fprintf(f, ":%u %s\n", mark,
> +				sha1_to_hex(deco->base->sha1));
> ...
> +}
> +
> +static void import_marks(char * input_file)
> ...
> +		add_decoration(&idnums, object, ((uint32_t *)NULL) + mark);

I am confused.

The type of object_decoration.decorattion is a (void*).  Why isn't it
sufficient to do it in a naïve and straightforward way?

	mark = (uint32_t)(deco->decoration);
        add_decoration(&idnums, object, (void*) mark);

Is this twisted pointer arithmetic done in order to avoid cast between int
and pointer of different size in the code?  Even if that is the case,
doesn't "(uint32_t *)deco->decoration - (uint32_t *)NULL" mean the value
range for deco->decoration is one-fourth of U32?  What are you gaining
from using "uint32_t *" instead of some other pointer types, say "char *"?

Re: [PATCH v2] builtin-fast-export: Add importing and exporting of revision marks

[Pieter de Bie]

On 7 jun 2008, at 01:09, Junio C Hamano wrote:

> I am confused.
>
> The type of object_decoration.decorattion is a (void*).  Why isn't it
> sufficient to do it in a naïve and straightforward way?
>
> 	mark = (uint32_t)(deco->decoration);
>        add_decoration(&idnums, object, (void*) mark);
>
> Is this twisted pointer arithmetic done in order to avoid cast  
> between int
> and pointer of different size in the code?

I'm not sure why this is done; I simply copied what the existing code  
already
did.

>  Even if that is the case,
> doesn't "(uint32_t *)deco->decoration - (uint32_t *)NULL" mean the  
> value
> range for deco->decoration is one-fourth of U32?

I'd imagine so, yes

- Pieter

[PATCH] Documentation/fast-export: Document --import-marks and --export-marks options

[Pieter de Bie]

This adds a description for git-fast-export's --import-marks and
--export-marks options to its man page.
---

I forgot to add the options to the man page. Perhaps this should be squashed
on top of the other patch?

 Documentation/git-fast-export.txt |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index 332346c..277a547 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -36,6 +36,26 @@ when encountering a signed tag.  With 'strip', the tags will be made
 unsigned, with 'verbatim', they will be silently exported
 and with 'warn', they will be exported, but you will see a warning.
 
+--export-marks=<file>::
+	Dumps the internal marks table to <file> when complete.
+	Marks are written one per line as `:markid SHA-1`. Only marks
+	for revisions are dumped; marks for blobs are ignored.
+	Backends can use this file to validate imports after they
+	have been completed, or to save the marks table across
+	incremental runs.  As <file> is only opened and truncated
+	at completion, the same path can also be safely given to
+	\--import-marks.
+
+--import-marks=<file>::
+	Before processing any input, load the marks specified in
+	<file>.  The input file must exist, must be readable, and
+	must use the same format as produced by \--export-marks.
++
+Any commits that have already been marked will not be exported again.
+If the backend uses a similar \--import-marks file, this allows for
+incremental bidirectional exporting of the repository by keeping the
+marks the same across runs.
+
 
 EXAMPLES
 --------
-- 
1.5.6.rc0.165.ge08d6b.dirty

Re: [PATCH v2] builtin-fast-export: Add importing and exporting of revision marks

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1261 bytes --]

Hi,

On Sat, 7 Jun 2008, Pieter de Bie wrote:

> On 7 jun 2008, at 01:09, Junio C Hamano wrote:
> 
> >I am confused.
> >
> >The type of object_decoration.decorattion is a (void*).  Why isn't it
> >sufficient to do it in a naïve and straightforward way?
> >
> > mark = (uint32_t)(deco->decoration);
> >       add_decoration(&idnums, object, (void*) mark);
> >
> >Is this twisted pointer arithmetic done in order to avoid cast between 
> >int and pointer of different size in the code?

Yes, it was done in response to a remark that pointers might not be 
allowed to be unaligned.

> I'm not sure why this is done; I simply copied what the existing code 
> already did.

Okay, I looked again, and indeed, you _copied_ it.  Instead of using the 
functions mark_object() and get_object_mark() which are there only to be 
used by you.

So please fix.

> >Even if that is the case, doesn't "(uint32_t *)deco->decoration - 
> >(uint32_t *)NULL" mean the value range for deco->decoration is 
> >one-fourth of U32?

It is.  But since every object needs already at least 20 bytes, and we do 
not even have the complete address space to put objects into, and we do 
not plan to support 64-bit only repositories, I think we are fine.  At 
least for the moment.

Ciao,
Dscho

Re: [PATCH] Documentation/fast-export: Document --import-marks and --export-marks options

[Johannes Schindelin]

Hi,

On Sat, 7 Jun 2008, Pieter de Bie wrote:

> This adds a description for git-fast-export's --import-marks and
> --export-marks options to its man page.
> ---
> 
> I forgot to add the options to the man page. Perhaps this should be 
> squashed on top of the other patch?

Yes, that and the patch to use the existing functions to set/get the 
marks instead of duplicating code.

Ciao,
Dscho

Re: [PATCH v2] builtin-fast-export: Add importing and exporting of revision marks

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Okay, I looked again, and indeed, you _copied_ it.  Instead of using the 
> functions mark_object() and get_object_mark() which are there only to be 
> used by you.
>
> So please fix.
>
>> >Even if that is the case, doesn't "(uint32_t *)deco->decoration - 
>> >(uint32_t *)NULL" mean the value range for deco->decoration is 
>> >one-fourth of U32?
>
> It is.  But since every object needs already at least 20 bytes, and we do 
> not even have the complete address space to put objects into, and we do 
> not plan to support 64-bit only repositories, I think we are fine.

Oh, I was not complaining about the one-fourthness.  I was wondering why
"(uint32_t *)", which makes it look like the type itself has very deep
meaning for this computation, was used, instead of "(char *)" or something
that makes it much clearer that what could be pointed at by the pointer
does not matter and you are only using them as fake integers.  If there is
such a deep meaning, it needs documented, and if there isn't then probably
the use of (uint32_t *) should also be fixed.

Re: [PATCH] Documentation/fast-export: Document --import-marks and --export-marks options

[Junio C Hamano]

Pieter de Bie <pdebie@ai.rug.nl> writes:

> This adds a description for git-fast-export's --import-marks and
> --export-marks options to its man page.
> ---
>
> I forgot to add the options to the man page. Perhaps this should be squashed
> on top of the other patch?

Sign-off and tests are missing.

[PATCH v3] builtin-fast-export: Add importing and exporting of revision marks

[Pieter de Bie]

This adds the --import-marks and --export-marks to fast-export. These import
and export the marks used to for all revisions exported in a similar fashion
to what fast-import does. The format is the same as fast-import, so you can
create a bidirectional importer / exporter by using the same marks file on
both sides.

Signed-off-by: Pieter de Bie <pdebie@ai.rug.nl>
---

On 10 jun 2008, at 08:47, Junio C Hamano wrote:
>Sign-off and tests are missing.
  
  I actually had this new patch ready, but I was hoping Dscho would answer
  this first:
  
On 7 jun 2008, at 18:37, Junio C Hamano wrote:
>Oh, I was not complaining about the one-fourthness.  I was wondering why
>"(uint32_t *)", which makes it look like the type itself has very deep
>meaning for this computation, was used, instead of "(char *)" or something
>that makes it much clearer that what could be pointed at by the pointer
>does not matter and you are only using them as fake integers.  If there is
>such a deep meaning, it needs documented, and if there isn't then probably
>the use of (uint32_t *) should also be fixed.
  
  since I don't know the answer to that :)
  
On 7 jun 2008, at 17:19, Johannes Schindelin wrote:
>Okay, I looked again, and indeed, you _copied_ it.  Instead of using the 
>functions mark_object() and get_object_mark() which are there only to be 
>used by you.
>
>So please fix.
  
  How about this, explicit enough?
  
 Documentation/git-fast-export.txt |   20 +++++++
 builtin-fast-export.c             |   99 ++++++++++++++++++++++++++++++++++--
 t/t9301-fast-export.sh            |   24 +++++++++
 3 files changed, 137 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index 332346c..277a547 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -36,6 +36,26 @@ when encountering a signed tag.  With 'strip', the tags will be made
 unsigned, with 'verbatim', they will be silently exported
 and with 'warn', they will be exported, but you will see a warning.
 
+--export-marks=<file>::
+	Dumps the internal marks table to <file> when complete.
+	Marks are written one per line as `:markid SHA-1`. Only marks
+	for revisions are dumped; marks for blobs are ignored.
+	Backends can use this file to validate imports after they
+	have been completed, or to save the marks table across
+	incremental runs.  As <file> is only opened and truncated
+	at completion, the same path can also be safely given to
+	\--import-marks.
+
+--import-marks=<file>::
+	Before processing any input, load the marks specified in
+	<file>.  The input file must exist, must be readable, and
+	must use the same format as produced by \--export-marks.
++
+Any commits that have already been marked will not be exported again.
+If the backend uses a similar \--import-marks file, this allows for
+incremental bidirectional exporting of the repository by keeping the
+marks the same across runs.
+
 
 EXAMPLES
 --------
diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index 1dfc01e..94123c3 100755
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -56,10 +56,24 @@ static int has_unshown_parent(struct commit *commit)
 }
 
 /* Since intptr_t is C99, we do not use it here */
-static void mark_object(struct object *object)
+static inline uint32_t *mark_to_ptr(uint32_t mark)
 {
-	last_idnum++;
-	add_decoration(&idnums, object, ((uint32_t *)NULL) + last_idnum);
+	return ((uint32_t *)NULL) + mark;
+}
+
+static inline uint32_t ptr_to_mark(void * mark)
+{
+	return (uint32_t *)mark - (uint32_t *)NULL;
+}
+
+static inline void mark_object(struct object *object, uint32_t mark)
+{
+	add_decoration(&idnums, object, mark_to_ptr(mark));
+}
+
+static inline void mark_next_object(struct object *object)
+{
+	mark_object(object, ++last_idnum);
 }
 
 static int get_object_mark(struct object *object)
@@ -67,7 +81,7 @@ static int get_object_mark(struct object *object)
 	void *decoration = lookup_decoration(&idnums, object);
 	if (!decoration)
 		return 0;
-	return (uint32_t *)decoration - (uint32_t *)NULL;
+	return ptr_to_mark(decoration);
 }
 
 static void show_progress(void)
@@ -100,7 +114,7 @@ static void handle_object(const unsigned char *sha1)
 	if (!buf)
 		die ("Could not read blob %s", sha1_to_hex(sha1));
 
-	mark_object(object);
+	mark_next_object(object);
 
 	printf("blob\nmark :%d\ndata %lu\n", last_idnum, size);
 	if (size && fwrite(buf, size, 1, stdout) != 1)
@@ -185,7 +199,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)
 	for (i = 0; i < diff_queued_diff.nr; i++)
 		handle_object(diff_queued_diff.queue[i]->two->sha1);
 
-	mark_object(&commit->object);
+	mark_next_object(&commit->object);
 	if (!is_encoding_utf8(encoding))
 		reencoded = reencode_string(message, "UTF-8", encoding);
 	printf("commit %s\nmark :%d\n%.*s\n%.*s\ndata %u\n%s",
@@ -352,18 +366,85 @@ static void handle_tags_and_duplicates(struct path_list *extra_refs)
 	}
 }
 
+static void export_marks(char *file)
+{
+	unsigned int i;
+	uint32_t mark;
+	struct object_decoration *deco = idnums.hash;
+	FILE *f;
+
+	f = fopen(file, "w");
+	if (!f)
+		error("Unable to open marks file %s for writing", file);
+
+	for (i = 0; i < idnums.size; ++i) {
+		deco++;
+		if (deco && deco->base && deco->base->type == 1) {
+			mark = ptr_to_mark(deco->decoration);
+			fprintf(f, ":%u %s\n", mark, sha1_to_hex(deco->base->sha1));
+		}
+	}
+
+	if (ferror(f) || fclose(f))
+		error("Unable to write marks file %s.", file);
+}
+
+static void import_marks(char * input_file)
+{
+	char line[512];
+	FILE *f = fopen(input_file, "r");
+	if (!f)
+		die("cannot read %s: %s", input_file, strerror(errno));
+
+	while (fgets(line, sizeof(line), f)) {
+		uint32_t mark;
+		char *line_end, *mark_end;
+		unsigned char sha1[20];
+		struct object *object;
+
+		line_end = strchr(line, '\n');
+		if (line[0] != ':' || !line_end)
+			die("corrupt mark line: %s", line);
+		*line_end = 0;
+
+		mark = strtoumax(line + 1, &mark_end, 10);
+		if (!mark || mark_end == line + 1
+			|| *mark_end != ' ' || get_sha1(mark_end + 1, sha1))
+			die("corrupt mark line: %s", line);
+
+		object = parse_object(sha1);
+		if (!object)
+			die ("Could not read blob %s", sha1_to_hex(sha1));
+
+		if (object->flags & SHOWN)
+			error("Object %s already has a mark", sha1);
+
+		mark_object(object, mark);
+		if (last_idnum < mark)
+			last_idnum = mark;
+
+		object->flags |= SHOWN;
+	}
+	fclose(f);
+}
+
 int cmd_fast_export(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
 	struct object_array commits = { 0, 0, NULL };
 	struct path_list extra_refs = { NULL, 0, 0, 0 };
 	struct commit *commit;
+	char *export_filename, *import_filename;
 	struct option options[] = {
 		OPT_INTEGER(0, "progress", &progress,
 			    "show progress after <n> objects"),
 		OPT_CALLBACK(0, "signed-tags", &signed_tag_mode, "mode",
 			     "select handling of signed tags",
 			     parse_opt_signed_tag_mode),
+		OPT_STRING(0, "export-marks", &export_filename, "FILE",
+			     "Dump marks to this file"),
+		OPT_STRING(0, "import-marks", &import_filename, "FILE",
+			     "Import marks from this file"),
 		OPT_END()
 	};
 
@@ -376,6 +457,9 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 	if (argc > 1)
 		usage_with_options (fast_export_usage, options);
 
+	if (import_filename)
+		import_marks(import_filename);
+
 	get_tags_and_duplicates(&revs.pending, &extra_refs);
 
 	if (prepare_revision_walk(&revs))
@@ -398,5 +482,8 @@ int cmd_fast_export(int argc, const char **argv, const char *prefix)
 
 	handle_tags_and_duplicates(&extra_refs);
 
+	if (export_filename)
+		export_marks(export_filename);
+
 	return 0;
 }
diff --git a/t/t9301-fast-export.sh b/t/t9301-fast-export.sh
index f09bfb1..60b5ee3 100755
--- a/t/t9301-fast-export.sh
+++ b/t/t9301-fast-export.sh
@@ -78,6 +78,30 @@ test_expect_success 'iso-8859-1' '
 		 git cat-file commit i18n | grep "Áéí óú")
 
 '
+test_expect_success 'import/export-marks' '
+
+	git checkout -b marks master &&
+	git fast-export --export-marks=tmp-marks HEAD &&
+	test -s tmp-marks &&
+	cp tmp-marks ~ &&
+	test $(wc -l < tmp-marks) -eq 3 &&
+	test $(
+		git fast-export --import-marks=tmp-marks\
+		--export-marks=tmp-marks HEAD |
+		grep ^commit |
+		wc -l) \
+	-eq 0 &&
+	echo change > file &&
+	git commit -m "last commit" file &&
+	test $(
+		git fast-export --import-marks=tmp-marks \
+		--export-marks=tmp-marks HEAD |
+		grep ^commit\  |
+		wc -l) \
+	-eq 1 &&
+	test $(wc -l < tmp-marks) -eq 4
+
+'
 
 cat > signed-tag-import << EOF
 tag sign-your-name
-- 
1.5.6.rc1.153.gc1d96

Re: [PATCH v3] builtin-fast-export: Add importing and exporting of revision marks

[Pieter de Bie]

On 11 jun 2008, at 13:17, Pieter de Bie wrote:

> +	cp tmp-marks ~ &&

but beware this stray line.

Re: [PATCH v3] builtin-fast-export: Add importing and exporting of revision marks

[Johannes Schindelin]

Hi,

On Wed, 11 Jun 2008, Pieter de Bie wrote:

>   I actually had this new patch ready, but I was hoping Dscho would answer
>   this first:
>   
> On 7 jun 2008, at 18:37, Junio C Hamano wrote:
> >Oh, I was not complaining about the one-fourthness.  I was wondering why
> >"(uint32_t *)", which makes it look like the type itself has very deep
> >meaning for this computation, was used, instead of "(char *)" or something
> >that makes it much clearer that what could be pointed at by the pointer
> >does not matter and you are only using them as fake integers.  If there is
> >such a deep meaning, it needs documented, and if there isn't then probably
> >the use of (uint32_t *) should also be fixed.
>   
>   since I don't know the answer to that :)

I think that your patch does not need to address that, as the logic is (or 
should be) confined to the functions markt_object() and get_object_mark() 
(except that you have to split off mark_to_ptr() from 
mark_object(), as you did).

Unfortunately, I did not yet have time to look up the discussion on the 
mailing list that led me to implement this funny pointer arithmetic.

Ciao,
Dscho

Re: [PATCH v2] builtin-fast-export: Add importing and exporting of revision marks

[Johannes Schindelin]

Hi,

On Sat, 7 Jun 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Okay, I looked again, and indeed, you _copied_ it.  Instead of using the 
> > functions mark_object() and get_object_mark() which are there only to be 
> > used by you.
> >
> > So please fix.
> >
> >> >Even if that is the case, doesn't "(uint32_t *)deco->decoration - 
> >> >(uint32_t *)NULL" mean the value range for deco->decoration is 
> >> >one-fourth of U32?
> >
> > It is.  But since every object needs already at least 20 bytes, and we do 
> > not even have the complete address space to put objects into, and we do 
> > not plan to support 64-bit only repositories, I think we are fine.
> 
> Oh, I was not complaining about the one-fourthness.  I was wondering why 
> "(uint32_t *)", which makes it look like the type itself has very deep 
> meaning for this computation, was used, instead of "(char *)" or 
> something that makes it much clearer that what could be pointed at by 
> the pointer does not matter and you are only using them as fake 
> integers.

Probably you are right.  I had the impression that you could not rely on 
(void *) having the full precision, but that was completely bogus.

It could be changed to (char *) safely.

Ciao,
Dscho

Re: [PATCH v3] builtin-fast-export: Add importing and exporting of revision marks

[Junio C Hamano]

Pieter de Bie <pdebie@ai.rug.nl> writes:

> This adds the --import-marks and --export-marks to fast-export. These import
> and export the marks used to for all revisions exported in a similar fashion
> to what fast-import does. The format is the same as fast-import, so you can
> create a bidirectional importer / exporter by using the same marks file on
> both sides.

Heh, I've long queued a fixed-up version in 'pu' as 4ba0575
(builtin-fast-export: Add importing and exporting of revision marks,
2008-06-05).

I think renaming the "mark_object" to "mark_next_object" makes quite a lot
of sense, and I do not have any preference between mark2deco vs mark_to_ptr 
nor between a macro vs a static inline function for something small like
these.

You still use export_filename and import_filename uninitialized in
cmd_fast_export() and breaks everybody who does not use export-marks
option.  Has this patch (and the previous one I fixed up before queuing it
in 'pu') ever been tested?