threads / rfc / 50546

RFC patch, 5 partsfast-import: fix erroneous handling of get-mark with empty orphan commits

Subject: [RFC PATCH 5/5] fast-import: fix erroneous handling of get-mark with empty orphan commits

## tl;dr

15 messages between Feb 20, 2019 and Apr 1, 2019. Diffs are folded; open one to read it.

replies: 14people: 2as markdown or json

Elijah Newren· Feb 20, 2019, 22:58 UTC · lore

[RFC PATCH 0/5] Fix some fast-import parsing issues

I found a few issues with parsing in fast-import (dating back to git-2.6.0). I was thrown off track for a while by the fact that the .git/fast_import_crash_$PID file erroneously claimed that the penultimate command it read was somehow truncated; e.g. for input of the form:

    commit refs/heads/master
    mark :1
    author Full Name <user@organization.tld> 1000000000 +0100
    committer Full Name <user@organization.tld> 1000000000 +0100
    data 53
    Commit message for intentionally empty orphan commit
    get-mark :1
    
the crash file was reporting
    Most Recent Commands Before Crash
    ---------------------------------
      commit refs/heads/master
      mark :1
      author Full Name <user@organization.tld> 1000000000 +0100
      committer Full Name <user@organization.tld> 1000000000 +0100
      data 53
    * get-mark :1
    
In other words, it dropped the commit message line entirely, which
made me wonder if I was having weird buffering or flushing issues
until I was able to narrow it down to the simple testcase above and
could duplicate with
    cat seven-line-test-file | git fast-import
I don't know if there are other ways to get fast-import to give a
corrupted "Most Recent Commands Before Crash" report, but this one
doesn't trigger after my other fixes below.

