Getting a commit sha1 from fast-import in a remote-helper

Hi,

For some reason, I need to know the sha1 corresponding to some marks
I'm putting in a fast-import stream. Unfortunately, this does not appear
to be possible.
- I'd rather not require a checkpoint to export marks each time I need
  such a sha1, and I'd rather not do that work that requires them after
  all the marks have been created (although currently, I'm forced to).
- fast-import's cat-blob only allows to cat blobs, which, well, is kind
  of in its name; how about adding an equivalent to the git-cat-file
  command?
- fast-import's `ls` command documentation about its output format
  mentions that the output may contain commits, so I tried the trick of
  creating a tree with commits, but fast-import then fails with:
    fatal: Not a blob (actually a commit)
  which I totally understand, but then I wonder why the documentation
  mentions it and how one would get a tree containing references to
  commits. I guess the documentation should be fixed.

So, while there's a possible solution with an equivalent to the
git-cat-file command if that'd be accepted, that's also overkill for my
need, which is to simply get the sha1 corresponding to a mark. Would you
consider a patch adding a command that allows such a resolution?

Cheers,

Mike

Mike Hommey wrote:

- fast-import's `ls` command documentation about its output format
  mentions that the output may contain commits, so I tried the trick of
  creating a tree with commits, but fast-import then fails with:
    fatal: Not a blob (actually a commit)
  which I totally understand, but then I wonder why the documentation
  mentions it and how one would get a tree containing references to
  commits. I guess the documentation should be fixed.

Odd.  Here's what happens when I try:

 $ echo "ls $(git rev-parse HEAD)" | git fast-import --quiet
 fatal: Missing space after tree-ish: ls a4a226a366ab0a173ed9e5f70f2a95d0d21e54c5
 fast-import: dumping crash report to .git/fast_import_crash_14080
 $ echo "ls $(git rev-parse HEAD) " | git fast-import --quiet
 040000 tree d3d38e7d71cb40ebbaf2798b01837b3de43fd4a1

How did you get that "Not a blob" message?

I think a good fix would be to teach parse_ls a mode with no <path>
parameter.  Something like this (untested; needs cleanup and tests):

Signed-off-by: Jonathan Nieder <redacted>

diff --git i/fast-import.c w/fast-import.c
index d0bd285..a9a46be 100644
--- i/fast-import.c
+++ w/fast-import.c

@@ -278,6 +278,8 @@ struct recent_command {
 	char *buf;
 };
 
+extern const char *tag_type;
+
 /* Configured limits on output */
 static unsigned long max_depth = 10;
 static off_t max_packsize;

@@ -3047,6 +3049,49 @@ static void parse_ls(const char *p, struct branch *b)
 	struct tree_entry *root = NULL;
 	struct tree_entry leaf = {NULL};
 
+	/* ls SP <tree-ish> */
+	if (*p != '"' && !strchr(p, ' ')) {
+		unsigned char sha1[20];
+		struct object_entry *e;
+		static struct strbuf line = STRBUF_INIT;
+		const char *type;
+
+		if (*p == ':') {	/* <mark> */
+			e = find_mark(parse_mark_ref_eol(p));
+			if (!e)
+				die("Unknown mark: %s", command_buf.buf);
+			hashcpy(sha1, e->idx.sha1);
+		} else {	/* <sha1> */
+			if (get_sha1_hex(p, sha1))
+				die("Invalid dataref: %s", command_buf.buf);
+			e = find_object(sha1);
+			p += 40;
+			if (*p)
+				die("Garbage after dataref: %s", command_buf.buf);
+		}
+
+		switch (e->type) {
+		case OBJ_COMMIT:
+			type = commit_type;
+			break;
+		case OBJ_TREE:
+			type = tree_type;
+			break;
+		case OBJ_BLOB:
+			type = blob_type;
+			break;
+		case OBJ_TAG:
+			type = tag_type;
+			break;
+		default:
+			die("Not a tree-ish: %s", command_buf.buf);
+		}
+
+		strbuf_reset(&line);
+		strbuf_addf(&line, "%s %s\n", type, sha1_to_hex(sha1));
+		cat_blob_write(line.buf, line.len);
+	}
+
 	/* ls SP (<tree-ish> SP)? <path> */
 	if (*p == '"') {
 		if (!b)

On Tue, Nov 18, 2014 at 09:34:26AM +0900, Mike Hommey wrote:

Hi,

For some reason, I need to know the sha1 corresponding to some marks
I'm putting in a fast-import stream. Unfortunately, this does not appear
to be possible.
- I'd rather not require a checkpoint to export marks each time I need
  such a sha1, and I'd rather not do that work that requires them after
  all the marks have been created (although currently, I'm forced to).

BTW, if it so happens that all the operations that were done end up
creating objects that already existed for some reason, checkpoint
doesn't do anything, which is fine for the pack and tags, but not
necessarily so for export-marks.

Mike

On Mon, Nov 17, 2014 at 05:40:28PM -0800, Jonathan Nieder wrote:

Mike Hommey wrote:

quoted

- fast-import's `ls` command documentation about its output format
  mentions that the output may contain commits, so I tried the trick of
  creating a tree with commits, but fast-import then fails with:
    fatal: Not a blob (actually a commit)
  which I totally understand, but then I wonder why the documentation
  mentions it and how one would get a tree containing references to
  commits. I guess the documentation should be fixed.

Odd.  Here's what happens when I try:

 $ echo "ls $(git rev-parse HEAD)" | git fast-import --quiet
 fatal: Missing space after tree-ish: ls a4a226a366ab0a173ed9e5f70f2a95d0d21e54c5
 fast-import: dumping crash report to .git/fast_import_crash_14080
 $ echo "ls $(git rev-parse HEAD) " | git fast-import --quiet
 040000 tree d3d38e7d71cb40ebbaf2798b01837b3de43fd4a1

How did you get that "Not a blob" message?

When trying to *create* a tree with a commit in it, so instead of giving
the mark for a blob to a filemodify command, giving a mark for a commit.
That is what fails with "Not a blob".
So it's not possible to create a tree with a reference to a commit, at
least with fast-import.
But, the documentation for the ls command says this:

   Output uses the same format as git ls-tree <tree> -- <path>:

        <mode> SP ('blob' | 'tree' | 'commit') SP <dataref> HT <path> LF

The 'commit' string certainly seems it cannot be there.

I think a good fix would be to teach parse_ls a mode with no <path>
parameter.  Something like this (untested; needs cleanup and tests):

This would make both your commands output the same thing, right? It
wouldn't help my case :)

Mike

On Tue, Nov 18, 2014 at 11:21:37AM +0900, Mike Hommey wrote:

On Tue, Nov 18, 2014 at 09:34:26AM +0900, Mike Hommey wrote:

quoted

Hi,

For some reason, I need to know the sha1 corresponding to some marks
I'm putting in a fast-import stream. Unfortunately, this does not appear
to be possible.
- I'd rather not require a checkpoint to export marks each time I need
  such a sha1, and I'd rather not do that work that requires them after
  all the marks have been created (although currently, I'm forced to).

BTW, if it so happens that all the operations that were done end up
creating objects that already existed for some reason, checkpoint
doesn't do anything, which is fine for the pack and tags, but not
necessarily so for export-marks.

And while I'm here, it's sad that one needs to emit a dummy cat-blob or
ls command to wait for a checkpoint to be finished because they are the
only commands that trigger something written on the cat-blob fd that can
be waited for.

Mike.

Mike Hommey wrote:

On Mon, Nov 17, 2014 at 05:40:28PM -0800, Jonathan Nieder wrote:

quoted

How did you get that "Not a blob" message?

When trying to *create* a tree with a commit in it, so instead of giving
the mark for a blob to a filemodify command, giving a mark for a commit.
That is what fails with "Not a blob".

Ah, I see what you were trying now.  It's complaining that the data
and mode don't match up.  See <mode> under 'filemodify' in the manual.

Something like

	M 160000 :1 mycommit

should work fine, though that's a pretty ugly workaround for the
inability to do

	ls :1

[...]

quoted

I think a good fix would be to teach parse_ls a mode with no <path>
parameter.  Something like this (untested; needs cleanup and tests):

This would make both your commands output the same thing, right? It
wouldn't help my case :)

It's easily possible my patch has a typo somewhere, but the expected
output format would be

	commit 6066a7eac4b2bcdb86971783b583e4e408b32e81

That wouldn't help?

Puzzled,
Jonathan

Mike Hommey wrote:

BTW, if it so happens that all the operations that were done end up
creating objects that already existed for some reason, checkpoint
doesn't do anything, which is fine for the pack and tags, but not
necessarily so for export-marks.

Does something like this help?

Do you have a short script that can demonstrate the failure?

Lazily,
Jonathan

Signed-off-by: Jonathan Nieder <redacted>

diff --git i/fast-import.c w/fast-import.c
index d0bd285..c3d53c8 100644
--- i/fast-import.c
+++ w/fast-import.c

@@ -3088,12 +3088,11 @@ static void parse_ls(const char *p, struct branch *b)
 static void checkpoint(void)
 {
 	checkpoint_requested = 0;
-	if (object_count) {
+	if (object_count)
 		cycle_packfile();
-		dump_branches();
-		dump_tags();
-		dump_marks();
-	}
+	dump_branches();
+	dump_tags();
+	dump_marks();
 }
 
 static void parse_checkpoint(void)

On Mon, Nov 17, 2014 at 06:51:31PM -0800, Jonathan Nieder wrote:

Mike Hommey wrote:

quoted

On Mon, Nov 17, 2014 at 05:40:28PM -0800, Jonathan Nieder wrote:

quoted

quoted

How did you get that "Not a blob" message?

When trying to *create* a tree with a commit in it, so instead of giving
the mark for a blob to a filemodify command, giving a mark for a commit.
That is what fails with "Not a blob".

Ah, I see what you were trying now.  It's complaining that the data
and mode don't match up.  See <mode> under 'filemodify' in the manual.

Something like

	M 160000 :1 mycommit

should work fine, though that's a pretty ugly workaround for the
inability to do

	ls :1

Actually, for my use, that ugly workaround actually improves things for
me, avoiding to use blobs in some of the stuff I want to store :) How
did I miss that? Thanks a lot for the enlightenment.

[...]

quoted

quoted

I think a good fix would be to teach parse_ls a mode with no <path>
parameter.  Something like this (untested; needs cleanup and tests):

This would make both your commands output the same thing, right? It
wouldn't help my case :)

It's easily possible my patch has a typo somewhere, but the expected
output format would be

	commit 6066a7eac4b2bcdb86971783b583e4e408b32e81

That wouldn't help?

Oh, so `ls <dataref>` would print out what <dataref> is? That would
definitely help, although with the trick above, I probably wouldn't
actually need it anymore.

Mike

On Mon, Nov 17, 2014 at 06:53:59PM -0800, Jonathan Nieder wrote:

Mike Hommey wrote:

quoted

BTW, if it so happens that all the operations that were done end up
creating objects that already existed for some reason, checkpoint
doesn't do anything, which is fine for the pack and tags, but not
necessarily so for export-marks.

Does something like this help?

I'm not sure about branches and tags, as they would mostly be noops,
but dump_marks definitely needs to happen. I did try to move dump_marks
out of the if already, and that did what I wanted.

Mike

Mike Hommey wrote:

And while I'm here, it's sad that one needs to emit a dummy cat-blob or
ls command to wait for a checkpoint to be finished

That's a good point.  (Though relying on checkpoints to read back
information is an ugly trick, so if we can get other commands to
provide the information you need then that would be better.)

The old-fashioned way is to do "progress checkpoint done".  Alas, that
writes to the progress fd, which doesn't go to the remote helper's
stdin.  How about something like this?

Signed-off-by: Jonathan Nieder <redacted>

diff --git i/Documentation/git-fast-import.txt w/Documentation/git-fast-import.txt
index 377eeaa..c0dedac 100644
--- i/Documentation/git-fast-import.txt
+++ w/Documentation/git-fast-import.txt

@@ -1055,6 +1055,12 @@ ls::
 	rather than wasting time on the early part of an import
 	before the unsupported command is detected.
 
+notify-on-checkpoint::
+	Cause the backend to write `checkpoint complete` to the file
+	descriptor set with `--cat-blob-fd`, or to `stdout` if
+	`--cat-blob-fd` was unspecified, when finishing a checkpoint
+	requested via a `checkpoint` command or SIGUSR1.
+
 notes::
 	Require that the backend support the 'notemodify' (N)
 	subcommand to the 'commit' command.

diff --git i/fast-import.c w/fast-import.c
index d0bd285..ccf4768 100644
--- i/fast-import.c
+++ w/fast-import.c

@@ -329,6 +329,7 @@ static const char *import_marks_file;
 static int import_marks_file_from_stream;
 static int import_marks_file_ignore_missing;
 static int relative_marks_paths;
+static int notify_on_checkpoint;
 
 /* Our last blob */
 static struct last_object last_blob = { STRBUF_INIT, 0, 0, 0 };

@@ -3094,6 +3095,9 @@ static void checkpoint(void)
 		dump_tags();
 		dump_marks();
 	}
+	if (notify_on_checkpoint)
+		cat_blob_write("checkpoint complete\n",
+			       strlen("checkpoint complete\n"));
 }
 
 static void parse_checkpoint(void)

