Re: [PATCH] gc: remove redundant check for gc_auto_threshold

[Brandon Casey]

On Wed, Oct 10, 2018 at 4:38 PM Junio C Hamano  wrote:
>
> Brandon Casey  writes:
>
> > ...  Again, I don't feel strongly about it, but I'm not
> > sure this change actually improves the code.
>
> Yeah, in the context of the current caller, this is a safe change
> that does not break anybody and reduces the number of instructions
> executed in this codepath.  A mistaken caller may be added in the
> future that fails to check auto-threashold beforehand, but that
> won't lead to anything bad like looping for a large number of times,
> so as long as the API contract into this helper function is clear
> that callers are responsible to check beforehand, it is still not
> too bad.
>
> So, I'd throw this into "Meh - I won't regret applying it, but it is
> not the end of the world if I forget to apply it, either" pile.
>
> I _think_ a change that actually improves the code would be to
> restructure so that there is a helper that is responsible for
> guestimating the number of loose objects, and another that uses the
> helper to see if there are too many loose objects.  The latter is
> the only one tha needs to know about auto-threashold.  But we are
> not in immdiate need for such a clean-up, I guess, unless somebody
> is actively looking into revamping how auto-gc works and doing a
> preparatory clean-up.

Agreed on all points, and as usual, said better than I could :-)

-Brandon

Re: [PATCH] gc: remove redundant check for gc_auto_threshold

[Brandon Casey]

On Wed, Oct 10, 2018 at 12:32 PM Ævar Arnfjörð Bjarmason
 wrote:
>
> Checking gc_auto_threshold in too_many_loose_objects() was added in
> 17815501a8 ("git-gc --auto: run "repack -A -d -l" as necessary.",
> 2007-09-17) when need_to_gc() itself was also reliant on
> gc_auto_pack_limit before its early return:
>
> gc_auto_threshold <= 0 && gc_auto_pack_limit <= 0
>
> When that check was simplified to just checking "gc_auto_threshold <=
> 0" in b14d255ba8 ("builtin-gc.c: allow disabling all auto-gc'ing by
> assigning 0 to gc.auto", 2008-03-19) this unreachable code should have
> been removed. We only call too_many_loose_objects() from within
> need_to_gc() itself, which will return if this condition holds, and in
> cmd_gc() which will return before ever getting to "auto_gc &&
> too_many_loose_objects()" if "auto_gc && !need_to_gc()" is true
> earlier in the function.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>
> I had this in my tree as part of some general gc cleanups I was
> working on, but since it's trivially considered as a stand-alone topic
> and unlikely to conflict with anything I or anyone else has planned
> I'm sending it as a one-off.

Hmm, yeah you're right that the check seems to be redundant for the
current uses of too_many_loose_objects().  I don't feel strongly about
it, but I think an argument could be made that it makes sense for
too_many_loose_object() and too_many_packs() to each inspect the
configuration variable that controls them and detect when they're
disabled, rather than having one of them require that the check be
done beforehand.  Again, I don't feel strongly about it, but I'm not
sure this change actually improves the code.

-Brandon

Re: Shawn Pearce has died

[Brandon Casey]

On Mon, Jan 29, 2018 at 2:55 PM, Christian Couder
 wrote:
> On Mon, Jan 29, 2018 at 6:21 PM, Jeff King  wrote:
>> On Mon, Jan 29, 2018 at 10:33:08AM +0100, Johannes Schindelin wrote:
>>
>>> I found these sad news in my timeline today:
>>>
>>> https://twitter.com/cdibona/status/957822400518696960
>>
>> Thanks for posting this.
>
> Yeah, thanks.

This is truly sad news.  Just wanted to voice my surprise, sadness,
and sympathy.

-Brandon

[PATCH 2/3] parse-options: write blank line to correct output stream

[Brandon Casey]

When commit 54e6dc7 added translation support to parse-options, an
fprintf was mistakenly replaced by a call to putchar().  Let's use fputc
instead.

Fixes t0040.11, t0040.12, t0040.33, and t1502.8.

Signed-off-by: Brandon Casey <draf...@gmail.com>
---
 parse-options.c   | 2 +-
 t/t0040-parse-options.sh  | 6 +++---
 t/t1502-rev-parse-parseopt.sh | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 0dd9fc6..6a03a52 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -599,7 +599,7 @@ static int usage_with_options_internal(struct 
parse_opt_ctx_t *ctx,
if (**usagestr)
fprintf_ln(outfile, _("%s"), _(*usagestr));
else
-   putchar('\n');
+   fputc('\n', outfile);
usagestr++;
}
 
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index a36434b..0c2fc81 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -92,8 +92,8 @@ test_expect_success 'OPT_BOOL() is idempotent #2' 'check 
boolean: 1 -DB'
 test_expect_success 'OPT_BOOL() negation #1' 'check boolean: 0 -D --no-yes'
 test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D 
--no-no-doubt'
 
-test_expect_failure 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'
-test_expect_failure 'OPT_BOOL() no negation #2' 'check_unknown_i18n 
--no-no-fear'
+test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'
+test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n 
--no-no-fear'
 
 test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'
 
@@ -288,7 +288,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '
 
 >expect
 
-test_expect_failure 'OPT_CALLBACK() and callback errors work' '
+test_expect_success 'OPT_CALLBACK() and callback errors work' '
test_must_fail test-parse-options --no-length >output 2>output.err &&
test_i18ncmp expect output &&
test_i18ncmp expect.err output.err
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 1bfa80f..ce7dda1 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -139,7 +139,7 @@ END_EXPECT
test_i18ncmp expect output
 '
 
-test_expect_failure 'test --parseopt invalid switch help output' '
+test_expect_success 'test --parseopt invalid switch help output' '
sed -e "s/^|//" >expect <<\END_EXPECT &&
 |error: unknown option `does-not-exist'\''
 |usage: some-command [options] ...
-- 
2.2.0.rc3

[PATCH 3/3] parse-options: only insert newline in help text if needed

[Brandon Casey]

Currently, when parse_options() produces a help message it always emits
a blank line after the usage text to separate it from the options text.
If the option spec does not define any switches, or only defines hidden
switches that will not be displayed, then the help text will end up with
two trailing blank lines instead of one.  Let's defer emitting the blank
line between the usage text and the options text until it is clear that
the options section will not be empty.

Fixes t1502.5, t1502.6.

Signed-off-by: Brandon Casey <draf...@gmail.com>
---
 parse-options.c   | 10 --
 t/t1502-rev-parse-parseopt.sh |  4 ++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 6a03a52..fca7159 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -581,6 +581,7 @@ static int usage_with_options_internal(struct 
parse_opt_ctx_t *ctx,
   const struct option *opts, int full, int 
err)
 {
FILE *outfile = err ? stderr : stdout;
+   int need_newline;
 
if (!usagestr)
return PARSE_OPT_HELP;
@@ -603,8 +604,7 @@ static int usage_with_options_internal(struct 
parse_opt_ctx_t *ctx,
usagestr++;
}
 
-   if (opts->type != OPTION_GROUP)
-   fputc('\n', outfile);
+   need_newline = 1;
 
for (; opts->type != OPTION_END; opts++) {
size_t pos;
@@ -612,6 +612,7 @@ static int usage_with_options_internal(struct 
parse_opt_ctx_t *ctx,
 
if (opts->type == OPTION_GROUP) {
fputc('\n', outfile);
+   need_newline = 0;
if (*opts->help)
fprintf(outfile, "%s\n", _(opts->help));
continue;
@@ -619,6 +620,11 @@ static int usage_with_options_internal(struct 
parse_opt_ctx_t *ctx,
if (!full && (opts->flags & PARSE_OPT_HIDDEN))
continue;
 
+   if (need_newline) {
+   fputc('\n', outfile);
+   need_newline = 0;
+   }
+
pos = fprintf(outfile, "");
if (opts->short_name) {
if (opts->flags & PARSE_OPT_NODASH)
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index ce7dda1..a859abe 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -98,7 +98,7 @@ END_EXPECT
test_i18ncmp expect output
 '
 
-test_expect_failure 'test --parseopt help output no switches' '
+test_expect_success 'test --parseopt help output no switches' '
sed -e "s/^|//" >expect <<\END_EXPECT &&
 |cat <<\EOF
 |usage: some-command [options] ...
@@ -111,7 +111,7 @@ END_EXPECT
test_i18ncmp expect output
 '
 
-test_expect_failure 'test --parseopt help output hidden switches' '
+test_expect_success 'test --parseopt help output hidden switches' '
sed -e "s/^|//" >expect <<\END_EXPECT &&
 |cat <<\EOF
 |usage: some-command [options] ...
-- 
2.2.0.rc3

[PATCH 1/3] t0040,t1502: Demonstrate parse_options bugs

[Brandon Casey]

When the option spec contains no switches or only hidden switches,
parse_options will emit an extra blank line at the end of help output so
that the help text will end in two blank lines instead of one.

When parse_options produces internal help output after an error has
occurred it will emit blank lines within the usage string to stdout
instead of stderr.

Update t/helper/test-parse-options.c to have a description body in the
usage string to exercise this second bug and mark tests as failing in
t0040.

Add tests to t1502 to demonstrate both of these problems.

Signed-off-by: Brandon Casey <draf...@gmail.com>
---

Notes:
FYI: this is built on top of bc/rev-parse-parseopt-fix (697bc88) merged
into next.

 t/helper/test-parse-options.c |   2 +
 t/t0040-parse-options.sh  |   8 ++--
 t/t1502-rev-parse-parseopt.sh | 100 ++
 3 files changed, 107 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index 75fe883..630c76d 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -99,6 +99,8 @@ int cmd_main(int argc, const char **argv)
const char *prefix = "prefix/";
const char *usage[] = {
"test-parse-options ",
+   "",
+   "A helper function for the parse-options API.",
NULL
};
struct string_list expect = STRING_LIST_INIT_NODUP;
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 74d2cd7..a36434b 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -10,6 +10,8 @@ test_description='our own option parser'
 cat >expect <<\EOF
 usage: test-parse-options 
 
+A helper function for the parse-options API.
+
 --yes get a boolean
 -D, --no-doubtbegins with 'no-'
 -B, --no-fear be brave
@@ -90,8 +92,8 @@ test_expect_success 'OPT_BOOL() is idempotent #2' 'check 
boolean: 1 -DB'
 test_expect_success 'OPT_BOOL() negation #1' 'check boolean: 0 -D --no-yes'
 test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D 
--no-no-doubt'
 
-test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'
-test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n 
--no-no-fear'
+test_expect_failure 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'
+test_expect_failure 'OPT_BOOL() no negation #2' 'check_unknown_i18n 
--no-no-fear'
 
 test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'
 
@@ -286,7 +288,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '
 
 >expect
 
-test_expect_success 'OPT_CALLBACK() and callback errors work' '
+test_expect_failure 'OPT_CALLBACK() and callback errors work' '
test_must_fail test-parse-options --no-length >output 2>output.err &&
test_i18ncmp expect output &&
test_i18ncmp expect.err output.err
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 6e1b45f..1bfa80f 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -38,6 +38,25 @@ test_expect_success 'setup optionspec' '
 EOF
 '
 
+test_expect_success 'setup optionspec-no-switches' '
+   sed -e "s/^|//" >optionspec_no_switches <<\EOF
+|some-command [options] ...
+|
+|some-command does foo and bar!
+|--
+EOF
+'
+
+test_expect_success 'setup optionspec-only-hidden-switches' '
+   sed -e "s/^|//" >optionspec_only_hidden_switches <<\EOF
+|some-command [options] ...
+|
+|some-command does foo and bar!
+|--
+|hidden1* A hidden switch
+EOF
+'
+
 test_expect_success 'test --parseopt help output' '
sed -e "s/^|//" >expect <<\END_EXPECT &&
 |cat <<\EOF
@@ -79,6 +98,87 @@ END_EXPECT
test_i18ncmp expect output
 '
 
+test_expect_failure 'test --parseopt help output no switches' '
+   sed -e "s/^|//" >expect <<\END_EXPECT &&
+|cat <<\EOF
+|usage: some-command [options] ...
+|
+|some-command does foo and bar!
+|
+|EOF
+END_EXPECT
+   test_expect_code 129 git rev-parse --parseopt -- -h > output < 
optionspec_no_switches &&
+   test_i18ncmp expect output
+'
+
+test_expect_failure 'test --parseopt help output hidden switches' '
+   sed -e "s/^|//" >expect <<\END_EXPECT &&
+|cat <<\EOF
+|usage: some-command [options] ...
+|
+|some-command does foo and bar!
+|
+|EOF
+END_EXPECT
+   test_expect_code 129 git rev-parse --parseopt -- -h > output < 
optionspec_only_hidden_switches &&
+   test_i18ncmp expect output
+'
+
+test_expect_success 'test --parseopt help-all output hidden switches' '
+   sed -e "s/^|//" >expect <<\END_EXPECT &&
+|cat <<\EOF
+|usage: some-command [options] ...
+|
+|so

[PATCH 1/4] t1502: demonstrate rev-parse --parseopt option mis-parsing

[Brandon Casey]

Since commit 2d893df rev-parse will scan forward from the beginning of
the option string looking for a flag character.  If there are no flag
characters then the scan will spill over into the help text and will
interpret the characters preceding the "flag" as part of the option-spec
i.e. the long option name.

For example, the following option spec:

exclame this does something!

will produce this 'set' expression when --exclame is specified:

set -- --exclame this does something --

which will be interpreted as four separate parameters by the shell.  And
will produce a help string that looks like:

--exclame this does something
  this does something!

git-rebase.sh has such an option (--autosquash), and so will add extra
parameters to the 'set' expression when --autosquash is used.
git-rebase continues to work correctly though because when it parses the
arguments, it ignores ones that it does not recognize.

Also, rev-parse --parseopt does not currently interpret a tab character
as a delimiter between the option spec and the help text.  If a tab is
used at the end of the option spec, before the help text, and before a
space has been specified, then rev-parse will interpret the tab as part
of the preceding component (either the long name or the arg hint).

For example, the following option spec (note: there is a  between
"frotz" and "enable"):

frotz   enable frotzing

will produce this 'set' expression when --frotz is specified:

set -- --frotz  enable --

which will be interpreted as 2 separate arguments by the shell.

git-rebase.sh has one of these too (--keep-empty).  In this case the tab
is immediately followed by spaces so there are no additional parameters
produced on the command line.  The only side-effect is misalignment in
the help text.

Signed-off-by: Brandon Casey <draf...@gmail.com>
---
 t/t1502-rev-parse-parseopt.sh | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 310f93f..910fc56 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -28,6 +28,9 @@ test_expect_success 'setup optionspec' '
 |g,fluf?path short and long option optional argument
 |longest=very-long-argument-hint  a very long argument hint
 |pair=key=value  with an equals sign in the hint
+|aswitch help te=t contains? fl*g characters!`
+|bswitch=hint   hint has trailing tab character
+|cswitchswitch has trailing tab character
 |short-hint=awith a one symbol hint
 |
 |Extras
@@ -35,7 +38,7 @@ test_expect_success 'setup optionspec' '
 EOF
 '
 
-test_expect_success 'test --parseopt help output' '
+test_expect_failure 'test --parseopt help output' '
sed -e "s/^|//" >expect <<\END_EXPECT &&
 |cat <<\EOF
 |usage: some-command [options] ...
@@ -62,6 +65,9 @@ test_expect_success 'test --parseopt help output' '
 |--longest 
 |  a very long argument hint
 |--pair 

[PATCH 2/4] rev-parse parseopt: do not search help text for flag chars

[Brandon Casey]

When searching for flag characters in the option spec, we should ensure
the search stays within the bounds of the option spec and does not enter
the help text portion of the spec.  So when we find the boundary white
space marking the start of the help text, let's mark it with a nul
character.  Then when we search for flag characters starting from the
beginning of the string we'll stop at the nul and won't enter the help
text.

Now, the following option spec:

exclame this does something!

will produce this 'set' expression when --exclame is specified:

set -- --exclame --

instead of this one:

set -- --exclame this does something --

Mark t1502.4 and t1502.5 as fixed.

Signed-off-by: Brandon Casey <draf...@gmail.com>
---
 builtin/rev-parse.c   | 6 --
 t/t1502-rev-parse-parseopt.sh | 4 ++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 2bd28d3..b19f677 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -434,7 +434,7 @@ static int cmd_parseopt(int argc, const char **argv, const 
char *prefix)
/* parse: (|,|)[*=?!]*? SP+  */
while (strbuf_getline(, stdin) != EOF) {
const char *s;
-   const char *help;
+   char *help;
struct option *o;
 
if (!sb.len)
@@ -451,8 +451,10 @@ static int cmd_parseopt(int argc, const char **argv, const 
char *prefix)
continue;
}
 
+   *help = '\0';
+
o->type = OPTION_CALLBACK;
-   o->help = xstrdup(skipspaces(help));
+   o->help = xstrdup(skipspaces(help+1));
o->value = 
o->flags = PARSE_OPT_NOARG;
o->callback = _dump;
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 910fc56..3d895e0 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -85,12 +85,12 @@ set -- --foo --bar 'ham' -b --aswitch -- 'arg'
 EOF
 "
 
-test_expect_failure 'test --parseopt' '
+test_expect_success 'test --parseopt' '
git rev-parse --parseopt -- --foo --bar=ham --baz --aswitch arg < 
optionspec > output &&
test_cmp expect output
 '
 
-test_expect_failure 'test --parseopt with mixed options and arguments' '
+test_expect_success 'test --parseopt with mixed options and arguments' '
git rev-parse --parseopt -- --foo arg --bar=ham --baz --aswitch < 
optionspec > output &&
test_cmp expect output
 '
-- 
2.2.0.rc3

[PATCH 3/4] rev-parse parseopt: interpret any whitespace as start of help text

[Brandon Casey]

Currently, rev-parse only interprets a space ' ' character as the
delimiter between the option spec and the help text.  So if a tab
character is placed between the option spec and the help text, it will
be interpreted as part of the long option name or as part of the arg
hint.  If it is interpreted as part of the long option name, then
rev-parse will produce what will be interpreted as multiple arguments
on the command line.

For example, the following option spec (note: there is a  between
"frotz" and "enable"):

frotz   enable frotzing

will produce the following set expression when --frotz is used:

set -- --frotz --

instead of this:

set -- --frotz  enable --

Mark t1502.2 as fixed.

Signed-off-by: Brandon Casey <draf...@gmail.com>
---
 builtin/rev-parse.c   | 12 ++--
 t/t1502-rev-parse-parseopt.sh |  2 +-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index b19f677..351b1a3 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -387,6 +387,14 @@ static const char *skipspaces(const char *s)
return s;
 }
 
+static char *findspace(const char *s)
+{
+   for (; *s; s++)
+   if (isspace(*s))
+   return (char*)s;
+   return NULL;
+}
+
 static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 {
static int keep_dashdash = 0, stop_at_non_option = 0;
@@ -444,8 +452,8 @@ static int cmd_parseopt(int argc, const char **argv, const 
char *prefix)
memset(opts + onb, 0, sizeof(opts[onb]));
 
o = [onb++];
-   help = strchr(sb.buf, ' ');
-   if (!help || *sb.buf == ' ') {
+   help = findspace(sb.buf);
+   if (!help || sb.buf == help) {
o->type = OPTION_GROUP;
o->help = xstrdup(skipspaces(sb.buf));
continue;
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 3d895e0..6e1b45f 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -38,7 +38,7 @@ test_expect_success 'setup optionspec' '
 EOF
 '
 
-test_expect_failure 'test --parseopt help output' '
+test_expect_success 'test --parseopt help output' '
sed -e "s/^|//" >expect <<\END_EXPECT &&
 |cat <<\EOF
 |usage: some-command [options] ...
-- 
2.2.0.rc3

[PATCH 4/4] git-rebase: don't ignore unexpected command line arguments

[Brandon Casey]

Currently, git-rebase will silently ignore any unexpected command-line
switches and arguments (the command-line produced by git rev-parse).
This allowed the rev-parse bug, fixed in the preceding commits, to go
unnoticed.  Let's make sure that doesn't happen again.  We shouldn't be
ignoring unexpected arguments.  Let's not.

Signed-off-by: Brandon Casey <draf...@gmail.com>
---
 git-rebase.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/git-rebase.sh b/git-rebase.sh
index ad8415e..6344e8d 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -350,6 +350,9 @@ do
shift
break
;;
+   *)
+   usage
+   ;;
esac
shift
 done
-- 
2.2.0.rc3

Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf

[Brandon Casey]

On Thu, Aug 24, 2017 at 9:52 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Brandon Casey <draf...@gmail.com> writes:
>
>> Ah, you probably meant something like this:
>>
>>const char strbuf_slopbuf = '\0';
>>
>> which gcc will apparently place in the read-only segment.  I did not know 
>> that.
>
> Yes but I highly suspect that it would be very compiler dependent
> and not something the language lawyers would recommend us to rely
> on.

I think compilers may have the option of placing variables that are
explicitly initialized to zero in the bss segment too, in addition to
those that are not explicitly initialized.  So I agree that no one
should write code that depends on their variables being placed in one
segment or the other, but I could see someone using this behavior as
an additional safety check; kind of a free assert, aside from the
additional space in the .rodata segment.

> My response was primarily to answer "why?" with "because we did not
> bother".  The above is a mere tangent, i.e. "multiple copies of
> empty strings is a horrible implementation (and there would be a way
> to do it with a single instance)".

Merely adding const to our current strbuf_slopbuf declaration does not
buy us anything since it will be allocated in r/w memory.  i.e. it
would still be possible to modify it via the buf member of strbuf.  So
you can't just do this:

   const char strbuf_slopbuf[1];

That's pretty much equivalent to what we currently have since it only
restricts modifying the contents of strbuf_slopbuf directly through
the strbuf_slopbuf variable, but it does not restrict modifying it
through a pointer to that object.

Until yesterday, I was under the impression that the only way to
access data in the rodata segment was through a constant literal.  So
my initial thought was that we could do something like:

   const char * const strbuf_slopbuf = "";

..but that variable cannot be used in a static assignment like:

   struct strbuf foo = {0, 0, (char*) strbuf_slopbuf};

So it seemed like our only option was to use a literal "" everywhere
instead of a slopbuf variable _if_ we wanted to have the guarantee
that our "slopbuf" could not be modified.

But what I learned yesterday, is that at least gcc/clang will place
the entire variable in the rodata segment if the variable is both
marked const _and_ initialized.

i.e. this will be allocated in the .rodata segment:

   const char strbuf_slopbuf[1] = "";

>>#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = (char*) 
>> _slopbuf }
>>
>> respectively.  Yeah, that's definitely preferable to a macro.
>> Something similar could be done in object.c.
>
> What is the main objective for doing this change?  The "make sure we
> do not write into that slopbuf" assert() bothers you and you want to
> replace it with an address in the read-only segment?

Actually nothing about the patch bothers me.  The point of that patch
is to make sure we don't accidentally modify the slopbuf.  I was just
looking for a way for the compiler to help out and wondering if there
was a reason we didn't attempt to do so in the first place.

I think the main takeaway here is that I learned something yesterday
:-)  I didn't actually intend to submit a patch for any of this, but
if anything useful came out of the discussion I thought Martin may
incorporate it into his patch if he wanted to.

-Brandon

Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf

[Brandon Casey]

On Wed, Aug 23, 2017 at 3:24 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Brandon Casey <draf...@gmail.com> writes:
>
>> On Wed, Aug 23, 2017 at 2:04 PM, Junio C Hamano <gits...@pobox.com> wrote:
>>> Brandon Casey <draf...@gmail.com> writes:
>>>
>>>> So is there any reason why didn't do something like the following in
>>>> the first place?
>>>
>>> My guess is that we didn't bother; if we cared, we would have used a
>>> single instance of const char in a read-only segment, instead of
>>> such a macro.
>>
>> I think you mean something like this:
>>
>>const char * const strbuf_slopbuf = "";
>
> Rather, more like "const char slopbuf[1];"

You'd think that would be sufficient right?  Apparently gcc and clang
will only place a variable marked as const in the read-only section if
you initialize it with a constant too.  (gcc 4.9.2, clang 3.8.1 on
linux, clang 8.1.0 on osx)

I might have to adjust my stance on initializing global variables
moving forward :-)

-Brandon

Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf

[Brandon Casey]

On Wed, Aug 23, 2017 at 2:54 PM, Brandon Casey <draf...@gmail.com> wrote:
> On Wed, Aug 23, 2017 at 2:20 PM, Brandon Casey <draf...@gmail.com> wrote:
>> On Wed, Aug 23, 2017 at 2:04 PM, Junio C Hamano <gits...@pobox.com> wrote:
>>> Brandon Casey <draf...@gmail.com> writes:
>>>
>>>> So is there any reason why didn't do something like the following in
>>>> the first place?
>>>
>>> My guess is that we didn't bother; if we cared, we would have used a
>>> single instance of const char in a read-only segment, instead of
>>> such a macro.
>>
>> I think you mean something like this:
>>
>>const char * const strbuf_slopbuf = "";

Hmm, apparently it is sufficient to mark our current strbuf_slopbuf
array as const and initialize it with a static string to trigger its
placement into the read-only section by gcc (and clang).

   const char strbuf_slopbuf[1] = "";

-Brandon

Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf

[Brandon Casey]

On Wed, Aug 23, 2017 at 2:20 PM, Brandon Casey <draf...@gmail.com> wrote:
> On Wed, Aug 23, 2017 at 2:04 PM, Junio C Hamano <gits...@pobox.com> wrote:
>> Brandon Casey <draf...@gmail.com> writes:
>>
>>> So is there any reason why didn't do something like the following in
>>> the first place?
>>
>> My guess is that we didn't bother; if we cared, we would have used a
>> single instance of const char in a read-only segment, instead of
>> such a macro.
>
> I think you mean something like this:
>
>const char * const strbuf_slopbuf = "";

