Re: [PATCH v2 14/25] shallow.c: implement a generic shallow boundary finder based on rev-list

[Duy Nguyen]

On Tue, Feb 9, 2016 at 4:09 AM, Junio C Hamano  wrote:
>> + is_repository_shallow(); /* make sure shallows are read */
>> +
>> + init_revisions(, NULL);
>> + save_commit_buffer = 0;
>> + setup_revisions(ac, av, , NULL);
>> +
>> + /* Mark all reachable commits as NOT_SHALLOW */
>> + if (prepare_revision_walk())
>> + die("revision walk setup failed");
>> + traverse_commit_list(, show_commit, NULL, _shallow_flag);
>> +
>> + /*
>> +  * mark border commits SHALLOW + NOT_SHALLOW.
>> +  * We cannot clear NOT_SHALLOW right now. Imagine border
>> +  * commit A is processed first, then commit B, whose parent is
>> +  * A, later. If NOT_SHALLOW on A is cleared at step 1, B
>> +  * itself is considered border at step 2, which is incorrect.
>> +  */
>> + nr = get_max_object_index();
>> + for (i = 0; i < nr; i++) {
>
> I'd really like not to see a loop over 0..get_max_object_index().
> Are there many codepaths that peek into the in-core entire object
> store already?

You started it with check_non_tip(). At least that's how I know about
this loop. But I think that's the only code path, not counting this.

> Would it work equally well to keep track of the
> commits discovered in show_commit() to use as the set of commits
> you need to visit in this second pass?

We can't do this in show_commit. In this loop, we check
not_shallow_flag of parent commits. If one parent commit is not
show_commit'd yet, the flag is not set and we may incorrectly think
this is a border commit. The only way to avoid going through the
entire in-core object database is keeping a new commit_list and go
through it here. Which way is preferred?

>> + struct object *o = get_indexed_object(i);
>> + struct commit *c = (struct commit *)o;
>> +
>> + if (!o || o->type != OBJ_COMMIT ||
>> + !(o->flags & not_shallow_flag))
>> + continue;
>> +
>> + if (parse_commit(c))
>> + die("unable to parse commit %s",
>> + oid_to_hex(>object.oid));
>> +
>> + for (p = c->parents; p; p = p->next)
>> + if (!(p->item->object.flags & not_shallow_flag)) {
>> + o->flags |= shallow_flag;
>> + commit_list_insert(c, );
>> + break;
>> + }
>> + }
-- 
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

Good day! Dear Sir or Madam! - please help our school site - пожалуйста, помогите нашему школьному сайту - будь ласка, допоможіть нашому шкільному сайту http://bilokaminsky-nvk.pp.ua/

[admin]

Good day! Dear Sir or Madam! 
please at the top of any page of site click once on the advertising banner,
so that we could pay for hosting our school site,
Thank you

ad...@bilokaminsky-nvk.pp.ua
http://bilokaminsky-nvk.pp.ua/

добрий день,
просимо на будь-якій сторінці вгорі один раз натиснути на рекламний банер,
щоб ми змогли оплатити хостинг нашого шкільного сайту,
Дякуємо

добрый день, 
просим на любой странице вверху один раз нажать на рекламный баннер,
чтобы мы смогли оплатить хостинг нашего школьного сайта,
спасибо

Sorry if this letter has caused you inconvenience. Your address is taken from 
public sources.
To unsubscribe, please send us a a message to our email address.

ad...@bilokaminsky-nvk.pp.ua
http://bilokaminsky-nvk.pp.ua/
--
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

[Johannes Schindelin]

Hi Junio,

On Sun, 14 Feb 2016, Junio C Hamano wrote:

> Felipe Gonçalves Assis  writes:
> 
> > The usual workaround is using the resolve strategy, but apparently it
> > ignores the custom merge driver.
> 
> Hmph.
> 
> Indeed, git-merge-file seems to call xdl_merge() directly, bypassing
> the ll_merge(), which is understandable as the former predates the
> latter.  That needs to be fixed, I think.

I think this is by design. (Because I designed it.)

The original idea of git-merge-file was to serve as a drop-in replacement
for GNU/BSD merge when you want to avoid to be subject to the vagaries of
the GNU vs BSD implementations.

Ciao,
Dscho

Re: Custom merge driver with no rename detection

[Johannes Schindelin]

Hi Felipe,

On Sun, 14 Feb 2016, Felipe Gonçalves Assis wrote:

> Attached is a quick and dirty patch that emulates the effect by
> allowing greater than 100% rename thresholds to mean "no-renames".

It is really hard to comment on attached patches.

First comment: the commit message is awfully empty, and lacks a sign-off.

> /* user says num divided by scale and we say internally that
>  * is MAX_SCORE * num / scale.
>  */
> -   return (int)((num >= scale) ? MAX_SCORE : (MAX_SCORE * num / scale));
> +   return (int)(MAX_SCORE * num / scale);

Uh oh. I suspect this opens the door pretty wide for integer overflows. I
could imagine that something like

return (int)(num > scale ? MAX_SCORE + 1 : MAX_SCORE * num / scale);

would work better, but it still would need some careful consideration.

>  static int diff_scoreopt_parse(const char *opt)
> diff --git a/diffcore-rename.c b/diffcore-rename.c
> index af1fe08..7cb5a3b 100644
> --- a/diffcore-rename.c
> +++ b/diffcore-rename.c
> @@ -497,7 +497,7 @@ void diffcore_rename(struct diff_options *options)
> register_rename_src(p);
> }
> }
> -   if (rename_dst_nr == 0 || rename_src_nr == 0)
> +   if (rename_dst_nr == 0 || rename_src_nr == 0 || minimum_score > 
> MAX_SCORE)

This line is too long now.

Ciao,
Johannes

Re: [PATCH v2 21/25] fetch: define shallow boundary with --shallow-exclude

[Duy Nguyen]

On Mon, Feb 15, 2016 at 12:52:26AM -0500, Eric Sunshine wrote:
> Yes, dropping 'const' was implied. I didn't examine it too deeply, but
> it did not appear as if there would be any major fallout from dropping
> 'const'. It feels a bit cleaner to keep it all self-contained than to
> have that somewhat oddball static string_list*, but it's not such a
> big deal that I'd insist upon a rewrite.

Dropping 'const' is not a big deal. But before we do that, how about
this instead? I think the code looks better, and the compiler can
still catch invalid updates to deepen_not.

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 0402e27..07570be 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -50,6 +50,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
struct child_process *conn;
struct fetch_pack_args args;
struct sha1_array shallow = SHA1_ARRAY_INIT;
+   struct string_list deepen_not = STRING_LIST_INIT_DUP;
 
packet_trace_identity("fetch-pack");
 
@@ -108,6 +109,10 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
args.deepen_since = xstrdup(arg);
continue;
}
+   if (skip_prefix(arg, "--shallow-exclude=", )) {
+   string_list_append(_not, arg);
+   continue;
+   }
if (!strcmp("--no-progress", arg)) {
args.no_progress = 1;
continue;
@@ -135,6 +140,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
}
usage(fetch_pack_usage);
}
+   if (deepen_not.nr)
+   args.deepen_not = _not;
 
if (i < argc)
dest = argv[i++];
--
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

Issues with cc-cmd

[Viresh Kumar]

Hi Guys,

I am facing a strange issue with git send-email, with cccmd and was
looking for some help.

I am using it with Linux Kernel..

I used get-maintainers today to submit few patches for OPP framework
and that is defined as below in MAINTAINERS:

OPERATING PERFORMANCE POINTS (OPP)
M:  Viresh Kumar 
M:  Nishanth Menon 
M:  Stephen Boyd 
L:  linux...@vger.kernel.org
S:  Maintained
T:  git git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git
F:  drivers/base/power/opp/
F:  include/linux/pm_opp.h
F:  Documentation/power/opp.txt
F:  Documentation/devicetree/bindings/opp/


The parenthesis in the subsystem-name causes the cc list to look like:

Cc: linaro-ker...@lists.linaro.org,
linux...@vger.kernel.org,
Viresh Kumar ,
Krzysztof Kozlowski ,
Greg Kroah-Hartman ,
Len Brown ,
linux-ker...@vger.kernel.org (open list),
linux...@vger.kernel.org) (open list:OPERATING PERFORMANCE POINTS
(OPP),
Nishanth Menon ,
Pavel Machek ,
Stephen Boyd ,
Viresh Kumar 


Look at the second linux-pm entry here, it adds a ')' at the end of
the list's address and removes it from the end of the line.

And so that becomes an invalid address to git-send-email.

Dropping () from the subsystem name fixes it though..

The output of get_maintainers looks fine, but when its being used by
putting following into .gitconfig:

[sendemail]
cccmd = scripts/get_maintainers


it doesn't work well..

-- 
viresh
--
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 0/4] .gitignore, reinclude rules, take 2

[Nguyễn Thái Ngọc Duy]

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.

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

-- 
2.7.0.377.g4cd97dd

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

[PATCH 1/4] dir.c: fix match_pathname()

[Nguyễn Thái Ngọc Duy]

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.

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;
}
 
-- 
2.7.0.377.g4cd97dd

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

[PATCH 2/4] dir.c: support tracing exclude

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-check-ignore.txt |  1 +
 Documentation/git.txt  |  5 +
 dir.c  | 20 
 3 files changed, 26 insertions(+)

diff --git a/Documentation/git-check-ignore.txt 
b/Documentation/git-check-ignore.txt
index e94367a..f60ee05 100644
--- a/Documentation/git-check-ignore.txt
+++ b/Documentation/git-check-ignore.txt
@@ -114,6 +114,7 @@ SEE ALSO
 linkgit:gitignore[5]
 linkgit:gitconfig[5]
 linkgit:git-ls-files[1]
+GIT_TRACE_EXCLUDE in linkgit:git[1]
 
 GIT
 ---
diff --git a/Documentation/git.txt b/Documentation/git.txt
index d987ad2..2c4f0f2 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1064,6 +1064,11 @@ of clones and fetches.
cloning of shallow repositories.
See 'GIT_TRACE' for available trace output options.
 
+'GIT_TRACE_EXCLUDE'::
+   Enables trace messages that can help debugging .gitignore
+   processing. See 'GIT_TRACE' for available trace output
+   options.
+
 'GIT_LITERAL_PATHSPECS'::
Setting this variable to `1` will cause Git to treat all
pathspecs literally, rather than as glob patterns. For example,
diff --git a/dir.c b/dir.c
index bcaafac..0be7cf1 100644
--- a/dir.c
+++ b/dir.c
@@ -53,6 +53,8 @@ static enum path_treatment read_directory_recursive(struct 
dir_struct *dir,
int check_only, const struct path_simplify *simplify);
 static int get_dtype(struct dirent *de, const char *path, int len);
 
+static struct trace_key trace_exclude = TRACE_KEY_INIT(EXCLUDE);
+
 /* helper string functions with support for the ignore_case flag */
 int strcmp_icase(const char *a, const char *b)
 {
@@ -905,6 +907,8 @@ static struct exclude 
*last_exclude_matching_from_list(const char *pathname,
if (!el->nr)
return NULL;/* undefined */
 
+   trace_printf_key(_exclude, "exclude: from %s\n", el->src);
+
for (i = el->nr - 1; 0 <= i; i--) {
struct exclude *x = el->excludes[i];
const char *exclude = x->pattern;
@@ -936,6 +940,16 @@ static struct exclude 
*last_exclude_matching_from_list(const char *pathname,
break;
}
}
+
+   if (!exc) {
+   trace_printf_key(_exclude, "exclude: %.*s => n/a\n",
+pathlen, pathname);
+   return NULL;
+   }
+
+   trace_printf_key(_exclude, "exclude: %.*s vs %s at line %d => 
%s\n",
+pathlen, pathname, exc->pattern, exc->srcpos,
+exc->flags & EXC_FLAG_NEGATIVE ? "no" : "yes");
return exc;
 }
 
@@ -1683,9 +1697,13 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
struct cached_dir cdir;
enum path_treatment state, subdir_state, dir_state = path_none;
struct strbuf path = STRBUF_INIT;
+   static int level = 0;
 
strbuf_add(, base, baselen);
 
+   trace_printf_key(_exclude, "exclude: [%d] enter '%.*s'\n",
+level++, baselen, base);
+
if (open_cached_dir(, dir, untracked, , check_only))
goto out;
 
@@ -1749,6 +1767,8 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
}
close_cached_dir();
  out:
+   trace_printf_key(_exclude, "exclude: [%d] leave '%.*s'\n",
+--level, baselen, base);
strbuf_release();
 
return dir_state;
-- 
2.7.0.377.g4cd97dd

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

[PATCH 3/4] dir.c: support marking some patterns already matched

[Nguyễn Thái Ngọc Duy]

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.

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,
-exc->flags & EXC_FLAG_NEGATIVE ? "no" : "yes");
+exc->flags & EXC_FLAG_NEGATIVE ? "no" : "yes",
+exc->sticky_paths.nr ? " (stuck)" : "");
return exc;
 }
 
@@ -2005,6 +2057,25 @@ static struct untracked_cache_dir 
*validate_untracked_cache(struct dir_struct *d
return root;
 }
 