@@ -3244,6 +3248,8 @@ static int parse_one_feature(const char *feature, int from_stream)
 		option_export_marks(arg);
 	} else if (!strcmp(feature, "cat-blob")) {
 		; /* Don't die - this feature is supported */
+	} else if (!strcmp(feature, "notify-on-checkpoint")) {
+		notify_on_checkpoint = 1;
 	} else if (!strcmp(feature, "relative-marks")) {
 		relative_marks_paths = 1;
 	} else if (!strcmp(feature, "no-relative-marks")) {

On Mon, Nov 17, 2014 at 07:27:41PM -0800, Jonathan Nieder wrote:

Mike Hommey wrote:

quoted

And while I'm here, it's sad that one needs to emit a dummy cat-blob or
ls command to wait for a checkpoint to be finished

That's a good point.  (Though relying on checkpoints to read back
information is an ugly trick, so if we can get other commands to
provide the information you need then that would be better.)

The old-fashioned way is to do "progress checkpoint done".  Alas, that
writes to the progress fd, which doesn't go to the remote helper's
stdin.  How about something like this?

How about something more generic, like "sync", which purpose would be to
request synchronisation with fast-import, which would respond "sync" on
the cat-blob fd?

Or "ping"<->"pong".

Although... I wonder if that would really be useful for anything else
than checkpoint...

