Proposed patch for mktree [0/3]

Proposed patch for mktree [0/3]

[Josh Micich]

Here is a series of 3 patches to git-mktree in addition to the recent work done 
on pu.

The first is just small fixes after the work on the '--missing' option.
The second patch adds a '--batch' option to mktree (similar to '--batch' in 
cat-file) that allows multiple tree objects to be created interactively by a 
single process.
Finally there are some improvements to the validation of the type of object 
identified by the tree entry sha1 (previously the sha1 was only used to check 
for existence, not type).

-josh


Josh Micich (3):
  --missing option for mktree: re-added strbuf_release(&p_uq), Updated man page
  added --batch option to mk-tree
  improved validation of entry type in mktree

 Documentation/git-mktree.txt |   16 ++++++++--
 builtin-mktree.c             |   63 +++++++++++++++++++++++++++++++----------
 2 files changed, 60 insertions(+), 19 deletions(-)

[PATCH 1/3] '--missing' option for mktree: re-added strbuf_release(&p_uq), Updated man page

[Josh Micich]

Re-added call to strbuf_release(&p_uq) that got lost in earlier changes.
Updated mktree_usage msg.

Updated man page to explain new '--missing' option.
Also clarified sorting behaviour.

Signed-off-by: Josh Micich <josh.micich@gmail.com>
---
 Documentation/git-mktree.txt |   11 ++++++++---
 builtin-mktree.c             |    5 ++++-
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-mktree.txt b/Documentation/git-mktree.txt
index af19f06..0461062 100644
--- a/Documentation/git-mktree.txt
+++ b/Documentation/git-mktree.txt
@@ -8,18 +8,23 @@ git-mktree - Build a tree-object from ls-tree formatted text
 
 SYNOPSIS
 --------
-'git mktree' [-z]
+'git mktree' [-z] [--missing]
 
 DESCRIPTION
 -----------
-Reads standard input in non-recursive `ls-tree` output format,
-and creates a tree object.  The object name of the tree object
+Reads standard input in non-recursive `ls-tree` output format, and creates
+a tree object.  The order of the tree entries is normalised by mktree so
+pre-sorting the input is not required.  The object name of the tree object
 built is written to the standard output.
 
 OPTIONS
 -------
 -z::
 	Read the NUL-terminated `ls-tree -z` output instead.
+--missing::
+	Allow missing objects.  The default behaviour (without this option)
+	is to verify that each tree entry's sha1 identifies an existing
+	object.
 
 Author
 ------
diff --git a/builtin-mktree.c b/builtin-mktree.c
index e1c9a27..db647ce 100644
--- a/builtin-mktree.c
+++ b/builtin-mktree.c
@@ -63,7 +63,7 @@ static void write_tree(unsigned char *sha1)
 }
 
 static const char *mktree_usage[] = {
-	"git mktree [-z]",
+	"git mktree [-z] [--missing]",
 	NULL
 };
 
@@ -112,6 +112,9 @@ static void mktree_line(char *buf, size_t len, int 
line_termination, int allow_m
 		if (unquote_c_style(&p_uq, path, NULL))
 			die("invalid quoting");
 		path = strbuf_detach(&p_uq, NULL);
+		append_to_tree(mode, sha1, path);
+		strbuf_release(&p_uq);
+		return;
 	}
 	append_to_tree(mode, sha1, path);
 }
-- 
1.6.3.165.g2cce5.dirty

[PATCH 2/3] added --batch option to mktree

[Josh Micich]

This option enables creation of many tree objects with a single process

Signed-off-by: Josh Micich <josh.micich@gmail.com>
---
 Documentation/git-mktree.txt |    7 ++++++-
 builtin-mktree.c             |   36 +++++++++++++++++++++++++++++-------
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-mktree.txt b/Documentation/git-mktree.txt
index 0461062..3d2237a 100644
--- a/Documentation/git-mktree.txt
+++ b/Documentation/git-mktree.txt
@@ -8,7 +8,7 @@ git-mktree - Build a tree-object from ls-tree formatted text
 
 SYNOPSIS
 --------