Ah, you probably meant something like this:

   const char strbuf_slopbuf = '\0';

which gcc will apparently place in the read-only segment.  I did not know that.

And assignment and initialization would look like:

   sb->buf = (char*) _slopbuf;

and

   #define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = (char*) _slopbuf }

respectively.  Yeah, that's definitely preferable to a macro.
Something similar could be done in object.c.

-Brandon

Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf

[Brandon Casey]

On Wed, Aug 23, 2017 at 2:04 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Brandon Casey <draf...@gmail.com> writes:
>
>> So is there any reason why didn't do something like the following in
>> the first place?
>
> My guess is that we didn't bother; if we cared, we would have used a
> single instance of const char in a read-only segment, instead of
> such a macro.

I think you mean something like this:

   const char * const strbuf_slopbuf = "";

..with or without "const" at the beginning.  We can't use an actual
variable like that since we also want to be able to do initialization
like:

   struct strbuf b = STRBUF_INIT;

i.e.

   struct strbuf b = { 0, 0, strbuf_slopbuf };

So the compiler needs to be able to determine that everything within
the curly braces is constant and apparently gcc cannot.


On a related note... I was just looking at object.c which also uses a
slopbuf.  We could similarly protect it from inadvertent modification
by doing something like this:

diff --git a/object.c b/object.c
index 321d7e9..4c7a041 100644
--- a/object.c
+++ b/object.c
@@ -303,7 +303,7 @@ int object_list_contains(struct object_list *list, struct ob
ject *obj)
  * A zero-length string to which object_array_entry::name can be
  * initialized without requiring a malloc/free.
  */
-static char object_array_slopbuf[1];
+static const char * const object_array_slopbuf = "";

 void add_object_array_with_path(struct object *obj, const char *name,
struct object_array *array,
@@ -326,7 +326,7 @@ void add_object_array_with_path(struct object
*obj, const char *name,
entry->name = NULL;
else if (!*name)
/* Use our own empty string instead of allocating one: */
-   entry->name = object_array_slopbuf;
+   entry->name = (char*) object_array_slopbuf;
else
entry->name = xstrdup(name);
entry->mode = mode;

Re: [PATCH v2 3/4] strbuf_setlen: don't write to strbuf_slopbuf

[Brandon Casey]

On Mon, Aug 21, 2017 at 10:43 AM, Martin Ågren  wrote:
> strbuf_setlen(., 0) writes '\0' to sb.buf[0], where buf is either
> allocated and unique to sb, or the global slopbuf. The slopbuf is meant
> to provide a guarantee that buf is not NULL and that a freshly
> initialized buffer contains the empty string, but it is not supposed to
> be written to. That strbuf_setlen writes to slopbuf has at least two
> implications:


> diff --git a/strbuf.h b/strbuf.h
> index e705b94db..7496cb8ec 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -147,7 +147,10 @@ static inline void strbuf_setlen(struct strbuf *sb, 
> size_t len)
> if (len > (sb->alloc ? sb->alloc - 1 : 0))
> die("BUG: strbuf_setlen() beyond buffer");
> sb->len = len;
> -   sb->buf[len] = '\0';
> +   if (sb->buf != strbuf_slopbuf)
> +   sb->buf[len] = '\0';
> +   else
> +   assert(!strbuf_slopbuf[0]);
>  }
>
>  /**
> --
> 2.14.1.151.gdfeca7a7e
>

I know this must have been discussed before and/or I'm having a neuron
misfire, but is there any reason why we didn't just make slopbuf a
macro for ""?

i.e.

   #define strbuf_slopbuf ""

That way it should point to readonly memory and any attempt to assign
to it should produce a crash.

One benefit that I can think of for making strbuf_slopbuf be a real
variable is that we can then compare the pointer stored in the strbuf
to the strbuf_slopbuf address to detect whether the strbuf held the
slopbuf.  With the static string macro, each execution unit may get
it's own instance of the empty string.  But, before this patch, we
don't actually seem to be doing that anywhere and instead rely on the
value of alloc being accurate to determine whether the strbuf contains
the slopbuf or not.

So is there any reason why didn't do something like the following in
the first place?

diff --git a/strbuf.h b/strbuf.h
index e705b94..fcca618 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -67,7 +67,7 @@ struct strbuf {
char *buf;
 };

-extern char strbuf_slopbuf[];
+#define strbuf_slopbuf ""
 #define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }

 /**
@@ -147,7 +147,9 @@ static inline void strbuf_setlen(struct strbuf
*sb, size_t len)
if (len > (sb->alloc ? sb->alloc - 1 : 0))
die("BUG: strbuf_setlen() beyond buffer");
sb->len = len;
-   sb->buf[len] = '\0';
+   if (sb->alloc) {
+   sb->buf[len] = '\0';
+   }
 }

-Brandon

Re: requesting permission to use some Git for Windows code

[Brandon Casey]

Hi Philippe,

Please feel free to use my portions of the mentioned works under the GPLv3.

-Brandon

On Tue, Jul 25, 2017 at 6:53 AM, Johannes Schindelin
<johannes.schinde...@gmx.de> wrote:
> Hi Philippe,
>
> I am not quite certain whether I have replied to this earlier or not.
> Under the assumption that I did not, I'll send this mail; Cc:ed to the
> mailing lists as discussed privately.
>
> On Fri, 23 Jun 2017, Philippe Joyez wrote:
>
>> This message is to request the permission to use code chunks from Git
>> for Windows in GNU TeXmacs <http://texmacs.org/>, to which I contribute.
>> The main developer of TeXmacs is Joris van der Hoeven (in cc).
>>
>> Context:
>>
>> Just like Git, TeXmacs originated on *nix platforms and was subsequently
>> ported to windows using MinGW. Naturally, some issues we have in that
>> port are the very same Git for Windows has faced.
>>
>> One specific problem you have solved and that TeXmacs still hasn't, is
>> dealing with unicode filenames. By taking relevant pieces of code in Git
>> for windows, I could easily come up with a patch that enables TeXmacs to
>> handle unicode filenames in windows.
>>
>> Now, the problem is that Git code is GPL V2, while TeXmacs is GPL V3:
>> Incorporating my patch in TeXmacs' trunk would be a violation of GPL
>> V2... /unless/ we are granted the permission to do so by the authors of
>> the code. This is precisely the reason for this message.
>
> It is great that you can make use of the code!
>
> As to the licensing problem, I agree it is a hassle. The biggest obstacle
> is that you have to have the consent of all the authors.
>
> You hereby have mine.
>
>> The chunks of code we would like to reuse are from these Git for Windows
>> files:
>> git-compat-util.h
>
> This file is quite large, maybe you can cut down on the authors to contact
> by restricting the `git annotate`/`git log`/`git shortlog` calls to
> specific parts, using the `-L ,` option?
>
>> ctype.c
>
> $ git shortlog -nse ctype.c
>  5  Junio C Hamano <gits...@pobox.com>
>  4  René Scharfe <l@web.de>
>  2  Nguyễn Thái Ngọc Duy <pclo...@gmail.com>
>  1  Ben Walton <bdwal...@gmail.com>
>  1  Brandon Casey <draf...@gmail.com>
>  1  Gary V. Vaughan <g...@mlists.thewrittenword.com>
>  1  Linus Torvalds <torva...@linux-foundation.org>
>  1  Namhyung Kim <namhy...@gmail.com>
>
> I *think* Ben Walton's change (189c860c9ec (kwset: use unsigned char to
> store values with high-bit set, 2015-03-02)) is not copyright-able, as it
> only changes the type from signed to unsigned. But I am not a lawyer ;-)
>
> Likewise, Namhyung Kim's change (1a191a22959 (ctype.c only wants
> git-compat-util.h, 2012-02-10)) only changes which header is included.
> That seems to be a too-obvious/too-trivial change to me.
>
> Also, it looks as if removing a comma as was done in 4b05548fc05 (enums:
> omit trailing comma for portability, 2010-05-14) by Gary V. Vaughan would
> not merit any copyright.
>
> If in doubt, you could simply take the version of ctype.c with those
> changes reverted as basis of your work.
>
> You still have to get the consent of Junio, René, Duy, Brandon and Linus
> to relicense the file's contents.
>
>> compat ¬
>>mingw.c
>
> I count 35 authors other than myself for that file... Maybe you can narrow
> down what you need?
>
>>mingw.h
>
> Still 29 authors other than me...
>
>>win32.h
>
> This is more manageable, as it only saw three authors. But then, you could
> simply reimplement the functionality, it's just two functions, and I do
> not think that get_file_attr() is implemented in the best way: we have a
> function called err_win_to_posix() in compat/mingw.c which is much more
> complete.
>
> Having said that, err_win_to_posix() is still not implemented in the best
> way. The best way is to abuse Windows' own (undocumented) _doserrmap()
> function along with the information in the header files winerror.h and
> errno.h to generate the mapping. Those two files, as per mingw-w64's
> headers, have the very nice preamble:
>
> /**
>  * This file has no copyright assigned and is placed in the Public 
> Domain.
>  * This file is part of the mingw-w64 runtime package.
>  * No warranty is given; refer to the file DISCLAIMER.PD within this
>  * package.
>  */
>
> Therefore, the result has no copyright assigned and is placed in the
> Public Domain and we can do the very same, too.
>
> As I wanted to have a Windows error -> errno mapping that I c

Re: [PATCH] gnome-keyring: Don't hard-code pkg-config executable

[Brandon Casey]

On Thu, Jun 16, 2016 at 2:50 AM, Jeff King  wrote:
> On Tue, Jun 14, 2016 at 01:27:05PM +0200, Heiko Becker wrote:
>
>> Helpful if your pkg-config executable has a prefix based on the
>> architecture, for example.
>>
>> Signed-off-by: Heiko Becker 
>
> Sounds like a reasonable thing to want to do...

ditto.

> ...and the implementation looks obviously correct.

ditto.

> Thanks.

ditto.

See I'm still alive, really!

-Brandon
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] git-send-email.perl: support no- prefix with older GetOptions

[Brandon Casey]

On Sun, Feb 15, 2015 at 1:51 AM, Kyle J. McKay mack...@gmail.com wrote:
 On Feb 14, 2015, at 22:32, Brandon Casey wrote:

 On Fri, Feb 13, 2015 at 12:19 PM, Junio C Hamano gits...@pobox.com
 wrote:

 From: Kyle J. McKay mack...@gmail.com

 Only Perl version 5.8.0 or later is required, but that comes with
 an older Getopt::Long (2.32) that does not support the 'no-'
 prefix.  Support for that was added in Getopt::Long version 2.33.

 Since the help only mentions the 'no-' prefix and not the 'no'
 prefix, add explicit support for the 'no-' prefix when running
 with older GetOptions versions.


 ultra-ultra-nit: s/when running/for when running/


 So it would say add explicit support for the 'no-'prefix for when running
 with...  That doesn't make sense to me.

 The current wording
 makes it sound like the explicit support is only enabled when running
 with older GetOpt versions.


 How about this instead:

 Since the help only mentions the 'no-' prefix and not the 'no'
 prefix, add explicit support for the 'no-' prefix to support
 older GetOptions versions.

Works for me.

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] Getopt::Long workaround in send-email

[Brandon Casey]

[apparently it is impossible to send a plain text email using Google
Inbox, maybe people on this list know someone to talk to about that?
Sorry for the dup for those on cc]

On Fri, Feb 13, 2015 at 12:19 PM, Junio C Hamano gits...@pobox.com wrote:
 The first one is a replay of Kyle's workaround for older versions of
 Getopt::Long that did not take --no-option to negate a boolean
 option --option.  The second one revert the workarounds made to
 the test script over time, and should break if the first one does
 not work well for older Getopt::Long (I have no reason to suspect it
 would break, though).

The only downside I can see is that we're going to end up carrying
around these extra options for the forseeable future and possibly
adding more over time with this precedent.  Maybe that's not so bad.
The extra options are not ugly at all.  My original thinking in just
fixing up the tests was that the platforms with ancient versions of
perl/Getopt::Long would just disappear over time and we'd eventually
stop fixing up the tests to be backwards compatible when people
stopped showing up saying that the tests failed on their ancient
system.

What platforms are actually affected?  RHEL3?  Other ancient UNIX?  I
know the systems I was fixing up were ancient SunOS and IRIX.

Unfortunately (or fortunately, depending on how you look at it), I
don't have access to any ancient systems to test on anymore.  So I
can't run the updated tests to make sure they still pass.  The patches
look fine to me though. :-)

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] git-send-email.perl: support no- prefix with older GetOptions

[Brandon Casey]

On Fri, Feb 13, 2015 at 12:19 PM, Junio C Hamano gits...@pobox.com wrote:
 From: Kyle J. McKay mack...@gmail.com

 Only Perl version 5.8.0 or later is required, but that comes with
 an older Getopt::Long (2.32) that does not support the 'no-'
 prefix.  Support for that was added in Getopt::Long version 2.33.

 Since the help only mentions the 'no-' prefix and not the 'no'
 prefix, add explicit support for the 'no-' prefix when running
 with older GetOptions versions.

ultra-ultra-nit: s/when running/for when running/  The current wording
makes it sound like the explicit support is only enabled when running
with older GetOpt versions.

 Reported-by: Tom G. Christensen t...@statsbiblioteket.dk
 Signed-off-by: Kyle J. McKay mack...@gmail.com
 Signed-off-by: Junio C Hamano gits...@pobox.com
 ---
  git-send-email.perl | 10 ++
  1 file changed, 10 insertions(+)

 diff --git a/git-send-email.perl b/git-send-email.perl
 index 3092ab3..a18a795 100755
 --- a/git-send-email.perl
 +++ b/git-send-email.perl
 @@ -299,6 +299,7 @@ sub signal_handler {
 bcc=s = \@bcclist,
 no-bcc = \$no_bcc,
 chain-reply-to! = \$chain_reply_to,
 +   no-chain-reply-to = sub {$chain_reply_to = 0},
 smtp-server=s = \$smtp_server,
 smtp-server-option=s = \@smtp_server_options,
 smtp-server-port=s = \$smtp_server_port,
 @@ -311,25 +312,34 @@ sub signal_handler {
 smtp-domain:s = \$smtp_domain,
 identity=s = \$identity,
 annotate! = \$annotate,
 +   no-annotate = sub {$annotate = 0},
 compose = \$compose,
 quiet = \$quiet,
 cc-cmd=s = \$cc_cmd,
 suppress-from! = \$suppress_from,
 +   no-suppress-from = sub {$suppress_from = 0},
 suppress-cc=s = \@suppress_cc,
 signed-off-cc|signed-off-by-cc! = \$signed_off_by_cc,
 +   no-signed-off-cc|no-signed-off-by-cc = sub 
 {$signed_off_by_cc = 0},
 cc-cover|cc-cover! = \$cover_cc,

I know it's not part of this patch, but does the above duplication of
cc-cover do something I'm not aware of?  Or should it just be
cc-cover!?

 +   no-cc-cover = sub {$cover_cc = 0},
 to-cover|to-cover! = \$cover_to,

Here (above) too.

 +   no-to-cover = sub {$cover_to = 0},
 confirm=s = \$confirm,
 dry-run = \$dry_run,
 envelope-sender=s = \$envelope_sender,
 thread! = \$thread,
 +   no-thread = sub {$thread = 0},
 validate! = \$validate,
 +   no-validate = sub {$validate = 0},
 transfer-encoding=s = \$target_xfer_encoding,
 format-patch! = \$format_patch,
 +   no-format-patch = sub {$format_patch = 0},
 8bit-encoding=s = \$auto_8bit_encoding,
 compose-encoding=s = \$compose_encoding,
 force = \$force,
 xmailer! = \$use_xmailer,
 +   no-xmailer = sub {$use_xmailer = 0},
  );

  usage() if $help;

Looks fine to me.

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] sequencer.c: check for lock failure and bail early in fast_forward_to

[Brandon Casey]

On Tue, Apr 15, 2014 at 4:46 PM, Ronnie Sahlberg sahlb...@google.com wrote:

snip well-worded commit message

 Signed-off-by: Ronnie Sahlberg sahlb...@google.com
 ---
  sequencer.c | 7 +++
  1 file changed, 7 insertions(+)

 diff --git a/sequencer.c b/sequencer.c
 index bde5f04..6aa3b50 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -281,8 +281,15 @@ static int fast_forward_to(const unsigned char *to, 
 const unsigned char *from,
 exit(1); /* the callee should have complained already */
 ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from,
0, NULL);
 +   if (!ref_lock) {
 +   ret = error(_(Failed to lock HEAD during fast_forward_to));
 +   goto leave;
 +   }
 +

Or just:

   if (!ref_lock)
   return error(_(Failed to lock HEAD ...));

We don't need to strbuf_release() since the strbuf has not been
modified at this point.  We've only initialized with a static
initializer.

 strbuf_addf(sb, %s: fast-forward, action_name(opts));
 ret = write_ref_sha1(ref_lock, to, sb.buf);
 +
 +leave:
 strbuf_release(sb);
 return ret;
  }
 --

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation/config.txt: denyDeleteCurrent applies to bare repos too

[Brandon Casey]

On Thu, Oct 17, 2013 at 3:23 PM, Junio C Hamano gits...@pobox.com wrote:
 Brandon Casey bca...@nvidia.com writes:

 From: Brandon Casey draf...@gmail.com

 The setting of denyDeleteCurrent applies to both bare and non-bare
 repositories.  Correct the description on this point, and expand it to
 provide some background justification for the current behavior and
 describe the full suite of settings.

 Signed-off-by: Brandon Casey draf...@gmail.com
 ---
  Documentation/config.txt | 11 +--
  1 file changed, 9 insertions(+), 2 deletions(-)

 diff --git a/Documentation/config.txt b/Documentation/config.txt
 index c3f7002..3d416ec 100644
 --- a/Documentation/config.txt
 +++ b/Documentation/config.txt
 @@ -1993,8 +1993,15 @@ receive.denyDeletes::
   the ref. Use this to prevent such a ref deletion via a push.

  receive.denyDeleteCurrent::
 - If set to true, git-receive-pack will deny a ref update that
 - deletes the currently checked out branch of a non-bare repository.
 + If set to true or refuse, git-receive-pack will deny a ref update
 + that deletes the currently checked out branch of a non-bare repository,
 + or the default branch in a bare repository.  i.e. the branch
 + that HEAD refers to.

 It reads just fine without the part that you found the need for
 clarification with i.e., i.e.

 or the branch that HEAD points at in a bare repository.

 without introducing a new word default branch that is not defined
 in the glossary.

Either way is fine with me.  The phrase the branch that HEAD points
at applies to either a bare or non-bare repo though, so the i.e.
was directed at both parts of the preceding sentence.  Guess we
haven't defined an alternative way to say the branch that HEAD points
at for a bare repository à la currently checked out branch for a
non-bare repository.

 + Deleting the current branch from a remote will
 + cause the HEAD symbolic ref to become dangling and will result in the
 + next clone from it to not check out anything.

 This sentence tells truth but does not fit in the logic flow in the
 paragraph. I am reading it as primarily meant to be an explanation
 why it would be a good idea to apply this seemingly non-bare only
 option (implied by current in its name---it is so rare for a bare
 repository to repoint its HEAD that the concept of current does
 not mesh well with a bare one) to a bare one.

Yep, that's the correct reading: as an explanation for why this should
apply to bare repos as well as non-bare.

 It may be a good thing
 to have, but the thought-process may flow better if it is made as a
 FYI after the main text, i.e.

 If set to true or refuse, `git-receive-pack` will deny a
 ref update that deletes the branch that HAED points at.  If
 set to warn, ... If set to false or ignore, ... Defaults
 to refuse.
 +
 Deleting the branch that HEAD points at will cause the HEAD symbolic
 ref to become dangling.  This causes the next commit to become a
 root commit, disconnected from the old history, in a non-bare
 repository.  It also causes the next clone from such a repository
 (either bare or non-bare) not to check out anything.

 perhaps?

Yes, much better as a note following the main text.  Thanks.

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/16] Make Gnome Credential helper more Gnome-y and support ancient distros

[Brandon Casey]

On 10/15/2013 3:40 PM, Junio C Hamano wrote:
 This seems to post-date what Jonathan has kept in his 'pu'; is this
 the latest (I have a huge backlog to wade through, so I'd rather
 skip it if another reroll is coming and move on to other topics).
 
 Thanks.

This is the latest.

I didn't have anything else planned.  I think Philipp planned to submit
some style cleanups on top for areas of the code I didn't touch.

-Brandon


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Documentation/config.txt: denyDeleteCurrent applies to bare repos too

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

The setting of denyDeleteCurrent applies to both bare and non-bare
repositories.  Correct the description on this point, and expand it to
provide some background justification for the current behavior and
describe the full suite of settings.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 Documentation/config.txt | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c3f7002..3d416ec 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1993,8 +1993,15 @@ receive.denyDeletes::
the ref. Use this to prevent such a ref deletion via a push.
 
 receive.denyDeleteCurrent::
-   If set to true, git-receive-pack will deny a ref update that
-   deletes the currently checked out branch of a non-bare repository.
+   If set to true or refuse, git-receive-pack will deny a ref update
+   that deletes the currently checked out branch of a non-bare repository,
+   or the default branch in a bare repository.  i.e. the branch
+   that HEAD refers to.  Deleting the current branch from a remote will
+   cause the HEAD symbolic ref to become dangling and will result in the
+   next clone from it to not check out anything.  If set to warn,
+   then a warning will be printed to stderr and the deletion will be
+   performed.  If set to false or ignore, then the deletion will be
+   performed with no warning message.  Defaults to refuse.
 
 receive.denyCurrentBranch::
If set to true or refuse, git-receive-pack will deny a ref update
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: return of the maintainer

[Brandon Casey]

For some reason, I have the theme for Star Wars in my head. :)

On Mon, Oct 14, 2013 at 10:37 AM, Junio C Hamano gits...@pobox.com wrote:
 I am physically back to work, but I'll have to coordinate the
 hand-off of topic branches updated during my absence with Jonathan
 before resuming to update my git.git repository at kernel.org and
 elsewhere.
 --
 To unsubscribe from this list: send the line unsubscribe git in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/15] contrib/git-credential-gnome-keyring.c: ensure buffer is non-empty before accessing

[Brandon Casey]

Thanks.

On Sun, Sep 22, 2013 at 10:43 PM, Felipe Contreras
felipe.contre...@gmail.com wrote:
 Brandon Casey wrote:
 Ensure buffer length is non-zero before attempting to access the last
 element.

 Signed-off-by: Brandon Casey draf...@gmail.com
 ---
  contrib/credential/gnome-keyring/git-credential-gnome-keyring.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
 b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
 index 1081224..8ae2eab 100644
 --- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
 +++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
 @@ -315,7 +315,7 @@ static int credential_read(struct credential *c)
   {
   line_len = strlen(buf);

 - if(buf[line_len-1]=='\n')
 + if(line_len  buf[line_len-1] == '\n')

 The style is if ().

 --
 Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/15] Make Gnome Credential helper more Gnome-y and support ancient distros

[Brandon Casey]

On Mon, Sep 23, 2013 at 3:20 AM, John Szakmeister j...@szakmeister.net wrote:
 On Sun, Sep 22, 2013 at 10:07:56PM -0700, Brandon Casey wrote:
 A few cleanups, followed by improved usage of the glib library (no need
 to reinvent the wheel when glib provides the necessary functionality), and
 then the addition of support for RHEL 4.x and 5.x.

 Brandon Casey (15):
snip

 I reviewed all of the commits in this series, and most are nice
 cleanups.  The only thing I'm a little shaky on is RHEL4
 support (patch 15).  In particular, this:

 +/*
 + * Just a guess to support RHEL 4.X.
 + * Glib 2.8 was roughly Gnome 2.12 ?
 + * Which was released with gnome-keyring 0.4.3 ??
 + */

 Has that code been tested on RHEL4 at all?  I imagine it's hard
 to come by--it's pretty darn old--but it feels like a mistake to
 commit untested code.

The (poorly worded) comment is referring to the version of glib that
is being tested by the pre-processor statements.  Since gnome-keyring
doesn't provide a way to test its version, and I'm not sure exactly
which gnome release included gnome-keyring 0.4.3 or which glib version
was distributed with which gnome version, I'm just roughly guessing
based on dates and not-conclusive google searches that 2.8 is
reasonable.

I'll try to clarify the wording here.

The code has been tested on CentOS 4.X.

 Otherwise, there are a few stylistic nits that I'd like to clean
 up.  Only one was introduced by you--Felipe pointed it out--and

Well, not exactly introduced by me, but I guess I can fix it in that
same patch. :)

 I have a patch for the rest that I can submit after your series
 has been applied.

Sounds good.

 Acked-by: John Szakmeister j...@szakmeister.net

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 00/16] Make Gnome Credential helper more Gnome-y and support ancient distros

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Mostly unchanged.

Inserts a patch to fix the style issues for block statements.
i.e. use if () instead of if()

A couple early patches were reordered to improve logical flow.

Updated the comment in the last patch to hopefully improve clarity
wrt RHEL 4.X

The only functional change is in 14/16 report failure to store.
We should accept GNOME_KEYRING_RESULT_CANCELLED as a successful
return and _not_ produce an error message.

Interdiff follows...

Brandon Casey (16):
  contrib/git-credential-gnome-keyring.c: remove unnecessary
pre-declarations
  contrib/git-credential-gnome-keyring.c: remove unused die() function
  contrib/git-credential-gnome-keyring.c: *style* use if () not if()
etc.
  contrib/git-credential-gnome-keyring.c: add static where applicable
  contrib/git-credential-gnome-keyring.c: exit non-zero when called
incorrectly
  contrib/git-credential-gnome-keyring.c: strlen() returns size_t, not
ssize_t
  contrib/git-credential-gnome-keyring.c: ensure buffer is non-empty