That said, I, for one, would be fine if this is not "fixed" as long as
marks can be resolved some other way (and, in fact, it may turn out that
I don't need to resolve marks to sha1s at all)

Thanks for you help

Mike

On Tue, Nov 18, 2014 at 12:11:47PM +0900, Mike Hommey wrote:

Oh, so `ls <dataref>` would print out what <dataref> is? That would
definitely help, although with the trick above, I probably wouldn't
actually need it anymore.

So, in the end, I was able to do everything with what's currently
provided by git fast-import, but one thing would probably make life
easier for me: being able to initialize a commit tree from a commit
that's not one of the direct parents.

Because the data I'm using gives diffs against possibly unrelated
commits, and because starting a tree from scratch is actually slow when
you have thousands of subdirectories, it would be easier if I could just
start from that tree I have a diff against and apply the changes.
Without this, there would be a lot of `ls` command emitting involved,
and I'm actually not sure that wouldn't be as slow as starting from
scratch (haven't implemented yet, so I can't tell). Also, I'm not sure
how I'm supposed to know how much to read back from `ls`.

Mike

Mike Hommey wrote:

So, in the end, I was able to do everything with what's currently
provided by git fast-import, but one thing would probably make life
easier for me: being able to initialize a commit tree from a commit
that's not one of the direct parents.

IIRC then 'M 040000' wants a tree object, not a commit object, so
you'd have to do

	ls <commit> ""
	M 040000 <tree> ""

On Tue, Nov 18, 2014 at 06:21:22PM -0800, Jonathan Nieder wrote:

Mike Hommey wrote:

quoted

So, in the end, I was able to do everything with what's currently
provided by git fast-import, but one thing would probably make life
easier for me: being able to initialize a commit tree from a commit
that's not one of the direct parents.

IIRC then 'M 040000' wants a tree object, not a commit object, so
you'd have to do

	ls <commit> ""
	M 040000 <tree> ""

That's what I'm planning to try ; Would doing:
M 040000 <tree> ""
M 0644 <blob> some/path
D other/path

work? Or do I have to actually build a tree from the combination of the
output from various ls and those filedelete/filemodify?

Mike

On Wed, Nov 19, 2014 at 11:27:59AM +0900, Mike Hommey wrote:

On Tue, Nov 18, 2014 at 06:21:22PM -0800, Jonathan Nieder wrote:

quoted

IIRC then 'M 040000' wants a tree object, not a commit object, so
you'd have to do

	ls <commit> ""
	M 040000 <tree> ""

That's what I'm planning to try ; Would doing:
M 040000 <tree> ""
M 0644 <blob> some/path
D other/path

work?

Yes, that should work fine.  That's how contrib/svn-fe/svn-fe.c deals
with 'Node-copyfrom-rev' in Subversion dumpfiles, for what it's worth.