-'git mktree' [-z] [--missing]
+'git mktree' [-z] [--missing] [--batch]
 
 DESCRIPTION
 -----------
@@ -25,6 +25,11 @@ OPTIONS
 	Allow missing objects.  The default behaviour (without this option)
 	is to verify that each tree entry's sha1 identifies an existing
 	object.
+--batch::
+	Allow building of more than one tree object before exiting.  Each
+	tree is separated by as single blank line. The final new-line is
+	optional.  Note - if the '-z' option is used, lines are terminated
+	with NUL.
 
 Author
 ------
diff --git a/builtin-mktree.c b/builtin-mktree.c
index db647ce..a56c917 100644
--- a/builtin-mktree.c
+++ b/builtin-mktree.c
@@ -63,7 +63,7 @@ static void write_tree(unsigned char *sha1)
 }
 
 static const char *mktree_usage[] = {
-	"git mktree [-z] [--missing]",
+	"git mktree [-z] [--missing] [--batch]",
 	NULL
 };
 
@@ -125,20 +125,42 @@ int cmd_mktree(int ac, const char **av, const char 
*prefix)
 	unsigned char sha1[20];
 	int line_termination = '\n';
 	int allow_missing = 0;
+	int is_batch_mode = 0;
+
 	const struct option option[] = {
 		OPT_SET_INT('z', NULL, &line_termination, "input is NUL 
terminated", '\0'),
 		OPT_SET_INT( 0 , "missing", &allow_missing, "allow missing 
objects", 1),
+		OPT_SET_INT( 0 , "batch", &is_batch_mode, "interactively create 
more than one tree", 1),
 		OPT_END()
 	};
 
 	ac = parse_options(ac, av, option, mktree_usage, 0);
 
-	while (strbuf_getline(&sb, stdin, line_termination) != EOF)
-		mktree_line(sb.buf, sb.len, line_termination, allow_missing);
-
+	int got_eof = 0;
+	while (!got_eof) {
+		while (1) {
+			if (strbuf_getline(&sb, stdin, line_termination) == 
EOF) {
+				got_eof = 1;
+				break;
+			}
+			if (sb.buf[0] == '\0') {
+				// empty lines denote tree boundaries in batch 
mode
+				if (is_batch_mode) {
+					break;
+				}
+				die("input format error: (blank line only valid 
in batch mode)");
+			}
+			mktree_line(sb.buf, sb.len, line_termination, 
allow_missing);
+		}
+		if (is_batch_mode && got_eof && used < 1) {
+			// allow input to finish with a new-line (or not)
+		} else {
+			write_tree(sha1);
+			puts(sha1_to_hex(sha1));
+			fflush(stdout);
+		}
+		used=0; // reset tree entry buffer for re-use in batch mode
+	}
 	strbuf_release(&sb);
-
-	write_tree(sha1);
-	puts(sha1_to_hex(sha1));
 	exit(0);
 }
-- 
1.6.3.165.g2cce5.dirty

[PATCH 3/3] improved validation of entry type in mktree

[Josh Micich]

Previously mktree would accept tree entries which had a mismatch between the 
declared type and the actual type of object identified by the sha.

Signed-off-by: Josh Micich <josh.micich@gmail.com>
---
 builtin-mktree.c |   34 +++++++++++++++++++++-------------
 1 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/builtin-mktree.c b/builtin-mktree.c
index a56c917..a37954a 100644
--- a/builtin-mktree.c
+++ b/builtin-mktree.c
@@ -71,7 +71,7 @@ static void mktree_line(char *buf, size_t len, int 
line_termination, int allow_m
 {
 	char *ptr, *ntr;
 	unsigned mode;
-	enum object_type type;
+	enum object_type mode_type;
 	char *path;
 	unsigned char sha1[20];
 
@@ -94,29 +94,37 @@ static void mktree_line(char *buf, size_t len, int 
line_termination, int allow_m
 	if (S_ISGITLINK(mode))
 		allow_missing = 1;
 
-	if (!allow_missing)
-		type = sha1_object_info(sha1, NULL);
-	else
-		type = object_type(mode);
-
-	if (type < 0)
-		die("object %s unavailable", sha1_to_hex(sha1));
 
 	*ntr++ = 0; /* now at the beginning of SHA1 */
-	if (type != type_from_string(ptr))
-		die("object type %s mismatch (%s)", ptr, typename(type));
 
 	path = ntr + 41;  /* at the beginning of name */
+	struct strbuf p_uq = STRBUF_INIT;
 	if (line_termination && path[0] == '"') {
-		struct strbuf p_uq = STRBUF_INIT;
 		if (unquote_c_style(&p_uq, path, NULL))
 			die("invalid quoting");
 		path = strbuf_detach(&p_uq, NULL);
 		append_to_tree(mode, sha1, path);
-		strbuf_release(&p_uq);
-		return;
 	}
+
+	mode_type = object_type(mode);
+	if (mode_type != type_from_string(ptr)) {
+		die("entry '%s' object type (%s) doesn't match mode type (%s)", 
path, ptr, typename(mode_type));
+	}
+
+	enum object_type obj_type = sha1_object_info(sha1, NULL);
+	if (obj_type < 0) {
+		if (!allow_missing) {
+			die("entry '%s' object %s is unavailable", path, 
sha1_to_hex(sha1));
+		}
+	} else {
+		if (obj_type != mode_type) {
+			die("entry '%s' object %s is a %s but specified type 
was (%s)",
+				path, sha1_to_hex(sha1), typename(obj_type), 
typename(mode_type));
+		}
+	}
+	
 	append_to_tree(mode, sha1, path);
+	strbuf_release(&p_uq); // safe to call on unused empty buffer
 }
 
 int cmd_mktree(int ac, const char **av, const char *prefix)
-- 
1.6.3.165.g2cce5.dirty

Re: [PATCH 1/3] '--missing' option for mktree: re-added strbuf_release(&p_uq), Updated man page

[Junio C Hamano]

Josh Micich <josh.micich@gmail.com> writes:

>  DESCRIPTION
>  -----------
> -Reads standard input in non-recursive `ls-tree` output format,
> -and creates a tree object.  The object name of the tree object
> +Reads standard input in non-recursive `ls-tree` output format, and creates
> +a tree object.  The order of the tree entries is normalised by mktree so
> +pre-sorting the input is not required.  The object name of the tree object
>  built is written to the standard output.

Thanks ;-)

>  OPTIONS
>  -------
>  -z::
>  	Read the NUL-terminated `ls-tree -z` output instead.
> +--missing::
> +	Allow missing objects.  The default behaviour (without this option)
> +	is to verify that each tree entry's sha1 identifies an existing
> +	object.

I'd suggest adding "... except for gitlink entries (aka "submodules") that
are always allowed to be missing" here.

> @@ -112,6 +112,9 @@ static void mktree_line(char *buf, size_t len, int 
> line_termination, int allow_m
>  		if (unquote_c_style(&p_uq, path, NULL))
>  			die("invalid quoting");
>  		path = strbuf_detach(&p_uq, NULL);
> +		append_to_tree(mode, sha1, path);
> +		strbuf_release(&p_uq);
> +		return;
>  	}
>  	append_to_tree(mode, sha1, path);
>  }

Ehh, why?  detach already detaches the allocated buffer from strbuf and
there is nothing to clean up by strbuf_release().

Re: [PATCH 2/3] added --batch option to mktree

[Junio C Hamano]

Josh Micich <josh.micich@gmail.com> writes:

> This option enables creation of many tree objects with a single process

... which is desirable in what way?  how does this justify the cost of
maintenance?  what is it used for?

>  DESCRIPTION
>  -----------
> @@ -25,6 +25,11 @@ OPTIONS
>  	Allow missing objects.  The default behaviour (without this option)
>  	is to verify that each tree entry's sha1 identifies an existing
>  	object.
> +--batch::
> +	Allow building of more than one tree object before exiting.  Each
> +	tree is separated by as single blank line. The final new-line is
> +	optional.  Note - if the '-z' option is used, lines are terminated
> +	with NUL.

I think you want an blank line before this.

>  Author
>  ------
> diff --git a/builtin-mktree.c b/builtin-mktree.c
> index db647ce..a56c917 100644
> --- a/builtin-mktree.c
> +++ b/builtin-mktree.c
> @@ -63,7 +63,7 @@ static void write_tree(unsigned char *sha1)
>  }
>  
>  static const char *mktree_usage[] = {
> -	"git mktree [-z] [--missing]",
> +	"git mktree [-z] [--missing] [--batch]",
>  	NULL
>  };
>  
> @@ -125,20 +125,42 @@ int cmd_mktree(int ac, const char **av, const char 
> *prefix)

Linewrapped and would not apply.

>  	unsigned char sha1[20];
>  	int line_termination = '\n';
>  	int allow_missing = 0;
> +	int is_batch_mode = 0;
> +
>  	const struct option option[] = {
>  		OPT_SET_INT('z', NULL, &line_termination, "input is NUL 
> terminated", '\0'),
>  		OPT_SET_INT( 0 , "missing", &allow_missing, "allow missing 
> objects", 1),
> +		OPT_SET_INT( 0 , "batch", &is_batch_mode, "interactively create 
> more than one tree", 1),

What do you mean by "interactively"?  Does anybody type from the standard
input?

>  	ac = parse_options(ac, av, option, mktree_usage, 0);
>  
> -	while (strbuf_getline(&sb, stdin, line_termination) != EOF)
> -		mktree_line(sb.buf, sb.len, line_termination, allow_missing);
> -
> +	int got_eof = 0;

Decl-after-statement. 

> +	while (!got_eof) {
> +		while (1) {
> +			if (strbuf_getline(&sb, stdin, line_termination) == 
> EOF) {
> +				got_eof = 1;
> +				break;
> +			}
> +			if (sb.buf[0] == '\0') {
> +				// empty lines denote tree boundaries in batch 
> mode

No C++ comment please;

> +				if (is_batch_mode) {
> +					break;
> +				}

Lose excess {} pair;

> +		if (is_batch_mode && got_eof && used < 1) {
> +			// allow input to finish with a new-line (or not)

Style: have an explicit ";" for an empty statement.

But more importantly, what does this comment mean?  Why do you want to be
loose in input format validation?

> +		} else {
> +			write_tree(sha1);
> +			puts(sha1_to_hex(sha1));
> +			fflush(stdout);
> +		}
> +		used=0; // reset tree entry buffer for re-use in batch mode

	used = 0; /* .... */

Re: [PATCH 3/3] improved validation of entry type in mktree

[Junio C Hamano]

Josh Micich <josh.micich@gmail.com> writes:

> diff --git a/builtin-mktree.c b/builtin-mktree.c
> index a56c917..a37954a 100644
> --- a/builtin-mktree.c
> +++ b/builtin-mktree.c
> @@ -71,7 +71,7 @@ static void mktree_line(char *buf, size_t len, int 
> line_termination, int allow_m
>  {
>  	char *ptr, *ntr;
>  	unsigned mode;
> -	enum object_type type;
> +	enum object_type mode_type;

Why?

> @@ -94,29 +94,37 @@ static void mktree_line(char *buf, size_t len, int 
> line_termination, int allow_m
>  	if (S_ISGITLINK(mode))
>  		allow_missing = 1;
>  
> -	if (!allow_missing)
> -		type = sha1_object_info(sha1, NULL);
> -	else
> -		type = object_type(mode);
> -
> -	if (type < 0)
> -		die("object %s unavailable", sha1_to_hex(sha1));
>  
>  	*ntr++ = 0; /* now at the beginning of SHA1 */
> -	if (type != type_from_string(ptr))
> -		die("object type %s mismatch (%s)", ptr, typename(type));
>  
>  	path = ntr + 41;  /* at the beginning of name */
> +	struct strbuf p_uq = STRBUF_INIT;
>  	if (line_termination && path[0] == '"') {
> -		struct strbuf p_uq = STRBUF_INIT;

Why make its lifetime longer even though you do not use it outside of this
block?

> ...
>  	}

> +
> +	mode_type = object_type(mode);
> +	if (mode_type != type_from_string(ptr)) {
> +		die("entry '%s' object type (%s) doesn't match mode type (%s)", 
> path, ptr, typename(mode_type));
> +	}
> +
> +	enum object_type obj_type = sha1_object_info(sha1, NULL);
> +	if (obj_type < 0) {
> +		if (!allow_missing) {

This is the other way around; when allow_missing is given you shouldn't
even consult the object database to read it.

Re: [PATCH 2/3] added --batch option to mktree

[Josh Micich]

On Wed, May 13, 2009 at 11:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> This option enables creation of many tree objects with a single process
>
>... which is desirable in what way?  how does this justify the cost of
>maintenance?  what is it used for?

I have been writing a few of my own utilities on top of basic git
commands and got mild performance gains from using the existing
'--batch' option with "git cat-file" instead of executing new "git
cat-file -p <sha>" processes for each object I wish to read.  Later I
was writing a utility for mirroring another SCM (creates many trees),
and I faced a similar performance issue with "git mktree".  After
adding the '--batch' option, I experienced about a 30% speed increase.

If you find this logic persuasive, I can fix all the other issues and
re-submit this patch.

>> +--batch::
>I think you want an blank line before this.
right


>> @@ -125,20 +125,42 @@ int cmd_mktree(int ac, const char **av, const char
>> *prefix)
>
>Linewrapped and would not apply.
sorry - gmane web interface did that.  BTW - does git have a coding
standard for maximum line length?


>What do you mean by "interactively"?  Does anybody type from the standard
>input?
I guess I was trying to stress the point that the created tree object
ids are output immediately as the complete text for each tree is
received.  Other (bad) applications I have seen wait for EOF on stdin
before sending anything to stdout, and this is exactly what I had
avoided in the impl.  Perhaps you could suggest better phrasing.


>Decl-after-statement.
>No C++ comment please;
>Lose excess {} pair;
>Style: have an explicit ";" for an empty statement.
>used = 0; /* .... */
sorry - bad habits. gotta work out how to get my compiler to warn me about these


>> +             if (is_batch_mode && got_eof && used < 1) {
>> +                     // allow input to finish with a new-line (or not)
>
>Style: have an explicit ";" for an empty statement.
>
>But more importantly, what does this comment mean?  Why do you want to be
>loose in input format validation?

I agree with your implied suggestion that tight input validation is
better.  I was actually trying to keep consistent with the way I
believe mktree works today.  The final \n is optional as far as I can
tell.

Re: [PATCH 2/3] added --batch option to mktree

[Sverre Rabbelier]

Heya,

On Thu, May 14, 2009 at 11:24, Josh Micich <josh.micich@gmail.com> wrote:
> If you find this logic persuasive, I can fix all the other issues and
> re-submit this patch.

I suspect Junio's point was more in the gist of "you should explain
this in your commit message". So if you do resend your patch, include
the above explanation (or at least the mention of the 30% speedup) in
your commit message.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 1/3] '--missing' option for mktree: re-added strbuf_release(&p_uq), Updated man page

[Josh Micich]

On Wed, May 13, 2009 at 11:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>               path = strbuf_detach(&p_uq, NULL);
>> +             append_to_tree(mode, sha1, path);
>> +             strbuf_release(&p_uq);
>> +             return;
>>       }
>>       append_to_tree(mode, sha1, path);
>>  }
>
> Ehh, why?  detach already detaches the allocated buffer from strbuf and
> there is nothing to clean up by strbuf_release().
>
My bad. While re-basing my work to fe0bb5, the strbuf_release() call
went missing, but I didn't notice you had also changed:
-                       path = p_uq.buf;
+               path = strbuf_detach(&p_uq, NULL);
That is much nicer

Re: [PATCH 2/3] added --batch option to mktree

[Josh Micich]

This option works in a similar way to the '--batch' option of 'git cat-file'.
It enables creation of many tree objects with a single process.

The change was motivated by performance considerations in applications that
need to create many tree objects. A non-rigorous test showed tree creation
times improved from (roughly) 200ms to 50ms.

Signed-off-by: Josh Micich <josh.micich@gmail.com>
---
 Documentation/git-mktree.txt |    8 +++++++-
 builtin-mktree.c             |   40 +++++++++++++++++++++++++++++++++-------
 2 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-mktree.txt b/Documentation/git-mktree.txt
index 7336f48..81e3326 100644
--- a/Documentation/git-mktree.txt
+++ b/Documentation/git-mktree.txt
@@ -8,7 +8,7 @@ git-mktree - Build a tree-object from ls-tree formatted text

 SYNOPSIS
 --------
-'git mktree' [-z] [--missing]
+'git mktree' [-z] [--missing] [--batch]

 DESCRIPTION
 -----------
@@ -28,6 +28,12 @@ OPTIONS
 	object.  This option has no effect on the treatment of gitlink entries
 	(aka "submodules") which are always allowed to be missing.

+--batch::
+	Allow building of more than one tree object before exiting.  Each
+	tree is separated by as single blank line. The final new-line is
+	optional.  Note - if the '-z' option is used, lines are terminated
+	with NUL.
+
 Author
 ------
 Written by Junio C Hamano <gitster@pobox.com>
diff --git a/builtin-mktree.c b/builtin-mktree.c
index 5ff0475..73b0abb 100644
--- a/builtin-mktree.c
+++ b/builtin-mktree.c
@@ -63,7 +63,7 @@ static void write_tree(unsigned char *sha1)
 }

 static const char *mktree_usage[] = {
-	"git mktree [-z] [--missing]",
+	"git mktree [-z] [--missing] [--batch]",
 	NULL
 };

@@ -122,20 +122,46 @@ int cmd_mktree(int ac, const char **av, const
char *prefix)
 	unsigned char sha1[20];
 	int line_termination = '\n';
 	int allow_missing = 0;
+	int is_batch_mode = 0;
+	int got_eof = 0;
+
 	const struct option option[] = {
 		OPT_SET_INT('z', NULL, &line_termination, "input is NUL terminated", '\0'),
 		OPT_SET_INT( 0 , "missing", &allow_missing, "allow missing objects", 1),
+		OPT_SET_INT( 0 , "batch", &is_batch_mode, "allow creation of more
than one tree", 1),
 		OPT_END()
 	};

 	ac = parse_options(ac, av, option, mktree_usage, 0);

-	while (strbuf_getline(&sb, stdin, line_termination) != EOF)
-		mktree_line(sb.buf, sb.len, line_termination, allow_missing);
-
+	while (!got_eof) {
+		while (1) {
+			if (strbuf_getline(&sb, stdin, line_termination) == EOF) {
+				got_eof = 1;
+				break;
+			}
+			if (sb.buf[0] == '\0') {
+				/* empty lines denote tree boundaries in batch mode */
+				if (is_batch_mode)
+					break;
+				die("input format error: (blank line only valid in batch mode)");
+			}
+			mktree_line(sb.buf, sb.len, line_termination, allow_missing);
+		}
+		if (is_batch_mode && got_eof && used < 1) {
+			/*
+			 * Execution gets here if the last tree entry is terminated with a
+			 * new-line.  The final new-line has been made optional to be
+			 * consistent with the original non-batch behaviour of mktree.
+			 */
+			; /* skip creating an empty tree */
+		} else {
+			write_tree(sha1);
+			puts(sha1_to_hex(sha1));
+			fflush(stdout);
+		}
+		used=0; /* reset tree entry buffer for re-use in batch mode */
+	}
 	strbuf_release(&sb);
-
-	write_tree(sha1);
-	puts(sha1_to_hex(sha1));
 	exit(0);
 }
-- 
1.6.3.1.181.gfc9b3

Re: [PATCH 3/3] improved validation of entry type in mktree

[Josh Micich]

On Wed, May 13, 2009 at 11:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> +     struct strbuf p_uq = STRBUF_INIT;
>>       if (line_termination && path[0] == '"') {
>> -             struct strbuf p_uq = STRBUF_INIT;
>
> Why make its lifetime longer even though you do not use it outside of this
> block?

Sorry, no need. Similar confusion cleared up in patch 1.

>> +	enum object_type obj_type = sha1_object_info(sha1, NULL);
>> +     if (obj_type < 0) {
>> +             if (!allow_missing) {
>
> This is the other way around; when allow_missing is given you shouldn't
> even consult the object database to read it.

I think mktree should verify the object types for objects that are available.

For example (given that 4b825d is empty tree and e69de2 is empty
blob), mktree should reject the following entries:

100644 blob 4b825dc642cb6eb9a060e54bf8d69288fbee4904	foo
040000 tree e69de29bb2d1d6434b8b29ae775ad8c2e48c5391	bar

I don't think the '--missing' option should cause mktree to accept a
tree which is clearly invalid.

Furthermore even with '--missing', a tree entry like this should be rejected:
160000 commit e69de29bb2d1d6434b8b29ae775ad8c2e48c5391  foo

Re: [PATCH 3/3] improved validation of entry type in mktree

[Josh Micich]

Previously mktree would accept tree entries which had a mismatch
between the declared type and the actual type of object identified by
the sha.

Signed-off-by: Josh Micich <josh.micich@gmail.com>
---
 builtin-mktree.c |   43 +++++++++++++++++++++++++++++++++----------
 1 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/builtin-mktree.c b/builtin-mktree.c
index 73b0abb..dc4f1a7 100644
--- a/builtin-mktree.c
+++ b/builtin-mktree.c
@@ -71,7 +71,8 @@ static void mktree_line(char *buf, size_t len, int
line_termination, int allow_m
 {
 	char *ptr, *ntr;
 	unsigned mode;
-	enum object_type type;
+	enum object_type mode_type; /* object type derived from mode */
+	enum object_type obj_type; /* object type derived from sha */
 	char *path;
 	unsigned char sha1[20];

@@ -94,17 +95,8 @@ static void mktree_line(char *buf, size_t len, int
line_termination, int allow_m
 	if (S_ISGITLINK(mode))
 		allow_missing = 1;

-	if (!allow_missing)
-		type = sha1_object_info(sha1, NULL);
-	else
-		type = object_type(mode);
-
-	if (type < 0)
-		die("object %s unavailable", sha1_to_hex(sha1));

 	*ntr++ = 0; /* now at the beginning of SHA1 */
-	if (type != type_from_string(ptr))
-		die("object type %s mismatch (%s)", ptr, typename(type));

 	path = ntr + 41;  /* at the beginning of name */
 	if (line_termination && path[0] == '"') {
@@ -113,6 +105,37 @@ static void mktree_line(char *buf, size_t len,
int line_termination, int allow_m
 			die("invalid quoting");
 		path = strbuf_detach(&p_uq, NULL);
 	}
+
+	/*
+	 * Object type is redundantly derivable three ways.
+	 * These should all agree.
+	 */
+	mode_type = object_type(mode);
+	if (mode_type != type_from_string(ptr)) {
+		die("entry '%s' object type (%s) doesn't match mode type (%s)",
+			path, ptr, typename(mode_type));
+	}
+
+	/* Check the type of object identified by sha1 */
+	obj_type = sha1_object_info(sha1, NULL);
+	if (obj_type < 0) {
+		if (allow_missing) {
+			; /* no problem - missing objects are presumed to be of the right type */
+		} else {
+			die("entry '%s' object %s is unavailable", path, sha1_to_hex(sha1));
+		}
+	} else {
+		if (obj_type != mode_type) {
+			/*
+			 * The object exists but is of the wrong type.
+			 * This is a problem regardless of allow_missing
+			 * because the new tree entry will never be correct.
+			 */
+			die("entry '%s' object %s is a %s but specified type was (%s)",
+				path, sha1_to_hex(sha1), typename(obj_type), typename(mode_type));
+		}
+	}
+
 	append_to_tree(mode, sha1, path);
 }

-- 
1.6.3.1.181.gfc9b3.dirty

Re: [PATCH 3/3] improved validation of entry type in mktree

[Jakub Narebski]

Josh Micich wrote:

> Furthermore even with '--missing', a tree entry like this should be rejected:
> 160000 commit e69de29bb2d1d6434b8b29ae775ad8c2e48c5391  foo

But with submodules you might not _have_ e69de29b in object database
to check its type!
-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH 2/3] added --batch option to mktree

[Junio C Hamano]

Josh Micich <josh.micich@gmail.com> writes:

> On Wed, May 13, 2009 at 11:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> This option enables creation of many tree objects with a single process
>>
>>... which is desirable in what way?  how does this justify the cost of
>>maintenance?  what is it used for?
>
> I have been writing a few of my own utilities on top of basic git commands
> and got mild performance gains from using the existing '--batch' option with
> "git cat-file" instead of executing new "git cat-file -p <sha>" processes
> for each object I wish to read.  Later I was writing a utility for mirroring
> another SCM (creates many trees), and I faced a similar performance issue
> with "git mktree".  After adding the '--batch' option, I experienced about a
> 30% speed increase.

"An operation that some people may want to do but the current toolset is
not very good at is _this_, and this patch rectifies that.  _Here_ is a
number to back up that claim." is a perfect way to justify your new
feature.  Why not have that in the proposed commit log message ;-)?

>>> +             if (is_batch_mode && got_eof && used < 1) {
>>> +                     // allow input to finish with a new-line (or not)
>>
>>Style: have an explicit ";" for an empty statement.
>>
>>But more importantly, what does this comment mean?  Why do you want to be
>>loose in input format validation?
>
> I agree with your implied suggestion that tight input validation is better.
> I was actually trying to keep consistent with the way I believe mktree
> works today.  The final \n is optional as far as I can tell.

Ok.

Re: [PATCH 3/3] improved validation of entry type in mktree

[Josh Micich]

Jakub Narebski <jnareb <at> gmail.com> writes:

> 
> Josh Micich wrote:
> 
> > Furthermore even with '--missing', a tree entry like this should be 
rejected:
> > 160000 commit e69de29bb2d1d6434b8b29ae775ad8c2e48c5391  foo
> 
> But with submodules you might not _have_ e69de29b in object database
> to check its type!

The code already accounted for that case.  If the tree entry is a GIT_LINK 
("160000 commit ..."), it is not mandatory for the referenced object to exist 
locally.  But if the object _does_ exist, the type should match.  I guess I 
should have prefixed that example with "assuming object e69de29b (empty tree) 
is present in the local database".