before accessing
  contrib/git-credential-gnome-keyring.c: set Gnome application name
  contrib/git-credential-gnome-keyring.c: use gnome helpers in
keyring_object()
  contrib/git-credential-gnome-keyring.c: use secure memory functions
for passwds
  contrib/git-credential-gnome-keyring.c: use secure memory for reading
passwords
  contrib/git-credential-gnome-keyring.c: use glib memory allocation
functions
  contrib/git-credential-gnome-keyring.c: use glib messaging functions
  contrib/git-credential-gnome-keyring.c: report failure to store
password
  contrib/git-credential-gnome-keyring.c: support ancient gnome-keyring
  contrib/git-credential-gnome-keyring.c: support really ancient
gnome-keyring

 contrib/credential/gnome-keyring/Makefile  |   4 +-
 .../gnome-keyring/git-credential-gnome-keyring.c   | 301 -
 2 files changed, 169 insertions(+), 136 deletions(-)

---

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index ce2ddee..635c96b 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -50,7 +50,7 @@
 
 /*
  * ancient gnome-keyring returns DENIED when an entry is not found.
- * Setting NO_MATCH to DENIED will prevent us from reporting denied
+ * Setting NO_MATCH to DENIED will prevent us from reporting DENIED
  * errors during get and erase operations, but we will still report
  * DENIED errors during a store.
  */
@@ -87,8 +87,8 @@ static const char* 
gnome_keyring_result_to_message(GnomeKeyringResult result)
 }
 
 /*
- * Just a guess to support RHEL 4.X.
- * Glib 2.8 was roughly Gnome 2.12 ?
+ * Support really ancient gnome-keyring, circ. RHEL 4.X.
+ * Just a guess for the Glib version.  Glib 2.8 was roughly Gnome 2.12 ?
  * Which was released with gnome-keyring 0.4.3 ??
  */
 #if GLIB_MAJOR_VERSION == 2  GLIB_MINOR_VERSION  8
@@ -162,7 +162,7 @@ static char* keyring_object(struct credential *c)
if (!c-path)
return NULL;
 
-   if(c-port)
+   if (c-port)
return g_strdup_printf(%s:%hd/%s, c-host, c-port, c-path);
 
return g_strdup_printf(%s/%s, c-host, c-path);
@@ -251,7 +251,8 @@ static int keyring_store(struct credential *c)
 
g_free(object);
 
-   if (result != GNOME_KEYRING_RESULT_OK) {
+   if (result != GNOME_KEYRING_RESULT_OK 
+   result != GNOME_KEYRING_RESULT_CANCELLED) {
g_critical(%s, gnome_keyring_result_to_message(result));
return EXIT_FAILURE;
}
@@ -363,14 +364,14 @@ static int credential_read(struct credential *c)
{
line_len = strlen(buf);
 
-   if(line_len  buf[line_len-1] == '\n')
+   if (line_len  buf[line_len-1] == '\n')
buf[--line_len]='\0';
 
-   if(!line_len)
+   if (!line_len)
break;
 
value = strchr(buf,'=');
-   if(!value) {
+   if (!value) {
g_warning(invalid credential line: %s, key);
gnome_keyring_memory_free(buf);
return -1;
@@ -432,9 +433,9 @@ static void usage(const char *name)
 
basename = (basename) ? basename + 1 : name;
fprintf(stderr, usage: %s , basename);
-   while(try_op-name) {
+   while (try_op-name) {
fprintf(stderr,%s,(try_op++)-name);
-   if(try_op-name)
+   if (try_op-name)
fprintf(stderr,%s,|);
}
fprintf(stderr,%s,\n);
@@ -455,15 +456,15 @@ int main(int argc, char *argv[])
g_set_application_name(Git Credential Helper);
 
/* lookup operation callback */
-   while(try_op-name  strcmp(argv[1], try_op-name))
+   while

[PATCH v2 02/16] contrib/git-credential-gnome-keyring.c: remove unused die() function

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../credential/gnome-keyring/git-credential-gnome-keyring.c| 10 --
 1 file changed, 10 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 15b0a1c..4334f23 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -91,16 +91,6 @@ static inline void error(const char *fmt, ...)
va_end(ap);
 }
 
-static inline void die(const char *fmt, ...)
-{
-   va_list ap;
-
-   va_start(ap,fmt);
-   error(fmt, ap);
-   va_end(ap);
-   exit(EXIT_FAILURE);
-}
-
 static inline void die_errno(int err)
 {
error(%s, strerror(err));
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 10/16] contrib/git-credential-gnome-keyring.c: use secure memory functions for passwds

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

gnome-keyring provides functions for allocating non-pageable memory (if
possible) intended to be used for storing passwords.  Let's use them.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../gnome-keyring/git-credential-gnome-keyring.c| 21 ++---
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index b692e1f..d8a7038 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -30,6 +30,7 @@
 #include errno.h
 #include glib.h
 #include gnome-keyring.h
+#include gnome-keyring-memory.h
 
 /*
  * This credential struct and API is simplified from git's credential.{h,c}
@@ -60,16 +61,6 @@ struct credential_operation
 
 /*  common helper functions - */
 
-static inline void free_password(char *password)
-{
-   char *c = password;
-   if (!password)
-   return;
-
-   while (*c) *c++ = '\0';
-   free(password);
-}
-
 static inline void warning(const char *fmt, ...)
 {
va_list ap;
@@ -159,8 +150,8 @@ static int keyring_get(struct credential *c)
/* pick the first one from the list */
password_data = (GnomeKeyringNetworkPasswordData *) entries-data;
 
-   free_password(c-password);
-   c-password = xstrdup(password_data-password);
+   gnome_keyring_memory_free(c-password);
+   c-password = gnome_keyring_memory_strdup(password_data-password);
 
if (!c-username)
c-username = xstrdup(password_data-user);
@@ -291,7 +282,7 @@ static void credential_clear(struct credential *c)
free(c-host);
free(c-path);
free(c-username);
-   free_password(c-password);
+   gnome_keyring_memory_free(c-password);
 
credential_init(c);
 }
@@ -338,8 +329,8 @@ static int credential_read(struct credential *c)
free(c-username);
c-username = xstrdup(value);
} else if (!strcmp(key, password)) {
-   free_password(c-password);
-   c-password = xstrdup(value);
+   gnome_keyring_memory_free(c-password);
+   c-password = gnome_keyring_memory_strdup(value);
while (*value) *value++ = '\0';
}
/*
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 03/16] contrib/git-credential-gnome-keyring.c: *style* use if () not if() etc.

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../gnome-keyring/git-credential-gnome-keyring.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 4334f23..809b1b7 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -117,10 +117,10 @@ static char* keyring_object(struct credential *c)
return object;
 
object = (char*) malloc(strlen(c-host)+strlen(c-path)+8);
-   if(!object)
+   if (!object)
die_errno(errno);
 
-   if(c-port)
+   if (c-port)
sprintf(object,%s:%hd/%s,c-host,c-port,c-path);
else
sprintf(object,%s/%s,c-host,c-path);
@@ -314,14 +314,14 @@ int credential_read(struct credential *c)
{
line_len = strlen(buf);
 
-   if(buf[line_len-1]=='\n')
+   if (buf[line_len-1]=='\n')
buf[--line_len]='\0';
 
-   if(!line_len)
+   if (!line_len)
break;
 
value = strchr(buf,'=');
-   if(!value) {
+   if (!value) {
warning(invalid credential line: %s, key);
return -1;
}
@@ -379,9 +379,9 @@ static void usage(const char *name)
 
basename = (basename) ? basename + 1 : name;
fprintf(stderr, usage: %s , basename);
-   while(try_op-name) {
+   while (try_op-name) {
fprintf(stderr,%s,(try_op++)-name);
-   if(try_op-name)
+   if (try_op-name)
fprintf(stderr,%s,|);
}
fprintf(stderr,%s,\n);
@@ -400,15 +400,15 @@ int main(int argc, char *argv[])
}
 
/* lookup operation callback */
-   while(try_op-name  strcmp(argv[1], try_op-name))
+   while (try_op-name  strcmp(argv[1], try_op-name))
try_op++;
 
/* unsupported operation given -- ignore silently */
-   if(!try_op-name || !try_op-op)
+   if (!try_op-name || !try_op-op)
goto out;
 
ret = credential_read(cred);
-   if(ret)
+   if (ret)
goto out;
 
/* perform credential operation */
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 15/16] contrib/git-credential-gnome-keyring.c: support ancient gnome-keyring

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

The gnome-keyring lib distributed with RHEL 5.X is ancient and does
not provide a few of the functions/defines that more recent versions
do, but mostly the API is the same.  Let's provide the missing bits
via macro definitions and function implementation.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../gnome-keyring/git-credential-gnome-keyring.c   | 58 ++
 1 file changed, 58 insertions(+)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 447e9aa..e1bc3fa 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -28,8 +28,66 @@
 #include stdlib.h
 #include glib.h
 #include gnome-keyring.h
+
+#ifdef GNOME_KEYRING_DEFAULT
+
+   /* Modern gnome-keyring */
+
 #include gnome-keyring-memory.h
 
+#else
+
+   /*
+* Support ancient gnome-keyring, circ. RHEL 5.X.
+* GNOME_KEYRING_DEFAULT seems to have been introduced with Gnome 2.22,
+* and the other features roughly around Gnome 2.20, 6 months before.
+* Ubuntu 8.04 used Gnome 2.22 (I think).  Not sure any distro used 2.20.
+* So the existence/non-existence of GNOME_KEYRING_DEFAULT seems like
+* a decent thing to use as an indicator.
+*/
+
+#define GNOME_KEYRING_DEFAULT NULL
+
+/*
+ * ancient gnome-keyring returns DENIED when an entry is not found.
+ * Setting NO_MATCH to DENIED will prevent us from reporting DENIED
+ * errors during get and erase operations, but we will still report
+ * DENIED errors during a store.
+ */
+#define GNOME_KEYRING_RESULT_NO_MATCH GNOME_KEYRING_RESULT_DENIED
+
+#define gnome_keyring_memory_alloc g_malloc
+#define gnome_keyring_memory_free gnome_keyring_free_password
+#define gnome_keyring_memory_strdup g_strdup
+
+static const char* gnome_keyring_result_to_message(GnomeKeyringResult result)
+{
+   switch (result) {
+   case GNOME_KEYRING_RESULT_OK:
+   return OK;
+   case GNOME_KEYRING_RESULT_DENIED:
+   return Denied;
+   case GNOME_KEYRING_RESULT_NO_KEYRING_DAEMON:
+   return No Keyring Daemon;
+   case GNOME_KEYRING_RESULT_ALREADY_UNLOCKED:
+   return Already UnLocked;
+   case GNOME_KEYRING_RESULT_NO_SUCH_KEYRING:
+   return No Such Keyring;
+   case GNOME_KEYRING_RESULT_BAD_ARGUMENTS:
+   return Bad Arguments;
+   case GNOME_KEYRING_RESULT_IO_ERROR:
+   return IO Error;
+   case GNOME_KEYRING_RESULT_CANCELLED:
+   return Cancelled;
+   case GNOME_KEYRING_RESULT_ALREADY_EXISTS:
+   return Already Exists;
+   default:
+   return Unknown Error;
+   }
+}
+
+#endif
+
 /*
  * This credential struct and API is simplified from git's credential.{h,c}
  */
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 06/16] contrib/git-credential-gnome-keyring.c: strlen() returns size_t, not ssize_t

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Also, initialization is not necessary since it is assigned before it is
used.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 contrib/credential/gnome-keyring/git-credential-gnome-keyring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 04852d7..b9bb794 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -306,7 +306,7 @@ static void credential_clear(struct credential *c)
 static int credential_read(struct credential *c)
 {
charbuf[1024];
-   ssize_t line_len = 0;
+   size_t line_len;
char   *key  = buf;
char   *value;
 
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 14/16] contrib/git-credential-gnome-keyring.c: report failure to store password

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Produce an error message when we fail to store a password to the keyring.

Signed-off-by: Brandon Casey draf...@gmail.com
---

Difference from v1:
Additionally interpret GNOME_KEYRING_RESULT_CANCELLED as a successful exit
status, since that means that the user intentionally cancelled the
operation from the gui.  We should not produce a warning in that case.

 .../credential/gnome-keyring/git-credential-gnome-keyring.c| 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index b70bd53..447e9aa 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -125,6 +125,7 @@ static int keyring_store(struct credential *c)
 {
guint32 item_id;
char  *object = NULL;
+   GnomeKeyringResult result;
 
/*
 * Sanity check that what we are storing is actually sensible.
@@ -139,7 +140,7 @@ static int keyring_store(struct credential *c)
 
object = keyring_object(c);
 
-   gnome_keyring_set_network_password_sync(
+   result = gnome_keyring_set_network_password_sync(
GNOME_KEYRING_DEFAULT,
c-username,
NULL /* domain */,
@@ -152,6 +153,13 @@ static int keyring_store(struct credential *c)
item_id);
 
g_free(object);
+
+   if (result != GNOME_KEYRING_RESULT_OK 
+   result != GNOME_KEYRING_RESULT_CANCELLED) {
+   g_critical(%s, gnome_keyring_result_to_message(result));
+   return EXIT_FAILURE;
+   }
+
return EXIT_SUCCESS;
 }
 
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 07/16] contrib/git-credential-gnome-keyring.c: ensure buffer is non-empty before accessing

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Ensure buffer length is non-zero before attempting to access the last
element.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 contrib/credential/gnome-keyring/git-credential-gnome-keyring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index b9bb794..0d2c55e 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -314,7 +314,7 @@ static int credential_read(struct credential *c)
{
line_len = strlen(buf);
 
-   if (buf[line_len-1]=='\n')
+   if (line_len  buf[line_len-1] == '\n')
buf[--line_len]='\0';
 
if (!line_len)
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 12/16] contrib/git-credential-gnome-keyring.c: use glib memory allocation functions

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Rather than roll our own, let's use the memory allocation/free routines
provided by glib.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../gnome-keyring/git-credential-gnome-keyring.c   | 48 --
 1 file changed, 16 insertions(+), 32 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 5e79669..273c43b 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -27,7 +27,6 @@
 #include string.h
 #include stdarg.h
 #include stdlib.h
-#include errno.h
 #include glib.h
 #include gnome-keyring.h
 #include gnome-keyring-memory.h
@@ -83,21 +82,6 @@ static inline void error(const char *fmt, ...)
va_end(ap);
 }
 
-static inline void die_errno(int err)
-{
-   error(%s, strerror(err));
-   exit(EXIT_FAILURE);
-}
-
-static inline char *xstrdup(const char *str)
-{
-   char *ret = strdup(str);
-   if (!ret)
-   die_errno(errno);
-
-   return ret;
-}
-
 /* - GNOME Keyring functions - */
 
 /* create a special keyring option string, if path is given */
@@ -134,7 +118,7 @@ static int keyring_get(struct credential *c)
c-port,
entries);
 
-   free(object);
+   g_free(object);
 
if (result == GNOME_KEYRING_RESULT_NO_MATCH)
return EXIT_SUCCESS;
@@ -154,7 +138,7 @@ static int keyring_get(struct credential *c)
c-password = gnome_keyring_memory_strdup(password_data-password);
 
if (!c-username)
-   c-username = xstrdup(password_data-user);
+   c-username = g_strdup(password_data-user);
 
gnome_keyring_network_password_list_free(entries);
 
@@ -192,7 +176,7 @@ static int keyring_store(struct credential *c)
c-password,
item_id);
 
-   free(object);
+   g_free(object);
return EXIT_SUCCESS;
 }
 
@@ -226,7 +210,7 @@ static int keyring_erase(struct credential *c)
c-port,
entries);
 
-   free(object);
+   g_free(object);
 
if (result == GNOME_KEYRING_RESULT_NO_MATCH)
return EXIT_SUCCESS;
@@ -278,10 +262,10 @@ static void credential_init(struct credential *c)
 
 static void credential_clear(struct credential *c)
 {
-   free(c-protocol);
-   free(c-host);
-   free(c-path);
-   free(c-username);
+   g_free(c-protocol);
+   g_free(c-host);
+   g_free(c-path);
+   g_free(c-username);
gnome_keyring_memory_free(c-password);
 
credential_init(c);
@@ -315,22 +299,22 @@ static int credential_read(struct credential *c)
*value++ = '\0';
 
if (!strcmp(key, protocol)) {
-   free(c-protocol);
-   c-protocol = xstrdup(value);
+   g_free(c-protocol);
+   c-protocol = g_strdup(value);
} else if (!strcmp(key, host)) {
-   free(c-host);
-   c-host = xstrdup(value);
+   g_free(c-host);
+   c-host = g_strdup(value);
value = strrchr(c-host,':');
if (value) {
*value++ = '\0';
c-port = atoi(value);
}
} else if (!strcmp(key, path)) {
-   free(c-path);
-   c-path = xstrdup(value);
+   g_free(c-path);
+   c-path = g_strdup(value);
} else if (!strcmp(key, username)) {
-   free(c-username);
-   c-username = xstrdup(value);
+   g_free(c-username);
+   c-username = g_strdup(value);
} else if (!strcmp(key, password)) {
gnome_keyring_memory_free(c-password);
c-password = gnome_keyring_memory_strdup(value);
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 11/16] contrib/git-credential-gnome-keyring.c: use secure memory for reading passwords

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

gnome-keyring provides functions to allocate non-pageable memory (if
possible).  Let's use them to allocate memory that may be used to hold
secure data read from the keyring.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../credential/gnome-keyring/git-credential-gnome-keyring.c  | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index d8a7038..5e79669 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -289,12 +289,14 @@ static void credential_clear(struct credential *c)
 
 static int credential_read(struct credential *c)
 {
-   charbuf[1024];
+   char*buf;
size_t line_len;
-   char   *key  = buf;
+   char   *key;
char   *value;
 
-   while (fgets(buf, sizeof(buf), stdin))
+   key = buf = gnome_keyring_memory_alloc(1024);
+
+   while (fgets(buf, 1024, stdin))
{
line_len = strlen(buf);
 
@@ -307,6 +309,7 @@ static int credential_read(struct credential *c)
value = strchr(buf,'=');
if (!value) {
warning(invalid credential line: %s, key);
+   gnome_keyring_memory_free(buf);
return -1;
}
*value++ = '\0';
@@ -339,6 +342,9 @@ static int credential_read(struct credential *c)
 * learn new lines, and the helpers are updated to match.
 */
}
+
+   gnome_keyring_memory_free(buf);
+
return 0;
 }
 
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 16/16] contrib/git-credential-gnome-keyring.c: support really ancient gnome-keyring

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

The gnome-keyring lib (0.4) distributed with RHEL 4.X is really ancient
and does not provide most of the synchronous functions that even ancient
releases do.  Thankfully, we're only using one function that is missing.
Let's emulate gnome_keyring_item_delete_sync() by calling the asynchronous
function and then triggering the event loop processing until our
callback is called.

Signed-off-by: Brandon Casey draf...@gmail.com
---

Clarified the comment about RHEL 4.X support.

 .../gnome-keyring/git-credential-gnome-keyring.c   | 39 ++
 1 file changed, 39 insertions(+)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index e1bc3fa..635c96b 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -86,6 +86,45 @@ static const char* 
gnome_keyring_result_to_message(GnomeKeyringResult result)
}
 }
 
+/*
+ * Support really ancient gnome-keyring, circ. RHEL 4.X.
+ * Just a guess for the Glib version.  Glib 2.8 was roughly Gnome 2.12 ?
+ * Which was released with gnome-keyring 0.4.3 ??
+ */
+#if GLIB_MAJOR_VERSION == 2  GLIB_MINOR_VERSION  8
+
+static void gnome_keyring_done_cb(GnomeKeyringResult result, gpointer 
user_data)
+{
+   gpointer *data = (gpointer*) user_data;
+   int *done = (int*) data[0];
+   GnomeKeyringResult *r = (GnomeKeyringResult*) data[1];
+
+   *r = result;
+   *done = 1;
+}
+
+static void wait_for_request_completion(int *done)
+{
+   GMainContext *mc = g_main_context_default();
+   while (!*done)
+   g_main_context_iteration(mc, TRUE);
+}
+
+static GnomeKeyringResult gnome_keyring_item_delete_sync(const char *keyring, 
guint32 id)
+{
+   int done = 0;
+   GnomeKeyringResult result;
+   gpointer data[] = { done, result };
+
+   gnome_keyring_item_delete(keyring, id, gnome_keyring_done_cb, data,
+   NULL);
+
+   wait_for_request_completion(done);
+
+   return result;
+}
+
+#endif
 #endif
 
 /*
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 13/16] contrib/git-credential-gnome-keyring.c: use glib messaging functions

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Rather than roll our own, let's use the messaging functions provided
by glib.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../gnome-keyring/git-credential-gnome-keyring.c   | 33 +++---
 1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 273c43b..b70bd53 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -25,7 +25,6 @@
 
 #include stdio.h
 #include string.h
-#include stdarg.h
 #include stdlib.h
 #include glib.h
 #include gnome-keyring.h
@@ -58,30 +57,6 @@ struct credential_operation
 #define CREDENTIAL_OP_END \
   { NULL,NULL }
 
-/*  common helper functions - */
-
-static inline void warning(const char *fmt, ...)
-{
-   va_list ap;
-
-   va_start(ap, fmt);
-   fprintf(stderr, warning: );
-   vfprintf(stderr, fmt, ap);
-   fprintf(stderr, \n );
-   va_end(ap);
-}
-
-static inline void error(const char *fmt, ...)
-{
-   va_list ap;
-
-   va_start(ap, fmt);
-   fprintf(stderr, error: );
-   vfprintf(stderr, fmt, ap);
-   fprintf(stderr, \n );
-   va_end(ap);
-}
-
 /* - GNOME Keyring functions - */
 
 /* create a special keyring option string, if path is given */
@@ -127,7 +102,7 @@ static int keyring_get(struct credential *c)
return EXIT_SUCCESS;
 
if (result != GNOME_KEYRING_RESULT_OK) {
-   error(%s,gnome_keyring_result_to_message(result));
+   g_critical(%s, gnome_keyring_result_to_message(result));
return EXIT_FAILURE;
}
 
@@ -220,7 +195,7 @@ static int keyring_erase(struct credential *c)
 
if (result != GNOME_KEYRING_RESULT_OK)
{
-   error(%s,gnome_keyring_result_to_message(result));
+   g_critical(%s, gnome_keyring_result_to_message(result));
return EXIT_FAILURE;
}
 
@@ -234,7 +209,7 @@ static int keyring_erase(struct credential *c)
 
if (result != GNOME_KEYRING_RESULT_OK)
{
-   error(%s,gnome_keyring_result_to_message(result));
+   g_critical(%s, gnome_keyring_result_to_message(result));
return EXIT_FAILURE;
}
 
@@ -292,7 +267,7 @@ static int credential_read(struct credential *c)
 
value = strchr(buf,'=');
if (!value) {
-   warning(invalid credential line: %s, key);
+   g_warning(invalid credential line: %s, key);
gnome_keyring_memory_free(buf);
return -1;
}
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 08/16] contrib/git-credential-gnome-keyring.c: set Gnome application name

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Since this is a Gnome application, let's set the application name to
something reasonable.  This will be displayed in Gnome dialog boxes
e.g. the one that prompts for the user's keyring password.

We add an include statement for glib.h and add the glib-2.0 cflags and
libs to the compilation arguments, but both of these are really noops
since glib is already a dependency of gnome-keyring.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 contrib/credential/gnome-keyring/Makefile   | 4 ++--
 contrib/credential/gnome-keyring/git-credential-gnome-keyring.c | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/contrib/credential/gnome-keyring/Makefile 
b/contrib/credential/gnome-keyring/Makefile
index e6561d8..c3c7c98 100644
--- a/contrib/credential/gnome-keyring/Makefile
+++ b/contrib/credential/gnome-keyring/Makefile
@@ -8,8 +8,8 @@ CFLAGS = -g -O2 -Wall
 -include ../../../config.mak.autogen
 -include ../../../config.mak
 
-INCS:=$(shell pkg-config --cflags gnome-keyring-1)
-LIBS:=$(shell pkg-config --libs gnome-keyring-1)
+INCS:=$(shell pkg-config --cflags gnome-keyring-1 glib-2.0)
+LIBS:=$(shell pkg-config --libs gnome-keyring-1 glib-2.0)
 
 SRCS:=$(MAIN).c
 OBJS:=$(SRCS:.c=.o)
diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 0d2c55e..43b19dd 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -28,6 +28,7 @@
 #include stdarg.h
 #include stdlib.h
 #include errno.h
+#include glib.h
 #include gnome-keyring.h
 
 /*
@@ -399,6 +400,8 @@ int main(int argc, char *argv[])
exit(EXIT_FAILURE);
}
 
+   g_set_application_name(Git Credential Helper);
+
/* lookup operation callback */
while (try_op-name  strcmp(argv[1], try_op-name))
try_op++;
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 01/16] contrib/git-credential-gnome-keyring.c: remove unnecessary pre-declarations

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

These are all defined before they are used, so it is not necessary to
pre-declare them.  Remove the pre-declarations.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../credential/gnome-keyring/git-credential-gnome-keyring.c | 13 -
 1 file changed, 13 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index f2cdefe..15b0a1c 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -46,11 +46,6 @@ struct credential
 #define CREDENTIAL_INIT \
   { NULL,NULL,0,NULL,NULL,NULL }
 
-void credential_init(struct credential *c);
-void credential_clear(struct credential *c);
-int  credential_read(struct credential *c);
-void credential_write(const struct credential *c);
-
 typedef int (*credential_op_cb)(struct credential*);
 
 struct credential_operation
@@ -62,14 +57,6 @@ struct credential_operation
 #define CREDENTIAL_OP_END \
   { NULL,NULL }
 
