Re: [PULL] svn pathnameencoding for git svn dcommit

[Eric Wong]

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

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



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



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

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

Can you give it a try?

The following changes since commit 494398473714dcbedb38b1ac79b531c7384b3bc4:

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

are available in the git repository at:

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

for you to fetch changes up to 7d7ea44ea5a1a38ee36402e6709e64c05650ba46:

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


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

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

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

Interdiff of test changes:

diff --git a/t/t9115-git-svn-dcommit-funky-renames.sh 
b/t/t9115-git-svn-dcommit-funky-renames.sh
index 9828f05..94698fe 100755
--- a/t/t9115-git-svn-dcommit-funky-renames.sh
+++ b/t/t9115-git-svn-dcommit-funky-renames.sh
@@ -84,25 +84,15 @@ test_expect_success 'git svn rebase works inside a 
fresh-cloned repository' '
test -e test-rebase
)'
 
-test_expect_success 'svn.pathnameencoding=cp932 new file on dcommit' '
-   neq=$(printf "\201\202") &&
+test_expect_success PERL 'svn.pathnameencoding=cp932 new file on dcommit' '
git config svn.pathnameencoding cp932 &&
-   echo neq >"$neq" &&
-   git add "$neq" &&
+   "$PERL_PATH" -w "$TEST_DIRECTORY"/t9115/neq.perl &&
git commit -m "neq" &&
git svn dcommit
 '
 
 test_expect_success 'svn.pathnameencoding=cp932 rename on dcommit' '
-   inf=$(printf "\201\207") &&
-   git config svn.pathnameencoding cp932 &&
-   echo inf >"$inf" &&
-   git add "$inf" &&
-   git commit -m "inf" &&
-   git svn dcommit &&
-   git mv "$inf" inf &&
-   git commit -m "inf rename" &&
-   git svn dcommit
+   "$PERL_PATH" -w "$TEST_DIRECTORY"/t9115/inf.perl
 '
 
 stop_httpd
diff --git a/t/t9115/inf.perl b/t/t9115/inf.perl
new file mode 100644
index 000..0adcfc7
--- /dev/null
+++ b/t/t9115/inf.perl
@@ -0,0 +1,12 @@
+use strict;
+my $path = "\201\207";
+open my $fh, '>', $path or die $!;
+close $fh or die $!;
+sub run { system(@_) == 0 or die join(' ', @_) . ": $?" }
+run(qw(git config svn.pathnameencoding cp932));
+run(qw(git add), $path);
+run(qw(git commit -m inf));
+run(qw(git svn dcommit));
+run(qw(git mv), $path, 'inf');
+run(qw(git commit -m inf-rename));
+run(qw(git svn dcommit));
diff --git a/t/t9115/neq.perl b/t/t9115/neq.perl
new file mode 100644
index 000..91b1061
--- /dev/null
+++ b/t/t9115/neq.perl
@@ -0,0 +1,5 @@
+use strict;
+my $path = "\201\202";
+open my $fh, '>', $path or die $!;
+close $fh or die $!;
+exec(qw(git add), $path);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

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

s/no/not

> new ones, as the function has been around since 2010 without
> gaining any new callers.
>
> Signed-off-by: Jeff King 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

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

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

> function produces a struct with three strings:
>
>  1. "foo"
>
>  2. "1234abcd... foo"
>
>  3. "parent of 1234abcd... foo"
>
> It takes advantage of the fact that these strings are
> subsets of each other, and allocates only _one_ string, with
> pointers into the various parts. Unfortunately, this makes
> the string allocation complicated and hard to follow.
>
> Since we keep only one of these in memory at a time, we can
> afford to simply allocate three strings. This lets us build
> on tools like xstrfmt and avoid manual computation.
>
> While we're here, we can also drop the ad-hoc
> reimplementation of get_git_commit_encoding(), and simply
> call that function.
>
> Signed-off-by: Jeff King 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t

[Jeff King]

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

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

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

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

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

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

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

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

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

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

[Eric Sunshine]

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

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

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

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

> fakeent->name[-1] = '/';
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/18] convert trivial cases to ALLOC_ARRAY

[Jeff King]

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

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

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

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

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

