[PATCH 1/2] Teach Git how to parse standard power of 2 suffixes.

[PATCH 1/2] Teach Git how to parse standard power of 2 suffixes.

[Shawn O. Pearce]

Sometimes its necessary to supply a value as a power of two in a
configuration parameter.  In this case the user may want to use the
standard suffixes such as K, M, or G to indicate that the numerical
value should be multiplied by a constant base before being used.

Shell scripts/etc. can also benefit from this automatic option
parsing with `git repo-config --int`.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 This is a resend of the prior version of this patch.  Junio
 convinced me on #git that this version might be better.  :-)

 Meant for the top of sp/mmap, but may be useful elsewhere.

 Documentation/git-repo-config.txt |    5 ++++-
 config.c                          |    8 ++++++++
 2 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-repo-config.txt b/Documentation/git-repo-config.txt
index b379ec5..c55a8ba 100644
--- a/Documentation/git-repo-config.txt
+++ b/Documentation/git-repo-config.txt
@@ -87,7 +87,10 @@ OPTIONS
 	git-repo-config will ensure that the output is "true" or "false"
 
 --int::
-	git-repo-config will ensure that the output is a simple decimal number
+	git-repo-config will ensure that the output is a simple
+	decimal number.  An optional value suffix of 'k', 'm', or 'g'
+	in the config file will cause the value to be multiplied
+	by 1024, 1048576, or 1073741824 prior to output.
 
 
 ENVIRONMENT
diff --git a/config.c b/config.c
index 2e0d5a8..83ce9e1 100644
--- a/config.c
+++ b/config.c
@@ -236,8 +236,16 @@ int git_config_int(const char *name, const char *value)
 	if (value && *value) {
 		char *end;
 		int val = strtol(value, &end, 0);
+		while (isspace(*end))
+			end++;
 		if (!*end)
 			return val;
+		if (!strcasecmp(end, "k"))
+			return val * 1024;
+		if (!strcasecmp(end, "m"))
+			return val * 1024 * 1024;
+		if (!strcasecmp(end, "g"))
+			return val * 1024 * 1024 * 1024;
 	}
 	die("bad config value for '%s' in %s", name, config_file_name);
 }
-- 
1.5.0.rc0.g6bb1

Re: [PATCH 1/2] Teach Git how to parse standard power of 2 suffixes.

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> diff --git a/Documentation/git-repo-config.txt b/Documentation/git-repo-config.txt
> index b379ec5..c55a8ba 100644
> --- a/Documentation/git-repo-config.txt
> +++ b/Documentation/git-repo-config.txt
> @@ -87,7 +87,10 @@ OPTIONS
>  	git-repo-config will ensure that the output is "true" or "false"
>  
>  --int::
> -	git-repo-config will ensure that the output is a simple decimal number
> +	git-repo-config will ensure that the output is a simple
> +	decimal number.  An optional value suffix of 'k', 'm', or 'g'
> +	in the config file will cause the value to be multiplied
> +	by 1024, 1048576, or 1073741824 prior to output.

Thanks.  I think this is applicable on top of 'master'.

>  ENVIRONMENT
> diff --git a/config.c b/config.c
> index 2e0d5a8..83ce9e1 100644
> --- a/config.c
> +++ b/config.c
> @@ -236,8 +236,16 @@ int git_config_int(const char *name, const char *value)
>  	if (value && *value) {
>  		char *end;
>  		int val = strtol(value, &end, 0);
> +		while (isspace(*end))
> +			end++;

Why?  Are you allowing "1024 k"?  Do we want to?

>  		if (!*end)
>  			return val;
> +		if (!strcasecmp(end, "k"))
> +			return val * 1024;
> +		if (!strcasecmp(end, "m"))
> +			return val * 1024 * 1024;
> +		if (!strcasecmp(end, "g"))
> +			return val * 1024 * 1024 * 1024;
>  	}
>  	die("bad config value for '%s' in %s", name, config_file_name);
>  }
> -- 
> 1.5.0.rc0.g6bb1