-/*
- * Table with operation callbacks is defined in concrete
- * credential helper implementation and contains entries
- * like { get, function_to_get_credential } terminated
- * by CREDENTIAL_OP_END.
- */
-struct credential_operation const credential_helper_ops[];
-
 /*  common helper functions - */
 
 static inline void free_password(char *password)
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 09/16] contrib/git-credential-gnome-keyring.c: use gnome helpers in keyring_object()

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Rather than carefully allocating memory for sprintf() to write into,
let's make use of the glib helper function g_strdup_printf(), which
makes things a lot easier and less error-prone.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../gnome-keyring/git-credential-gnome-keyring.c   | 14 +++---
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 43b19dd..b692e1f 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -112,21 +112,13 @@ static inline char *xstrdup(const char *str)
 /* create a special keyring option string, if path is given */
 static char* keyring_object(struct credential *c)
 {
-   char* object = NULL;
-
if (!c-path)
-   return object;
-
-   object = (char*) malloc(strlen(c-host)+strlen(c-path)+8);
-   if (!object)
-   die_errno(errno);
+   return NULL;
 
if (c-port)
-   sprintf(object,%s:%hd/%s,c-host,c-port,c-path);
-   else
-   sprintf(object,%s/%s,c-host,c-path);
+   return g_strdup_printf(%s:%hd/%s, c-host, c-port, c-path);
 
-   return object;
+   return g_strdup_printf(%s/%s, c-host, c-path);
 }
 
 static int keyring_get(struct credential *c)
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 05/16] contrib/git-credential-gnome-keyring.c: exit non-zero when called incorrectly

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

If the correct arguments were not specified, this program should exit
non-zero.  Let's do so.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 contrib/credential/gnome-keyring/git-credential-gnome-keyring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 3ff541c..04852d7 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -396,7 +396,7 @@ int main(int argc, char *argv[])
 
if (!argv[1]) {
usage(argv[0]);
-   goto out;
+   exit(EXIT_FAILURE);
}
 
/* lookup operation callback */
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 04/16] contrib/git-credential-gnome-keyring.c: add static where applicable

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Mark global variable and functions as static.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../gnome-keyring/git-credential-gnome-keyring.c   | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 809b1b7..3ff541c 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -128,7 +128,7 @@ static char* keyring_object(struct credential *c)
return object;
 }
 
-int keyring_get(struct credential *c)
+static int keyring_get(struct credential *c)
 {
char* object = NULL;
GList *entries;
@@ -178,7 +178,7 @@ int keyring_get(struct credential *c)
 }
 
 
-int keyring_store(struct credential *c)
+static int keyring_store(struct credential *c)
 {
guint32 item_id;
char  *object = NULL;
@@ -212,7 +212,7 @@ int keyring_store(struct credential *c)
return EXIT_SUCCESS;
 }
 
-int keyring_erase(struct credential *c)
+static int keyring_erase(struct credential *c)
 {
char  *object = NULL;
GList *entries;
@@ -277,7 +277,7 @@ int keyring_erase(struct credential *c)
  * Table with helper operation callbacks, used by generic
  * credential helper main function.
  */
-struct credential_operation const credential_helper_ops[] =
+static struct credential_operation const credential_helper_ops[] =
 {
{ get,   keyring_get   },
{ store, keyring_store },
@@ -287,12 +287,12 @@ struct credential_operation const credential_helper_ops[] 
=
 
 /* -- credential functions -- */
 
-void credential_init(struct credential *c)
+static void credential_init(struct credential *c)
 {
memset(c, 0, sizeof(*c));
 }
 
-void credential_clear(struct credential *c)
+static void credential_clear(struct credential *c)
 {
free(c-protocol);
free(c-host);
@@ -303,7 +303,7 @@ void credential_clear(struct credential *c)
credential_init(c);
 }
 
-int credential_read(struct credential *c)
+static int credential_read(struct credential *c)
 {
charbuf[1024];
ssize_t line_len = 0;
@@ -358,14 +358,14 @@ int credential_read(struct credential *c)
return 0;
 }
 
-void credential_write_item(FILE *fp, const char *key, const char *value)
+static void credential_write_item(FILE *fp, const char *key, const char *value)
 {
if (!value)
return;
fprintf(fp, %s=%s\n, key, value);
 }
 
-void credential_write(const struct credential *c)
+static void credential_write(const struct credential *c)
 {
/* only write username/password, if set */
credential_write_item(stdout, username, c-username);
-- 
1.8.4.rc4.6.gd19


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 04/15] contrib/git-credential-gnome-keyring.c: exit non-zero when called incorrectly

[Brandon Casey]

If the correct arguments were not specified, this program should exit
non-zero.  Let's do so.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 contrib/credential/gnome-keyring/git-credential-gnome-keyring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index ad23dbf..5459ba7 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -396,7 +396,7 @@ int main(int argc, char *argv[])
 
if (!argv[1]) {
usage(argv[0]);
-   goto out;
+   exit(EXIT_FAILURE);
}
 
/* lookup operation callback */
-- 
1.8.4.489.g545bc72

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 06/15] contrib/git-credential-gnome-keyring.c: strlen() returns size_t, not ssize_t

[Brandon Casey]

Also, initialization is not necessary since it is assigned before it is
used.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 contrib/credential/gnome-keyring/git-credential-gnome-keyring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 5779770..1081224 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -307,7 +307,7 @@ static void credential_clear(struct credential *c)
 static int credential_read(struct credential *c)
 {
charbuf[1024];
-   ssize_t line_len = 0;
+   size_t line_len;
char   *key  = buf;
char   *value;
 
-- 
1.8.4.489.g545bc72

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 00/15] Make Gnome Credential helper more Gnome-y and support ancient distros

[Brandon Casey]

A few cleanups, followed by improved usage of the glib library (no need
to reinvent the wheel when glib provides the necessary functionality), and
then the addition of support for RHEL 4.x and 5.x.

Brandon Casey (15):
  contrib/git-credential-gnome-keyring.c: remove unnecessary
pre-declarations
  contrib/git-credential-gnome-keyring.c: remove unused die() function
  contrib/git-credential-gnome-keyring.c: add static where applicable
  contrib/git-credential-gnome-keyring.c: exit non-zero when called
incorrectly
  contrib/git-credential-gnome-keyring.c: set Gnome application name
  contrib/git-credential-gnome-keyring.c: strlen() returns size_t, not
ssize_t
  contrib/git-credential-gnome-keyring.c: ensure buffer is non-empty
before accessing
  contrib/git-credential-gnome-keyring.c: use gnome helpers in
keyring_object()
  contrib/git-credential-gnome-keyring.c: use secure memory functions
for passwds
  contrib/git-credential-gnome-keyring.c: use secure memory for reading
passwords
  contrib/git-credential-gnome-keyring.c: use glib memory allocation
functions
  contrib/git-credential-gnome-keyring.c: use glib messaging functions
  contrib/git-credential-gnome-keyring.c: report failure to store
password
  contrib/git-credential-gnome-keyring.c: support ancient gnome-keyring
  contrib/git-credential-gnome-keyring.c: support really ancient
gnome-keyring

 contrib/credential/gnome-keyring/Makefile  |   4 +-
 .../gnome-keyring/git-credential-gnome-keyring.c   | 280 -
 2 files changed, 158 insertions(+), 126 deletions(-)

-- 
1.8.4.489.g545bc72

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/15] contrib/git-credential-gnome-keyring.c: remove unnecessary pre-declarations

[Brandon Casey]

These are all defined before they are used, so it is not necessary to
pre-declare them.  Remove the pre-declarations.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../credential/gnome-keyring/git-credential-gnome-keyring.c | 13 -
 1 file changed, 13 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index f2cdefe..15b0a1c 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -46,11 +46,6 @@ struct credential
 #define CREDENTIAL_INIT \
   { NULL,NULL,0,NULL,NULL,NULL }
 
-void credential_init(struct credential *c);
-void credential_clear(struct credential *c);
-int  credential_read(struct credential *c);
-void credential_write(const struct credential *c);
-
 typedef int (*credential_op_cb)(struct credential*);
 
 struct credential_operation
@@ -62,14 +57,6 @@ struct credential_operation
 #define CREDENTIAL_OP_END \
   { NULL,NULL }
 
-/*
- * Table with operation callbacks is defined in concrete
- * credential helper implementation and contains entries
- * like { get, function_to_get_credential } terminated
- * by CREDENTIAL_OP_END.
- */
-struct credential_operation const credential_helper_ops[];
-
 /*  common helper functions - */
 
 static inline void free_password(char *password)
-- 
1.8.4.489.g545bc72

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/15] contrib/git-credential-gnome-keyring.c: use secure memory for reading passwords

[Brandon Casey]

gnome-keyring provides functions to allocate non-pageable memory (if
possible).  Let's use them to allocate memory that may be used to hold
secure data read from the keyring.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../credential/gnome-keyring/git-credential-gnome-keyring.c  | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index ff2f48c..94a65b2 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -289,12 +289,14 @@ static void credential_clear(struct credential *c)
 
 static int credential_read(struct credential *c)
 {
-   charbuf[1024];
+   char*buf;
size_t line_len;
-   char   *key  = buf;
+   char   *key;
char   *value;
 
-   while (fgets(buf, sizeof(buf), stdin))
+   key = buf = gnome_keyring_memory_alloc(1024);
+
+   while (fgets(buf, 1024, stdin))
{
line_len = strlen(buf);
 
@@ -307,6 +309,7 @@ static int credential_read(struct credential *c)
value = strchr(buf,'=');
if(!value) {
warning(invalid credential line: %s, key);
+   gnome_keyring_memory_free(buf);
return -1;
}
*value++ = '\0';
@@ -339,6 +342,9 @@ static int credential_read(struct credential *c)
 * learn new lines, and the helpers are updated to match.
 */
}
+
+   gnome_keyring_memory_free(buf);
+
return 0;
 }
 
-- 
1.8.4.489.g545bc72

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/15] contrib/git-credential-gnome-keyring.c: use gnome helpers in keyring_object()

[Brandon Casey]

Rather than carefully allocating memory for sprintf() to write into,
let's make use of the glib helper function g_strdup_printf(), which
makes things a lot easier and less error-prone.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../gnome-keyring/git-credential-gnome-keyring.c   | 14 +++---
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 8ae2eab..7565765 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -112,21 +112,13 @@ static inline char *xstrdup(const char *str)
 /* create a special keyring option string, if path is given */
 static char* keyring_object(struct credential *c)
 {
-   char* object = NULL;
-
if (!c-path)
-   return object;
-
-   object = (char*) malloc(strlen(c-host)+strlen(c-path)+8);
-   if(!object)
-   die_errno(errno);
+   return NULL;
 
if(c-port)
-   sprintf(object,%s:%hd/%s,c-host,c-port,c-path);
-   else
-   sprintf(object,%s/%s,c-host,c-path);
+   return g_strdup_printf(%s:%hd/%s, c-host, c-port, c-path);
 
-   return object;
+   return g_strdup_printf(%s/%s, c-host, c-path);
 }
 
 static int keyring_get(struct credential *c)
-- 
1.8.4.489.g545bc72

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 11/15] contrib/git-credential-gnome-keyring.c: use glib memory allocation functions

[Brandon Casey]

Rather than roll our own, let's use the memory allocation/free routines
provided by glib.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../gnome-keyring/git-credential-gnome-keyring.c   | 48 --
 1 file changed, 16 insertions(+), 32 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 94a65b2..5b10e3e 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -27,7 +27,6 @@
 #include string.h
 #include stdarg.h
 #include stdlib.h
-#include errno.h
 #include glib.h
 #include gnome-keyring.h
 #include gnome-keyring-memory.h
@@ -83,21 +82,6 @@ static inline void error(const char *fmt, ...)
va_end(ap);
 }
 
-static inline void die_errno(int err)
-{
-   error(%s, strerror(err));
-   exit(EXIT_FAILURE);
-}
-
-static inline char *xstrdup(const char *str)
-{
-   char *ret = strdup(str);
-   if (!ret)
-   die_errno(errno);
-
-   return ret;
-}
-
 /* - GNOME Keyring functions - */
 
 /* create a special keyring option string, if path is given */
@@ -134,7 +118,7 @@ static int keyring_get(struct credential *c)
c-port,
entries);
 
-   free(object);
+   g_free(object);
 
if (result == GNOME_KEYRING_RESULT_NO_MATCH)
return EXIT_SUCCESS;
@@ -154,7 +138,7 @@ static int keyring_get(struct credential *c)
c-password = gnome_keyring_memory_strdup(password_data-password);
 
if (!c-username)
-   c-username = xstrdup(password_data-user);
+   c-username = g_strdup(password_data-user);
 
gnome_keyring_network_password_list_free(entries);
 
@@ -192,7 +176,7 @@ static int keyring_store(struct credential *c)
c-password,
item_id);
 
-   free(object);
+   g_free(object);
return EXIT_SUCCESS;
 }
 
@@ -226,7 +210,7 @@ static int keyring_erase(struct credential *c)
c-port,
entries);
 
-   free(object);
+   g_free(object);
 
if (result == GNOME_KEYRING_RESULT_NO_MATCH)
return EXIT_SUCCESS;
@@ -278,10 +262,10 @@ static void credential_init(struct credential *c)
 
 static void credential_clear(struct credential *c)
 {
-   free(c-protocol);
-   free(c-host);
-   free(c-path);
-   free(c-username);
+   g_free(c-protocol);
+   g_free(c-host);
+   g_free(c-path);
+   g_free(c-username);
gnome_keyring_memory_free(c-password);
 
credential_init(c);
@@ -315,22 +299,22 @@ static int credential_read(struct credential *c)
*value++ = '\0';
 
if (!strcmp(key, protocol)) {
-   free(c-protocol);
-   c-protocol = xstrdup(value);
+   g_free(c-protocol);
+   c-protocol = g_strdup(value);
} else if (!strcmp(key, host)) {
-   free(c-host);
-   c-host = xstrdup(value);
+   g_free(c-host);
+   c-host = g_strdup(value);
value = strrchr(c-host,':');
if (value) {
*value++ = '\0';
c-port = atoi(value);
}
} else if (!strcmp(key, path)) {
-   free(c-path);
-   c-path = xstrdup(value);
+   g_free(c-path);
+   c-path = g_strdup(value);
} else if (!strcmp(key, username)) {
-   free(c-username);
-   c-username = xstrdup(value);
+   g_free(c-username);
+   c-username = g_strdup(value);
} else if (!strcmp(key, password)) {
gnome_keyring_memory_free(c-password);
c-password = gnome_keyring_memory_strdup(value);
-- 
1.8.4.489.g545bc72

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 13/15] contrib/git-credential-gnome-keyring.c: report failure to store password

[Brandon Casey]

Produce an error message when we fail to store a password to the keyring.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 contrib/credential/gnome-keyring/git-credential-gnome-keyring.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index a6369a3..6fa1278 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -125,6 +125,7 @@ static int keyring_store(struct credential *c)
 {
guint32 item_id;
char  *object = NULL;
+   GnomeKeyringResult result;
 
/*
 * Sanity check that what we are storing is actually sensible.
@@ -139,7 +140,7 @@ static int keyring_store(struct credential *c)
 
object = keyring_object(c);
 
-   gnome_keyring_set_network_password_sync(
+   result = gnome_keyring_set_network_password_sync(
GNOME_KEYRING_DEFAULT,
c-username,
NULL /* domain */,
@@ -152,6 +153,12 @@ static int keyring_store(struct credential *c)
item_id);
 
g_free(object);
+
+   if (result != GNOME_KEYRING_RESULT_OK) {
+   g_critical(%s, gnome_keyring_result_to_message(result));
+   return EXIT_FAILURE;
+   }
+
return EXIT_SUCCESS;
 }
 
-- 
1.8.4.489.g545bc72

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/15] contrib/git-credential-gnome-keyring.c: set Gnome application name

[Brandon Casey]

Since this is a Gnome application, let's set the application name to
something reasonable.  This will be displayed in Gnome dialog boxes
e.g. the one that prompts for the user's keyring password.

We add an include statement for glib.h and add the glib-2.0 cflags and
libs to the compilation arguments, but both of these are really noops
since glib is already a dependency of gnome-keyring.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 contrib/credential/gnome-keyring/Makefile   | 4 ++--
 contrib/credential/gnome-keyring/git-credential-gnome-keyring.c | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/contrib/credential/gnome-keyring/Makefile 
b/contrib/credential/gnome-keyring/Makefile
index e6561d8..c3c7c98 100644
--- a/contrib/credential/gnome-keyring/Makefile
+++ b/contrib/credential/gnome-keyring/Makefile
@@ -8,8 +8,8 @@ CFLAGS = -g -O2 -Wall
 -include ../../../config.mak.autogen
 -include ../../../config.mak
 
-INCS:=$(shell pkg-config --cflags gnome-keyring-1)
-LIBS:=$(shell pkg-config --libs gnome-keyring-1)
+INCS:=$(shell pkg-config --cflags gnome-keyring-1 glib-2.0)
+LIBS:=$(shell pkg-config --libs gnome-keyring-1 glib-2.0)
 
 SRCS:=$(MAIN).c
 OBJS:=$(SRCS:.c=.o)
diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 5459ba7..5779770 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -28,6 +28,7 @@
 #include stdarg.h
 #include stdlib.h
 #include errno.h
+#include glib.h
 #include gnome-keyring.h
 
 /*
@@ -399,6 +400,8 @@ int main(int argc, char *argv[])
exit(EXIT_FAILURE);
}
 
+   g_set_application_name(Git Credential Helper);
+
/* lookup operation callback */
while(try_op-name  strcmp(argv[1], try_op-name))
try_op++;
-- 
1.8.4.489.g545bc72

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/15] contrib/git-credential-gnome-keyring.c: remove unused die() function

[Brandon Casey]

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../credential/gnome-keyring/git-credential-gnome-keyring.c| 10 --
 1 file changed, 10 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 15b0a1c..4334f23 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -91,16 +91,6 @@ static inline void error(const char *fmt, ...)
va_end(ap);
 }
 
-static inline void die(const char *fmt, ...)
-{
-   va_list ap;
-
-   va_start(ap,fmt);
-   error(fmt, ap);
-   va_end(ap);
-   exit(EXIT_FAILURE);
-}
-
 static inline void die_errno(int err)
 {
error(%s, strerror(err));
-- 
1.8.4.489.g545bc72

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 12/15] contrib/git-credential-gnome-keyring.c: use glib messaging functions

[Brandon Casey]

Rather than roll our own, let's use the messaging functions provided
by glib.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../gnome-keyring/git-credential-gnome-keyring.c   | 33 +++---
 1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 5b10e3e..a6369a3 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -25,7 +25,6 @@
 
 #include stdio.h
 #include string.h
-#include stdarg.h
 #include stdlib.h
 #include glib.h
 #include gnome-keyring.h
@@ -58,30 +57,6 @@ struct credential_operation
 #define CREDENTIAL_OP_END \
   { NULL,NULL }
 
-/*  common helper functions - */
-
-static inline void warning(const char *fmt, ...)
-{
-   va_list ap;
-
-   va_start(ap, fmt);
-   fprintf(stderr, warning: );
-   vfprintf(stderr, fmt, ap);
-   fprintf(stderr, \n );
-   va_end(ap);
-}
-
-static inline void error(const char *fmt, ...)
-{
-   va_list ap;
-
-   va_start(ap, fmt);
-   fprintf(stderr, error: );
-   vfprintf(stderr, fmt, ap);
-   fprintf(stderr, \n );
-   va_end(ap);
-}
-
 /* - GNOME Keyring functions - */
 
 /* create a special keyring option string, if path is given */
@@ -127,7 +102,7 @@ static int keyring_get(struct credential *c)
return EXIT_SUCCESS;
 
if (result != GNOME_KEYRING_RESULT_OK) {
-   error(%s,gnome_keyring_result_to_message(result));
+   g_critical(%s, gnome_keyring_result_to_message(result));
return EXIT_FAILURE;
}
 
@@ -220,7 +195,7 @@ static int keyring_erase(struct credential *c)
 
if (result != GNOME_KEYRING_RESULT_OK)
{
-   error(%s,gnome_keyring_result_to_message(result));
+   g_critical(%s, gnome_keyring_result_to_message(result));
return EXIT_FAILURE;
}
 
@@ -234,7 +209,7 @@ static int keyring_erase(struct credential *c)
 
if (result != GNOME_KEYRING_RESULT_OK)
{
-   error(%s,gnome_keyring_result_to_message(result));
+   g_critical(%s, gnome_keyring_result_to_message(result));
return EXIT_FAILURE;
}
 
@@ -292,7 +267,7 @@ static int credential_read(struct credential *c)
 
value = strchr(buf,'=');
if(!value) {
-   warning(invalid credential line: %s, key);
+   g_warning(invalid credential line: %s, key);
gnome_keyring_memory_free(buf);
return -1;
}
-- 
1.8.4.489.g545bc72

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 14/15] contrib/git-credential-gnome-keyring.c: support ancient gnome-keyring

[Brandon Casey]

The gnome-keyring lib distributed with RHEL 5.X is ancient and does
not provide a few of the functions/defines that more recent versions
do, but mostly the API is the same.  Let's provide the missing bits
via macro definitions and function implementation.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../gnome-keyring/git-credential-gnome-keyring.c   | 58 ++
 1 file changed, 58 insertions(+)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 6fa1278..f8f4df9 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -28,8 +28,66 @@
 #include stdlib.h
 #include glib.h
 #include gnome-keyring.h
+
+#ifdef GNOME_KEYRING_DEFAULT
+
+   /* Modern gnome-keyring */
+
 #include gnome-keyring-memory.h
 
+#else
+
+   /*
+* Support ancient gnome-keyring, circ. RHEL 5.X.
+* GNOME_KEYRING_DEFAULT seems to have been introduced with Gnome 2.22,
+* and the other features roughly around Gnome 2.20, 6 months before.
+* Ubuntu 8.04 used Gnome 2.22 (I think).  Not sure any distro used 2.20.
+* So the existence/non-existence of GNOME_KEYRING_DEFAULT seems like
+* a decent thing to use as an indicator.
+*/
+
+#define GNOME_KEYRING_DEFAULT NULL
+
+/*
+ * ancient gnome-keyring returns DENIED when an entry is not found.
+ * Setting NO_MATCH to DENIED will prevent us from reporting denied
+ * errors during get and erase operations, but we will still report
+ * DENIED errors during a store.
+ */
+#define GNOME_KEYRING_RESULT_NO_MATCH GNOME_KEYRING_RESULT_DENIED
+
+#define gnome_keyring_memory_alloc g_malloc
+#define gnome_keyring_memory_free gnome_keyring_free_password
+#define gnome_keyring_memory_strdup g_strdup
+
+static const char* gnome_keyring_result_to_message(GnomeKeyringResult result)
+{
+   switch (result) {
+   case GNOME_KEYRING_RESULT_OK:
+   return OK;
+   case GNOME_KEYRING_RESULT_DENIED:
+   return Denied;
+   case GNOME_KEYRING_RESULT_NO_KEYRING_DAEMON:
+   return No Keyring Daemon;
+   case GNOME_KEYRING_RESULT_ALREADY_UNLOCKED:
+   return Already UnLocked;
+   case GNOME_KEYRING_RESULT_NO_SUCH_KEYRING:
+   return No Such Keyring;
+   case GNOME_KEYRING_RESULT_BAD_ARGUMENTS:
+   return Bad Arguments;
+   case GNOME_KEYRING_RESULT_IO_ERROR:
+   return IO Error;
+   case GNOME_KEYRING_RESULT_CANCELLED:
+   return Cancelled;
+   case GNOME_KEYRING_RESULT_ALREADY_EXISTS:
+   return Already Exists;
+   default:
+   return Unknown Error;
+   }
+}
+
+#endif
+
 /*
  * This credential struct and API is simplified from git's credential.{h,c}
  */
-- 
1.8.4.489.g545bc72

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/15] contrib/git-credential-gnome-keyring.c: ensure buffer is non-empty before accessing

[Brandon Casey]

Ensure buffer length is non-zero before attempting to access the last
element.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 contrib/credential/gnome-keyring/git-credential-gnome-keyring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 1081224..8ae2eab 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -315,7 +315,7 @@ static int credential_read(struct credential *c)
{
line_len = strlen(buf);
 
-   if(buf[line_len-1]=='\n')
+   if(line_len  buf[line_len-1] == '\n')
buf[--line_len]='\0';
 
if(!line_len)
-- 
1.8.4.489.g545bc72

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 15/15] contrib/git-credential-gnome-keyring.c: support really ancient gnome-keyring

[Brandon Casey]

The gnome-keyring lib (0.4) distributed with RHEL 4.X is really ancient
and does not provide most of the synchronous functions that even ancient
releases do.  Thankfully, we're only using one function that is missing.
Let's emulate gnome_keyring_item_delete_sync() by calling the asynchronous
function and then triggering the event loop processing until our
callback is called.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../gnome-keyring/git-credential-gnome-keyring.c   | 39 ++
 1 file changed, 39 insertions(+)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index f8f4df9..ce2ddee 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -86,6 +86,45 @@ static const char* 
gnome_keyring_result_to_message(GnomeKeyringResult result)
}
 }
 