Switching gears from the bad reporting to the original parsing bug though, it is clear fast-import somehow mishandled get-mark directives after empty orphan commits. Digging into it, I think it was caused by following a precedent set by a somewhat unsafe implementation of trying to allow cat-blob directives to appear in the middle of a commit. My spelunking showed the following commits seemed to be the most relevant as to how we got to the current state:

    777f80d7429b ("fast-import: Allow cat-blob requests at arbitrary
                 points in stream", 2010-11-28)
    8dc6a373d201 ("fast-import: add 'ls' command", 2010-12-02)
    97313bef2a16 ("fast-import: use skip_prefix for parsing input",
                  2014-06-18)
    28c7b1f7b7b7 ("fast-import: add a get-mark command", 2015-07-01)

I've cc'ed the relevant folks, and have a few patches that fix the issue and I think make the parser more robust against future issues in a way that I think is safe enough for backward compatibility, but "backward compatible enough" might concern some folks; if so, please take a look at patches 4 and 5.

Elijah Newren (5):
  t9300: demonstrate bug with get-mark and empty orphan commits
  git-fast-import.txt: fix wording about where ls command can appear
  fast-import: check most prominent commands first
  fast-import: only allow cat-blob requests where it makes sense
  fast-import: fix erroneous handling of get-mark with empty orphan
    commits
 Documentation/git-fast-import.txt | 22 +++++++++---------
 fast-import.c                     | 31 ++++++++++++++------------
 t/t9300-fast-import.sh            | 37 +++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 24 deletions(-)
-- 
2.21.0.rc2.5.g8f70af2367
Elijah Newren· Feb 20, 2019, 22:58 UTC · re: Elijah Newren · lore

When get-mark was introduced in commit 28c7b1f7b7b7 ("fast-import: add a get-mark command", 2015-07-01), it followed the precedent of the cat-blob command to be allowed on any line other than in the middle of a data directive; see commit 777f80d7429b ("fast-import: Allow cat-blob requests at arbitrary points in stream", 2010-11-28). It was useful to allow cat-blob directives in the middle of a commit to get more data that would be used in writing the current commit object. get-mark is not similarly useful since fast-import can already use either object id or mark. Further, trying to allow this command anywhere caused parsing bugs. Fix the parsing problems by only allowing get-mark commands to appear when other commands have completed.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-fast-import.txt | 4 ----
 fast-import.c                     | 8 ++------
 t/t9300-fast-import.sh            | 4 ++--
 3 files changed, 4 insertions(+), 12 deletions(-)
Show changes to 3 files +4 −12

Documentation/git-fast-import.txt, fast-import.c, t/t9300-fast-import.sh

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 982f82b0b3..33cce1e150 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -971,10 +971,6 @@ might want to refer to in their commit messages.
 	'get-mark' SP ':' <idnum> LF
 ....
 
-This command can be used anywhere in the stream that comments are
-accepted.  In particular, the `get-mark` command can be used in the
-middle of a commit but not in the middle of a `data` command.
-
 See ``Responses To Commands'' below for details about how to read
 this output safely.
 
diff --git a/fast-import.c b/fast-import.c
index 338db61e6e..064c55e8be 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1748,8 +1748,6 @@ static int read_next_command(void)
 	}
 
 	for (;;) {
-		const char *p;
-
 		if (unread_command_buf) {
 			unread_command_buf = 0;
 		} else {
@@ -1782,10 +1780,6 @@ static int read_next_command(void)
 			rc->prev->next = rc;
 			cmd_tail = rc;
 		}
-		if (skip_prefix(command_buf.buf, "get-mark ", &p)) {
-			parse_get_mark(p);
-			continue;
-		}
 		if (command_buf.buf[0] == '#')
 			continue;
 		return 0;
@@ -3318,6 +3312,8 @@ int cmd_main(int argc, const char **argv)
 			parse_ls(v, NULL);
 		else if (skip_prefix(command_buf.buf, "cat-blob ", &v))
 			parse_cat_blob(v);
+		else if (skip_prefix(command_buf.buf, "get-mark ", &v))
+			parse_get_mark(v);
 		else if (!strcmp("checkpoint", command_buf.buf))
 			parse_checkpoint();
 		else if (!strcmp("done", command_buf.buf))
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index c304c8c47c..3668263c40 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -3276,11 +3276,11 @@ cat >>W-input <<-W_INPUT_END
 	LFsget-mark :1
 	W_INPUT_END
 
-test_expect_failure !MINGW 'W: get-mark & empty orphan commit with no newlines' '
+test_expect_success !MINGW 'W: get-mark & empty orphan commit with no newlines' '
 	sed -e s/LFs// W-input | tr L "\n" | git fast-import
 '
 
-test_expect_failure !MINGW 'W: get-mark & empty orphan commit with one newline' '
+test_expect_success !MINGW 'W: get-mark & empty orphan commit with one newline' '
 	sed -e s/LFs/L/ W-input | tr L "\n" | git fast-import
 '
 

-- 
2.21.0.rc2.5.g8f70af2367
Elijah Newren· Feb 20, 2019, 22:58 UTC · re: Elijah Newren · lore

[RFC PATCH 2/5] git-fast-import.txt: fix wording about where ls command can appear

The docs claimed `ls` commands could appear almost anywhere, but the code told a different story. Modify the docs to match the code.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-fast-import.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to Documentation/git-fast-import.txt +2 −2
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 339b6e7e98..f7e2d330b1 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -1016,8 +1016,8 @@ printing a blob from the active commit (with `cat-blob`) or copying a
 blob or tree from a previous commit for use in the current one (with
 `filemodify`).
 
-The `ls` command can be used anywhere in the stream that comments are
-accepted, including the middle of a commit.
+The `ls` command can also be used where a `filemodify` directive can
+appear, allowing it to be used in the middle of a commit.
 
 Reading from the active commit::
 	This form can only be used in the middle of a `commit`.

-- 
2.21.0.rc2.5.g8f70af2367
Elijah Newren· Feb 20, 2019, 22:58 UTC · re: Elijah Newren · lore

[RFC PATCH 3/5] fast-import: check most prominent commands first

This is not a very important change, and one that I expect to have no performance impact whatsoever, but reading the code bothered me. The parsing of command types in cmd_main() mostly runs in order of most common to least common commands; sure, it's hard to say for sure what the most common are without some type of study, but it seems fairly clear to mark the original four ("blob", "commit", "tag", "reset") as the most prominent. Indeed, the parsing for most other commands were added to later in the list. However, when "ls" was added, it was stuck near the top of the list, with no rationale for that particular location. Move it down to later to appease my Tourette's-like internal twitching that its former location was causing.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 fast-import.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to fast-import.c +2 −2
diff --git a/fast-import.c b/fast-import.c
index b7ba755c2b..3114ce17f1 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3303,14 +3303,14 @@ int cmd_main(int argc, const char **argv)
 		const char *v;
 		if (!strcmp("blob", command_buf.buf))
 			parse_new_blob();
-		else if (skip_prefix(command_buf.buf, "ls ", &v))
-			parse_ls(v, NULL);
 		else if (skip_prefix(command_buf.buf, "commit ", &v))
 			parse_new_commit(v);
 		else if (skip_prefix(command_buf.buf, "tag ", &v))
 			parse_new_tag(v);
 		else if (skip_prefix(command_buf.buf, "reset ", &v))
 			parse_reset_branch(v);
+		else if (skip_prefix(command_buf.buf, "ls ", &v))
+			parse_ls(v, NULL);
 		else if (!strcmp("checkpoint", command_buf.buf))
 			parse_checkpoint();
 		else if (!strcmp("done", command_buf.buf))

-- 
2.21.0.rc2.5.g8f70af2367
Elijah Newren· Feb 20, 2019, 22:58 UTC · re: Elijah Newren · lore

[RFC PATCH 4/5] fast-import: only allow cat-blob requests where it makes sense

In commit 777f80d7429b ("fast-import: Allow cat-blob requests at arbitrary points in stream", 2010-11-28), fast-import started allowing cat-blob commands to appear on the start of any line except in the middle of a "data" command. It could be in the middle of various directives that were part of a tag command, or in the middle of checkpoints or progresses (each of which allow an optional second empty newline), or even immediately after the mark command of a blob before the data directive appeared (raising the question of what if it used the mark for the blob that just barely appeared in the stream that we do not yet have the data for). None of these locations make any sense as places to put cat-blob requests.

The purpose of this change as stated in that commit message was to
   [save] frontends from having to loop over everything they want to
   commit in the next commit and cat-ing the necessary objects in
   advance.

However, that can be achieved by simply allowing cat-blob requests to appear whenever a filemodify directive is allowed. Further, it avoids setting a bad precedent for other commands to follow (e.g. get-mark); a precedent which caused parsing problems in corner cases.

Technically, inline filemodify directives add a slight wrinkle in that frontends might want to have cat-blob directives appear after the start of the filemodify and before the data directive contained within it. I think it would have been better to disallow such a case (it would be trivial to use cat-blob before the filemodify instead), but since there is evidence this was used, for backwards compatibility let's support that case too.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-fast-import.txt |  7 ++++---
 fast-import.c                     | 19 +++++++++++++------
 2 files changed, 17 insertions(+), 9 deletions(-)
Show changes to 2 files +17 −9

Documentation/git-fast-import.txt, fast-import.c

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index f7e2d330b1..982f82b0b3 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -1001,9 +1001,10 @@ Output uses the same format as `git cat-file --batch`:
 	<contents> LF
 ====
 
-This command can be used anywhere in the stream that comments are
-accepted.  In particular, the `cat-blob` command can be used in the
-middle of a commit but not in the middle of a `data` command.
+This command can be used where a `filemodify` directive can appear,
+allowing it to be used in the middle of a commit.  For a `filemodify`
+using an inline directive, it can also appear right before the `data`
+directive.
 
 See ``Responses To Commands'' below for details about how to read
 this output safely.
diff --git a/fast-import.c b/fast-import.c
index 3114ce17f1..338db61e6e 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1786,10 +1786,6 @@ static int read_next_command(void)
 			parse_get_mark(p);
 			continue;
 		}