Re: [PATCH 1/2] Teach Git how to parse standard power of 2 suffixes.

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> > diff --git a/config.c b/config.c
> > index 2e0d5a8..83ce9e1 100644
> > --- a/config.c
> > +++ b/config.c
> > @@ -236,8 +236,16 @@ int git_config_int(const char *name, const char *value)
> >  	if (value && *value) {
> >  		char *end;
> >  		int val = strtol(value, &end, 0);
> > +		while (isspace(*end))
> > +			end++;
> 
> Why?  Are you allowing "1024 k"?  Do we want to?

Yes.  Why not?

-- 
Shawn.

Re: [PATCH 1/2] Teach Git how to parse standard power of 2 suffixes.

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Junio C Hamano <junkio@cox.net> wrote:
>
>> Why?  Are you allowing "1024 k"?  Do we want to?
>
> Yes.  Why not?

Stricter input checking.  Allowing extra things later is far
easier than later finding problems with a looser way we started
from and having to tighten it.

Re: [PATCH 1/2] Teach Git how to parse standard power of 2 suffixes.

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > Junio C Hamano <junkio@cox.net> wrote:
> >
> >> Why?  Are you allowing "1024 k"?  Do we want to?
> >
> > Yes.  Why not?
> 
> Stricter input checking.  Allowing extra things later is far
> easier than later finding problems with a looser way we started
> from and having to tighten it.

Good point.  I'm currently writing tests for this btw.

-- 
Shawn.

Re: [PATCH 1/2] Teach Git how to parse standard power of 2 suffixes.

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Junio C Hamano <junkio@cox.net> wrote:
>> "Shawn O. Pearce" <spearce@spearce.org> writes:
>> 
>> > Junio C Hamano <junkio@cox.net> wrote:
>> >
>> >> Why?  Are you allowing "1024 k"?  Do we want to?
>> >
>> > Yes.  Why not?
>> 
>> Stricter input checking.  Allowing extra things later is far
>> easier than later finding problems with a looser way we started
>> from and having to tighten it.
>
> Good point.  I'm currently writing tests for this btw.

I have this already in the private 'master' I am prepareing for
pushout:

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index e48a4ec..a29caa0 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -391,5 +391,15 @@ EOF
 
 test_expect_success "rename succeeded" "diff -u expect .git/config"
 
+test_expect_success numbers '
+
+	git-repo-config kilo.gram 1k &&
+	git-repo-config mega.ton 1m &&
+	k=$(git-repo-config --int --get kilo.gram) &&
+	test z1024 = "z$k" &&
+	m=$(git-repo-config --int --get mega.ton) &&
+	test z1048576 = "z$m"
+'
+
 test_done
 
-- 
1.5.0.rc0.g81760

Re: [PATCH 1/2] Teach Git how to parse standard power of 2 suffixes.

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> I have this already in the private 'master' I am prepareing for
> pushout:

Does it pass?  I can't get something similar to work here...
I must be a moron or something.
 

-- 
Shawn.

Re: [PATCH 1/2] Teach Git how to parse standard power of 2 suffixes.

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> +test_expect_success numbers '
> +
> +	git-repo-config kilo.gram 1k &&
> +	git-repo-config mega.ton 1m &&
> +	k=$(git-repo-config --int --get kilo.gram) &&
> +	test z1024 = "z$k" &&
> +	m=$(git-repo-config --int --get mega.ton) &&
> +	test z1048576 = "z$m"
> +'
> +

I'm a moron.  I copied and pasted test_expect_failure here in my
own version.  Commit and push yours, its just as good as mine,
but is shorter.  :-)

-- 
Shawn.

Re: [PATCH 1/2] Teach Git how to parse standard power of 2 suffixes.

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> Shawn Pearce <spearce@spearce.org> writes:
> 
> > So in your test case where you thought you entered 20 pages, what you
> > really had was just 2 pages.  We rounded 20 bytes down to 0 pages,
> > then forced that to a minimum of 2 pages.  :-)
> 
> Yes.
> 
> And I found something else that is interesting.  I still had 2
> pages windows but reduced packedgitlimit to 256kB and re-run the
> same test to reproduce all merges in git.git.  It still did not
> fail, but it took even shorter wallclock time (again, it is only
> 3 seconds or so out of 5 1/2 minuts, so it is probably
> statistically insignificant).

Its possible the packed_git.windows list is hurting us here.
Its O(n) to locate a given window.  The larger the ratio between
packedGitWindowSize and packedGitLimit the longer that list can get,
and the longer it can take to locate a window.

Maybe that should be a simple red-black tree?

More testing probably needs to be done though.

-- 
Shawn.

Re: [PATCH 1/2] Teach Git how to parse standard power of 2 suffixes.

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

> So in your test case where you thought you entered 20 pages, what you
> really had was just 2 pages.  We rounded 20 bytes down to 0 pages,
> then forced that to a minimum of 2 pages.  :-)

Yes.

And I found something else that is interesting.  I still had 2
pages windows but reduced packedgitlimit to 256kB and re-run the
same test to reproduce all merges in git.git.  It still did not
fail, but it took even shorter wallclock time (again, it is only
3 seconds or so out of 5 1/2 minuts, so it is probably
statistically insignificant).

Re: [PATCH 1/2] Teach Git how to parse standard power of 2 suffixes.

[Shawn Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> Shawn Pearce <spearce@spearce.org> writes:
> 
> >> Also can we fix the definition of core.packedGitWindowSize to be
> >> independent of the page size on a particular platform?
> >
> > What do you mean?  mmap() only works in page units, and because of
> > the way the code is built the minimum size we can allow is 2 pages.
> >
> > Asking mmap() to map less than a full page in the last page of
> > a given window is silly, as that is just going to waste virtual
> > address space or cause pain for the OS, depending on how that
> > gets implemented.
> 
> Ah, I misread the code.  I thought the value is integer that
> states number of pages, but you are only rounding it down and
> enforcing two-page minimum.

So in your test case where you thought you entered 20 pages, what you
really had was just 2 pages.  We rounded 20 bytes down to 0 pages,
then forced that to a minimum of 2 pages.  :-)