+/*
+ * Just a guess to support RHEL 4.X.
+ * Glib 2.8 was roughly Gnome 2.12 ?
+ * Which was released with gnome-keyring 0.4.3 ??
+ */
+#if GLIB_MAJOR_VERSION == 2  GLIB_MINOR_VERSION  8
+
+static void gnome_keyring_done_cb(GnomeKeyringResult result, gpointer 
user_data)
+{
+   gpointer *data = (gpointer*) user_data;
+   int *done = (int*) data[0];
+   GnomeKeyringResult *r = (GnomeKeyringResult*) data[1];
+
+   *r = result;
+   *done = 1;
+}
+
+static void wait_for_request_completion(int *done)
+{
+   GMainContext *mc = g_main_context_default();
+   while (!*done)
+   g_main_context_iteration(mc, TRUE);
+}
+
+static GnomeKeyringResult gnome_keyring_item_delete_sync(const char *keyring, 
guint32 id)
+{
+   int done = 0;
+   GnomeKeyringResult result;
+   gpointer data[] = { done, result };
+
+   gnome_keyring_item_delete(keyring, id, gnome_keyring_done_cb, data,
+   NULL);
+
+   wait_for_request_completion(done);
+
+   return result;
+}
+
+#endif
 #endif
 
 /*
-- 
1.8.4.489.g545bc72

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/15] contrib/git-credential-gnome-keyring.c: use secure memory functions for passwds

[Brandon Casey]

gnome-keyring provides functions for allocating non-pageable memory (if
possible) intended to be used for storing passwords.  Let's use them.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../gnome-keyring/git-credential-gnome-keyring.c| 21 ++---
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 7565765..ff2f48c 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -30,6 +30,7 @@
 #include errno.h
 #include glib.h
 #include gnome-keyring.h
+#include gnome-keyring-memory.h
 
 /*
  * This credential struct and API is simplified from git's credential.{h,c}
@@ -60,16 +61,6 @@ struct credential_operation
 
 /*  common helper functions - */
 
-static inline void free_password(char *password)
-{
-   char *c = password;
-   if (!password)
-   return;
-
-   while (*c) *c++ = '\0';
-   free(password);
-}
-
 static inline void warning(const char *fmt, ...)
 {
va_list ap;
@@ -159,8 +150,8 @@ static int keyring_get(struct credential *c)
/* pick the first one from the list */
password_data = (GnomeKeyringNetworkPasswordData *) entries-data;
 
-   free_password(c-password);
-   c-password = xstrdup(password_data-password);
+   gnome_keyring_memory_free(c-password);
+   c-password = gnome_keyring_memory_strdup(password_data-password);
 
if (!c-username)
c-username = xstrdup(password_data-user);
@@ -291,7 +282,7 @@ static void credential_clear(struct credential *c)
free(c-host);
free(c-path);
free(c-username);
-   free_password(c-password);
+   gnome_keyring_memory_free(c-password);
 
credential_init(c);
 }
@@ -338,8 +329,8 @@ static int credential_read(struct credential *c)
free(c-username);
c-username = xstrdup(value);
} else if (!strcmp(key, password)) {
-   free_password(c-password);
-   c-password = xstrdup(value);
+   gnome_keyring_memory_free(c-password);
+   c-password = gnome_keyring_memory_strdup(value);
while (*value) *value++ = '\0';
}
/*
-- 
1.8.4.489.g545bc72

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/15] contrib/git-credential-gnome-keyring.c: add static where applicable

[Brandon Casey]

Mark global variable and functions as static.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 .../gnome-keyring/git-credential-gnome-keyring.c   | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c 
b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
index 4334f23..ad23dbf 100644
--- a/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
+++ b/contrib/credential/gnome-keyring/git-credential-gnome-keyring.c
@@ -128,7 +128,7 @@ static char* keyring_object(struct credential *c)
return object;
 }
 
-int keyring_get(struct credential *c)
+static int keyring_get(struct credential *c)
 {
char* object = NULL;
GList *entries;
@@ -178,7 +178,7 @@ int keyring_get(struct credential *c)
 }
 
 
-int keyring_store(struct credential *c)
+static int keyring_store(struct credential *c)
 {
guint32 item_id;
char  *object = NULL;
@@ -212,7 +212,7 @@ int keyring_store(struct credential *c)
return EXIT_SUCCESS;
 }
 
-int keyring_erase(struct credential *c)
+static int keyring_erase(struct credential *c)
 {
char  *object = NULL;
GList *entries;
@@ -277,7 +277,7 @@ int keyring_erase(struct credential *c)
  * Table with helper operation callbacks, used by generic
  * credential helper main function.
  */