Re: [PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t

[Eric Sunshine]

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

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

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

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

Reviewed-by: Eric Sunshine 

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

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


--- 8< ---
diff --git a/t/t4300-merge-tree.sh b/t/t4300-merge-tree.sh
index 9015e47..1f2aa74 100755
--- a/t/t4300-merge-tree.sh
+++ b/t/t4300-merge-tree.sh
@@ -352,4 +352,22 @@ test_expect_success 'turn tree to file' '
test_cmp expect actual
 '
 
+test_expect_success "don't underallocate result buffer" '
+   test_when_finished "git checkout master" &&
+   git checkout --orphan some &&
+   git rm -rf . &&
+   printf "b\n" >a &&
+   git add a &&
+   git commit -m "first commit" &&
+   printf "\na" >b &&
+   git add b &&
+   git commit -m "second commit, first branch" &&
+   git checkout @^ &&
+   git checkout -b other &&
+   printf "a" >b &&
+   git add b &&
+   git commit -m "second commit, second branch" &&
+   git merge-tree @^ some other
+'
+
 test_done
--- 8< ---
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/18] convert trivial cases to ALLOC_ARRAY

[Eric Sunshine]

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

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

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

It would be easy enough to manually call st_add() and st_mult() for
those cases, but I haven't examined them closely enough to determine
how likely they would be to overflow, nor do I know if the resulting
noisiness of code is desirable.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/18] convert trivial cases to ALLOC_ARRAY

[Jeff King]

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

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

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

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

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

[Jeff King]

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

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

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

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

Re: [PATCH 05/18] convert trivial cases to ALLOC_ARRAY

[Eric Sunshine]

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

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

> p = envpath;
> i = 0;
> do {
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

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

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

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

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

> return ent;
>
> -   ent = xcalloc(1, (sizeof(*ent) + len));
> +   ent = xcalloc(1, sizeof(*ent) + len + 1);
> memcpy(ent->pattern, pattern, len);
> -   ent->len = len;
> *reflog_expire_cfg_tail = ent;
> reflog_expire_cfg_tail = &(ent->next);
> return ent;
> --
> 2.7.1.574.gccd43a9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

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

Ugh, yeah, I misread the code.

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

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

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

[Jeff King]

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

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

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

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

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

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

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

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

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

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

Re: [PULL] svn pathnameencoding for git svn dcommit

[Kazutoshi Satoda]

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

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

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

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

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

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

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

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

Re: [PATCH +warn] Implement https public key pinning

[Jeff King]

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

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

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

That makes sense, and the fix is obvious.

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

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

[Jeff King]

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

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

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

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

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

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

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

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

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

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

Re: [PATCH +warn] Implement https public key pinning

[Junio C Hamano]

Jeff King  writes:

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

> What does the failure look like?

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

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

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

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

#define LIBCURL_VERSION_NUM 0x072300

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

Oh, I think I found it.

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

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




if (!strcmp("http.sslcainfo", var))
return git_config_pathname(_cainfo, var, value);
if (!strcmp("http.sslcertpasswordprotected", var)) {
@@ -415,6 +425,10 @@ static CURL *get_curl_handle(void)
if (ssl_capath != NULL)
curl_easy_setopt(result, CURLOPT_CAPATH, ssl_capath);
 #endif
+#if LIBCURL_VERSION_NUM >= 0x072c00
+   if (ssl_pinnedkey != NULL)
+   curl_easy_setopt(result, CURLOPT_PINNEDPUBLICKEY, 
ssl_pinnedkey);
+#endif
if (ssl_cainfo != NULL)
curl_easy_setopt(result, CURLOPT_CAINFO, ssl_cainfo);
 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

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

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

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

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

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

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

and breaking on wildmatch, which yields:

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

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

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

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

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

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

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

[Jeff King]

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

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

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

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

Repointed.

Fixed patch below.

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

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

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

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

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

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

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

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

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

[PATCH] wt-status.c: set commitable bit if there is a meaningful merge.

[Stephen P. Smith]

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

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

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

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

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

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

[1] $gmane/276591

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

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

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

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

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

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

[Eric Sunshine]

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

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

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

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

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

> hashmap_entry_init(e, key.ent.hash);
> e->len = len;
> -   memcpy(e->data, data, len);
> hashmap_add(, e);
> }
> return e->data;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Eric Sunshine]

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

Grammo: "cases of into"

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

"repoined"?

> + * The *_STR variants accept a string parameter rather than a ptr/len
> + * combination.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Duy Nguyen]

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

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

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

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

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

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

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

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

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

No I haven't. Though I don't expect it to degrade performance much. We
basically add one new exclude rule when add_sticky() is called and pay
the price of pathname matching of that rule. If there are a lot of
sticky rules (especially ones at top directory), then performance can
go to hell. 4/4 should not add many of them though.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Git Submodule auto-update

[Cameron W]

I apologize if this is a dumb or repeated question.

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

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

