[PATCH v2 0/4] git remote improvements

[Thomas Gummerer]

Thanks Peff for the review on the previous round ($gmane/286214).

The series now uses parse_config_key() instead of skip_prefix, and I
added REMOTE_UNCONFIGURED to the enum used in the origin field in
struct remote.

Interdiff below:

diff --git a/remote.c b/remote.c
index 3a4ca9b..d10ae00 100644
--- a/remote.c
+++ b/remote.c
@@ -318,90 +318,88 @@ static void read_branches_file(struct remote *remote)
 static int handle_config(const char *key, const char *value, void *cb)
 {
const char *name;
+   int namelen;
const char *subkey;
struct remote *remote;
struct branch *branch;
-   if (skip_prefix(key, "branch.", )) {
-   subkey = strrchr(name, '.');
-   if (!subkey)
+   if (parse_config_key(key, "branch", , , ) >= 0) {
+   if (!name)
return 0;
-   branch = make_branch(name, subkey - name);
-   if (!strcmp(subkey, ".remote")) {
+   branch = make_branch(name, namelen);
+   if (!strcmp(subkey, "remote")) {
return git_config_string(>remote_name, key, 
value);
-   } else if (!strcmp(subkey, ".pushremote")) {
+   } else if (!strcmp(subkey, "pushremote")) {
return git_config_string(>pushremote_name, key, 
value);
-   } else if (!strcmp(subkey, ".merge")) {
+   } else if (!strcmp(subkey, "merge")) {
if (!value)
return config_error_nonbool(key);
add_merge(branch, xstrdup(value));
}
return 0;
}
-   if (skip_prefix(key, "url.", )) {
+   if (parse_config_key(key, "url", , , ) >= 0) {
struct rewrite *rewrite;
-   subkey = strrchr(name, '.');
-   if (!subkey)
+   if (!name)
return 0;
-   if (!strcmp(subkey, ".insteadof")) {
-   rewrite = make_rewrite(, name, subkey - name);
+   if (!strcmp(subkey, "insteadof")) {
+   rewrite = make_rewrite(, name, namelen);
if (!value)
return config_error_nonbool(key);
add_instead_of(rewrite, xstrdup(value));
-   } else if (!strcmp(subkey, ".pushinsteadof")) {
-   rewrite = make_rewrite(_push, name, subkey - 
name);
+   } else if (!strcmp(subkey, "pushinsteadof")) {
+   rewrite = make_rewrite(_push, name, namelen);
if (!value)
return config_error_nonbool(key);
add_instead_of(rewrite, xstrdup(value));
}
}
 
-   if (!skip_prefix(key, "remote.", ))
+   if (parse_config_key(key, "remote", , , ) < 0)
return 0;
 
/* Handle remote.* variables */
-   if (!strcmp(name, "pushdefault"))
+   if (!strcmp(subkey, "pushdefault"))
return git_config_string(_name, key, value);
 
/* Handle remote..* variables */
-   if (*name == '/') {
+   if (*(name ? name : subkey) == '/') {
warning("Config remote shorthand cannot begin with '/': %s",
-   name);
+   name ? name : subkey);
return 0;
}
-   subkey = strrchr(name, '.');
-   if (!subkey)
+   if (!name)
return 0;
-   remote = make_remote(name, subkey - name);
+   remote = make_remote(name, namelen);
remote->origin = REMOTE_CONFIG;
-   if (!strcmp(subkey, ".mirror"))
+   if (!strcmp(subkey, "mirror"))
remote->mirror = git_config_bool(key, value);
-   else if (!strcmp(subkey, ".skipdefaultupdate"))
+   else if (!strcmp(subkey, "skipdefaultupdate"))
remote->skip_default_update = git_config_bool(key, value);
-   else if (!strcmp(subkey, ".skipfetchall"))
+   else if (!strcmp(subkey, "skipfetchall"))
remote->skip_default_update = git_config_bool(key, value);
-   else if (!strcmp(subkey, ".prune"))
+   else if (!strcmp(subkey, "prune"))
remote->prune = git_config_bool(key, value);
-   else if (!strcmp(subkey, ".url")) {
+   else if (!strcmp(subkey, "url")) {
const char *v;
if (git_config_string(, key, value))
return -1;
add_url(remote, v);
-   } else if (!strcmp(subkey, ".pushurl")) {
+   } else if (!strcmp(subkey, "pushurl")) {
const char *v;
if (git_config_string(, key, value))
return -1;
add_pushurl(remote, v);
-   } else if (!strcmp(subkey, ".push")) {
+   } else if (!strcmp(subkey, "push")) {
const char *v;
if (git_config_string(, 

Re: [PATCH v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Ramsay Jones]

On 15/02/16 21:40, Jeff King wrote:
> On Mon, Feb 15, 2016 at 09:36:23PM +, Ramsay Jones wrote:
> 
>>> +test_expect_success '--show-origin stdin' '
>>> +   cat >expect <<-\EOF &&
>>> +   stdin:  user.custom=true
>>
>> So, as with the previous patch, I think this should be:
>>  file:user.custom=true
> 
> That's ambiguous with a file named "", which was the point of
> having the two separate prefixes in the first place.
> 
> I think in practice we _could_ get by with an ambiguous output (it's not
> like "" is a common filename), but that was discussed earlier in
> the thread, and Lars decided to go for something unambiguous.

sure, I just don't think it would cause a problem in practice.
How about using '-' for ? Hmm, you can actually create
such a file in the filesystem! Oh well, I guess its not a big deal.

> 
> That doesn't necessarily have to bleed over into the error messages,
> though (which could continue to use "" if we want to put in a
> little extra code to covering the cases separately.

Yep.

ATB,
Ramsay Jones


--
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 2/4] remote: simplify remote_is_configured()

[Thomas Gummerer]

The remote_is_configured() function allows checking whether a remote
exists or not.  The function however only works if remote_get() wasn't
called before calling it.  In addition, it only checks the configuration
for remotes, but not remotes or branches files.

Make use of the origin member of struct remote instead, which indicates
where the remote comes from.  It will be set to some value if the remote
is configured in any file in the repository, but is initialized to 0 if
the remote is only created in make_remote().

Signed-off-by: Thomas Gummerer 
---
 builtin/fetch.c  |  5 ++---
 builtin/remote.c | 12 ++--
 remote.c | 13 ++---
 remote.h |  3 ++-
 4 files changed, 12 insertions(+), 21 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8e74213..8121830 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1016,10 +1016,9 @@ static int add_remote_or_group(const char *name, struct 
string_list *list)
 
git_config(get_remote_group, );
if (list->nr == prev_nr) {
-   struct remote *remote;
-   if (!remote_is_configured(name))
+   struct remote *remote = remote_get(name);
+   if (!remote_is_configured(remote))
return 0;
-   remote = remote_get(name);
string_list_append(list, remote->name);
}
return 1;
diff --git a/builtin/remote.c b/builtin/remote.c
index 2b2ff9b..d966262 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1441,9 +1441,9 @@ static int set_remote_branches(const char *remotename, 
const char **branches,
 
strbuf_addf(, "remote.%s.fetch", remotename);
 
-   if (!remote_is_configured(remotename))
-   die(_("No such remote '%s'"), remotename);
remote = remote_get(remotename);
+   if (!remote_is_configured(remote))
+   die(_("No such remote '%s'"), remotename);
 
if (!add_mode && remove_all_fetch_refspecs(remotename, key.buf)) {
strbuf_release();
@@ -1498,9 +1498,9 @@ static int get_url(int argc, const char **argv)
 
remotename = argv[0];
 
-   if (!remote_is_configured(remotename))
-   die(_("No such remote '%s'"), remotename);
remote = remote_get(remotename);
+   if (!remote_is_configured(remote))
+   die(_("No such remote '%s'"), remotename);
 
url_nr = 0;
if (push_mode) {
@@ -1566,9 +1566,9 @@ static int set_url(int argc, const char **argv)
if (delete_mode)
oldurl = newurl;
 
-   if (!remote_is_configured(remotename))
-   die(_("No such remote '%s'"), remotename);
remote = remote_get(remotename);
+   if (!remote_is_configured(remote))
+   die(_("No such remote '%s'"), remotename);
 
if (push_mode) {
strbuf_addf(_buf, "remote.%s.pushurl", remotename);
diff --git a/remote.c b/remote.c
index bcd8eb6..d10ae00 100644
--- a/remote.c
+++ b/remote.c
@@ -713,18 +713,9 @@ struct remote *pushremote_get(const char *name)
return remote_get_1(name, pushremote_for_branch);
 }
 
-int remote_is_configured(const char *name)
+int remote_is_configured(struct remote *remote)
 {
-   struct remotes_hash_key lookup;
-   struct hashmap_entry lookup_entry;
-   read_config();
-
-   init_remotes_hash();
-   lookup.str = name;
-   lookup.len = strlen(name);
-   hashmap_entry_init(_entry, memhash(name, lookup.len));
-
-   return hashmap_get(_hash, _entry, ) != NULL;
+   return remote && remote->origin;
 }
 
 int for_each_remote(each_remote_fn fn, void *priv)
diff --git a/remote.h b/remote.h
index 4fd7a0f..c21fd37 100644
--- a/remote.h
+++ b/remote.h
@@ -5,6 +5,7 @@
 #include "hashmap.h"
 
 enum {
+   REMOTE_UNCONFIGURED = 0,
REMOTE_CONFIG,
REMOTE_REMOTES,
REMOTE_BRANCHES
@@ -59,7 +60,7 @@ struct remote {
 
 struct remote *remote_get(const char *name);
 struct remote *pushremote_get(const char *name);
-int remote_is_configured(const char *name);
+int remote_is_configured(struct remote *remote);
 
 typedef int each_remote_fn(struct remote *remote, void *priv);
 int for_each_remote(each_remote_fn fn, void *priv);
-- 
2.7.1.410.g6faf27b

--
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 4/4] remote: use remote_is_configured() for add and rename

[Thomas Gummerer]

Both remote add and remote rename use a slightly different hand-rolled
check if the remote exits.  The hand-rolled check may have some subtle
cases in which it might fail to detect when a remote already exists.
One such case was fixed in fb86e32 ("git remote: allow adding remotes
agreeing with url.<...>.insteadOf").  Another case is when a remote is
configured as follows:

  [remote "foo"]
vcs = bar

If we try to run `git remote add foo bar` with the above remote
configuration, git segfaults.  This change fixes it.

In addition, git remote rename $existing foo with the configuration for
foo as above silently succeeds, even though foo already exists,
modifying its configuration.  With this patch it fails with "remote foo
already exists".

Signed-off-by: Thomas Gummerer 
---
 builtin/remote.c  |  7 ++-
 t/t5505-remote.sh | 18 ++
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 981c487..bd57f1b 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -186,10 +186,7 @@ static int add(int argc, const char **argv)
url = argv[1];
 
remote = remote_get(name);
-   if (remote && (remote->url_nr > 1 ||
-   (strcmp(name, remote->url[0]) &&
-   strcmp(url, remote->url[0])) ||
-   remote->fetch_refspec_nr))
+   if (remote_is_configured(remote))
die(_("remote %s already exists."), name);
 
strbuf_addf(, "refs/heads/test:refs/remotes/%s/test", name);
@@ -641,7 +638,7 @@ static int mv(int argc, const char **argv)
return migrate_file(oldremote);
 
newremote = remote_get(rename.new);
-   if (newremote && (newremote->url_nr > 1 || newremote->fetch_refspec_nr))
+   if (remote_is_configured(newremote))
die(_("remote %s already exists."), rename.new);
 
strbuf_addf(, "refs/heads/test:refs/remotes/%s/test", rename.new);
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index f1d073f..142ae62 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -157,6 +157,24 @@ test_expect_success 'rename errors out early when deleting 
non-existent branch'
)
 '
 
+test_expect_success 'add existing foreign_vcs remote' '
+   git config --add remote.foo.vcs "bar" &&
+   test_when_finished git remote rm foo &&
+   echo "fatal: remote foo already exists." >expect &&
+   test_must_fail git remote add foo bar 2>actual &&
+   test_i18ncmp expect actual
+'
+
+test_expect_success 'add existing foreign_vcs remote' '
+   git config --add remote.foo.vcs "bar" &&
+   git config --add remote.bar.vcs "bar" &&
+   test_when_finished git remote rm foo &&
+   test_when_finished git remote rm bar &&
+   echo "fatal: remote bar already exists." >expect &&
+   test_must_fail git remote rename foo bar 2>actual &&
+   test_i18ncmp expect actual
+'
+
 cat >test/expect 

[PATCH v2 3/4] remote: actually check if remote exits

[Thomas Gummerer]

When converting the git remote command to a builtin in 211c89 ("Make
git-remote a builtin"), a few calls to check if a remote exists were
converted from:
   if (!exists $remote->{$name}) {
  [...]
to:
   remote = remote_get(argv[1]);
   if (!remote)
  [...]

The new check is not quite correct, because remote_get() never returns
NULL if a name is given.  This leaves us with the somewhat cryptic error
message "error: Could not remove config section 'remote.test'", if we
are trying to remove a remote that does not exist, or a similar error if
we try to rename a remote.

Use the remote_is_configured() function to check whether the remote
actually exists, and die with a more sensible error message ("No such
remote: $remotename") instead if it doesn't.

Signed-off-by: Thomas Gummerer 
---
 builtin/remote.c  |  4 ++--
 t/t5505-remote.sh | 18 ++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index d966262..981c487 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -634,7 +634,7 @@ static int mv(int argc, const char **argv)
rename.remote_branches = _branches;
 
oldremote = remote_get(rename.old);
-   if (!oldremote)
+   if (!remote_is_configured(oldremote))
die(_("No such remote: %s"), rename.old);
 
if (!strcmp(rename.old, rename.new) && oldremote->origin != 
REMOTE_CONFIG)
@@ -773,7 +773,7 @@ static int rm(int argc, const char **argv)
usage_with_options(builtin_remote_rm_usage, options);
 
remote = remote_get(argv[1]);
-   if (!remote)
+   if (!remote_is_configured(remote))
die(_("No such remote: %s"), argv[1]);
 
known_remotes.to_delete = remote;
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 1a8e3b8..f1d073f 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -139,6 +139,24 @@ test_expect_success 'remove remote protects local 
branches' '
)
 '
 
+test_expect_success 'remove errors out early when deleting non-existent 
branch' '
+   (
+   cd test &&
+   echo "fatal: No such remote: foo" >expect &&
+   test_must_fail git remote rm foo 2>actual &&
+   test_i18ncmp expect actual
+   )
+'
+
+test_expect_success 'rename errors out early when deleting non-existent 
branch' '
+   (
+   cd test &&
+   echo "fatal: No such remote: foo" >expect &&
+   test_must_fail git remote rename foo bar 2>actual &&
+   test_i18ncmp expect actual
+   )
+'
+
 cat >test/expect 

[PATCH v2 1/4] remote: use parse_config_key

[Thomas Gummerer]

95b567c7 ("use skip_prefix to avoid repeating strings") transformed
calls using starts_with() and then skipping the length of the prefix to
skip_prefix() calls.  In remote.c there are a few calls like:

  if (starts_with(foo, "bar"))
  foo += 3

These calls weren't touched by the aformentioned commit, but can be
replaced by calls to parse_config_key(), to simplify the code and
clarify the intentions.  Do that.

Helped-by: Jeff King 
Signed-off-by: Thomas Gummerer 
---
 remote.c | 71 ++--
 1 file changed, 33 insertions(+), 38 deletions(-)

diff --git a/remote.c b/remote.c
index 02e698a..bcd8eb6 100644
--- a/remote.c
+++ b/remote.c
@@ -318,93 +318,88 @@ static void read_branches_file(struct remote *remote)
 static int handle_config(const char *key, const char *value, void *cb)
 {
const char *name;
+   int namelen;
const char *subkey;
struct remote *remote;
struct branch *branch;
-   if (starts_with(key, "branch.")) {
-   name = key + 7;
-   subkey = strrchr(name, '.');
-   if (!subkey)
+   if (parse_config_key(key, "branch", , , ) >= 0) {
+   if (!name)
return 0;
-   branch = make_branch(name, subkey - name);
-   if (!strcmp(subkey, ".remote")) {
+   branch = make_branch(name, namelen);
+   if (!strcmp(subkey, "remote")) {
return git_config_string(>remote_name, key, 
value);
-   } else if (!strcmp(subkey, ".pushremote")) {
+   } else if (!strcmp(subkey, "pushremote")) {
return git_config_string(>pushremote_name, key, 
value);
-   } else if (!strcmp(subkey, ".merge")) {
+   } else if (!strcmp(subkey, "merge")) {
if (!value)
return config_error_nonbool(key);
add_merge(branch, xstrdup(value));
}
return 0;
}
-   if (starts_with(key, "url.")) {
+   if (parse_config_key(key, "url", , , ) >= 0) {
struct rewrite *rewrite;
-   name = key + 4;
-   subkey = strrchr(name, '.');
-   if (!subkey)
+   if (!name)
return 0;
-   if (!strcmp(subkey, ".insteadof")) {
-   rewrite = make_rewrite(, name, subkey - name);
+   if (!strcmp(subkey, "insteadof")) {
+   rewrite = make_rewrite(, name, namelen);
if (!value)
return config_error_nonbool(key);
add_instead_of(rewrite, xstrdup(value));
-   } else if (!strcmp(subkey, ".pushinsteadof")) {
-   rewrite = make_rewrite(_push, name, subkey - 
name);
+   } else if (!strcmp(subkey, "pushinsteadof")) {
+   rewrite = make_rewrite(_push, name, namelen);
if (!value)
return config_error_nonbool(key);
add_instead_of(rewrite, xstrdup(value));
}
}
 
-   if (!starts_with(key,  "remote."))
+   if (parse_config_key(key, "remote", , , ) < 0)
return 0;
-   name = key + 7;
 
/* Handle remote.* variables */
-   if (!strcmp(name, "pushdefault"))
+   if (!strcmp(subkey, "pushdefault"))
return git_config_string(_name, key, value);
 
/* Handle remote..* variables */
-   if (*name == '/') {
+   if (*(name ? name : subkey) == '/') {
warning("Config remote shorthand cannot begin with '/': %s",
-   name);
+   name ? name : subkey);
return 0;
}
-   subkey = strrchr(name, '.');
-   if (!subkey)
+   if (!name)
return 0;
-   remote = make_remote(name, subkey - name);
+   remote = make_remote(name, namelen);
remote->origin = REMOTE_CONFIG;
-   if (!strcmp(subkey, ".mirror"))
+   if (!strcmp(subkey, "mirror"))
remote->mirror = git_config_bool(key, value);
-   else if (!strcmp(subkey, ".skipdefaultupdate"))
+   else if (!strcmp(subkey, "skipdefaultupdate"))
remote->skip_default_update = git_config_bool(key, value);
-   else if (!strcmp(subkey, ".skipfetchall"))
+   else if (!strcmp(subkey, "skipfetchall"))
remote->skip_default_update = git_config_bool(key, value);
-   else if (!strcmp(subkey, ".prune"))
+   else if (!strcmp(subkey, "prune"))
remote->prune = git_config_bool(key, value);
-   else if (!strcmp(subkey, ".url")) {
+   else if (!strcmp(subkey, "url")) {
const char *v;
if (git_config_string(, key, value))

Re: [PATCH v2 4/4] remote: use remote_is_configured() for add and rename

[Eric Sunshine]

On Mon, Feb 15, 2016 at 5:39 PM, Thomas Gummerer  wrote:
> Both remote add and remote rename use a slightly different hand-rolled
> check if the remote exits.  The hand-rolled check may have some subtle
> cases in which it might fail to detect when a remote already exists.
> One such case was fixed in fb86e32 ("git remote: allow adding remotes
> agreeing with url.<...>.insteadOf").  Another case is when a remote is
> configured as follows:
> [...]
> Signed-off-by: Thomas Gummerer 
> ---
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> @@ -157,6 +157,24 @@ test_expect_success 'rename errors out early when 
> deleting non-existent branch'
> +test_expect_success 'add existing foreign_vcs remote' '
> +   git config --add remote.foo.vcs "bar" &&
> +   git config --add remote.bar.vcs "bar" &&
> +   test_when_finished git remote rm foo &&
> +   test_when_finished git remote rm bar &&

Nit: If the second git-config fails, then none of the cleanup will
happen. You'd either want to re-order them like this:

git config --add remote.foo.vcs "bar" &&
test_when_finished git remote rm foo &&
git config --add remote.bar.vcs "bar" &&
test_when_finished git remote rm bar &&

or this:

test_when_finished git remote rm foo &&
git config --add remote.foo.vcs "bar" &&
test_when_finished git remote rm bar &&
git config --add remote.bar.vcs "bar" &&

or this:

test_when_finished git remote rm foo &&
test_when_finished git remote rm bar &&
git config --add remote.foo.vcs "bar" &&
git config --add remote.bar.vcs "bar" &&

Probably not worth a re-roll, though.

> +   echo "fatal: remote bar already exists." >expect &&
> +   test_must_fail git remote rename foo bar 2>actual &&
> +   test_i18ncmp expect actual
> +'
--
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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Jeff King]

On Mon, Feb 15, 2016 at 02:18:18PM -0800, Junio C Hamano wrote:

> larsxschnei...@gmail.com writes:
> 
> > +test_expect_success 'set up custom config file' '
> > +   CUSTOM_CONFIG_FILE=$(printf "file\twith\ttabs.conf") &&
> > +   cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
> > +   [user]
> > +   custom = true
> > +   EOF
> > +'
> > +
> > +test_expect_success '--show-origin escape special file name characters' '
> > +   cat >expect <<-\EOF &&
> > +   file:"file\twith\ttabs.conf"user.custom=true
> > +   EOF
> > +   git config --file "$CUSTOM_CONFIG_FILE" --show-origin --list >output &&
> > +   test_cmp expect output
> > +'
> 
> Do we really need to use a file with such a name?
> 
> An existing test t3300 tells me that a test that uses a path with a
> tab needs to be skipped on FAT/NTFS.  If your goal is to make sure
> dquote is exercised, can't we just do with a path with a SP in it or
> something?

It has to trigger quote_c_style(). You can see the complete set of
quoted characters in quote.c:sq_lookup, but space is not one of them.
Probably double-quote or backslash is the best bet, as the rest are all
control characters.

-Peff
--
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/4] remote: use parse_config_key

[Jeff King]

On Mon, Feb 15, 2016 at 11:39:41PM +0100, Thomas Gummerer wrote:

> - if (!starts_with(key,  "remote."))
> + if (parse_config_key(key, "remote", , , ) < 0)
>   return 0;
> - name = key + 7;
>  
>   /* Handle remote.* variables */
> - if (!strcmp(name, "pushdefault"))
> + if (!strcmp(subkey, "pushdefault"))
>   return git_config_string(_name, key, value);

I think this needs to become:

  if (!name && !strcmp(subkey, "pushdefault"))

so that we do not match "remote.foo.pushdefault", which is nonsense. The
original avoided it by conflating "name" and "subkey" at various points,
and not parsing out the subkey until later. Making that more explicit is
one of the things that I think is improved by your patch. :)

>   /* Handle remote..* variables */
> - if (*name == '/') {
> + if (*(name ? name : subkey) == '/') {
>   warning("Config remote shorthand cannot begin with '/': %s",
> - name);
> + name ? name : subkey);
>   return 0;
>   }
> - subkey = strrchr(name, '.');
> - if (!subkey)
> + if (!name)
>   return 0;

I think you can bump the "if (!name)" check earlier. If it is empty, we
know that it does not start with "/". And then you can avoid the extra
NULL-checks.

The rest of the patch looks good to me. I hadn't realized initially that
all of the subkey compares would become "foo" and not ".foo". That makes
the diff noisier, but IMHO the result is much better.

-Peff
--
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 4/4] remote: use remote_is_configured() for add and rename

[Jeff King]

On Mon, Feb 15, 2016 at 05:52:14PM -0500, Eric Sunshine wrote:

> > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> > @@ -157,6 +157,24 @@ test_expect_success 'rename errors out early when 
> > deleting non-existent branch'
> > +test_expect_success 'add existing foreign_vcs remote' '
> > +   git config --add remote.foo.vcs "bar" &&
> > +   git config --add remote.bar.vcs "bar" &&
> > +   test_when_finished git remote rm foo &&
> > +   test_when_finished git remote rm bar &&
> 
> Nit: If the second git-config fails, then none of the cleanup will
> happen. You'd either want to re-order them like this:
> 
> git config --add remote.foo.vcs "bar" &&
> test_when_finished git remote rm foo &&
> git config --add remote.bar.vcs "bar" &&
> test_when_finished git remote rm bar &&

Good catch. Do we actually care about "--add" here at all? We do not
expect these remotes to have any existing config, I think. So would:

  test_config remote.foo.vcs bar &&
  test_config remote.bar.vcs bar

do? I guess technically the failing "git remote rename" could introduce
extra config that is not cleaned up by those invocations, and we need to
"git remote rm" to get a clean slate, but I don't think that is the case
now (and it does not seem likely to become so in the future).

-Peff
--
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 +warn] Implement https public key pinning

[Junio C Hamano]

Thanks.  This, when applied on top of 2.7.1, however seems to break
at least t5541 and t5551.



--
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/4] dir.c: fix match_pathname()

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> Given the pattern "1/2/3/4" and the path "1/2/3/4/f", the pattern
> prefix is "1/2/3/4". We will compare and remove the prefix from both
> pattern and path and come to this code
>
>   /*
>* If the whole pattern did not have a wildcard,
>* then our prefix match is all we need; we
>* do not need to call fnmatch at all.
>*/
>   if (!patternlen && !namelen)
>   return 1;
>
> where patternlen is zero (full pattern consumed) and the remaining
> path in "name" is "/f". We fail to realize it's matched in this case
> and fall back to fnmatch(), which also fails to catch it. Fix it.

OK.  And by checking *name against '/', we won't mistakenly say that
"1/2/3/4f" matches the pattern.  Nicely explained.

Can a pattern end with a '/'?

>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  dir.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/dir.c b/dir.c
> index f0b6d0a..bcaafac 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -878,7 +878,7 @@ int match_pathname(const char *pathname, int pathlen,
>* then our prefix match is all we need; we
>* do not need to call fnmatch at all.
>*/
> - if (!patternlen && !namelen)
> + if (!patternlen && (!namelen || *name == '/'))
>   return 1;
>   }
--
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/4] dir.c: support marking some patterns already matched

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> Given path "a" and the pattern "a", it's matched. But if we throw path
> "a/b" to pattern "a", the code fails to realize that if "a" matches
> "a" then "a/b" should also be matched.
>
> When the pattern is matched the first time, we can mark it "sticky", so
> that all files and dirs inside the matched path also matches. This is a
> simpler solution than modify all match scenarios to fix that.

I am not quite sure what this one tries to achieve.  Is this a
performance thing, or is it a correctness thing?

"This is a simpler solution than" is skimpy on the description of
what the solution is.

When you see 'a' and path 'a/', you would throw it in the sticky
list.  when you descend into 'a/' and see things under it,
e.g. 'a/b', you would say "we have a match" because 'a' is sticky.
Do you throw 'a/b' also into the sticky list so that you would catch
'a/b/c' later?  Do you rely on the order of tree walking to cull
entries from the sticky list that are no longer relevant?
e.g. after you enumerate everything in 'a/b', you do not need 'a/b'
anymore.

Or do you notice that 'a/' matched at the top-level and stop
bothering the sticky list when you descend into 'a/b' and others?

How does this interact with negative patterns?

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  dir.c | 77 
> ---
>  dir.h |  3 +++
>  2 files changed, 77 insertions(+), 3 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index 0be7cf1..8a9d8c0 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -521,6 +521,7 @@ void add_exclude(const char *string, const char *base,
>   x->baselen = baselen;
>   x->flags = flags;
>   x->srcpos = srcpos;
> + string_list_init(>sticky_paths, 1);
>   ALLOC_GROW(el->excludes, el->nr + 1, el->alloc);
>   el->excludes[el->nr++] = x;
>   x->el = el;
> @@ -561,8 +562,10 @@ void clear_exclude_list(struct exclude_list *el)
>  {
>   int i;
>  
> - for (i = 0; i < el->nr; i++)
> + for (i = 0; i < el->nr; i++) {
> + string_list_clear(>excludes[i]->sticky_paths, 0);
>   free(el->excludes[i]);
> + }
>   free(el->excludes);
>   free(el->filebuf);
>  
> @@ -889,6 +892,44 @@ int match_pathname(const char *pathname, int pathlen,
>WM_PATHNAME) == 0;
>  }
>  
> +static void add_sticky(struct exclude *exc, const char *pathname, int 
> pathlen)
> +{
> + struct strbuf sb = STRBUF_INIT;
> + int i;
> +
> + for (i = exc->sticky_paths.nr - 1; i >= 0; i--) {
> + const char *sticky = exc->sticky_paths.items[i].string;
> + int len = strlen(sticky);
> +
> + if (pathlen < len && sticky[pathlen] == '/' &&
> + !strncmp(pathname, sticky, pathlen))
> + return;
> + }
> +
> + strbuf_add(, pathname, pathlen);
> + string_list_append_nodup(>sticky_paths, strbuf_detach(, NULL));
> +}
> +
> +static int match_sticky(struct exclude *exc, const char *pathname, int 
> pathlen, int dtype)
> +{
> + int i;
> +
> + for (i = exc->sticky_paths.nr - 1; i >= 0; i--) {
> + const char *sticky = exc->sticky_paths.items[i].string;
> + int len = strlen(sticky);
> +
> + if (pathlen == len && dtype == DT_DIR &&
> + !strncmp(pathname, sticky, len))
> + return 1;
> +
> + if (pathlen > len && pathname[len] == '/' &&
> + !strncmp(pathname, sticky, len))
> + return 1;
> + }
> +
> + return 0;
> +}
> +
>  /*
>   * Scan the given exclude list in reverse to see whether pathname
>   * should be ignored.  The first match (i.e. the last on the list), if
> @@ -914,6 +955,16 @@ static struct exclude 
> *last_exclude_matching_from_list(const char *pathname,
>   const char *exclude = x->pattern;
>   int prefix = x->nowildcardlen;
>  
> + if (x->sticky_paths.nr) {
> + if (*dtype == DT_UNKNOWN)
> + *dtype = get_dtype(NULL, pathname, pathlen);
> + if (match_sticky(x, pathname, pathlen, *dtype)) {
> + exc = x;
> + break;
> + }
> + continue;
> + }
> +
>   if (x->flags & EXC_FLAG_MUSTBEDIR) {
>   if (*dtype == DT_UNKNOWN)
>   *dtype = get_dtype(NULL, pathname, pathlen);
> @@ -947,9 +998,10 @@ static struct exclude 
> *last_exclude_matching_from_list(const char *pathname,
>   return NULL;
>   }
>  
> - trace_printf_key(_exclude, "exclude: %.*s vs %s at line %d => 
> %s\n",
> + trace_printf_key(_exclude, "exclude: %.*s vs %s at line %d => 
> %s%s\n",
>pathlen, pathname, exc->pattern, exc->srcpos,
> -  

Re: [PATCH 0/4] .gitignore, reinclude rules, take 2

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> Take one was bad and reverted in commit 8c72236. Take two provides a
> more complete solution to the pair of rules
>
>   exclude/this
>   !exclude/this/except/this
>
> 3/4 should do a better job at stopping regressions in take 1. 4/4
> provides the solution. I think I have tested (and wrote tests) for all
> the cases I can imagine.

Thanks.  The data structure used in 3/4 smells iffy from performance
point of view--have you tried it on a large trees with deep nesting?

>
> Nguyễn Thái Ngọc Duy (4):
>   dir.c: fix match_pathname()
>   dir.c: support tracing exclude
>   dir.c: support marking some patterns already matched
>   dir.c: don't exclude whole dir prematurely
>
>  Documentation/git-check-ignore.txt  |   1 +
>  Documentation/git.txt   |   5 +
>  Documentation/gitignore.txt |  17 ++-
>  dir.c   | 204 
> +++-
>  dir.h   |   3 +
>  t/t3001-ls-files-others-exclude.sh  |   7 +-
>  t/t3007-ls-files-other-negative.sh (new +x) | 153 +
>  7 files changed, 378 insertions(+), 12 deletions(-)
>  create mode 100755 t/t3007-ls-files-other-negative.sh
--
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] worktree: fix "add -B"

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> Current code does not update "symref" when -B is used. This string
> contains the new HEAD. Because it's empty "git worktree add -B" fails at
> symbolic-ref step.
>
> Because branch creation is already done before calling add_worktree(),
> -B is equivalent to -b from add_worktree() point of view. We do not need
> the special case for -B.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  Complete patch.
>
>  builtin/worktree.c  | 4 +---
>  t/t2025-worktree-add.sh | 8 
>  2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 475b958..6b9c946 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -201,9 +201,7 @@ static int add_worktree(const char *path, const char 
> *refname,
>   die(_("'%s' already exists"), path);
>  
>   /* is 'refname' a branch or commit? */
> - if (opts->force_new_branch) /* definitely a branch */
> - ;
> - else if (!opts->detach && !strbuf_check_branch_ref(, refname) &&
> + if (!opts->detach && !strbuf_check_branch_ref(, refname) &&
>ref_exists(symref.buf)) { /* it's a branch */

Makes a reader wonder why the original thought it was OK to do
nothing when -B is given here.

What does symref.buf have at this point in the codeflow?  Will it
always an existing branch?  In what case can it be the name of a
branch that does not yet exist?

Thanks.

>   if (!opts->force)
>   die_if_checked_out(symref.buf);
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 0a804da..a4d36c0 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -193,6 +193,14 @@ test_expect_success '"add" -B/--detach mutually 
> exclusive' '
>   test_must_fail git worktree add -B poodle --detach bamboo master
>  '
>  
> +test_expect_success 'add -B' '
> + git worktree add -B poodle bamboo2 master^ &&
> + git -C bamboo2 symbolic-ref HEAD >actual &&
> + echo refs/heads/poodle >expected &&
> + test_cmp expected actual &&
> + test_cmp_rev master^ poodle
> +'
> +
>  test_expect_success 'local clone from linked checkout' '
>   git clone --local here here-clone &&
>   ( cd here-clone && git fsck )
--
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/4] remote: use parse_config_key

[Thomas Gummerer]

On 02/15, Jeff King wrote:
> On Mon, Feb 15, 2016 at 11:39:41PM +0100, Thomas Gummerer wrote:
>
> > -   if (!starts_with(key,  "remote."))
> > +   if (parse_config_key(key, "remote", , , ) < 0)
> > return 0;
> > -   name = key + 7;
> >
> > /* Handle remote.* variables */
> > -   if (!strcmp(name, "pushdefault"))
> > +   if (!strcmp(subkey, "pushdefault"))
> > return git_config_string(_name, key, value);
>
> I think this needs to become:
>
>   if (!name && !strcmp(subkey, "pushdefault"))
>
> so that we do not match "remote.foo.pushdefault", which is nonsense. The
> original avoided it by conflating "name" and "subkey" at various points,
> and not parsing out the subkey until later. Making that more explicit is
> one of the things that I think is improved by your patch. :)

Good catch.  I'll fix this in the re-roll.

> > /* Handle remote..* variables */
> > -   if (*name == '/') {
> > +   if (*(name ? name : subkey) == '/') {
> > warning("Config remote shorthand cannot begin with '/': %s",
> > -   name);
> > +   name ? name : subkey);
> > return 0;
> > }
> > -   subkey = strrchr(name, '.');
> > -   if (!subkey)
> > +   if (!name)
> > return 0;
>
> I think you can bump the "if (!name)" check earlier. If it is empty, we
> know that it does not start with "/". And then you can avoid the extra
> NULL-checks.

Thanks, will change that.

> The rest of the patch looks good to me. I hadn't realized initially that
> all of the subkey compares would become "foo" and not ".foo". That makes
> the diff noisier, but IMHO the result is much better.
>
> -Peff

--
Thomas
--
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 4/4] remote: use remote_is_configured() for add and rename

[Thomas Gummerer]

On 02/15, Jeff King wrote:
> On Mon, Feb 15, 2016 at 05:52:14PM -0500, Eric Sunshine wrote:
>
> > > diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> > > @@ -157,6 +157,24 @@ test_expect_success 'rename errors out early when 
> > > deleting non-existent branch'
> > > +test_expect_success 'add existing foreign_vcs remote' '
> > > +   git config --add remote.foo.vcs "bar" &&
> > > +   git config --add remote.bar.vcs "bar" &&
> > > +   test_when_finished git remote rm foo &&
> > > +   test_when_finished git remote rm bar &&
> >
> > Nit: If the second git-config fails, then none of the cleanup will
> > happen. You'd either want to re-order them like this:
> >
> > git config --add remote.foo.vcs "bar" &&
> > test_when_finished git remote rm foo &&
> > git config --add remote.bar.vcs "bar" &&
> > test_when_finished git remote rm bar &&
>
> Good catch. Do we actually care about "--add" here at all? We do not
> expect these remotes to have any existing config, I think. So would:
>
>   test_config remote.foo.vcs bar &&
>   test_config remote.bar.vcs bar
>
> do? I guess technically the failing "git remote rename" could introduce
> extra config that is not cleaned up by those invocations, and we need to
> "git remote rm" to get a clean slate, but I don't think that is the case
> now (and it does not seem likely to become so in the future).

Good point, I think the test_config is indeed enough.  Thanks, both,
will fix in the re-roll.

> -Peff

--
Thomas
--
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] merge-recursive: option to disable renames

[Felipe Gonçalves Assis]

The recursive strategy turns on rename detection by default. Add a
strategy option to disable rename detection even for exact renames.

Signed-off-by: Felipe Gonçalves Assis 
---

Hi, this is a patch relative to the "Custom merge driver with no rename
detection" thread, based on suggestions by Junio C Hamano.

 Documentation/merge-strategies.txt |  4 
 merge-recursive.c  | 16 +---
 merge-recursive.h  |  1 +
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/Documentation/merge-strategies.txt 
b/Documentation/merge-strategies.txt
index 7bbd19b..0528d85 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -81,6 +81,10 @@ no-renormalize;;
Disables the `renormalize` option.  This overrides the
`merge.renormalize` configuration variable.
 
+no-renames;;
+   Turn off rename detection.
+   See also linkgit:git-diff[1] `--no-renames`.
+
 rename-threshold=;;
Controls the similarity threshold used for rename detection.
See also linkgit:git-diff[1] `-M`.
diff --git a/merge-recursive.c b/merge-recursive.c
index 8eabde2..ca67805 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1839,9 +1839,16 @@ int merge_trees(struct merge_options *o,
 
entries = get_unmerged();
record_df_conflict_files(o, entries);
-   re_head  = get_renames(o, head, common, head, merge, entries);
-   re_merge = get_renames(o, merge, common, head, merge, entries);
-   clean = process_renames(o, re_head, re_merge);
+   if (o->detect_rename) {
+   re_head  = get_renames(o, head, common, head, merge, 
entries);
+   re_merge = get_renames(o, merge, common, head, merge, 
entries);
+   clean = process_renames(o, re_head, re_merge);
+   }
+   else {
+   re_head  = xcalloc(1, sizeof(struct string_list));
+   re_merge = xcalloc(1, sizeof(struct string_list));
+   clean = 1;
+   }
for (i = entries->nr-1; 0 <= i; i--) {
const char *path = entries->items[i].string;
struct stage_data *e = entries->items[i].util;
@@ -2039,6 +2046,7 @@ void init_merge_options(struct merge_options *o)
o->diff_rename_limit = -1;
o->merge_rename_limit = -1;
o->renormalize = 0;
+   o->detect_rename = DIFF_DETECT_RENAME;
merge_recursive_config(o);
if (getenv("GIT_MERGE_VERBOSITY"))
o->verbosity =
@@ -2088,6 +2096,8 @@ int parse_merge_opt(struct merge_options *o, const char 
*s)
o->renormalize = 1;
else if (!strcmp(s, "no-renormalize"))
o->renormalize = 0;
+   else if (!strcmp(s, "no-renames"))
+   o->detect_rename = 0;
else if (skip_prefix(s, "rename-threshold=", )) {
if ((o->rename_score = parse_rename_score()) == -1 || *arg 
!= 0)
return -1;
diff --git a/merge-recursive.h b/merge-recursive.h
index 9e090a3..52f0201 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -17,6 +17,7 @@ struct merge_options {
unsigned renormalize : 1;
long xdl_opts;
int verbosity;
+   int detect_rename;
int diff_rename_limit;
int merge_rename_limit;
int rename_score;
-- 
2.7.1.291.gd6478c6

--
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: Custom merge driver with no rename detection

[Felipe Gonçalves Assis]

On 15 February 2016 at 09:03, Junio C Hamano  wrote:
> Felipe Gonçalves Assis  writes:
>
>> However, if you do find this approach acceptable/desirable
>> (rename-threshold > 100%), I can work on the issues pointed out and
>> propose a proper patch.
>
> The caller asks diffcore-rename to detect rename, and the algorithm
> compares things to come up with a similarity score and match things
> up.  And you add an option to the rename detection logic to forbid
> finding any?
>
> To be bluntly honest, the approach sounds like a crazy talk.
>
> If your goal is not to allow rename detection at all, why not teach
> the caller of the diff machinery not to even ask for rename
> detection at all?  merge-recursive.c has a helper function called
> get_renames(), and it calls into the diff machinery passing
> DIFF_DETECT_RENAME option.  As a dirty hack, I think you would
> achieve your desired result if you stop passing that option there.
>
> I called that a "dirty hack", because for the purpose of not
> allowing rename detection inside merge-recursive, I think an even
> better thing to do is to teach the get_renames() helper to report to
> its caller, under your new option, "I found no renames at all"
> without doing anything.
>
> It might be just a simple matter of teaching its callers (there
> probably are two of them, one between the common ancestor and our
> branch and the other between the common ancestor and their branch)
> not call the get_renames() helper at all under your new option, but
> I didn't check these callers closely.

Thanks for all the ideas. In order to have something concrete to
discuss, I submitted the patch "merge-recursive: option to disable
renames".

Is that what you had in mind? I would be happy to receive comments and
either improve it or try something else.

Felipe
--
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] merge_blobs: use strbuf instead of manually-sized mmfile_t

[Jeff King]

On Mon, Feb 15, 2016 at 10:39:39PM +0100, Stefan Frühwirth wrote:

> in one specific circumstance, git-merge-tree exits with a segfault caused by
> "*** Error in `git': malloc(): memory corruption (fast)":
> 
> There has to be at least one commit first (as far as I can tell it doesn't
> matter what content). Then create a tree containing a file with a leading
> newline character (\n) followed by some random string, and another tree with
> a file containing a string without leading newline. Now merge trees:
> Segmentation fault.
> 
> There is a test case[1] kindly provided by chrisrossi, which he crafted
> after I discovered the problem[2] in the context of Pylons/acidfs.

Wow, I had to look up what "git merge-tree" even is. It looks like a
proof-of-concept added by 492e075 (Handling large files with GIT,
2006-02-14) that has somehow hung around forever.

I find some of the merging code there questionable, and I wonder if
people are actually using it.  And yet there is this report, and it has
received one or two fixes over the years. So maybe people are.

Anyway, here is an immediate fix for the memory corruption. I'm pretty
sure the _result_ is still buggy in this case, as explained below. I
suspect this weird add/add case should just be a full conflict (like it
is for the normal merge code), and we should just be using ll_merge()
directly. But I have to admit I have very little desire to think hard on
this crufty code. My first preference would be to remove it, but I don't
want to hurt people who might actually be using it. But they can do
their own hard-thinking.

-- >8 --
Subject: merge_blobs: use strbuf instead of manually-sized mmfile_t

The ancient merge_blobs function (which is used nowhere
except in the equally ancient git-merge-tree, which does
not itself seem to be called by any modern git code), tries
to create a plausible base object for an add/add conflict by
finding the common parts of the "ours" and "theirs" blobs.
It does so by calling xdiff with XDIFF_EMIT_COMMON, and
stores the result in a buffer that is as big as the smaller
of "ours" and "theirs".

In theory, this is right; we cannot have more common content
than is in the smaller of the two blobs. But in practice,
xdiff may give us more: if neither file ends in a newline,
we get the "\nNo newline at end of file" marker.

This is somewhat of a bug in itself (the "no newline" string
becomes part of the blob output!), but much worse is that we
may overflow our output buffer with this string (if the
common content was otherwise close to the size of the
smaller blob).

The minimal fix for the memory corruption is to size the
buffer appropriately. We could do so by manually adding in
an extra 29 bytes for the "no newline" string to our buffer
size. But that's somewhat fragile. Instead, let's replace
the fixed-size output buffer with a strbuf which can grow as
necessary.

Reported-by: Stefan Frühwirth 
Signed-off-by: Jeff King 
---
 merge-blobs.c | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/merge-blobs.c b/merge-blobs.c
index ddca601..acfd110 100644
--- a/merge-blobs.c
+++ b/merge-blobs.c
@@ -51,19 +51,16 @@ static void *three_way_filemerge(const char *path, mmfile_t 
*base, mmfile_t *our
 static int common_outf(void *priv_, mmbuffer_t *mb, int nbuf)
 {
int i;
-   mmfile_t *dst = priv_;
+   struct strbuf *dst = priv_;
 
-   for (i = 0; i < nbuf; i++) {
-   memcpy(dst->ptr + dst->size, mb[i].ptr, mb[i].size);
-   dst->size += mb[i].size;
-   }
+   for (i = 0; i < nbuf; i++)
+   strbuf_add(dst, mb[i].ptr, mb[i].size);
return 0;
 }
 
 static int generate_common_file(mmfile_t *res, mmfile_t *f1, mmfile_t *f2)
 {
-   unsigned long size = f1->size < f2->size ? f1->size : f2->size;
-   void *ptr = xmalloc(size);
+   struct strbuf out = STRBUF_INIT;
xpparam_t xpp;
xdemitconf_t xecfg;
xdemitcb_t ecb;
@@ -75,11 +72,15 @@ static int generate_common_file(mmfile_t *res, mmfile_t 
*f1, mmfile_t *f2)
xecfg.flags = XDL_EMIT_COMMON;
ecb.outf = common_outf;
 
-   res->ptr = ptr;
-   res->size = 0;
+   ecb.priv = 
+   if (xdi_diff(f1, f2, , , ) < 0) {
+   strbuf_release();
+   return -1;
+   }
 
-   ecb.priv = res;
-   return xdi_diff(f1, f2, , , );
+   res->size = out.len; /* avoid long/size_t pointer mismatch below */
+   res->ptr = strbuf_detach(, NULL);
+   return 0;
 }
 
 void *merge_blobs(const char *path, struct blob *base, struct blob *our, 
struct blob *their, unsigned long *size)
-- 
2.7.1.574.gccd43a9



--
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] worktree: fix "add -B"

[Duy Nguyen]

On Tue, Feb 16, 2016 at 6:53 AM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>> Current code does not update "symref" when -B is used. This string
>> contains the new HEAD. Because it's empty "git worktree add -B" fails at
>> symbolic-ref step.
>>
>> Because branch creation is already done before calling add_worktree(),
>> -B is equivalent to -b from add_worktree() point of view. We do not need
>> the special case for -B.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>>  Complete patch.
>>
>>  builtin/worktree.c  | 4 +---
>>  t/t2025-worktree-add.sh | 8 
>>  2 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> index 475b958..6b9c946 100644
>> --- a/builtin/worktree.c
>> +++ b/builtin/worktree.c
>> @@ -201,9 +201,7 @@ static int add_worktree(const char *path, const char 
>> *refname,
>>   die(_("'%s' already exists"), path);
>>
>>   /* is 'refname' a branch or commit? */
>> - if (opts->force_new_branch) /* definitely a branch */
>> - ;
>> - else if (!opts->detach && !strbuf_check_branch_ref(, refname) &&
>> + if (!opts->detach && !strbuf_check_branch_ref(, refname) &&
>>ref_exists(symref.buf)) { /* it's a branch */
>
> Makes a reader wonder why the original thought it was OK to do
> nothing when -B is given here.

The bug is quite subtle. This code is added in f7c9dac. At that
commit, I believe symref is simply a temporary var, to be used by
ref_exists() and nothing else. The next commit 7f44e3d replaces
git-checkout with git-symbolic-ref and symref is used again. But the
symref initialization code is not in the diff context lines, so it's
hard to see that there's one case where symref remains empty.

> What does symref.buf have at this point in the codeflow?

Empty.

> Will it always an existing branch?

It should be.

> In what case can it be the name of a branch that does not yet exist?

You can do "worktree add  ".
-- 
Duy
--
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/4] dir.c: fix match_pathname()

[Duy Nguyen]

On Tue, Feb 16, 2016 at 6:29 AM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>> Given the pattern "1/2/3/4" and the path "1/2/3/4/f", the pattern
>> prefix is "1/2/3/4". We will compare and remove the prefix from both
>> pattern and path and come to this code
>>
>>   /*
>>* If the whole pattern did not have a wildcard,
>>* then our prefix match is all we need; we
>>* do not need to call fnmatch at all.
>>*/
>>   if (!patternlen && !namelen)
>>   return 1;
>>
>> where patternlen is zero (full pattern consumed) and the remaining
>> path in "name" is "/f". We fail to realize it's matched in this case
>> and fall back to fnmatch(), which also fails to catch it. Fix it.
>
> OK.  And by checking *name against '/', we won't mistakenly say that
> "1/2/3/4f" matches the pattern.  Nicely explained.
>
> Can a pattern end with a '/'?

No. At this point, we must have stripped that trailing slash and
turned it to flag EXC_FLAG_MUSTBEDIR.
-- 
Duy
--
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 +warn] Implement https public key pinning

[Jeff King]

On Mon, Feb 15, 2016 at 03:25:32PM -0800, Junio C Hamano wrote:

> Thanks.  This, when applied on top of 2.7.1, however seems to break
> at least t5541 and t5551.

Hrm. I cannot see how the new code can possibly do anything unless
http.pinnedpubkey is set, and our tests don't do that. Neither t5541 nor
t5551 fails for me with the patch on top of v2.7.1 (or current "pu", for
that matter).

What does the failure look like?

-Peff
--
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

Git Submodule auto-update

[Cameron W]

I apologize if this is a dumb or repeated question.

Is there a way to have a submodule automatically 'update' on pull of the 
parent repository, WITHOUT requiring each user/committer to change any 
settings (hooks or git config aliases)?

Ideally, a setting I can change at the repository level and then commit 
and apply to all users who clone the repository.

Thanks. 

--
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/4] dir.c: support marking some patterns already matched

[Duy Nguyen]

On Tue, Feb 16, 2016 at 6:47 AM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>> Given path "a" and the pattern "a", it's matched. But if we throw path
>> "a/b" to pattern "a", the code fails to realize that if "a" matches
>> "a" then "a/b" should also be matched.
>>
>> When the pattern is matched the first time, we can mark it "sticky", so
>> that all files and dirs inside the matched path also matches. This is a
>> simpler solution than modify all match scenarios to fix that.
>
> I am not quite sure what this one tries to achieve.  Is this a
> performance thing, or is it a correctness thing?
>
> "This is a simpler solution than" is skimpy on the description of
> what the solution is.

As an example, let's look at pattern "a/". For this pattern to match,
"a" must be a directory. When we examine "a", we can stat() and see if
it is a directory. But when we descend inside, say "a/b", current code
is not prepared to deal with it and will try to stat("a/b") to see if
it's a directory instead of stat("a").

> When you see 'a' and path 'a/', you would throw it in the sticky
> list.  when you descend into 'a/' and see things under it,
> e.g. 'a/b', you would say "we have a match" because 'a' is sticky.
> Do you throw 'a/b' also into the sticky list so that you would catch
> 'a/b/c' later?

No because "a/" can still do the job. I know it's a bit hard to see
because add_sticky() is not used yet in this patch.

> Do you rely on the order of tree walking to cull
> entries from the sticky list that are no longer relevant?
> e.g. after you enumerate everything in 'a/b', you do not need 'a/b'
> anymore.

No explicit culling. But notice that these sticky lists are indirectly
contained in exclude_list. Suppose "a/b" is sticky because of
"a/.gitignore". As soon as we move out of "a", I would expect
prep_exclude() to remove the exclude_list of "a/.gitignore" and the
related sticky lists.

> Or do you notice that 'a/' matched at the top-level and stop
> bothering the sticky list when you descend into 'a/b' and others?
>
> How does this interact with negative patterns?

Negative or not is irrelevant. If "a" is matched negatively and we see
"a/b", we return the same response, "negative match".

> Thanks.  The data structure used in 3/4 smells iffy from performance
> point of view--have you tried it on a large trees with deep nesting?

No I haven't. Though I don't expect it to degrade performance much. We
basically add one new exclude rule when add_sticky() is called and pay
the price of pathname matching of that rule. If there are a lot of
sticky rules (especially ones at top directory), then performance can
go to hell. 4/4 should not add many of them though.
-- 
Duy
--
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 04/18] add helpers for allocating flex-array structs

[Eric Sunshine]

On Mon, Feb 15, 2016 at 4:50 PM, Jeff King  wrote:
> Allocating a struct with a flex array is pretty simple in
> practice: you over-allocate the struct, then copy some data
> into the over-allocation. But it can be a slight pain to
> make sure you're allocating and copying the right amounts.
>
> This patch adds a few helpers to turn simple cases of into a

Grammo: "cases of into"

> one-liner that properly checks for overflow. See the
> embedded documentation for details.
> [...]
> Signed-off-by: Jeff King 
> ---
> diff --git a/git-compat-util.h b/git-compat-util.h
> @@ -782,6 +782,68 @@ extern FILE *fopen_for_writing(const char *path);
> + *   struct foo *f;
> + *   FLEX_ALLOC_STR(f, name, src);
> + *
> + * and "name" will point to a block of memory after the struct, which will be
> + * freed along with the struct (but the pointer can be repoined anywhere).

"repoined"?

> + * The *_STR variants accept a string parameter rather than a ptr/len
> + * combination.
--
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/18] convert trivial cases to FLEX_ARRAY macros

[Eric Sunshine]

On Mon, Feb 15, 2016 at 4:52 PM, Jeff King  wrote:
> Using FLEX_ARRAY macros reduces the amount of manual
> computation size we have to do. It also ensures we don't
> overflow size_t, and it makes sure we write the same number
> of bytes that we allocated.
>
> Signed-off-by: Jeff King 
> ---
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> @@ -412,8 +410,7 @@ static struct reflog_expire_cfg *find_cfg_ent(const char 
> *pattern, size_t len)
> !memcmp(ent->pattern, pattern, len))
> return ent;
>
> -   ent = xcalloc(1, (sizeof(*ent) + len));
> -   memcpy(ent->pattern, pattern, len);
> +   FLEX_ALLOC_MEM(ent, pattern, pattern, len);

Does the incoming 'len' already account for the NUL terminator, or was
the original code underallocating?

Answering my own question: Looking at reflog_expire_config() and
parse_config_key(), I gather that 'len' already accounts for the NUL,
thus the new code is overallocating (which should not be a problem).

> ent->len = len;
> *reflog_expire_cfg_tail = ent;
> reflog_expire_cfg_tail = &(ent->next);
> diff --git a/hashmap.c b/hashmap.c
> @@ -256,10 +256,9 @@ const void *memintern(const void *data, size_t len)
> e = hashmap_get(, , data);
> if (!e) {
> /* not found: create it */
> -   e = xmallocz(sizeof(struct pool_entry) + len);
> +   FLEX_ALLOC_MEM(e, data, data, len);

Ditto. I guess the new code is overallocating (which should be okay).

> hashmap_entry_init(e, key.ent.hash);
> e->len = len;
> -   memcpy(e->data, data, len);
> hashmap_add(, e);
> }
> return e->data;
--
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] wt-status.c: set commitable bit if there is a meaningful merge.

[Stephen P. Smith]

The 'commit --dry-run' and commit return values differed if a
conflicted merge had been resolved and the commit would be the same as
the parent.

Update show_merge_in_progress to set the commitable bit if conflicts
have been resolved and a merge is in progress.

Signed-off-by: Stephen P. Smith 
---

Notes:
In the original report when the dry run switch was passed and after
the merge commit was resolved head and index matched leading to a
returned value of 1. [1]

If the dry run switch was not passed, the commit would succeed to
correctly record the resolution.

The result was that a dry run would report that there would be a
failure, but there really isn't a failure if the commit is actually
attemped.

[1] $gmane/276591

It appeared that the conditional for 'Reject an attempt to record a
non-merge empty commit without * explicit --allow-empty.' could be
simplified after adding this patch.

This change can't be propagated to the conditional because it allows
a commit that was previously disallowed.

 t/t7501-commit.sh | 20 
 wt-status.c   |  1 +
 2 files changed, 21 insertions(+)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 63e0427..363abb1 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -587,4 +587,24 @@ test_expect_success '--only works on to-be-born branch' '
test_cmp expected actual
 '
 
+test_expect_success '--dry-run with conflicts fixed from a merge' '
+   # setup two branches with conflicting information
+   # in the same file, resolve the conflict,
+   # call commit with --dry-run
+   echo "Initial contents, unimportant" >test-file &&
+   git add test-file &&
+   git commit -m "Initial commit" &&
+   echo "commit-1-state" >test-file &&
+   git commit -m "commit 1" -i test-file &&
+   git tag commit-1 &&
+   git checkout -b branch-2 HEAD^1 &&
+   echo "commit-2-state" >test-file &&
+   git commit -m "commit 2" -i test-file &&
+   ! $(git merge --no-commit commit-1) &&
+   echo "commit-2-state" >test-file &&
+   git add test-file &&
+   git commit --dry-run &&
+   git commit -m "conflicts fixed from merge."
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index ab4f80d..1374b48 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -950,6 +950,7 @@ static void show_merge_in_progress(struct wt_status *s,
status_printf_ln(s, color,
_("  (fix conflicts and run \"git commit\")"));
} else {
+   s-> commitable = 1;
status_printf_ln(s, color,
_("All conflicts fixed but you are still merging."));
if (s->hints)
-- 
2.7.0.GIT

--
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 04/18] add helpers for allocating flex-array structs

[Jeff King]

On Mon, Feb 15, 2016 at 08:47:30PM -0500, Eric Sunshine wrote:

> > This patch adds a few helpers to turn simple cases of into a
> 
> Grammo: "cases of into"

Oops. Cases of "flex-array struct allocation into...".

> > + * and "name" will point to a block of memory after the struct, which will 
> > be
> > + * freed along with the struct (but the pointer can be repoined anywhere).
> 
> "repoined"?

Repointed.

Fixed patch below.

-- >8 --
Subject: [PATCH] add helpers for allocating flex-array structs

Allocating a struct with a flex array is pretty simple in
practice: you over-allocate the struct, then copy some data
into the over-allocation. But it can be a slight pain to
make sure you're allocating and copying the right amounts.

This patch adds a few helpers to turn simple cases of
flex-array struct allocation into a one-liner that properly
checks for overflow. See the embedded documentation for
details.

Ideally we could provide a more flexible version that could
handle multiple strings, like:

  FLEX_ALLOC_FMT(ref, name, "%s%s", prefix, name);

But we have to implement this as a macro (because of the
offset calculation of the flex member), which means we would
need all compilers to support variadic macros.

Signed-off-by: Jeff King 
---
 git-compat-util.h | 62 +++
 1 file changed, 62 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 55c073d..e71e615 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -782,6 +782,68 @@ extern FILE *fopen_for_writing(const char *path);
 #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult((alloc), sizeof(*(x
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult((alloc), 
sizeof(*(x
 
+/*
+ * These functions help you allocate structs with flex arrays, and copy
+ * the data directly into the array. For example, if you had:
+ *
+ *   struct foo {
+ * int bar;
+ * char name[FLEX_ARRAY];
+ *   };
+ *
+ * you can do:
+ *
+ *   struct foo *f;
+ *   FLEX_ALLOC_MEM(f, name, src, len);
+ *
+ * to allocate a "foo" with the contents of "src" in the "name" field.
+ * The resulting struct is automatically zero'd, and the flex-array field
+ * is NUL-terminated (whether the incoming src buffer was or not).
+ *
+ * The FLEXPTR_* variants operate on structs that don't use flex-arrays,
+ * but do want to store a pointer to some extra data in the same allocated
+ * block. For example, if you have:
+ *
+ *   struct foo {
+ * char *name;
+ * int bar;
+ *   };
+ *
+ * you can do:
+ *
+ *   struct foo *f;
+ *   FLEX_ALLOC_STR(f, name, src);
+ *
+ * and "name" will point to a block of memory after the struct, which will be
+ * freed along with the struct (but the pointer can be repointed anywhere).
+ *
+ * The *_STR variants accept a string parameter rather than a ptr/len
+ * combination.
+ *
+ * Note that these macros will evaluate the first parameter multiple
+ * times, and it must be assignable as an lvalue.
+ */
+#define FLEX_ALLOC_MEM(x, flexname, buf, len) do { \
+   (x) = NULL; /* silence -Wuninitialized for offset calculation */ \
+   (x) = xalloc_flex(sizeof(*(x)), (char *)(&((x)->flexname)) - (char 
*)(x), (buf), (len)); \
+} while (0)
+#define FLEXPTR_ALLOC_MEM(x, ptrname, buf, len) do { \
+   (x) = xalloc_flex(sizeof(*(x)), sizeof(*(x)), (buf), (len)); \
+   (x)->ptrname = (void *)((x)+1); \
+} while(0)
+#define FLEX_ALLOC_STR(x, flexname, str) \
+   FLEX_ALLOC_MEM((x), flexname, (str), strlen(str))
+#define FLEXPTR_ALLOC_STR(x, ptrname, str) \
+   FLEXPTR_ALLOC_MEM((x), ptrname, (str), strlen(str))
+
+static inline void *xalloc_flex(size_t base_len, size_t offset,
+   const void *src, size_t src_len)
+{
+   unsigned char *ret = xcalloc(1, st_add3(base_len, src_len, 1));
+   memcpy(ret + offset, src, src_len);
+   return ret;
+}
+
 static inline char *xstrdup_or_null(const char *str)
 {
return str ? xstrdup(str) : NULL;
-- 
2.7.1.574.gccd43a9

--
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/18] convert trivial cases to FLEX_ARRAY macros

[Jeff King]

On Mon, Feb 15, 2016 at 09:17:05PM -0500, Eric Sunshine wrote:

> > ---
> > diff --git a/builtin/reflog.c b/builtin/reflog.c
> > @@ -412,8 +410,7 @@ static struct reflog_expire_cfg *find_cfg_ent(const 
> > char *pattern, size_t len)
> > !memcmp(ent->pattern, pattern, len))
> > return ent;
> >
> > -   ent = xcalloc(1, (sizeof(*ent) + len));
> > -   memcpy(ent->pattern, pattern, len);
> > +   FLEX_ALLOC_MEM(ent, pattern, pattern, len);
> 
> Does the incoming 'len' already account for the NUL terminator, or was
> the original code underallocating?
> 
> Answering my own question: Looking at reflog_expire_config() and
> parse_config_key(), I gather that 'len' already accounts for the NUL,
> thus the new code is overallocating (which should not be a problem).

Actually, I think the original underallocates. If we have
gc.foobar.reflogExpire, then "pattern" will poitn to "foobar" and "len"
will be 6. Meaning we allocate without a trailing NUL.

That _should_ be OK, because the struct has a "len" field, and readers
can be careful not to go past it. And indeed, in the loop above, we
check the length and use memcmp().

But later, in set_reflog_expiry_param(), we walk through the list and
hand ent->pattern directly to wildmatch, which assumes a NUL-terminated
string. In practice, it probably works out 7 out of 8 times, because
malloc will align the struct, and we're on a zeroed page, so unless the
string is exactly 8 characters, we'll get some extra NULs afterwards.
But I could demonstrate it by doing:

  gdb --args git -c gc.foobar12.reflogexpire=never reflog expire --all

and breaking on wildmatch, which yields:

  Breakpoint 1, wildmatch (pattern=0x85eb70 "foobar12Q", text=0x85e4d4
"refs/heads/master", flags=0, wo=0x0)

So this is in fact fixing a bug. I can't say I'm terribly surprised
nobody noticed it, as per-ref reflog expiration is pretty obscure.

I hope this increases confidence in my patch series. Even though I
didn't _know_ there was a bug here, I did know that malloc computations
are a potential source of errors. And the FLEX_ALLOC helpers are
designed to remove that work and have a simple interface. We don't know
whether the caller will want a NUL afterwards or not, but we err on the
side of over-allocating by a byte (just as we over-allocate
read_sha1_file() output by a byte), because safety is better than
squeezing out a single byte.

> > diff --git a/hashmap.c b/hashmap.c
> > @@ -256,10 +256,9 @@ const void *memintern(const void *data, size_t len)
> > e = hashmap_get(, , data);
> > if (!e) {
> > /* not found: create it */
> > -   e = xmallocz(sizeof(struct pool_entry) + len);
> > +   FLEX_ALLOC_MEM(e, data, data, len);
> 
> Ditto. I guess the new code is overallocating (which should be okay).

It is, but so was the original (it used xmallocz to get an extra NUL).

-Peff
--
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 +warn] Implement https public key pinning

[Junio C Hamano]

Jeff King  writes:

> On Mon, Feb 15, 2016 at 03:25:32PM -0800, Junio C Hamano wrote:
>
>> Thanks.  This, when applied on top of 2.7.1, however seems to break
>> at least t5541 and t5551.
>
> Hrm. I cannot see how the new code can possibly do anything unless
> http.pinnedpubkey is set, and our tests don't do that. Neither t5541 nor
> t5551 fails for me with the patch on top of v2.7.1 (or current "pu", for
> that matter).

> What does the failure look like?

In t5541, #17 "push (chunked)" fails.

The test expects to see "POST git-receive-pack (chunked)" in the
error output, but here is what I see in $TRASH/test_repo_clone/err:

Pushing to http://127.0.0.1:5541/smart/test_repo.git
POST git-receive-pack (467 bytes)
To http://127.0.0.1:5541/smart/test_repo.git
   8598732..09a7db2  master -> master
updating local tracking ref 'refs/remotes/origin/master'

"git reset --hard HEAD^" to get rid of this patch before retesting
makes the same test pass, so even though I cannot see how this could
make any difference, it apparently is making some difference.

#define LIBCURL_VERSION_NUM 0x072300

I suspect that "#else" is too agressive to bail out or something
silly like that.

Oh, I think I found it.

@@ -216,6 +219,13 @@ static int http_options(const char *var, const char 
*value, void *cb)
if (!strcmp("http.sslcapath", var))
return git_config_pathname(_capath, var, value);
 #endif
+   if (!strcmp("http.pinnedpubkey", var))
+#if LIBCURL_VERSION_NUM >= 0x072c00
+   return git_config_pathname(_pinnedkey, var, value);
+#else
+   warning(_("Public key pinning not supported with cURL < 
7.44.0"));
+   return 0;
+#endif

We are not writing in Python.  Indenting the second line the same
way does not make it part of the block.  Of course by inserting the
new config in the earlier part of the function, it broke everything
that comes after.




if (!strcmp("http.sslcainfo", var))
return git_config_pathname(_cainfo, var, value);
if (!strcmp("http.sslcertpasswordprotected", var)) {
@@ -415,6 +425,10 @@ static CURL *get_curl_handle(void)
if (ssl_capath != NULL)
curl_easy_setopt(result, CURLOPT_CAPATH, ssl_capath);
 #endif
+#if LIBCURL_VERSION_NUM >= 0x072c00
+   if (ssl_pinnedkey != NULL)
+   curl_easy_setopt(result, CURLOPT_PINNEDPUBLICKEY, 
ssl_pinnedkey);
+#endif
if (ssl_cainfo != NULL)
curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
 
--
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/18] convert trivial cases to FLEX_ARRAY macros

[Jeff King]

On Mon, Feb 15, 2016 at 10:15:54PM -0500, Jeff King wrote:

> > Answering my own question: Looking at reflog_expire_config() and
> > parse_config_key(), I gather that 'len' already accounts for the NUL,
> > thus the new code is overallocating (which should not be a problem).
> 
> Actually, I think the original underallocates. If we have
> gc.foobar.reflogExpire, then "pattern" will poitn to "foobar" and "len"
> will be 6. Meaning we allocate without a trailing NUL.
> 
> That _should_ be OK, because the struct has a "len" field, and readers
> can be careful not to go past it. And indeed, in the loop above, we
> check the length and use memcmp().
> 
> But later, in set_reflog_expiry_param(), we walk through the list and
> hand ent->pattern directly to wildmatch, which assumes a NUL-terminated
> string. In practice, it probably works out 7 out of 8 times, because
> malloc will align the struct, and we're on a zeroed page, so unless the
> string is exactly 8 characters, we'll get some extra NULs afterwards.
> But I could demonstrate it by doing:
> 
>   gdb --args git -c gc.foobar12.reflogexpire=never reflog expire --all
> 
> and breaking on wildmatch, which yields:
> 
>   Breakpoint 1, wildmatch (pattern=0x85eb70 "foobar12Q", text=0x85e4d4
>   "refs/heads/master", flags=0, wo=0x0)
> 
> So this is in fact fixing a bug. I can't say I'm terribly surprised
> nobody noticed it, as per-ref reflog expiration is pretty obscure.

We could do this on top of my series (I can also factor out the fix
separately to go at the beginning if we don't want to hold the bugfix
hostage).

-- >8 --
Subject: [PATCH] reflog_expire_cfg: drop misleading "len" parameter

You can tweak the reflog expiration for a particular subset
of refs by configuring gc.foo.reflogexpire. We keep a linked
list of reflog_expire_cfg structs, each of which holds the
pattern and a "len" field for the length of the pattern.

However, we feed the pattern directly to wildmatch(), which
means that it must be a NUL-terminated string. Before the
recent conversion to FLEX_ALLOC_MEM, we got this wrong, and
could feed extra garbage to wildmatch(). That's now fixed,
but the "len" parameter is simply misleading. The pattern is
a string, and we don't need to record its length.

To get rid of it, we do need to tweak the "do we have it
already?" search in find_cfg_ent(), but we can do so without
having a recorded length by just using strncmp.

Signed-off-by: Jeff King 
---
 builtin/reflog.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 7c1990f..2d46b64 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -394,7 +394,6 @@ static struct reflog_expire_cfg {
struct reflog_expire_cfg *next;
unsigned long expire_total;
unsigned long expire_unreachable;
-   size_t len;
char pattern[FLEX_ARRAY];
 } *reflog_expire_cfg, **reflog_expire_cfg_tail;
 
@@ -406,12 +405,11 @@ static struct reflog_expire_cfg *find_cfg_ent(const char 
*pattern, size_t len)
reflog_expire_cfg_tail = _expire_cfg;
 
for (ent = reflog_expire_cfg; ent; ent = ent->next)
-   if (ent->len == len &&
-   !memcmp(ent->pattern, pattern, len))
+   if (!strncmp(ent->pattern, pattern, len) &&
+   ent->pattern[len] == '\0')
return ent;
 
FLEX_ALLOC_MEM(ent, pattern, pattern, len);
-   ent->len = len;
*reflog_expire_cfg_tail = ent;
reflog_expire_cfg_tail = &(ent->next);
return ent;
-- 
2.7.1.574.gccd43a9

--
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 +warn] Implement https public key pinning

[Jeff King]

On Mon, Feb 15, 2016 at 07:19:07PM -0800, Junio C Hamano wrote:

> I suspect that "#else" is too agressive to bail out or something
> silly like that.
> 
> Oh, I think I found it.
> 
> @@ -216,6 +219,13 @@ static int http_options(const char *var, const char 
> *value, void *cb)
>   if (!strcmp("http.sslcapath", var))
>   return git_config_pathname(_capath, var, value);
>  #endif
> + if (!strcmp("http.pinnedpubkey", var))
> +#if LIBCURL_VERSION_NUM >= 0x072c00
> + return git_config_pathname(_pinnedkey, var, value);
> +#else
> + warning(_("Public key pinning not supported with cURL < 
> 7.44.0"));
> + return 0;
> +#endif
> 
> We are not writing in Python.  Indenting the second line the same
> way does not make it part of the block.  Of course by inserting the
> new config in the earlier part of the function, it broke everything
> that comes after.

Oof. Good eyes. I suspected something funny with the #if, but after
starting at it for minutes, couldn't see it.

That makes sense, and the fix is obvious.

-Peff
--
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: [PULL] svn pathnameencoding for git svn dcommit

[Kazutoshi Satoda]

On 2016/02/15 9:52 +0900, Eric Wong wrote:
> I've amended tests to both commits, but the URL encoding one
> requires an HTTP server to test effectively.

Thank you for the tests. But, on my environment, both of them failed
unexpectedly. (Windows 7 SP1, x86_64 Cygwin, LANG=ja_JP.UTF-8)

For now, I don't have enough time to investigate this further. Let me
just share the result.

> $ ./t9115-git-svn-dcommit-funky-renames.sh -x --run='11-12'
(snip)
> expecting success:
> neq=$(printf "\201\202") &&
> git config svn.pathnameencoding cp932 &&
> echo neq >"$neq" &&
> git add "$neq" &&
> git commit -m "neq" &&
> git svn dcommit
>
> +++ printf '\201\202'
> ++ neq=$'\201\202'
> ++ git config svn.pathnameencoding cp932
> ++ echo neq
> ++ git add $'\201\202'
> ++ git commit -m neq
> On branch master
>
> Initial commit
>
> Untracked files:
> svnrepo/
> "\357\202\201\357\202\202"
>
> nothing added to commit but untracked files present
> error: last command exited with $?=1
> not ok 11 - svn.pathnameencoding=cp932 new file on dcommit
> #
> #   neq=$(printf "\201\202") &&
> #   git config svn.pathnameencoding cp932 &&
> #   echo neq >"$neq" &&
> #   git add "$neq" &&
> #   git commit -m "neq" &&
> #   git svn dcommit
> #
>
> expecting success:
> inf=$(printf "\201\207") &&
> git config svn.pathnameencoding cp932 &&
> echo inf >"$inf" &&
> git add "$inf" &&
> git commit -m "inf" &&
> git svn dcommit &&
> git mv "$inf" inf &&
> git commit -m "inf rename" &&
> git svn dcommit
>
> +++ printf '\201\207'
> ++ inf=$'\201\207'
> ++ git config svn.pathnameencoding cp932
> ++ echo inf
> ++ git add $'\201\207'
> ++ git commit -m inf
> On branch master
>
> Initial commit
>
> Untracked files:
> svnrepo/
> "\357\202\201\357\202\202"
> "\357\202\201\357\202\207"
>
> nothing added to commit but untracked files present
> error: last command exited with $?=1
> not ok 12 - svn.pathnameencoding=cp932 rename on dcommit
> #
> #   inf=$(printf "\201\207") &&
> #   git config svn.pathnameencoding cp932 &&
> #   echo inf >"$inf" &&
> #   git add "$inf" &&
> #   git commit -m "inf" &&
> #   git svn dcommit &&
> #   git mv "$inf" inf &&
> #   git commit -m "inf rename" &&
> #   git svn dcommit
> #

Strangely, after the test failure, "git status" in the trash directory
reports different status.
> $ cd 'trash directory.t9115-git-svn-dcommit-funky-renames'
> $ git status
> On branch master
>
> Initial commit
>
> Untracked files:
>   (use "git add ..." to include in what will be committed)
>
> svnrepo/
> "\201\202"
> "\201\207"
>
> nothing added to commit but untracked files present (use "git add" to track)

I can manually add and commit them.
> $ neq=$(printf "\201\202")
> $ git add "$neq"
> $ git commit -m "neq"
> [master (root-commit) 3fd464f] neq
>  1 file changed, 1 insertion(+)
>  create mode 100644 "\201\202"

I can't see how "\357\202\201\357\202\202" came as output in the test.

-- 
k_satoda
--
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/18] convert trivial cases to FLEX_ARRAY macros

[Jeff King]

On Mon, Feb 15, 2016 at 10:26:26PM -0500, Jeff King wrote:

> We could do this on top of my series (I can also factor out the fix
> separately to go at the beginning if we don't want to hold the bugfix
> hostage).
> 
> -- >8 --
> Subject: [PATCH] reflog_expire_cfg: drop misleading "len" parameter

Here it is as a separate fix. Applying my series on top would need a
minor and obvious tweak. I'll hold a re-roll for more comments, but will
otherwise plan to stick this at the front of the series.

-- >8 --
Subject: [PATCH] reflog_expire_cfg: NUL-terminate pattern field

You can tweak the reflog expiration for a particular subset
of refs by configuring gc.foo.reflogexpire. We keep a linked
list of reflog_expire_cfg structs, each of which holds the
pattern and a "len" field for the length of the pattern. The
pattern itself is _not_ NUL-terminated.

However, we feed the pattern directly to wildmatch(), which
expects a NUL-terminated string, meaning it may keep reading
random junk after our struct.

We can fix this by allocating an extra byte for the NUL
(which is already zero because we use xcalloc). Let's also
drop the misleading "len" field, which is no longer
necessary. The existing use of "len" can be converted to use
strncmp().

Signed-off-by: Jeff King 
---
 builtin/reflog.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index f39960e..9980731 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -396,7 +396,6 @@ static struct reflog_expire_cfg {
struct reflog_expire_cfg *next;
unsigned long expire_total;
unsigned long expire_unreachable;
-   size_t len;
char pattern[FLEX_ARRAY];
 } *reflog_expire_cfg, **reflog_expire_cfg_tail;
 
@@ -408,13 +407,12 @@ static struct reflog_expire_cfg *find_cfg_ent(const char 
*pattern, size_t len)
reflog_expire_cfg_tail = _expire_cfg;
 
for (ent = reflog_expire_cfg; ent; ent = ent->next)
-   if (ent->len == len &&
-   !memcmp(ent->pattern, pattern, len))
+   if (!strncmp(ent->pattern, pattern, len) &&
+   ent->pattern[len] == '\0')
return ent;
 
-   ent = xcalloc(1, (sizeof(*ent) + len));
+   ent = xcalloc(1, sizeof(*ent) + len + 1);
memcpy(ent->pattern, pattern, len);
-   ent->len = len;
*reflog_expire_cfg_tail = ent;
reflog_expire_cfg_tail = &(ent->next);
return ent;
-- 
2.7.1.574.gccd43a9

--
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/18] convert trivial cases to FLEX_ARRAY macros

[Eric Sunshine]

On Mon, Feb 15, 2016 at 10:15 PM, Jeff King  wrote:
> On Mon, Feb 15, 2016 at 09:17:05PM -0500, Eric Sunshine wrote:
>> > -   ent = xcalloc(1, (sizeof(*ent) + len));
>> > -   memcpy(ent->pattern, pattern, len);
>> > +   FLEX_ALLOC_MEM(ent, pattern, pattern, len);
>>
>> Does the incoming 'len' already account for the NUL terminator, or was
>> the original code underallocating?
>>
>> Answering my own question: Looking at reflog_expire_config() and
>> parse_config_key(), I gather that 'len' already accounts for the NUL,
>> thus the new code is overallocating (which should not be a problem).
>
> Actually, I think the original underallocates. If we have
> gc.foobar.reflogExpire, then "pattern" will poitn to "foobar" and "len"
> will be 6. Meaning we allocate without a trailing NUL.

Ugh, yeah, I misread the code.

> That _should_ be OK, because the struct has a "len" field, and readers
> can be careful not to go past it. And indeed, in the loop above, we
> check the length and use memcmp().
>
> But later, in set_reflog_expiry_param(), we walk through the list and
> hand ent->pattern directly to wildmatch, which assumes a NUL-terminated
> string. In practice, it probably works out 7 out of 8 times, because
> malloc will align the struct, and we're on a zeroed page, so unless the
> string is exactly 8 characters, we'll get some extra NULs afterwards.
> But I could demonstrate it by doing:
>
>   gdb --args git -c gc.foobar12.reflogexpire=never reflog expire --all
>
> and breaking on wildmatch, which yields:
>
>   Breakpoint 1, wildmatch (pattern=0x85eb70 "foobar12Q", text=0x85e4d4
> "refs/heads/master", flags=0, wo=0x0)

Nice confirmation.
--
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/18] convert trivial cases to FLEX_ARRAY macros

[Eric Sunshine]

On Mon, Feb 15, 2016 at 10:36 PM, Jeff King  wrote:
> On Mon, Feb 15, 2016 at 10:26:26PM -0500, Jeff King wrote:
>> We could do this on top of my series (I can also factor out the fix
>> separately to go at the beginning if we don't want to hold the bugfix
>> hostage).
>>
>> -- >8 --
>> Subject: [PATCH] reflog_expire_cfg: drop misleading "len" parameter
>
> Here it is as a separate fix. Applying my series on top would need a
> minor and obvious tweak. I'll hold a re-roll for more comments, but will
> otherwise plan to stick this at the front of the series.

Yep, I prefer this version of the patch too, as it makes it explicit
that a bug is being fixed rather than it happening "by accident" via
the FLEX_ALLOC_MEM conversion, which is easily overlooked.

> -- >8 --
> Subject: [PATCH] reflog_expire_cfg: NUL-terminate pattern field
>
> You can tweak the reflog expiration for a particular subset
> of refs by configuring gc.foo.reflogexpire. We keep a linked
> list of reflog_expire_cfg structs, each of which holds the
> pattern and a "len" field for the length of the pattern. The
> pattern itself is _not_ NUL-terminated.
>
> However, we feed the pattern directly to wildmatch(), which
> expects a NUL-terminated string, meaning it may keep reading
> random junk after our struct.
>
> We can fix this by allocating an extra byte for the NUL
> (which is already zero because we use xcalloc). Let's also
> drop the misleading "len" field, which is no longer
> necessary. The existing use of "len" can be converted to use
> strncmp().
>
> Signed-off-by: Jeff King 
> ---
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> @@ -408,13 +407,12 @@ static struct reflog_expire_cfg *find_cfg_ent(const 
> char *pattern, size_t len)
> reflog_expire_cfg_tail = _expire_cfg;
>
> for (ent = reflog_expire_cfg; ent; ent = ent->next)
> -   if (ent->len == len &&
> -   !memcmp(ent->pattern, pattern, len))
> +   if (!strncmp(ent->pattern, pattern, len) &&
> +   ent->pattern[len] == '\0')

If 'ent->pattern' is shorter than 'pattern' then the strncmp() will
fail, thus it will short-circuit before ent->pattern[len] has a chance
to access beyond the end of memory allocated for 'ent->pattern'. Okay,
makes sense.

> return ent;
>
> -   ent = xcalloc(1, (sizeof(*ent) + len));
> +   ent = xcalloc(1, sizeof(*ent) + len + 1);
> memcpy(ent->pattern, pattern, len);
> -   ent->len = len;
> *reflog_expire_cfg_tail = ent;
> reflog_expire_cfg_tail = &(ent->next);
> return ent;
> --
> 2.7.1.574.gccd43a9
--
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 05/18] convert trivial cases to ALLOC_ARRAY

[Eric Sunshine]

On Mon, Feb 15, 2016 at 4:51 PM, Jeff King  wrote:
> Each of these cases can be converted to use ALLOC_ARRAY or
> REALLOC_ARRAY, which has two advantages:
>
>   1. It automatically checks the array-size multiplication
>  for overflow.
>
>   2. It always uses sizeof(*array) for the element-size,
>  so that it can never go out of sync with the declared
>  type of the array.
>
> Signed-off-by: Jeff King 
> ---
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 77a51d3..0eabe68 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -854,7 +854,7 @@ static char **get_path_split(void)
> if (!n)
> return NULL;
>
> -   path = xmalloc((n+1)*sizeof(char *));
> +   ALLOC_ARRAY(path, n+1);

Elsewhere in this patch, you've reformatted "x+c" as "x + c"; perhaps
do so here, as well.

> p = envpath;
> i = 0;
> do {
--
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/18] convert trivial cases to FLEX_ARRAY macros

[Jeff King]

On Mon, Feb 15, 2016 at 11:18:56PM -0500, Eric Sunshine wrote:

> > diff --git a/builtin/reflog.c b/builtin/reflog.c
> > @@ -408,13 +407,12 @@ static struct reflog_expire_cfg *find_cfg_ent(const 
> > char *pattern, size_t len)
> > reflog_expire_cfg_tail = _expire_cfg;
> >
> > for (ent = reflog_expire_cfg; ent; ent = ent->next)
> > -   if (ent->len == len &&
> > -   !memcmp(ent->pattern, pattern, len))
> > +   if (!strncmp(ent->pattern, pattern, len) &&
> > +   ent->pattern[len] == '\0')
> 
> If 'ent->pattern' is shorter than 'pattern' then the strncmp() will
> fail, thus it will short-circuit before ent->pattern[len] has a chance
> to access beyond the end of memory allocated for 'ent->pattern'. Okay,
> makes sense.

Yeah. It took me a minute to convince myself that this was correct. If
you have a shorter or more clear way of writing it, I'm open to it. The
best I could come up with is running an extra "strlen" and otherwise
keeping the loop as it is; the performance on that is not as good, but
if performance is a concern, maybe something besides a linear search
would be in order. :)

-Peff
--
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 05/18] convert trivial cases to ALLOC_ARRAY

[Jeff King]

On Mon, Feb 15, 2016 at 11:22:12PM -0500, Eric Sunshine wrote:

> On Mon, Feb 15, 2016 at 4:51 PM, Jeff King  wrote:
> > Each of these cases can be converted to use ALLOC_ARRAY or
> > REALLOC_ARRAY, which has two advantages:
> >
> >   1. It automatically checks the array-size multiplication
> >  for overflow.
> >
> >   2. It always uses sizeof(*array) for the element-size,
> >  so that it can never go out of sync with the declared
> >  type of the array.
> >
> > Signed-off-by: Jeff King 
> > ---
> > diff --git a/compat/mingw.c b/compat/mingw.c
> > index 77a51d3..0eabe68 100644
> > --- a/compat/mingw.c
> > +++ b/compat/mingw.c
> > @@ -854,7 +854,7 @@ static char **get_path_split(void)
> > if (!n)
> > return NULL;
> >
> > -   path = xmalloc((n+1)*sizeof(char *));
> > +   ALLOC_ARRAY(path, n+1);
> 
> Elsewhere in this patch, you've reformatted "x+c" as "x + c"; perhaps
> do so here, as well.

Will do. I noticed while going over this before sending it out that it
may also be technically possible for "n+1" to overflow here (and I think
in a few other places in this patch). I don't know how paranoid we want
to be.

-Peff
--
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 05/18] convert trivial cases to ALLOC_ARRAY

[Eric Sunshine]

On Mon, Feb 15, 2016 at 11:23 PM, Jeff King  wrote:
> On Mon, Feb 15, 2016 at 11:22:12PM -0500, Eric Sunshine wrote:
>> On Mon, Feb 15, 2016 at 4:51 PM, Jeff King  wrote:
>> > -   path = xmalloc((n+1)*sizeof(char *));
>> > +   ALLOC_ARRAY(path, n+1);
>>
>> Elsewhere in this patch, you've reformatted "x+c" as "x + c"; perhaps
>> do so here, as well.
>
> Will do. I noticed while going over this before sending it out that it
> may also be technically possible for "n+1" to overflow here (and I think
> in a few other places in this patch). I don't know how paranoid we want
> to be.

Yes, I also noticed those and considered mentioning it. There was also
some multiplication which might be of concern.

ALLOC_ARRAY(graph->mapping, 2 * graph->column_capacity);

It would be easy enough to manually call st_add() and st_mult() for
those cases, but I haven't examined them closely enough to determine
how likely they would be to overflow, nor do I know if the resulting
noisiness of code is desirable.
--
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] merge_blobs: use strbuf instead of manually-sized mmfile_t

[Eric Sunshine]

On Mon, Feb 15, 2016 at 08:12:58PM -0500, Jeff King wrote:
> On Mon, Feb 15, 2016 at 10:39:39PM +0100, Stefan Frühwirth wrote:
> > in one specific circumstance, git-merge-tree exits with a segfault caused by
> > "*** Error in `git': malloc(): memory corruption (fast)":
> > 
> > There is a test case[1] kindly provided by chrisrossi, which he crafted
> > after I discovered the problem[2] in the context of Pylons/acidfs.
> 
> -- >8 --
> Subject: merge_blobs: use strbuf instead of manually-sized mmfile_t
> 
> [...]
> It does so by calling xdiff with XDIFF_EMIT_COMMON, and
> stores the result in a buffer that is as big as the smaller
> of "ours" and "theirs".
> 
> In theory, this is right; we cannot have more common content
> than is in the smaller of the two blobs. But in practice,
> xdiff may give us more: if neither file ends in a newline,
> we get the "\nNo newline at end of file" marker.
> [...]
> 
> Reported-by: Stefan Frühwirth 
> Signed-off-by: Jeff King 
> ---
> diff --git a/merge-blobs.c b/merge-blobs.c
> @@ -51,19 +51,16 @@ static void *three_way_filemerge(const char *path, 
> mmfile_t *base, mmfile_t *our
>  static int common_outf(void *priv_, mmbuffer_t *mb, int nbuf)
>  {
>   int i;
> - mmfile_t *dst = priv_;
> + struct strbuf *dst = priv_;
>  
> - for (i = 0; i < nbuf; i++) {
> - memcpy(dst->ptr + dst->size, mb[i].ptr, mb[i].size);
> - dst->size += mb[i].size;
> - }
> + for (i = 0; i < nbuf; i++)
> + strbuf_add(dst, mb[i].ptr, mb[i].size);
>   return 0;
>  }
>  
>  static int generate_common_file(mmfile_t *res, mmfile_t *f1, mmfile_t *f2)
>  {
> - unsigned long size = f1->size < f2->size ? f1->size : f2->size;
> - void *ptr = xmalloc(size);
> + struct strbuf out = STRBUF_INIT;
>   xpparam_t xpp;
>   xdemitconf_t xecfg;
>   xdemitcb_t ecb;
> @@ -75,11 +72,15 @@ static int generate_common_file(mmfile_t *res, mmfile_t 
> *f1, mmfile_t *f2)
>   xecfg.flags = XDL_EMIT_COMMON;
>   ecb.outf = common_outf;
>  
> - res->ptr = ptr;
> - res->size = 0;
> + ecb.priv = 
> + if (xdi_diff(f1, f2, , , ) < 0) {
> + strbuf_release();
> + return -1;
> + }
>  
> - ecb.priv = res;
> - return xdi_diff(f1, f2, , , );
> + res->size = out.len; /* avoid long/size_t pointer mismatch below */

It took a minute or two for me to realize that "mismatch below" was
talking about the second argument to strbuf_detach(). I tried
rewriting the comment to mention the second argument explicitly, but
couldn't come up with one sufficiently succinct. Oh well.

> + res->ptr = strbuf_detach(, NULL);
> + return 0;
>  }

My reviewed-by may not be worth much since this code is new to me
too, but this patch looks "obviously correct" to me, so:

Reviewed-by: Eric Sunshine 

Perhaps squash in the following test which I adapted from the
reproduction recipe provided by Chris Rossi[1]?

[1] https://gist.github.com/chrisrossi/f09c8bed70b364f9f12e


--- 8< ---
diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
index 9015e47..1f2aa74 100755
--- a/t/t4300-merge-tree.sh
+++ b/t/t4300-merge-tree.sh
@@ -352,4 +352,22 @@ test_expect_success 'turn tree to file' '
test_cmp expect actual
 '
 
+test_expect_success "don't underallocate result buffer" '
+   test_when_finished "git checkout master" &&
+   git checkout --orphan some &&
+   git rm -rf . &&
+   printf "b\n" >a &&
+   git add a &&
+   git commit -m "first commit" &&
+   printf "\na" >b &&
+   git add b &&
+   git commit -m "second commit, first branch" &&
+   git checkout @^ &&
+   git checkout -b other &&
+   printf "a" >b &&
+   git add b &&
+   git commit -m "second commit, second branch" &&
+   git merge-tree @^ some other
+'
+
 test_done
--- 8< ---
--
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 05/18] convert trivial cases to ALLOC_ARRAY

[Jeff King]

On Mon, Feb 15, 2016 at 11:32:25PM -0500, Eric Sunshine wrote:

> On Mon, Feb 15, 2016 at 11:23 PM, Jeff King  wrote:
> > On Mon, Feb 15, 2016 at 11:22:12PM -0500, Eric Sunshine wrote:
> >> On Mon, Feb 15, 2016 at 4:51 PM, Jeff King  wrote:
> >> > -   path = xmalloc((n+1)*sizeof(char *));
> >> > +   ALLOC_ARRAY(path, n+1);
> >>
> >> Elsewhere in this patch, you've reformatted "x+c" as "x + c"; perhaps
> >> do so here, as well.
> >
> > Will do. I noticed while going over this before sending it out that it
> > may also be technically possible for "n+1" to overflow here (and I think
> > in a few other places in this patch). I don't know how paranoid we want
> > to be.
> 
> Yes, I also noticed those and considered mentioning it. There was also
> some multiplication which might be of concern.
> 
> ALLOC_ARRAY(graph->mapping, 2 * graph->column_capacity);
> 
> It would be easy enough to manually call st_add() and st_mult() for
> those cases, but I haven't examined them closely enough to determine
> how likely they would be to overflow, nor do I know if the resulting
> noisiness of code is desirable.

Yeah, I'm quite sure that one is safe (we set column_capacity to a fixed
integer immediately beforehand). And many of the "+" ones are likely
safe, too.  If "n" is close to wrapping, then allocating "n" structs
will probably fail beforehand (though not always, if you have a ton of
RAM and "n" is a signed int).

But part of the point of this series is that we shouldn't have to wonder
if things are safe. They should just be obviously so, and we should err
on the side of caution. So I think it probably _is_ worth sprinkling
st_add() calls in those places. I'll take a look for the re-roll.

-Peff
--
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 08/18] use st_add and st_mult for allocation size computation

[Eric Sunshine]

On Mon, Feb 15, 2016 at 4:53 PM, Jeff King  wrote:
> If our size computation overflows size_t, we may allocate a
> much smaller buffer than we expected and overflow it. It's
> probably impossible to trigger an overflow in most of these
> sites in practice, but it is easy enough convert their
> additions and multiplications into overflow-checking
> variants. This may be fixing real bugs, and it makes
> auditing the code easier.
>
> Signed-off-by: Jeff King 
> ---
> diff --git a/builtin/apply.c b/builtin/apply.c
> @@ -2632,7 +2632,7 @@ static void update_image(struct image *img,
> -   result = xmalloc(img->len + insert_count - remove_count + 1);
> +   result = xmalloc(st_add3(st_sub(img->len, remove_count), 
> insert_count, 1));

Phew, what a mouthful, and not easy to read compared to the original.
Fortunately, the remainder of the changes in this patch are
straightforward and often simple.

> diff --git a/sha1_name.c b/sha1_name.c
> @@ -87,9 +87,8 @@ static void find_short_object_filename(int len, const char 
> *hex_pfx, struct disa
> const char *objdir = get_object_directory();
> -   int objdir_len = strlen(objdir);
> -   int entlen = objdir_len + 43;
> -   fakeent = xmalloc(sizeof(*fakeent) + entlen);
> +   size_t objdir_len = strlen(objdir);
> +   fakeent = xmalloc(st_add3(sizeof(*fakeent), objdir_len, 43));
> memcpy(fakeent->base, objdir, objdir_len);
> fakeent->name = fakeent->base + objdir_len + 1;

If we've gotten this far without die()ing due to overflow in st_add3()
when invoking xmalloc(), then we know that this fakeent->name
computation won't overflow. Okay.

> fakeent->name[-1] = '/';
--
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] merge_blobs: use strbuf instead of manually-sized mmfile_t

[Jeff King]

On Tue, Feb 16, 2016 at 12:09:15AM -0500, Eric Sunshine wrote:

> > -   ecb.priv = res;
> > -   return xdi_diff(f1, f2, , , );
> > +   res->size = out.len; /* avoid long/size_t pointer mismatch below */
> 
> It took a minute or two for me to realize that "mismatch below" was
> talking about the second argument to strbuf_detach(). I tried
> rewriting the comment to mention the second argument explicitly, but
> couldn't come up with one sufficiently succinct. Oh well.

Maybe "avoid long/size_t mismatch in strbuf_detach" would be better.

> > +   res->ptr = strbuf_detach(, NULL);
> > +   return 0;
> >  }
> 
> My reviewed-by may not be worth much since this code is new to me
> too, but this patch looks "obviously correct" to me, so:
> 
> Reviewed-by: Eric Sunshine 
> 
> Perhaps squash in the following test which I adapted from the
> reproduction recipe provided by Chris Rossi[1]?
> 
> [1] https://gist.github.com/chrisrossi/f09c8bed70b364f9f12e

Yeah, maybe. There were two reasons I avoided adding a test.

One, I secretly hoped that by dragging my feet we could get consensus on
just ripping out merge-tree entirely. ;)

Two, I'm not sure what the test output _should_ be. I think this case is
buggy. So we can check that we don't corrupt the heap, which is nice,
but I don't like adding a test that doesn't test_cmp the expected output
(and see my earlier comments re: thinking hard).

But if we are going to keep it, maybe some exercise of the code is
better than none.

-Peff
--
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 13/18] sequencer: simplify memory allocation of get_message

[Eric Sunshine]

On Mon, Feb 15, 2016 at 4:56 PM, Jeff King  wrote:
> For a commit with has "1234abcd" and subject "foo", this

Did you mean s/with has/which has ID/ or something?

> function produces a struct with three strings:
>
>  1. "foo"
>
>  2. "1234abcd... foo"
>
>  3. "parent of 1234abcd... foo"
>
> It takes advantage of the fact that these strings are
> subsets of each other, and allocates only _one_ string, with
> pointers into the various parts. Unfortunately, this makes
> the string allocation complicated and hard to follow.
>
> Since we keep only one of these in memory at a time, we can
> afford to simply allocate three strings. This lets us build
> on tools like xstrfmt and avoid manual computation.
>
> While we're here, we can also drop the ad-hoc
> reimplementation of get_git_commit_encoding(), and simply
> call that function.
>
> Signed-off-by: Jeff King 
--
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 14/18] git-compat-util: drop mempcpy compat code

[Eric Sunshine]

On Mon, Feb 15, 2016 at 4:56 PM, Jeff King  wrote:
> There are no callers of this left, as the last one was
> dropped in the previous patch. And there are no likely to be

s/no/not

> new ones, as the function has been around since 2010 without
> gaining any new callers.
>
> Signed-off-by: Jeff King 
--
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: [PULL] svn pathnameencoding for git svn dcommit

[Eric Wong]

Junio, sorry about basing on next, I somehow
thought I was going to need to depend on something there.
Updated pull below if Kazutoshi can run the test effectively.

Kazutoshi Satoda  wrote:
> Thank you for the tests. But, on my environment, both of them failed
> unexpectedly. (Windows 7 SP1, x86_64 Cygwin, LANG=ja_JP.UTF-8)
> 
> For now, I don't have enough time to investigate this further. Let me
> just share the result.



> > Untracked files:
> > svnrepo/
> > "\357\202\201\357\202\202"
> > "\357\202\201\357\202\207"
> >



> I can't see how "\357\202\201\357\202\202" came as output in the test.

I wonder if it's a shell or printf portability problem.  I've
repushed the branch against master and implemented the weird
character use inside Perl scripts.

Can you give it a try?

The following changes since commit 494398473714dcbedb38b1ac79b531c7384b3bc4:

  Sixth batch for the 2.8 cycle (2016-02-10 14:24:14 -0800)

are available in the git repository at:

  git://bogomips.org/git-svn.git ks/svn-pathnameencoding

for you to fetch changes up to 7d7ea44ea5a1a38ee36402e6709e64c05650ba46:

  git-svn: apply "svn.pathnameencoding" before URL encoding (2016-02-16 
06:25:01 +)


Kazutoshi Satoda (2):
  git-svn: enable "svn.pathnameencoding" on dcommit
  git-svn: apply "svn.pathnameencoding" before URL encoding

 perl/Git/SVN/Editor.pm   |  4 +++-
 t/t9115-git-svn-dcommit-funky-renames.sh | 16 ++--
 t/t9115/inf.perl | 12 
 t/t9115/neq.perl |  5 +
 4 files changed, 34 insertions(+), 3 deletions(-)
 create mode 100644 t/t9115/inf.perl
 create mode 100644 t/t9115/neq.perl

 t/t9115-git-svn-dcommit-funky-renames.sh | 16 +++-
 t/t9115/inf.perl | 12 
 t/t9115/neq.perl |  5 +
 3 files changed, 20 insertions(+), 13 deletions(-)

Interdiff of test changes:

diff --git a/t/t9115-git-svn-dcommit-funky-renames.sh 
b/t/t9115-git-svn-dcommit-funky-renames.sh
index 9828f05..94698fe 100755
--- a/t/t9115-git-svn-dcommit-funky-renames.sh
+++ b/t/t9115-git-svn-dcommit-funky-renames.sh
@@ -84,25 +84,15 @@ test_expect_success 'git svn rebase works inside a 
fresh-cloned repository' '
test -e test-rebase
)'
 
-test_expect_success 'svn.pathnameencoding=cp932 new file on dcommit' '
-   neq=$(printf "\201\202") &&
+test_expect_success PERL 'svn.pathnameencoding=cp932 new file on dcommit' '
git config svn.pathnameencoding cp932 &&
-   echo neq >"$neq" &&
-   git add "$neq" &&
+   "$PERL_PATH" -w "$TEST_DIRECTORY"/t9115/neq.perl &&
git commit -m "neq" &&
git svn dcommit
 '
 
 test_expect_success 'svn.pathnameencoding=cp932 rename on dcommit' '
-   inf=$(printf "\201\207") &&
-   git config svn.pathnameencoding cp932 &&
-   echo inf >"$inf" &&
-   git add "$inf" &&
-   git commit -m "inf" &&
-   git svn dcommit &&
-   git mv "$inf" inf &&
-   git commit -m "inf rename" &&
-   git svn dcommit
+   "$PERL_PATH" -w "$TEST_DIRECTORY"/t9115/inf.perl
 '
 
 stop_httpd
diff --git a/t/t9115/inf.perl b/t/t9115/inf.perl
new file mode 100644
index 000..0adcfc7
--- /dev/null
+++ b/t/t9115/inf.perl
@@ -0,0 +1,12 @@
+use strict;
+my $path = "\201\207";
+open my $fh, '>', $path or die $!;
+close $fh or die $!;
+sub run { system(@_) == 0 or die join(' ', @_) . ": $?" }
+run(qw(git config svn.pathnameencoding cp932));
+run(qw(git add), $path);
+run(qw(git commit -m inf));
+run(qw(git svn dcommit));
+run(qw(git mv), $path, 'inf');
+run(qw(git commit -m inf-rename));
+run(qw(git svn dcommit));
diff --git a/t/t9115/neq.perl b/t/t9115/neq.perl
new file mode 100644
index 000..91b1061
--- /dev/null
+++ b/t/t9115/neq.perl
@@ -0,0 +1,5 @@
+use strict;
+my $path = "\201\202";
+open my $fh, '>', $path or die $!;
+close $fh or die $!;
+exec(qw(git add), $path);
--
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