-- 
Shawn.

Re: [PATCH 1/2] Teach Git how to parse standard power of 2 suffixes.

[Junio C Hamano]

Shawn Pearce <spearce@spearce.org> writes:

>> Also can we fix the definition of core.packedGitWindowSize to be
>> independent of the page size on a particular platform?
>
> What do you mean?  mmap() only works in page units, and because of
> the way the code is built the minimum size we can allow is 2 pages.
>
> Asking mmap() to map less than a full page in the last page of
> a given window is silly, as that is just going to waste virtual
> address space or cause pain for the OS, depending on how that
> gets implemented.

Ah, I misread the code.  I thought the value is integer that
states number of pages, but you are only rounding it down and
enforcing two-page minimum.

Sorry for the noise.

Re: [PATCH 1/2] Teach Git how to parse standard power of 2 suffixes.

[Shawn Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > Sometimes its necessary to supply a value as a power of two in a
> > configuration parameter.  In this case the user may want to use
> > the standard suffixes such as K, KB, KiB, etc. to indicate that
> > the numerical value should be multiplied by a constant base before
> > being used.
> 
> If you are really going to do this, I think we would need
> something similar to --bool to repo-config command.

That shouldn't be difficult.
 
> Also can we fix the definition of core.packedGitWindowSize to be
> independent of the page size on a particular platform?

What do you mean?  mmap() only works in page units, and because of
the way the code is built the minimum size we can allow is 2 pages.

Asking mmap() to map less than a full page in the last page of
a given window is silly, as that is just going to waste virtual
address space or cause pain for the OS, depending on how that
gets implemented.

-- 
Shawn.

Re: [PATCH 1/2] Teach Git how to parse standard power of 2 suffixes.

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Sometimes its necessary to supply a value as a power of two in a
> configuration parameter.  In this case the user may want to use
> the standard suffixes such as K, KB, KiB, etc. to indicate that
> the numerical value should be multiplied by a constant base before
> being used.

If you are really going to do this, I think we would need
something similar to --bool to repo-config command.

Also can we fix the definition of core.packedGitWindowSize to be
independent of the page size on a particular platform?

[PATCH 1/2] Teach Git how to parse standard power of 2 suffixes.

[Shawn O. Pearce]

Sometimes its necessary to supply a value as a power of two in a
configuration parameter.  In this case the user may want to use
the standard suffixes such as K, KB, KiB, etc. to indicate that
the numerical value should be multiplied by a constant base before
being used.

The new git_config_datasize function can be used in config file
handler functions to obtain a size_t in bytes.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---

 Applies on top of sp/mmap, but was broken out to make it easier
 to apply earlier in case someone else needed this function.

 cache.h  |    1 +
 config.c |   25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/cache.h b/cache.h
index a5fc232..abbcab3 100644
--- a/cache.h
+++ b/cache.h
@@ -418,6 +418,7 @@ extern int git_config_from_file(config_fn_t fn, const char *);
 extern int git_config(config_fn_t fn);
 extern int git_config_int(const char *, const char *);
 extern int git_config_bool(const char *, const char *);
+extern size_t git_config_datasize(const char *, const char *);
 extern int git_config_set(const char *, const char *);
 extern int git_config_set_multivar(const char *, const char *, const char *, int);
 extern int git_config_rename_section(const char *, const char *);
diff --git a/config.c b/config.c
index 2e0d5a8..07ad2f1 100644
--- a/config.c
+++ b/config.c
@@ -255,6 +255,31 @@ int git_config_bool(const char *name, const char *value)
 	return git_config_int(name, value) != 0;
 }
 
+size_t git_config_datasize(const char *name, const char *value)
+{
+	if (value && *value) {
+		char *end;
+		unsigned long val = strtoul(value, &end, 0);
+		while (isspace(*end))
+			end++;
+		if (!*end)
+			return val;
+		if (!strcasecmp(end, "k")
+			|| !strcasecmp(end, "kb")
+			|| !strcasecmp(end, "kib"))
+			return val * 1024;
+		if (!strcasecmp(end, "m")
+			|| !strcasecmp(end, "mb")
+			|| !strcasecmp(end, "mib"))
+			return val * 1024 * 1024;
+		if (!strcasecmp(end, "g")
+			|| !strcasecmp(end, "gb")
+			|| !strcasecmp(end, "gib"))
+			return val * 1024 * 1024 * 1024;
+	}
+	die("bad config value for '%s' in %s", name, config_file_name);
+}
+
 int git_default_config(const char *var, const char *value)
 {
 	/* This needs a better name */
-- 
1.5.0.rc0.g6bb1