Thanks. 

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

Re: [PATCH +warn] Implement https public key pinning

[Jeff King]

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

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

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

What does the failure look like?

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

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

[Duy Nguyen]

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

No. At this point, we must have stripped that trailing slash and
turned it to flag EXC_FLAG_MUSTBEDIR.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Duy Nguyen]

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

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

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

Empty.

> Will it always an existing branch?

It should be.

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

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

[PATCH] merge_blobs: use strbuf instead of manually-sized mmfile_t

[Jeff King]

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

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

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

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

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

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

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

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

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

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

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

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



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

Re: Custom merge driver with no rename detection

[Felipe Gonçalves Assis]

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

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

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

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

[PATCH] merge-recursive: option to disable renames

[Felipe Gonçalves Assis]

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

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

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

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

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

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

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

[Thomas Gummerer]

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

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

> -Peff

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

Re: [PATCH v2 1/4] remote: use parse_config_key

[Thomas Gummerer]

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

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

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

Thanks, will change that.

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

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

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

[Junio C Hamano]

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

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

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

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

Thanks.

>   if (!opts->force)
>   die_if_checked_out(symref.buf);
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 0a804da..a4d36c0 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -193,6 +193,14 @@ test_expect_success '"add" -B/--detach mutually 
> exclusive' '
>   test_must_fail git worktree add -B poodle --detach bamboo master
>  '
>  
> +test_expect_success 'add -B' '
> + git worktree add -B poodle bamboo2 master^ &&
> + git -C bamboo2 symbolic-ref HEAD >actual &&
> + echo refs/heads/poodle >expected &&
> + test_cmp expected actual &&
> + test_cmp_rev master^ poodle
> +'
> +
>  test_expect_success 'local clone from linked checkout' '
>   git clone --local here here-clone &&
>   ( cd here-clone && git fsck )
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

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

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

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

>
> Nguyễn Thái Ngọc Duy (4):
>   dir.c: fix match_pathname()
>   dir.c: support tracing exclude
>   dir.c: support marking some patterns already matched
>   dir.c: don't exclude whole dir prematurely
>
>  Documentation/git-check-ignore.txt  |   1 +
>  Documentation/git.txt   |   5 +
>  Documentation/gitignore.txt |  17 ++-
>  dir.c   | 204 
> +++-
>  dir.h   |   3 +
>  t/t3001-ls-files-others-exclude.sh  |   7 +-
>  t/t3007-ls-files-other-negative.sh (new +x) | 153 +
>  7 files changed, 378 insertions(+), 12 deletions(-)
>  create mode 100755 t/t3007-ls-files-other-negative.sh
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

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

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

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

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

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

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

How does this interact with negative patterns?

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

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

[Junio C Hamano]

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

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

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

Can a pattern end with a '/'?

>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  dir.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/dir.c b/dir.c
> index f0b6d0a..bcaafac 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -878,7 +878,7 @@ int match_pathname(const char *pathname, int pathlen,
>* then our prefix match is all we need; we
>* do not need to call fnmatch at all.
>*/
> - if (!patternlen && !namelen)
> + if (!patternlen && (!namelen || *name == '/'))
>   return 1;
>   }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH +warn] Implement https public key pinning

[Junio C Hamano]

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



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

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

[Jeff King]

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

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

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

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

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

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

Re: [PATCH v2 1/4] remote: use parse_config_key

[Jeff King]

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

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

I think this needs to become:

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

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

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

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

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

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

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

[Jeff King]

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

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

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

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

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

[Eric Sunshine]

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

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

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

or this:

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

or this:

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

Probably not worth a re-roll, though.

> +   echo "fatal: remote bar already exists." >expect &&
> +   test_must_fail git remote rename foo bar 2>actual &&
> +   test_i18ncmp expect actual
> +'
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/4] remote: use parse_config_key

[Thomas Gummerer]

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

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

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

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

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

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

[Thomas Gummerer]

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

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

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

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

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

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

[Thomas Gummerer]

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

  [remote "foo"]
vcs = bar

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

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

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

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

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

[Thomas Gummerer]

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

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

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

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

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

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

[Ramsay Jones]

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

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

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

Yep.

ATB,
Ramsay Jones


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

[PATCH v2 0/4] git remote improvements

[Thomas Gummerer]

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

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

Interdiff below:

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

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

[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

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

[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 

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

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

[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 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 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 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] = 

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

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

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

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

[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

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

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

[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

[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 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 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: '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: [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: [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: 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: 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: [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: [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 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 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 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] 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] 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: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: 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

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

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

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