-		if (skip_prefix(command_buf.buf, "cat-blob ", &p)) {
-			parse_cat_blob(p);
-			continue;
-		}
 		if (command_buf.buf[0] == '#')
 			continue;
 		return 0;
@@ -2254,8 +2250,15 @@ static void file_change_m(const char *p, struct branch *b)
 			strbuf_addstr(&uq, p);
 			p = uq.buf;
 		}
-		read_next_command();
-		parse_and_store_blob(&last_blob, &oid, 0);
+		while (read_next_command() != EOF) {
+			const char *v;
+			if (skip_prefix(command_buf.buf, "cat-blob ", &v))
+				parse_cat_blob(v);
+			else {
+				parse_and_store_blob(&last_blob, &oid, 0);
+				break;
+			}
+		}
 	} else {
 		enum object_type expected = S_ISDIR(mode) ?
 						OBJ_TREE: OBJ_BLOB;
@@ -2627,6 +2630,8 @@ static void parse_new_commit(const char *arg)
 			file_change_deleteall(b);
 		else if (skip_prefix(command_buf.buf, "ls ", &v))
 			parse_ls(v, b);
+		else if (skip_prefix(command_buf.buf, "cat-blob ", &v))
+			parse_cat_blob(v);
 		else {
 			unread_command_buf = 1;
 			break;
@@ -3311,6 +3316,8 @@ int cmd_main(int argc, const char **argv)
 			parse_reset_branch(v);
 		else if (skip_prefix(command_buf.buf, "ls ", &v))
 			parse_ls(v, NULL);
+		else if (skip_prefix(command_buf.buf, "cat-blob ", &v))
+			parse_cat_blob(v);
 		else if (!strcmp("checkpoint", command_buf.buf))
 			parse_checkpoint();
 		else if (!strcmp("done", command_buf.buf))

-- 
2.21.0.rc2.5.g8f70af2367
Elijah Newren· Feb 20, 2019, 22:58 UTC · re: Elijah Newren · lore

[RFC PATCH 1/5] t9300: demonstrate bug with get-mark and empty orphan commits

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-fast-import.txt |  7 +++++-
 t/t9300-fast-import.sh            | 37 +++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)
Show changes to 2 files +43 −1

Documentation/git-fast-import.txt, t/t9300-fast-import.sh

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 43ab3b1637..339b6e7e98 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -422,7 +422,12 @@ However it is recommended that a `filedeleteall` command precede
 all `filemodify`, `filecopy`, `filerename` and `notemodify` commands in
 the same commit, as `filedeleteall` wipes the branch clean (see below).
 
-The `LF` after the command is optional (it used to be required).
+The `LF` after the command is optional (it used to be required).  Note
+that for reasons of backward compatibility, if the commit ends with a
+`data` command (i.e. it has has no `from`, `merge`, `filemodify`,
+`filedelete`, `filecopy`, `filerename`, `filedeleteall` or
+`notemodify` commands) then two `LF` commands may appear at the end of
+the command instead of just one.
 
 `author`
 ^^^^^^^^
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 59a13b6a77..c304c8c47c 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -3262,4 +3262,41 @@ test_expect_success PIPE 'V: checkpoint updates tags after tag' '
 	background_import_still_running
 '
 
+###
+### series W (get-mark and empty orphan commits)
+###
+
+cat >>W-input <<-W_INPUT_END
+	commit refs/heads/W-branch
+	mark :1
+	author Full Name <user@company.tld> 1000000000 +0100
+	committer Full Name <user@company.tld> 1000000000 +0100
+	data 27
+	Intentionally empty commit
+	LFsget-mark :1
+	W_INPUT_END
+
+test_expect_failure !MINGW 'W: get-mark & empty orphan commit with no newlines' '
+	sed -e s/LFs// W-input | tr L "\n" | git fast-import
+'
+
+test_expect_failure !MINGW 'W: get-mark & empty orphan commit with one newline' '
+	sed -e s/LFs/L/ W-input | tr L "\n" | git fast-import
+'
+
+test_expect_success !MINGW 'W: get-mark & empty orphan commit with ugly second newline' '
+	# Technically, this should fail as it has too many linefeeds
+	# according to the grammar in fast-import.txt.  But, for whatever
+	# reason, it works.  Since using the correct number of newlines
+	# does not work with older (pre-2.22) versions of git, allow apps
+	# that used this second-newline workaround to keep working by
+	# checking it with this test...
+	sed -e s/LFs/LL/ W-input | tr L "\n" | git fast-import
+'
+
+test_expect_success !MINGW 'W: get-mark & empty orphan commit with erroneous third newline' '
+	# ...but do NOT allow more empty lines than that (see previous test).
+	sed -e s/LFs/LLL/ W-input | tr L "\n" | test_must_fail git fast-import
+'
+
 test_done

-- 
2.21.0.rc2.5.g8f70af2367
Elijah Newren· Mar 8, 2019, 16:53 UTC · re: Elijah Newren · lore

Re: [RFC PATCH 0/5] Fix some fast-import parsing issues

Hi,
On Wed, Feb 20, 2019 at 2:58 PM Elijah Newren <newren@gmail.com> wrote:
>
> I found a few issues with parsing in fast-import (dating back to
....
Show 5 quoted lines
> I've cc'ed the relevant folks, and have a few patches that fix the
> issue and I think make the parser more robust against future issues in
> a way that I think is safe enough for backward compatibility, but
> "backward compatible enough" might concern some folks; if so, please
> take a look at patches 4 and 5.

Just thought I'd ping to see if folks have any concerns with this slight tweak to backward compatibility; if not, I'll just repost the patches removing the RFC label.

Junio C Hamano· Apr 1, 2019, 10:44 UTC · re: Elijah Newren · lore

Re: [RFC PATCH 0/5] Fix some fast-import parsing issues

Elijah Newren <newren@gmail.com> writes:
Show 13 quoted lines
> On Wed, Feb 20, 2019 at 2:58 PM Elijah Newren <newren@gmail.com> wrote:
>>
>> I found a few issues with parsing in fast-import (dating back to
> ....
>> I've cc'ed the relevant folks, and have a few patches that fix the
>> issue and I think make the parser more robust against future issues in
>> a way that I think is safe enough for backward compatibility, but
>> "backward compatible enough" might concern some folks; if so, please
>> take a look at patches 4 and 5.
>
> Just thought I'd ping to see if folks have any concerns with this
> slight tweak to backward compatibility; if not, I'll just repost the
> patches removing the RFC label.

It's been more than a month and we haven't hard from anybody, perhaps?

Elijah Newren· Apr 1, 2019, 15:52 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH 0/5] Fix some fast-import parsing issues

On Mon, Apr 1, 2019 at 3:44 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
>
> Elijah Newren <newren@gmail.com> writes:
>
> > On Wed, Feb 20, 2019 at 2:58 PM Elijah Newren <newren@gmail.com> wrote:
> >>
> >> I found a few issues with parsing in fast-import (dating back to
> > ....
> >> I've cc'ed the relevant folks, and have a few patches that fix the
> >> issue and I think make the parser more robust against future issues in
> >> a way that I think is safe enough for backward compatibility, but
> >> "backward compatible enough" might concern some folks; if so, please
> >> take a look at patches 4 and 5.
> >
> > Just thought I'd ping to see if folks have any concerns with this
> > slight tweak to backward compatibility; if not, I'll just repost the
> > patches removing the RFC label.
>
> It's been more than a month and we haven't hard from anybody,
> perhaps?

Indeed; I was going to resend sooner but got wrapped up in directory-rename-conflict-by-default, switch and restore series discussions, etc. I'll rip off the RFC label and resend.

Elijah Newren· Apr 1, 2019, 16:00 UTC · re: Elijah Newren · lore

[PATCH v2 0/5] Fix some fast-import parsing issues

This series fixes a few issues with parsing in fast-import, dating back to git-2.6.0. The only change since v1 is removing the RFC label; sadly, no one commented on the first round.

Text from my original submission:

The point behind this series is that fast-import somehow mishandled get-mark directives after empty orphan commits. Digging into it, I think it was caused by following a precedent set by a somewhat unsafe implementation of trying to allow cat-blob directives to appear in the middle of a commit. My spelunking showed the following commits seemed to be the most relevant as to how we got to the current state:

    777f80d7429b ("fast-import: Allow cat-blob requests at arbitrary
                 points in stream", 2010-11-28)
    8dc6a373d201 ("fast-import: add 'ls' command", 2010-12-02)
    97313bef2a16 ("fast-import: use skip_prefix for parsing input",
                  2014-06-18)
    28c7b1f7b7b7 ("fast-import: add a get-mark command", 2015-07-01)

I have a few patches that fix the issue and I think make the parser more robust against future issues in a way that I think is safe enough for backward compatibility, but "backward compatible enough" might concern some folks; if so, please take a look at patches 4 and 5.

Elijah Newren (5):
  t9300: demonstrate bug with get-mark and empty orphan commits
  git-fast-import.txt: fix wording about where ls command can appear
  fast-import: check most prominent commands first
  fast-import: only allow cat-blob requests where it makes sense
  fast-import: fix erroneous handling of get-mark with empty orphan
    commits
 Documentation/git-fast-import.txt | 22 +++++++++---------
 fast-import.c                     | 31 ++++++++++++++------------
 t/t9300-fast-import.sh            | 37 +++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 24 deletions(-)
-- 
2.21.0.1.g0a561c1dbd
Elijah Newren· Apr 1, 2019, 16:00 UTC · re: Elijah Newren · lore

[PATCH v2 1/5] t9300: demonstrate bug with get-mark and empty orphan commits

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-fast-import.txt |  7 +++++-
 t/t9300-fast-import.sh            | 37 +++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 1 deletion(-)
Show changes to 2 files +43 −1

Documentation/git-fast-import.txt, t/t9300-fast-import.sh

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 43ab3b1637..339b6e7e98 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -422,7 +422,12 @@ However it is recommended that a `filedeleteall` command precede
 all `filemodify`, `filecopy`, `filerename` and `notemodify` commands in
 the same commit, as `filedeleteall` wipes the branch clean (see below).
 
-The `LF` after the command is optional (it used to be required).
+The `LF` after the command is optional (it used to be required).  Note
+that for reasons of backward compatibility, if the commit ends with a
+`data` command (i.e. it has has no `from`, `merge`, `filemodify`,
+`filedelete`, `filecopy`, `filerename`, `filedeleteall` or
+`notemodify` commands) then two `LF` commands may appear at the end of
+the command instead of just one.
 
 `author`
 ^^^^^^^^
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 59a13b6a77..c304c8c47c 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -3262,4 +3262,41 @@ test_expect_success PIPE 'V: checkpoint updates tags after tag' '
 	background_import_still_running
 '
 
+###
+### series W (get-mark and empty orphan commits)
+###
+
+cat >>W-input <<-W_INPUT_END
+	commit refs/heads/W-branch
+	mark :1
+	author Full Name <user@company.tld> 1000000000 +0100
+	committer Full Name <user@company.tld> 1000000000 +0100
+	data 27
+	Intentionally empty commit
+	LFsget-mark :1
+	W_INPUT_END
+
+test_expect_failure !MINGW 'W: get-mark & empty orphan commit with no newlines' '
+	sed -e s/LFs// W-input | tr L "\n" | git fast-import
+'
+
+test_expect_failure !MINGW 'W: get-mark & empty orphan commit with one newline' '
+	sed -e s/LFs/L/ W-input | tr L "\n" | git fast-import
+'
+
+test_expect_success !MINGW 'W: get-mark & empty orphan commit with ugly second newline' '
+	# Technically, this should fail as it has too many linefeeds
+	# according to the grammar in fast-import.txt.  But, for whatever
+	# reason, it works.  Since using the correct number of newlines
+	# does not work with older (pre-2.22) versions of git, allow apps
+	# that used this second-newline workaround to keep working by
+	# checking it with this test...
+	sed -e s/LFs/LL/ W-input | tr L "\n" | git fast-import
+'
+
+test_expect_success !MINGW 'W: get-mark & empty orphan commit with erroneous third newline' '
+	# ...but do NOT allow more empty lines than that (see previous test).
+	sed -e s/LFs/LLL/ W-input | tr L "\n" | test_must_fail git fast-import
+'
+
 test_done

-- 
2.21.0.1.g0a561c1dbd
Elijah Newren· Apr 1, 2019, 16:00 UTC · re: Elijah Newren · lore

[PATCH v2 5/5] fast-import: fix erroneous handling of get-mark with empty orphan commits

When get-mark was introduced in commit 28c7b1f7b7b7 ("fast-import: add a get-mark command", 2015-07-01), it followed the precedent of the cat-blob command to be allowed on any line other than in the middle of a data directive; see commit 777f80d7429b ("fast-import: Allow cat-blob requests at arbitrary points in stream", 2010-11-28). It was useful to allow cat-blob directives in the middle of a commit to get more data that would be used in writing the current commit object. get-mark is not similarly useful since fast-import can already use either object id or mark. Further, trying to allow this command anywhere caused parsing bugs. Fix the parsing problems by only allowing get-mark commands to appear when other commands have completed.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-fast-import.txt | 4 ----
 fast-import.c                     | 8 ++------
 t/t9300-fast-import.sh            | 4 ++--
 3 files changed, 4 insertions(+), 12 deletions(-)
Show changes to 3 files +4 −12

Documentation/git-fast-import.txt, fast-import.c, t/t9300-fast-import.sh

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 982f82b0b3..33cce1e150 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -971,10 +971,6 @@ might want to refer to in their commit messages.
 	'get-mark' SP ':' <idnum> LF
 ....
 
-This command can be used anywhere in the stream that comments are
-accepted.  In particular, the `get-mark` command can be used in the
-middle of a commit but not in the middle of a `data` command.
-
 See ``Responses To Commands'' below for details about how to read
 this output safely.
 
diff --git a/fast-import.c b/fast-import.c
index 338db61e6e..064c55e8be 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1748,8 +1748,6 @@ static int read_next_command(void)
 	}
 
 	for (;;) {
-		const char *p;
-
 		if (unread_command_buf) {
 			unread_command_buf = 0;
 		} else {
@@ -1782,10 +1780,6 @@ static int read_next_command(void)
 			rc->prev->next = rc;
 			cmd_tail = rc;
 		}
-		if (skip_prefix(command_buf.buf, "get-mark ", &p)) {
-			parse_get_mark(p);
-			continue;
-		}
 		if (command_buf.buf[0] == '#')
 			continue;
 		return 0;
@@ -3318,6 +3312,8 @@ int cmd_main(int argc, const char **argv)
 			parse_ls(v, NULL);
 		else if (skip_prefix(command_buf.buf, "cat-blob ", &v))
 			parse_cat_blob(v);
+		else if (skip_prefix(command_buf.buf, "get-mark ", &v))
+			parse_get_mark(v);
 		else if (!strcmp("checkpoint", command_buf.buf))
 			parse_checkpoint();
 		else if (!strcmp("done", command_buf.buf))
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index c304c8c47c..3668263c40 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -3276,11 +3276,11 @@ cat >>W-input <<-W_INPUT_END
 	LFsget-mark :1
 	W_INPUT_END
 
-test_expect_failure !MINGW 'W: get-mark & empty orphan commit with no newlines' '
+test_expect_success !MINGW 'W: get-mark & empty orphan commit with no newlines' '
 	sed -e s/LFs// W-input | tr L "\n" | git fast-import
 '
 
-test_expect_failure !MINGW 'W: get-mark & empty orphan commit with one newline' '
+test_expect_success !MINGW 'W: get-mark & empty orphan commit with one newline' '
 	sed -e s/LFs/L/ W-input | tr L "\n" | git fast-import
 '
 

-- 
2.21.0.1.g0a561c1dbd
Elijah Newren· Apr 1, 2019, 16:00 UTC · re: Elijah Newren · lore

[PATCH v2 3/5] fast-import: check most prominent commands first

This is not a very important change, and one that I expect to have no performance impact whatsoever, but reading the code bothered me. The parsing of command types in cmd_main() mostly runs in order of most common to least common commands; sure, it's hard to say for sure what the most common are without some type of study, but it seems fairly clear to mark the original four ("blob", "commit", "tag", "reset") as the most prominent. Indeed, the parsing for most other commands were added to later in the list. However, when "ls" was added, it was stuck near the top of the list, with no rationale for that particular location. Move it down to later to appease my Tourette's-like internal twitching that its former location was causing.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 fast-import.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to fast-import.c +2 −2
diff --git a/fast-import.c b/fast-import.c
index b7ba755c2b..3114ce17f1 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3303,14 +3303,14 @@ int cmd_main(int argc, const char **argv)
 		const char *v;
 		if (!strcmp("blob", command_buf.buf))
 			parse_new_blob();
-		else if (skip_prefix(command_buf.buf, "ls ", &v))
-			parse_ls(v, NULL);
 		else if (skip_prefix(command_buf.buf, "commit ", &v))
 			parse_new_commit(v);
 		else if (skip_prefix(command_buf.buf, "tag ", &v))
 			parse_new_tag(v);
 		else if (skip_prefix(command_buf.buf, "reset ", &v))
 			parse_reset_branch(v);
+		else if (skip_prefix(command_buf.buf, "ls ", &v))
+			parse_ls(v, NULL);
 		else if (!strcmp("checkpoint", command_buf.buf))
 			parse_checkpoint();
 		else if (!strcmp("done", command_buf.buf))

-- 
2.21.0.1.g0a561c1dbd
Elijah Newren· Apr 1, 2019, 16:00 UTC · re: Elijah Newren · lore

[PATCH v2 4/5] fast-import: only allow cat-blob requests where it makes sense

In commit 777f80d7429b ("fast-import: Allow cat-blob requests at arbitrary points in stream", 2010-11-28), fast-import started allowing cat-blob commands to appear on the start of any line except in the middle of a "data" command. It could be in the middle of various directives that were part of a tag command, or in the middle of checkpoints or progresses (each of which allow an optional second empty newline), or even immediately after the mark command of a blob before the data directive appeared (raising the question of what if it used the mark for the blob that just barely appeared in the stream that we do not yet have the data for). None of these locations make any sense as places to put cat-blob requests.

The purpose of this change as stated in that commit message was to
   [save] frontends from having to loop over everything they want to
   commit in the next commit and cat-ing the necessary objects in
   advance.

However, that can be achieved by simply allowing cat-blob requests to appear whenever a filemodify directive is allowed. Further, it avoids setting a bad precedent for other commands to follow (e.g. get-mark); a precedent which caused parsing problems in corner cases.

Technically, inline filemodify directives add a slight wrinkle in that frontends might want to have cat-blob directives appear after the start of the filemodify and before the data directive contained within it. I think it would have been better to disallow such a case (it would be trivial to use cat-blob before the filemodify instead), but since there is evidence this was used, for backwards compatibility let's support that case too.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-fast-import.txt |  7 ++++---
 fast-import.c                     | 19 +++++++++++++------
 2 files changed, 17 insertions(+), 9 deletions(-)
Show changes to 2 files +17 −9

Documentation/git-fast-import.txt, fast-import.c

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index f7e2d330b1..982f82b0b3 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -1001,9 +1001,10 @@ Output uses the same format as `git cat-file --batch`:
 	<contents> LF
 ====
 
-This command can be used anywhere in the stream that comments are
-accepted.  In particular, the `cat-blob` command can be used in the
-middle of a commit but not in the middle of a `data` command.
+This command can be used where a `filemodify` directive can appear,
+allowing it to be used in the middle of a commit.  For a `filemodify`
+using an inline directive, it can also appear right before the `data`
+directive.
 
 See ``Responses To Commands'' below for details about how to read
 this output safely.
diff --git a/fast-import.c b/fast-import.c
index 3114ce17f1..338db61e6e 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1786,10 +1786,6 @@ static int read_next_command(void)
 			parse_get_mark(p);
 			continue;
 		}
-		if (skip_prefix(command_buf.buf, "cat-blob ", &p)) {
-			parse_cat_blob(p);
-			continue;
-		}
 		if (command_buf.buf[0] == '#')
 			continue;
 		return 0;
@@ -2254,8 +2250,15 @@ static void file_change_m(const char *p, struct branch *b)
 			strbuf_addstr(&uq, p);
 			p = uq.buf;
 		}
-		read_next_command();
-		parse_and_store_blob(&last_blob, &oid, 0);
+		while (read_next_command() != EOF) {
+			const char *v;
+			if (skip_prefix(command_buf.buf, "cat-blob ", &v))
+				parse_cat_blob(v);
+			else {
+				parse_and_store_blob(&last_blob, &oid, 0);
+				break;
+			}
+		}
 	} else {
 		enum object_type expected = S_ISDIR(mode) ?
 						OBJ_TREE: OBJ_BLOB;
@@ -2627,6 +2630,8 @@ static void parse_new_commit(const char *arg)
 			file_change_deleteall(b);
 		else if (skip_prefix(command_buf.buf, "ls ", &v))
 			parse_ls(v, b);
+		else if (skip_prefix(command_buf.buf, "cat-blob ", &v))
+			parse_cat_blob(v);
 		else {
 			unread_command_buf = 1;
 			break;
@@ -3311,6 +3316,8 @@ int cmd_main(int argc, const char **argv)
 			parse_reset_branch(v);
 		else if (skip_prefix(command_buf.buf, "ls ", &v))
 			parse_ls(v, NULL);
+		else if (skip_prefix(command_buf.buf, "cat-blob ", &v))
+			parse_cat_blob(v);
 		else if (!strcmp("checkpoint", command_buf.buf))
 			parse_checkpoint();
 		else if (!strcmp("done", command_buf.buf))

-- 
2.21.0.1.g0a561c1dbd
Elijah Newren· Apr 1, 2019, 16:00 UTC · re: Elijah Newren · lore

[PATCH v2 2/5] git-fast-import.txt: fix wording about where ls command can appear

The docs claimed `ls` commands could appear almost anywhere, but the code told a different story. Modify the docs to match the code.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-fast-import.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to Documentation/git-fast-import.txt +2 −2
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 339b6e7e98..f7e2d330b1 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -1016,8 +1016,8 @@ printing a blob from the active commit (with `cat-blob`) or copying a
 blob or tree from a previous commit for use in the current one (with
 `filemodify`).
 
-The `ls` command can be used anywhere in the stream that comments are
-accepted, including the middle of a commit.
+The `ls` command can also be used where a `filemodify` directive can
+appear, allowing it to be used in the middle of a commit.
 
 Reading from the active commit::
 	This form can only be used in the middle of a `commit`.

-- 
2.21.0.1.g0a561c1dbd

← back to recent threads