-struct credential_operation const credential_helper_ops[] =
+static struct credential_operation const credential_helper_ops[] =
 {
{ get,   keyring_get   },
{ store, keyring_store },
@@ -287,12 +287,12 @@ struct credential_operation const credential_helper_ops[] 
=
 
 /* -- credential functions -- */
 
-void credential_init(struct credential *c)
+static void credential_init(struct credential *c)
 {
memset(c, 0, sizeof(*c));
 }
 
-void credential_clear(struct credential *c)
+static void credential_clear(struct credential *c)
 {
free(c-protocol);
free(c-host);
@@ -303,7 +303,7 @@ void credential_clear(struct credential *c)
credential_init(c);
 }
 
-int credential_read(struct credential *c)
+static int credential_read(struct credential *c)
 {
charbuf[1024];
ssize_t line_len = 0;
@@ -358,14 +358,14 @@ int credential_read(struct credential *c)
return 0;
 }
 
-void credential_write_item(FILE *fp, const char *key, const char *value)
+static void credential_write_item(FILE *fp, const char *key, const char *value)
 {
if (!value)
return;
fprintf(fp, %s=%s\n, key, value);
 }
 
-void credential_write(const struct credential *c)
+static void credential_write(const struct credential *c)
 {
/* only write username/password, if set */
credential_write_item(stdout, username, c-username);
-- 
1.8.4.489.g545bc72

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] t9902-completion.sh: old Bash still does not support array+=('') notation

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Old Bash (3.0) which is distributed with RHEL 4.X and other ancient
platforms that are still in wide use, does not understand the
array+=() notation.  Let's use an explicit assignment to the new array
element which works everywhere, like:

   array[${#array[@]}+1]=''

The right-hand side '' is not strictly necessary, but in this case
I think it is more clear.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 t/t9902-completion.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 272a071..2d4beb5 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -69,7 +69,7 @@ run_completion ()
local -a COMPREPLY _words
local _cword
_words=( $1 )
-   test ${1: -1} = ' '  _words+=('')
+   test ${1: -1} = ' '  _words[${#_words[@]}+1]=''
(( _cword = ${#_words[@]} - 1 ))
__git_wrap__git_main  print_comp
 }
-- 
1.8.4.rc0.2.g6cf5c31


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] git-completion.bash: use correct Bash/Zsh array length syntax

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

The syntax for retrieving the number of elements in an array is:

   ${#name[@]}

Signed-off-by: Brandon Casey draf...@gmail.com
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 5da920e..e1b7313 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2580,7 +2580,7 @@ if [[ -n ${ZSH_VERSION-} ]]; then
--*=*|*.) ;;
*) c=$c  ;;
esac
-   array[$#array+1]=$c
+   array[${#array[@]}+1]=$c
done
compset -P '*[=:]'
compadd -Q -S '' -p ${2-} -a -- array  _ret=0
-- 
1.8.4.rc0.2.g6cf5c31


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] Revert bash prompt: avoid command substitution when finalizing gitstring

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

This reverts commit 69a8141a5d81925b7e08cb228535e9ea4a7a02e3.

Old Bash (3.0) which is distributed with RHEL 4.X and other ancient
platforms that are still in wide use, does not have a printf that
supports -v.  Let's revert this patch and go back to using printf
in the traditional way.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 contrib/completion/git-prompt.sh | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index a81ef5a..7698ec4 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -433,11 +433,7 @@ __git_ps1 ()
local gitstring=$c${b##refs/heads/}${f:+$z$f}$r$p
 
if [ $pcmode = yes ]; then
-   if [[ -n ${ZSH_VERSION-} ]]; then
-   gitstring=$(printf -- $printf_format $gitstring)
-   else
-   printf -v gitstring -- $printf_format $gitstring
-   fi
+   gitstring=$(printf -- $printf_format $gitstring)
PS1=$ps1pc_start$gitstring$ps1pc_end
else
printf -- $printf_format $gitstring
-- 
1.8.4.rc0.2.g6cf5c31


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] Revert bash prompt: avoid command substitution when finalizing gitstring

[Brandon Casey]

On Wed, Aug 21, 2013 at 2:47 PM, Junio C Hamano gits...@pobox.com wrote:
 Brandon Casey bca...@nvidia.com writes:

 From: Brandon Casey draf...@gmail.com

 This reverts commit 69a8141a5d81925b7e08cb228535e9ea4a7a02e3.

 Old Bash (3.0) which is distributed with RHEL 4.X and other ancient
 platforms that are still in wide use, does not have a printf that
 supports -v.  Let's revert this patch and go back to using printf
 in the traditional way.

 Signed-off-by: Brandon Casey draf...@gmail.com
 ---

 Is this something you can detect at load-time once, store the result
 in a private variable and then switch on it at runtime, something
 along the lines of...


 # on load...
 printf -v __git_printf_supports_v -- %s yes /dev/null 21

 ...

 if test ${__git_printf_supports_v} = yes
 then
 printf -v gitstring -- $printf_format $gitstring
 else
 gitstring=$(printf -- $printf_format $gitstring)
 fi

Yes, that appears to work.

-Brandon


  contrib/completion/git-prompt.sh | 6 +-
  1 file changed, 1 insertion(+), 5 deletions(-)

 diff --git a/contrib/completion/git-prompt.sh 
 b/contrib/completion/git-prompt.sh
 index a81ef5a..7698ec4 100644
 --- a/contrib/completion/git-prompt.sh
 +++ b/contrib/completion/git-prompt.sh
 @@ -433,11 +433,7 @@ __git_ps1 ()
   local gitstring=$c${b##refs/heads/}${f:+$z$f}$r$p

   if [ $pcmode = yes ]; then
 - if [[ -n ${ZSH_VERSION-} ]]; then
 - gitstring=$(printf -- $printf_format $gitstring)
 - else
 - printf -v gitstring -- $printf_format $gitstring
 - fi
 + gitstring=$(printf -- $printf_format $gitstring)
   PS1=$ps1pc_start$gitstring$ps1pc_end
   else
   printf -- $printf_format $gitstring
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] Revert bash prompt: avoid command substitution when finalizing gitstring

[Brandon Casey]

On Wed, Aug 21, 2013 at 5:22 PM, Junio C Hamano gits...@pobox.com wrote:
 Brandon Casey draf...@gmail.com writes:

 On Wed, Aug 21, 2013 at 2:47 PM, Junio C Hamano gits...@pobox.com wrote:

 # on load...
 printf -v __git_printf_supports_v -- %s yes /dev/null 21

 ...

 if test ${__git_printf_supports_v} = yes
 then
 printf -v gitstring -- $printf_format $gitstring
 else
 gitstring=$(printf -- $printf_format $gitstring)
 fi

 Yes, that appears to work.

 A real patch needs to be a bit more careful, though.  The variable
 needs to be cleared before all of the above,

Agreed.

 and the testing would
 want to consider that the variable may not be set (i.e. use
 ${var-} when checking).

Why is ${var-} necessary?  Wouldn't that be equivalent to ${var}
or $var?  We obviously wouldn't want to do 'if test $var = yes', but
I would have thought it was sufficient to wrap the variable
dereference in quotes as your original did.

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] contrib/git-prompt.sh: handle missing 'printf -v' more gracefully

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Old Bash (3.0) which is distributed with RHEL 4.X and other ancient
platforms that are still in wide use, do not have a printf that
supports -v.  Neither does Zsh (which is already handled in the code).

As suggested by Junio, let's test whether printf supports the -v
option and store the result.  Then later, we can use it to
determine whether 'printf -v' can be used, or whether printf
must be called in a subshell.

Signed-off-by: Brandon Casey draf...@gmail.com
---

This replaces [PATCH 3/3] Revert bash prompt: avoid command substitution
when finalizing gitstring.

This may or may not need to be updated to use ${var-} depending on
your response to my other email, but this seems sufficient.

-Brandon

 contrib/completion/git-prompt.sh | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index a81ef5a..639888a 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -84,6 +84,10 @@
 # the colored output of git status -sb and are available only when
 # using __git_ps1 for PROMPT_COMMAND or precmd.
 
+# check whether printf supports -v
+__git_printf_supports_v=
+printf -v __git_printf_supports_v -- '%s' yes /dev/null 21
+
 # stores the divergence from upstream in $p
 # used by GIT_PS1_SHOWUPSTREAM
 __git_ps1_show_upstream ()
@@ -433,10 +437,10 @@ __git_ps1 ()
local gitstring=$c${b##refs/heads/}${f:+$z$f}$r$p
 
if [ $pcmode = yes ]; then
-   if [[ -n ${ZSH_VERSION-} ]]; then
-   gitstring=$(printf -- $printf_format $gitstring)
-   else
+   if test $__git_printf_supports_v = yes; then
printf -v gitstring -- $printf_format $gitstring
+   else
+   gitstring=$(printf -- $printf_format $gitstring)
fi
PS1=$ps1pc_start$gitstring$ps1pc_end
else
-- 
1.8.4.rc0.2.g6cf5c31


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] contrib/git-prompt.sh: handle missing 'printf -v' more gracefully

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Old Bash (3.0) which is distributed with RHEL 4.X and other ancient
platforms that are still in wide use, do not have a printf that
supports -v.  Neither does Zsh (which is already handled in the code).

As suggested by Junio, let's test whether printf supports the -v
option and store the result.  Then later, we can use it to
determine whether 'printf -v' can be used, or whether printf
must be called in a subshell.

Signed-off-by: Brandon Casey draf...@gmail.com
---


On 8/21/2013 6:27 PM, Junio C Hamano wrote: Brandon Casey draf...@gmail.com 
writes:

 Why is ${var-} necessary?  Wouldn't that be equivalent to ${var}
 or $var?

 set -u

Ah.  Thanks.  Updated.  Also minor tweak to use [ ] instead of test ...
to conform with the rest of the script.

-Brandon


 contrib/completion/git-prompt.sh | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index a81ef5a..ca7fb35 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -84,6 +84,10 @@
 # the colored output of git status -sb and are available only when
 # using __git_ps1 for PROMPT_COMMAND or precmd.
 
+# check whether printf supports -v
+__git_printf_supports_v=
+printf -v __git_printf_supports_v -- '%s' yes /dev/null 21
+
 # stores the divergence from upstream in $p
 # used by GIT_PS1_SHOWUPSTREAM
 __git_ps1_show_upstream ()
@@ -433,10 +437,10 @@ __git_ps1 ()
local gitstring=$c${b##refs/heads/}${f:+$z$f}$r$p
 
if [ $pcmode = yes ]; then
-   if [[ -n ${ZSH_VERSION-} ]]; then
-   gitstring=$(printf -- $printf_format $gitstring)
-   else
+   if [ ${__git_printf_supports_v-} = yes ]; then
printf -v gitstring -- $printf_format $gitstring
+   else
+   gitstring=$(printf -- $printf_format $gitstring)
fi
PS1=$ps1pc_start$gitstring$ps1pc_end
else
-- 
1.8.4.rc0.2.g6cf5c31


---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [regression] Re: git-cat-file --batch reversion; cannot query filenames with spaces

[Brandon Casey]

On Fri, Aug 2, 2013 at 8:27 AM, Joey Hess j...@kitenet.net wrote:
 Jeff King wrote:
 By the way, Joey, I am not sure how safe git cat-file --batch-check is
 for arbitrary filenames. In particular, I don't know how it would react
 to a filename with an embedded newline (and I do not think it will undo
 quoting). Certainly that does not excuse this regression; even if what
 you are doing is not 100% reliable, it is good enough in sane situations
 and we should not be breaking it. But you may want to double-check the
 behavior of your scripts in such a case, and we may need to add a -z
 to support it reliably.

 Yes, I would prefer to have a -z mode. I think my code otherwise handles
 newlines.

 Thanks for the quick fix. I agree that only enabling the behavior with
 %{rest} makes sense.

 --
 see shy jo

/methinks we've identified a gap in our test coverage.  Care to add a
test that covers the functionality that git-annex depends on?

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] sha1_file: introduce close_one_pack() to close packs on fd pressure

[Brandon Casey]

On Fri, Aug 2, 2013 at 9:26 AM, Junio C Hamano gits...@pobox.com wrote:
 Brandon Casey draf...@gmail.com writes:

 +/*
 + * The LRU pack is the one with the oldest MRU window, preferring packs
 + * with no used windows, or the oldest mtime if it has no windows allocated.
 + */
 +static void find_lru_pack(struct packed_git *p, struct packed_git **lru_p, 
 struct pack_window **mru_w, int *accept_windows_inuse)
 +{
 + struct pack_window *w, *this_mru_w;
 + int has_windows_inuse = 0;
 +
 + /*
 +  * Reject this pack if it has windows and the previously selected
 +  * one does not.  If this pack does not have windows, reject
 +  * it if the pack file is newer than the previously selected one.
 +  */
 + if (*lru_p  !*mru_w  (p-windows || p-mtime  (*lru_p)-mtime))
 + return;
 +
 + for (w = this_mru_w = p-windows; w; w = w-next) {
 + /*
 +  * Reject this pack if any of its windows are in use,
 +  * but the previously selected pack did not have any
 +  * inuse windows.  Otherwise, record that this pack
 +  * has windows in use.
 +  */
 + if (w-inuse_cnt) {
 + if (*accept_windows_inuse)
 + has_windows_inuse = 1;
 + else
 + return;
 + }
 +
 + if (w-last_used  this_mru_w-last_used)
 + this_mru_w = w;
 +
 + /*
 +  * Reject this pack if it has windows that have been
 +  * used more recently than the previously selected pack.
 +  * If the previously selected pack had windows inuse and
 +  * we have not encountered a window in this pack that is
 +  * inuse, skip this check since we prefer a pack with no
 +  * inuse windows to one that has inuse windows.
 +  */
 + if (*mru_w  *accept_windows_inuse == has_windows_inuse 
 + this_mru_w-last_used  (*mru_w)-last_used)
 + return;

 The *accept_windows_inuse == has_windows_inuse part is hard to
 grok, together with the fact that this statement is evaluated for
 each and every w, even though it is about this_mru_w and that
 variable is not updated in every iteration of the loop.  Can you
 clarify/simplify this part of the code a bit more?

 For example, would the above be equivalent to this?

 if (w-last_used  this_mru_w-last_used)
 continue;

 this_mru_w = w;
 if (has_windows_inuse  *mru_w 
 w-last_used  (*mru_w)-last_used)
 return;

 That is, if we already know a more recently used window in this
 pack, we do not have to do anything to maintain mru_w.  Otherwise,
 remember that this window is the most recently used one in this
 pack, and if it is newer than the newest one from the pack we are
 going to pick, we refrain from picking this pack.

 But we do not reject ourselves if we haven't seen a window that is
 in use (yet).

No that wouldn't be the same.  The function of *accept_windows_inuse
== has_windows_inuse and the testing of this_mru_w in every loop
rather than w, is too subtle.  I tried to draw attention to it in the
comment, but I agree it's not enough.

The case that your example would not catch is when the new pack's mru
window has already been found, but has_windows_inuse is not set until
later.  When has_windows_inuse is later set, we need to test
this_mru_w regardless of whether we have just assigned it.

For example, if mru_w points to a pack with last_used == 11 and
*accept_windows_inuse = 1, and p-windows looks like this:

   last_used  in_use
   12 0
   10 1

Then the first time through the loop, this_mru_w would be set to the
first window with last_used equal to 12.  The if statement that tests
this_mru_w-last_used  (*mru_w)-last_used would be skipped since
has_windows_inuse would still be 0.  The second time through the loop,
this_mru_w would _not_ be reset, but has_windows_inuse _would_ be set.
 This time, we would want to enter the last for loop so that we can
reject the pack.

I'll try to rework this loop or add comments to clarify.

-Brandon


 + }
 +
 + /*
 +  * Select this pack.
 +  */
 + *mru_w = this_mru_w;
 + *lru_p = p;
 + *accept_windows_inuse = has_windows_inuse;
 +}
 +
 +static int close_one_pack(void)
 +{
 + struct packed_git *p, *lru_p = NULL;
 + struct pack_window *mru_w = NULL;
 + int accept_windows_inuse = 1;
 +
 + for (p = packed_git; p; p = p-next) {
 + if (p-pack_fd == -1)
 + continue;
 + find_lru_pack(p, lru_p, mru_w, accept_windows_inuse);
 + }
 +
 + if (lru_p) {
 + close(lru_p-pack_fd);
 + pack_open_fds--;
 + lru_p-pack_fd = -1;
 + return 1

Re: [PATCH v2 1/2] sha1_file: introduce close_one_pack() to close packs on fd pressure

[Brandon Casey]

On Thu, Aug 1, 2013 at 10:12 AM, Junio C Hamano gits...@pobox.com wrote:
 Brandon Casey bca...@nvidia.com writes:

 If the refs are loose, then upload-pack will read each ref from the
 pack (allocating one or more mmap windows) so it can peel tags and
 advertise the underlying object. If the refs are packed and peeled,
 then upload-pack will use the peeled sha1 in the packed-refs file and
 will not need to read from the pack files, so no mmap windows will be
 allocated and just like with receive-pack, unuse_one_window() will

 Even though what it says is not incorrect, the phrasing around here,
 especially so it can, confused me in my first reading.  It reads
 objects in order to peel and advertise (and as a side-effect it
 can lead to windows into packs that eventually help relieaving the
 fd pressure), but a quick scan led me to misread it as so it can do
 peel and advertise just fine, which misses the point, because it is
 not like we are having trouble peeling and advertising.

 Also, the objects at the tips of refs and the objects they point at
 may be loose objects, which is very likely for branch tips.  The fd
 pressure will not be relieved in such a case even if these refs were
 packed.

 I've tentatively reworded the above section like so:

 ... If the refs are loose, then upload-pack will read each ref
 from the object database (if the object is in a pack, allocating
 one or more mmap windows for it) in order to peel tags and
 advertise the underlying object.  But when the refs are packed
 and peeled, upload-pack will use the peeled sha1 in the
 packed-refs file and will not need to read from the pack files,
 so no mmap windows will be allocated ...

Thanks.

 +static int close_one_pack(void)
 +{
 + struct packed_git *p, *lru_p = NULL;
 + struct pack_window *mru_w = NULL;
 +
 + for (p = packed_git; p; p = p-next) {
 + if (p-pack_fd == -1)
 + continue;
 + find_lru_pack(p, lru_p, mru_w);
 + }
 +
 + if (lru_p) {
 + close_pack_windows(lru_p);
 + close(lru_p-pack_fd);
 + pack_open_fds--;
 + lru_p-pack_fd = -1;
 + if (lru_p == last_found_pack)
 + last_found_pack = NULL;
 + return 1;
 + }
 +
 + return 0;
 +}

 OK, so in this codepath where we know we are under fd pressure, we
 find the pack that is least recently used that can be closed, and
 use close_pack_windows() to reclaim all of its open windows (if
 any),

I've been looking closer at uses of p-windows everywhere, and it
seems that we always open_packed_git() before we try to create new
windows.  There doesn't seem to be any reason that we can't continue
to use the existing open windows even after closing the pack file.  We
obviously do this when the window spans the entire file.

So, I'm thinking we can drop the close_pack_windows() and refrain from
resetting last_found_pack, so the last block will become simply:

 + if (lru_p) {
 + close(lru_p-pack_fd);
 + pack_open_fds--;
 + lru_p-pack_fd = -1;
 + return 1;
 + }

If the pack file needs to be reopened later and it has been rewritten
in the mean time, open_packed_git_1() should notice when it compares
either the file size or the pack's sha1 checksum to what was
previously read from the pack index.  So this seems safe.

If we don't need to close_pack_windows(), find_lru_pack() doesn't
strictly need to reject packs that have windows in use.  I think the
algorithm can be tweaked to prefer to close packs that have no windows
in use, but still select them for closing if not.  The order of
preference would look like:

   1. pack with no open windows, oldest mtime
   2. pack with oldest MRU window but none in use
   3. pack with oldest MRU window

 which takes care of the accounting for pack_mapped and
 pack_open_windows, but we need to do the pack_open_fds accounting
 here ourselves.  Makes sense to me.

 Thanks.

Sorry about the additional reroll.  I'll make the above changes and resubmit.

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] sha1_file: introduce close_one_pack() to close packs on fd pressure

[Brandon Casey]

On Thu, Aug 1, 2013 at 11:39 AM, Junio C Hamano gits...@pobox.com wrote:
 Brandon Casey draf...@gmail.com writes:

 I've been looking closer at uses of p-windows everywhere, and it
 seems that we always open_packed_git() before we try to create new
 windows.  There doesn't seem to be any reason that we can't continue
 to use the existing open windows even after closing the pack file.
 ...
 If we don't need to close_pack_windows(), find_lru_pack() doesn't
 strictly need to reject packs that have windows in use.

 That makes me feel somewhat uneasy.  Yes, you can open/mmap/close
 and hold onto the contents of a file still mapped in-core, and it
 may not count as open filedescriptor, but do OSes allow infinite
 such mmapped regions to us?  We do keep track of number of open
 windows, but is there a way for us to learn how close we are to the
 limit?

Not that I know of, but xmmap() does already try to unmap existing
windows when mmap() fails, and then retries the mmap.  It calls
release_pack_memory() which calls unuse_one_window().  mmap returns
ENOMEM when either there is no available memory or if the limit of
mmap mappings has been exceeded.

So, I think we'll be ok.  It's the same situation we'd be in if there
were many large packs (but fewer than pack_max_fds) and a small
packedGitWindowSize, requiring many mmap windows.  We'd try to map an
additional segment, fail, release some unused segments, and retry.

The memory usage of all mmap segments would still be bounded by
packedGitLimit.  It's just that now, when we're only under file
descriptor pressure, we won't close the mmap windows unnecessarily
when they may be needed again.

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] sha1_file: introduce close_one_pack() to close packs on fd pressure

[Brandon Casey]

On Thu, Aug 1, 2013 at 12:16 PM, Brandon Casey draf...@gmail.com wrote:
 On Thu, Aug 1, 2013 at 11:39 AM, Junio C Hamano gits...@pobox.com wrote:
 Brandon Casey draf...@gmail.com writes:

 I've been looking closer at uses of p-windows everywhere, and it
 seems that we always open_packed_git() before we try to create new
 windows.  There doesn't seem to be any reason that we can't continue
 to use the existing open windows even after closing the pack file.
 ...
 If we don't need to close_pack_windows(), find_lru_pack() doesn't
 strictly need to reject packs that have windows in use.

 That makes me feel somewhat uneasy.  Yes, you can open/mmap/close
 and hold onto the contents of a file still mapped in-core, and it
 may not count as open filedescriptor, but do OSes allow infinite
 such mmapped regions to us?  We do keep track of number of open
 windows, but is there a way for us to learn how close we are to the
 limit?

 Not that I know of, but xmmap() does already try to unmap existing
 windows when mmap() fails, and then retries the mmap.  It calls
 release_pack_memory() which calls unuse_one_window().  mmap returns
 ENOMEM when either there is no available memory or if the limit of
 mmap mappings has been exceeded.

 So, I think we'll be ok.  It's the same situation we'd be in if there
 were many large packs (but fewer than pack_max_fds) and a small
 packedGitWindowSize, requiring many mmap windows.  We'd try to map an
 additional segment, fail, release some unused segments, and retry.

Also, it's the same situation we'd be in if there were many small
packs that were smaller than packedGitWindowSize.  We'd mmap the
entire pack file into memory and then close the file descriptor,
allowing us to have many more pack files mapped into memory than
pack_max_fds would allow us to have open.  With enough small packs,
we'd eventually reach the mmap limit and xmmap would try to release
some mappings.

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] sha1_file: introduce close_one_pack() to close packs on fd pressure

[Brandon Casey]

On Thu, Aug 1, 2013 at 1:02 PM, Junio C Hamano gits...@pobox.com wrote:
 Brandon Casey draf...@gmail.com writes:

 On Thu, Aug 1, 2013 at 11:39 AM, Junio C Hamano gits...@pobox.com wrote:
 That makes me feel somewhat uneasy.  Yes, you can open/mmap/close
 and hold onto the contents of a file still mapped in-core, and it
 may not count as open filedescriptor, but do OSes allow infinite
 such mmapped regions to us?  We do keep track of number of open
 windows, but is there a way for us to learn how close we are to the
 limit?

 Not that I know of, but xmmap() does already try to unmap existing
 windows when mmap() fails, and then retries the mmap.  It calls
 release_pack_memory() which calls unuse_one_window().  mmap returns
 ENOMEM when either there is no available memory or if the limit of
 mmap mappings has been exceeded.

 OK, so if there were such an OS limit, the unuse_one_window() will
 hopefully reduce the number of open windows and as a side effect we
 may go below that limit.

 What I was worried about was if there was a limit on the number of
 files we have windows into (i.e. having one window each in N files,
 with fds all closed, somehow costs more than having N window in one
 file with the fd closed).

Ah, yeah, I've never heard of that type of limit and I do not know if
there is one.

If there is such a limit, like you said unuse_one_window() will
_hopefully_ release enough windows to reduce the number of packs we
have windows into, but it is certainly not guaranteed.

 We currently have knobs for total number
 of windows and number of open fds consumed for packs, and the latter
 indirectly controls the number of active packfiles we have windows
 into. Your proposed change will essentially make the number of
 active packfiles unlimited by any of our knobs, and that was where
 my uneasiness was coming from.

Yes and no.  The limit on the number of open fds used for packs only
indirectly controls the number of active packfiles we have windows
into for the packs that are larger than packedGitWindowSize.  For pack
files smaller than packedGitWindowSize, the number was unlimited too
since we close the file descriptor if the whole pack fits within one
window.

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] sha1_file: introduce close_one_pack() to close packs on fd pressure

[Brandon Casey]

When the number of open packs exceeds pack_max_fds, unuse_one_window()
is called repeatedly to attempt to release the least-recently-used
pack windows, which, as a side-effect, will also close a pack file
after closing its last open window.  If a pack file has been opened,
but no windows have been allocated into it, it will never be selected
by unuse_one_window() and hence its file descriptor will not be
closed.  When this happens, git may exceed the number of file
descriptors permitted by the system.

This latter situation can occur in show-ref or receive-pack during ref
advertisement.  During ref advertisement, receive-pack will iterate
over every ref in the repository and advertise it to the client after
ensuring that the ref exists in the local repository.  If the ref is
located inside a pack, then the pack is opened to ensure that it
exists, but since the object is not actually read from the pack, no
mmap windows are allocated.  When the number of open packs exceeds
pack_max_fds, unuse_one_window() will not be able to find any windows to
free and will not be able to close any packs.  Once the per-process
file descriptor limit is exceeded, receive-pack will produce a warning,
not an error, for each pack it cannot open, and will then most likely
fail with an error to spawn rev-list or index-pack like:

   error: cannot create standard input pipe for rev-list: Too many open files
   error: Could not run 'git rev-list'

This may also occur during upload-pack when refs are packed (in the
packed-refs file) and the number of packs that must be opened to
verify that these packed refs exist exceeds the file descriptor
limit.  If the refs are loose, then upload-pack will read each ref
from the object database (if the object is in a pack, allocating one
or more mmap windows for it) in order to peel tags and advertise the
underlying object.  But when the refs are packed and peeled,
upload-pack will use the peeled sha1 in the packed-refs file and
will not need to read from the pack files, so no mmap windows will
be allocated and just like with receive-pack, unuse_one_window()
will never select these opened packs to close.

When we have file descriptor pressure, we just need to find an open
pack to close.  We can leave the existing mmap windows open.  If
additional windows need to be mapped into the pack file, it will be
reopened when necessary.  If the pack file has been rewritten in the
mean time, open_packed_git_1() should notice when it compares the file
size or the pack's sha1 checksum to what was previously read from the
pack index, and reject it.

Let's introduce a new function close_one_pack() designed specifically
for this purpose to search for and close the least-recently-used pack,
where LRU is defined as (in order of preference):

   * pack with oldest mtime and no allocated mmap windows
   * pack with the least-recently-used windows, i.e. the pack
 with the oldest most-recently-used window, where none of
 the windows are in use
   * pack with the least-recently-used windows

Signed-off-by: Brandon Casey draf...@gmail.com
---

Here's the version that leaves the mmap windows open after closing
the pack file descriptor.

-Brandon

 sha1_file.c | 79 -
 1 file changed, 78 insertions(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index 40b2329..263cf71 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -673,6 +673,83 @@ void close_pack_windows(struct packed_git *p)
}
 }
 
+/*
+ * The LRU pack is the one with the oldest MRU window, preferring packs
+ * with no used windows, or the oldest mtime if it has no windows allocated.
+ */
+static void find_lru_pack(struct packed_git *p, struct packed_git **lru_p, 
struct pack_window **mru_w, int *accept_windows_inuse)
+{
+   struct pack_window *w, *this_mru_w;
+   int has_windows_inuse = 0;
+
+   /*
+* Reject this pack if it has windows and the previously selected
+* one does not.  If this pack does not have windows, reject
+* it if the pack file is newer than the previously selected one.
+*/
+   if (*lru_p  !*mru_w  (p-windows || p-mtime  (*lru_p)-mtime))
+   return;
+
+   for (w = this_mru_w = p-windows; w; w = w-next) {
+   /*
+* Reject this pack if any of its windows are in use,
+* but the previously selected pack did not have any
+* inuse windows.  Otherwise, record that this pack
+* has windows in use.
+*/
+   if (w-inuse_cnt) {
+   if (*accept_windows_inuse)
+   has_windows_inuse = 1;
+   else
+   return;
+   }
+
+   if (w-last_used  this_mru_w-last_used)
+   this_mru_w = w;
+
+   /*
+* Reject this pack if it has windows that have been

[PATCH v2 1/2] sha1_file: introduce close_one_pack() to close packs on fd pressure

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

When the number of open packs exceeds pack_max_fds, unuse_one_window()
is called repeatedly to attempt to release the least-recently-used
pack windows, which, as a side-effect, will also close a pack file
after closing its last open window.  If a pack file has been opened,
but no windows have been allocated into it, it will never be selected
by unuse_one_window() and hence its file descriptor will not be
closed.  When this happens, git may exceed the number of file
descriptors permitted by the system.

This latter situation can occur in show-ref or receive-pack during ref
advertisement.  During ref advertisement, receive-pack will iterate
over every ref in the repository and advertise it to the client after
ensuring that the ref exists in the local repository.  If the ref is
located inside a pack, then the pack is opened to ensure that it
exists, but since the object is not actually read from the pack, no
mmap windows are allocated.  When the number of open packs exceeds
pack_max_fds, unuse_one_window() will not be able to find any windows to
free and will not be able to close any packs.  Once the per-process
file descriptor limit is exceeded, receive-pack will produce a warning,
not an error, for each pack it cannot open, and will then most likely
fail with an error to spawn rev-list or index-pack like:

   error: cannot create standard input pipe for rev-list: Too many open files
   error: Could not run 'git rev-list'

This may also occur during upload-pack when refs are packed (in the
packed-refs file) and the number of packs that must be opened to
verify that these packed refs exist exceeds the file descriptor limit.
If the refs are loose, then upload-pack will read each ref from the
pack (allocating one or more mmap windows) so it can peel tags and
advertise the underlying object.  If the refs are packed and peeled,
then upload-pack will use the peeled sha1 in the packed-refs file and
will not need to read from the pack files, so no mmap windows will be
allocated and just like with receive-pack, unuse_one_window() will
never select these opened packs to close.

When we have file descriptor pressure, in contrast to memory pressure,
we need to free all windows and close the pack file descriptor so that
a new pack can be opened.  Let's introduce a new function
close_one_pack() designed specifically for this purpose to search
for and close the least-recently-used pack, where LRU is defined as

   * pack with oldest mtime and no allocated mmap windows or
   * pack with the least-recently-used windows, i.e. the pack
 with the oldest most-recently-used window

Signed-off-by: Brandon Casey draf...@gmail.com
---

The commit message was updated to fix the grammatical error that Eric
Sunshine pointed out, and to correct the paragraph about upload-pack.

-Brandon

 sha1_file.c | 63 -
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index 8e27db1..7731ab1 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -682,6 +682,67 @@ void close_pack_windows(struct packed_git *p)
}
 }
 
+/*
+ * The LRU pack is the one with the oldest MRU window or the oldest mtime
+ * if it has no windows allocated.
+ */
+static void find_lru_pack(struct packed_git *p, struct packed_git **lru_p, 
struct pack_window **mru_w)
+{
+   struct pack_window *w, *this_mru_w;
+
+   /*
+* Reject this pack if it has windows and the previously selected
+* one does not.  If this pack does not have windows, reject
+* it if the pack file is newer than the previously selected one.
+*/
+   if (*lru_p  !*mru_w  (p-windows || p-mtime  (*lru_p)-mtime))
+   return;
+
+   for (w = this_mru_w = p-windows; w; w = w-next) {
+   /* Reject this pack if any of its windows are in use */
+   if (w-inuse_cnt)
+   return;
+   /*
+* Reject this pack if it has windows that have been
+* used more recently than the previously selected pack.
+*/
+   if (*mru_w  w-last_used  (*mru_w)-last_used)
+   return;
+   if (w-last_used  this_mru_w-last_used)
+   this_mru_w = w;
+   }
+
+   /*
+* Select this pack.
+*/
+   *mru_w = this_mru_w;
+   *lru_p = p;
+}
+
+static int close_one_pack(void)
+{
+   struct packed_git *p, *lru_p = NULL;
+   struct pack_window *mru_w = NULL;
+
+   for (p = packed_git; p; p = p-next) {
+   if (p-pack_fd == -1)
+   continue;
+   find_lru_pack(p, lru_p, mru_w);
+   }
+
+   if (lru_p) {
+   close_pack_windows(lru_p);
+   close(lru_p-pack_fd);
+   pack_open_fds--;
+   lru_p-pack_fd = -1;
+   if (lru_p == last_found_pack

[PATCH 2/2] Don't close pack fd when free'ing pack windows

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Now that close_one_pack() has been introduced to handle file
descriptor pressure, it is not strictly necessary to close the
pack file descriptor in unuse_one_window() when we're under memory
pressure.

Jeff King provided a justification for leaving the pack file open:

   If you close packfile descriptors, you can run into racy situations
   where somebody else is repacking and deleting packs, and they go away
   while you are trying to access them. If you keep a descriptor open,
   you're fine; they last to the end of the process. If you don't, then
   they disappear from under you.

   For normal object access, this isn't that big a deal; we just rescan
   the packs and retry. But if you are packing yourself (e.g., because
   you are a pack-objects started by upload-pack for a clone or fetch),
   it's much harder to recover (and we print some warnings).

Let's do so (or uh, not do so).

Signed-off-by: Brandon Casey draf...@gmail.com
---
 builtin/pack-objects.c |  2 +-
 git-compat-util.h  |  2 +-
 sha1_file.c| 21 +++--
 3 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f069462..4eb0521 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1809,7 +1809,7 @@ static void find_deltas(struct object_entry **list, 
unsigned *list_size,
 static void try_to_free_from_threads(size_t size)
 {
read_lock();
-   release_pack_memory(size, -1);
+   release_pack_memory(size);
read_unlock();
 }
 
diff --git a/git-compat-util.h b/git-compat-util.h
index cc4ba4d..29b1ee3 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -517,7 +517,7 @@ int inet_pton(int af, const char *src, void *dst);
 const char *inet_ntop(int af, const void *src, char *dst, size_t size);
 #endif
 
-extern void release_pack_memory(size_t, int);
+extern void release_pack_memory(size_t);
 
 typedef void (*try_to_free_t)(size_t);
 extern try_to_free_t set_try_to_free_routine(try_to_free_t);
diff --git a/sha1_file.c b/sha1_file.c
index 7731ab1..d26121a 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -614,7 +614,7 @@ static void scan_windows(struct packed_git *p,
}
 }
 
-static int unuse_one_window(struct packed_git *current, int keep_fd)
+static int unuse_one_window(struct packed_git *current)
 {
struct packed_git *p, *lru_p = NULL;
struct pack_window *lru_w = NULL, *lru_l = NULL;
@@ -628,15 +628,8 @@ static int unuse_one_window(struct packed_git *current, 
int keep_fd)
pack_mapped -= lru_w-len;
if (lru_l)
lru_l-next = lru_w-next;
-   else {
+   else
lru_p-windows = lru_w-next;
-   if (!lru_p-windows  lru_p-pack_fd != -1
-lru_p-pack_fd != keep_fd) {
-   close(lru_p-pack_fd);
-   pack_open_fds--;
-   lru_p-pack_fd = -1;
-   }
-   }
free(lru_w);
pack_open_windows--;
return 1;
@@ -644,10 +637,10 @@ static int unuse_one_window(struct packed_git *current, 
int keep_fd)
return 0;
 }
 
-void release_pack_memory(size_t need, int fd)
+void release_pack_memory(size_t need)
 {
size_t cur = pack_mapped;
-   while (need = (cur - pack_mapped)  unuse_one_window(NULL, fd))
+   while (need = (cur - pack_mapped)  unuse_one_window(NULL))
; /* nothing */
 }
 
@@ -658,7 +651,7 @@ void *xmmap(void *start, size_t length,
if (ret == MAP_FAILED) {
if (!length)
return NULL;
-   release_pack_memory(length, fd);
+   release_pack_memory(length);
ret = mmap(start, length, prot, flags, fd, offset);
if (ret == MAP_FAILED)
die_errno(Out of memory? mmap failed);
@@ -954,7 +947,7 @@ unsigned char *use_pack(struct packed_git *p,
win-len = (size_t)len;
pack_mapped += win-len;
while (packed_git_limit  pack_mapped
-unuse_one_window(p, p-pack_fd))
+unuse_one_window(p))
; /* nothing */
win-base = xmmap(NULL, win-len,
PROT_READ, MAP_PRIVATE,
@@ -1000,7 +993,7 @@ static struct packed_git *alloc_packed_git(int extra)
 
 static void try_to_free_pack_memory(size_t size)
 {
-   release_pack_memory(size, -1);
+   release_pack_memory(size);
 }
 
 struct packed_git *add_packed_git(const char *path, int path_len, int local)
-- 
1.8.4.rc0.2.g6cf5c31


---
This email message is for the sole use of the intended recipient(s

Re: [PATCH 2/2] Don't close pack fd when free'ing pack windows

[Brandon Casey]

On Wed, Jul 31, 2013 at 2:08 PM, Antoine Pelisse apeli...@gmail.com wrote:
 On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey bca...@nvidia.com wrote:
 ---
 This email message is for the sole use of the intended recipient(s) and may 
 contain
 confidential information.  Any unauthorized review, use, disclosure or 
 distribution
 is prohibited.  If you are not the intended recipient, please contact the 
 sender by
 reply email and destroy all copies of the original message.
 ---

 I'm certainly not a lawyer, and I'm sorry for not reviewing the
 content of the patch instead, but is that not a problem from a legal
 point of view ?
 I remember a video of Greg Kroah-Hartman where he talked about that
 (the video was posted by Junio on G+).

Me either thank God.  Are those footers even enforceable?  I mean,
really, if someone mistakenly sends me their corporate financial
numbers am I supposed to be under some legal obligation not to share
it?  I always assumed it was a scare tactic that lawyers like to use.

To address the text of the footer, I'd say the intended recipient(s)
are those on the to line which includes git@vger.kernel.org and the
implicit use is for inclusion and distribution in the git source code.

Anyway, I doubt I would have any influence on getting the footer
removed.  If Junio would rather me not submit patches with that
footer, then I'd try to find a workaround.

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Don't close pack fd when free'ing pack windows

[Brandon Casey]

On Wed, Jul 31, 2013 at 2:21 PM, Fredrik Gustafsson iv...@iveqy.com wrote:
 On Wed, Jul 31, 2013 at 11:08:21PM +0200, Antoine Pelisse wrote:
 On Wed, Jul 31, 2013 at 9:51 PM, Brandon Casey bca...@nvidia.com wrote:
  ---
  This email message is for the sole use of the intended recipient(s) and 
  may contain
  confidential information.  Any unauthorized review, use, disclosure or 
  distribution
  is prohibited.  If you are not the intended recipient, please contact the 
  sender by
  reply email and destroy all copies of the original message.
  ---

 I'm certainly not a lawyer, and I'm sorry for not reviewing the
 content of the patch instead, but is that not a problem from a legal
 point of view ?

 Talking about legal, is it a problem if a commit isn't signed-off by
 it's committer or author e-mail? Like in this case where the sign-off is
 from gmail.com and the committer from nvidia.com?

It never has been.  My commits should have the author and committer
set to my gmail address actually.

Others have sometimes used the two fields to distinguish between a
corporate identity (i.e. m...@somecompany.com) that represents the
funder of the work and a canonical identity (m...@personalemail.com)
that identifies the person that performed the work.

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure

[Brandon Casey]

On Tue, Jul 30, 2013 at 12:52 PM, Jeff King p...@peff.net wrote:
 On Tue, Jul 30, 2013 at 08:39:48AM -0700, Junio C Hamano wrote:

 Brandon Casey bca...@nvidia.com writes:

  From: Brandon Casey draf...@gmail.com
 
  When the number of open packs exceeds pack_max_fds, unuse_one_window()
  is called repeatedly to attempt to release the least-recently-used
  pack windows, which, as a side-effect, will also close a pack file
  after closing its last open window.  If a pack file has been opened,
  but no windows have been allocated into it, it will never be selected
  by unuse_one_window() and hence its file descriptor will not be
  closed.  When this happens, git may exceed the number of file
  descriptors permitted by the system.

 An interesting find.  The patch from a cursory look reads OK.

 Yeah. I wonder if unuse_one_window() should actually leave the pack fd
 open now in general.

 If you close packfile descriptors, you can run into racy situations
 where somebody else is repacking and deleting packs, and they go away
 while you are trying to access them. If you keep a descriptor open,
 you're fine; they last to the end of the process. If you don't, then
 they disappear from under you.

 For normal object access, this isn't that big a deal; we just rescan the
 packs and retry. But if you are packing yourself (e.g., because you are
 a pack-objects started by upload-pack for a clone or fetch), it's much
 harder to recover (and we print some warnings).

 We had our core.packedGitWindowSize lowered on GitHub for a while, and
 we ran into this warning on busy repositories when we were running git
 gc on the server. We solved it by bumping the window size so we never
 release memory.

 But just not closing the descriptor wouldn't work until Brandon's patch,
 because we used the same function to release memory and descriptor
 pressure. Now we could do them separately (and progressively if we need
 to).

I had thought about whether to stop closing the pack file in
unuse_one_window(), but didn't have a reason to do so.  I think the
scenario you described provides a justification.  If we're not under
file descriptor pressure and we can possibly avoid rescanning the pack
directory, it sounds like a net win.

  This is not likely to occur during upload-pack since upload-pack
  reads each object from the pack so that it can peel tags and
  advertise the exposed object.

 Another interesting find.  Perhaps there is a room for improvements,
 as packed-refs file knows what objects the tags peel to?  I vaguely
 recall Peff was actively reducing the object access during ref
 enumeration in not so distant past...

 Yeah, we should be reading almost no objects these days due to the
 packed-refs peel lines. I just did a double-check on what git
 upload-pack . /dev/null /dev/null reads on my git.git repo, and it is
 only three objects: the v1.8.3.3, v1.8.3.4, and v1.8.4-rc0 tag objects.
 In other words, the tags I got since the last time I ran git gc. So I
 think all is working as designed.

Ok, looks like this has been the case since your 435c8332 which taught
upload-pack to use peel_ref().  So looks like we do avoid reaching
into the pack for any ref that was read from a (modern) packed-refs
file.  The repository I was testing with had mostly loose refs.
Indeed, after packing refs, upload-pack encounters the same problem as
receive-pack and runs out of file descriptors.

So my comment about upload-pack is not completely accurate.
Upload-pack _can_ run into this problem, but the refs must be packed,
as well as there being enough of them that exist in enough different
pack files to exceed the processes fd limit.

 We could give receive-pack the same treatment; I've spent less time
 micro-optimizing it because because we (and most sites, I would think)
 get an order of magnitude more fetches than pushes.

I don't think it would need the 435c8332 treatment since receive-pack
doesn't peel refs when it advertises them to the client and hence does
not need to load the ref object from the pack file during ref
advertisement, but possibly some of the other stuff you did would be
applicable.  But like you said, the number of fetches far exceed the
number of pushes.

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] sha1_file: introduce close_one_pack() to close packs on fd pressure

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

When the number of open packs exceeds pack_max_fds, unuse_one_window()
is called repeatedly to attempt to release the least-recently-used
pack windows, which, as a side-effect, will also close a pack file
after closing its last open window.  If a pack file has been opened,
but no windows have been allocated into it, it will never be selected
by unuse_one_window() and hence its file descriptor will not be
closed.  When this happens, git may exceed the number of file
descriptors permitted by the system.

This latter situation can occur in show-ref or receive-pack during ref
advertisement.  During ref advertisement, receive-pack will iterate
over every ref in the repository and advertise it to the client after
ensuring that the ref exists in the local repository.  If the ref is
located inside a pack, then the pack is opened to ensure that it
exists, but since the object is not actually read from the pack, no
mmap windows are allocated.  When the number of open packs exceeds
pack_max_fds, unuse_one_window() will not able to find any windows to
free and will not be able to close any packs.  Once the per-process
file descriptor limit is exceeded, receive-pack will produce a warning,
not an error, for each pack it cannot open, and will then most likely
fail with an error to spawn rev-list or index-pack like:

   error: cannot create standard input pipe for rev-list: Too many open files
   error: Could not run 'git rev-list'

This is not likely to occur during upload-pack since upload-pack
reads each object from the pack so that it can peel tags and
advertise the exposed object.  So during upload-pack, mmap windows
will be allocated for each pack that is opened and unuse_one_window()
will eventually be able to close unused packs after freeing all of
their windows.

When we have file descriptor pressure, in contrast to memory pressure,
we need to free all windows and close the pack file descriptor so that
a new pack can be opened.  Let's introduce a new function
close_one_pack() designed specifically for this purpose to search
for and close the least-recently-used pack, where LRU is defined as

   * pack with oldest mtime and no allocated mmap windows or
   * pack with the least-recently-used windows, i.e. the pack
 with the oldest most-recently-used window

Signed-off-by: Brandon Casey draf...@gmail.com
---
 sha1_file.c | 63 -
 1 file changed, 62 insertions(+), 1 deletion(-)

diff --git a/sha1_file.c b/sha1_file.c
index 8e27db1..7731ab1 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -682,6 +682,67 @@ void close_pack_windows(struct packed_git *p)
}
 }
 
+/*
+ * The LRU pack is the one with the oldest MRU window or the oldest mtime
+ * if it has no windows allocated.
+ */
+static void find_lru_pack(struct packed_git *p, struct packed_git **lru_p, 
struct pack_window **mru_w)
+{
+   struct pack_window *w, *this_mru_w;
+
+   /*
+* Reject this pack if it has windows and the previously selected
+* one does not.  If this pack does not have windows, reject
+* it if the pack file is newer than the previously selected one.
+*/
+   if (*lru_p  !*mru_w  (p-windows || p-mtime  (*lru_p)-mtime))
+   return;
+
+   for (w = this_mru_w = p-windows; w; w = w-next) {
+   /* Reject this pack if any of its windows are in use */
+   if (w-inuse_cnt)
+   return;
+   /*
+* Reject this pack if it has windows that have been
+* used more recently than the previously selected pack.
+*/
+   if (*mru_w  w-last_used  (*mru_w)-last_used)
+   return;
+   if (w-last_used  this_mru_w-last_used)
+   this_mru_w = w;
+   }
+
+   /*
+* Select this pack.
+*/
+   *mru_w = this_mru_w;
+   *lru_p = p;
+}
+
+static int close_one_pack(void)
+{
+   struct packed_git *p, *lru_p = NULL;
+   struct pack_window *mru_w = NULL;
+
+   for (p = packed_git; p; p = p-next) {
+   if (p-pack_fd == -1)
+   continue;
+   find_lru_pack(p, lru_p, mru_w);
+   }
+
+   if (lru_p) {
+   close_pack_windows(lru_p);
+   close(lru_p-pack_fd);
+   pack_open_fds--;
+   lru_p-pack_fd = -1;
+   if (lru_p == last_found_pack)
+   last_found_pack = NULL;
+   return 1;
+   }
+
+   return 0;
+}
+
 void unuse_pack(struct pack_window **w_cursor)
 {
struct pack_window *w = *w_cursor;
@@ -777,7 +838,7 @@ static int open_packed_git_1(struct packed_git *p)
pack_max_fds = 1;
}
 
-   while (pack_max_fds = pack_open_fds  unuse_one_window(NULL, -1))
+   while (pack_max_fds = pack_open_fds  close_one_pack

Re: [PATCHv3 10/10] pack-revindex: radix-sort the revindex

[Brandon Casey]

On Thu, Jul 11, 2013 at 5:16 AM, Jeff King p...@peff.net wrote:
   Here's an update of the radix-sort patch. It fixes the unsigned issue
   Brandon pointed out, along with a few other comment/naming/style fixes.
   I also updated the commit message with more explanation of the
   timings.

Very nice.

For what it's worth:

Reviewed-by: Brandon Casey draf...@gmail.com

remainder retained for reference (or whatever Jonathan usually says)

   The interdiff is:

   diff --git a/pack-revindex.c b/pack-revindex.c
   index 9365bc2..b4d2b35 100644
   --- a/pack-revindex.c
   +++ b/pack-revindex.c
   @@ -61,6 +61,10 @@ static void init_pack_revindex(void)

/*
 * This is a least-significant-digit radix sort.
   + *
   + * It sorts each of the n items in entries by its offset field. The 
 max
   + * parameter must be at least as large as the largest offset in the array,
   + * and lets us quit the sort early.
 */
static void sort_revindex(struct revindex_entry *entries, unsigned n, 
 off_t max)
{
   @@ -78,18 +82,25 @@ static void sort_revindex(struct revindex_entry 
 *entries, unsigned n, off_t max)
#define BUCKET_FOR(a, i, bits) (((a)[(i)].offset  (bits))  (BUCKETS-1))

 /*
   -  * We need O(n) temporary storage, so we sort back and forth between
   -  * the real array and our tmp storage. To keep them straight, we 
 always
   -  * sort from a into buckets in b.
   +  * We need O(n) temporary storage. Rather than do an extra copy of the
   +  * partial results into entries, we sort back and forth between the
   +  * real array and temporary storage. In each iteration of the loop, we
   +  * keep track of them with alias pointers, always sorting from from
   +  * to to.
  */
   - struct revindex_entry *tmp = xcalloc(n, sizeof(*tmp));
   - struct revindex_entry *a = entries, *b = tmp;
   - int bits = 0;
   + struct revindex_entry *tmp = xmalloc(n * sizeof(*tmp));
   + struct revindex_entry *from = entries, *to = tmp;
   + int bits;
 unsigned *pos = xmalloc(BUCKETS * sizeof(*pos));

   - while (max  bits) {
   + /*
   +  * If (max  bits) is zero, then we know that the radix digit we are
   +  * on (and any higher) will be zero for all entries, and our loop will
   +  * be a no-op, as everybody lands in the same zero-th bucket.
   +  */
   + for (bits = 0; max  bits; bits += DIGIT_SIZE) {
 struct revindex_entry *swap;
   - int i;
   + unsigned i;

 memset(pos, 0, BUCKETS * sizeof(*pos));

   @@ -102,7 +113,7 @@ static void sort_revindex(struct revindex_entry 
 *entries, unsigned n, off_t max)
  * previous bucket to get the true index.
  */
 for (i = 0; i  n; i++)
   - pos[BUCKET_FOR(a, i, bits)]++;
   + pos[BUCKET_FOR(from, i, bits)]++;
 for (i = 1; i  BUCKETS; i++)
 pos[i] += pos[i-1];

   @@ -112,32 +123,37 @@ static void sort_revindex(struct revindex_entry 
 *entries, unsigned n, off_t max)
  * to avoid using an extra index to count up. And since we are
  * going backwards there, we must also go backwards through 
 the
  * array itself, to keep the sort stable.
   +  *
   +  * Note that we use an unsigned iterator to make sure we can
   +  * handle 2^32-1 objects, even on a 32-bit system. But this
   +  * means we cannot use the more obvious i = 0 loop 
 condition
   +  * for counting backwards, and must instead check for
   +  * wrap-around with UINT_MAX.
  */
   - for (i = n - 1; i = 0; i--)
   - b[--pos[BUCKET_FOR(a, i, bits)]] = a[i];
   + for (i = n - 1; i != UINT_MAX; i--)
   + to[--pos[BUCKET_FOR(from, i, bits)]] = from[i];

 /*
   -  * Now b contains the most sorted list, so we swap a and
   -  * b for the next iteration.
   +  * Now to contains the most sorted list, so we swap from 
 and
   +  * to for the next iteration.
  */
   - swap = a;
   - a = b;
   - b = swap;
   -
   - /* And bump our bits for the next round. */
   - bits += DIGIT_SIZE;
   + swap = from;
   + from = to;
   + to = swap;
 }

 /*
  * If we ended with our data in the original array, great. If not,
  * we have to move it back from the temporary storage.
  */
   - if (a != entries)
   + if (from != entries)
 memcpy(entries, tmp, n * sizeof(*entries));
 free(tmp);
 free(pos);

#undef BUCKET_FOR
   +#undef BUCKETS
   +#undef DIGIT_SIZE
}

/*

 -- 8 --
 Subject: [PATCH

Re: [PATCH 10/10] pack-revindex: radix-sort the revindex

[Brandon Casey]

On Wed, Jul 10, 2013 at 4:55 AM, Jeff King p...@peff.net wrote:
 The pack revindex stores the offsets of the objects in the
 pack in sorted order, allowing us to easily find the on-disk
 size of each object. To compute it, we populate an array
 with the offsets from the sha1-sorted idx file, and then use
 qsort to order it by offsets.

 That does O(n log n) offset comparisons, and profiling shows
 that we spend most of our time in cmp_offset. However, since
 we are sorting on a simple off_t, we can use numeric sorts
 that perform better. A radix sort can run in O(k*n), where k
 is the number of digits in our number. For a 64-bit off_t,
 using 16-bit digits gives us k=4.

 On the linux.git repo, with about 3M objects to sort, this
 yields a 400% speedup. Here are the best-of-five numbers for
 running echo HEAD | git cat-file --batch-disk-size, which
 is dominated by time spent building the pack revindex:

   before after
   real0m0.834s   0m0.204s
   user0m0.788s   0m0.164s
   sys 0m0.040s   0m0.036s

 On a smaller repo, the radix sort would not be
 as impressive (and could even be worse), as we are trading
 the log(n) factor for the k=4 of the radix sort. However,
 even on git.git, with 173K objects, it shows some
 improvement:

   before after
   real0m0.046s   0m0.017s
   user0m0.036s   0m0.012s
   sys 0m0.008s   0m0.000s

k should only be 2 for git.git.  I haven't packed in a while, but I
think it should all fit within 4G.  :)  The pathological case would be
a pack file with very few very very large objects, large enough to
push the pack size over the 2^48 threshold so we'd have to do all four
radixes.

It's probably worth mentioning here and/or in the code that k is
dependent on the pack file size and that we can jump out early for
small pack files.  That's my favorite part of this code by the way. :)

 Signed-off-by: Jeff King p...@peff.net
 ---
 I changed a few things from the original, including:

   1. We take an unsigned number of objects to match the fix in the
  last patch.

   2. The 16-bit digit size is factored out to a single place, which
  avoids magic numbers and repeating ourselves.

   3. The digits variable is renamed to bits, which is more accurate.

   4. The outer loop condition uses the simpler while (max  bits).

   5. We use memcpy instead of an open-coded loop to copy the whole array
  at the end. The individual bucket-assignment is still done by
  struct assignment. I haven't timed if memcpy would make a
  difference there.

   6. The 64K*sizeof(int) pos array is now heap-allocated, in case
  there are platforms with a small stack.

 I re-ran my timings to make sure none of the above impacted them; it
 turned out the same.

  pack-revindex.c | 84 
 +
  1 file changed, 79 insertions(+), 5 deletions(-)

 diff --git a/pack-revindex.c b/pack-revindex.c
 index 1aa9754..9365bc2 100644
 --- a/pack-revindex.c
 +++ b/pack-revindex.c
 @@ -59,11 +59,85 @@ static int cmp_offset(const void *a_, const void *b_)
 /* revindex elements are lazily initialized */
  }

 -static int cmp_offset(const void *a_, const void *b_)
 +/*
 + * This is a least-significant-digit radix sort.
 + */
 +static void sort_revindex(struct revindex_entry *entries, unsigned n, off_t 
 max)
  {
 -   const struct revindex_entry *a = a_;
 -   const struct revindex_entry *b = b_;
 -   return (a-offset  b-offset) ? -1 : (a-offset  b-offset) ? 1 : 0;
 +   /*
 +* We use a digit size of 16 bits. That keeps our memory
 +* usage reasonable, and we can generally (for a 4G or smaller
 +* packfile) quit after two rounds of radix-sorting.
 +*/
 +#define DIGIT_SIZE (16)
 +#define BUCKETS (1  DIGIT_SIZE)
 +   /*
 +* We want to know the bucket that a[i] will go into when we are using
 +* the digit that is N bits from the (least significant) end.
 +*/
 +#define BUCKET_FOR(a, i, bits) (((a)[(i)].offset  (bits))  (BUCKETS-1))
 +
 +   /*
 +* We need O(n) temporary storage, so we sort back and forth between
 +* the real array and our tmp storage. To keep them straight, we 
 always
 +* sort from a into buckets in b.
 +*/
 +   struct revindex_entry *tmp = xcalloc(n, sizeof(*tmp));

Didn't notice it the first time I read this, but do we really need
calloc here?  Or will malloc do?

 +   struct revindex_entry *a = entries, *b = tmp;
 +   int bits = 0;
 +   unsigned *pos = xmalloc(BUCKETS * sizeof(*pos));
 +
 +   while (max  bits) {
 +   struct revindex_entry *swap;
 +   int i;

You forgot to make i unsigned.  See below too...

 +
 +   memset(pos, 0, BUCKETS * sizeof(*pos));
 +
 +   /*
 +* We want pos[i] to store the index of the last element that
 +* will go in bucket i (actually one past the last 

[PATCH v2] remote.c: avoid O(m*n) behavior in match_push_refs

[Brandon Casey]

When pushing using a matching refspec or a pattern refspec, each ref
in the local repository must be paired with a ref advertised by the
remote server.  This is accomplished by using the refspec to transform
the name of the local ref into the name it should have in the remote
repository, and then performing a linear search through the list of
remote refs to see if the remote ref was advertised by the remote
system.

Each of these lookups has O(n) complexity and makes match_push_refs()
be an O(m*n) operation, where m is the number of local refs and n is
the number of remote refs.  If there are many refs 100,000+, then this
ref matching can take a significant amount of time.  Let's prepare an
index of the remote refs to allow searching in O(log n) time and
reduce the complexity of match_push_refs() to O(m log n).

We prepare the index lazily so that it is only created when necessary.
So, there should be no impact when _not_ using a matching or pattern
refspec, i.e. when pushing using only explicit refspecs.

Dry-run push of a repository with 121,913 local and remote refs:

before after
real1m40.582s  0m0.804s
user1m39.914s  0m0.515s
sys 0m0.125s   0m0.106s

The creation of the index has overhead.  So, if there are very few
local refs, then it could take longer to create the index than it
would have taken to just perform n linear lookups into the remote
ref space.  Using the index should provide some improvement when
the number of local refs is roughly greater than the log of the
number of remote refs (i.e. m = log n).  The pathological case is
when there is a single local ref and very many remote refs.

Dry-run push of a repository with 121,913 remote refs and a single
local ref:

beforeafter
real0m0.525s  0m0.566s
user0m0.243s  0m0.279s
sys 0m0.075s  0m0.099s

Using an index takes 41 ms longer, or roughly 7.8% longer.

Jeff King measured a no-op push of a single ref into a remote repo
with 370,000 refs:

beforeafter
real0m1.087s  0m1.156s
user0m1.344s  0m1.412s
sys 0m0.288s  0m0.284s

Using an index takes 69 ms longer, or roughly 6.3% longer.

None of the measurements above required transferring any objects to
the remote repository.  If the push required transferring objects and
updating the refs in the remote repository, the impact of preparing
the search index would be even smaller.

Note, we refrain from using an index in the send_prune block since it
is expected that the number of refs that are being pruned is more
commonly much smaller than the number of local refs (i.e. m  n,
and particularly m  log(n), where m is the number of refs that
should be pruned and n is the number of local refs), so the overhead
of creating the search index would likely exceed the benefit of using
it.

Signed-off-by: Brandon Casey draf...@gmail.com
---

Here is the reroll with an updated commit message that hopefully
provides a little more detail to justify this change.  I removed
the use of the search index in the send_prune block since I think
that pruning many refs is an uncommon operation and the overhead
of creating the index will more commonly exceed the benefit of
using it.

This version now lazily builds the search index in the first loop,
so there should be no impact when pushing using explicit refspecs.

e.g. pushing a change for review to Gerrit

   $ git push origin HEAD:refs/for/master

I suspect that this is the most common form of pushing and furthermore
will become the default once push.default defaults to 'current'.

The remaining push cases can be distilled into the following:

  ref-countimpact
  m = log n   improved with this patch
  m  log nregressed with this patch roughly ~6-7%

So, I think what we have to consider is whether the improvement to
something like 'git push --mirror' is worth the impact to an asymmetric
push where the number of local refs is much smaller than the number of
remote refs.  I'm not sure how common the latter really is though.
Gerrit does produce repositories with many refs on the remote end in
the refs/changes/ namespace, but do people commonly push to Gerrit
using matching or pattern refspecs?  Not sure, but I'd tend to think
that they don't.

-Brandon

 remote.c | 20 +++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index 6f57830..8bca65a 100644
--- a/remote.c
+++ b/remote.c
@@ -1302,6 +1302,14 @@ static void add_missing_tags(struct ref *src, struct ref 
**dst, struct ref ***ds
free(sent_tips.tip);
 }
 
+static void prepare_ref_index(struct string_list *ref_index, struct ref *ref)
+{
+   for ( ; ref; ref = ref-next)
+   string_list_append_nodup(ref_index, ref-name)-util = ref;
+
+   sort_string_list(ref_index);
+}
+
 /*
  * Given the set of refs the local repository has, the set of refs the
  * remote repository has, and the refspec used for push, determine
@@ -1320,6 +1328,7 @@ int match_push_refs(struct ref *src

[PATCH v2 w/prune index] remote.c: avoid O(m*n) behavior in match_push_refs

[Brandon Casey]

When pushing using a matching refspec or a pattern refspec, each ref
in the local repository must be paired with a ref advertised by the
remote server.  This is accomplished by using the refspec to transform
the name of the local ref into the name it should have in the remote
repository, and then performing a linear search through the list of
remote refs to see if the remote ref was advertised by the remote
system.

Each of these lookups has O(n) complexity and makes match_push_refs()
be an O(m*n) operation, where m is the number of local refs and n is
the number of remote refs.  If there are many refs 100,000+, then this
ref matching can take a significant amount of time.  Let's prepare an
index of the remote refs to allow searching in O(log n) time and
reduce the complexity of match_push_refs() to O(m log n).

We prepare the index lazily so that it is only created when necessary.
So, there should be no impact when _not_ using a matching or pattern
refspec, i.e. when pushing using only explicit refspecs.

Dry-run push of a repository with 121,913 local and remote refs:

before after
real1m40.582s  0m0.804s
user1m39.914s  0m0.515s
sys 0m0.125s   0m0.106s

The creation of the index has overhead.  So, if there are very few
local refs, then it could take longer to create the index than it
would have taken to just perform n linear lookups into the remote
ref space.  Using the index should provide some improvement when
the number of local refs is roughly greater than the log of the
number of remote refs (i.e. m = log n).  The pathological case is
when there is a single local ref and very many remote refs.

Dry-run push of a repository with 121,913 remote refs and a single
local ref:

beforeafter
real0m0.525s  0m0.566s
user0m0.243s  0m0.279s
sys 0m0.075s  0m0.099s

Using an index takes 41 ms longer, or roughly 7.8% longer.

Jeff King measured a no-op push of a single ref into a remote repo
with 370,000 refs:

beforeafter
real0m1.087s  0m1.156s
user0m1.344s  0m1.412s
sys 0m0.288s  0m0.284s

Using an index takes 69 ms longer, or roughly 6.3% longer.

None of the measurements above required transferring any objects to
the remote repository.  If the push required transferring objects and
updating the refs in the remote repository, the impact of preparing
the search index would be even smaller.

A similar operation is performed in the reverse direction when pruning
using a matching or pattern refspec.  Let's avoid O(m*n) behavior in
the same way by lazily preparing an index on the local refs.

Signed-off-by: Brandon Casey draf...@gmail.com
---

On Mon, Jul 8, 2013 at 12:50 AM, Jeff King p...@peff.net wrote:
 On Mon, Jul 08, 2013 at 12:02:11AM -0700, Brandon Casey wrote:
 
  Here is the reroll with an updated commit message that hopefully
  provides a little more detail to justify this change.  I removed
  the use of the search index in the send_prune block since I think
  that pruning many refs is an uncommon operation and the overhead
  of creating the index will more commonly exceed the benefit of
  using it.
 
 I don't know. I'd think that if you are using pruning, you might delete
 a large chunk at one time (e.g., rearranging your ref hierarchy,
 followed by git push --mirror). But that is just my gut feeling. I
 haven't actually run into this slow-down in the real world (we typically
 fetch from our giant repositories rather than push into them).

Firstly, why are you still awake?!?! :)

Secondly, fair enough.  I don't think the change to the pruning block
will have much impact in real repos either way.  In this block, the
search is being performed on the local refs.  There would have to be
many refs on the local side for the generation of the index to be
significant enough to notice.

So, I'm fine with using the index when pruning too to avoid worst-case
behavior when there are many local refs and many deletions.

-Brandon

 remote.c | 27 +--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index 6f57830..efcba93 100644
--- a/remote.c
+++ b/remote.c
@@ -1302,6 +1302,14 @@ static void add_missing_tags(struct ref *src, struct ref 
**dst, struct ref ***ds
free(sent_tips.tip);
 }
 
+static void prepare_ref_index(struct string_list *ref_index, struct ref *ref)
+{
+   for ( ; ref; ref = ref-next)
+   string_list_append_nodup(ref_index, ref-name)-util = ref;
+
+   sort_string_list(ref_index);
+}
+
 /*
  * Given the set of refs the local repository has, the set of refs the
  * remote repository has, and the refspec used for push, determine
@@ -1320,6 +1328,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
int errs;
static const char *default_refspec[] = { :, NULL };
struct ref *ref, **dst_tail = tail_ref(dst);
+   struct string_list dst_ref_index = STRING_LIST_INIT_NODUP;
 
if (!nr_refspec

Re: [PATCH 4/4] pack-revindex: radix-sort the revindex

[Brandon Casey]

On Sun, Jul 7, 2013 at 3:14 AM, Jeff King p...@peff.net wrote:
 The pack revindex stores the offsets of the objects in the
 pack in sorted order, allowing us to easily find the on-disk
 size of each object. To compute it, we populate an array
 with the offsets from the sha1-sorted idx file, and then use
 qsort to order it by offsets.

 That does O(n log n) offset comparisons, and profiling shows
 that we spend most of our time in cmp_offset. However, since
 we are sorting on a simple off_t, we can use numeric sorts
 that perform better. A radix sort can run in O(k*n), where k
 is the number of digits in our number. For a 64-bit off_t,
 using 16-bit digits gives us k=4.

 On the linux.git repo, with about 3M objects to sort, this
 yields a 400% speedup. Here are the best-of-five numbers for
 running echo HEAD | git cat-file --batch-disk-size, which
 is dominated by time spent building the pack revindex:

   before after
   real0m0.834s   0m0.204s
   user0m0.788s   0m0.164s
   sys 0m0.040s   0m0.036s

 On a smaller repo, the radix sort would not be
 as impressive (and could even be worse), as we are trading
 the log(n) factor for the k=4 of the radix sort. However,
 even on git.git, with 173K objects, it shows some
 improvement:

   before after
   real0m0.046s   0m0.017s
   user0m0.036s   0m0.012s
   sys 0m0.008s   0m0.000s

 Signed-off-by: Jeff King p...@peff.net
 ---
 I think there are probably still two potential issues here:

   1. My while() loop termination probably has issues when we have to use
  all 64 bits to represent the pack offset (not likely, but...)

   2. We put int pos[65536] on the stack. This is a little big, but is
  probably OK, as I think the usual small stack problems we have seen
  are always in threaded code. But it would not be a big deal to heap
  allocate it (it would happen once per radix step, which is only 4
  times for the whole sort).

  pack-revindex.c | 77 
 +
  1 file changed, 72 insertions(+), 5 deletions(-)

 diff --git a/pack-revindex.c b/pack-revindex.c
 index 77a0465..d2adf36 100644
 --- a/pack-revindex.c
 +++ b/pack-revindex.c
 @@ -59,11 +59,78 @@ static int cmp_offset(const void *a_, const void *b_)
 /* revindex elements are lazily initialized */
  }

 -static int cmp_offset(const void *a_, const void *b_)
 +/*
 + * This is a least-significant-digit radix sort using a 16-bit digit.
 + */
 +static void sort_revindex(struct revindex_entry *entries, int n, off_t max)

If 'n' is the number of objects in the pack, shouldn't it be unsigned?

The data type for struct packed_git.num_objects is uint32_t.  Looks
like create_pack_revindex uses the wrong datatype when it captures
num_objects in the int num_ent and passes it to sort_revindex.  So, it
looks like that function needs to be updated too.

  {
 -   const struct revindex_entry *a = a_;
 -   const struct revindex_entry *b = b_;
 -   return (a-offset  b-offset) ? -1 : (a-offset  b-offset) ? 1 : 0;
 +   /*
 +* We need O(n) temporary storage, so we sort back and forth between
 +* the real array and our tmp storage. To keep them straight, we 
 always
 +* sort from a into buckets in b.
 +*/
 +   struct revindex_entry *tmp = xcalloc(n, sizeof(*tmp));
 +   struct revindex_entry *a = entries, *b = tmp;
 +   int digits = 0;
 +
 +   /*
 +* We want to know the bucket that a[i] will go into when we are using
 +* the digit that is N bits from the (least significant) end.
 +*/
 +#define BUCKET_FOR(a, i, digits) ((a[i].offset  digits)  0x)
 +
 +   while (max / (((off_t)1)  digits)) {

Is there any reason this shouldn't be simplified to just:

   while (max  digits) {

I glanced briefly at the assembly and it appears that gcc does
actually emit a divide instruction to accomplish this, which I think
we can avoid by just rearranging the operation.

 +   struct revindex_entry *swap;
 +   int i;
 +   int pos[65536] = {0};
 +
 +   /*
 +* We want pos[i] to store the index of the last element that
 +* will go in bucket i (actually one past the last element).
 +* To do this, we first count the items that will go in each
 +* bucket, which gives us a relative offset from the last
 +* bucket. We can then cumulatively add the index from the
 +* previous bucket to get the true index.
 +*/
 +   for (i = 0; i  n; i++)
 +   pos[BUCKET_FOR(a, i, digits)]++;
 +   for (i = 1; i  ARRAY_SIZE(pos); i++)
 +   pos[i] += pos[i-1];
 +
 +   /*
 +* Now we can drop the elements into their correct buckets (in
 +* our temporary array).  We iterate the pos counter backwards

Re: [PATCH 4/4] pack-revindex: radix-sort the revindex

[Brandon Casey]

On Mon, Jul 8, 2013 at 1:50 PM, Brandon Casey draf...@gmail.com wrote:
 On Sun, Jul 7, 2013 at 3:14 AM, Jeff King p...@peff.net wrote:

 diff --git a/pack-revindex.c b/pack-revindex.c
 index 77a0465..d2adf36 100644
 --- a/pack-revindex.c
 +++ b/pack-revindex.c
 @@ -59,11 +59,78 @@ static int cmp_offset(const void *a_, const void *b_)
 /* revindex elements are lazily initialized */
  }

 -static int cmp_offset(const void *a_, const void *b_)
 +/*
 + * This is a least-significant-digit radix sort using a 16-bit digit.
 + */
 +static void sort_revindex(struct revindex_entry *entries, int n, off_t max)

 If 'n' is the number of objects in the pack, shouldn't it be unsigned?

 The data type for struct packed_git.num_objects is uint32_t.  Looks
 like create_pack_revindex uses the wrong datatype when it captures
 num_objects in the int num_ent and passes it to sort_revindex.  So, it
 looks like that function needs to be updated too.

Hmm.  It seems more than just create_pack_revindex holds num_objects
in a signed int.  Don't we support 4G objects in packs?

find_pack_revindex uses a signed int for the index variables in its
binary search which means it will fail when num_objects = 2^31.

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] remote.c: avoid O(n^2) behavior in match_push_refs by using string_list

[Brandon Casey]

On Tue, Jul 2, 2013 at 11:23 PM, Jeff King p...@peff.net wrote:
 On Tue, Jul 02, 2013 at 04:53:48PM -0700, Brandon Casey wrote:

 From: Brandon Casey draf...@gmail.com

 When pushing, each ref in the local repository must be paired with a
 ref advertised by the remote server.  Currently, this is performed by
 first applying the refspec to the local ref to transform the local ref
 into the name of the remote ref, and then performing a linear search
 through the list of remote refs to see if the remote ref was advertised
 by the remote system.

 This has O(n) complexity and makes match_push_refs() be an O(n^2)
 operation.

 Just to be sure I understand correctly, is this actually O(m*n) where
 m is the number of local refs and n is the number of remote refs?

Yes, O(m*n) is more correct.  I think you understand completely.

 For a repository that repeatedly pushes everything it has to the remote,
 we end up with m=n, but it would not necessarily be the case if you are
 pushing a subset of your refs. But even pushing a small number of refs
 into a repository with a very large number of refs would be
 unnecessarily slow, as we would do several O(n) lookups which could be
 O(log n). So it may speed things up even in the case of a normal-sized
 repo pushing to a large one.

Right.  For repos with few refs on either side, I don't think there
will be any measurable difference.  When pushing a single ref to a
repo with a very large number of refs, we will see a very small net
loss for the time required to prepare the string list (which grows
linearly with the number of remote refs).  After 2 or 3 refs, we
should see a net gain.

So we're really just improving our worst case performance here.

 Dry-run push of a repository with 121913 refs:

 before after
 real1m40.582s  0m0.804s
 user1m39.914s  0m0.515s
 sys 0m0.125s   0m0.106s

 Very nice. :)

 Signed-off-by: Brandon Casey draf...@gmail.com
 ---
  remote.c | 26 --
  1 file changed, 24 insertions(+), 2 deletions(-)

 Patch itself looks good to me, although...

 @@ -1362,6 +1378,8 @@ int match_push_refs(struct ref *src, struct ref **dst,
   free(dst_name);
   }

 + string_list_clear(ref_list, 0);
 +
   if (flags  MATCH_REFS_FOLLOW_TAGS)
   add_missing_tags(src, dst, dst_tail);

 @@ -1376,11 +1394,15 @@ int match_push_refs(struct ref *src, struct ref 
 **dst,

   src_name = get_ref_match(rs, nr_refspec, ref, 
 send_mirror, FROM_DST, NULL);
   if (src_name) {
 - if (!find_ref_by_name(src, src_name))
 + if (!ref_list.nr)
 + prepare_searchable_ref_list(src,
 + ref_list);
 + if (!string_list_has_string(ref_list, 
 src_name))

 This hunk threw me for a bit, as it looked like we were lazily
 initializing ref_list in case we had not done so earlier. But we would
 have cleared it mid-way through the function (in the hunk above), and it
 is only that we are reusing the same ref_list for two different
 purposes.

 I do not feel strongly about it, but it might be a little more obvious
 to just declare a new variable in the block, like:

 diff --git a/remote.c b/remote.c
 index 75255af..53bef82 100644
 --- a/remote.c
 +++ b/remote.c
 @@ -1399,6 +1399,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
 add_missing_tags(src, dst, dst_tail);

 if (send_prune) {
 +   struct string_list src_ref_index = STRING_LIST_INIT_NODUP;
 /* check for missing refs on the remote */
 for (ref = *dst; ref; ref = ref-next) {
 char *src_name;
 @@ -1409,15 +1410,15 @@ int match_push_refs(struct ref *src, struct ref **dst,

 src_name = get_ref_match(rs, nr_refspec, ref, 
 send_mirror, FROM_DST, NULL);
 if (src_name) {
 -   if (!ref_list.nr)
 +   if (!src_ref_index.nr)
 prepare_searchable_ref_list(src,
 -   ref_list);
 -   if (!string_list_has_string(ref_list, 
 src_name))
 +   src_ref_index);
 +   if (!string_list_has_string(src_ref_index, 
 src_name))
 ref-peer_ref = alloc_delete_ref();
 free(src_name);
 }
 }
 -   string_list_clear(ref_list, 0);
 +   string_list_clear(src_ref_index, 0);
 }
 if (errs)
 return -1;

 And similarly maybe call the outer ref_list dst_ref_index or something.

That looks/sounds reasonable.

 I also note that we don't do the lazy-prepare

Re: [PATCH] remote.c: avoid O(n^2) behavior in match_push_refs by using string_list

[Brandon Casey]

On 7/3/2013 11:40 AM, Junio C Hamano wrote:
 Brandon Casey draf...@gmail.com writes:
 
 Right.  For repos with few refs on either side, I don't think there
 will be any measurable difference.  When pushing a single ref to a
 repo with a very large number of refs, we will see a very small net
 loss for the time required to prepare the string list (which grows
 linearly with the number of remote refs).  After 2 or 3 refs, we
 should see a net gain.

 So we're really just improving our worst case performance here.
 
 ... by penalizing the common case by how much?  If it is not too
 much, then this obviously would be a good change.

For something the size of the git repo, 5 branches, and pushing with
matching refspecs, I can't measure any difference.  The fastest time I
record with or without this patch is the same:

   $ time git push -n
   real0m0.178s
   user0m0.020s
   sys 0m0.008s

Ditto, when only pushing a single branch.  Preparing the string list for
a repo with a normal number of refs has very little overhead.

When the remote side has very many refs, then there is a small penalty
when the local side is pushing very few refs.  But still, the penalty is
small.

My measurements for pushing from a repo with a single local branch into
my 10+ ref repo showed 10% hit and the numbers were in the tens of
milliseconds.

beforeafter
real0m0.525s  0m0.566s
user0m0.243s  0m0.279s
sys 0m0.075s  0m0.099s

 ...  But, I don't see a down side to doing the lazy prepare in
 the other loop too, and in fact, it looks like we may be able to avoid
 building the string list when only explicit refspecs are used.  So,
 yeah, we should lazy build in both loops.
 
 OK, so will see a reroll sometime?

Yeah, I'll reroll.

-Brandon

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] remote.c: avoid O(n^2) behavior in match_push_refs by using string_list

[Brandon Casey]

On 7/3/2013 12:00 PM, Jeff King wrote:
 On Wed, Jul 03, 2013 at 11:40:12AM -0700, Junio C Hamano wrote:
 
 Brandon Casey draf...@gmail.com writes:

 Right.  For repos with few refs on either side, I don't think there
 will be any measurable difference.  When pushing a single ref to a
 repo with a very large number of refs, we will see a very small net
 loss for the time required to prepare the string list (which grows
 linearly with the number of remote refs).  After 2 or 3 refs, we
 should see a net gain.

 So we're really just improving our worst case performance here.

 ... by penalizing the common case by how much?  If it is not too
 much, then this obviously would be a good change.
 
 I don't think by much. If we have m local refs to push and n remote
 refs, right now we do O(m*n) work (m linear searches of the remote
 namespace). With Brandon's patch, we do O(n log n) to build the index,

Whoops, yes, n log n, not linear as I misspoke.

 plus O(m log n) for lookups.
 
 So our break-even point is basically m = log n, and for m smaller than
 that, we do more work building the index. Your absolute biggest
 difference would be pushing a single ref to a repository with a very
 large number of refs.
 
 Here are the timings before and after Brandon's patch for pushing a
 no-op single ref from a normal repo to one with 370K refs (the same
 pathological repo from the upload-pack tests). Times are
 best-of-five.
 
  before after
  real0m1.087s   0m1.156s
  user0m1.344s   0m1.412s
  sys 0m0.288s   0m0.284s
 
 So it's measurable, but even on a pathological worst-case, we're talking
 about 6% slowdown.

That agrees with what I've observed.

 You could try to guess about when to build the index based on the size
 of m and n, but I suspect you'd waste more time calculating whether
 to build the index than you would simply building it in most cases.

I agree, I don't think it's worth trying to guess when to build an index
and when to just perform linear searches.  If building the payload for
each element in the index was more expensive than just assigning to a
pointer, than it could be worth it, but we're not, so I don't think it
is worth it.

-Brandon

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: How to still kill git fetch with too many refs

[Brandon Casey]

On Mon, Jul 1, 2013 at 10:01 PM, Jeff King p...@peff.net wrote:
 On Tue, Jul 02, 2013 at 12:41:51AM -0400, Jeff King wrote:

 I replicated your test setup, and the problem is that we have many
 common objects on both sides during the ref negotiation. So we end up in
 rev_list_push for each one, which has the same O(n^2) behavior.
 Switching it to just sort at the end is not trivial; we first insert all
 of the objects, but then we actually walk the parents, pushing onto the
 list as we go. So I think we'd want a better data structure (like a
 priority queue).

 Like the patch below,

snip

Looks obviously correct to me since I've got essentially the same
patch sitting in my local repo. :b

I've got the patch coming that fixes the same problem on the push side
of things and provides the same order of magnitude improvement.

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] remote.c: avoid O(n^2) behavior in match_push_refs by using string_list

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

When pushing, each ref in the local repository must be paired with a
ref advertised by the remote server.  Currently, this is performed by
first applying the refspec to the local ref to transform the local ref
into the name of the remote ref, and then performing a linear search
through the list of remote refs to see if the remote ref was advertised
by the remote system.

This has O(n) complexity and makes match_push_refs() be an O(n^2)
operation.  If there are many refs 100,000+, then this ref matching
can take a significant amount of time.  Let's populate a string_list
with the remote ref names to allow searching in O(log n) time and
reduce the complexity of match_push_refs() to O(n log n).

Dry-run push of a repository with 121913 refs:

before after
real1m40.582s  0m0.804s
user1m39.914s  0m0.515s
sys 0m0.125s   0m0.106s

Signed-off-by: Brandon Casey draf...@gmail.com
---
 remote.c | 26 --
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index 6f57830..b416b8e 100644
--- a/remote.c
+++ b/remote.c
@@ -1302,6 +1302,15 @@ static void add_missing_tags(struct ref *src, struct ref 
**dst, struct ref ***ds
free(sent_tips.tip);
 }
 
+static void prepare_searchable_ref_list(struct ref *ref, struct string_list 
*ref_list)
+{
+   for ( ; ref; ref = ref-next)
+   string_list_append_nodup(ref_list, ref-name)-util = ref;
+
+   sort_string_list(ref_list);
+}
+
+
 /*
  * Given the set of refs the local repository has, the set of refs the
  * remote repository has, and the refspec used for push, determine
@@ -1320,6 +1329,7 @@ int match_push_refs(struct ref *src, struct ref **dst,
int errs;
static const char *default_refspec[] = { :, NULL };
struct ref *ref, **dst_tail = tail_ref(dst);
+   struct string_list ref_list = STRING_LIST_INIT_NODUP;
 
if (!nr_refspec) {
nr_refspec = 1;
@@ -1328,8 +1338,11 @@ int match_push_refs(struct ref *src, struct ref **dst,
rs = parse_push_refspec(nr_refspec, (const char **) refspec);
errs = match_explicit_refs(src, *dst, dst_tail, rs, nr_refspec);
 
+   prepare_searchable_ref_list(*dst, ref_list);
+
/* pick the remainder */
for (ref = src; ref; ref = ref-next) {
+   struct string_list_item *dst_item;
struct ref *dst_peer;
const struct refspec *pat = NULL;
char *dst_name;
@@ -1338,7 +1351,8 @@ int match_push_refs(struct ref *src, struct ref **dst,
if (!dst_name)
continue;
 
-   dst_peer = find_ref_by_name(*dst, dst_name);
+   dst_item = string_list_lookup(ref_list, dst_name);
+   dst_peer = dst_item ? dst_item-util : NULL;
if (dst_peer) {
if (dst_peer-peer_ref)
/* We're already sending something to this ref. 
*/
@@ -1355,6 +1369,8 @@ int match_push_refs(struct ref *src, struct ref **dst,
/* Create a new one and link it */
dst_peer = make_linked_ref(dst_name, dst_tail);
hashcpy(dst_peer-new_sha1, ref-new_sha1);
+   string_list_insert(ref_list, dst_peer-name)-util =
+   dst_peer;
}
dst_peer-peer_ref = copy_ref(ref);
dst_peer-force = pat-force;
@@ -1362,6 +1378,8 @@ int match_push_refs(struct ref *src, struct ref **dst,
free(dst_name);
}
 
+   string_list_clear(ref_list, 0);
+
if (flags  MATCH_REFS_FOLLOW_TAGS)
add_missing_tags(src, dst, dst_tail);
 
@@ -1376,11 +1394,15 @@ int match_push_refs(struct ref *src, struct ref **dst,
 
src_name = get_ref_match(rs, nr_refspec, ref, 
send_mirror, FROM_DST, NULL);
if (src_name) {
-   if (!find_ref_by_name(src, src_name))
+   if (!ref_list.nr)
+   prepare_searchable_ref_list(src,
+   ref_list);
+   if (!string_list_has_string(ref_list, 
src_name))
ref-peer_ref = alloc_delete_ref();
free(src_name);
}
}
+   string_list_clear(ref_list, 0);
}
if (errs)
return -1;
-- 
1.8.3.1.440.gc2bf105

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] http.c: don't rewrite the user:passwd string multiple times

[Brandon Casey]

On Mon, Jun 17, 2013 at 10:19 PM, Jeff King p...@peff.net wrote:
 On Mon, Jun 17, 2013 at 07:00:40PM -0700, Brandon Casey wrote:

 Curl requires that we manage any strings that we pass to it as pointers.
 So, we should not be overwriting this strbuf after we've passed it to
 curl.

 My understanding of curl's pointer requirements are:

   1. Older versions of curl (and I do not recall which version off-hand,
  but it is not important) stored just the pointer. Calling code was
  required to manage the string lifetime itself.

Daniel mentions that the change happened in libcurl 7.17.  RHEL 4.X
(yes, ancient, dead, I realize) provides 7.12 and RHEL 5.X (yes,
ancient, but still widely in use) provides 7.15.  Just pointing it
out.

   2. Newer versions of curl will strdup the string in curl_easy_setopt.

 So we do not have to worry about newer versions, as they do not care
 about our pointer after curl_easy_setopt returns.

I was probably reading the docs on one of these older platforms when I
wrote this.  I've actually had this patch sitting around for a while.

 For older versions, if we were to grow the strbuf, we might free() the
 pointer provided to an earlier call to curl_easy_setopt. But since we
 are about to call curl_easy_setopt with the new value, I would assume
 that curl will never actually look at the old one (i.e., when replacing
 an old pointer, it would not dereference it, but simply overwrite it
 with the new value).

 So for a single curl handle, I don't think it is a problem.

 It could be a problem when we have multiple handles in play
 simultaneously (we invalidate the pointer that another simultaneous
 handle is using, but do not immediately reset its pointer).

Don't we have multiple handles in play at the same time?  What's going
on in get_active_slot() when USE_CURL_MULTI is defined?  It appears to
be maintaining a list of slot 's, each with its own curl handle
initialized either by curl_easy_duphandle() or get_curl_handle().

So, yeah, this is what I was referring to when I mentioned
potentially dangerous.  Since the current code does not change the
size of the string, the pointer will never change, so we won't ever
invalidate a pointer that another handle is using.

The other thing I thought was potentially dangerous, was just
truncating the string.  Again, if there are multiple curl handles in
use (which I thought was a possibility), then merely truncating the
string that contains the username/password could potentially cause a
problem for another handle that could be in the middle of
authenticating using the string.  But, I don't know if there is any
multi-processing happening within the curl library.

snip

Snip the remaining comments about allowing the user to specify
multiple passwords since I'm not sure they're relevant if we are
indeed using multiple curl handles.

If we _don't_ ever use multiple curl handles, and/or if there is no
threading going on in the background within libcurl, then I don't
think there is really any danger in what the current code does.  It
would just be an issue of needlessly rewriting the same string over
and over again, which is probably not a big deal depending on how
often that happens.

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] http.c: don't rewrite the user:passwd string multiple times

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Curl older than 7.17 (RHEL 4.X provides 7.12 and RHEL 5.X provides
7.15) requires that we manage any strings that we pass to it as
pointers.  So, we really shouldn't be modifying this strbuf after we
have passed it to curl.

Our interaction with curl is currently safe (before or after this
patch) since the pointer that is passed to curl is never invalidated;
it is repeatedly rewritten with the same sequence of characters but
the strbuf functions never need to allocate a larger string, so the
same memory buffer is reused.

This guarantee of safety is somewhat subtle and could be overlooked
by someone who may want to add a more complex handling of the username
and password.  So, let's stop modifying this strbuf after we have
passed it to curl, but also leave a note to describe the assumptions
that have been made about username/password lifetime and to draw
attention to the code.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 http.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/http.c b/http.c
index 92aba59..2d086ae 100644
--- a/http.c
+++ b/http.c
@@ -228,9 +228,15 @@ static void init_curl_http_auth(CURL *result)
 #else
{
static struct strbuf up = STRBUF_INIT;
-   strbuf_reset(up);
-   strbuf_addf(up, %s:%s,
-   http_auth.username, http_auth.password);
+   /*
+* Note that we assume we only ever have a single set of
+* credentials in a given program run, so we do not have
+* to worry about updating this buffer, only setting its
+* initial value.
+*/
+   if (!up.len)
+   strbuf_addf(up, %s:%s,
+   http_auth.username, http_auth.password);
curl_easy_setopt(result, CURLOPT_USERPWD, up.buf);
}
 #endif
-- 
1.8.3.1.440.gc2bf105

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] builtin/checkout.c: don't leak memory in check_tracking_name

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

remote_find_tracking() populates the query struct with an allocated
string in the dst member.  So, we do not need to xstrdup() the string,
since we can transfer ownership from the query struct (which will go
out of scope at the end of this function) to our callback struct, but
we must free the string if it will not be used so we will not leak
memory.

Let's do so.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 builtin/checkout.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index f5b50e5..3be0018 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -838,13 +838,16 @@ static int check_tracking_name(struct remote *remote, 
void *cb_data)
memset(query, 0, sizeof(struct refspec));
query.src = cb-src_ref;
if (remote_find_tracking(remote, query) ||
-   get_sha1(query.dst, cb-dst_sha1))
+   get_sha1(query.dst, cb-dst_sha1)) {
+   free(query.dst);
return 0;
+   }
if (cb-dst_ref) {
+   free(query.dst);
cb-unique = 0;
return 0;
}
-   cb-dst_ref = xstrdup(query.dst);
+   cb-dst_ref = query.dst;
return 0;
 }
 
-- 
1.8.2.415.g63cec41

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] t/t9802: explicitly name the upstream branch to use as a base

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Prior to commit fa83a33b, the 'git checkout' DWIMery would create a
new local branch if the specified branch name did not exist and it
matched exactly one ref in the remotes namespace.  It searched
the remotes namespace for matching refs using a simple comparison
of the trailing portion of the remote ref names.  This approach
could sometimes produce false positives or negatives.

Since fa83a33b, the DWIMery more strictly excludes the remote name
from the ref comparison by iterating through the remotes that are
configured in the .gitconfig file.  This has the side-effect that
any refs that exist in the remotes namespace, but do not match
the destination side of any remote refspec, will not be used by
the DWIMery.

This change in behavior breaks the tests in t9802 which relied on
the old behavior of searching all refs in the remotes namespace,
since the git-p4 script does not configure any remotes in the
.gitconfig.  Let's work around this in these tests by explicitly
naming the upstream branch to base the new local branch on when
calling 'git checkout'.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 t/t9802-git-p4-filetype.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9802-git-p4-filetype.sh b/t/t9802-git-p4-filetype.sh
index eeefa67..b0d1d94 100755
--- a/t/t9802-git-p4-filetype.sh
+++ b/t/t9802-git-p4-filetype.sh
@@ -95,7 +95,7 @@ test_expect_success 'gitattributes setting eol=lf produces lf 
newlines' '
git init 
echo * eol=lf .gitattributes 
git p4 sync //depot@all 
-   git checkout master 
+   git checkout -b master p4/master 
test_cmp $cli/f-unix-orig f-unix 
test_cmp $cli/f-win-as-lf f-win
)
@@ -109,7 +109,7 @@ test_expect_success 'gitattributes setting eol=crlf 
produces crlf newlines' '
git init 
echo * eol=crlf .gitattributes 
git p4 sync //depot@all 
-   git checkout master 
+   git checkout -b master p4/master 
test_cmp $cli/f-unix-as-crlf f-unix 
test_cmp $cli/f-win-orig f-win
)
-- 
1.8.2.415.g63cec41

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] http.c: don't rewrite the user:passwd string multiple times

[Brandon Casey]

From: Brandon Casey draf...@gmail.com

Curl requires that we manage any strings that we pass to it as pointers.
So, we should not be overwriting this strbuf after we've passed it to
curl.

Additionally, it is unnecessary since we only prompt for the user name
and password once, so we end up overwriting the strbuf with the same
sequence of characters each time.  This is why in practice it has not
caused any problems for git's use of curl; the internal strbuf char
pointer does not change, and get's overwritten with the same string
each time.

But it's unnecessary and potentially dangerous, so let's avoid it.

Signed-off-by: Brandon Casey draf...@gmail.com
---
 http.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/http.c b/http.c
index 92aba59..6828269 100644
--- a/http.c
+++ b/http.c
@@ -228,8 +228,8 @@ static void init_curl_http_auth(CURL *result)
 #else
{
static struct strbuf up = STRBUF_INIT;
-   strbuf_reset(up);
-   strbuf_addf(up, %s:%s,
+   if (!up.len)
+   strbuf_addf(up, %s:%s,
http_auth.username, http_auth.password);
curl_easy_setopt(result, CURLOPT_USERPWD, up.buf);
}
-- 
1.8.3.1.440.gc2bf105

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation/CommunityGuidelines

[Brandon Casey]

On Tue, Jun 11, 2013 at 7:40 AM, Michael Haggerty mhag...@alum.mit.edu wrote:

 At the risk of being
 presumptuous myself, I suggest that you show a copy of your email to
 somebody whom you know and respect in the real world, somebody who is
 not immersed in the Git community meltdown.  For example, somebody like
 your mother or father, or a teacher whom you respect, or a member of
 clergy if you are so inclined.  Ask that person's opinion about your email.

 It is so easy to lose perspective in the Internet.

Such excellent advice.  Even if the advice is not taken literally, it
is probably enough to just imagine how that person whom you respect
would respond to the words in your emails.  I am sure I do not do this
enough in my own communications.

I just wanted to draw attention to this wonderful suggestion again.
Sometimes it is necessary to take a step back when discussions get
heated, to regain perspective.

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}

[Brandon Casey]

On Tue, Apr 16, 2013 at 11:09 AM, Ramkumar Ramachandra
artag...@gmail.com wrote:
 While a 'git stash show stash^{/quuxery}' works just fine, a 'git
 stash pop stash^{/quuxery}' complains with: 'stash^{/quuxery} is not a
 stash reference'.

I don't think it is appropriate to use the ^{/text} notation with stashes.

The stash is implemented using the reflog.  The ^{/text} notation
searches the commit history, not the reflog.  So I think it will be
able to match the first entry in your stash stack, but not any of the
other ones.

Try inserting another stash (see below) on top of the one that
contains the string quuxery and I think you'll find that your 'git
stash show stash^{/quuxery}' no longer works.

An extension to the reflog dwimery that implements @{/text} could be
interesting though.

 This confusing behavior arises from the differences
 in logic that 'show' and 'pop' internally employ to validate the
 specified ref.  Document this bug by adding a failing testcase for it.

 Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
 ---
  So sorry about misspelling Junio's address in my previous email.
  Please respond to this one instead.

  So if you look at git-stash.sh:377, you'll notice that it's doing a
  the shell substitution ${REV%@*} to figure out whether the stash
  ref is a valid ref.

 This hacky myopic design has to be done away
  with immediately, and we should really compare the SHA-1 hex of the
  specified ref with those in the stash reflog.

Just a bit of advice, maybe you should think about softening your tone
a bit hmm?  I find this last sentence to be somewhat repelling and
tend to refrain from responding to such.

  The only reason I haven't written a fix yet is because I'm not sure
  why you need this convoluted IS_STASH_LIKE and IS_STASH_REF logic in
  the first place.  Can someone enlighten me as to what is going on?

  t/t3903-stash.sh | 9 +
  1 file changed, 9 insertions(+)

 diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
 index 5dfbda7..04ba983 100755
 --- a/t/t3903-stash.sh
 +++ b/t/t3903-stash.sh
 @@ -583,6 +583,15 @@ test_expect_success 'invalid ref of the form stash@{n}, 
 n = N' '
 git stash drop
  '

 +test_expect_failure 'valid ref of the form stash^{/message}' '
 +   git stash clear 
 +   echo bar  file 
 +   git add file 
 +   git stash save quuxery 

# Save another stash here

echo bash file
git add file
git stash save something

# Now git stash show stash^{/quuxery} no longer works.

 +   git stash show stash^{/quuxery} 
 +   git stash pop stash^{/quuxery}
 +'
 +
  test_expect_success 'stash branch should not drop the stash if the branch 
 exists' '
 git stash clear 
 echo foo file 

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Resend PATCH] t3903 (stash): add failing test for ref of form ^{/message}

[Brandon Casey]

On Tue, Apr 16, 2013 at 12:11 PM, Junio C Hamano gits...@pobox.com wrote:
 Brandon Casey draf...@gmail.com writes:

 The stash is implemented using the reflog.  The ^{/text} notation
 searches the commit history, not the reflog.  So I think it will be
 able to match the first entry in your stash stack, but not any of the
 other ones.

 Good point, together with...

 An extension to the reflog dwimery that implements @{/text} could be
 interesting though.

 log -g --grep=text gives you a way to eyeball, but with
 @{/text} you _might_ have a good way to name the revision.

 I am not however so sure if it is useful outside the context of the
 stash, because the ones you would want to recover from a normal
 reflog is most likely the older version of what you already amended,
 so the latest hit will likely be the post-amend version, not the one
 closer to the original.  You would end up eyeballing the output of
 log --oneline -g -grep=text and cutting from it.

Yeah, I think that's true.  I can't think of a reason, at the moment,
where it would be useful outside of with 'git stash'.  I mainly wanted
to spell out @{/text} so that the mental link could be made back
to the code in git-stash that removes the @* suffix.

-Brandon
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html