+static void clear_sticky(struct dir_struct *dir)
+{
+   struct exclude_list_group *g;
+   struct exclude_list *el;
+   struct exclude *x;
+   int i, j, k;
+
+   for (i = EXC_CMDL; i <= EXC_FILE; i++) {
+   g = >exclude_list_group[i];
+   for (j = g->nr - 1; j >= 0; j--) {
+   el = >el[j];
+   for (k = el->nr - 1; 0 <= k; k--) {
+   x = el->excludes[k];
+   string_list_clear(>sticky_paths, 0);
+   }
+   }
+   }
+}
+
 int read_directory(struct dir_struct *dir, const char *path, int 

[PATCH 4/4] dir.c: don't exclude whole dir prematurely

[Nguyễn Thái Ngọc Duy]

If there is a pattern "!foo/bar", this patch makes it not exclude
"foo" right away. This gives us a chance to examine "foo" and
re-include "foo/bar".

Helped-by: brian m. carlson 
Helped-by: Micha Wiedenmann 
Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/gitignore.txt |  17 +++-
 dir.c   | 109 +++-
 t/t3001-ls-files-others-exclude.sh  |   7 +-
 t/t3007-ls-files-other-negative.sh (new +x) | 153 
 4 files changed, 276 insertions(+), 10 deletions(-)
 create mode 100755 t/t3007-ls-files-other-negative.sh

diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
index 473623d..3ded6fd 100644
--- a/Documentation/gitignore.txt
+++ b/Documentation/gitignore.txt
@@ -82,12 +82,12 @@ PATTERN FORMAT
 
  - An optional prefix "`!`" which negates the pattern; any
matching file excluded by a previous pattern will become
-   included again. It is not possible to re-include a file if a parent
-   directory of that file is excluded. Git doesn't list excluded
-   directories for performance reasons, so any patterns on contained
-   files have no effect, no matter where they are defined.
+   included again.
Put a backslash ("`\`") in front of the first "`!`" for patterns
that begin with a literal "`!`", for example, "`\!important!.txt`".
+   It is possible to re-include a file if a parent directory of that
+   file is excluded if certain conditions are met. See section NOTES
+   for detail.
 
  - If the pattern ends with a slash, it is removed for the
purpose of the following description, but it would only find
@@ -141,6 +141,15 @@ not tracked by Git remain untracked.
 To stop tracking a file that is currently tracked, use
 'git rm --cached'.
 
+To re-include files or directories when their parent directory is
+excluded, the following conditions must be met:
+
+ - The rules to exclude a directory and re-include a subset back must
+   be in the same .gitignore file.
+
+ - The directory part in the re-include rules must be literal (i.e. no
+   wildcards)
+
 EXAMPLES
 
 
diff --git a/dir.c b/dir.c
index 8a9d8c0..552af23 100644
--- a/dir.c
+++ b/dir.c
@@ -930,6 +930,75 @@ static int match_sticky(struct exclude *exc, const char 
*pathname, int pathlen,
return 0;
 }
 
+static inline int different_decisions(const struct exclude *a,
+ const struct exclude *b)
+{
+   return (a->flags & EXC_FLAG_NEGATIVE) != (b->flags & EXC_FLAG_NEGATIVE);
+}
+
+/*
+ * Return non-zero if pathname is a directory and an ancestor of the
+ * literal path in a pattern.
+ */
+static int match_directory_part(const char *pathname, int pathlen,
+   int *dtype, struct exclude *x)
+{
+   const char  *base   = x->base;
+   int  baselen= x->baselen ? x->baselen - 1 : 0;
+   const char  *pattern= x->pattern;
+   int  prefix = x->nowildcardlen;
+   int  patternlen = x->patternlen;
+
+   if (*dtype == DT_UNKNOWN)
+   *dtype = get_dtype(NULL, pathname, pathlen);
+   if (*dtype != DT_DIR)
+   return 0;
+
+   if (*pattern == '/') {
+   pattern++;
+   patternlen--;
+   prefix--;
+   }
+
+   if (baselen) {
+   if (((pathlen < baselen && base[pathlen] == '/') ||
+pathlen == baselen) &&
+   !strncmp_icase(pathname, base, pathlen))
+   return 1;
+   pathname += baselen + 1;
+   pathlen  -= baselen + 1;
+   }
+
+
+   if (prefix &&
+   (((pathlen < prefix && pattern[pathlen] == '/') ||
+ pathlen == prefix) &&
+!strncmp_icase(pathname, pattern, pathlen)))
+   return 1;
+
+   return 0;
+}
+
+static struct exclude *should_descend(const char *pathname, int pathlen,
+ int *dtype, struct exclude_list *el,
+ struct exclude *exc)
+{
+   int i;
+
+   for (i = el->nr - 1; 0 <= i; i--) {
+   struct exclude *x = el->excludes[i];
+
+   if (x == exc)
+   break;
+
+   if (!(x->flags & EXC_FLAG_NODIR) &&
+   different_decisions(x, exc) &&
+   match_directory_part(pathname, pathlen, dtype, x))
+   return x;
+   }
+   return NULL;
+}
+
 /*
  * 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
@@ -943,7 +1012,7 @@ static struct exclude 
*last_exclude_matching_from_list(const char *pathname,
   struct exclude_list *el)
 {
struct exclude *exc = NULL; /* undecided */
-   

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

[Lars Schneider]

On 13 Feb 2016, at 19:15, Jeff King  wrote:

> On Sat, Feb 13, 2016 at 01:04:12PM -0500, Jeff King wrote:
> 
>> On Sat, Feb 13, 2016 at 12:44:49PM -0500, Jeff King wrote:
>> 
 +test_expect_success '--show-origin' '
>>> [...]
>>> I see you split this up more, but there's still quite a bit going on in
>>> this one block. IMHO, it would be more customary in our tests to put the
>>> setup into one test_expect_success block, then each of these
>>> expect-run-cmp blocks into their own test_expect_success.
>> 
>> Here's a squashable patch that shows what I mean.
> 
> And here are a few comments on the changes...
> 
>> -test_expect_success '--show-origin' '
>> ->.git/config &&
>> ->"$HOME"/.gitconfig &&
>> +test_expect_success 'set up --show-origin tests' '
>>  INCLUDE_DIR="$HOME/include" &&
>>  mkdir -p "$INCLUDE_DIR" &&
>> -cat >"$INCLUDE_DIR"/absolute.include <<-EOF &&
>> +cat >"$INCLUDE_DIR"/absolute.include <<-\EOF &&
>>  [user]
>>  absolute = include
>>  EOF
>> -cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
>> +cat >"$INCLUDE_DIR"/relative.include <<-\EOF &&
>>  [user]
>>  relative = include
>>  EOF
>> -test_config_global user.global "true" &&
>> -test_config_global user.override "global" &&
>> -test_config_global include.path "$INCLUDE_DIR"/absolute.include &&
>> -test_config include.path ../include/relative.include &&
>> -test_config user.local "true" &&
>> -test_config user.override "local" &&
>> +cat >"$HOME"/.gitconfig <<-EOF &&
>> +[user]
>> +global = true
>> +override = global
>> +[include]
>> +path = "$INCLUDE_DIR/absolute.include"
>> +EOF
>> +cat >.git/config <<-\EOF
>> +[include]
>> +path = ../include/relative.include
>> +[user]
>> +local = true
>> +override = local
>> +EOF
> 
> I preserved your ordering here (as the later "--list" tests care). But
> it might be worth ordering both files the same way, so that a reader
> does not wonder if it is significant (and just update the --list
> output expectation later).
OK, fixed!


> 
>> @@ -1253,25 +1263,32 @@ test_expect_success '--show-origin' '
>>  localQcmdline:Quser.cmdline
>>  trueQ
>>  EOF
>> -git -c user.cmdline=true config --null --list --show-origin | nul_to_q 
>> >output &&
>> +git -c user.cmdline=true config --null --list --show-origin >output.raw 
>> &&
>> +nul_to_q output &&
> 
> We usually try to avoid putting git on the left-hand side of a pipe,
> because it hides the exit code, and we want to make sure git does not
> fail. I won't be surprised if you copied the style from elsewhere in the
> script, though, as this is an old one and we were not always consistent.
Make sense, fixed!


> 
>>  echo >>output &&
>> -test_cmp expect output &&
>> +test_cmp expect output
> 
> This "echo" might be worth a comment. I think we are just adding the
> extra newline that the here-doc insists on, but that --null output would
> not include.
Done.

> 
> Overall, I find the "--show-origin --null" output pretty weird to read.
> We use a newline to split the config key/value, a NUL between config
> entries, but now also a NUL between the filename and the rest of the
> config entry.
> 
> That makes parsing pretty weird, as you cannot just use something like
> 
>  git config --show-origin --list --null | perl -0ne ...
> 
> to process entries; every other entry you get will be a filename. But at
> the same time, we do not go all out and say "there is a NUL between each
> field", because the key/value separator is a newline in this case, and
> the reader has to parse that separately after splitting on NULs.
> 
> But I think it's too late to do anything about it now. The weirdness is
> really the mixed NUL/newline thing, and you are not introducing that.
> 
>> -CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
>> -cat >"$CUSTOM_CONFIG_FILE" <<-\EOF &&
>> -[user]
>> -custom = true
>> -EOF
>> +test_expect_success 'set up custom config file' '
>> +CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
>> +cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
>> +[user]
>> +custom = true
>> +EOF
>> +'
> 
> Everything, even mundane setup, should generally go in a test_expect
> block. That means we'll notice unexpected failures, and any output will
> follow the usual "--verbose" rules.
> 
> Arguably this setup could just go into the initial setup block.
> 
> Also, you may not that the filename does _not_ actually have tabs in it,
> because the shell does not expand "\t". It does have backslashes in it,
> though, which is enough to trigger our C-style quoting.
Oh, thanks for the explanation. I was already wondering about the double
backslash earlier...

> 
> So the 

Re: git worktree fails to recreate existing branch even with -B

[Duy Nguyen]

On Tue, Feb 09, 2016 at 05:54:01PM +0300, Kirill Likhodedov wrote:
> Git doesn’t allow me to execute 
> git worktree add -B   
> where  already exists in the repository.
> 
> The command prints the following:
> Preparing  (identifier )
> fatal: Refusing to point HEAD outside of refs/
> 
> I’m trying to create a worktree on an existing branch ,
> which should point to . This obviously should fail with
> “-b”, but I use “-B” and expect it to be reset to  as
> mentioned in the docs:
> 
> By default, -b refuses to create a new branch if it already exists.  
> -B overrides this safeguard, resetting  to .
> 
> Do I miss something or there is a bug?

According to the man page, this looks like a bug.

> Steps to reproduce:
> 
> git init wt
> cd wt
> echo 'asd' > a.txt ; git add a.txt ; git commit -m initial
> git branch br1
> git worktree add -B br1 ~/temp/br1 master
> 
> Error message:
> Preparing /Users/loki/temp/br1 (identifier br1)
> fatal: Refusing to point HEAD outside of refs/

GIT_TRACE=2 gives me

trace: built-in: git 'symbolic-ref' 'HEAD' ''
fatal: Refusing to point HEAD outside of refs/

So we pass wrong argument to symbolic-ref. The '' should be
'refs/heads/br1'. This patch seems to fix it.

-- 8< --
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 475b958..d5b319f 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -202,7 +202,7 @@ static int add_worktree(const char *path, const char 
*refname,
 
/* is 'refname' a branch or commit? */
if (opts->force_new_branch) /* definitely a branch */
-   ;
+   strbuf_addf(, "refs/heads/%s", refname);
else if (!opts->detach && !strbuf_check_branch_ref(, refname) &&
 ref_exists(symref.buf)) { /* it's a branch */
if (!opts->force)
-- 8< --
--
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 v4 20/21] refs: add LMDB refs storage backend

[Duy Nguyen]

On Sun, Feb 14, 2016 at 7:04 PM, Duy Nguyen  wrote:
> On Sat, Feb 6, 2016 at 2:44 AM, David Turner  wrote:
>> +static char *get_refdb_path(const char *base)
>> +{
>> +   static struct strbuf path_buf = STRBUF_INIT;
>> +   strbuf_reset(_buf);
>> +   strbuf_addf(_buf, "%s/refdb", base);
>> +   return path_buf.buf;
>> +}
> ...
>> +static int lmdb_init_db(struct strbuf *err, int shared)
>> +{
>> +   /*
>> +* To create a db, all we need to do is make a directory for
>> +* it to live in; lmdb will do the rest.
>> +*/
>> +
>> +   if (!db_path)
>> +   db_path = 
>> xstrdup(real_path(get_refdb_path(get_git_common_dir(;
>
> This works for multiple worktrees. But scripts may have harder time
> getting the path. The recommended way is "git rev-parse --git-path
> refdb" but because "refdb" is not registered in path.c:common_list[],
> that command becomes git_path("refdb") instead of
> get_refdb(get_git_... like here. And I will need to know that
> .git/refdb is _not_ per-worktree when I migrate/convert main worktree
> (it's very likely I have to go that route to solve .git/config issue
> in multi worktree).
>
> The solution is register refdb to common_list[] and you can do
> git_path("refdb") here. But then what happens when another backend is
> added? Will the new backend use the same path "refdb", or say
> "refdb.sqlite"? If all backends share the name "refdb", why can't we
> just reuse "refs" instead because the default filesystem-based backend
> is technically just another backend?

To answer myself: I forgot that there were per-worktree refs (e.g.
refs/bisect). It makes me wonder if we should put per-worktree refs to
lmdb as well (maybe one per worktree if we don't want to put all in
one db). One of the advantages of moving away from fs-based backend is
the ability to deal with case sensitivity, "nested" refs (e.g. a/b and
a/b/c are both refs). With this split, I think some refs are still
left behind.. Sorry if this was discussed before, I haven't followed
this closely.
-- 
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: Custom merge driver with no rename detection

[Junio C Hamano]

Johannes Schindelin  writes:

> On Sun, 14 Feb 2016, Junio C Hamano wrote:
>
>> Felipe Gonçalves Assis  writes:
>> 
>> > The usual workaround is using the resolve strategy, but apparently it
>> > ignores the custom merge driver.
>> 
>> Hmph.
>> 
>> Indeed, git-merge-file seems to call xdl_merge() directly, bypassing
>> the ll_merge(), which is understandable as the former predates the
>> latter.  That needs to be fixed, I think.
>
> I think this is by design. (Because I designed it.)
>
> The original idea of git-merge-file was to serve as a drop-in replacement
> for GNU/BSD merge when you want to avoid to be subject to the vagaries of
> the GNU vs BSD implementations.

We did use to use "merge" from the RCS suite originally, and we did
want to use our own, but the primary reason we added our own was so
that it can be enhanced in sync with the remainder of Git in a
consistent way.

If the rest of Git can be told via the attribute system to make use
of a three-way merge driver, it should have learnt the trick to be
kept up with the rest of the system.  I see the current state of the
program merely be staying a bit behind; I do not think it was part
of the grand design to forbidd it forever from learning new optional
tricks.
--
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

stdout vs stderr

[Florian Lohoff]

Hi,
is there a reason why those messages for pull are sent to stdout and
for push are sent to stderr?

flo@p3:~$ git pull
Already up-to-date.
flo@p3:~$ git pull >/dev/null

flo@p3:~$ git push
Everything up-to-date
flo@p3:~$ git push >/dev/null
Everything up-to-date

I am regularly trying to build automatisms around git and typically i
redirect stdout to /dev/null in the hope that when something goes
wrong i see error messages on stderr. This does not work for e.g. push
for obvious reason.

Flo
-- 
Florian Lohoff f...@zz.de
  We need to self-defend - GnuPG/PGP enable your email today!


signature.asc
Description: Digital signature

Re: git svn dcommit doesn't support --username option for file:/// urls

[Eric Wong]

Tim Ringenbach  wrote:
> Hi,
> 
> 'git svn dcommit' doesn't seem to honor the --username argument when
> my svn repository url is a file:/// url.  It doesn't complain either,
> it just seems to silently ignore the option. My dcommits show up as
> the user I'm logged in as. The only way I found to change that is to
> 'sudo' to some other user.
> 
> The actual 'svn' command does support --username with 'svn commit'.

Interesting, I didn't know --username would be handled with
file:// at all by svn(1).  I don't think we do anything special
depending on the URL scheme for auth, either.

I took a quick look at the svn(1) code
(subversion/svn/svn.c in git://git.apache.org/subversion.git)
but didn't see anything jump out at me (I'm not really familiar
with that code, either).

Totally untested, but does flipping the order of auth providers
help at all?

diff --git a/perl/Git/SVN/Ra.pm b/perl/Git/SVN/Ra.pm
index e764696..c6ce247 100644
--- a/perl/Git/SVN/Ra.pm
+++ b/perl/Git/SVN/Ra.pm
@@ -43,6 +43,7 @@ END {
 sub _auth_providers () {
require SVN::Client;
my @rv = (
+ SVN::Client::get_username_provider(),
  SVN::Client::get_simple_provider(),
  SVN::Client::get_ssl_server_trust_file_provider(),
  SVN::Client::get_simple_prompt_provider(
@@ -53,7 +54,6 @@ sub _auth_providers () {
  SVN::Client::get_ssl_client_cert_pw_file_provider(),
  SVN::Client::get_ssl_client_cert_pw_prompt_provider(
\::SVN::Prompt::ssl_client_cert_pw, 2),
- SVN::Client::get_username_provider(),
  SVN::Client::get_ssl_server_trust_prompt_provider(
\::SVN::Prompt::ssl_server_trust),
  SVN::Client::get_username_prompt_provider(


(I'm not sure if it breaks other things, either).
--
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: Issues with cc-cmd

[Junio C Hamano]

Viresh Kumar  writes:

> linux...@vger.kernel.org) (open list:OPERATING PERFORMANCE POINTS
> (OPP),

Hmm

http://lists.kernelnewbies.org/pipermail/kernelnewbies/2014-July/011343.html

comes to mind.  It tells you not to blindly automate this process.

I do not offhand know if it was even designed to be directly used
(and usable) as a cccmd, but anyway, I see this mention in its
"Notes:" section:

  Using "--roles" or "--rolestats" with git send-email --cc-cmd or any
  other automated tools that expect only ["name"] 
  may not work because of additional output after .

I suspect that what you are showing is the "additional output after
address" the above talks about.

In any case, this command is maintained as part of the kernel, no?
I see Joe Perches signed at the beginning of file in 2007; I do not
know if he still maintains this script, or somebody else is
primarily enhancing it these days (ehh, I probably could use the
script on itself, but I am way too lazy late at night), but asking
him would be a good starting point, instead of asking here.

--
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: Issues with cc-cmd

[Viresh Kumar]

On 15-02-16, 02:06, Junio C Hamano wrote:
> Viresh Kumar  writes:
> 
> > linux...@vger.kernel.org) (open list:OPERATING PERFORMANCE POINTS
> > (OPP),
> 
> Hmm
> 
> http://lists.kernelnewbies.org/pipermail/kernelnewbies/2014-July/011343.html
> 
> comes to mind.  It tells you not to blindly automate this process.
> 
> I do not offhand know if it was even designed to be directly used
> (and usable) as a cccmd, but anyway, I see this mention in its
> "Notes:" section:
> 
>   Using "--roles" or "--rolestats" with git send-email --cc-cmd or any
>   other automated tools that expect only ["name"] 
>   may not work because of additional output after .
> 
> I suspect that what you are showing is the "additional output after
> address" the above talks about.
> 
> In any case, this command is maintained as part of the kernel, no?
> I see Joe Perches signed at the beginning of file in 2007; I do not
> know if he still maintains this script, or somebody else is
> primarily enhancing it these days (ehh, I probably could use the
> script on itself, but I am way too lazy late at night), but asking
> him would be a good starting point, instead of asking here.

Yeah, he told me this sometime back and that solved the issues I was
facing, and in fact it looks far better now. Thanks Junio.

git send-email requires a simple list of names and addresses
without decorations like roles, commit stats or section names.

You are supposed to use something like:

tocmd = "scripts/get_maintainer.pl --nogit --nogit-fallback --norolestats 
--pattern-depth=1 --nol"
cccmd = "scripts/get_maintainer.pl --nogit --nogit-fallback --norolestats --nom 
--nor"

-- 
viresh
--
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 v4 0/3] config: add '--sources' option to print the source of a config value

[larsxschneider]

From: Lars Schneider 

diff to v3:

* pass type as parameter to "git_config_from_mem" (renamed from
  "git_config_from_buf") and "do_config_from_file"
* split current_config_type_name into two functions
* explain usage of fwrite with a comment
* use tabs instead of spaces to fix indentation error
* squash Peff's commit to split the test cases
* sort all test configs key/value pairs in the same way
* add comment to explain newline introduce by here-doc
* use real tabs for test file name
* add a test teardown
* fix hiding of git exit code t1300 and t7008
* drop "git-config.txt: describe '--includes' default behavior" patch

I like the idea of a "test set up block" within a test script. In order
to clean up nicely before any subsequent tests I would like to propose
a "tear down" block. Would that work as a compromise in our "test cases
depend on earlier test cases" discussion?

In "t: do not hide Git's exit code in tests" I also fixed a few more
places where Git's exit code was hidden. Please drop this patch if you
think that this should not be part of this series.

Thanks a lot for the reviews and explainations,
Lars

Lars Schneider (3):
  t: do not hide Git's exit code in tests
  config: add 'type' to config_source struct that identifies config type
  config: add '--show-origin' option to print the origin of a config
value

 Documentation/git-config.txt |  15 ++--
 builtin/config.c |  33 +
 cache.h  |   6 +-
 config.c |  36 +++---
 submodule-config.c   |   4 +-
 t/t1300-repo-config.sh   | 167 ++-
 t/t1308-config-set.sh|   4 +-
 t/t7008-grep-binary.sh   |   3 +-
 8 files changed, 242 insertions(+), 26 deletions(-)

--
2.5.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

[PATCH v4 1/3] t: do not hide Git's exit code in tests

[larsxschneider]

From: Lars Schneider 

Git should not be on the left-hand side of a pipe, because it hides the exit
code, and we want to make sure git does not fail.

There is one more occurrence of this pattern in t9010-svn-fe.sh which is to
evolved to change it easily.

Helped-by: Jeff King 
Signed-off-by: Lars Schneider 
---
 t/t1300-repo-config.sh | 6 --
 t/t7008-grep-binary.sh | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 52678e7..1782add 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -957,13 +957,15 @@ Qsection.sub=section.val4
 Qsection.sub=section.val5Q
 EOF
 test_expect_success '--null --list' '
-   git config --null --list | nul_to_q >result &&
+   git config --null --list >result.raw &&
+   nul_to_q result &&
echo >>result &&
test_cmp expect result
 '
 
 test_expect_success '--null --get-regexp' '
-   git config --null --get-regexp "val[0-9]" | nul_to_q >result &&
+   git config --null --get-regexp "val[0-9]" >result.raw &&
+   nul_to_q result &&
echo >>result &&
test_cmp expect result
 '
diff --git a/t/t7008-grep-binary.sh b/t/t7008-grep-binary.sh
index b146406..9c9c378 100755
--- a/t/t7008-grep-binary.sh
+++ b/t/t7008-grep-binary.sh
@@ -141,7 +141,8 @@ test_expect_success 'grep respects not-binary diff 
attribute' '
test_cmp expect actual &&
echo "b diff" >.gitattributes &&
echo "b:binQary" >expect &&
-   git grep bin b | nul_to_q >actual &&
+   git grep bin b >actual.raw &&
+   nul_to_q actual &&
test_cmp expect actual
 '
 
-- 
2.5.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

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

[larsxschneider]

From: Lars Schneider 

If config values are queried using 'git config' (e.g. via --get,
--get-all, --get-regexp, or --list flag) then it is sometimes hard to
find the configuration file where the values were defined.

Teach 'git config' the '--show-origin' option to print the source
configuration file for every printed value.

Based-on-patch-by: Jeff King 
Signed-off-by: Lars Schneider 
---
 Documentation/git-config.txt |  15 +++--
 builtin/config.c |  33 ++
 t/t1300-repo-config.sh   | 153 +++
 3 files changed, 196 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 2608ca7..6374997 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,18 +9,18 @@ git-config - Get and set repository or global options
 SYNOPSIS
 
 [verse]
-'git config' [] [type] [-z|--null] name [value [value_regex]]
+'git config' [] [type] [--show-origin] [-z|--null] name [value 
[value_regex]]
 'git config' [] [type] --add name value
 'git config' [] [type] --replace-all name value [value_regex]
-'git config' [] [type] [-z|--null] --get name [value_regex]
-'git config' [] [type] [-z|--null] --get-all name [value_regex]
-'git config' [] [type] [-z|--null] [--name-only] --get-regexp 
name_regex [value_regex]
+'git config' [] [type] [--show-origin] [-z|--null] --get name 
[value_regex]
+'git config' [] [type] [--show-origin] [-z|--null] --get-all name 
[value_regex]
+'git config' [] [type] [--show-origin] [-z|--null] [--name-only] 
--get-regexp name_regex [value_regex]
 'git config' [] [type] [-z|--null] --get-urlmatch name URL
 'git config' [] --unset name [value_regex]
 'git config' [] --unset-all name [value_regex]
 'git config' [] --rename-section old_name new_name
 'git config' [] --remove-section name
-'git config' [] [-z|--null] [--name-only] -l | --list
+'git config' [] [--show-origin] [-z|--null] [--name-only] -l | 
--list
 'git config' [] --get-color name [default]
 'git config' [] --get-colorbool name [stdout-is-tty]
 'git config' [] -e | --edit
@@ -194,6 +194,11 @@ See also <>.
Output only the names of config variables for `--list` or
`--get-regexp`.
 
+--show-origin::
+   Augment the output of all queried config options with the
+   origin type (file, stdin, blob, cmdline) and the actual origin
+   (config file path, ref, or blob id if applicable).
+
 --get-colorbool name [stdout-is-tty]::
 
Find the color setting for `name` (e.g. `color.diff`) and output
diff --git a/builtin/config.c b/builtin/config.c
index adc7727..7bad0c0 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -3,6 +3,7 @@
 #include "color.h"
 #include "parse-options.h"
 #include "urlmatch.h"
+#include "quote.h"
 
 static const char *const builtin_config_usage[] = {
N_("git config []"),
@@ -27,6 +28,7 @@ static int actions, types;
 static const char *get_color_slot, *get_colorbool_slot;
 static int end_null;
 static int respect_includes = -1;
+static int show_origin;
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)
@@ -81,6 +83,7 @@ static struct option builtin_config_options[] = {
OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")),
OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
OPT_BOOL(0, "includes", _includes, N_("respect include 
directives on lookup")),
+   OPT_BOOL(0, "show-origin", _origin, N_("show origin of config 
(file, stdin, blob, cmdline)")),
OPT_END(),
 };
 
@@ -91,8 +94,28 @@ static void check_argc(int argc, int min, int max) {
usage_with_options(builtin_config_usage, builtin_config_options);
 }
 
+static void show_config_origin(struct strbuf *buf)
+{
+   const char term = end_null ? '\0' : '\t';
+
+   strbuf_addstr(buf, current_config_type());
+   strbuf_addch(buf, ':');
+   if (end_null)
+   strbuf_addstr(buf, current_config_name());
+   else
+   quote_c_style(current_config_name(), buf, NULL, 0);
+   strbuf_addch(buf, term);
+}
+
 static int show_all_config(const char *key_, const char *value_, void *cb)
 {
+   if (show_origin) {
+   struct strbuf buf = STRBUF_INIT;
+   show_config_origin();
+   /* Use fwrite as "buf" can contain \0's if "end_null" is set. */
+   fwrite(buf.buf, 1, buf.len, stdout);
+   strbuf_release();
+   }
if (!omit_values && value_)
printf("%s%c%s%c", key_, delim, value_, term);
else
@@ -108,6 +131,8 @@ struct strbuf_list {
 
 static int format_config(struct strbuf *buf, const char *key_, const char 
*value_)
 {
+   if (show_origin)
+   show_config_origin(buf);
if (show_keys)
strbuf_addstr(buf, key_);
if (!omit_values) {
@@ -538,6 +563,14 @@ int cmd_config(int argc, 

[PATCH v4 2/3] config: add 'type' to config_source struct that identifies config type

[larsxschneider]

From: Lars Schneider 

Use the config type to print more detailed error messages that inform
the user about the origin of a config error (file, stdin, blob).

Signed-off-by: Lars Schneider 
---
 cache.h|  6 --
 config.c   | 36 +---
 submodule-config.c |  4 ++--
 t/t1300-repo-config.sh |  8 +++-
 t/t1308-config-set.sh  |  4 ++--
 5 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/cache.h b/cache.h
index c63fcc1..8d86e5c 100644
--- a/cache.h
+++ b/cache.h
@@ -1485,8 +1485,8 @@ struct git_config_source {
 typedef int (*config_fn_t)(const char *, const char *, void *);
 extern int git_default_config(const char *, const char *, void *);
 extern int git_config_from_file(config_fn_t fn, const char *, void *);
-extern int git_config_from_buf(config_fn_t fn, const char *name,
-  const char *buf, size_t len, void *data);
+extern int git_config_from_mem(config_fn_t fn, const char *type,
+   const char *name, const char *buf, 
size_t len, void *data);
 extern void git_config_push_parameter(const char *text);
 extern int git_config_from_parameters(config_fn_t fn, void *data);
 extern void git_config(config_fn_t fn, void *);
@@ -1525,6 +1525,8 @@ extern const char *get_log_output_encoding(void);
 extern const char *get_commit_output_encoding(void);
 
 extern int git_config_parse_parameter(const char *, config_fn_t fn, void 
*data);
+extern const char *current_config_type();
+extern const char *current_config_name();
 
 struct config_include_data {
int depth;
diff --git a/config.c b/config.c
index 86a5eb2..5cf11b5 100644
--- a/config.c
+++ b/config.c
@@ -24,6 +24,7 @@ struct config_source {
size_t pos;
} buf;
} u;
+   const char *type;
const char *name;
const char *path;
int die_on_error;
@@ -471,9 +472,9 @@ static int git_parse_source(config_fn_t fn, void *data)
break;
}
if (cf->die_on_error)
-   die(_("bad config file line %d in %s"), cf->linenr, cf->name);
+   die(_("bad config line %d in %s %s"), cf->linenr, cf->type, 
cf->name);
else
-   return error(_("bad config file line %d in %s"), cf->linenr, 
cf->name);
+   return error(_("bad config line %d in %s %s"), cf->linenr, 
cf->type, cf->name);
 }
 
 static int parse_unit_factor(const char *end, uintmax_t *val)
@@ -588,9 +589,9 @@ static void die_bad_number(const char *name, const char 
*value)
if (!value)
value = "";
 
-   if (cf && cf->name)
-   die(_("bad numeric config value '%s' for '%s' in %s: %s"),
-   value, name, cf->name, reason);
+   if (cf && cf->type && cf->name)
+   die(_("bad numeric config value '%s' for '%s' in %s %s: %s"),
+   value, name, cf->type, cf->name, reason);
die(_("bad numeric config value '%s' for '%s': %s"), value, name, 
reason);
 }
 
@@ -1061,11 +1062,13 @@ static int do_config_from(struct config_source *top, 
config_fn_t fn, void *data)
 }
 
 static int do_config_from_file(config_fn_t fn,
-   const char *name, const char *path, FILE *f, void *data)
+   const char *type, const char *name, const char *path, FILE *f,
+   void *data)
 {
struct config_source top;
 
top.u.file = f;
+   top.type = type;
top.name = name;
top.path = path;
top.die_on_error = 1;
@@ -1078,7 +1081,7 @@ static int do_config_from_file(config_fn_t fn,
 
 static int git_config_from_stdin(config_fn_t fn, void *data)
 {
-   return do_config_from_file(fn, "", NULL, stdin, data);
+   return do_config_from_file(fn, "stdin", "", NULL, stdin, data);
 }
 
 int git_config_from_file(config_fn_t fn, const char *filename, void *data)
@@ -1089,21 +1092,22 @@ int git_config_from_file(config_fn_t fn, const char 
*filename, void *data)
f = fopen(filename, "r");
if (f) {
flockfile(f);
-   ret = do_config_from_file(fn, filename, filename, f, data);
+   ret = do_config_from_file(fn, "file", filename, filename, f, 
data);
funlockfile(f);
fclose(f);
}
return ret;
 }
 
-int git_config_from_buf(config_fn_t fn, const char *name, const char *buf,
-   size_t len, void *data)
+int git_config_from_mem(config_fn_t fn, const char *type, const char *name,
+   const char *buf, size_t len, void *data)
 {
struct config_source top;
 
top.u.buf.buf = buf;
top.u.buf.len = len;
top.u.buf.pos = 0;
+   top.type = type;
top.name = name;
top.path = NULL;
top.die_on_error = 0;
@@ -1132,7 +1136,7 @@ static int git_config_from_blob_sha1(config_fn_t fn,
 

Re: Custom merge driver with no rename detection

[Felipe Gonçalves Assis]

On 15 February 2016 at 06:06, Johannes Schindelin
 wrote:
> Hi Felipe,
>
> On Sun, 14 Feb 2016, Felipe Gonçalves Assis wrote:
>
>> Attached is a quick and dirty patch that emulates the effect by
>> allowing greater than 100% rename thresholds to mean "no-renames".
>
> It is really hard to comment on attached patches.
>
> First comment: the commit message is awfully empty, and lacks a sign-off.
>

Thanks. I am sorry for not submitting the patch separately in an
email, but, it was really not meant for serious review, just for
emulating the desired effect with minimal effort.

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.

Regards,
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

Re: Custom merge driver with no rename detection

[Junio C Hamano]

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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] worktree: fix "add -B"

[Nguyễn Thái Ngọc Duy]

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 */
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 )
-- 
2.7.0.377.g4cd97dd

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

[PATCH 2/2] worktree add -B: do the checkout test before update branch

[Nguyễn Thái Ngọc Duy]

If --force is not given but -B is, we should not proceed if the given
branch is already checked out elsewhere. add_worktree() has this test,
but it kicks in too late when "git branch --force" is already
executed. As a result, even though we correctly refuse to create a new
worktree, we have already updated the branch and mess up the other
checkout.

Repeat the die_if_checked_out() test again for this specific case before
"git branch" runs.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Something extra while I was studying this code. I'm not so sure if
 this is the right way.
 
 Another option is do it in "git branch" which rejects with "Cannot
 force update the current branch", but only for current worktree.

 builtin/worktree.c  | 11 ++-
 t/t2025-worktree-add.sh |  7 +++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 6b9c946..20cf67a 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -334,9 +334,18 @@ static int add(int ac, const char **av, const char *prefix)
branch = ac < 2 ? "HEAD" : av[1];
 
opts.force_new_branch = !!new_branch_force;
-   if (opts.force_new_branch)
+   if (opts.force_new_branch) {
+   struct strbuf symref = STRBUF_INIT;
+
opts.new_branch = new_branch_force;
 
+   if (!opts.force &&
+   !strbuf_check_branch_ref(, opts.new_branch) &&
+   ref_exists(symref.buf))
+   die_if_checked_out(symref.buf);
+   strbuf_release();
+   }
+
if (ac < 2 && !opts.new_branch && !opts.detach) {
int n;
const char *s = worktree_basename(path, );
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index a4d36c0..cbfa41e 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -193,6 +193,13 @@ test_expect_success '"add" -B/--detach mutually exclusive' 
'
test_must_fail git worktree add -B poodle --detach bamboo master
 '
 
+test_expect_success '"add -B" fails if the branch is checked out' '
+   git rev-parse newmaster >before &&
+   test_must_fail git worktree add -B newmaster bamboo master &&
+   git rev-parse newmaster >after &&
+   test_cmp before after
+'
+
 test_expect_success 'add -B' '
git worktree add -B poodle bamboo2 master^ &&
git -C bamboo2 symbolic-ref HEAD >actual &&
-- 
2.7.0.377.g4cd97dd

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

[Christoph Egger]

Jeff King  writes:
> We can't do this perfectly, because older versions of git do not yet
> know about the option, and will therefore just silently ignore it. And
> for consistency there, we usually do the same for features that we know
> about but are unsupported.

Jep that's why I originally did it this way. But if I (the user) just
have to check the git version to know I'm fine (and not also check which
version of curl it is linked with) to be sure I'd assume that's an
improvement still.

> But I agree for something with security implications like this, we are
> better off warning when we know support is not built in. That's not
> perfect, but it's better than nothing.

I'll add an updated patch taking this into account

> I wonder if there are other options which should get the same treatment.
> Most of the obvious ones I could think of (e.g., http.sslverify) do not
> need it, because either they always have support built, or they
> fail-closed, or both.

does CURLOPT_CAPATH add to CURLOPT_CAINFO or replace it? The
documentation [0] is inconclusive to me in this regard.

  Christoph

[0] https://curl.haxx.se/libcurl/c/CURLOPT_CAPATH.html


signature.asc
Description: PGP signature

[PATCH +warn] Implement https public key pinning

[Christoph Egger]

Add the http.pinnedpubkey configuration option for public key
pinning. It allows any string supported by libcurl --
base64(sha256(pubkey)) or filename of the full public key.

If cURL does not support pinning (is too old) output a warning to the
user.

Signed-off-by: Christoph Egger 
---

 This version of the patch adds a warning to the user if the option is
 not supported.

 Documentation/config.txt |  8 
 http.c   | 14 ++
 2 files changed, 22 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 27f02be..0f2643b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1727,6 +1727,14 @@ http.sslCAPath::
with when fetching or pushing over HTTPS. Can be overridden
by the 'GIT_SSL_CAPATH' environment variable.
 
+http.pinnedpubkey::
+   Public key of the https service. It may either be the filename of
+   a PEM or DER encoded public key file or a string starting with
+   'sha256//' followed by the base64 encoded sha256 hash of the
+   public key. See also libcurl 'CURLOPT_PINNEDPUBLICKEY'. git will
+   exit with an error if this option is set but not supported by
+   cURL.
+
 http.sslTry::
Attempt to use AUTH SSL/TLS and encrypted data transfers
when connecting via regular FTP protocol. This might be needed
diff --git a/http.c b/http.c
index dfc53c1..0bb9237 100644
--- a/http.c
+++ b/http.c
@@ -57,6 +57,9 @@ static const char *ssl_key;
 #if LIBCURL_VERSION_NUM >= 0x070908
 static const char *ssl_capath;
 #endif
+#if LIBCURL_VERSION_NUM >= 0x072c00
+static const char *ssl_pinnedkey;
+#endif
 static const char *ssl_cainfo;
 static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
@@ -239,6 +242,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
if (!strcmp("http.sslcainfo", var))
return git_config_pathname(_cainfo, var, value);
if (!strcmp("http.sslcertpasswordprotected", var)) {
@@ -499,6 +509,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);
 
-- 
2.7.0


-- 
--
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] Disown ssh+git and git+ssh

[Carlos Martín Nieto]

These were silly from the beginning, but we have to support them for
compatibility. That doesn't mean we have to show them in the
documentation. These were already left out of the main list, but a
reference in the main manpage was left, so remove that.

Also add a note to discourage their use if anybody goes looking for them
in the source code.

Signed-off-by: Carlos Martín Nieto 
---

I've updated the wording, so we talk about different ways of spelling
ssh rather than talking about schemes.

 Documentation/git.txt | 2 +-
 connect.c | 4 
 transport.c   | 4 
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index d987ad2..2f90635 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1122,7 +1122,7 @@ of clones and fetches.
connection (or proxy, if configured)
 
  - `ssh`: git over ssh (including `host:path` syntax,
-   `git+ssh://`, etc).
+   `ssh://`, etc).
 
  - `rsync`: git over rsync
 
diff --git a/connect.c b/connect.c
index fd7ffe1..d3eaa0e 100644
--- a/connect.c
+++ b/connect.c
@@ -267,6 +267,10 @@ static enum protocol get_protocol(const char *name)
return PROTO_SSH;
if (!strcmp(name, "git"))
return PROTO_GIT;
+   /*
+* These ways to spell the ssh transport remain supported for
+* compat but are undocumented and their use is discouraged
+*/
if (!strcmp(name, "git+ssh"))
return PROTO_SSH;
if (!strcmp(name, "ssh+git"))
diff --git a/transport.c b/transport.c
index 9ae7184..ed61e72 100644
--- a/transport.c
+++ b/transport.c
@@ -1002,6 +1002,10 @@ struct transport *transport_get(struct remote *remote, 
const char *url)
|| starts_with(url, "file://")
|| starts_with(url, "git://")
|| starts_with(url, "ssh://")
+   /*
+* These ways to spell the ssh transport remain supported for
+* compat but are undocumented and their use is discouraged
+*/
|| starts_with(url, "git+ssh://")
|| starts_with(url, "ssh+git://")) {
/*
-- 
2.7.0

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

颞下颌关节炎

[颞下颌关节炎]

你的老朋友邀你来Q群:343257759

How can I get a list of checkout history?

[Robert Dailey]

As you know, I can checkout the Nth checked out branch via this syntax:

$ git checkout @{-N}

Is there a built-in mechanism to get a listing of previously checked
out refs? Basically, this would be similar to 'history' command in
linux where instead of actual commands, it lists like this:

HEAD@{-1}: master
HEAD@{-2}: topic1
HEAD@{-3}: 3f556e9 (detached)

Seems like reflog should be able to do this, and maybe it can, but I'm
not sure. Any tips? I'd be fine making a convenient alias for this if
it ends up being a series of piped commands.

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

Re: How can I get a list of checkout history?

[Jeff King]

On Mon, Feb 15, 2016 at 09:18:50AM -0600, Robert Dailey wrote:

> As you know, I can checkout the Nth checked out branch via this syntax:
> 
> $ git checkout @{-N}
> 
> Is there a built-in mechanism to get a listing of previously checked
> out refs? Basically, this would be similar to 'history' command in
> linux where instead of actual commands, it lists like this:
> 
> HEAD@{-1}: master
> HEAD@{-2}: topic1
> HEAD@{-3}: 3f556e9 (detached)
> 
> Seems like reflog should be able to do this, and maybe it can, but I'm
> not sure. Any tips? I'd be fine making a convenient alias for this if
> it ends up being a series of piped commands.

The "@{-N}" syntax works by reading the HEAD reflog backwards and
grepping for "checkout: moving from ...". The implementation is in
grab_nth_branch_switch.

You could do it yourself like:

  git reflog HEAD |
  perl -lne '/checkout: moving from (\S+) to/ and print $1'

That includes detached HEADs, too. If you want just "real" branaches,
you could possibly omit entries which match [0-9a-f]{40}. But that's
just a heuristic (you _could_ have a branch that matches that).

-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: 'Failed to create .git/index.lock'

[Lars Schneider]

Hi Andreas,

I am looking into a similar issue with SourceTree on Windows right now. 
However, in my case it only happens when I switch branches. I investigated the 
Git commands that SourceTree triggers and it looks like it is doing something 
like this:

(1) Run checkout command
git.exe --no-pager -c color.branch=false -c color.diff=false -c 
color.status=false -c diff.mnemonicprefix=false -c core.quotepath=false 
checkout MY-BRANCH

(2) Run rev-parse command 
git.exe --no-pager -c color.branch=false -c color.diff=false -c 
color.status=false -c diff.mnemonicprefix=false -c core.quotepath=false 
rev-parse HEAD^1

My assumption is that SourceTree triggers these two commands in parallel and 
sometimes (2) locks the repo first which makes (1) fail. Does your Git process 
run on Windows, too? Is there a possibility that you issue Git commands in 
parallel? I also have read that certain anti virus software can trigger these 
errors on Windows.

I looked through the Git code found that increasing "core.packedrefstimeout" 
[1] might help in some cases that trigger this error [2] but not in others [3]. 
In my case this seems to help. Maybe it's worth a try for you, too?

Cheers,
Lars


[1] 
https://github.com/git/git/blob/494398473714dcbedb38b1ac79b531c7384b3bc4/refs/files-backend.c#L2070
[2] 
https://github.com/git/git/blob/494398473714dcbedb38b1ac79b531c7384b3bc4/lockfile.c#L171-L178
[3] 
https://github.com/git/git/blob/494398473714dcbedb38b1ac79b531c7384b3bc4/builtin/update-index.c#L1156



> On 09 Feb 2016, at 08:58, Andreas Krey  wrote:
> 
> Hi all,
> 
> I have a single workspace where executing merges
> occasionally leads to a left-over .git/index.lock file
> that prevents me from adding resolved conflicted files.
> 
> I'm running a sped-up integration/feature branch process,
> and the merges and conflict resolution are automated.
> But the index.lock thing is happening only in one repo
> of the three that are doing this.
> 
> Any hints as to debugging this?
> 
> (It is not a running background gc, the lock file
> is still around when there is nothing running
> any more, for hours.)
> 
> Andreas
> 
> -- 
> "Totally trivial. Famous last words."
> From: Linus Torvalds 
> Date: Fri, 22 Jan 2010 07:29:21 -0800
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH v4 1/3] t: do not hide Git's exit code in tests

[Jeff King]

On Mon, Feb 15, 2016 at 11:17:44AM +0100, larsxschnei...@gmail.com wrote:

> From: Lars Schneider 
> 
> Git should not be on the left-hand side of a pipe, because it hides the exit
> code, and we want to make sure git does not fail.

I think this is a nice cleanup.

> There is one more occurrence of this pattern in t9010-svn-fe.sh which is to
> evolved to change it easily.

The final sentence in the commit message needs s/to/too/. However, I'm
not sure this is the only remaining case. Doing:

  git grep -E 'git.*[^|]\|($|[^|])'

shows quite a few. I guess you just looked for "nul_to_q". There is
certainly no need to fix all of them, but you may want to note the
extent of your grepping in your commit message.

-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 v4 2/3] config: add 'type' to config_source struct that identifies config type

[Jeff King]

On Mon, Feb 15, 2016 at 11:17:45AM +0100, larsxschnei...@gmail.com wrote:

> From: Lars Schneider 
> 
> Use the config type to print more detailed error messages that inform
> the user about the origin of a config error (file, stdin, blob).
> 
> Signed-off-by: Lars Schneider 
> ---
>  cache.h|  6 --
>  config.c   | 36 +---
>  submodule-config.c |  4 ++--
>  t/t1300-repo-config.sh |  8 +++-
>  t/t1308-config-set.sh  |  4 ++--
>  5 files changed, 40 insertions(+), 18 deletions(-)

Looks good to me.

-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

[PATCH 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 |  4 ++--
 4 files changed, 12 insertions(+), 22 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 4b5b576..3a4ca9b 100644
--- a/remote.c
+++ b/remote.c
@@ -715,18 +715,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..7a5ee77 100644
--- a/remote.h
+++ b/remote.h
@@ -5,7 +5,7 @@
 #include "hashmap.h"
 
 enum {
-   REMOTE_CONFIG,
+   REMOTE_CONFIG = 1,
REMOTE_REMOTES,
REMOTE_BRANCHES
 };
@@ -59,7 +59,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 0/4] git remote improvements

[Thomas Gummerer]

In builtin/remote.c there are a few cases which check whether a remote
exists or not.  These checks are however done inconsistently, and in a
few cases they don't check anything at all.  This patch series tries
to improve the situation.

I stumbled upon this when I mistyped the remote name in git remote rm,
and got a cryptic error message, and thought the other cases might be
worth fixing as well.

There is one such check left in get_remote_ref_states, but I'm not
quite sure what to do about that one.  That function gets called in
git remote show/set-head/prune, where it might make sense for the user
to provide a url, or a local repository, at least in the git remote
show case.

I think we have the following options there:

 (1) just remove the check, as nobody seems to have noticed until now,
 and the check doesn't actually do anything (get_remote() is never
 called with NULL, so it will never return NULL).  In this case
 git dies when a non-existent remote is encountered, e.g. git
 remote show nonexistent existent will die immediately, while git
 remote show existent nonexistent will show info about the
 existing remote, and die afterwards.
 
 (2) add an option not to die in the transport function, and let the
 caller of get_remote_ref_states handle the error i.e. show the
 error and keep going with the next remote.

 (3) anything I might be missing?

Thomas Gummerer (4):
  remote: use skip_prefix
  remote: simplify remote_is_configured()
  remote: actually check if remote exits
  remote: use remote_is_configured() for add and rename

 builtin/fetch.c   |  5 ++---
 builtin/remote.c  | 23 ++-
 remote.c  | 22 +-
 remote.h  |  4 ++--
 t/t5505-remote.sh | 36 
 5 files changed, 55 insertions(+), 35 deletions(-)

-- 
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 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 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 1/4] remote: use skip_prefix

[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 commit mentioned above, but can
benefit from the same treatment to avoid magic numbers.

Signed-off-by: Thomas Gummerer 
---
 remote.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/remote.c b/remote.c
index 02e698a..4b5b576 100644
--- a/remote.c
+++ b/remote.c
@@ -321,8 +321,7 @@ static int handle_config(const char *key, const char 
*value, void *cb)
const char *subkey;
struct remote *remote;
struct branch *branch;
-   if (starts_with(key, "branch.")) {
-   name = key + 7;
+   if (skip_prefix(key, "branch.", )) {
subkey = strrchr(name, '.');
if (!subkey)
return 0;
@@ -338,9 +337,8 @@ static int handle_config(const char *key, const char 
*value, void *cb)
}
return 0;
}
-   if (starts_with(key, "url.")) {
+   if (skip_prefix(key, "url.", )) {
struct rewrite *rewrite;
-   name = key + 4;
subkey = strrchr(name, '.');
if (!subkey)
return 0;
@@ -357,9 +355,8 @@ static int handle_config(const char *key, const char 
*value, void *cb)
}
}
 
-   if (!starts_with(key,  "remote."))
+   if (!skip_prefix(key, "remote.", ))
return 0;
-   name = key + 7;
 
/* Handle remote.* variables */
if (!strcmp(name, "pushdefault"))
-- 
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

Re: What's cooking in git.git (Feb 2016, #04; Fri, 12)

[Torsten Bögershausen]

On 15/02/16 05:50, Junio C Hamano wrote:

Torsten Bögershausen  writes:


* tb/conversion (2016-02-10) 6 commits
   (merged to 'next' on 2016-02-12 at 6faf27b)

Could we keep it in next for a while ?

I found issues that needs to be fixed before going to master,
updates follow soonish.

Hmph, I somehow thought that everything was a no-op clean-up.  Any
regressions I failed to spot?

The "text" attribute was reported incorrectly in ls-files --eol.
A preleminary fix is here:

A proper fix will come next week.

--
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 0/3] config: add '--sources' option to print the source of a config value

[Jeff King]

On Mon, Feb 15, 2016 at 11:17:43AM +0100, larsxschnei...@gmail.com wrote:

> I like the idea of a "test set up block" within a test script. In order
> to clean up nicely before any subsequent tests I would like to propose
> a "tear down" block. Would that work as a compromise in our "test cases
> depend on earlier test cases" discussion?

I don't have any real problem with what you've written in the final
patch, but I also don't think it's accomplishing much (and is more lines
of code, and more running processes).

If you want to run test N without having run all of 1..N-1, what you
really want is some known, reliable state when that test starts. But the
tests before it do not necessarily know what that state is.  The best
they can do is roughly restore the original state before they ran. But:

  1. What does the state consist of? Which files (and their contents)
 are important to the test?

 In your tear-down you get rid of $INCLUDE_DIR, and you zero-out the
 config files. But you leave expect, output, output.raw, and the
 oddly named $CUSTOM_CONFIG_FILE. Nor do you clean up the
 environment variables.

 To be clear, I think it's perfectly fine to leave those. But you
 are still making assumptions about what the next test relies on.

  2. We may create a clean slate, but that is probably not what the next
 test wants. It will want to do its own setup. I.e., it will
 probably not want a blank .git/config, and will create it itself,
 just as you did in your setup step.

So rather than tearing down, I think we are better off trying to make
tests themselves (or blocks of them) set up their own assumptions. E.g.,
by overwriting files rather than appending to them. By using unique
filenames, commit messages, etc for their tests. That's less of a big
deal here, but in many tests that create commits, "test_commit foo"
would fail a second time, because there are no changes to "foo". Doing
"test_commit subdir/check-diff-in-subdir" is less likely to clash
without another test.

Sometimes we _are_ better off with a teardown step, because subsequent
tests would not reasonably think to clear some state we've set (e.g., in
non-config tests, if we set some random config variable, we use
test_config to tear it down afterwards rather than have each test clean
out all of the config). So there's definitely a subjective judgement
call on what is "reasonable" there. But I find it unlikely that your
tear-down will help anybody in this case. Further tests will not care
about $INCLUDE_DIR unless they reference it, and any further tests would
set up their own .git/config, etc.

-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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Jeff King]

On Mon, Feb 15, 2016 at 11:17:46AM +0100, larsxschnei...@gmail.com wrote:

> From: Lars Schneider 
> 
> If config values are queried using 'git config' (e.g. via --get,
> --get-all, --get-regexp, or --list flag) then it is sometimes hard to
> find the configuration file where the values were defined.
> 
> Teach 'git config' the '--show-origin' option to print the source
> configuration file for every printed value.
> 
> Based-on-patch-by: Jeff King 
> Signed-off-by: Lars Schneider 
> ---
>  Documentation/git-config.txt |  15 +++--
>  builtin/config.c |  33 ++
>  t/t1300-repo-config.sh   | 153 
> +++
>  3 files changed, 196 insertions(+), 5 deletions(-)

I packed all of my thoughts on test tear-down into my response to the
cover letter. :)

Other than that minor point, this version looks good to me.

-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] Disown ssh+git and git+ssh

[Eric Sunshine]

On Mon, Feb 15, 2016 at 9:29 AM, Carlos Martín Nieto  wrote:
> These were silly from the beginning, but we have to support them for

It might be helpful to cite some reference to support the claim that
they are "silly" since it's not necessarily obvious to readers who did
not following the discussion.

More below...

> compatibility. That doesn't mean we have to show them in the
> documentation. These were already left out of the main list, but a
> reference in the main manpage was left, so remove that.
>
> Also add a note to discourage their use if anybody goes looking for them
> in the source code.
>
> Signed-off-by: Carlos Martín Nieto 
> ---
> if (!strcmp(name, "git"))
> return PROTO_GIT;
> +   /*
> +* These ways to spell the ssh transport remain supported for
> +* compat but are undocumented and their use is discouraged
> +*/
> if (!strcmp(name, "git+ssh"))
> return PROTO_SSH;
> if (!strcmp(name, "ssh+git"))
> @@ -1002,6 +1002,10 @@ struct transport *transport_get(struct remote *remote, 
> const char *url)
> || starts_with(url, "file://")
> || starts_with(url, "git://")
> || starts_with(url, "ssh://")
> +   /*
> +* These ways to spell the ssh transport remain supported for
> +* compat but are undocumented and their use is discouraged
> +*/
> || starts_with(url, "git+ssh://")
> || starts_with(url, "ssh+git://")) {

A little "comment" bikeshedding: Aside from undesirably interrupting
the code flow, these large comment blocks draw far too much attention
from the reader than these deprecated spellings of "ssh" deserve, thus
making them seem overly important. How about minimizing their
importance by giving them minimal commentary?

|| starts_with(url, "ssh://")
|| starts_with(url, "git+ssh://") /* deprecated */
|| starts_with(url, "ssh+git://") { /* deprecated */

The term "deprecated" should be sufficient to explain that their use
is discouraged and why they are not documented anymore, and if a
reader wants to know more, the commit message can be consulted for the
full story.
--
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] remote: use skip_prefix

[Jeff King]

On Mon, Feb 15, 2016 at 06:42:27PM +0100, Thomas Gummerer wrote:

> 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 commit mentioned above, but can
> benefit from the same treatment to avoid magic numbers.

This is definitely an improvement, but I think we can actually go a step
further here, and use parse_config_key. Like:

diff --git a/remote.c b/remote.c
index 21e4ec3..8d2c3ca 100644
--- a/remote.c
+++ b/remote.c
@@ -318,15 +318,14 @@ 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 (starts_with(key, "branch", , , )) {
+   if (!name)
return 0;
-   branch = make_branch(name, subkey - name);
+   branch = make_branch(name, namelen);
if (!strcmp(subkey, ".remote")) {
return git_config_string(>remote_name, key, 
value);
} else if (!strcmp(subkey, ".pushremote")) {

and so on. The difference in lines of code isn't that great, but I think
it makes the resulting code more obvious to read.

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

[Jeff King]

On Mon, Feb 15, 2016 at 06:42:28PM +0100, Thomas Gummerer wrote:

> 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().

Makes sense. I wonder if we would want to give this an explicit slot in
the enum. I.e.:

> diff --git a/remote.h b/remote.h
> index 4fd7a0f..7a5ee77 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -5,7 +5,7 @@
>  #include "hashmap.h"
>  
>  enum {
> - REMOTE_CONFIG,
> + REMOTE_CONFIG = 1,
>   REMOTE_REMOTES,
>   REMOTE_BRANCHES
>  };

Add in "REMOTE_UNCONFIGURED = 0" here. It makes no difference to
correctness, but is perhaps documents what is going on a bit 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 3/4] remote: actually check if remote exits

[Jeff King]

On Mon, Feb 15, 2016 at 06:42:29PM +0100, Thomas Gummerer wrote:

> 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(-)

Nice. Very cleanly explained and implemented.

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

[Jeff King]

On Mon, Feb 15, 2016 at 06:42:30PM +0100, 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:
> 
>   [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".

Checking is_configured() certainly sounds like a better test, but...

> 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);

This original is quite confusing. I thought at first that there was
perhaps something going on with allowing repeated re-configuration of
the same remote, as long as some parameters matched. I.e., I am
wondering if there is a case here that does _not_ segfault, that we
would be breaking.

But reading over fb86e32dcc, I think I have convinced myself that it was
merely an ad-hoc check for "is_configured", and using that function is a
better replacement.

-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 1/4] remote: use skip_prefix

[Eric Sunshine]

On Mon, Feb 15, 2016 at 1:18 PM, Jeff King  wrote:
> This is definitely an improvement, but I think we can actually go a step
> further here, and use parse_config_key. Like:
>
> -   if (starts_with(key, "branch.")) {
> -   name = key + 7;
> -   subkey = strrchr(name, '.');
> -   if (!subkey)
> +   if (starts_with(key, "branch", , , )) {

I guess you meant: s/starts_with/parse_config_key/

> +   if (!name)
> return 0;
> -   branch = make_branch(name, subkey - name);
> +   branch = make_branch(name, namelen);
> if (!strcmp(subkey, ".remote")) {
> return git_config_string(>remote_name, key, 
> value);
> } else if (!strcmp(subkey, ".pushremote")) {
--
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] remote: use skip_prefix

[Jeff King]

On Mon, Feb 15, 2016 at 01:35:03PM -0500, Eric Sunshine wrote:

> On Mon, Feb 15, 2016 at 1:18 PM, Jeff King  wrote:
> > This is definitely an improvement, but I think we can actually go a step
> > further here, and use parse_config_key. Like:
> >
> > -   if (starts_with(key, "branch.")) {
> > -   name = key + 7;
> > -   subkey = strrchr(name, '.');
> > -   if (!subkey)
> > +   if (starts_with(key, "branch", , , )) {
> 
> I guess you meant: s/starts_with/parse_config_key/

Whoops, yeah. I'll belatedly add a "not even compile-tested" disclaimer.
:)

-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

[PATCH] http: add option to try authentication without username

[brian m. carlson]

Performing GSS-Negotiate authentication using Kerberos does not require
specifying a username or password, since that information is already
included in the ticket itself.  However, libcurl refuses to perform
authentication if it has not been provided with a username and password.
Add an option, http.emptyAuth, that provides libcurl with an empty
username and password to make it attempt authentication anyway.
---
I'm not super excited about this name, but I couldn't think of a better
one.  Suggestions welcome.

 Documentation/config.txt |  6 ++
 http.c   | 13 +++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 27f02be3..f11de77e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1648,6 +1648,12 @@ http.proxyAuthMethod::
 * `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)
 --
 
+http.emptyAuth::
+   Attempt authentication without seeking a username or password.  This
+   can be used to attempt GSS-Negotiate authentication without specifying
+   a username in the URL, as libcurl normally requires a username for
+   authentication.
+
 http.cookieFile::
File containing previously stored cookie lines which should be used
in the Git http session, if they match the server. The file format
diff --git a/http.c b/http.c
index dfc53c1e..a12a804b 100644
--- a/http.c
+++ b/http.c
@@ -87,6 +87,7 @@ static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
 static int http_proactive_auth;
 static const char *user_agent;
+static int curl_empty_auth;
 
 #if LIBCURL_VERSION_NUM >= 0x071700
 /* Use CURLOPT_KEYPASSWD as is */
@@ -299,14 +300,22 @@ static int http_options(const char *var, const char 
*value, void *cb)
if (!strcmp("http.useragent", var))
return git_config_string(_agent, var, value);
 
+   if (!strcmp("http.emptyauth", var)) {
+   curl_empty_auth = git_config_bool(var, value);
+   return 0;
+   }
+
/* Fall back on the default ones */
return git_default_config(var, value, cb);
 }
 
 static void init_curl_http_auth(CURL *result)
 {
-   if (!http_auth.username)
+   if (!http_auth.username) {
+   if (curl_empty_auth)
+   curl_easy_setopt(result, CURLOPT_USERPWD, ":");
return;
+   }
 
credential_fill(_auth);
 
@@ -827,7 +836,7 @@ struct active_request_slot *get_active_slot(void)
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
 #endif
-   if (http_auth.password)
+   if (http_auth.password || curl_empty_auth)
init_curl_http_auth(slot->curl);
 
return slot;
--
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: git svn dcommit doesn't support --username option for file:/// urls

[Tim Ringenbach]

On Mon, Feb 15, 2016 at 4:06 AM, Eric Wong  wrote:
[snip]
> Totally untested, but does flipping the order of auth providers
> help at all?

Thanks for looking into this. Unfortunately, that didn't seem to make
a difference.
I tried several times, and I tried both with and without
--interactive, but the commits
always shared up as my unix user.

I added a "print "test\n";" to make sure my modify copy was being
used, and I did see
that output, so I know I was running the right code.

For reference, here's what diff outputs on my side.

--- git-2.7.1/perl/Git/SVN/Ra.pm 2016-02-05 17:31:08.0 -0600
+++ local/share/perl/5.10.1/Git/SVN/Ra.pm 2016-02-15 13:06:27.0 -0600
@@ -42,7 +42,9 @@ END {

 sub _auth_providers () {
  require SVN::Client;
+ print "test\n";
  my @rv = (
+  SVN::Client::get_username_provider(),
   SVN::Client::get_simple_provider(),
   SVN::Client::get_ssl_server_trust_file_provider(),
   SVN::Client::get_simple_prompt_provider(
@@ -53,7 +55,6 @@ sub _auth_providers () {
   SVN::Client::get_ssl_client_cert_pw_file_provider(),
   SVN::Client::get_ssl_client_cert_pw_prompt_provider(
 \::SVN::Prompt::ssl_client_cert_pw, 2),
-  SVN::Client::get_username_provider(),
   SVN::Client::get_ssl_server_trust_prompt_provider(
 \::SVN::Prompt::ssl_server_trust),
   SVN::Client::get_username_prompt_provider(


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

Re: [PATCH] http: add option to try authentication without username

[Eric Sunshine]

On Mon, Feb 15, 2016 at 1:44 PM, brian m. carlson
 wrote:
> Performing GSS-Negotiate authentication using Kerberos does not require
> specifying a username or password, since that information is already
> included in the ticket itself.  However, libcurl refuses to perform
> authentication if it has not been provided with a username and password.
> Add an option, http.emptyAuth, that provides libcurl with an empty
> username and password to make it attempt authentication anyway.

I'm not familiar with this code, so let me know if my comments (below)
are off the mark...

> ---
> diff --git a/http.c b/http.c
> +++ b/http.c
> @@ -299,14 +300,22 @@ static int http_options(const char *var, const char 
> *value, void *cb)
>  static void init_curl_http_auth(CURL *result)
>  {
> -   if (!http_auth.username)
> +   if (!http_auth.username) {
> +   if (curl_empty_auth)
> +   curl_easy_setopt(result, CURLOPT_USERPWD, ":");

Does this need to take LIBCURL_VERSION_NUM into account? Other code
which uses CURLOPT_USERPWD only does so for certain versions of
libcurl, otherwise CURLOPT_USERNAME and CURLOPT_PASSWORD is used.

> return;
> +   }
>
> credential_fill(_auth);
>
> @@ -827,7 +836,7 @@ struct active_request_slot *get_active_slot(void)
>  #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
>  #endif
> -   if (http_auth.password)
> +   if (http_auth.password || curl_empty_auth)
> init_curl_http_auth(slot->curl);
>
> return slot;

Rather than sprinkling curl_empty_auth special cases here and there,
would it be possible to simply set http_auth.username and
http_auth.password to empty strings early on if they are not already
set and curl_empty_auth is true, and then let the:

strbuf_addf(, "%s:%s",
http_auth.username, http_auth.password);

in init_curl_http_auth() handle them in the normal fashion, with the
end result being the same ":" set explicitly by this patch?
--
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: git archive should use vendor extension in pax header

[René Scharfe]

Am 06.02.2016 um 15:57 schrieb f...@fuz.su:

On Sat, Feb 06, 2016 at 02:23:11PM +0100, René Scharfe wrote:

Am 28.01.2016 um 00:45 schrieb f...@fuz.su:

There is git get-tar-commit-id, which prints the commit ID if it
finds a comment entry which looks like a hexadecimal SHA-1 hash.
It's better than a hex editor at least. :)


This is incredibly fuzzy and can get wrong for a pleothora of reasons.
I hope you agree though that the situation is suboptimal, git is doing
the equivalent of using a custom file format without an easily
recognizable magic number.


It is fuzzy in theory. But which other programs allow writing a
comment header?  I'm not aware of any, but I have to admit that I
didn't look too hard.


Well, let's say what happens if the Mercurial folks were to implement
the same thing? Suddenly there is a conflict. Yes, of course, right now
there might be no program that uses the comment field for its own
purpose but such design decisions tend to be not future proof. There is
a very good reason why file formats typically have magic numbers and
don't just rely on people knowing that the file has a certain type and
that is the same reason why git should mark its meta data in a unique
fashion.


Chances are good that Mercurial would do it in a way that doesn't 
conflict with git's tar comments.  I get your point, though, and agree 
that it's not ideal.  However, so far it's just a potential problem.



But I'm still interested how you got a collection of tar files with
unknown origin.  Just curious.


Easy: Just download the (source) distribution archives of a distribution
of choice and try to verify that the tarballs they use to compile their
packages actually come from the project's public git repositories.


OK, that's easier than calculating checksums and comparing them with
those published by the respective projects, but also less
trustworthy.


If you have a known trusted archive, you could use it directly, no need
for cross-verification. The intent is to be able to check if archives
generated by someone from some sources could have plausibly been
generated from these sources.


It's probably not too important, but I think I still don't fully 
understand.  So you have a tar file of unknown origin.  You hand it to 
git get-tar-commit-id or a similar tool and get back 
a08595f76159b09d57553e37a5123f1091bb13e7.  You can google this string 
and find out it's the commit ID for git v2.7.1.


Your tar file could have been modified in various ways, though, e.g. 
with tar u or tar --delete.  So you try to find a download site for the 
software that includes file hashes for archives of this release, like in

https://www.kernel.org/pub/software/scm/git/sha256sums.asc.

If the published hash and a hash of your file match then you can be 
reasonably sure the files are the same.  If they don't then it could be 
due to variations added by the compressor.  You can download the 
authoritative archive and compare it with yours.


Is that how it goes?


I'm very interested in hearing about any git specific bugs.


I don't know any. Bugs tens to be known only after 1000s of buggy
archives have been published (just as with some GNU tar bugs). It's
great to have a way to detect that the archive might be affected by
a bug so you know that you need to work around it.


That requires a field containing the git version which was used to 
create the archive, no?



Thinking about the problem a bit more and discussion with the
aforementioned Jörg Schilling we came to the conclusion that the best
way to deal with an “file omitted” attribute is to attach it to the
directory that would normally contain the omitted file.


Sounds sensible, but the ordering can be a bit tricky.  If d/a is 
included and d/b is not then it would be easy to write d/, d/a and the 
extended header that says that d/b is excluded, in that order.  Writing 
the extended header first is a bit harder and I'm not sure if it's 
needed.  And it gets tricky if more than one entry is excluded per 
directory. (Just thinking out loud here.)



Letting archivers extract meta data as regular files is annoying to
those that are not interested in it.  Extended headers themselves
(type g) are bad enough already in this regard for those stuck with
old tar versions.


I think we can safely assume that systems support pax headers 15 years
after they have been standardized. I was actually unable to find a
non-historical version of a serious archiver that claims to support tar
archives but doesn't support pax headers.


Well, that depends on your definition of "serious".  Plan 9's tar 
perhaps doesn't fit it, but what about 7-Zip (http://www.7-zip.org/)?


And there is no way (or did I overlook it?) to modify or display the 
comment extended header using GNU tar.  That's actually surprising to 
me: I'd think the ability to add a human-readable description to a 
backup on tape is quite important.  (But I didn't touch an actual tape 
for quite a while, and I never used tar 

Re: [PATCH] http: add option to try authentication without username

[brian m. carlson]

On Mon, Feb 15, 2016 at 03:19:25PM -0500, Eric Sunshine wrote:
> On Mon, Feb 15, 2016 at 1:44 PM, brian m. carlson
>  wrote:
> > Performing GSS-Negotiate authentication using Kerberos does not require
> > specifying a username or password, since that information is already
> > included in the ticket itself.  However, libcurl refuses to perform
> > authentication if it has not been provided with a username and password.
> > Add an option, http.emptyAuth, that provides libcurl with an empty
> > username and password to make it attempt authentication anyway.
> 
> I'm not familiar with this code, so let me know if my comments (below)
> are off the mark...
> 
> > ---
> > diff --git a/http.c b/http.c
> > +++ b/http.c
> > @@ -299,14 +300,22 @@ static int http_options(const char *var, const char 
> > *value, void *cb)
> >  static void init_curl_http_auth(CURL *result)
> >  {
> > -   if (!http_auth.username)
> > +   if (!http_auth.username) {
> > +   if (curl_empty_auth)
> > +   curl_easy_setopt(result, CURLOPT_USERPWD, ":");
> 
> Does this need to take LIBCURL_VERSION_NUM into account? Other code
> which uses CURLOPT_USERPWD only does so for certain versions of
> libcurl, otherwise CURLOPT_USERNAME and CURLOPT_PASSWORD is used.

This is the oldest version, which means it's the most compatible.  Since
we don't need to consider odd usernames, it seemed silly to have both
cases present in the code.  The benefit of using the pair of options is
that we don't leak memory in the non-empty auth case.

> > return;
> > +   }
> >
> > credential_fill(_auth);
> >
> > @@ -827,7 +836,7 @@ struct active_request_slot *get_active_slot(void)
> >  #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> > curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
> >  #endif
> > -   if (http_auth.password)
> > +   if (http_auth.password || curl_empty_auth)
> > init_curl_http_auth(slot->curl);
> >
> > return slot;
> 
> Rather than sprinkling curl_empty_auth special cases here and there,
> would it be possible to simply set http_auth.username and
> http_auth.password to empty strings early on if they are not already
> set and curl_empty_auth is true, and then let the:
> 
> strbuf_addf(, "%s:%s",
> http_auth.username, http_auth.password);
> 
> in init_curl_http_auth() handle them in the normal fashion, with the
> end result being the same ":" set explicitly by this patch?

That would work.  I was concerned about the credential_fill call
actually prompting the user, but it appears that it doesn't do that if
the password already exists.  I don't know if we want to rely on that
functionality, though.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: [PATCH] http: add option to try authentication without username

[Jeff King]

On Mon, Feb 15, 2016 at 08:29:37PM +, brian m. carlson wrote:

> > Rather than sprinkling curl_empty_auth special cases here and there,
> > would it be possible to simply set http_auth.username and
> > http_auth.password to empty strings early on if they are not already
> > set and curl_empty_auth is true, and then let the:
> > 
> > strbuf_addf(, "%s:%s",
> > http_auth.username, http_auth.password);
> > 
> > in init_curl_http_auth() handle them in the normal fashion, with the
> > end result being the same ":" set explicitly by this patch?
> 
> That would work.  I was concerned about the credential_fill call
> actually prompting the user, but it appears that it doesn't do that if
> the password already exists.  I don't know if we want to rely on that
> functionality, though.

Yeah, credential_fill() will treat that as a noop, as it is no different
than getting "https://user:p...@example.com; in the URL in the first
place. But it will _also_ send the result to credential_approve() and
credential_reject(), which you probably don't want (because you do not
want to store these useless dummy credentials in your keystore).

So I think this hack should remain purely at the curl level, and never
touch the credential struct at all.

Which is a shame, because I think Eric's suggestion is otherwise much
more readable. :)

-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] http: add option to try authentication without username

[brian m. carlson]

On Mon, Feb 15, 2016 at 03:34:51PM -0500, Jeff King wrote:
> On Mon, Feb 15, 2016 at 08:29:37PM +, brian m. carlson wrote:
> > That would work.  I was concerned about the credential_fill call
> > actually prompting the user, but it appears that it doesn't do that if
> > the password already exists.  I don't know if we want to rely on that
> > functionality, though.
> 
> Yeah, credential_fill() will treat that as a noop, as it is no different
> than getting "https://user:p...@example.com; in the URL in the first
> place. But it will _also_ send the result to credential_approve() and
> credential_reject(), which you probably don't want (because you do not
> want to store these useless dummy credentials in your keystore).

Correct.

> So I think this hack should remain purely at the curl level, and never
> touch the credential struct at all.
> 
> Which is a shame, because I think Eric's suggestion is otherwise much
> more readable. :)

Yes, I agree.  That would have been a much nicer and smaller change.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: [PATCH 1/4] remote: use skip_prefix

[Thomas Gummerer]

On 02/15, Jeff King wrote:
> On Mon, Feb 15, 2016 at 06:42:27PM +0100, Thomas Gummerer wrote:
>
> > 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 commit mentioned above, but can
> > benefit from the same treatment to avoid magic numbers.
>
> This is definitely an improvement, but I think we can actually go a step
> further here, and use parse_config_key. Like:

Thanks, I had no idea about this function :) It makes the diff a lot
noisier, but I do think the end result is better.

> diff --git a/remote.c b/remote.c
> index 21e4ec3..8d2c3ca 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -318,15 +318,14 @@ 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 (starts_with(key, "branch", , , )) {
> + if (!name)
>   return 0;
> - branch = make_branch(name, subkey - name);
> + branch = make_branch(name, namelen);
>   if (!strcmp(subkey, ".remote")) {
>   return git_config_string(>remote_name, key, 
> value);
>   } else if (!strcmp(subkey, ".pushremote")) {
>
> and so on. The difference in lines of code isn't that great, but I think
> it makes the resulting code more obvious to read.
>
> -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 2/4] remote: simplify remote_is_configured()

[Thomas Gummerer]

On 02/15, Jeff King wrote:
> On Mon, Feb 15, 2016 at 06:42:28PM +0100, Thomas Gummerer wrote:
>
> > 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().
>
> Makes sense. I wonder if we would want to give this an explicit slot in
> the enum. I.e.:
>
> > diff --git a/remote.h b/remote.h
> > index 4fd7a0f..7a5ee77 100644
> > --- a/remote.h
> > +++ b/remote.h
> > @@ -5,7 +5,7 @@
> >  #include "hashmap.h"
> >
> >  enum {
> > -   REMOTE_CONFIG,
> > +   REMOTE_CONFIG = 1,
> > REMOTE_REMOTES,
> > REMOTE_BRANCHES
> >  };
>
> Add in "REMOTE_UNCONFIGURED = 0" here. It makes no difference to
> correctness, but is perhaps documents what is going on a bit better.

Agreed, will change.  Thanks.

>
> -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 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 06:42:30PM +0100, 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:
> >
> >   [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".
>
> Checking is_configured() certainly sounds like a better test, but...
>
> > 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);
>
> This original is quite confusing. I thought at first that there was
> perhaps something going on with allowing repeated re-configuration of
> the same remote, as long as some parameters matched. I.e., I am
> wondering if there is a case here that does _not_ segfault, that we
> would be breaking.
>
> But reading over fb86e32dcc, I think I have convinced myself that it was
> merely an ad-hoc check for "is_configured", and using that function is a
> better replacement.

It took me a while too to convince myself there is nothing strange
going on.  But I could neither find anything in the history, nor could
I think of any case that we could break.

Thanks for your review!

> -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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Eric Sunshine]

On Mon, Feb 15, 2016 at 5:17 AM,   wrote:
> If config values are queried using 'git config' (e.g. via --get,
> --get-all, --get-regexp, or --list flag) then it is sometimes hard to
> find the configuration file where the values were defined.
>
> Teach 'git config' the '--show-origin' option to print the source
> configuration file for every printed value.
>
> Based-on-patch-by: Jeff King 
> Signed-off-by: Lars Schneider 
> ---
> diff --git a/builtin/config.c b/builtin/config.c
> @@ -27,6 +28,7 @@ static int actions, types;
>  static const char *get_color_slot, *get_colorbool_slot;
>  static int end_null;

Not related to your changes, but I just realized that this variable
really ought to be named 'end_nul' since we're talking about the
character NUL, not a NULL pointer.

>  static int respect_includes = -1;
> +static int show_origin;
> @@ -81,6 +83,7 @@ static struct option builtin_config_options[] = {
> OPT_BOOL('z', "null", _null, N_("terminate values with NUL 
> byte")),

Likewise, the long option name should be --nul rather than --null, or
the long name could be dropped altogether since some other commands
just recognize short option -z.

There is no need for this patch series to address this anomaly; it's
perhaps low-hanging fruit for someone wanting to join the project. The
only very minor wrinkle is that we'd still need to recognize --null as
a deprecated (and undocumented) alias for --nul.

> OPT_BOOL(0, "name-only", _values, N_("show variable names 
> only")),
> OPT_BOOL(0, "includes", _includes, N_("respect include 
> directives on lookup")),
> +   OPT_BOOL(0, "show-origin", _origin, N_("show origin of config 
> (file, stdin, blob, cmdline)")),
> OPT_END(),
>  };
--
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 03:58:12PM -0500, Eric Sunshine wrote:

> > diff --git a/builtin/config.c b/builtin/config.c
> > @@ -27,6 +28,7 @@ static int actions, types;
> >  static const char *get_color_slot, *get_colorbool_slot;
> >  static int end_null;
> 
> Not related to your changes, but I just realized that this variable
> really ought to be named 'end_nul' since we're talking about the
> character NUL, not a NULL pointer.

Yeah, I noticed that, too. We just went through a round of related fixes
here, and the "usual" name is now "nul_term_line". I don't especially
like that one either, but at least it is correct and consistent. :)

> >  static int respect_includes = -1;
> > +static int show_origin;
> > @@ -81,6 +83,7 @@ static struct option builtin_config_options[] = {
> > OPT_BOOL('z', "null", _null, N_("terminate values with NUL 
> > byte")),
> 
> Likewise, the long option name should be --nul rather than --null, or
> the long name could be dropped altogether since some other commands
> just recognize short option -z.
> 
> There is no need for this patch series to address this anomaly; it's
> perhaps low-hanging fruit for someone wanting to join the project. The
> only very minor wrinkle is that we'd still need to recognize --null as
> a deprecated (and undocumented) alias for --nul.

I think that would be OK as long as we keep the compatible option.

-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: git svn dcommit doesn't support --username option for file:/// urls

[Eric Wong]

Tim Ringenbach  wrote:
> On Mon, Feb 15, 2016 at 4:06 AM, Eric Wong  wrote:
> [snip]
> > Totally untested, but does flipping the order of auth providers
> > help at all?
> 
> Thanks for looking into this. Unfortunately, that didn't seem to make
> a difference.

Thanks for trying.

It might take a while for me to get around to looking at this
more, so it would be very helpful if you poked around and tried
some different things in the source.

It should be helpful to look at any other SVN wrappers (or code
SVN itself).  In the past, I got a lot of help from looking at
svk/SVN::Mirror.

I'm certainly no expert when it comes to using the SVN API,
so it's likely we're doing something wrong...

Btw, which version of SVN are you using?  I also wonder if
there's something version-dependent.
--
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: What's cooking in git.git (Feb 2016, #04; Fri, 12)

[Junio C Hamano]

Torsten Bögershausen  writes:

> On 15/02/16 05:50, Junio C Hamano wrote:
>> Torsten Bögershausen  writes:
>>
 * tb/conversion (2016-02-10) 6 commits
(merged to 'next' on 2016-02-12 at 6faf27b)
>>> Could we keep it in next for a while ?
>>>
>>> I found issues that needs to be fixed before going to master,
>>> updates follow soonish.
>> Hmph, I somehow thought that everything was a no-op clean-up.  Any
>> regressions I failed to spot?
> The "text" attribute was reported incorrectly in ls-files --eol.

Interesting. I vaguely recall that I said something about the change
in the semantics for the reporting during the review...

> A preleminary fix is here:
> 
> A proper fix will come next week.

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 v4 2/3] config: add 'type' to config_source struct that identifies config type

[Ramsay Jones]

On 15/02/16 10:17, larsxschnei...@gmail.com wrote:
> From: Lars Schneider 
> 
> Use the config type to print more detailed error messages that inform
> the user about the origin of a config error (file, stdin, blob).
> 
> Signed-off-by: Lars Schneider 
> ---
>  cache.h|  6 --
>  config.c   | 36 +---
>  submodule-config.c |  4 ++--
>  t/t1300-repo-config.sh |  8 +++-
>  t/t1308-config-set.sh  |  4 ++--
>  5 files changed, 40 insertions(+), 18 deletions(-)
> 
> diff --git a/cache.h b/cache.h
> index c63fcc1..8d86e5c 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1485,8 +1485,8 @@ struct git_config_source {
>  typedef int (*config_fn_t)(const char *, const char *, void *);
>  extern int git_default_config(const char *, const char *, void *);
>  extern int git_config_from_file(config_fn_t fn, const char *, void *);
> -extern int git_config_from_buf(config_fn_t fn, const char *name,
> -const char *buf, size_t len, void *data);
> +extern int git_config_from_mem(config_fn_t fn, const char *type,
> + const char *name, const char *buf, 
> size_t len, void *data);
>  extern void git_config_push_parameter(const char *text);
>  extern int git_config_from_parameters(config_fn_t fn, void *data);
>  extern void git_config(config_fn_t fn, void *);
> @@ -1525,6 +1525,8 @@ extern const char *get_log_output_encoding(void);
>  extern const char *get_commit_output_encoding(void);
>  
>  extern int git_config_parse_parameter(const char *, config_fn_t fn, void 
> *data);
> +extern const char *current_config_type();
> +extern const char *current_config_name();
>  
>  struct config_include_data {
>   int depth;
> diff --git a/config.c b/config.c
> index 86a5eb2..5cf11b5 100644
> --- a/config.c
> +++ b/config.c
> @@ -24,6 +24,7 @@ struct config_source {
>   size_t pos;
>   } buf;
>   } u;
> + const char *type;
>   const char *name;
>   const char *path;
>   int die_on_error;
> @@ -471,9 +472,9 @@ static int git_parse_source(config_fn_t fn, void *data)
>   break;
>   }
>   if (cf->die_on_error)
> - die(_("bad config file line %d in %s"), cf->linenr, cf->name);
> + die(_("bad config line %d in %s %s"), cf->linenr, cf->type, 
> cf->name);
>   else
> - return error(_("bad config file line %d in %s"), cf->linenr, 
> cf->name);
> + return error(_("bad config line %d in %s %s"), cf->linenr, 
> cf->type, cf->name);
>  }
>  
>  static int parse_unit_factor(const char *end, uintmax_t *val)
> @@ -588,9 +589,9 @@ static void die_bad_number(const char *name, const char 
> *value)
>   if (!value)
>   value = "";
>  
> - if (cf && cf->name)
> - die(_("bad numeric config value '%s' for '%s' in %s: %s"),
> - value, name, cf->name, reason);
> + if (cf && cf->type && cf->name)
> + die(_("bad numeric config value '%s' for '%s' in %s %s: %s"),
> + value, name, cf->type, cf->name, reason);
>   die(_("bad numeric config value '%s' for '%s': %s"), value, name, 
> reason);
>  }
>  
> @@ -1061,11 +1062,13 @@ static int do_config_from(struct config_source *top, 
> config_fn_t fn, void *data)
>  }
>  
>  static int do_config_from_file(config_fn_t fn,
> - const char *name, const char *path, FILE *f, void *data)
> + const char *type, const char *name, const char *path, FILE *f,
> + void *data)
>  {
>   struct config_source top;
>  
>   top.u.file = f;
> + top.type = type;
>   top.name = name;
>   top.path = path;
>   top.die_on_error = 1;
> @@ -1078,7 +1081,7 @@ static int do_config_from_file(config_fn_t fn,
>  
>  static int git_config_from_stdin(config_fn_t fn, void *data)
>  {
> - return do_config_from_file(fn, "", NULL, stdin, data);
> + return do_config_from_file(fn, "stdin", "", NULL, stdin, data);

I think this should be:
return do_config_from_file(fn, "file", "", NULL, stdin, data);

ie.  is not a separate type, but a file that does not exist in
the filesystem and, thus, has no name. (what you use internally is a
separate issue, but  works for me.)

Also, I'm so used to '' as the 'name' (token/handle) of that
file, that it looks very odd spelt otherwise. :-D

>  }
>  
>  int git_config_from_file(config_fn_t fn, const char *filename, void *data)
> @@ -1089,21 +1092,22 @@ int git_config_from_file(config_fn_t fn, const char 
> *filename, void *data)
>   f = fopen(filename, "r");
>   if (f) {
>   flockfile(f);
> - ret = do_config_from_file(fn, filename, filename, f, data);
> + ret = do_config_from_file(fn, "file", filename, filename, f, 
> data);
>   funlockfile(f);
>   fclose(f);
>   }
>   

Re: [PULL] svn pathnameencoding for git svn dcommit

[Junio C Hamano]

Eric Wong  writes:

> I've amended tests to both commits, but the URL encoding one
> requires an HTTP server to test effectively.
>
> I couldn't find a test prereq for httpd, but perhaps it's good
> to test by default regardless in case a future SVN changes
> file:// behavior.  I've only tested this with SVN 1.6.17 under
> Debian wheezy.
>
> The following changes since commit 6faf27b4ff26804a07363078b238b5cfd3dfa976:
>
>   Merge branch 'tb/conversion' into next (2016-02-12 10:20:20 -0800)
>
> are available in the git repository at:
>
>   git://bogomips.org/git-svn.git ks/svn-pathnameencoding
>
> for you to fetch changes up to dfee0cf8123e7f63268f05a02731ce82db136188:
>
>   git-svn: apply "svn.pathnameencoding" before URL encoding (2016-02-15 
> 00:31:21 +)

Sorry, Eric, I cannot pull this.  You made your branch on top of
'next', there are many commits that are not ready.

>
> 
> 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 | 26 --
>  2 files changed, 27 insertions(+), 3 deletions(-)
--
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: 'Failed to create .git/index.lock'

[Andreas Krey]

On Mon, 15 Feb 2016 18:06:33 +, Lars Schneider wrote:
> Hi Andreas,
> 
> I am looking into a similar issue with SourceTree on Windows right now. 
> However, in my case it only happens when I switch branches.

Semi-bingo. That is actually a difference between the workspace this
happens in and the others. This one works on many branches; the others
clone anew for each new branch.

I investigated the Git commands that SourceTree triggers and it looks like it 
is doing something like this:

> (1) Run checkout command
> git.exe --no-pager -c color.branch=false -c color.diff=false -c 
> color.status=false -c diff.mnemonicprefix=false -c core.quotepath=false 
> checkout MY-BRANCH
> 
> (2) Run rev-parse command 
> git.exe --no-pager -c color.branch=false -c color.diff=false -c 
> color.status=false -c diff.mnemonicprefix=false -c core.quotepath=false 
> rev-parse HEAD^1
> 
> My assumption is that SourceTree triggers these two commands in parallel and 
> sometimes (2) locks the repo first which makes (1) fail.

That would be partly a bug in SourceTree to do so.

> Does your Git process run on Windows, too?

No. RHEL7 Linux.

> Is there a possibility that you issue Git commands in parallel?

I don't think so, except for background GC (which isn't quite background -
one phase seems to still run foregrounded).

> I also have read that certain anti virus software can trigger these errors on 
> Windows.

That is an ugly problem indeed.

> I looked through the Git code found that increasing "core.packedrefstimeout" 
> [1] might help in some cases that trigger this error [2] but not in others 
> [3]. In my case this seems to help. Maybe it's worth a try for you, too?

I may need to look into that. Because sometimes the lock causes my
automatic process to fail but the lock file isn't there anymore when
I get to cleanup. But sometimes it keeps existing.

Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds 
Date: Fri, 22 Jan 2010 07:29:21 -0800
--
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

[Ramsay Jones]

On 15/02/16 10:17, larsxschnei...@gmail.com wrote:
> From: Lars Schneider 
> 
> If config values are queried using 'git config' (e.g. via --get,
> --get-all, --get-regexp, or --list flag) then it is sometimes hard to
> find the configuration file where the values were defined.
> 
> Teach 'git config' the '--show-origin' option to print the source
> configuration file for every printed value.
> 
> Based-on-patch-by: Jeff King 
> Signed-off-by: Lars Schneider 
> ---
>  Documentation/git-config.txt |  15 +++--
>  builtin/config.c |  33 ++
>  t/t1300-repo-config.sh   | 153 
> +++
>  3 files changed, 196 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 2608ca7..6374997 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -9,18 +9,18 @@ git-config - Get and set repository or global options
>  SYNOPSIS
>  
>  [verse]
> -'git config' [] [type] [-z|--null] name [value [value_regex]]
> +'git config' [] [type] [--show-origin] [-z|--null] name [value 
> [value_regex]]
>  'git config' [] [type] --add name value
>  'git config' [] [type] --replace-all name value [value_regex]
> -'git config' [] [type] [-z|--null] --get name [value_regex]
> -'git config' [] [type] [-z|--null] --get-all name [value_regex]
> -'git config' [] [type] [-z|--null] [--name-only] --get-regexp 
> name_regex [value_regex]
> +'git config' [] [type] [--show-origin] [-z|--null] --get name 
> [value_regex]
> +'git config' [] [type] [--show-origin] [-z|--null] --get-all 
> name [value_regex]
> +'git config' [] [type] [--show-origin] [-z|--null] 
> [--name-only] --get-regexp name_regex [value_regex]
>  'git config' [] [type] [-z|--null] --get-urlmatch name URL
>  'git config' [] --unset name [value_regex]
>  'git config' [] --unset-all name [value_regex]
>  'git config' [] --rename-section old_name new_name
>  'git config' [] --remove-section name
> -'git config' [] [-z|--null] [--name-only] -l | --list
> +'git config' [] [--show-origin] [-z|--null] [--name-only] -l | 
> --list
>  'git config' [] --get-color name [default]
>  'git config' [] --get-colorbool name [stdout-is-tty]
>  'git config' [] -e | --edit
> @@ -194,6 +194,11 @@ See also <>.
>   Output only the names of config variables for `--list` or
>   `--get-regexp`.
>  
> +--show-origin::
> + Augment the output of all queried config options with the
> + origin type (file, stdin, blob, cmdline) and the actual origin

file, blob, cmdline (hmm, maybe cmd-line? ;-) )

> + (config file path, ref, or blob id if applicable).
> +
>  --get-colorbool name [stdout-is-tty]::
>  
>   Find the color setting for `name` (e.g. `color.diff`) and output
> diff --git a/builtin/config.c b/builtin/config.c
> index adc7727..7bad0c0 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -3,6 +3,7 @@
>  #include "color.h"
>  #include "parse-options.h"
>  #include "urlmatch.h"
> +#include "quote.h"
>  
>  static const char *const builtin_config_usage[] = {
>   N_("git config []"),
> @@ -27,6 +28,7 @@ static int actions, types;
>  static const char *get_color_slot, *get_colorbool_slot;
>  static int end_null;
>  static int respect_includes = -1;
> +static int show_origin;
>  
>  #define ACTION_GET (1<<0)
>  #define ACTION_GET_ALL (1<<1)
> @@ -81,6 +83,7 @@ static struct option builtin_config_options[] = {
>   OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")),
>   OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
>   OPT_BOOL(0, "includes", _includes, N_("respect include 
> directives on lookup")),
> + OPT_BOOL(0, "show-origin", _origin, N_("show origin of config 
> (file, stdin, blob, cmdline)")),
>   OPT_END(),
>  };
>  
> @@ -91,8 +94,28 @@ static void check_argc(int argc, int min, int max) {
>   usage_with_options(builtin_config_usage, builtin_config_options);
>  }
>  
> +static void show_config_origin(struct strbuf *buf)
> +{
> + const char term = end_null ? '\0' : '\t';
> +
> + strbuf_addstr(buf, current_config_type());
> + strbuf_addch(buf, ':');
> + if (end_null)
> + strbuf_addstr(buf, current_config_name());
> + else
> + quote_c_style(current_config_name(), buf, NULL, 0);
> + strbuf_addch(buf, term);
> +}
> +
>  static int show_all_config(const char *key_, const char *value_, void *cb)
>  {
> + if (show_origin) {
> + struct strbuf buf = STRBUF_INIT;
> + show_config_origin();
> + /* Use fwrite as "buf" can contain \0's if "end_null" is set. */
> + fwrite(buf.buf, 1, buf.len, stdout);
> + strbuf_release();
> + }
>   if (!omit_values && value_)
>   printf("%s%c%s%c", key_, delim, value_, term);
>   else
> @@ -108,6 +131,8 @@ struct strbuf_list {
>  
>  

Re: [PATCH 2/4] remote: simplify remote_is_configured()

[Junio C Hamano]

Thomas Gummerer  writes:

> Agreed, will change.  Thanks.

Thanks for working on this, and thanks reviewers for helping.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] http: add option to try authentication without username

[Junio C Hamano]

"brian m. carlson"  writes:

> On Mon, Feb 15, 2016 at 03:34:51PM -0500, Jeff King wrote:
> ...
>> So I think this hack should remain purely at the curl level, and never
>> touch the credential struct at all.
>> 
>> Which is a shame, because I think Eric's suggestion is otherwise much
>> more readable. :)
>
> Yes, I agree.  That would have been a much nicer and smaller change.

Alright, reading all reviews and taking them into account, the
original, when a Sign-off is added, would be acceptable, it seems.

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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Jeff King]

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.

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.

-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] http: add option to try authentication without username

[brian m. carlson]

On Mon, Feb 15, 2016 at 01:39:30PM -0800, Junio C Hamano wrote:
> "brian m. carlson"  writes:
> 
> > On Mon, Feb 15, 2016 at 03:34:51PM -0500, Jeff King wrote:
> > ...
> >> So I think this hack should remain purely at the curl level, and never
> >> touch the credential struct at all.
> >> 
> >> Which is a shame, because I think Eric's suggestion is otherwise much
> >> more readable. :)
> >
> > Yes, I agree.  That would have been a much nicer and smaller change.
> 
> Alright, reading all reviews and taking them into account, the
> original, when a Sign-off is added, would be acceptable, it seems.

Sorry about that.  Please add my

Signed-off-by: brian m. carlson 
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

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

[Junio C Hamano]

Ramsay Jones  writes:

>> +--show-origin::
>> +Augment the output of all queried config options with the
>> +origin type (file, stdin, blob, cmdline) and the actual origin
>
> file, blob, cmdline (hmm, maybe cmd-line? ;-) )

If we are going to spell it out, "command-line" would be the way to
go.  "cmdline" is probably OK.
--
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 0/18] hardening allocations against integer overflow

[Jeff King]

About 6 months or so ago, I did an audit of git's code base for uses of
strcpy and sprintf that could overflow, fixing any bugs and cleaning up
any suspect spots to make further audits simpler.  This is a
continuation of that work, for size computations which can overflow and
cause us to allocate a too-small buffer. E.g., something like:

  char *concat(const char *a, const char *b)
  {
unsigned len_a = strlen(a);
unsigned len_b = strlen(b);
char *ret = xmalloc(len_a + len_b);
memcpy(ret, a, len_a);
memcpy(ret, b, len_b);
  }

will behave badly if the sum of "a" and "b" overflows "unsigned". There
are other variants, too (we are also truncating the return value from
strlen, and we'd frequently use "int" here, so the lengths can actually
be negative!). It also varies based on platform. If the sites use size_t
instead of int, then 64-bit systems are typically hard to trigger in
practice (just because you'd need petabytes to store "a" and "b" in the
first place).

The only bug I have actually confirmed in practice here is fixed by
patch 2 (which is why it's at the front). There's another one in
path_name(), but that function is already dropped by the nearby
jk/lose-name-path topic.

The rest are cleanups of spots which _might_ be buggy, but I didn't dig
too far to find out. As with the earlier audit, I tried to refactor
using helpers that make the code clearer and less error-prone. So maybe
they're fixing bugs or not, but they certainly make it easier to audit
the result for problems.

  [01/18]: add helpers for detecting size_t overflow
  [02/18]: tree-diff: catch integer overflow in combine_diff_path allocation
  [03/18]: harden REALLOC_ARRAY and xcalloc against size_t overflow
  [04/18]: add helpers for allocating flex-array structs
  [05/18]: convert trivial cases to ALLOC_ARRAY
  [06/18]: use xmallocz to avoid size arithmetic
  [07/18]: convert trivial cases to FLEX_ARRAY macros
  [08/18]: use st_add and st_mult for allocation size computation
  [09/18]: write_untracked_extension: use FLEX_ALLOC helper
  [10/18]: fast-import: simplify allocation in start_packfile
  [11/18]: fetch-pack: simplify add_sought_entry
  [12/18]: test-path-utils: fix normalize_path_copy output buffer size
  [13/18]: sequencer: simplify memory allocation of get_message
  [14/18]: git-compat-util: drop mempcpy compat code
  [15/18]: transport_anonymize_url: use xstrfmt
  [16/18]: diff_populate_gitlink: use a strbuf
  [17/18]: convert ewah/bitmap code to use xmalloc
  [18/18]: ewah: convert to REALLOC_ARRAY, etc

-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

malloc memory corruption on merge-tree with leading newline

[Stefan Frühwirth]

Hi,

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.


Best,
Stefan

[1] https://gist.github.com/chrisrossi/f09c8bed70b364f9f12e
[2] https://github.com/Pylons/acidfs/issues/3

For in-line reference, here's the test case:

git init bug
cd bug
echo b > a
git add a
git commit -m "first commit"
echo > b
echo -n a >> b
git add b
git commit -m "second commit, first branch"
git checkout HEAD~1
git checkout -b other
echo -n a > b
git add b
git commit -m "second commit, second branch"
git merge-tree HEAD~1 master other
cd ..
rm -rf bug
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] http: add option to try authentication without username

[Eric Sunshine]

On Mon, Feb 15, 2016 at 4:39 PM, Junio C Hamano  wrote:
> "brian m. carlson"  writes:
>> On Mon, Feb 15, 2016 at 03:34:51PM -0500, Jeff King wrote:
>>> So I think this hack should remain purely at the curl level, and never
>>> touch the credential struct at all.
>>>
>>> Which is a shame, because I think Eric's suggestion is otherwise much
>>> more readable. :)
>>
>> Yes, I agree.  That would have been a much nicer and smaller change.
>
> Alright, reading all reviews and taking them into account, the
> original, when a Sign-off is added, would be acceptable, it seems.

One final question: Keeping in mind my lack of familiarity with this
particular use-case, would it be possible to infer the need to employ
this curl-specific workaround rather than making users tweak a config
setting? Or would that be a security risk or an otherwise stupid idea?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/18] add helpers for detecting size_t overflow

[Jeff King]

Performing computations on size_t variables that we feed to
xmalloc and friends can be dangerous, as an integer overflow
can cause us to allocate a much smaller chunk than we
realized.

We already have unsigned_add_overflows(), but let's add
unsigned_mult_overflows() to that. Furthermore, rather than
have each site manually check and die on overflow, we can
provide some helpers that will:

  - promote the arguments to size_t, so that we know we are
doing our computation in the same size of integer that
will ultimately be fed to xmalloc

  - check and die on overflow

  - return the result so that computations can be done in
the parameter list of xmalloc.

These functions are a lot uglier to use than normal
arithmetic operators (you have to do "st_add(foo, bar)"
instead of "foo + bar"). To at least limit the damage, we
also provide multi-valued versions. So rather than:

  st_add(st_add(a, b), st_add(c, d));

you can write:

  st_add4(a, b, c, d);

This isn't nearly as elegant as a varargs function, but it's
a lot harder to get it wrong. You don't have to remember to
add a sentinel value at the end, and the compiler will
complain if you get the number of arguments wrong. This
patch adds only the numbered variants required to convert
the current code base; we can easily add more later if
needed.

Signed-off-by: Jeff King 
---
The st_* names aren't amazing, but I think they mostly work. Suggestions
welcome, but please keep bikeshedding to a minimum. :)

I almost went with checked_add(), checked_mult(), etc. But these
inherently promote their arguments to size_t, so:

  int x = checked_add(a, b);

is buggy; the user _should_ be reminded of the result type in each call.

These could also build on compiler intrinsics for extra speed. I'm happy
to do that as a follow-up, but I doubt it really matters in practice.
We're about to call malloc() in all cases, so an extra integer
computation is almost certainly irrelevant. So I went for simplicity to
start with.

 git-compat-util.h | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 693a336..0c65033 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -96,6 +96,14 @@
 #define unsigned_add_overflows(a, b) \
 ((b) > maximum_unsigned_value_of_type(a) - (a))
 
+/*
+ * Returns true if the multiplication of "a" and "b" will
+ * overflow. The types of "a" and "b" must match and must be unsigned.
+ * Note that this macro evaluates "a" twice!
+ */
+#define unsigned_mult_overflows(a, b) \
+((a) && (b) > maximum_unsigned_value_of_type(a) / (a))
+
 #ifdef __GNUC__
 #define TYPEOF(x) (__typeof__(x))
 #else
@@ -713,6 +721,32 @@ extern void release_pack_memory(size_t);
 typedef void (*try_to_free_t)(size_t);
 extern try_to_free_t set_try_to_free_routine(try_to_free_t);
 
+static inline size_t st_add(size_t a, size_t b)
+{
+   if (unsigned_add_overflows(a, b))
+   die("size_t overflow: %"PRIuMAX" + %"PRIuMAX,
+   (uintmax_t)a, (uintmax_t)b);
+   return a + b;
+}
+#define st_add3(a,b,c)   st_add((a),st_add((b),(c)))
+#define st_add4(a,b,c,d) st_add((a),st_add3((b),(c),(d)))
+
+static inline size_t st_mult(size_t a, size_t b)
+{
+   if (unsigned_mult_overflows(a, b))
+   die("size_t overflow: %"PRIuMAX" * %"PRIuMAX,
+   (uintmax_t)a, (uintmax_t)b);
+   return a * b;
+}
+
+static inline size_t st_sub(size_t a, size_t b)
+{
+   if (a < b)
+   die("size_t underflow: %"PRIuMAX" - %"PRIuMAX,
+   (uintmax_t)a, (uintmax_t)b);
+   return a - b;
+}
+
 #ifdef HAVE_ALLOCA_H
 # include 
 # define xalloca(size)  (alloca(size))
-- 
2.7.1.572.gf718037

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

[PATCH 02/18] tree-diff: catch integer overflow in combine_diff_path allocation

[Jeff King]

A combine_diff_path struct has two "flex" members allocated
alongside the struct: a string to hold the pathname, and an
array of parent pointers. We use an "int" to compute this,
meaning we may easily overflow it if the pathname is
extremely long.

We can fix this by using size_t, and checking for overflow
with the st_add helper.

Signed-off-by: Jeff King 
---
 diff.h  | 4 ++--
 tree-diff.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/diff.h b/diff.h
index 70b2d70..beafbbd 100644
--- a/diff.h
+++ b/diff.h
@@ -222,8 +222,8 @@ struct combine_diff_path {
} parent[FLEX_ARRAY];
 };
 #define combine_diff_path_size(n, l) \
-   (sizeof(struct combine_diff_path) + \
-sizeof(struct combine_diff_parent) * (n) + (l) + 1)
+   st_add4(sizeof(struct combine_diff_path), (l), 1, \
+   st_mult(sizeof(struct combine_diff_parent), (n)))
 
 extern void show_combined_diff(struct combine_diff_path *elem, int num_parent,
  int dense, struct rev_info *);
diff --git a/tree-diff.c b/tree-diff.c
index 290a1da..4dda9a1 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -124,8 +124,8 @@ static struct combine_diff_path *path_appendnew(struct 
combine_diff_path *last,
unsigned mode, const unsigned char *sha1)
 {
struct combine_diff_path *p;
-   int len = base->len + pathlen;
-   int alloclen = combine_diff_path_size(nparent, len);
+   size_t len = st_add(base->len, pathlen);
+   size_t alloclen = combine_diff_path_size(nparent, len);
 
/* if last->next is !NULL - it is a pre-allocated memory, we can reuse 
*/
p = last->next;
-- 
2.7.1.572.gf718037

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

[PATCH 03/18] harden REALLOC_ARRAY and xcalloc against size_t overflow

[Jeff King]

REALLOC_ARRAY inherently involves a multiplication which can
overflow size_t, resulting in a much smaller buffer than we
think we've allocated. We can easily harden it by using
st_mult() to check for overflow.  Likewise, we can add
ALLOC_ARRAY to do the same thing for xmalloc calls.

xcalloc() should already be fine, because it takes the two
factors separately, assuming the system calloc actually
checks for overflow. However, before we even hit the system
calloc(), we do our memory_limit_check, which involves a
multiplication. Let's check for overflow ourselves so that
this limit cannot be bypassed.

Signed-off-by: Jeff King 
---
 git-compat-util.h | 3 ++-
 wrapper.c | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 0c65033..55c073d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -779,7 +779,8 @@ extern int odb_pack_keep(char *name, size_t namesz, const 
unsigned char *sha1);
 extern char *xgetcwd(void);
 extern FILE *fopen_for_writing(const char *path);
 
-#define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))
+#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
 
 static inline char *xstrdup_or_null(const char *str)
 {
diff --git a/wrapper.c b/wrapper.c
index 29a45d2..9afc1a0 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -152,6 +152,9 @@ void *xcalloc(size_t nmemb, size_t size)
 {
void *ret;
 
+   if (unsigned_mult_overflows(nmemb, size))
+   die("data too large to fit into virtual memory space");
+
memory_limit_check(size * nmemb, 0);
ret = calloc(nmemb, size);
if (!ret && (!nmemb || !size))
-- 
2.7.1.572.gf718037

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

[PATCH 04/18] add helpers for allocating flex-array structs

[Jeff King]

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
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..8f23801 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 repoined 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.572.gf718037

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

[PATCH 05/18] convert trivial cases to ALLOC_ARRAY

[Jeff King]

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 
---
 alias.c  |  2 +-
 attr.c   |  2 +-
 bisect.c |  4 ++--
 builtin/blame.c  |  3 ++-
 builtin/clean.c  |  2 +-
 builtin/fast-export.c|  2 +-
 builtin/grep.c   |  3 ++-
 builtin/index-pack.c |  4 ++--
 builtin/merge-base.c |  2 +-
 builtin/mv.c |  3 ++-
 builtin/pack-objects.c   |  7 ---
 builtin/pack-redundant.c |  2 +-
 builtin/receive-pack.c   |  7 +++
 builtin/remote-ext.c |  2 +-
 column.c |  2 +-
 combine-diff.c   |  4 ++--
 commit.c |  2 +-
 compat/mingw.c   |  6 +++---
 daemon.c |  2 +-
 diffcore-order.c |  4 ++--
 dir.c|  6 +++---
 exec_cmd.c   |  2 +-
 fast-import.c|  5 +++--
 fsck.c   |  3 ++-
 git.c|  2 +-
 graph.c  | 10 --
 khash.h  |  2 +-
 levenshtein.c|  8 +---
 line-log.c   | 10 +-
 notes.c  |  2 +-
 pack-check.c |  2 +-
 pack-revindex.c  | 12 
 pathspec.c   |  5 +++--
 remote-curl.c|  6 --
 run-command.c|  2 +-
 sha1_file.c  |  4 ++--
 shallow.c|  6 +++---
 show-index.c |  3 ++-
 transport.c  |  2 +-
 xdiff-interface.c|  2 +-
 40 files changed, 86 insertions(+), 73 deletions(-)

diff --git a/alias.c b/alias.c
index a11229d..3b90397 100644
--- a/alias.c
+++ b/alias.c
@@ -23,7 +23,7 @@ int split_cmdline(char *cmdline, const char ***argv)
int src, dst, count = 0, size = 16;
char quoted = 0;
 
-   *argv = xmalloc(sizeof(**argv) * size);
+   ALLOC_ARRAY(*argv, size);
 
/* split alias_string */
(*argv)[count++] = cmdline;
diff --git a/attr.c b/attr.c
index 086c08d..c83ec49 100644
--- a/attr.c
+++ b/attr.c
@@ -799,7 +799,7 @@ int git_all_attrs(const char *path, int *num, struct 
git_attr_check **check)
++count;
}
*num = count;
-   *check = xmalloc(sizeof(**check) * count);
+   ALLOC_ARRAY(*check, count);
j = 0;
for (i = 0; i < attr_nr; i++) {
const char *value = check_all_attr[i].value;
diff --git a/bisect.c b/bisect.c
index 06ec54e..7996c29 100644
--- a/bisect.c
+++ b/bisect.c
@@ -708,10 +708,10 @@ static struct commit *get_commit_reference(const unsigned 
char *sha1)
 
 static struct commit **get_bad_and_good_commits(int *rev_nr)
 {
-   int len = 1 + good_revs.nr;
-   struct commit **rev = xmalloc(len * sizeof(*rev));
+   struct commit **rev;
int i, n = 0;
 
+   ALLOC_ARRAY(rev, 1 + good_revs.nr);
rev[n++] = get_commit_reference(current_bad_oid->hash);
for (i = 0; i < good_revs.nr; i++)
rev[n++] = get_commit_reference(good_revs.sha1[i]);
diff --git a/builtin/blame.c b/builtin/blame.c
index 55bf5fa..b4ed462 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2059,7 +2059,8 @@ static int prepare_lines(struct scoreboard *sb)
for (p = buf; p < end; p = get_next_line(p, end))
num++;
 
-   sb->lineno = lineno = xmalloc(sizeof(*sb->lineno) * (num + 1));
+   ALLOC_ARRAY(sb->lineno, num + 1);
+   lineno = sb->lineno;
 
for (p = buf; p < end; p = get_next_line(p, end))
*lineno++ = p - buf;
diff --git a/builtin/clean.c b/builtin/clean.c
index 7b08237..8229f7e 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -543,7 +543,7 @@ static int *list_and_choose(struct menu_opts *opts, struct 
menu_stuff *stuff)
int eof = 0;
int i;
 
-   chosen = xmalloc(sizeof(int) * stuff->nr);
+   ALLOC_ARRAY(chosen, stuff->nr);
/* set chosen as uninitialized */
for (i = 0; i < stuff->nr; i++)
chosen[i] = -1;
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 2471297..8164b58 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -1021,7 +1021,7 @@ int cmd_fast_export(int argc, const char **argv, const 
char *prefix)
const char **refspecs_str;
int i;
 
-   refspecs_str = xmalloc(sizeof(*refspecs_str) * 
refspecs_list.nr);
+   ALLOC_ARRAY(refspecs_str, refspecs_list.nr);
for (i = 0; i < refspecs_list.nr; i++)
refspecs_str[i] = refspecs_list.items[i].string;
 
diff --git a/builtin/grep.c b/builtin/grep.c
index 8c516a9..fa9c9ef 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -365,9 

Re: [PATCH] http: add option to try authentication without username

[brian m. carlson]

On Mon, Feb 15, 2016 at 04:46:43PM -0500, Eric Sunshine wrote:
> On Mon, Feb 15, 2016 at 4:39 PM, Junio C Hamano  wrote:
> > "brian m. carlson"  writes:
> >> On Mon, Feb 15, 2016 at 03:34:51PM -0500, Jeff King wrote:
> >>> So I think this hack should remain purely at the curl level, and never
> >>> touch the credential struct at all.
> >>>
> >>> Which is a shame, because I think Eric's suggestion is otherwise much
> >>> more readable. :)
> >>
> >> Yes, I agree.  That would have been a much nicer and smaller change.
> >
> > Alright, reading all reviews and taking them into account, the
> > original, when a Sign-off is added, would be acceptable, it seems.
> 
> One final question: Keeping in mind my lack of familiarity with this
> particular use-case, would it be possible to infer the need to employ
> this curl-specific workaround rather than making users tweak a config
> setting? Or would that be a security risk or an otherwise stupid idea?

It's not very easy to infer whether it's needed.  We'd need to know what
types of authentication are offered, and somehow we'd have to intuit
proper behavior when both GSS-Negotiate and Basic are enabled.  Some
sites do that because you can use Basic against the Kerberos database.
One user might legitimately want to always use Basic (e.g. with a
password manager) and another might always want to use Negotiate.
Setting this option is one way to ensure the latter.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

[PATCH 07/18] convert trivial cases to FLEX_ARRAY macros

[Jeff King]

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 
---
 attr.c   |  4 +---
 builtin/blame.c  |  4 +---
 builtin/help.c   |  9 +++--
 builtin/mktree.c |  9 +
 builtin/reflog.c |  7 ++-
 cache-tree.c |  4 +---
 combine-diff.c   |  4 +---
 diff.c   |  7 ++-
 dir.c| 16 +++-
 hashmap.c|  3 +--
 help.c   |  6 ++
 log-tree.c   |  5 ++---
 name-hash.c  |  3 +--
 ref-filter.c |  6 ++
 refs.c   |  6 ++
 refs/files-backend.c | 19 +--
 remote.c |  5 +
 17 files changed, 35 insertions(+), 82 deletions(-)

diff --git a/attr.c b/attr.c
index c83ec49..6537a43 100644
--- a/attr.c
+++ b/attr.c
@@ -93,9 +93,7 @@ static struct git_attr *git_attr_internal(const char *name, 
int len)
if (invalid_attr_name(name, len))
return NULL;
 
-   a = xmalloc(sizeof(*a) + len + 1);
-   memcpy(a->name, name, len);
-   a->name[len] = 0;
+   FLEX_ALLOC_MEM(a, name, name, len);
a->h = hval;
a->next = git_attr_hash[pos];
a->attr_nr = attr_nr++;
diff --git a/builtin/blame.c b/builtin/blame.c
index b4ed462..e982fb8 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -466,13 +466,11 @@ static void queue_blames(struct scoreboard *sb, struct 
origin *porigin,
 static struct origin *make_origin(struct commit *commit, const char *path)
 {
struct origin *o;
-   size_t pathlen = strlen(path) + 1;
-   o = xcalloc(1, sizeof(*o) + pathlen);
+   FLEX_ALLOC_STR(o, path, path);
o->commit = commit;
o->refcnt = 1;
o->next = commit->util;
commit->util = o;
-   memcpy(o->path, path, pathlen); /* includes NUL */
return o;
 }
 
diff --git a/builtin/help.c b/builtin/help.c
index 1cd0c1e..3c55ce4 100644
--- a/builtin/help.c
+++ b/builtin/help.c
@@ -171,12 +171,10 @@ static void exec_man_cmd(const char *cmd, const char 
*page)
 static void add_man_viewer(const char *name)
 {
struct man_viewer_list **p = _viewer_list;
-   size_t len = strlen(name);
 
while (*p)
p = &((*p)->next);
-   *p = xcalloc(1, (sizeof(**p) + len + 1));
-   memcpy((*p)->name, name, len); /* NUL-terminated by xcalloc */
+   FLEX_ALLOC_STR(*p, name, name);
 }
 
 static int supported_man_viewer(const char *name, size_t len)
@@ -190,9 +188,8 @@ static void do_add_man_viewer_info(const char *name,
   size_t len,
   const char *value)
 {
-   struct man_viewer_info_list *new = xcalloc(1, sizeof(*new) + len + 1);
-
-   memcpy(new->name, name, len); /* NUL-terminated by xcalloc */
+   struct man_viewer_info_list *new;
+   FLEX_ALLOC_MEM(new, name, name, len);
new->info = xstrdup(value);
new->next = man_viewer_info_list;
man_viewer_info_list = new;
diff --git a/builtin/mktree.c b/builtin/mktree.c
index a237caa..4282b62 100644
--- a/builtin/mktree.c
+++ b/builtin/mktree.c
@@ -19,16 +19,17 @@ static int alloc, used;
 static void append_to_tree(unsigned mode, unsigned char *sha1, char *path)
 {
struct treeent *ent;
-   int len = strlen(path);
+   size_t len = strlen(path);
if (strchr(path, '/'))
die("path %s contains slash", path);
 
-   ALLOC_GROW(entries, used + 1, alloc);
-   ent = entries[used++] = xmalloc(sizeof(**entries) + len + 1);
+   FLEX_ALLOC_MEM(ent, name, path, len);
ent->mode = mode;
ent->len = len;
hashcpy(ent->sha1, sha1);
-   memcpy(ent->name, path, len+1);
+
+   ALLOC_GROW(entries, used + 1, alloc);
+   entries[used++] = ent;
 }
 
 static int ent_compare(const void *a_, const void *b_)
diff --git a/builtin/reflog.c b/builtin/reflog.c
index f39960e..7c1990f 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -382,11 +382,9 @@ static int collect_reflog(const char *ref, const struct 
object_id *oid, int unus
 {
struct collected_reflog *e;
struct collect_reflog_cb *cb = cb_data;
-   size_t namelen = strlen(ref);
 
-   e = xmalloc(sizeof(*e) + namelen + 1);
+   FLEX_ALLOC_STR(e, reflog, ref);
hashcpy(e->sha1, oid->hash);
-   memcpy(e->reflog, ref, namelen + 1);
ALLOC_GROW(cb->e, cb->nr + 1, cb->alloc);
cb->e[cb->nr++] = e;
return 0;
@@ -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);

[PATCH 06/18] use xmallocz to avoid size arithmetic

[Jeff King]

We frequently allocate strings as xmalloc(len + 1), where
the extra 1 is for the NUL terminator. This can be done more
simply with xmallocz, which also checks for integer
overflow.

There's no case where switching xmalloc(n+1) to xmallocz(n)
is wrong; the result is the same length, and malloc made no
guarantees about what was in the buffer anyway. But in some
cases, we can stop manually placing NUL at the end of the
allocated buffer. But that's only safe if it's clear that
the contents will always fill the buffer.

In each case where this patch does so, I manually examined
the control flow, and I tried to err on the side of caution.

Signed-off-by: Jeff King 
---
 builtin/check-ref-format.c | 2 +-
 builtin/merge-tree.c   | 2 +-
 builtin/worktree.c | 2 +-
 column.c   | 3 +--
 combine-diff.c | 4 +---
 config.c   | 4 +---
 dir.c  | 2 +-
 entry.c| 2 +-
 grep.c | 3 +--
 imap-send.c| 5 ++---
 ll-merge.c | 2 +-
 progress.c | 2 +-
 refs.c | 2 +-
 setup.c| 5 ++---
 strbuf.c   | 2 +-
 15 files changed, 17 insertions(+), 25 deletions(-)

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index fd915d5..eac4994 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -20,7 +20,7 @@ static const char builtin_check_ref_format_usage[] =
  */
 static char *collapse_slashes(const char *refname)
 {
-   char *ret = xmalloc(strlen(refname) + 1);
+   char *ret = xmallocz(strlen(refname));
char ch;
char prev = '/';
char *cp = ret;
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index d4f0cbd..ca57004 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -174,7 +174,7 @@ static struct merge_list *create_entry(unsigned stage, 
unsigned mode, const unsi
 
 static char *traverse_path(const struct traverse_info *info, const struct 
name_entry *n)
 {
-   char *path = xmalloc(traverse_path_len(info, n) + 1);
+   char *path = xmallocz(traverse_path_len(info, n));
return make_traverse_path(path, info, n);
 }
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 475b958..0a45710 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -52,7 +52,7 @@ static int prune_worktree(const char *id, struct strbuf 
*reason)
return 1;
}
len = st.st_size;
-   path = xmalloc(len + 1);
+   path = xmallocz(len);
read_in_full(fd, path, len);
close(fd);
while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
diff --git a/column.c b/column.c
index f9fda68..d55ead1 100644
--- a/column.c
+++ b/column.c
@@ -173,9 +173,8 @@ static void display_table(const struct string_list *list,
if (colopts & COL_DENSE)
shrink_columns();
 
-   empty_cell = xmalloc(initial_width + 1);
+   empty_cell = xmallocz(initial_width);
memset(empty_cell, ' ', initial_width);
-   empty_cell[initial_width] = '\0';
for (y = 0; y < data.rows; y++) {
for (x = 0; x < data.cols; x++)
if (display_cell(, initial_width, empty_cell, x, 
y))
diff --git a/combine-diff.c b/combine-diff.c
index a698016..890c415 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1043,7 +1043,7 @@ static void show_patch_diff(struct combine_diff_path 
*elem, int num_parent,
elem->mode = canon_mode(S_IFLNK);
 
result_size = len;
-   result = xmalloc(len + 1);
+   result = xmallocz(len);
 
done = read_in_full(fd, result, len);
if (done < 0)
@@ -1051,8 +1051,6 @@ static void show_patch_diff(struct combine_diff_path 
*elem, int num_parent,
else if (done < len)
die("early EOF '%s'", elem->path);
 
-   result[len] = 0;
-
/* If not a fake symlink, apply filters, e.g. autocrlf 
*/
if (is_file) {
struct strbuf buf = STRBUF_INIT;
diff --git a/config.c b/config.c
index b95ac3a..e7b589a 100644
--- a/config.c
+++ b/config.c
@@ -1902,7 +1902,7 @@ static int git_config_parse_key_1(const char *key, char 
**store_key, int *basele
 * Validate the key and while at it, lower case it for matching.
 */
if (store_key)
-   *store_key = xmalloc(strlen(key) + 1);
+   *store_key = xmallocz(strlen(key));
 
dot = 0;
for (i = 0; key[i]; i++) {
@@ -1926,8 +1926,6 @@ static int git_config_parse_key_1(const char *key, char 
**store_key, int *basele
if (store_key)
(*store_key)[i] = c;
}
-   if (store_key)
-   (*store_key)[i] = 

[PATCH 08/18] use st_add and st_mult for allocation size computation

[Jeff King]

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 
---
 archive.c  |  4 ++--
 builtin/apply.c|  2 +-
 builtin/clean.c|  2 +-
 builtin/fetch.c|  2 +-
 builtin/index-pack.c   |  4 ++--
 builtin/merge.c|  2 +-
 builtin/mv.c   |  4 ++--
 builtin/receive-pack.c |  2 +-
 combine-diff.c | 14 +++---
 commit.c   |  2 +-
 compat/mingw.c |  4 ++--
 compat/qsort.c |  2 +-
 compat/setenv.c|  2 +-
 compat/win32/syslog.c  |  4 ++--
 diffcore-delta.c   |  6 --
 diffcore-rename.c  |  2 +-
 dir.c  |  4 ++--
 fast-import.c  |  2 +-
 refs.c |  2 +-
 remote.c   |  8 
 revision.c |  2 +-
 sha1_file.c| 20 +++-
 sha1_name.c|  5 ++---
 shallow.c  |  2 +-
 submodule.c|  6 +++---
 25 files changed, 56 insertions(+), 53 deletions(-)

diff --git a/archive.c b/archive.c
index 0687afa..5d735ae 100644
--- a/archive.c
+++ b/archive.c
@@ -171,8 +171,8 @@ static void queue_directory(const unsigned char *sha1,
unsigned mode, int stage, struct archiver_context *c)
 {
struct directory *d;
-   size_t len = base->len + 1 + strlen(filename) + 1;
-   d = xmalloc(sizeof(*d) + len);
+   size_t len = st_add4(base->len, 1, strlen(filename), 1);
+   d = xmalloc(st_add(sizeof(*d), len));
d->up  = c->bottom;
d->baselen = base->len;
d->mode= mode;
diff --git a/builtin/apply.c b/builtin/apply.c
index d61ac65..42c610e 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -2632,7 +2632,7 @@ static void update_image(struct image *img,
insert_count = postimage->len;
 
/* Adjust the contents */
-   result = xmalloc(img->len + insert_count - remove_count + 1);
+   result = xmalloc(st_add3(st_sub(img->len, remove_count), insert_count, 
1));
memcpy(result, img->buf, applied_at);
memcpy(result + applied_at, postimage->buf, postimage->len);
memcpy(result + applied_at + postimage->len,
diff --git a/builtin/clean.c b/builtin/clean.c
index 8229f7e..0371010 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -615,7 +615,7 @@ static int *list_and_choose(struct menu_opts *opts, struct 
menu_stuff *stuff)
nr += chosen[i];
}
 
-   result = xcalloc(nr + 1, sizeof(int));
+   result = xcalloc(st_add(nr, 1), sizeof(int));
for (i = 0; i < stuff->nr && j < nr; i++) {
if (chosen[i])
result[j++] = i;
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8e74213..373a89d 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1110,7 +1110,7 @@ static int fetch_one(struct remote *remote, int argc, 
const char **argv)
if (argc > 0) {
int j = 0;
int i;
-   refs = xcalloc(argc + 1, sizeof(const char *));
+   refs = xcalloc(st_add(argc, 1), sizeof(const char *));
for (i = 0; i < argc; i++) {
if (!strcmp(argv[i], "tag")) {
i++;
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a60bcfa..193908a 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1744,9 +1744,9 @@ int cmd_index_pack(int argc, const char **argv, const 
char *prefix)
 
curr_pack = open_pack_file(pack_name);
parse_pack_header();
-   objects = xcalloc(nr_objects + 1, sizeof(struct object_entry));
+   objects = xcalloc(st_add(nr_objects, 1), sizeof(struct object_entry));
if (show_stat)
-   obj_stat = xcalloc(nr_objects + 1, sizeof(struct object_stat));
+   obj_stat = xcalloc(st_add(nr_objects, 1), sizeof(struct 
object_stat));
ofs_deltas = xcalloc(nr_objects, sizeof(struct ofs_delta_entry));
parse_pack_objects(pack_sha1);
resolve_deltas();
diff --git a/builtin/merge.c b/builtin/merge.c
index b98a348..101ffef 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -939,7 +939,7 @@ static int setup_with_upstream(const char ***argv)
if (!branch->merge_nr)
die(_("No default upstream defined for the current branch."));
 
-   args = xcalloc(branch->merge_nr + 1, sizeof(char *));
+   args = xcalloc(st_add(branch->merge_nr, 1), sizeof(char *));
for (i = 0; i < branch->merge_nr; i++) {
if (!branch->merge[i]->dst)
die(_("No remote-tracking branch for %s from %s"),

[PATCH 09/18] write_untracked_extension: use FLEX_ALLOC helper

[Jeff King]

We perform unchecked additions when computing the size of a
"struct ondisk_untracked_cache". This is unlikely to have an
integer overflow in practice, but we'd like to avoid this
dangerous pattern to make further audits easier.

Note that there's one subtlety here, though.  We protect
ourselves against a NULL exclude_per_dir entry in our
source, and avoid calling strlen() on it, keeping "len" at
0. But later, we unconditionally memcpy "len + 1" bytes to
get the trailing NUL byte. If we did have a NULL
exclude_per_dir, we would read from bogus memory.

As it turns out, though, we always create this field
pointing to a string literal, so there's no bug. We can just
get rid of the pointless extra conditional.

Signed-off-by: Jeff King 
---
 dir.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/dir.c b/dir.c
index 2c91541..a4a9d9f 100644
--- a/dir.c
+++ b/dir.c
@@ -2360,16 +2360,15 @@ void write_untracked_extension(struct strbuf *out, 
struct untracked_cache *untra
struct ondisk_untracked_cache *ouc;
struct write_data wd;
unsigned char varbuf[16];
-   int len = 0, varint_len;
-   if (untracked->exclude_per_dir)
-   len = strlen(untracked->exclude_per_dir);
-   ouc = xmalloc(sizeof(*ouc) + len + 1);
+   int varint_len;
+   size_t len = strlen(untracked->exclude_per_dir);
+
+   FLEX_ALLOC_MEM(ouc, exclude_per_dir, untracked->exclude_per_dir, len);
stat_data_to_disk(>info_exclude_stat, 
>ss_info_exclude.stat);
stat_data_to_disk(>excludes_file_stat, 
>ss_excludes_file.stat);
hashcpy(ouc->info_exclude_sha1, untracked->ss_info_exclude.sha1);
hashcpy(ouc->excludes_file_sha1, untracked->ss_excludes_file.sha1);
ouc->dir_flags = htonl(untracked->dir_flags);
-   memcpy(ouc->exclude_per_dir, untracked->exclude_per_dir, len + 1);
 
varint_len = encode_varint(untracked->ident.len, varbuf);
strbuf_add(out, varbuf, varint_len);
-- 
2.7.1.572.gf718037

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

[PATCH 10/18] fast-import: simplify allocation in start_packfile

[Jeff King]

This function allocates a packed_git flex-array, and adds a
mysterious 2 bytes to the length of the pack_name field. One
is for the trailing NUL, but the other has no purpose. This
is probably cargo-culted from add_packed_git, which gets the
".idx" path and needs to allocate enough space to hold the
matching ".pack" (though since 48bcc1c, we calculate the
size there differently).

This site, however, is using the raw path of a tempfile, and
does not need the extra byte. We can just replace the
allocation with FLEX_ALLOC_STR, which handles the allocation
and the NUL for us.

Signed-off-by: Jeff King 
---
 fast-import.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 3053bb8..9fc7093 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -865,15 +865,12 @@ static void start_packfile(void)
 {
static char tmp_file[PATH_MAX];
struct packed_git *p;
-   int namelen;
struct pack_header hdr;
int pack_fd;
 
pack_fd = odb_mkstemp(tmp_file, sizeof(tmp_file),
  "pack/tmp_pack_XX");
-   namelen = strlen(tmp_file) + 2;
-   p = xcalloc(1, sizeof(*p) + namelen);
-   xsnprintf(p->pack_name, namelen, "%s", tmp_file);
+   FLEX_ALLOC_STR(p, pack_name, tmp_file);
p->pack_fd = pack_fd;
p->do_not_close = 1;
pack_file = sha1fd(pack_fd, p->pack_name);
-- 
2.7.1.572.gf718037

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

[PATCH 11/18] fetch-pack: simplify add_sought_entry

[Jeff King]

We have two variants of this function, one that takes a
string and one that takes a ptr/len combo. But we only call
the latter with the length of a NUL-terminated string, so
our first simplification is to drop it in favor of the
string variant.

Since we know we have a string, we can also replace the
manual memory computation with a call to alloc_ref().

Furthermore, we can rely on get_oid_hex() to complain if it
hits the end of the string. That means we can simplify the
check for " " versus just "". Rather than
manage the ptr/len pair, we can just bump the start of our
string forward. The original code over-allocated based on
the original "namelen" (which wasn't _wrong_, but was simply
wasteful and confusing).

Signed-off-by: Jeff King 
---
 builtin/fetch-pack.c | 27 +--
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 9b2a514..79a611f 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -10,33 +10,24 @@ static const char fetch_pack_usage[] =
 "[--include-tag] [--upload-pack=] [--depth=] "
 "[--no-progress] [--diag-url] [-v] [:] [...]";
 
-static void add_sought_entry_mem(struct ref ***sought, int *nr, int *alloc,
-const char *name, int namelen)
+static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,
+const char *name)
 {
-   struct ref *ref = xcalloc(1, sizeof(*ref) + namelen + 1);
+   struct ref *ref;
struct object_id oid;
-   const int chunksz = GIT_SHA1_HEXSZ + 1;
 
-   if (namelen > chunksz && name[chunksz - 1] == ' ' &&
-   !get_oid_hex(name, )) {
-   oidcpy(>old_oid, );
-   name += chunksz;
-   namelen -= chunksz;
-   }
+   if (!get_oid_hex(name, ) && name[GIT_SHA1_HEXSZ] == ' ')
+   name += GIT_SHA1_HEXSZ + 1;
+   else
+   oidclr();
 
-   memcpy(ref->name, name, namelen);
-   ref->name[namelen] = '\0';
+   ref = alloc_ref(name);
+   oidcpy(>old_oid, );
(*nr)++;
ALLOC_GROW(*sought, *nr, *alloc);
(*sought)[*nr - 1] = ref;
 }
 
-static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,
-const char *string)
-{
-   add_sought_entry_mem(sought, nr, alloc, string, strlen(string));
-}
-
 int cmd_fetch_pack(int argc, const char **argv, const char *prefix)
 {
int i, ret;
-- 
2.7.1.572.gf718037

--
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: malloc memory corruption on merge-tree with leading newline

[Stefan Frühwirth]

Addendum: Problem occurs with version 2.7.1 as well as version 1.9.1.

On 15/02/16 22:39, Stefan Frühwirth wrote:


in one specific circumstance, git-merge-tree exits with a segfault
caused by "*** Error in `git': malloc(): memory corruption (fast)":


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

[PATCH 12/18] test-path-utils: fix normalize_path_copy output buffer size

[Jeff King]

The normalize_path_copy function needs an output buffer that
is at least as long as its input (it may shrink the path,
but never expand it). However, this test program feeds it
static PATH_MAX-sized buffers, which have no relation to the
input size.

In the normalize_ceiling_entry case, we do at least check
the size against PATH_MAX and die(), but that case is even
more convoluted. We normalize into a fixed-size buffer, free
the original, and then replace it with a strdup'd copy of
the result. But normalize_path_copy explicitly allows
normalizing in-place, so we can simply do that.

Signed-off-by: Jeff King 
---
 test-path-utils.c | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/test-path-utils.c b/test-path-utils.c
index c3adcd8..0c15f18 100644
--- a/test-path-utils.c
+++ b/test-path-utils.c
@@ -8,21 +8,14 @@
  */
 static int normalize_ceiling_entry(struct string_list_item *item, void *unused)
 {
-   const char *ceil = item->string;
-   int len = strlen(ceil);
-   char buf[PATH_MAX+1];
+   char *ceil = item->string;
 
-   if (len == 0)
+   if (!*ceil)
die("Empty path is not supported");
-   if (len > PATH_MAX)
-   die("Path \"%s\" is too long", ceil);
if (!is_absolute_path(ceil))
die("Path \"%s\" is not absolute", ceil);
-   if (normalize_path_copy(buf, ceil) < 0)
+   if (normalize_path_copy(ceil, ceil) < 0)
die("Path \"%s\" could not be normalized", ceil);
-   len = strlen(buf);
-   free(item->string);
-   item->string = xstrdup(buf);
return 1;
 }
 
@@ -166,7 +159,7 @@ static struct test_data dirname_data[] = {
 int main(int argc, char **argv)
 {
if (argc == 3 && !strcmp(argv[1], "normalize_path_copy")) {
-   char *buf = xmalloc(PATH_MAX + 1);
+   char *buf = xmallocz(strlen(argv[2]));
int rv = normalize_path_copy(buf, argv[2]);
if (rv)
buf = "++failed++";
-- 
2.7.1.572.gf718037

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

[PATCH 13/18] sequencer: simplify memory allocation of get_message

[Jeff King]

For a commit with has "1234abcd" and subject "foo", this
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 
---
 sequencer.c | 29 ++---
 1 file changed, 10 insertions(+), 19 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8048786..e66f2fe 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -124,42 +124,33 @@ static const char *action_name(const struct replay_opts 
*opts)
 
 struct commit_message {
char *parent_label;
-   const char *label;
-   const char *subject;
+   char *label;
+   char *subject;
const char *message;
 };
 
 static int get_message(struct commit *commit, struct commit_message *out)
 {
const char *abbrev, *subject;
-   int abbrev_len, subject_len;
-   char *q;
-
-   if (!git_commit_encoding)
-   git_commit_encoding = "UTF-8";
+   int subject_len;
 
-   out->message = logmsg_reencode(commit, NULL, git_commit_encoding);
+   out->message = logmsg_reencode(commit, NULL, 
get_commit_output_encoding());
abbrev = find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV);
-   abbrev_len = strlen(abbrev);
 
subject_len = find_commit_subject(out->message, );
 
-   out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
- strlen("... ") + subject_len + 1);
-   q = out->parent_label;
-   q = mempcpy(q, "parent of ", strlen("parent of "));
-   out->label = q;
-   q = mempcpy(q, abbrev, abbrev_len);
-   q = mempcpy(q, "... ", strlen("... "));
-   out->subject = q;
-   q = mempcpy(q, subject, subject_len);
-   *q = '\0';
+   out->subject = xmemdupz(subject, subject_len);
+   out->label = xstrfmt("%s... %s", abbrev, out->subject);
+   out->parent_label = xstrfmt("parent of %s", out->label);
+
return 0;
 }
 
 static void free_message(struct commit *commit, struct commit_message *msg)
 {
free(msg->parent_label);
+   free(msg->label);
+   free(msg->subject);
unuse_commit_buffer(commit, msg->message);
 }
 
-- 
2.7.1.572.gf718037

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

[PATCH 14/18] git-compat-util: drop mempcpy compat code

[Jeff King]

There are no callers of this left, as the last one was
dropped in the previous patch. And there are no likely to be
new ones, as the function has been around since 2010 without
gaining any new callers.

Signed-off-by: Jeff King 
---
 git-compat-util.h | 9 -
 1 file changed, 9 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 8f23801..6892e72 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -681,7 +681,6 @@ extern int git_vsnprintf(char *str, size_t maxsize,
 #ifdef __GLIBC_PREREQ
 #if __GLIBC_PREREQ(2, 1)
 #define HAVE_STRCHRNUL
-#define HAVE_MEMPCPY
 #endif
 #endif
 
@@ -695,14 +694,6 @@ static inline char *gitstrchrnul(const char *s, int c)
 }
 #endif
 
-#ifndef HAVE_MEMPCPY
-#define mempcpy gitmempcpy
-static inline void *gitmempcpy(void *dest, const void *src, size_t n)
-{
-   return (char *)memcpy(dest, src, n) + n;
-}
-#endif
-
 #ifdef NO_INET_PTON
 int inet_pton(int af, const char *src, void *dst);
 #endif
-- 
2.7.1.572.gf718037

--
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 16/18] diff_populate_gitlink: use a strbuf

[Jeff King]

We allocate 100 bytes to hold the "Submodule commit ..."
text. This is enough, but it's not immediately obvious that
this is the case, and we have to repeat the magic 100 twice.

We could get away with xstrfmt here, but we want to know the
size, as well, so let's use a real strbuf. And while we're
here, we can clean up the logic around size_only. It
currently sets and clears the "data" field pointlessly, and
leaves the "should_free" flag on even after we have cleared
the data.

Signed-off-by: Jeff King 
---
 diff.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/diff.c b/diff.c
index 27d14a7..a70ec6e 100644
--- a/diff.c
+++ b/diff.c
@@ -2704,21 +2704,21 @@ static int reuse_worktree_file(const char *name, const 
unsigned char *sha1, int
 
 static int diff_populate_gitlink(struct diff_filespec *s, int size_only)
 {
-   int len;
-   char *data = xmalloc(100), *dirty = "";
+   struct strbuf buf = STRBUF_INIT;
+   char *dirty = "";
 
/* Are we looking at the work tree? */
if (s->dirty_submodule)
dirty = "-dirty";
 
-   len = snprintf(data, 100,
-  "Subproject commit %s%s\n", sha1_to_hex(s->sha1), dirty);
-   s->data = data;
-   s->size = len;
-   s->should_free = 1;
+   strbuf_addf(, "Subproject commit %s%s\n", sha1_to_hex(s->sha1), 
dirty);
+   s->size = buf.len;
if (size_only) {
s->data = NULL;
-   free(data);
+   strbuf_release();
+   } else {
+   s->data = strbuf_detach(, NULL);
+   s->should_free = 1;
}
return 0;
 }
-- 
2.7.1.572.gf718037

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

[PATCH 15/18] transport_anonymize_url: use xstrfmt

[Jeff King]

This function uses xcalloc and two memcpy calls to
concatenate two strings. We can do this as an xstrfmt
one-liner, and then it is more clear that we are allocating
the correct amount of memory.

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

diff --git a/transport.c b/transport.c
index 5c63295..d53e4aa 100644
--- a/transport.c
+++ b/transport.c
@@ -1351,7 +1351,7 @@ int transport_disconnect(struct transport *transport)
  */
 char *transport_anonymize_url(const char *url)
 {
-   char *anon_url, *scheme_prefix, *anon_part;
+   char *scheme_prefix, *anon_part;
size_t anon_len, prefix_len = 0;
 
anon_part = strchr(url, '@');
@@ -1385,10 +1385,8 @@ char *transport_anonymize_url(const char *url)
goto literal_copy;
prefix_len = scheme_prefix - url + 3;
}
-   anon_url = xcalloc(1, 1 + prefix_len + anon_len);
-   memcpy(anon_url, url, prefix_len);
-   memcpy(anon_url + prefix_len, anon_part, anon_len);
-   return anon_url;
+   return xstrfmt("%.*s%.*s", (int)prefix_len, url,
+  (int)anon_len, anon_part);
 literal_copy:
return xstrdup(url);
 }
-- 
2.7.1.572.gf718037

--
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 18/18] ewah: convert to REALLOC_ARRAY, etc

[Jeff King]

Now that we're built around xmalloc and friends, we can use
helpers like REALLOC_ARRAY, ALLOC_GROW, and so on to make
the code shorter and protect against integer overflow.

Signed-off-by: Jeff King 
---
 ewah/bitmap.c  | 16 
 ewah/ewah_bitmap.c |  5 ++---
 ewah/ewah_io.c |  6 ++
 3 files changed, 8 insertions(+), 19 deletions(-)

diff --git a/ewah/bitmap.c b/ewah/bitmap.c
index c88daa0..7103cee 100644
--- a/ewah/bitmap.c
+++ b/ewah/bitmap.c
@@ -17,7 +17,7 @@
  * along with this program; if not, write to the Free Software
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, 
USA.
  */
-#include "git-compat-util.h"
+#include "cache.h"
 #include "ewok.h"
 
 #define EWAH_MASK(x) ((eword_t)1 << (x % BITS_IN_EWORD))
@@ -38,9 +38,7 @@ void bitmap_set(struct bitmap *self, size_t pos)
if (block >= self->word_alloc) {
size_t old_size = self->word_alloc;
self->word_alloc = block * 2;
-   self->words = xrealloc(self->words,
- self->word_alloc * sizeof(eword_t));
-
+   REALLOC_ARRAY(self->words, self->word_alloc);
memset(self->words + old_size, 0x0,
(self->word_alloc - old_size) * sizeof(eword_t));
}
@@ -100,12 +98,7 @@ struct bitmap *ewah_to_bitmap(struct ewah_bitmap *ewah)
ewah_iterator_init(, ewah);
 
while (ewah_iterator_next(, )) {
-   if (i >= bitmap->word_alloc) {
-   bitmap->word_alloc *= 1.5;
-   bitmap->words = xrealloc(
-   bitmap->words, bitmap->word_alloc * 
sizeof(eword_t));
-   }
-
+   ALLOC_GROW(bitmap->words, i + 1, bitmap->word_alloc);
bitmap->words[i++] = blowup;
}
 
@@ -134,8 +127,7 @@ void bitmap_or_ewah(struct bitmap *self, struct ewah_bitmap 
*other)
 
if (self->word_alloc < other_final) {
self->word_alloc = other_final;
-   self->words = xrealloc(self->words,
-   self->word_alloc * sizeof(eword_t));
+   REALLOC_ARRAY(self->words, self->word_alloc);
memset(self->words + original_size, 0x0,
(self->word_alloc - original_size) * sizeof(eword_t));
}
diff --git a/ewah/ewah_bitmap.c b/ewah/ewah_bitmap.c
index fcd465e..2dc9c82 100644
--- a/ewah/ewah_bitmap.c
+++ b/ewah/ewah_bitmap.c
@@ -39,8 +39,7 @@ static inline void buffer_grow(struct ewah_bitmap *self, 
size_t new_size)
return;
 
self->alloc_size = new_size;
-   self->buffer = xrealloc(self->buffer,
-   self->alloc_size * sizeof(eword_t));
+   REALLOC_ARRAY(self->buffer, self->alloc_size);
self->rlw = self->buffer + (rlw_offset / sizeof(eword_t));
 }
 
@@ -283,8 +282,8 @@ struct ewah_bitmap *ewah_new(void)
struct ewah_bitmap *self;
 
self = xmalloc(sizeof(struct ewah_bitmap));
-   self->buffer = xmalloc(32 * sizeof(eword_t));
self->alloc_size = 32;
+   ALLOC_ARRAY(self->buffer, self->alloc_size);
 
ewah_clear(self);
return self;
diff --git a/ewah/ewah_io.c b/ewah/ewah_io.c
index 4acff97..61f6a43 100644
--- a/ewah/ewah_io.c
+++ b/ewah/ewah_io.c
@@ -134,8 +134,7 @@ int ewah_read_mmap(struct ewah_bitmap *self, const void 
*map, size_t len)
self->buffer_size = self->alloc_size = get_be32(ptr);
ptr += sizeof(uint32_t);
 
-   self->buffer = xrealloc(self->buffer,
-   self->alloc_size * sizeof(eword_t));
+   REALLOC_ARRAY(self->buffer, self->alloc_size);
 
/*
 * Copy the raw data for the bitmap as a whole chunk;
@@ -177,8 +176,7 @@ int ewah_deserialize(struct ewah_bitmap *self, int fd)
return -1;
 
self->buffer_size = self->alloc_size = (size_t)ntohl(word_count);
-   self->buffer = xrealloc(self->buffer,
-   self->alloc_size * sizeof(eword_t));
+   REALLOC_ARRAY(self->buffer, self->alloc_size);
 
/** 64 bit x N -- compressed words */
buffer = self->buffer;
-- 
2.7.1.572.gf718037
--
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 17/18] convert ewah/bitmap code to use xmalloc

[Jeff King]

This code was originally written with the idea that it could
be spun off into its own ewah library, and uses the
overrideable ewah_malloc to do allocations.

We plug in xmalloc as our ewah_malloc, of course. But over
the years the ewah code itself has become more entangled
with git, and the return value of many ewah_malloc sites is
not checked.

Let's just drop the level of indirection and use xmalloc and
friends directly. This saves a few lines, and will let us
adapt these sites to our more advanced malloc helpers.

Signed-off-by: Jeff King 
---
 ewah/bitmap.c  | 12 ++--
 ewah/ewah_bitmap.c |  9 +++--
 ewah/ewah_io.c | 10 ++
 ewah/ewok.h| 10 --
 4 files changed, 11 insertions(+), 30 deletions(-)

diff --git a/ewah/bitmap.c b/ewah/bitmap.c
index 47ad674..c88daa0 100644
--- a/ewah/bitmap.c
+++ b/ewah/bitmap.c
@@ -25,8 +25,8 @@
 
 struct bitmap *bitmap_new(void)
 {
-   struct bitmap *bitmap = ewah_malloc(sizeof(struct bitmap));
-   bitmap->words = ewah_calloc(32, sizeof(eword_t));
+   struct bitmap *bitmap = xmalloc(sizeof(struct bitmap));
+   bitmap->words = xcalloc(32, sizeof(eword_t));
bitmap->word_alloc = 32;
return bitmap;
 }
@@ -38,8 +38,8 @@ void bitmap_set(struct bitmap *self, size_t pos)
if (block >= self->word_alloc) {
size_t old_size = self->word_alloc;
self->word_alloc = block * 2;
-   self->words = ewah_realloc(self->words,
-   self->word_alloc * sizeof(eword_t));
+   self->words = xrealloc(self->words,
+ self->word_alloc * sizeof(eword_t));
 
memset(self->words + old_size, 0x0,
(self->word_alloc - old_size) * sizeof(eword_t));
@@ -102,7 +102,7 @@ struct bitmap *ewah_to_bitmap(struct ewah_bitmap *ewah)
while (ewah_iterator_next(, )) {
if (i >= bitmap->word_alloc) {
bitmap->word_alloc *= 1.5;
-   bitmap->words = ewah_realloc(
+   bitmap->words = xrealloc(
bitmap->words, bitmap->word_alloc * 
sizeof(eword_t));
}
 
@@ -134,7 +134,7 @@ void bitmap_or_ewah(struct bitmap *self, struct ewah_bitmap 
*other)
 
if (self->word_alloc < other_final) {
self->word_alloc = other_final;
-   self->words = ewah_realloc(self->words,
+   self->words = xrealloc(self->words,
self->word_alloc * sizeof(eword_t));
memset(self->words + original_size, 0x0,
(self->word_alloc - original_size) * sizeof(eword_t));
diff --git a/ewah/ewah_bitmap.c b/ewah/ewah_bitmap.c
index b522437..fcd465e 100644
--- a/ewah/ewah_bitmap.c
+++ b/ewah/ewah_bitmap.c
@@ -39,7 +39,7 @@ static inline void buffer_grow(struct ewah_bitmap *self, 
size_t new_size)
return;
 
self->alloc_size = new_size;
-   self->buffer = ewah_realloc(self->buffer,
+   self->buffer = xrealloc(self->buffer,
self->alloc_size * sizeof(eword_t));
self->rlw = self->buffer + (rlw_offset / sizeof(eword_t));
 }
@@ -282,11 +282,8 @@ struct ewah_bitmap *ewah_new(void)
 {
struct ewah_bitmap *self;
 
-   self = ewah_malloc(sizeof(struct ewah_bitmap));
-   if (self == NULL)
-   return NULL;
-
-   self->buffer = ewah_malloc(32 * sizeof(eword_t));
+   self = xmalloc(sizeof(struct ewah_bitmap));
+   self->buffer = xmalloc(32 * sizeof(eword_t));
self->alloc_size = 32;
 
ewah_clear(self);
diff --git a/ewah/ewah_io.c b/ewah/ewah_io.c
index 43481b9..4acff97 100644
--- a/ewah/ewah_io.c
+++ b/ewah/ewah_io.c
@@ -134,12 +134,9 @@ int ewah_read_mmap(struct ewah_bitmap *self, const void 
*map, size_t len)
self->buffer_size = self->alloc_size = get_be32(ptr);
ptr += sizeof(uint32_t);
 
-   self->buffer = ewah_realloc(self->buffer,
+   self->buffer = xrealloc(self->buffer,
self->alloc_size * sizeof(eword_t));
 
-   if (!self->buffer)
-   return -1;
-
/*
 * Copy the raw data for the bitmap as a whole chunk;
 * if we're in a little-endian platform, we'll perform
@@ -180,12 +177,9 @@ int ewah_deserialize(struct ewah_bitmap *self, int fd)
return -1;
 
self->buffer_size = self->alloc_size = (size_t)ntohl(word_count);
-   self->buffer = ewah_realloc(self->buffer,
+   self->buffer = xrealloc(self->buffer,
self->alloc_size * sizeof(eword_t));
 
-   if (!self->buffer)
-   return -1;
-
/** 64 bit x N -- compressed words */
buffer = self->buffer;
words_left = self->buffer_size;
diff --git a/ewah/ewok.h b/ewah/ewok.h
index 6e2c5e1..269a1a8 100644
--- a/ewah/ewok.h
+++ b/ewah/ewok.h
@@ -20,16 +20,6 @@
 #ifndef __EWOK_BITMAP_H__
 #define 

Re: git svn dcommit doesn't support --username option for file:/// urls

[Tim Ringenbach]

On Mon, Feb 15, 2016 at 3:14 PM, Eric Wong  wrote:
> It might take a while for me to get around to looking at this
> more, so it would be very helpful if you poked around and tried
> some different things in the source.

Ok, I played around with it some and found something that works.
I commented out all the providers except for:

   SVN::Client::get_username_prompt_provider(
 \::SVN::Prompt::username, 2)

And that seems to actually work!

Interestingly, it doesn't actually interactively prompt me for a
username. At least, not when I ran 'git svn dcommit --username test'.
It did when I later ran a 'git svn fetch'.

I don't know this API at all, and it's been a long time since I've
done any perl. (And I didn't even realize you used perl bindings to
libsvn until a few minutes ago, I just assumed you somehow implemented
everything from scratch.)

But my guess is that 'SVN::Client::get_username_provider()' is
provided by the perl binding and isn't git-svn specific, and so it
knows nothing of the --username argument, it simply is reading ~/.svn.
(Assuming git-svn reads ~/.svn at all.) (That hints at maybe I could
control the user with the files in ~/.svn, which I didn't even
consider previously.) And if it knows nothing about git-svn or any
arguments passed, then that explains why it didn't work.

Meanwhile, 'SVN::Client::get_username_prompt_provider' also looks like
a stock SVN::Client function, but it's passed in a Git::SVN::
argument, that I'm assuming is some sort of callback. So in that case
it's able to provided it with the passed in --username argument, or
failing that, it prompts me.

So I have something that I think will work for me. I'm not sure how to
turn it into a reasonable patch though. Maybe we need to eliminate
some of the auth_provides from the list if the --username option is
passed in?

> Btw, which version of SVN are you using?  I also wonder if
> there's something version-dependent.

svn --version
svn, version 1.6.12 (r955767)

I know that's pretty old.

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

Re: [PATCH 0/18] hardening allocations against integer overflow

[Jeff King]

On Mon, Feb 15, 2016 at 04:45:16PM -0500, Jeff King wrote:

> The only bug I have actually confirmed in practice here is fixed by
> patch 2 (which is why it's at the front). There's another one in
> path_name(), but that function is already dropped by the nearby
> jk/lose-name-path topic.
> 
> The rest are cleanups of spots which _might_ be buggy, but I didn't dig
> too far to find out. As with the earlier audit, I tried to refactor
> using helpers that make the code clearer and less error-prone. So maybe
> they're fixing bugs or not, but they certainly make it easier to audit
> the result for problems.

After this, looking for /[cm]alloc.*\+/ is pretty clean. I _didn't_ fix
any sites in xdiff, but those are already protected by dcd1742 (xdiff:
reject files larger than ~1GB, 2015-09-24).

That's not necessarily complete coverage, though, as you can always
screw up the computation outside of the xmalloc call, and pass in the
truncated version. E.g.:

  int alloc = a + b;
  char *foo = xmalloc(alloc);

However, this is only a big problem if you then copy "a" and "b"
separately. If you use "alloc" later as the limit, like:

  xsnprintf(foo, alloc, "%s%s", a, b);

that's only a minor bug (we notice the overflow and complain, rather
than smashing the heap).

-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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Junio C Hamano]

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?

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