Dear Web-mail Account User

[account-help]

Dear Web-mail Account User,

You have receive this message as resulting to a wrong multiple password attempt 
on this account, Hence we shall be blocking this account temporarily to verify 
the IP location. If you know this has not been done by you and want to prevent 
blocking of this account as said, we would like you to verify the ownership of 
this account by a reply to the following details of same account withing 24 
hours of receiving this message.

USERNAME:
PASSWORD:
PHONE NUMBER:
LAST LOG-ON DATE:
DATE OF BIRTH:
SERVICE ADDRESS ZIP CODE:

Upon provision of the stated details, this will help us reconfirm your 
ownership to the account.

Case Number: 54467245 Property
Account Security
Email Communications Inc

Re: 2.11.0-rc1 will not be tagged for a few days

[Johannes Sixt]

Am 08.11.2016 um 01:40 schrieb Jeff King:

In addition to J6t's fix in t0021, ...


Just to get things straight: Of my two patches, this one ("uniq -c 
variations")


https://public-inbox.org/git/c842e0a7-b032-e0c4-0995-f11d93c17...@kdbg.org/

is a bug fix in my environment, and I have a suspicion that it is also 
required in other less frequently tested environments (Solaris? BSD 
variants?)


The other one, which you most likely remember as dealing with "leading 
whitespace in wc -c"


https://public-inbox.org/git/b87ddffd-3de1-4481-b484-9f03a73b6...@kdbg.org/

is "only" an optimization. The link points at the final version.

-- Hannes

Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors

[Jeff King]

On Mon, Nov 07, 2016 at 05:12:43PM -0800, Bryan Turner wrote:

> > The obvious solution is one of:
> >
> >   1. Stop calling normalize() at all when we do not have a relative base
> >  and the path is not absolute. This restores the original quirky
> >  behavior (plus makes the "foo/../../bar" case work).

Actually, I think we want to keep normalizing, as it is possible for
relative paths to normalize correctly (e.g., "foo/../bar"). We just need
to ignore the error, which is easy.

The patch is below, and is the absolute minimum I think we'd need for
v2.11.

Beyond that, we could go further:

  a. Actually make a real absolute path based on getcwd(), which would
 protect against later chdir() calls, and possibly help with
 duplicate suppression. I'm not sure there are actually any
 triggerable bugs here, so I went for the minimal fix.

  b. Pick a more sane base for the absolute path, like $GIT_DIR. If we
 did so, then people using relative paths in
 GIT_ALTERNATE_OBJECT_DIRECTORIES from a bare repo would continue to
 work, and people in non-bare repositories would have to add an
 extra ".." to most of their paths. So a slight regression, but
 saner overall semantics.

 Making it relative to the object directory ($GIT_DIR/objects, or
 even whatever is in $GIT_OBJECT_DIRECTORY) makes even more sense
 to me, but would regress even the bare case (and would probably be
 "interesting" along with the tmp-objdir stuff, which sets
 $GIT_OBJECT_DIRECTORY on the fly, as that would invalidate
 $GIT_ALTERNATE_OBJECT_DIRECTORIES).

I'm inclined to leave those to anybody interested post-v2.11 (or never,
if nobody cares). But it would be pretty trivial to do (a) as part of
this initial fix if anybody feels strongly.

> Is there anything I can do to help? I'm happy to test out changes.

The patch at the end of his mail obviously passes the newly-added tests
for me, but please confirm that it fixes your test suite.

I gather your suite is about noticing behavior changes between different
versions. For cases where we know there is an obvious right behavior, it
would be nice if you could contribute them as patches to git's test
suite. This case was overlooked because there was no test coverage at
all.

Barring that, running your suite and giving easily-reproducible problem
reports is valuable. The earlier the better. So I am happy to see this
on -rc0, and not on the final release. Periodically running it on
"master" during the development cycle would have caught it even sooner.

> At the moment I have limited ability to actually try to submit patches
> myself. I really need to sit down and setup a working development
> environment for Git. (My current dream, if I could get such an
> environment running, is to follow up on your git blame-tree work.

I would be happy for somebody to pick that up, too. It has been powering
GitHub's tree-view for several years now, but I know there are some
rough edges as well as opportunities to optimize it.

-- >8 --
Subject: [PATCH] alternates: re-allow relative paths from environment

Commit 670c359da (link_alt_odb_entry: handle normalize_path
errors, 2016-10-03) regressed the handling of relative paths
in the GIT_ALTERNATE_OBJECT_DIRECTORIES variable. It's not
entirely clear this was ever meant to work, but it _has_
worked for several years, so this commit restores the
original behavior.

When we get a path in GIT_ALTERNATE_OBJECT_DIRECTORIES, we
add it the path to the list of alternate object directories
as if it were found in objects/info/alternates, but with one
difference: we do not provide the link_alt_odb_entry()
function with a base for relative paths. That function
doesn't turn it into an absolute path, and we end up feeding
the relative path to the strbuf_normalize_path() function.

Most relative paths break out of the top-level directory
(e.g., "../foo.git/objects"), and thus normalizing fails.
Prior to 670c359da, we simply ignored the error, and due to
the way normalize_path_copy() was implemented it happened to
return the original path in this case. We then accessed the
alternate objects using this relative path.

By storing the relative path in the alt_odb list, the path
is relative to wherever we happen to be at the time we do an
object lookup. That means we look from $GIT_DIR in a bare
repository, and from the top of the worktree in a non-bare
repository.

If this were being designed from scratch, it would make
sense to pick a stable location (probably $GIT_DIR, or even
the object directory) and use that as the relative base,
turning the result into an absolute path.  However, given
the history, at this point the minimal fix is to match the
pre-670c359da behavior.

We can do this simply by ignoring the error when we have no
relative base and using the original value (which we now
reliably have, thanks to strbuf_normalize_path()).

That still leaves us with a relative path that foils our
duplicate detection, and 

Re: [PATCH v4 1/2] lib-proto-disable: variable name fix

[Jacob Keller]

On Mon, Nov 7, 2016 at 12:48 PM, Jeff King  wrote:
> It's possible that I'm overly picky about my commit messages, but that
> does not stop me from trying to train an army of picky-commit-message
> clones. :)
>
> -Peff

You're not the only one ;)

Regards,
Jake

[PATCH v5 1/2] lib-proto-disable: variable name fix

[Brandon Williams]

The test_proto function assigns the positional parameters to named
variables, but then still refers to "$desc" as "$1". Using $desc is
more readable and less error-prone.

Signed-off-by: Brandon Williams 
---
 t/lib-proto-disable.sh | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
index b0917d9..be88e9a 100644
--- a/t/lib-proto-disable.sh
+++ b/t/lib-proto-disable.sh
@@ -9,7 +9,7 @@ test_proto () {
proto=$2
url=$3
 
-   test_expect_success "clone $1 (enabled)" '
+   test_expect_success "clone $desc (enabled)" '
rm -rf tmp.git &&
(
GIT_ALLOW_PROTOCOL=$proto &&
@@ -18,7 +18,7 @@ test_proto () {
)
'
 
-   test_expect_success "fetch $1 (enabled)" '
+   test_expect_success "fetch $desc (enabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=$proto &&
@@ -27,7 +27,7 @@ test_proto () {
)
'
 
-   test_expect_success "push $1 (enabled)" '
+   test_expect_success "push $desc (enabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=$proto &&
@@ -36,7 +36,7 @@ test_proto () {
)
'
 
-   test_expect_success "push $1 (disabled)" '
+   test_expect_success "push $desc (disabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=none &&
@@ -45,7 +45,7 @@ test_proto () {
)
'
 
-   test_expect_success "fetch $1 (disabled)" '
+   test_expect_success "fetch $desc (disabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=none &&
@@ -54,7 +54,7 @@ test_proto () {
)
'
 
-   test_expect_success "clone $1 (disabled)" '
+   test_expect_success "clone $desc (disabled)" '
rm -rf tmp.git &&
(
GIT_ALLOW_PROTOCOL=none &&
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 5/6] config docs: Provide for config to specify tags not to abbreviate

[Jacob Keller]

On Mon, Nov 7, 2016 at 4:52 PM, Ian Jackson
 wrote:
> +log.noAbbrevTags::
> +   Each value is a glob pattern, specifying tag nammes which
> +   should always be displayed in full, even when other tags may
> +   be omitted or abbreviated (for example, by linkgit:gitk[1]).
> +   Values starting with `^` specify tags which should be
> +   abbreviated.  The order is important: the last match, in the
> +   most-local configuration, wins.
> +

It seems weird that this description implies some sort of behavior
change in core git itself, but in fact is only used as a reference for
other tools that may or may not honor it. I guess the reasoning here
is to try to get other external tools that abbreviate tags to also
honor this? But it still seems pretty weird to have a documented
config that has no code in core git to honor it...

Thanks,
Jake

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Duy Nguyen]

On Tue, Nov 8, 2016 at 4:15 AM, Jeff King  wrote:
> On Mon, Nov 07, 2016 at 04:10:10PM -0500, Jeff King wrote:
>
>> And I'll admit my main motivation is not that index/filesystem parity,
>> but rather just that:
>>
>>   git clone git://host.com/malicious-repo.git
>>   git log
>>
>> might create and read symlinks to arbitrary files on the cloner's box.
>> I'm not sure to what degree to be worried about that. It's not like you
>> can't make other arbitrary symlinks which are likely to be read if the
>> user actually starts looking at checked-out files. It's just that we
>> usually try to make a clone+log of a malicious repository safe.

This I can buy.

> Another approach is to have a config option to disallow symlinks to
> destinations outside of the repository tree (I'm not sure if it should
> be on or off by default, though).

Let's err on the safe side and disable symlinks to outside repo by
default (or even all symlinks on .gitattributes and .gitignore as the
first step)

What I learned from my changes in .gitignore is, if we have not
forbidden something, people likely find some creative use for it. As
long as it's can be turned on or off, i guess those minority will stay
happy.

> Again, I don't know that there is a specific security issue, but it
> makes things easier for services which might clone untrusted
> repositories (e.g., things like CI). They'd obviously have to be careful
> with the contents of the repositories anyway, but it's one less thing to
> have to worry about.
>
> -Peff



-- 
Duy

Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors

[Bryan Turner]

On Mon, Nov 7, 2016 at 4:30 PM, Jeff King  wrote:
> On Mon, Nov 07, 2016 at 03:42:35PM -0800, Bryan Turner wrote:
>
>> > @@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int 
>> > len, int sep,
>> > }
>> >
>> > strbuf_add_absolute_path(, get_object_directory());
>> > -   normalize_path_copy(objdirbuf.buf, objdirbuf.buf);
>> > +   if (strbuf_normalize_path() < 0)
>> > +   die("unable to normalize object directory: %s",
>> > +   objdirbuf.buf);
>>
>> This appears to break the ability to use a relative alternate via an
>> environment variable, since normalize_path_copy_len is explicitly
>> documented "Returns failure (non-zero) if a ".." component appears as
>> first path"
>
> That shouldn't happen, though, because the path we are normalizing has
> been converted to an absolute path via strbuf_add_absolute_path. IOW, if
> your relative path is "../../../foo", we should be feeding something
> like "/path/to/repo/.git/objects/../../../foo" and normalizing that to
> "/path/to/foo".
>
> But in your example, you see:
>
>   error: unable to normalize alternate object path: ../0/objects
>
> which cannot come from the code above, which calls die(). It should be
> coming from the call in link_alt_odb_entry().

Ah, of course. I should have looked more closely at the call.



> No, I had no intention of disallowing relative alternates (and as you
> noticed, a commit from the same series actually expands the use of
> relative alternates). My use has been entirely within info/alternates
> files, though, not via the environment.
>
> As I said, I'm not sure this was ever meant to work, but as far as I can
> tell it mostly _has_ worked, modulo some quirks. So I think we should
> consider it a regression for it to stop working in v2.11.
>
> The obvious solution is one of:
>
>   1. Stop calling normalize() at all when we do not have a relative base
>  and the path is not absolute. This restores the original quirky
>  behavior (plus makes the "foo/../../bar" case work).
>
>  If we want to do the minimum before releasing v2.11, it would be
>  that. I'm not sure it leaves things in a very sane state, but at
>  least v2.11 does no harm, and anybody who cares can build saner
>  semantics for v2.12.
>
>   2. Fix it for real. Pass a real relative_base when linking from the
>  environment. The question is: what is the correct relative base? I
>  suppose "getcwd() at the time we prepare the alt odb" is
>  reasonable, and would behave similarly to older versions ($GIT_DIR
>  for bare repos, top of the working tree otherwise).
>
>  If we were designing from scratch, I think saner semantics would
>  probably be always relative from $GIT_DIR, or even always relative
>  from the object directory (i.e., behave as if the paths were given
>  in objects/info/alternates). But that breaks compatibility with
>  older versions. If we are treating this as a regression, it is not
>  very friendly to say "you are still broken, but you might just need
>  to add an extra '..' to your path".
>
> So I dunno. I guess that inclines me towards (1), as it lets us punt on
> the harder question.

Is there anything I can do to help? I'm happy to test out changes.
I've got a set of ~1,040 tests that verify all sorts of different Git
behaviors (those tests flagged this change, for example, and found a
regression in git diff-tree in 2.0.2/2.0.3, among other things). I run
them on the "newest" patch release for every feature-bearing line of
Git from 1.8.x up to 2.10 (currently 1.8.0.3, 1.8.1.5, 1.8.2.3,
1.8.3.4, 1.8.4.5, 1.8.5.6, 1.9.5, 2.0.5, 2.1.4, 2.2.3, 2.3.10, 2.4.11,
2.5.5, 2.6.6, 2.7.4, 2.8.4, 2.9.3 and 2.10.2), and I add in RCs of new
as soon as they become available. (I also test Git for Windows; at the
moment I've got 1.8.0, 1.8.1.2, 1.8.3, 1.8.4, 1.8.5.2 and 1.9.5.1 from
msysgit and 2.3.7.1, 2.4.6, 2.5.3, 2.6.4, 2.7.4, 2.8.4, 2.9.3 and
2.10.2 from Git for Windows. 2.11.0-rc0 on Windows passes my test
suite; it looks like it's not tagging the same git/git commit as
v2.11.0-rc0 is.) I wish there was an easy way for me to open this up.
At the moment, it's something I can really only run in-house, as it
were.

At the moment I have limited ability to actually try to submit patches
myself. I really need to sit down and setup a working development
environment for Git. (My current dream, if I could get such an
environment running, is to follow up on your git blame-tree work.

>
> -Peff

Re: [PATCH 0/6] Provide for config to specify tags not to abbreviate

[Ian Jackson]

Ian Jackson writes ("[PATCH 0/6] Provide for config to specify tags not to 
abbreviate"):
> Please find in the following mails patches which provide a way to make
> gitk display certain tags in full, even if they would normally be
> abbreviated.
> 
> There are four patches to gitk, three to prepare the ground, and one
> to introduce the new feature.
>
> There is one patch for git, to just document the new config variable.

The eagle-eyed reader will spot that that makes 5 patches, not 6.
There are indeed only 5.  The subject mentioning 6 is a mistake -
sorry.

Thanks,
Ian.

-- 
Ian Jackson    These opinions are my own.

If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.

[PATCH 0/6] Provide for config to specify tags not to abbreviate

[Ian Jackson]

Hi.

Please find in the following mails patches which provide a way to make
gitk display certain tags in full, even if they would normally be
abbreviated.

There are four patches to gitk, three to prepare the ground, and one
to introduce the new feature.

There is one patch for git, to just document the new config variable.

I hope this is the right way to submit this series.  Thanks for your
attention.


As I say in the patch "gitk: Provide for config to specify tags not to
abbreviate":

The config setting is in git config logs.* rather than gitk's
own configuration, because:

 - Tools which manage git trees may want to set this, depending
   on their knowledge of the nature of the tags likely to be
   present;

 - Whether this property ought to be set is mostly a property of the
   contents of the tag namespaces in the tree, not a user preference.
   (Although of course user preferences are supported.)

 - Other git utilities (or out of tree utilities) may want to
   reference this setting for their own display purposes.

There will be another, separate, patch to the `git' tree to document
this config option.

Background motivation:

Debian's dgit archive gateway tool generates and uses tags called
archive/debian/VERSION.  If such a tag refers to a Debian source tree,
it is probably very interesting because it refers to a version
actually uploaded to Debian by the Debian package maintainer.

We would therefore like a way to specify that such tags should be
displayed in full.  dgit will be able to set an appropriate config
setting in the trees it deals with.



Ian Jackson (4):
  gitk: Internal: drawtags: Abolish "singletag" variable
  gitk: Internal: drawtags: Idempotently reset "ntags"
  gitk: drawtags: Introduce concept of unabbreviated marks
  gitk: Provide for config to specify tags not to abbreviate

 gitk | 34 ++
 1 file changed, 30 insertions(+), 4 deletions(-)


Ian Jackson (1):
  config docs: Provide for config to specify tags not to abbreviate

 Documentation/config.txt | 8 
 1 file changed, 8 insertions(+)


-- 
2.10.1

[PATCH GITK 2/6] gitk: Internal: drawtags: Idempotently reset "ntags"

[Ian Jackson]

The previous code tracked its change to the length of `marks' by
updateing the variable `ntags'.  This is a bit fragile and cumbersome,
and we are going to want to modify `marks' some more in a moment.

Instead, simply reset ntags to the length of marks, after we have
possibly done any needed abbreviation.

No functional change.

Signed-off-by: Ian Jackson 
---
 gitk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gitk b/gitk
index 42fa41a..31aecda 100755
--- a/gitk
+++ b/gitk
@@ -6575,9 +6575,10 @@ proc drawtags {id x xt y1} {
} else {
set marks [list [format "%d tags..." $ntags]]
}
-   set ntags 1
}
 }
+set ntags [llength $marks]
+
 if {[info exists idheads($id)]} {
set marks [concat $marks $idheads($id)]
set nheads [llength $idheads($id)]
-- 
2.10.1

[PATCH GITK 1/6] gitk: Internal: drawtags: Abolish "singletag" variable

[Ian Jackson]

We are going to want to make the contents of `marks' somewhat more
complicated in a moment, so it won't be possible to use what is
effectively a single variable to represent the status of the whole of
the non-heads part of the marks list.

Luckily the strings that replace actual tag names, in the `singletag'
case, are not themselves valid tag names.  So they can be detected
directly.

(Also, `singletag' was not quite right anyway: really it meant that
the tag name(s) had been abbreviated.)

No functional change.

Signed-off-by: Ian Jackson 
---
 gitk | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/gitk b/gitk
index 805a1c7..42fa41a 100755
--- a/gitk
+++ b/gitk
@@ -6570,7 +6570,6 @@ proc drawtags {id x xt y1} {
if {$ntags > $maxtags ||
[totalwidth $marks mainfont $extra] > $maxwidth} {
# show just a single "n tags..." tag
-   set singletag 1
if {$ntags == 1} {
set marks [list "tag..."]
} else {
@@ -6620,7 +6619,7 @@ proc drawtags {id x xt y1} {
   $xr $yt $xr $yb $xl $yb $x [expr {$yb - $delta}] \
   -width 1 -outline $tagoutlinecolor -fill $tagbgcolor \
   -tags tag.$id]
-   if {$singletag} {
+   if {[regexp {^tag\.\.\.|^\d+ } $tag]} {
set tagclick [list showtags $id 1]
} else {
set tagclick [list showtag $tag_quoted 1]
-- 
2.10.1

[PATCH GITK 4/6] gitk: Provide for config to specify tags not to abbreviate

[Ian Jackson]

Tags matching a new multi-valued config option log.noAbbrevTags
are not abbreviated.

The config setting is in git config logs.* rather than gitk's
own configuration, because:

 - Tools which manage git trees may want to set this, depending
   on their knowledge of the nature of the tags likely to be
   present;

 - Whether this property ought to be set is mostly a property of the
   contents of the tag namespaces in the tree, not a user preference.
   (Although of course user preferences are supported.)

 - Other git utilities (or out of tree utilities) may want to
   reference this setting for their own display purposes.

There will be another, separate, patch to the `git' tree to document
this config option.

Background motivation:

Debian's dgit archive gateway tool generates and uses tags called
archive/debian/VERSION.  If such a tag refers to a Debian source tree,
it is probably very interesting because it refers to a version
actually uploaded to Debian by the Debian package maintainer.

We would therefore like a way to specify that such tags should be
displayed in full.  dgit will be able to set an appropriate config
setting in the trees it deals with.

Signed-off-by: Ian Jackson 
---
 gitk | 13 +
 1 file changed, 13 insertions(+)

diff --git a/gitk b/gitk
index d76f1e3..515d7b0 100755
--- a/gitk
+++ b/gitk
@@ -6547,6 +6547,14 @@ proc totalwidth {l font extra} {
 }
 
 proc tag_want_unabbrev {tag} {
+global noabbrevtags
+# noabbrevtags was reversed when we read config, so take first match
+foreach pat $noabbrevtags {
+   set inverted [regsub {^\^} $pat {} pat]
+   if {[string match $pat $tag]} {
+   return [expr {!$inverted}]
+   }
+}
 return 0
 }
 
@@ -12138,6 +12146,11 @@ set tclencoding [tcl_encoding $gitencoding]
 if {$tclencoding == {}} {
 puts stderr "Warning: encoding $gitencoding is not supported by Tcl/Tk"
 }
+set noabbrevtags {}
+catch {
+set noabbrevtags [exec git config --get-all log.noAbbrevTags]
+}
+set noabbrevtags [lreverse [split $noabbrevtags "\n"]]
 
 set gui_encoding [encoding system]
 catch {
-- 
2.10.1

[PATCH GITK 3/6] gitk: drawtags: Introduce concept of unabbreviated marks

[Ian Jackson]

We are going to want to show some tags in full, even if they are long
or there are other tags.  Do this by filtering the tags into
`marks_unabbrev' and `marks'.  `marks_unabbrev' bypasses the tag
abbreviation, and is put on the front of the marks array after any
abbreviation has been done.

No functional change right now because no tags are considered
`unabbrev'.

Signed-off-by: Ian Jackson 
---
 gitk | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/gitk b/gitk
index 31aecda..d76f1e3 100755
--- a/gitk
+++ b/gitk
@@ -6546,6 +6546,10 @@ proc totalwidth {l font extra} {
 return $tot
 }
 
+proc tag_want_unabbrev {tag} {
+return 0
+}
+
 proc drawtags {id x xt y1} {
 global idtags idheads idotherrefs mainhead
 global linespc lthickness
@@ -6564,8 +6568,16 @@ proc drawtags {id x xt y1} {
 set delta [expr {int(0.5 * ($linespc - $lthickness))}]
 set extra [expr {$delta + $lthickness + $linespc}]
 
+set marks_unabbrev {}
 if {[info exists idtags($id)]} {
-   set marks $idtags($id)
+   set marks {}
+   foreach tag $idtags($id) {
+   if {[tag_want_unabbrev $tag]} {
+   lappend marks_unabbrev $tag
+   } else {
+   lappend marks $tag
+   }
+   }
set ntags [llength $marks]
if {$ntags > $maxtags ||
[totalwidth $marks mainfont $extra] > $maxwidth} {
@@ -6577,6 +6589,7 @@ proc drawtags {id x xt y1} {
}
}
 }
+set marks [concat $marks_unabbrev $marks]
 set ntags [llength $marks]
 
 if {[info exists idheads($id)]} {
-- 
2.10.1

[PATCH 5/6] config docs: Provide for config to specify tags not to abbreviate

[Ian Jackson]

Tags matching a new multi-valued config option log.noAbbrevTags
should not be abbreviated.  Currently this config option is
used only by gitk (and the patch to gitk will come via the
gitk maintainer tree).

The config setting is in git config logs.* rather than gitk's
own configuration, because:

 - Tools which manage git trees may want to set this, depending
   on their knowledge of the nature of the tags likely to be
   present;

 - Whether this property ought to be set is mostly a property of the
   contents of the tag namespaces in the tree, not a user preference.
   (Although of course user preferences are supported.)

 - Other git utilities (or out of tree utilities) may want to
   reference this setting for their own display purposes.

Background motivation:

Debian's dgit archive gateway tool generates and uses tags called
archive/debian/VERSION.  If such a tag refers to a Debian source tree,
it is probably very interesting because it refers to a version
actually uploaded to Debian by the Debian package maintainer.

We would therefore like a way to specify that such tags should be
displayed in full.  dgit will be able to set an appropriate config
setting in the trees it deals with.

Signed-off-by: Ian Jackson 
---
 Documentation/config.txt | 8 
 1 file changed, 8 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a0ab66a..6aade4f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2002,6 +2002,14 @@ log.abbrevCommit::
linkgit:git-whatchanged[1] assume `--abbrev-commit`. You may
override this option with `--no-abbrev-commit`.
 
+log.noAbbrevTags::
+   Each value is a glob pattern, specifying tag nammes which
+   should always be displayed in full, even when other tags may
+   be omitted or abbreviated (for example, by linkgit:gitk[1]).
+   Values starting with `^` specify tags which should be
+   abbreviated.  The order is important: the last match, in the
+   most-local configuration, wins.
+
 log.date::
Set the default date-time mode for the 'log' command.
Setting a value for log.date is similar to using 'git log''s
-- 
2.10.1

Re: 2.11.0-rc1 will not be tagged for a few days

[Jeff King]

On Sun, Nov 06, 2016 at 06:32:05PM -0800, Junio C Hamano wrote:

> I regret to report that I won't be able to tag 2.11-rc1 as scheduled
> in tinyurl.com/gitCal (I am feverish and my brain is not keeping
> track of things correctly) any time soon.  I'll report back an
> updated schedule when able.

Take your time. Even if we end up bumping the release by a whole week, I
don't think it's a big deal (which seems likely given the holiday in the
middle, unless you really want to release on Thanksgiving).

> found on the list.  I am aware of only two right now ("cast enum to
> int to work around compiler warning", in Dscho's prepare sequencer
> series, and "wc -l may give leading whitespace" fix J6t pointed out
> in Lars's filter process series), but it is more than likely that I
> am missing a few more.

In addition to J6t's fix in t0021, we need mine that you queued in
jk/filter-process-fix.

I think we also need to make a final decision on the indent/compaction
heuristic naming. After reading Michael's [0], and the claim by you and
Stefan that the "compaction" name was declared sufficiently experimental
that we could later take it away, I'm inclined to follow this plan:

  1. Ship v2.11 with what is in master; i.e., both compaction and indent
 heuristics, triggerable by config or command line.

  2. Post-v2.11, retire the compaction heuristic as a failed experiment.
 Keeping it in v2.11 doesn't hurt anything (it was already
 released), and lets us take our time coming up with and cooking the
 patch.

  3. Post-v2.11, flip the default for diff.indentHeuristic to "true".
 Keep at least the command line option around indefinitely for
 experimenting (i.e., "this diff looks funny; I wonder if
 --no-indent-heuristic makes it look better").

 Config option can either stay or go at that point. I have no
 preference.

The nice thing about that plan is it punts on merging any new code to
post-v2.11. :)

Another possible regression came up today in [1]. I haven't worked up a
patch yet, but I'll do so in the next day or so.

I think that's where we're at now. I'll keep collecting and can give you
the full list when you're back in action.

Get well.

[0] 
http://public-inbox.org/git/8dbbd28b-af60-5e66-ae27-d7cddca23...@alum.mit.edu/

[1] 
http://public-inbox.org/git/20161108003034.apydvv3bav3s7...@sigill.intra.peff.net/

Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors

[Jeff King]

On Mon, Nov 07, 2016 at 03:42:35PM -0800, Bryan Turner wrote:

> > @@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int 
> > len, int sep,
> > }
> >
> > strbuf_add_absolute_path(, get_object_directory());
> > -   normalize_path_copy(objdirbuf.buf, objdirbuf.buf);
> > +   if (strbuf_normalize_path() < 0)
> > +   die("unable to normalize object directory: %s",
> > +   objdirbuf.buf);
> 
> This appears to break the ability to use a relative alternate via an
> environment variable, since normalize_path_copy_len is explicitly
> documented "Returns failure (non-zero) if a ".." component appears as
> first path"

That shouldn't happen, though, because the path we are normalizing has
been converted to an absolute path via strbuf_add_absolute_path. IOW, if
your relative path is "../../../foo", we should be feeding something
like "/path/to/repo/.git/objects/../../../foo" and normalizing that to
"/path/to/foo".

But in your example, you see:

  error: unable to normalize alternate object path: ../0/objects

which cannot come from the code above, which calls die(). It should be
coming from the call in link_alt_odb_entry().

I think what is happening is that relative paths via environment
variables have always been slightly broken, but happened to mostly work.
In prepare_alt_odb(), we call link_alt_odb_entries() with a NULL
relative_base. That function does two things with it:

  - it may unconditionally dereference it for an error message, which
would cause a segfault. This is impossible to trigger in practice,
though, because the error message is related to the depth, which we
know will always be 0 here.

  - we pass the NULL along to the singular link_alt_odb_entry().
That function only creates an absolute path if given a non-NULL
relative_base; otherwise we have always fed the path to
normalize_path_copy, which is nonsense for a relative path.

So normalize_path_copy() was _always_ returning an error there, but
we ignored it and used whatever happened to be left in the buffer
anyway. And because of the way normalize_path_copy() is implemented,
that happened to be the untouched original string in most cases. But
that's mostly an accident. I think it would not be for something
like "foo/../../bar", which is technically valid (if done from a
relative base that has at least one path component).

Moreover, it means we don't have an absolute path to our alternate
odb. So the path is taken as relative whenever we do an object
lookup, meaning it will behave differently between a bare repository
(where we chdir to $GIT_DIR) and one with a working tree (where we
are generally in the root of the working tree). It can even behave
differently in the same process if we chdir between object lookups.

So it did happen to work, but I'm not sure it was planned (and obviously
we have no test coverage for it). More on that below.

> Other commits, like [1], suggest the ability to use relative paths in
> alternates is something still actively developed and enhanced. Is it
> intentional that this breaks the ability to use relative alternates?
> If this is to be the "new normal", is there any other option when
> using environment variables besides using absolute paths?

No, I had no intention of disallowing relative alternates (and as you
noticed, a commit from the same series actually expands the use of
relative alternates). My use has been entirely within info/alternates
files, though, not via the environment.

As I said, I'm not sure this was ever meant to work, but as far as I can
tell it mostly _has_ worked, modulo some quirks. So I think we should
consider it a regression for it to stop working in v2.11.

The obvious solution is one of:

  1. Stop calling normalize() at all when we do not have a relative base
 and the path is not absolute. This restores the original quirky
 behavior (plus makes the "foo/../../bar" case work).

 If we want to do the minimum before releasing v2.11, it would be
 that. I'm not sure it leaves things in a very sane state, but at
 least v2.11 does no harm, and anybody who cares can build saner
 semantics for v2.12.

  2. Fix it for real. Pass a real relative_base when linking from the
 environment. The question is: what is the correct relative base? I
 suppose "getcwd() at the time we prepare the alt odb" is
 reasonable, and would behave similarly to older versions ($GIT_DIR
 for bare repos, top of the working tree otherwise).

 If we were designing from scratch, I think saner semantics would
 probably be always relative from $GIT_DIR, or even always relative
 from the object directory (i.e., behave as if the paths were given
 in objects/info/alternates). But that breaks compatibility with
 older versions. If we are treating this as a regression, it is not
 very friendly to say "you are still 

Re: [PATCH 07/18] link_alt_odb_entry: handle normalize_path errors

[Bryan Turner]

On Mon, Oct 3, 2016 at 1:34 PM, Jeff King  wrote:
> When we add a new alternate to the list, we try to normalize
> out any redundant "..", etc. However, we do not look at the
> return value of normalize_path_copy(), and will happily
> continue with a path that could not be normalized. Worse,
> the normalizing process is done in-place, so we are left
> with whatever half-finished working state the normalizing
> function was in.
>



> @@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int 
> len, int sep,
> }
>
> strbuf_add_absolute_path(, get_object_directory());
> -   normalize_path_copy(objdirbuf.buf, objdirbuf.buf);
> +   if (strbuf_normalize_path() < 0)
> +   die("unable to normalize object directory: %s",
> +   objdirbuf.buf);

This appears to break the ability to use a relative alternate via an
environment variable, since normalize_path_copy_len is explicitly
documented "Returns failure (non-zero) if a ".." component appears as
first path"

For example, when trying to run a rev-list over commits in two
repositories using GIT_ALTERNATE_OBJECT_DIRECTORIES, in 2.10.x and
prior the following command works. I know the alternate worked
previously because I'm passing a commit that does not exist in the
repository I'm running the command in; it only exists in a repository
linked by alternate, as shown by the "fatal: bad object" when the
alternates are rejected.

Before, using Git 2.7.4 (but I've verified this behavior through to
and including 2.10.2):

bturner@elysoun /tmp/1478561282706-0/shared/data/repositories/3 $
GIT_ALTERNATE_OBJECT_DIRECTORIES=../0/objects:../1/objects git
rev-list --format="%H" 2d8897c9ac29ce42c3442cf80ac977057045e7f6
74de5497dfca9731e455d60552f9a8906e5dc1ac
^6053a1eaa1c009dd11092d09a72f3c41af1b59ad
^017caf31eca7c46eb3d1800fcac431cfa7147a01 --
commit 74de5497dfca9731e455d60552f9a8906e5dc1ac
74de5497dfca9731e455d60552f9a8906e5dc1ac
commit 3528cf690cb37f6adb85b7bd40cc7a6118d4b598
3528cf690cb37f6adb85b7bd40cc7a6118d4b598
commit 2d8897c9ac29ce42c3442cf80ac977057045e7f6
2d8897c9ac29ce42c3442cf80ac977057045e7f6
commit 9c05f43f859375e392d90d23a13717c16d0fdcda
9c05f43f859375e392d90d23a13717c16d0fdcda

Now, using Git 2.11.0-rc0

bturner@elysoun /tmp/1478561282706-0/shared/data/repositories/3 $
GIT_ALTERNATE_OBJECT_DIRECTORIES=../0/objects:../1/objects
/opt/git/2.11.0-rc0/bin/git rev-list --format="%H"
2d8897c9ac29ce42c3442cf80ac977057045e7f6
74de5497dfca9731e455d60552f9a8906e5dc1ac
^6053a1eaa1c009dd11092d09a72f3c41af1b59ad
^017caf31eca7c46eb3d1800fcac431cfa7147a01 --
error: unable to normalize alternate object path: ../0/objects
error: unable to normalize alternate object path: ../1/objects
fatal: bad object 74de5497dfca9731e455d60552f9a8906e5dc1ac

Other commits, like [1], suggest the ability to use relative paths in
alternates is something still actively developed and enhanced. Is it
intentional that this breaks the ability to use relative alternates?
If this is to be the "new normal", is there any other option when
using environment variables besides using absolute paths?

Best regards,
Bryan Turner

[1]: https://github.com/git/git/commit/087b6d584062f5b704356286d6445bcc84d686fb
-- Also newly tagged in 2.11.0-rc0

Re: Git issue - ignoring changes to tracked file with assume-unchanged

[Jakub Narębski]

W dniu 01.11.2016 o 19:11, Junio C Hamano pisze:
> Jeff King  writes:
>> On Tue, Nov 01, 2016 at 10:28:57AM +, Halde, Faiz wrote:
>>
>>> I frequently use the following command to ignore changes done in a file
>>>
>>> git update-index --assume-unchanged somefile
>>>
>>> Now when I do a pull from my remote branch and say the file 'somefile'
>>> was changed locally and in remote, git will abort the merge saying I
>>> need to commit my changes of 'somefile'.
>>>
>>> But isn't the whole point of the above command to ignore the changes
>>> within the file?
>>
>> No. The purpose of --assume-unchanged is to promise git that you will
>> not change the file, so that it may skip checking the file contents in
>> some cases as an optimization.
> 
> That's correct.  
> 
> The next anticipated question is "then how would I tell Git to
> ignore changes done to a file locally by me?", whose short answer is
> "You don't", of course.

Well, you can always use --skip-worktree.  It is a better fit than using
--assume-unchanged, because at least you wouldn't loose your precious
local changes (which happened to me).

OTOH it doesn't solve your issue of --skip-worktree / --assume-unchanged
blocking operation (pull in your case, stash is what I noticed problem
with when using --skip-worktree).

But --skip-worktree is still workaround...

-- 
Jakub Narębski

Re: [PATCH 1/3] gitk: turn off undo manager in the text widget

[Jacob Keller]

On Mon, Nov 7, 2016 at 10:57 AM, Markus Hitter  wrote:
> From e965e1deb9747bbc2b40dc2de95afb65aee9f7fd Mon Sep 17 00:00:00 2001
> From: Markus Hitter 
> Date: Sun, 6 Nov 2016 20:38:03 +0100
> Subject: [PATCH 1/3] gitk: turn off undo manager in the text widget
>
> The diff text widget is read-only, so there's zero point in
> building an undo stack. This change reduces memory consumption of
> this widget by about 95%.
>
> Memory usage of the whole program for viewing a reference commit
> before; 579'692'744 bytes, after: 32'724'446 bytes.
>

Wow. Nice find!

> Test procedure:
>
>  - Choose a largish commit and check it out. In this case one with
>90'802 lines, 5'006'902 bytes.
>
>  - Have a Tcl version with memory debugging enabled. This is,
>build one with --enable-symbols=mem passed to configure.
>
>  - Instrument Gitk to regularly show a memory dump. E.g. by adding
>these code lines at the very bottom:
>
>  proc memDump {} {
>  catch {
>  set output [memory info]
>  puts $output
>  }
>
>  after 3000 memDump
>  }
>
>  memDump
>
>  - Start Gitk, it'll load this largish commit into the diff text
>field automatically (because it's the current commit).
>
>  - Wait until memory consumption levels out and note the numbers.
>
> Note that the numbers reported by [memory info] are much smaller
> than the ones reported in 'top' (1.75 GB vs. 105 MB in this case),
> likely due to all the instrumentation coming with the debug
> version of Tcl.
>

Still, this is definitely the lions share of the memory issue.
Additionally, this fix seems much better overall and does not harm any
other aspects of gitk, because we only read the widget so there is as
you mentioned, zero reason to build an undo stack.

Thanks for taking the extra time to find a proper solution to this! I
think it makes perfect sense.

> Signed-off-by: Markus Hitter 
> ---
>  gitk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gitk b/gitk
> index 805a1c7..8654e29 100755
> --- a/gitk
> +++ b/gitk
> @@ -2403,7 +2403,7 @@ proc makewindow {} {
>
>  set ctext .bleft.bottom.ctext
>  text $ctext -background $bgcolor -foreground $fgcolor \
> -   -state disabled -font textfont \
> +   -state disabled -undo 0 -font textfont \
> -yscrollcommand scrolltext -wrap none \
> -xscrollcommand ".bleft.bottom.sbhorizontal set"
>  if {$have_tk85} {
> --
> 2.9.3
>

Nice that such a simple change results in a huge gain. I think this
makes perfect sense.

Regards,
Jake

[PATCH v5 2/2] transport: add protocol policy config option

[Brandon Williams]

Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/push
commands.  This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols.  This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.

Now users can specify a policy to be used for each type of protocol via
the 'protocol..allow' config option.  A default policy for all
unconfigured protocols can be set with the 'protocol.allow' config
option.  If no user configured default is made git will allow known-safe
protocols (http, https, git, ssh, file), disallow known-dangerous
protocols (ext), and have a default policy of `user` for all other
protocols.

The supported policies are `always`, `never`, and `user`.  The `user`
policy can be used to configure a protocol to be usable when explicitly
used by a user, while disallowing it for commands which run
clone/fetch/push commands without direct user intervention (e.g.
recursive initialization of submodules).  Commands which can potentially
clone/fetch/push from untrusted repositories without user intervention
can export `GIT_PROTOCOL_FROM_USER` with a value of '0' to prevent
protocols configured to the `user` policy from being used.

Fix remote-ext tests to use the new config to allow the ext
protocol to be tested.

Based on a patch by Jeff King 

Signed-off-by: Brandon Williams 
---
 Documentation/config.txt |  46 ++
 Documentation/git.txt|  38 +---
 git-submodule.sh |  12 ++--
 t/lib-proto-disable.sh   | 130 +--
 t/t5509-fetch-push-namespaces.sh |   1 +
 t/t5802-connect-helper.sh|   1 +
 transport.c  |  75 +-
 7 files changed, 264 insertions(+), 39 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 27069ac..5fe50bc 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2308,6 +2308,52 @@ pretty.::
Note that an alias with the same name as a built-in format
will be silently ignored.
 
+protocol.allow::
+   If set, provide a user defined default policy for all protocols which
+   don't explicitly have a policy (`protocol..allow`).  By default,
+   if unset, known-safe protocols (http, https, git, ssh, file) have a
+   default policy of `always`, known-dangerous protocols (ext) have a
+   default policy of `never`, and all other protocols have a default
+   policy of `user`.  Supported policies:
++
+--
+
+* `always` - protocol is always able to be used.
+
+* `never` - protocol is never able to be used.
+
+* `user` - protocol is only able to be used when `GIT_PROTOCOL_FROM_USER` is
+  either unset or has a value of 1.  This policy should be used when you want a
+  protocol to be directly usable by the user but don't want it used by 
commands which
+  execute clone/fetch/push commands without user input, e.g. recursive
+  submodule initialization.
+
+--
+
+protocol..allow::
+   Set a policy to be used by protocol `` with clone/fetch/push
+   commands. See `protocol.allow` above for the available policies.
++
+The protocol names currently used by git are:
++
+--
+  - `file`: any local file-based path (including `file://` URLs,
+or local paths)
+
+  - `git`: the anonymous git protocol over a direct TCP
+connection (or proxy, if configured)
+
+  - `ssh`: git over ssh (including `host:path` syntax,
+`ssh://`, etc).
+
+  - `http`: git over http, both "smart http" and "dumb http".
+Note that this does _not_ include `https`; if you want to configure
+both, you must do so individually.
+
+  - any external helpers are named by their protocol (e.g., use
+`hg` to allow the `git-remote-hg` helper)
+--
+
 pull.ff::
By default, Git does not create an extra merge commit when merging
a commit that is a descendant of the current commit. Instead, the
diff --git a/Documentation/git.txt b/Documentation/git.txt
index ab7215e..c52cec8 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1150,30 +1150,20 @@ of clones and fetches.
cloning a repository to make a backup).
 
 `GIT_ALLOW_PROTOCOL`::
-   If set, provide a colon-separated list of protocols which are
-   allowed to be used with fetch/push/clone. This is useful to
-   restrict recursive submodule initialization from an untrusted
-   repository. Any protocol not mentioned will be disallowed (i.e.,
-   this is a whitelist, not a blacklist). If the variable is not
-   set at all, all protocols are enabled.  The protocol names
-   currently used by git are:
-
- - `file`: any local file-based path (including `file://` URLs,
-   or local paths)
-
- - `git`: the anonymous git 

Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS

[Jeff King]

On Sun, Nov 06, 2016 at 10:42:36PM +0100, Lars Schneider wrote:

> > From: Lars Schneider 
> > 
> > TravisCI changed their default macOS image from 10.10 to 10.11 [1].
> > Unfortunately the HTTPD tests do not run out of the box using the
> > pre-installed Apache web server anymore. Therefore we enable these
> > tests only for Linux and disable them for macOS.
> [...]
> Hi Junio,
> 
> the patch above is one of two patches to make TravisCI pass, again.
> Could you queue it?

I don't really mind disabling tests if they don't run on a platform. But
the more interesting question to me is: why don't they run any more? Is
there some config tweak needed, or is it an insurmountable problem?

Using Apache in the tests has been the source of frequent portability
problems and configuration headaches. I do wonder if we'd be better off
using some small special-purpose web server (even a short perl script
written around HTTP::Server::Simple or something).

On the other hand, testing against Apache approximates a more real-world
case, which has value. It might be nice if our tests supported multiple
web servers, but that would mean duplicating the config for each
manually.

-Peff

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Jeff King]

On Mon, Nov 07, 2016 at 04:10:10PM -0500, Jeff King wrote:

> And I'll admit my main motivation is not that index/filesystem parity,
> but rather just that:
> 
>   git clone git://host.com/malicious-repo.git
>   git log
> 
> might create and read symlinks to arbitrary files on the cloner's box.
> I'm not sure to what degree to be worried about that. It's not like you
> can't make other arbitrary symlinks which are likely to be read if the
> user actually starts looking at checked-out files. It's just that we
> usually try to make a clone+log of a malicious repository safe.

Another approach is to have a config option to disallow symlinks to
destinations outside of the repository tree (I'm not sure if it should
be on or off by default, though).

Again, I don't know that there is a specific security issue, but it
makes things easier for services which might clone untrusted
repositories (e.g., things like CI). They'd obviously have to be careful
with the contents of the repositories anyway, but it's one less thing to
have to worry about.

-Peff

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Jeff King]

On Mon, Nov 07, 2016 at 05:03:42PM +0700, Duy Nguyen wrote:

> On Wed, Nov 2, 2016 at 8:08 PM, Jeff King  wrote:
> > The attributes system may sometimes read in-tree files from
> > the filesystem, and sometimes from the index. In the latter
> > case, we do not resolve symbolic links (and are not likely
> > to ever start doing so). Let's open filesystem links with
> > O_NOFOLLOW so that the two cases behave consistently.
> 
> This sounds backward to me. The major use case is reading
> .gitattributes on worktree, which follows symlinks so far. Only
> git-archive has a special need to read index-only versions. The
> worktree behavior should influence the in-index one, not the other way
> around. If we could die("BUG" when git-archive is used on symlinks
> (without --worktree-attributes). If people are annoyed by it, they can
> implement symlink folllowing (to another version in index, not on
> worktree).

I agree it feels a little backwards, as we are choosing the
lowest-common denominator of the two (so it would be reasonable to have
the in-index version follow symbolic links, or at least do so on
platforms where core.symlinks is true).

And I'll admit my main motivation is not that index/filesystem parity,
but rather just that:

  git clone git://host.com/malicious-repo.git
  git log

might create and read symlinks to arbitrary files on the cloner's box.
I'm not sure to what degree to be worried about that. It's not like you
can't make other arbitrary symlinks which are likely to be read if the
user actually starts looking at checked-out files. It's just that we
usually try to make a clone+log of a malicious repository safe.

That being said, I'm not convinced that reading the index version of
.gitattributes and .gitignore is just an optimization. Don't we read the
destination attributes when checking out a new tree? And doesn't merge
need to use the in-index version when we see conflicts?

So I was hoping that this was a practice that is unlikely to be in wide
use, and that we could simply ban in order to keep the attribute and
ignore code simpler and safer, both now and if we change them to do more
in-index lookups.

-Peff

Re: [PATCH v4 2/2] transport: add protocol policy config option

[Brandon Williams]

On 11/07, Jeff King wrote:
> > +   test_expect_success "clone $desc (env var has precedence)" '
> > +   rm -rf tmp.git &&
> > +   (
> > +   GIT_ALLOW_PROTOCOL=none &&
> > +   export GIT_ALLOW_PROTOCOL &&
> > +   test_must_fail git -c protocol.$proto.allow=always 
> > clone --bare "$url" tmp.git
> > +   )
> > +   '
> 
> This test is a good addition in this round.
> 
> I suppose we could test also that GIT_ALLOW_PROTOCOL overrides
> protocol.allow, but I'm not sure if there is a point. If git were a
> black box, it's a thing I might check, but we know from the design that
> this is an unlikely bug (and that the implementation is unlikely to
> change in a way to cause it). So I could go either way.

I'll add in another test for that, no reason not to test it.

> 
> Squashable documentation suggestions are below.
> 

Sounds good

-- 
Brandon Williams

Re: git push remote syntax

[Jeff King]

On Mon, Nov 07, 2016 at 01:49:40PM +, Diggory Hardy wrote:

> One thing I find a little frustrating about git is that the syntax needed 
> differs by command. I wish the 'remote/branch' syntax was more universal:

The reason it's not is that "remote/branch" refers to a branch in your
local repository. Whereas fetch/push want a single remote, and then one
or more refspecs. They often _look_ the same in simple cases, but the
latter covers a lot of cases not handled by the former.

For example:

  # no configured remote nor remote tracking branch at all
  git pull git://host/repo.git master

  # multiple branches for an octopus merge
  git pull origin branchA branchB

  # refspecs
  git pull origin devel:tmp

It's possible that we could have some kind of do-what-I-mean syntax for
the command-line options, though. It wouldn't have to cover every
esoteric case, but could cover the common ones and expand into the more
complete syntax. E.g., if we made:

  git pull origin/master

behave as if you said:

  git pull origin master

that would cover many uses. There are still some corner cases, though:

  - you could have a remote with a slash in it; presumably we would
check that first and fallback to the DWIM behavior

  - These commands only handle a single remote at once, so something
like:

  git pull origin/foo other-remote/bar

is nonsensical. We'd have to catch and disallow multiple remotes.
Probably we could only kick in the DWIM when there is a single
argument (otherwise you're just repeating the remote name over and
over, at which point you might as well use the "remote [refspec...]"
syntax.

It seems like it's probably do-able.

I'm still undecided on whether it is a good idea or not. In one sense,
it does unify the syntax you use to refer to a remote branch. But it
also blurs the meanings. Normally "origin/master" refers only to your
local refs/remotes copy of what is on the remote, but this is blurring
the line. It's not clear to me if that reduces confusion (because you
don't have to care about that line anymore), or if it increases it
(because sometimes it _does_ matter, and somebody who doesn't learn the
difference between the two will get bitten later. Plus now there are
multiple ways of spelling the same thing).

> > git pull myremote/somebranch
> complains about the syntax; IMO it should either pull from that branch (and 
> merge if necessary) or complain instead that pulling from a different branch 
> is not supported (and suggest using merge).

Reading this, I wonder if I've misinterpreted your request. It sounds
like you want this to be the same as:

  git merge myremote/somebranch

which is at least consistent in the use of "remote/branch" syntax. But
weird, because you're asking "pull" not to pull. Or another way to think
of it is that "git pull foo/bar" effectively becomes "git pull .
foo/bar". Which seems like it may be potentially error-prone, especially
if you use slashes in your remote names.

-Peff

Re: [PATCH v4 1/2] lib-proto-disable: variable name fix

[Jeff King]

On Mon, Nov 07, 2016 at 12:40:28PM -0800, Brandon Williams wrote:

> On 11/07, Jeff King wrote:
> > On Mon, Nov 07, 2016 at 11:35:22AM -0800, Brandon Williams wrote:
> > 
> > > Small fix to use '$desc' instead of '$1' in lib-proto-disable.sh.
> > 
> > Even for a trivial fixup like this, I think it's good to say why.
> > Because what seems trivial and obvious to you while working on the patch
> > may not be so to a reviewer, or somebody reading it 6 months later.
> > 
> > Just something simple like:
> > 
> >   The test_proto function assigns the positional parameters to named
> >   variables, but then still refers to "$desc" as "$1". Using $desc is
> >   more readable and less error-prone.
> > 
> > -Peff
> 
> Alright will do.  Commit messages don't seem to be an area of strength
> for me, but I'm working on it! :D

It's possible that I'm overly picky about my commit messages, but that
does not stop me from trying to train an army of picky-commit-message
clones. :)

-Peff

Re: [PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick

[Stefan Beller]

On Mon, Nov 7, 2016 at 12:38 PM, David Turner  wrote:
>> -Original Message-
>> From: Stefan Beller [mailto:sbel...@google.com]
>> Sent: Monday, November 07, 2016 2:14 PM
>> To: David Turner
>> Cc: git@vger.kernel.org
>> Subject: Re: [PATCH] submodules: allow empty working-tree dirs in
>> merge/cherry-pick
>>
>> On Mon, Nov 7, 2016 at 10:31 AM, David Turner 
>> wrote:
>> > When a submodule is being merged or cherry-picked into a working tree
>> > that already contains a corresponding empty directory, do not record a
>> > conflict.
>> >
>> > One situation where this bug appears is:
>> >
>> > - Commit 1 adds a submodule
>>
>> "... at sub1" as inferred by text below.
>>
>> > - Commit 2 removes that submodule and re-adds it into a subdirectory
>> >(sub1 to sub1/sub1).
>> > - Commit 3 adds an unrelated file.
>> >
>> > Now the user checks out commit 1 (first deinitializing the submodule),
>> > and attempts to cherry-pick commit 3.  Previously, this would fail,
>> > because the incoming submodule sub1/sub1 would falsely conflict with
>> > the empty sub1 directory.
>>
>> So you'd want to achieve:
>>   $ # on commit 3:
>>   git checkout 
>>   git cherry-pick 
>>
>> which essentially moves the gitlink back to its original place (from
>> sub1/sub1 -> sub1).  This sounds reasonable.
>> But what if the submodule contains a (file/directory) named sub1? We'd
>> first remove the sub1/sub1 submodule (and even delete the inner
>> directory?), such that "sub1/"
>> becomes an empty dir, which is perfect for having a submodule right there
>> at "sub1/"
>
> I'm confused about the "what if" here.
>
> In our particular situation, the submodule in question was not initialized.

oops. That explains it. I somehow assumed we were talking about
initialized submodules.

>
> If your approach also fixes the same tests that mine fixes, then I am happy 
> to use your series over mine.  Please CC me so I can take a peek.

No, my series seems to be orthogonal to this one. I plan
on cc'ing you nevertheless as it is still nearby.

> basically nobody needs two copies of one submodule in the same repo.

IIRC this is how gitlinks were used in very early days :/
(kernel people were using gitlinks to track different kernel versions
and see if they were interoperable or working at all.
e.g. see d92a39590d1126e195f1bbccf182a2cdb79218e7, which
only makes sense (for the update command) if the referenced repository
contains references of all submodules, which either means a huge reference
pile that contains different projects at the same time, or the same project
at different versions.

>  I think that case fails for other reasons anyway.
>

Yes. I agree that the patch as-is is applicable. I did not try to oppose
your approach, but rather give some thoughts I had.

Stefan

Re: [PATCH v4 1/2] lib-proto-disable: variable name fix

[Brandon Williams]

On 11/07, Jeff King wrote:
> On Mon, Nov 07, 2016 at 11:35:22AM -0800, Brandon Williams wrote:
> 
> > Small fix to use '$desc' instead of '$1' in lib-proto-disable.sh.
> 
> Even for a trivial fixup like this, I think it's good to say why.
> Because what seems trivial and obvious to you while working on the patch
> may not be so to a reviewer, or somebody reading it 6 months later.
> 
> Just something simple like:
> 
>   The test_proto function assigns the positional parameters to named
>   variables, but then still refers to "$desc" as "$1". Using $desc is
>   more readable and less error-prone.
> 
> -Peff

Alright will do.  Commit messages don't seem to be an area of strength
for me, but I'm working on it! :D

-- 
Brandon Williams

Re: [PATCH v4 2/2] transport: add protocol policy config option

[Jeff King]

On Mon, Nov 07, 2016 at 11:35:23AM -0800, Brandon Williams wrote:

> Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
> specify a whitelist of protocols to be used in clone/fetch/push
> commands.  This patch introduces new configuration options for more
> fine-grained control for allowing/disallowing protocols.  This also has
> the added benefit of allowing easier construction of a protocol
> whitelist on systems where setting an environment variable is
> non-trivial.
> 
> Now users can specify a policy to be used for each type of protocol via
> the 'protocol..allow' config option.  A default policy for all
> unconfigured protocols can be set with the 'protocol.allow' config
> option.  If no user configured default is made git, by default, will
> allow known-safe protocols (http, https, git, ssh, file), disallow

A minor nit, but in "If no user configured default is made git, by
default, will..." the second "by default" is redundant. And possibly
misleading. This _is_ the default case, there is no other way to change
it. :)

> +protocol..allow::
> + Set a policy to be used by protocol  with clone/fetch/push 
> commands.

`` isn't defined here at all.  I still think the list of protocols
should go here, but at the very least, you need to point the user to the
existing list in git(1).

>  `GIT_ALLOW_PROTOCOL`::
> - If set, provide a colon-separated list of protocols which are
> - allowed to be used with fetch/push/clone. This is useful to
> - restrict recursive submodule initialization from an untrusted
> - repository. Any protocol not mentioned will be disallowed (i.e.,
> - this is a whitelist, not a blacklist). If the variable is not
> - set at all, all protocols are enabled.  The protocol names
> - currently used by git are:
> + The preferred way to configure allowed protocols is done through the
> + config interface, though this setting takes precedences.  See

s/precedences/precedence/.

I actually wonder if we should even drop "the preferred way" here. I had
initially thought we would want it just for backwards-compatibility, but
I actually think it is useful in its own right as a shorthand for more
complicated config (and since we have to keep it around effectively
forever anyway, there's no real cost to continuing to call it a feature
versus a deprecated feature).

I'm including a squashable patch at the end of this email with suggested
wording (and which also moves the protocol list).

> + test_expect_success "clone $desc (env var has precedence)" '
> + rm -rf tmp.git &&
> + (
> + GIT_ALLOW_PROTOCOL=none &&
> + export GIT_ALLOW_PROTOCOL &&
> + test_must_fail git -c protocol.$proto.allow=always 
> clone --bare "$url" tmp.git
> + )
> + '

This test is a good addition in this round.

I suppose we could test also that GIT_ALLOW_PROTOCOL overrides
protocol.allow, but I'm not sure if there is a point. If git were a
black box, it's a thing I might check, but we know from the design that
this is an unlikely bug (and that the implementation is unlikely to
change in a way to cause it). So I could go either way.

> [...]

The rest of it looks good to me.

Squashable documentation suggestions are below.

-Peff

---
diff --git a/Documentation/config.txt b/Documentation/config.txt
index e89b33f9e..a9dc23f82 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2331,7 +2331,28 @@ protocol.allow::
 --
 
 protocol..allow::
-   Set a policy to be used by protocol  with clone/fetch/push 
commands.
+   Set a policy to be used by protocol `` with clone/fetch/push
+   commands. See `protocol.allow` above for the available policies.
++
+The protocol names currently used by git are:
++
+--
+  - `file`: any local file-based path (including `file://` URLs,
+   or local paths)
+
+  - `git`: the anonymous git protocol over a direct TCP
+connection (or proxy, if configured)
+
+  - `ssh`: git over ssh (including `host:path` syntax,
+`ssh://`, etc).
+
+  - `http`: git over http, both "smart http" and "dumb http".
+Note that this does _not_ include `https`; if you want to configure
+both, you must do so individually.
+
+  - any external helpers are named by their protocol (e.g., use
+`hg` to allow the `git-remote-hg` helper)
+--
 
 pull.ff::
By default, Git does not create an extra merge commit when merging
diff --git a/Documentation/git.txt b/Documentation/git.txt
index c9823e34a..c52cec840 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1150,30 +1150,13 @@ of clones and fetches.
cloning a repository to make a backup).
 
 `GIT_ALLOW_PROTOCOL`::
-   The preferred way to configure allowed protocols is done through the
-   config interface, though this setting takes precedences.  See
-   linkgit:git-config[1] for more details.  If set to a colon-separated
-   

RE: [PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick

[David Turner]

> -Original Message-
> From: Stefan Beller [mailto:sbel...@google.com]
> Sent: Monday, November 07, 2016 2:14 PM
> To: David Turner
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH] submodules: allow empty working-tree dirs in
> merge/cherry-pick
> 
> On Mon, Nov 7, 2016 at 10:31 AM, David Turner 
> wrote:
> > When a submodule is being merged or cherry-picked into a working tree
> > that already contains a corresponding empty directory, do not record a
> > conflict.
> >
> > One situation where this bug appears is:
> >
> > - Commit 1 adds a submodule
> 
> "... at sub1" as inferred by text below.
> 
> > - Commit 2 removes that submodule and re-adds it into a subdirectory
> >(sub1 to sub1/sub1).
> > - Commit 3 adds an unrelated file.
> >
> > Now the user checks out commit 1 (first deinitializing the submodule),
> > and attempts to cherry-pick commit 3.  Previously, this would fail,
> > because the incoming submodule sub1/sub1 would falsely conflict with
> > the empty sub1 directory.
> 
> So you'd want to achieve:
>   $ # on commit 3:
>   git checkout 
>   git cherry-pick 
> 
> which essentially moves the gitlink back to its original place (from
> sub1/sub1 -> sub1).  This sounds reasonable.
> But what if the submodule contains a (file/directory) named sub1? We'd
> first remove the sub1/sub1 submodule (and even delete the inner
> directory?), such that "sub1/"
> becomes an empty dir, which is perfect for having a submodule right there
> at "sub1/"

I'm confused about the "what if" here.

In our particular situation, the submodule in question was not initialized.  
Basically, the submodule move by developer A messed up developer B's rebase, 
where developers A and B had been working on completely disjoint sets of 
submodules.  If it had been initialized, that might be a different story.  It 
would be somewhat less surprising, and thus probably OK.  The "first 
deinitializing the submodule" bit above, I think, describes the situation.

If the "what if" you are worried about is corruption caused the move of 
sub1/sub1 into sub1, don't worry about it.  sub1/ would still contain the .git 
file, and so would not be empty.  Even if this patch were really wacky, the 
worst it could do is delete already-empty directories.

> > This patch ignores the empty sub1 directory, fixing the bug.  We only
> > ignore the empty directory if the object being emplaced is a
> > submodule, which expects an empty directory.
> >
> > Signed-off-by: David Turner 
> > ---
> >  merge-recursive.c   | 21 +++--
> >  t/t3030-merge-recursive.sh  |  4 ++--  t/t3426-rebase-submodule.sh |
> > 3 ---
> >  3 files changed, 17 insertions(+), 11 deletions(-)
> >
> > Note that there are four calls to dir_in_way, and only two of them
> > have changed their semantics.  This is because the merge code is quite
> > complicated, and I don't fully understand it.
> 
> A good approach. I was trying to haggle with unpack-trees.c and the
> merging code and put way more on my plate than I could eat in one sitting.
> Trying to get the mess sorted now to prepare a patch series this week.

If your approach also fixes the same tests that mine fixes, then I am happy to 
use your series over mine.  Please CC me so I can take a peek.

> > So I did not have time
> > to analyze the remaining calls to see whether they, too, should be
> > changed.
> 
> The call in line 1205 (in handle_file, which is only called from
> conflict_rename_rename_1to2) may be relevant if we move around submodules
> on the same level and modifying it in different branches.
> However I think preserving current behavior is ok.

So, the case there would be moving sub1 to sub2, where sub2 was previously a 
different submodule?  It appears that this works at least after my patch, if 
not before.  But I gather from the name rename_1to2 that I actually need to 
copy the submodule not move it?  This seems like such a rare case that I don't 
actually need to handle it; basically nobody needs two copies of one submodule 
in the same repo.  I think that case fails for other reasons anyway.

> The other one in handle_change_delete also doesn't look obvious one way or
> another, so I'd stick with current behavior.

This appears to be implicated in the t6022 test that I mentioned -- if I change 
empty_ok unconditionally to 1, the test fails.

> >For me, there are no test failures either way, indicating  that
> >probably these cases are rare.
> 
> The tests have to be crafted for this specific code pattern,
> 
> >
> > The reason behind the empty_ok parameter (as opposed to just always
> > allowing empy directories to be blown away) is found in t6022's 'pair
> > rename to parent of other (D/F conflicts) w/ untracked dir'.  This
> > test would fail with an unconditional rename, because it wouldn't
> > generate the conflict file.
> 
> Or the submodule from your commit message contains a "sub1/..." itself.

See above.

Re: [PATCH v4 1/2] lib-proto-disable: variable name fix

[Jeff King]

On Mon, Nov 07, 2016 at 11:35:22AM -0800, Brandon Williams wrote:

> Small fix to use '$desc' instead of '$1' in lib-proto-disable.sh.

Even for a trivial fixup like this, I think it's good to say why.
Because what seems trivial and obvious to you while working on the patch
may not be so to a reviewer, or somebody reading it 6 months later.

Just something simple like:

  The test_proto function assigns the positional parameters to named
  variables, but then still refers to "$desc" as "$1". Using $desc is
  more readable and less error-prone.

-Peff

[PATCH v4 1/2] lib-proto-disable: variable name fix

[Brandon Williams]

Small fix to use '$desc' instead of '$1' in lib-proto-disable.sh.

Signed-off-by: Brandon Williams 
---
 t/lib-proto-disable.sh | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
index b0917d9..be88e9a 100644
--- a/t/lib-proto-disable.sh
+++ b/t/lib-proto-disable.sh
@@ -9,7 +9,7 @@ test_proto () {
proto=$2
url=$3
 
-   test_expect_success "clone $1 (enabled)" '
+   test_expect_success "clone $desc (enabled)" '
rm -rf tmp.git &&
(
GIT_ALLOW_PROTOCOL=$proto &&
@@ -18,7 +18,7 @@ test_proto () {
)
'
 
-   test_expect_success "fetch $1 (enabled)" '
+   test_expect_success "fetch $desc (enabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=$proto &&
@@ -27,7 +27,7 @@ test_proto () {
)
'
 
-   test_expect_success "push $1 (enabled)" '
+   test_expect_success "push $desc (enabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=$proto &&
@@ -36,7 +36,7 @@ test_proto () {
)
'
 
-   test_expect_success "push $1 (disabled)" '
+   test_expect_success "push $desc (disabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=none &&
@@ -45,7 +45,7 @@ test_proto () {
)
'
 
-   test_expect_success "fetch $1 (disabled)" '
+   test_expect_success "fetch $desc (disabled)" '
(
cd tmp.git &&
GIT_ALLOW_PROTOCOL=none &&
@@ -54,7 +54,7 @@ test_proto () {
)
'
 
-   test_expect_success "clone $1 (disabled)" '
+   test_expect_success "clone $desc (disabled)" '
rm -rf tmp.git &&
(
GIT_ALLOW_PROTOCOL=none &&
-- 
2.8.0.rc3.226.g39d4020

[PATCH v4 2/2] transport: add protocol policy config option

[Brandon Williams]

Previously the `GIT_ALLOW_PROTOCOL` environment variable was used to
specify a whitelist of protocols to be used in clone/fetch/push
commands.  This patch introduces new configuration options for more
fine-grained control for allowing/disallowing protocols.  This also has
the added benefit of allowing easier construction of a protocol
whitelist on systems where setting an environment variable is
non-trivial.

Now users can specify a policy to be used for each type of protocol via
the 'protocol..allow' config option.  A default policy for all
unconfigured protocols can be set with the 'protocol.allow' config
option.  If no user configured default is made git, by default, will
allow known-safe protocols (http, https, git, ssh, file), disallow
known-dangerous protocols (ext), and have a default policy of `user` for
all other protocols.

The supported policies are `always`, `never`, and `user`.  The `user`
policy can be used to configure a protocol to be usable when explicitly
used by a user, while disallowing it for commands which run
clone/fetch/push commands without direct user intervention (e.g.
recursive initialization of submodules).  Commands which can potentially
clone/fetch/push from untrusted repositories without user intervention
can export `GIT_PROTOCOL_FROM_USER` with a value of '0' to prevent
protocols configured to the `user` policy from being used.

Fix remote-ext tests to use the new config to allow the ext
protocol to be tested.

Based on a patch by Jeff King 

Signed-off-by: Brandon Williams 
---
 Documentation/config.txt |  25 
 Documentation/git.txt|  21 ---
 git-submodule.sh |  12 ++--
 t/lib-proto-disable.sh   | 129 +--
 t/t5509-fetch-push-namespaces.sh |   1 +
 t/t5802-connect-helper.sh|   1 +
 transport.c  |  75 ++-
 7 files changed, 242 insertions(+), 22 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 27069ac..0b1c186 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2308,6 +2308,31 @@ pretty.::
Note that an alias with the same name as a built-in format
will be silently ignored.
 
+protocol.allow::
+   If set, provide a user defined default policy for all protocols which
+   don't explicitly have a policy (`protocol..allow`).  By default,
+   if unset, known-safe protocols (http, https, git, ssh, file) have a
+   default policy of `always`, known-dangerous protocols (ext) have a
+   default policy of `never`, and all other protocols have a default
+   policy of `user`.  Supported policies:
++
+--
+
+* `always` - protocol is always able to be used.
+
+* `never` - protocol is never able to be used.
+
+* `user` - protocol is only able to be used when `GIT_PROTOCOL_FROM_USER` is
+  either unset or has a value of 1.  This policy should be used when you want a
+  protocol to be directly usable by the user but don't want it used by 
commands which
+  execute clone/fetch/push commands without user input, e.g. recursive
+  submodule initialization.
+
+--
+
+protocol..allow::
+   Set a policy to be used by protocol  with clone/fetch/push 
commands.
+
 pull.ff::
By default, Git does not create an extra merge commit when merging
a commit that is a descendant of the current commit. Instead, the
diff --git a/Documentation/git.txt b/Documentation/git.txt
index ab7215e..c9823e3 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1150,13 +1150,14 @@ of clones and fetches.
cloning a repository to make a backup).
 
 `GIT_ALLOW_PROTOCOL`::
-   If set, provide a colon-separated list of protocols which are
-   allowed to be used with fetch/push/clone. This is useful to
-   restrict recursive submodule initialization from an untrusted
-   repository. Any protocol not mentioned will be disallowed (i.e.,
-   this is a whitelist, not a blacklist). If the variable is not
-   set at all, all protocols are enabled.  The protocol names
-   currently used by git are:
+   The preferred way to configure allowed protocols is done through the
+   config interface, though this setting takes precedences.  See
+   linkgit:git-config[1] for more details.  If set to a colon-separated
+   list of protocols, behave as if `protocol.allow` is set to `never`, and
+   each of the listed protocols has `protocol..allow` set to
+   `always`.  In other words, any protocol not mentioned will be
+   disallowed (i.e., this is a whitelist, not a blacklist).  The protocol
+   names currently used by git are:
 
  - `file`: any local file-based path (including `file://` URLs,
or local paths)
@@ -1174,6 +1175,12 @@ of clones and fetches.
  - any external helpers are named by their protocol (e.g., use
`hg` to allow the 

Re: [PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick

[Stefan Beller]

On Mon, Nov 7, 2016 at 10:31 AM, David Turner  wrote:
> When a submodule is being merged or cherry-picked into a working
> tree that already contains a corresponding empty directory, do not
> record a conflict.
>
> One situation where this bug appears is:
>
> - Commit 1 adds a submodule

"... at sub1" as inferred by text below.

> - Commit 2 removes that submodule and re-adds it into a subdirectory
>(sub1 to sub1/sub1).
> - Commit 3 adds an unrelated file.
>
> Now the user checks out commit 1 (first deinitializing the submodule),
> and attempts to cherry-pick commit 3.  Previously, this would fail,
> because the incoming submodule sub1/sub1 would falsely conflict with
> the empty sub1 directory.

So you'd want to achieve:
  $ # on commit 3:
  git checkout 
  git cherry-pick 

which essentially moves the gitlink back to its original place
(from sub1/sub1 -> sub1).  This sounds reasonable.
But what if the submodule contains a (file/directory)
named sub1? We'd first remove the sub1/sub1 submodule
(and even delete the inner directory?), such that "sub1/"
becomes an empty dir, which is perfect for having a
submodule right there at "sub1/"

>
> This patch ignores the empty sub1 directory, fixing the bug.  We only
> ignore the empty directory if the object being emplaced is a
> submodule, which expects an empty directory.
>
> Signed-off-by: David Turner 
> ---
>  merge-recursive.c   | 21 +++--
>  t/t3030-merge-recursive.sh  |  4 ++--
>  t/t3426-rebase-submodule.sh |  3 ---
>  3 files changed, 17 insertions(+), 11 deletions(-)
>
> Note that there are four calls to dir_in_way, and only two of them
> have changed their semantics.  This is because the merge code is quite
> complicated, and I don't fully understand it.

A good approach. I was trying to haggle with unpack-trees.c and
the merging code and put way more on my plate than I could eat
in one sitting. Trying to get the mess sorted now to prepare a patch
series this week.

> So I did not have time
> to analyze the remaining calls to see whether they, too, should be
> changed.

The call in line 1205 (in handle_file, which is only called from
conflict_rename_rename_1to2) may be relevant if we move around
submodules on the same level and modifying it in different branches.
However I think preserving current behavior is ok.

The other one in handle_change_delete also doesn't look obvious
one way or another, so I'd stick with current behavior.

>For me, there are no test failures either way, indicating
> that probably these cases are rare.

The tests have to be crafted for this specific code pattern,

>
> The reason behind the empty_ok parameter (as opposed to just always
> allowing empy directories to be blown away) is found in t6022's 'pair
> rename to parent of other (D/F conflicts) w/ untracked dir'.  This
> test would fail with an unconditional rename, because it wouldn't
> generate the conflict file.

Or the submodule from your commit message contains a "sub1/..." itself.

>  It's not clear how important that
> behavior is (I do not recall ever noticing the file~branch thing
> before), but it seemed better to preserve it in case it was important.
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 9041c2f..e64b48b 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -664,7 +664,13 @@ static char *unique_path(struct merge_options *o, const 
> char *path, const char *
> return strbuf_detach(, NULL);
>  }
>
> -static int dir_in_way(const char *path, int check_working_copy)
> +/**
> + * Check whether a directory in the index is in the way of an incoming
> + * file.  Return 1 if so.  If check_working_copy is non-zero, also
> + * check the working directory.  If empty_ok is non-zero, also return
> + * 0 in the case where the working-tree dir exists but is empty.
> + */

Thanks for the documenting comment! This is probably fine as is with just
two boolean parameters. If we'd add more, we might have thought about
adding a flags parameter with bits for each flag.

Looks good to me,
Thanks,
Stefan

Re: [PATCH v3] transport: add protocol policy config option

[Brandon Williams]

On 11/04, Jeff King wrote:
> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index 27069ac..5d845c4 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -2308,6 +2308,31 @@ pretty.::
> > Note that an alias with the same name as a built-in format
> > will be silently ignored.
> >  
> > +protocol.allow::
> > +   If set, provide a user defined default policy for all protocols which
> > +   don't explicitly have a policy (protocol..allow).  By default,
> > +   if unset, known-safe protocols (http, https, git, ssh, file) have a
> > +   default policy of `always`, known-dangerous protocols (ext) have a
> > +   default policy of `never`, and all other protocols have a default policy
> > +   of `user`.  Supported policies:
> > ++
> > +--
> > +
> > +* `always` - protocol is always able to be used.
> > +
> > +* `never` - protocol is never able to be used.
> > +
> > +* `user` - protocol is only able to be used when `GIT_PROTOCOL_FROM_USER` 
> > is
> > +  either unset or has a value of 1.  This policy should be used when you 
> > want a
> > +  protocol to be usable by the user but don't want it used by commands 
> > which
> > +  execute clone/fetch/pull commands without user input, e.g. recursive
> > +  submodule initialization.
> 
> Makes sense. I wonder if it would be good to emphasize _directly_ usable
> here. I.e., "...when you want a protocol to be directly usable by the
> user but don't want...".
> 
> Should clone/fetch/pull also include push?

You're right, that should really have been clone/fetch/push.

> 
> > +protocol..allow::
> > +   Set a policy to be used by protocol  with clone/fetch/pull 
> > commands.
> > +
> 
> Nice that this matches protocol.allow, so we don't need to re-explain
> that.
> 
> Should the list of protocols be here? I know they're covered under
> GIT_ALLOW_PROTOCOL already, but if this is the preferred system, we
> should probably explain them here, and then just have GIT_ALLOW_PROTOCOL
> refer the user.

Right now the list of protocols under GIT_ALLOW_PROTOCOL looks like it
has a bit more documentation with how the colon list works.  The
protocols are also mentioned above with their default behaviour.

> 
> > diff --git a/Documentation/git.txt b/Documentation/git.txt
> > index ab7215e..ab25580 100644
> > --- a/Documentation/git.txt
> > +++ b/Documentation/git.txt
> > @@ -1150,13 +1150,13 @@ of clones and fetches.
> > cloning a repository to make a backup).
> >  
> >  `GIT_ALLOW_PROTOCOL`::
> > -   If set, provide a colon-separated list of protocols which are
> > -   allowed to be used with fetch/push/clone. This is useful to
> > -   restrict recursive submodule initialization from an untrusted
> > -   repository. Any protocol not mentioned will be disallowed (i.e.,
> > -   this is a whitelist, not a blacklist). If the variable is not
> > -   set at all, all protocols are enabled.  The protocol names
> > -   currently used by git are:
> > +   The new way to configure allowed protocols is done through the config
> > +   interface, though this setting takes precedences.  See
> > +   linkgit:git-config[1] for more details.  If set, provide a
> > +   colon-separated list of protocols which are allowed to be used with
> > +   fetch/push/clone.  Any protocol not mentioned will be disallowed (i.e.,
> > +   this is a whitelist, not a blacklist).  The protocol names currently
> > +   used by git are:
> 
> I wonder if we can explain this in terms of the config system. Something
> like:
> 
>   If set to a colon-separated list of zero or more protocols, behave as
>   if `protocol.allow` is set to `never`, and each of the listed
>   protocols has `protocol.$protocol.allow` set to `always`.

Yeah that makes sense.

> 
> > +`GIT_PROTOCOL_FROM_USER`::
> > +   Set to 0 to prevent protocols used by fetch/push/clone which are
> > +   configured to the `user` state.  This is useful to restrict recursive
> > +   submodule initialization from an untrusted repository.  See
> > +   linkgit:git-config[1] for more details.
> 
> Under "this is useful", it may make sense to make it clear that external
> programs can use this, too. Something like:
> 
>   It may also be useful for programs which feed potentially-untrusted
>   URLs to git commands.

I'll put that in too.

> 
> > diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh
> > index b0917d9..5950fbf 100644
> > --- a/t/lib-proto-disable.sh
> > +++ b/t/lib-proto-disable.sh
> > @@ -1,15 +1,12 @@
> >  # Test routines for checking protocol disabling.
> >  
> > -# test cloning a particular protocol
> > -#   $1 - description of the protocol
> > -#   $2 - machine-readable name of the protocol
> > -#   $3 - the URL to try cloning
> > -test_proto () {
> > +# Test clone/fetch/push with GIT_ALLOW_PROTOCOL whitelist
> > +test_whitelist () {
> > desc=$1
> > proto=$2
> > url=$3
> >  
> > -   test_expect_success "clone $1 (enabled)" '
> > +   test_expect_success "clone $desc (enabled)" '
> 

[PATCH 3/3] gitk: clear array 'commitinfo' on reload

[Markus Hitter]

>From 8359452f426c68cc02250f25f20eaaacd2ddd001 Mon Sep 17 00:00:00 2001
From: Markus Hitter 
Date: Mon, 7 Nov 2016 19:02:51 +0100
Subject: [PATCH 3/3] gitk: clear array 'commitinfo' on reload

After a reload we might have an entirely different set of commits,
so keeping all of them leaks memory. Remove them all because
re-creating them is not more expensive than testing wether they're
still valid. Lazy (re-)creation is already well established, so
a missing entry can't cause harm.

Signed-off-by: Markus Hitter 
---
 gitk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/gitk b/gitk
index 518a4ce..aef6db6 100755
--- a/gitk
+++ b/gitk
@@ -588,7 +588,7 @@ proc updatecommits {} {
 proc reloadcommits {} {
 global curview viewcomplete selectedline currentid thickerline
 global showneartags treediffs commitinterest cached_commitrow
-global targetid
+global targetid commitinfo
 
 set selid {}
 if {$selectedline ne {}} {
@@ -609,6 +609,7 @@ proc reloadcommits {} {
getallcommits
 }
 clear_display
+unset -nocomplain commitinfo
 unset -nocomplain commitinterest
 unset -nocomplain cached_commitrow
 unset -nocomplain targetid
-- 
2.9.3

[PATCH 2/3] gitk: remove closed file descriptors from $blobdifffd

[Markus Hitter]

>From 0a463fcd977dc9558835c373e24a095e35ca3c82 Mon Sep 17 00:00:00 2001
From: Markus Hitter 
Date: Mon, 7 Nov 2016 16:01:17 +0100
Subject: [PATCH 2/3] gitk: remove closed file descriptors from $blobdifffd

One shouldn't have descriptors of already closed files around.

The first idea to deal with this (previously) ever growing array
was to remove it entirely, but it's needed to detect start of a
new diff with ths old diff not yet done. This happens when a user
clicks on the same commit in the commit list repeatedly without
delay.

Signed-off-by: Markus Hitter 
---
 gitk | 5 +
 1 file changed, 5 insertions(+)

diff --git a/gitk b/gitk
index 8654e29..518a4ce 100755
--- a/gitk
+++ b/gitk
@@ -8069,7 +8069,11 @@ proc getblobdiffline {bdf ids} {
 $ctext conf -state normal
 while {[incr nr] <= 1000 && [gets $bdf line] >= 0} {
if {$ids != $diffids || $bdf != $blobdifffd($ids)} {
+   # Older diff read. Abort it.
catch {close $bdf}
+   if {$ids != $diffids} {
+   array unset blobdifffd $ids
+   }
return 0
}
parseblobdiffline $ids $line
@@ -8078,6 +8082,7 @@ proc getblobdiffline {bdf ids} {
 blobdiffmaybeseehere [eof $bdf]
 if {[eof $bdf]} {
catch {close $bdf}
+   array unset blobdifffd $ids
return 0
 }
 return [expr {$nr >= 1000? 2: 1}]
-- 
2.9.3

[PATCH 1/3] gitk: turn off undo manager in the text widget

[Markus Hitter]

>From e965e1deb9747bbc2b40dc2de95afb65aee9f7fd Mon Sep 17 00:00:00 2001
From: Markus Hitter 
Date: Sun, 6 Nov 2016 20:38:03 +0100
Subject: [PATCH 1/3] gitk: turn off undo manager in the text widget

The diff text widget is read-only, so there's zero point in
building an undo stack. This change reduces memory consumption of
this widget by about 95%.

Memory usage of the whole program for viewing a reference commit
before; 579'692'744 bytes, after: 32'724'446 bytes.

Test procedure:

 - Choose a largish commit and check it out. In this case one with
   90'802 lines, 5'006'902 bytes.

 - Have a Tcl version with memory debugging enabled. This is,
   build one with --enable-symbols=mem passed to configure.

 - Instrument Gitk to regularly show a memory dump. E.g. by adding
   these code lines at the very bottom:

 proc memDump {} {
 catch {
 set output [memory info]
 puts $output
 }

 after 3000 memDump
 }

 memDump

 - Start Gitk, it'll load this largish commit into the diff text
   field automatically (because it's the current commit).

 - Wait until memory consumption levels out and note the numbers.

Note that the numbers reported by [memory info] are much smaller
than the ones reported in 'top' (1.75 GB vs. 105 MB in this case),
likely due to all the instrumentation coming with the debug
version of Tcl.

Signed-off-by: Markus Hitter 
---
 gitk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gitk b/gitk
index 805a1c7..8654e29 100755
--- a/gitk
+++ b/gitk
@@ -2403,7 +2403,7 @@ proc makewindow {} {
 
 set ctext .bleft.bottom.ctext
 text $ctext -background $bgcolor -foreground $fgcolor \
-   -state disabled -font textfont \
+   -state disabled -undo 0 -font textfont \
-yscrollcommand scrolltext -wrap none \
-xscrollcommand ".bleft.bottom.sbhorizontal set"
 if {$have_tk85} {
-- 
2.9.3

[PATCH 0/3] gitk: memory consumption improvements

[Markus Hitter]

List, Paul,

after searching for a while on why Gitk sometimes consumes exorbitant amounts 
of memory I found a pair of minor issues and also a big one: the text widget 
comes with an unlimited undo manager, which is turned on be default. 
Considering that each line is inserted seperately, this piles up a huuuge undo 
stack ... for a read-only text widget. Simply turning off this undo manager 
saves about 95% of memory when viewing large commits (with tens of thousands of 
diff lines).

3 patches are about to follow:

 - turn off the undo manager,

 - forget already closed file descriptors and

 - forget the 'commitinfo' array on a reload to enforce reloading it.

I hope this finds you appreciation.


Markus

-- 
- - - - - - - - - - - - - - - - - - -
Dipl. Ing. (FH) Markus Hitter
http://www.jump-ing.de/

Re: [PATCH v3] transport: add protocol policy config option

[Brandon Williams]

On 11/04, Stefan Beller wrote:
> By default, if unset, ... have a default policy ...
> sounds strange. How about just dropping the first 4 words here:
> 
>  Known-safe protocols (http, https, git, ssh, file) have a
>  default policy of `always`, known-dangerous protocols (ext) have a
>  default policy of `never`, and all other protocols have a default policy
>  of `user`.  Supported policies:
> 
> 
> What happens if protocol.allow is set to true?

That wouldn't be allowed, its an unknown value as the only permitted
values are always, never, and user.

> 
> 
> 
> > +   if unset, known-safe protocols (http, https, git, ssh, file) have a
> > +   default policy of `always`, known-dangerous protocols (ext) have a
> > +   default policy of `never`, and all other protocols have a default 
> > policy
> > +   of `user`.  Supported policies:
> > ++
> > +--
> > +
> > +* `always` - protocol is always able to be used.
> > +
> > +* `never` - protocol is never able to be used.
> > +
> > +* `user` - protocol is only able to be used when `GIT_PROTOCOL_FROM_USER` 
> > is
> > +  either unset or has a value of 1.  This policy should be used when you 
> > want a
> > +  protocol to be usable by the user but don't want it used by commands 
> > which
> > +  execute clone/fetch/pull commands without user input, e.g. recursive
> > +  submodule initialization.
> > +
> > +--
> > +
> > +protocol..allow::
> > +   Set a policy to be used by protocol  with clone/fetch/pull 
> > commands.
> 
> How does this interact with protocol.allow?
> 
> When protocol.allow is set, this overrides the specific protocol.
> If protocol is not set, it overrides the specific protocol as well(?)

protocol.allow is a default for protocols which don't have a specific
protocol..allow entry

> > +   test_expect_success "clone $desc (disabled)" '
> > +   rm -rf tmp.git &&
> > +   test_must_fail git clone --bare "$url" tmp.git
> > +   '
> 
> 
> I could not spot a test for GIT_ALLOW_PROTOCOL overriding
> any protocol*allow policy. Is that also worth testing? (for
> backwards compatibility of tools that make use of GIT_ALLOW_PROTOCOL
> but the user already setup a policy.

I can add in one quick test for that.

-- 
Brandon Williams

Re: git submodule add broken (2.11.0-rc1): Cannot open git-sh-i18n

[Stefan Beller]

On Mon, Nov 7, 2016 at 10:30 AM, Anthony Sottile  wrote:
> This has worked great up until now (and is very convenient for trying things
> out without blowing away the system installation).  What changed?
>

(Just guessing myself:)

$ git log --grep git-sh-i18n v2.10.0..v2.11.0-rc0
commit da14d73d5eacfb2fa9d054f94d9eecb2244c3ce5
Merge: 2f445c17e5 1073094f30
Author: Junio C Hamano 
Date:   Mon Oct 31 13:15:25 2016 -0700

Merge branch 'ak/sh-setup-dot-source-i18n-fix'

Recent update to git-sh-setup (a library of shell functions that
are used by our in-tree scripted Porcelain commands) included
another shell library git-sh-i18n without specifying where it is,
relying on the $PATH.  This has been fixed to be more explicit by
prefixing $(git --exec-path) output in front.

* ak/sh-setup-dot-source-i18n-fix:
  git-sh-setup: be explicit where to dot-source git-sh-i18n from.

commit 1073094f30a8dd5ae49f2146f587085c4fe86410
Author: Anders Kaseorg 
Date:   Sat Oct 29 22:10:02 2016 -0400

git-sh-setup: be explicit where to dot-source git-sh-i18n from.

d323c6b641 ("i18n: git-sh-setup.sh: mark strings for translation",
2016-06-17) started to dot-source git-sh-i18n shell script library,
assuming that $PATH is already adjusted for our scripts, namely,
$GIT_EXEC_PATH is at the beginning of $PATH.

Old contrib scripts like contrib/convert-grafts-to-replace-refs.sh
and contrib/rerere-train.sh and third-party scripts like guilt may
however be using this as ". $(git --exec-path)/git-sh-setup",
without satisfying that assumption.  Be more explicit by specifying
its path prefixed with "$(git --exec-path)/". to be safe.

While we’re here, move the sourcing of git-sh-i18n below the shell
portability fixes.

Signed-off-by: Anders Kaseorg 
Signed-off-by: Junio C Hamano 

[PATCH] submodules: allow empty working-tree dirs in merge/cherry-pick

[David Turner]

When a submodule is being merged or cherry-picked into a working
tree that already contains a corresponding empty directory, do not
record a conflict.

One situation where this bug appears is:

- Commit 1 adds a submodule
- Commit 2 removes that submodule and re-adds it into a subdirectory
   (sub1 to sub1/sub1).
- Commit 3 adds an unrelated file.

Now the user checks out commit 1 (first deinitializing the submodule),
and attempts to cherry-pick commit 3.  Previously, this would fail,
because the incoming submodule sub1/sub1 would falsely conflict with
the empty sub1 directory.

This patch ignores the empty sub1 directory, fixing the bug.  We only
ignore the empty directory if the object being emplaced is a
submodule, which expects an empty directory.

Signed-off-by: David Turner 
---
 merge-recursive.c   | 21 +++--
 t/t3030-merge-recursive.sh  |  4 ++--
 t/t3426-rebase-submodule.sh |  3 ---
 3 files changed, 17 insertions(+), 11 deletions(-)

Note that there are four calls to dir_in_way, and only two of them
have changed their semantics.  This is because the merge code is quite
complicated, and I don't fully understand it.  So I did not have time
to analyze the remaining calls to see whether they, too, should be
changed.  For me, there are no test failures either way, indicating
that probably these cases are rare.

The reason behind the empty_ok parameter (as opposed to just always
allowing empy directories to be blown away) is found in t6022's 'pair
rename to parent of other (D/F conflicts) w/ untracked dir'.  This
test would fail with an unconditional rename, because it wouldn't
generate the conflict file.  It's not clear how important that
behavior is (I do not recall ever noticing the file~branch thing
before), but it seemed better to preserve it in case it was important.

diff --git a/merge-recursive.c b/merge-recursive.c
index 9041c2f..e64b48b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -664,7 +664,13 @@ static char *unique_path(struct merge_options *o, const 
char *path, const char *
return strbuf_detach(, NULL);
 }
 
-static int dir_in_way(const char *path, int check_working_copy)
+/**
+ * Check whether a directory in the index is in the way of an incoming
+ * file.  Return 1 if so.  If check_working_copy is non-zero, also
+ * check the working directory.  If empty_ok is non-zero, also return
+ * 0 in the case where the working-tree dir exists but is empty.
+ */
+static int dir_in_way(const char *path, int check_working_copy, int empty_ok)
 {
int pos;
struct strbuf dirpath = STRBUF_INIT;
@@ -684,7 +690,8 @@ static int dir_in_way(const char *path, int 
check_working_copy)
}
 
strbuf_release();
-   return check_working_copy && !lstat(path, ) && S_ISDIR(st.st_mode);
+   return check_working_copy && !lstat(path, ) && S_ISDIR(st.st_mode) &&
+   !(empty_ok && is_empty_dir(path));
 }
 
 static int was_tracked(const char *path)
@@ -1062,7 +1069,7 @@ static int handle_change_delete(struct merge_options *o,
 {
char *renamed = NULL;
int ret = 0;
-   if (dir_in_way(path, !o->call_depth)) {
+   if (dir_in_way(path, !o->call_depth, 0)) {
renamed = unique_path(o, path, a_oid ? o->branch1 : o->branch2);
}
 
@@ -1195,7 +1202,7 @@ static int handle_file(struct merge_options *o,
remove_file(o, 0, rename->path, 0);
dst_name = unique_path(o, rename->path, cur_branch);
} else {
-   if (dir_in_way(rename->path, !o->call_depth)) {
+   if (dir_in_way(rename->path, !o->call_depth, 0)) {
dst_name = unique_path(o, rename->path, cur_branch);
output(o, 1, _("%s is a directory in %s adding as %s 
instead"),
   rename->path, other_branch, dst_name);
@@ -1704,7 +1711,8 @@ static int merge_content(struct merge_options *o,
 o->branch2 == rename_conflict_info->branch1) ?
pair1->two->path : pair1->one->path;
 
-   if (dir_in_way(path, !o->call_depth))
+   if (dir_in_way(path, !o->call_depth,
+  S_ISGITLINK(pair1->two->mode)))
df_conflict_remains = 1;
}
if (merge_file_special_markers(o, , , ,
@@ -1862,7 +1870,8 @@ static int process_entry(struct merge_options *o,
oid = b_oid;
conf = _("directory/file");
}
-   if (dir_in_way(path, !o->call_depth)) {
+   if (dir_in_way(path, !o->call_depth,
+  S_ISGITLINK(a_mode))) {
char *new_path = unique_path(o, path, add_branch);
clean_merge = 0;
output(o, 1, _("CONFLICT (%s): There is a directory 
with name %s in %s. "
diff --git a/t/t3030-merge-recursive.sh 

Re: git submodule add broken (2.11.0-rc1): Cannot open git-sh-i18n

[Stefan Beller]

$ git --version
git version 1.8.5.6
$ if [ "$LATEST_GIT" = "1" ]; then
...
 export PATH="/tmp/git:$PATH"
 fi

make -j 8
...
$ git --version
git version 2.11.0-rc0

So you compile 2.11.0-rc0 yourself, but you do not install it, instead
the $PATH is pointed to /tmp/git instead, however the later calls fail with:

Can't open /home/travis/libexec/git-core/git-sh-i18n

which is where 1.8.5.6 is installed.

So you're running into an internationalization problem
between 2 very different versions of Git (1.8 seems to be ancient)

Not sure if i can offer advice except from
"Don't do that, instead install Git properly". ;)

Thanks,
Stefan

Re: [PATCH v1 1/2] config.mak.in: set NO_OPENSSL and APPLE_COMMON_CRYPTO for macOS >10.11

[Paul Smith]

On Mon, 2016-11-07 at 12:46 -0500, Jeff King wrote:
> Specifically I wanted to make sure that
> 
>   FOO = bar
>   FOO =
>   ifdef FOO
>   ... something ...
>   endif
> 
> works as if FOO had never been set in the first place. Which it seems
> to, at least in GNU make (and that is the only one we support, for
> other reasons).

Yes, it will work.  Confusingly, "ifdef" actually tests whether the
variable has a non-empty value, not whether it's defined:

> The 'ifdef' form takes the _name_ of a variable as its argument, not
> a reference to a variable.  The value of that variable has a non-
> empty value, the TEXT-IF-TRUE is effective; otherwise, the TEXT-IF-
> FALSE, if any, is effective

*sigh* History...

Re: [PATCH v1 1/2] config.mak.in: set NO_OPENSSL and APPLE_COMMON_CRYPTO for macOS >10.11

[Jeff King]

On Mon, Nov 07, 2016 at 12:36:34PM -0500, Paul Smith wrote:

> On Mon, 2016-11-07 at 12:26 -0500, Jeff King wrote:
> > I have in the back of my mind a fear that it is harder to unset a
> > make variable than it is to override it with a new value (which is
> > what you'd want to do here to turn openssl back on),
> 
> It depends on what you mean by "unset".
> 
> If you mean it as per the shell "unset" command, where the variable is
> completely forgotten as if it never was set at all, that's tricky.  You
> have to use the "undefine" special command which was introduced in GNU
> make 3.82 (released in 2010).
> 
> But if you just want to set the variable to the empty string, using
> "FOO=" works fine for that in all versions of make (GNU and otherwise)
> and using all the normal rules (command line override, etc.)

Specifically I wanted to make sure that

  FOO = bar
  FOO =
  ifdef FOO
  ... something ...
  endif

works as if FOO had never been set in the first place. Which it seems
to, at least in GNU make (and that is the only one we support, for other
reasons).

-Peff

Re: [PATCH v1 1/2] config.mak.in: set NO_OPENSSL and APPLE_COMMON_CRYPTO for macOS >10.11

[Paul Smith]

On Mon, 2016-11-07 at 12:26 -0500, Jeff King wrote:
> I have in the back of my mind a fear that it is harder to unset a
> make variable than it is to override it with a new value (which is
> what you'd want to do here to turn openssl back on),

It depends on what you mean by "unset".

If you mean it as per the shell "unset" command, where the variable is
completely forgotten as if it never was set at all, that's tricky.  You
have to use the "undefine" special command which was introduced in GNU
make 3.82 (released in 2010).

But if you just want to set the variable to the empty string, using
"FOO=" works fine for that in all versions of make (GNU and otherwise)
and using all the normal rules (command line override, etc.)

It's not easy to distinguish between a variable that is empty and one
that is actually not defined, in make, so it's a difference without a
distinction in almost all situations.

git submodule add broken (2.11.0-rc1): Cannot open git-sh-i18n

[Anthony Sottile]

Noticed as part of my automated tests here:
https://travis-ci.org/pre-commit/pre-commit/jobs/173957051

Minimal reproduction:

rm -rf /tmp/git /tmp/foo /tmp/bar
git clone git://github.com/git/git --depth 1 /tmp/git
pushd /tmp/git
make -j 8
popd
export PATH="/tmp/git:$PATH"
git init /tmp/foo
git init /tmp/bar
cd /tmp/foo
git submodule add /tmp/bar baz

Output:

$ rm -rf /tmp/git /tmp/foo /tmp/bar
$ git clone git://github.com/git/git --depth 1 /tmp/git
Cloning into '/tmp/git'...
remote: Counting objects: 3074, done.
remote: Compressing objects: 100% (2735/2735), done.
remote: Total 3074 (delta 249), reused 1871 (delta 215), pack-reused 0
Receiving objects: 100% (3074/3074), 6.38 MiB | 905.00 KiB/s, done.
Resolving deltas: 100% (249/249), done.
Checking connectivity... done.
$ pushd /tmp/git
/tmp/git /tmp
$ make -j 8
GIT_VERSION = 2.11.0-rc0

... lots of make output ...

$ popd
/tmp
$ export PATH="/tmp/git:$PATH"
$ git init /tmp/foo
warning: templates not found /home/asottile/share/git-core/templates
Initialized empty Git repository in /tmp/foo/.git/
$ git init /tmp/bar
warning: templates not found /home/asottile/share/git-core/templates
Initialized empty Git repository in /tmp/bar/.git/
$ cd /tmp/foo
$ git submodule add /tmp/bar baz
/tmp/git/git-submodule: 46: .: Can't open
/home/asottile/libexec/git-core/git-sh-i18n
$ echo $?
2


Thanks

Anthony

Re: [PATCH v1 1/2] config.mak.in: set NO_OPENSSL and APPLE_COMMON_CRYPTO for macOS >10.11

[Jeff King]

On Sun, Nov 06, 2016 at 08:35:04PM +0100, Lars Schneider wrote:

> Good point. I think I found an even easier way to achieve the same.
> What do you think about the patch below?
>
> [...]
>
> diff --git a/Makefile b/Makefile
> index 9d6c245..f53fcc9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1047,6 +1047,7 @@ ifeq ($(uname_S),Darwin)
>   endif
>   endif
>   ifndef NO_APPLE_COMMON_CRYPTO
> + NO_OPENSSL = YesPlease
>   APPLE_COMMON_CRYPTO = YesPlease
>   COMPAT_CFLAGS += -DAPPLE_COMMON_CRYPTO
>   endif

That is much simpler.

I have in the back of my mind a fear that it is harder to unset a make
variable than it is to override it with a new value (which is what you'd
want to do here to turn openssl back on), but I can't seem to come up
with a case that doesn't work. So I am probably misremembering, or just
thinking of something that used to be a problem long ago.

-Peff

Re: git -C has unexpected behaviour

[Johannes Schindelin]

Hi Felix,

On Mon, 7 Nov 2016, Felix Nairz wrote:

> From what you are saying I can see that this expects as designed. It's
> confusing in the submodule case, but I get you don't want to add extra
> rules which slow down performance and mess with other people at the
> same time.

The "messing with other people" is the key point. There are most likely
other users than just me who use the fact that `git -C sub/directory/ ...`
works in a subdirectory of a checkout.

Ciao,
Johannes

Re: Regarding "git log" on "git series" metadata

[Josh Triplett]

On Mon, Nov 07, 2016 at 04:42:04PM +0700, Duy Nguyen wrote:
> On Mon, Nov 7, 2016 at 8:18 AM, Josh Triplett  wrote:
> > Once we have gitrefs, you have both alternatives: reachable (gitref) or
> > not reachable (gitlink).
> >
> > However, if you want some way to mark reachable objects as not
> > reachable, such as for a sparse checkout, external large-object storage,
> > or similar, then you can use a single unified mechanism for that whether
> > working with gitrefs, trees, or blobs.
> 
> How? Whether an object reachable or not is baked in the definition (of
> either gitlink or gitref). I don't think you can have a "maybe
> reachable" type then rely on an external source to determine
> reachability,

You'd have various "reachable by default" entries in trees, including
trees, blobs, and gitrefs, and then have an external mechanism (likely
via .git/config) to say "ignore objects with these hashes/paths".  For
instance, you might say "ignore all objects only reachable from the path
'assets/video/*' within a commit's tree".  With the right set of client
and server extensions, you could then avoid downloading those objects.

Re: [PATCH v4 08/14] i18n: add--interactive: mark edit_hunk_manually message for translation

[Vasco Almeida]

A Seg, 10-10-2016 às 12:54 +, Vasco Almeida escreveu:
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 045b847..861f7b0 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1058,6 +1058,30 @@ sub color_diff {
> } @_;
>  }
>  
> +my %edit_hunk_manually_modes = (
> +   stage => N__(
> +"# If the patch applies cleanly, the edited hunk will immediately be
> +# marked for staging."),
> +   stash => N__(
> +"# If the patch applies cleanly, the edited hunk will immediately be
> +# marked for stashing."),
> +   reset_head => N__(
> +"# If the patch applies cleanly, the edited hunk will immediately be
> +# marked for unstaging."),
> +   reset_nothead => N__(
> +"# If the patch applies cleanly, the edited hunk will immediately be
> +# marked for applying."),
> +   checkout_index => N__(
> +"# If the patch applies cleanly, the edited hunk will immediately be
> +# marked for discarding"),
> +   checkout_head => N__(
> +"# If the patch applies cleanly, the edited hunk will immediately be
> +# marked for discarding."),
> +   checkout_nothead => N__(
> +"# If the patch applies cleanly, the edited hunk will immediately be
> +# marked for applying."),
> +);
> +

I think marking strings comment with '#' for translation is not the
best thing because there is a change that a translator will miss to
comment a line. I will change this to mark only the text and then, at
run time, prefix comment chars to that text.

>  sub edit_hunk_manually {
> my ($oldtext) = @_;
>  
> @@ -1065,22 +1089,21 @@ sub edit_hunk_manually {
> my $fh;
> open $fh, '>', $hunkfile
> or die sprintf(__("failed to open hunk edit file for
> writing: %s"), $!);
> -   print $fh "# Manual hunk edit mode -- see bottom for a quick
> guide\n";
> +   print $fh __("# Manual hunk edit mode -- see bottom for a
> quick guide\n");

Here too.

> print $fh @$oldtext;
> -   my $participle = $patch_mode_flavour{PARTICIPLE};
> my $is_reverse = $patch_mode_flavour{IS_REVERSE};
> my ($remove_plus, $remove_minus) = $is_reverse ? ('-', '+') :
> ('+', '-');
> -   print $fh < -# ---
> -# To remove '$remove_minus' lines, make them ' ' lines (context).
> -# To remove '$remove_plus' lines, delete them.
> +   print $fh sprintf(__(
> +"# ---
> +# To remove '%s' lines, make them ' ' lines (context).
> +# To remove '%s' lines, delete them.
>  # Lines starting with # will be removed.
> -#
> -# If the patch applies cleanly, the edited hunk will immediately be
> -# marked for $participle. If it does not apply cleanly, you will be
> given
> +#\n"), $remove_minus, $remove_plus),
> +__($edit_hunk_manually_modes{$patch_mode}), __(
> +# TRANSLATORS: 'it' refers to the patch mentioned in the previous
> messages.
> +" If it does not apply cleanly, you will be given
>  # an opportunity to edit again. If all lines of the hunk are
> removed,
> -# then the edit is aborted and the hunk is left unchanged.
> -EOF
> +# then the edit is aborted and the hunk is left unchanged.\n");

And here too.
Currently this joins the two sentences/parts in the same line like so

# If the patch applies cleanly, the edited hunk will immediately be
# marked for staging. If it does not apply cleanly, you will be given
# an opportunity to edit again. If all lines of the hunk are removed,
# then the edit is aborted and the hunk is left unchanged.

But since the translator translates each sentence separately, it is
hard to align them properly to not make lines too long. Hence I am
considering to break each sentence to start on its own line.

git push remote syntax

[Diggory Hardy]

Dear all,

One thing I find a little frustrating about git is that the syntax needed 
differs by command. I wish the 'remote/branch' syntax was more universal:

> git pull myremote/somebranch
complains about the syntax; IMO it should either pull from that branch (and 
merge if necessary) or complain instead that pulling from a different branch 
is not supported (and suggest using merge).

> git push myremote/somebranch
should push there, i.e. be equivalent to
> git push myremote HEAD:somebranch

These are just some comments about how I think git could be made easier to 
use. Apologies if the suggestions have already been discussed before.

Regards,

D Hardy

Re: gitk: avoid obscene memory consumption

[Markus Hitter]

Am 07.11.2016 um 05:11 schrieb Paul Mackerras:
>> - Storing only the actually viewed diff. It's an interactive tool, so 
>> there's no advantage in displaying the diff in 0.001 seconds over viewing it 
>> in 0.1 seconds. As far as I can see, Gitk currently stores every diff it 
>> gets a hold of forever.
> It does?  That would be a bug. :)
> 

So far I've found three arrays being populated lazily (which is good) but never 
being released (which ignores changes to the underlying repo):

$commitinfo: one entry of about 500 bytes per line viewed in the list of 
commits. Maximum size of the array is the number of commits. As far as I can 
see, this array should be removed on a reload (Shift-F5).

$blobdifffd: one entry of about 45 bytes for every commit ever read. The 
underlying file descriptor gets closed, but the entry in this array remains. So 
far I didn't find the reason why this array exists at all. It's also not 
removed on a reload.

$treediffs: always the same number of entries as $blobdiffd, but > 1000 
bytes/entry. Removed/refreshed on a reload (good!), different number of entries 
from that point on.

In case you want to play as well, here's the code I wrote for the 
investigation, it can be appended right at the bottom of the gitk script:

--8<---
proc variableSizes {} {
# Add variable here to get them shown.
global diffcontext diffids blobdifffd currdiffsubmod commitinfo
global diffnexthead diffnextnote difffilestart
global diffinhdr treediffs

puts "---"
foreach V [info vars] {
if { ! [info exists $V] } {
continue
}

set count 0
set bytes 0
if [array exists $V] {
set count [array size $V]
foreach I [array get $V] {
set bytes [expr $bytes + [string bytelength $I]]
}
} elseif [catch {llength [set $V]}] {
set count [llength [set $V]]
#   set bytes [string bytelength [list {*}[set $V]]]
} else {
set bytes [string bytelength [set $V]]
}
puts [format "%20s: %5d items, %10d bytes" $V $count $bytes]
}

#catch {
#   set output [memory info]
#   puts $output
#}

after 3000 variableSizes
}

variableSizes
-->8---

[memory info] requires a Tcl with memory debug enabled.


Markus
-- 
- - - - - - - - - - - - - - - - - - -
Dipl. Ing. (FH) Markus Hitter
http://www.jump-ing.de/

Forbid access to /gitweb but authorize the sub projets

[Alexandre Duplaix]

Hello,

I have several projects under https://myserver/gitweb and I would like 
to forbid the access to the root, so that the users can't list the 
differents projects.


However, I need to let the access to the sub projects (ex: 
https://myserver/gitweb/?p=project1;a=summary


How can I do please ?

Thank you in advance :)

" Ce courriel et les documents qui lui sont joints peuvent contenir des
informations confidentielles ou ayant un caractère privé. 
S'ils ne vous sont pas destinés nous vous signalons qu'il est strictement interdit de les

divulguer, de les reproduire ou d'en utiliser de quelque mani�re que ce
soit le contenu. Si ce message vous a �t� transmis par erreur, merci d'en
informer l'exp�diteur et de supprimer imm�diatement de votre syst�me
informatique ce courriel ainsi que tous les documents qui y sont attach�s"

  **

" This e-mail and any attached documents may contain confidential or
proprietary information. If you are not the intended recipient, you are
notified that any dissemination, copying of this e-mail and any attachments
thereto or use of their contents by any means whatsoever is strictly
prohibited. If you have received this e-mail in error, please advise the
sender immediately and delete this e-mail and all attached documents
from your computer system."
#

Re: [PATCH v1 03/19] split-index: add {add,remove}_split_index() functions

[Duy Nguyen]

On Sun, Oct 30, 2016 at 5:06 AM, Christian Couder
 wrote:
> On Tue, Oct 25, 2016 at 11:58 AM, Duy Nguyen  wrote:
>> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
>>  wrote:
>>> +void remove_split_index(struct index_state *istate)
>>> +{
>>> +   if (istate->split_index) {
>>> +   /*
>>> +* can't discard_split_index(_index); because that
>>> +* will destroy split_index->base->cache[], which may
>>> +* be shared with the_index.cache[]. So yeah we're
>>> +* leaking a bit here.
>>
>> In the context of update-index, this is a one-time thing and leaking
>> is tolerable. But because it becomes a library function now, this leak
>> can become more serious, I think.
>>
>> The only other (indirect) caller is read_index_from() so probably not
>> bad most of the time (we read at the beginning of a command only).
>> sequencer.c may discard and re-read the index many times though,
>> leaking could be visible there.
>
> So is it enough to check if split_index->base->cache[] is shared with
> the_index.cache[] and then decide if discard_split_index(_index)
> should be called?

It's likely shared though. We could un-share cache[] by duplicating
index entries in the_index.cache[] if they point back to
split_index->base (we know what entries are shared by examining the
"index" field). Once we do that, we can discard_split_index()
unconditionally. There's another place that has similar leak:
move_cache_to_base_index(), which could receive the same treatment.
-- 
Duy

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Duy Nguyen]

On Wed, Nov 2, 2016 at 8:08 PM, Jeff King  wrote:
> The attributes system may sometimes read in-tree files from
> the filesystem, and sometimes from the index. In the latter
> case, we do not resolve symbolic links (and are not likely
> to ever start doing so). Let's open filesystem links with
> O_NOFOLLOW so that the two cases behave consistently.

This sounds backward to me. The major use case is reading
.gitattributes on worktree, which follows symlinks so far. Only
git-archive has a special need to read index-only versions. The
worktree behavior should influence the in-index one, not the other way
around. If we could die("BUG" when git-archive is used on symlinks
(without --worktree-attributes). If people are annoyed by it, they can
implement symlink folllowing (to another version in index, not on
worktree).

The story is similar for .gitignore where in-index version is merely
an optimization. If it's symlinks and we can't follow, we should fall
back to worktree version.
-- 
Duy

Re: Regarding "git log" on "git series" metadata

[Duy Nguyen]

On Mon, Nov 7, 2016 at 8:18 AM, Josh Triplett  wrote:
> Once we have gitrefs, you have both alternatives: reachable (gitref) or
> not reachable (gitlink).
>
> However, if you want some way to mark reachable objects as not
> reachable, such as for a sparse checkout, external large-object storage,
> or similar, then you can use a single unified mechanism for that whether
> working with gitrefs, trees, or blobs.

How? Whether an object reachable or not is baked in the definition (of
either gitlink or gitref). I don't think you can have a "maybe
reachable" type then rely on an external source to determine
reachability,
-- 
Duy

Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange

[Duy Nguyen]

(sorry I got sick in the last few weeks and could not respond to this earlier)

On Mon, Nov 7, 2016 at 4:44 AM, Christian Couder
 wrote:
> Le 6 nov. 2016 09:16, "Junio C Hamano"  a écrit :
>>
>> Christian Couder  writes:
>>
>> > I think it is easier for user to be able to just set core.splitIndex
>> > to true to enable split-index.
>>
>> You can have that exact benefit by making core.splitIndex to
>> bool-or-more.  If your default is 20%, take 'true' as if the user
>> specified 20% and take 'false' as if the user specified 100% (or is
>> it 0%?  I do not care about the details but you get the point).
>
> Then if we ever add 'auto' and the user wants for example 10% instead of the
> default 20%, we will have to make it accept things like "auto,10".

In my opinion, "true" _is_ auto, which is a way to say "I trust you to
do the right thing, just re-split the index when it makes sense", "no"
is disabled of course. If the user wants to be specific, just write
"10" or some other percentage.(and either 0 or 100 would mean enable
split-index but do not re-split automatically, let _me_ do it when I
want it)
-- 
Duy

NOTIFICATION

[Mr]

We are pleased to inform you of the result of the last Email qqannual draw of 
our Chevrolet International Lottery Programs. The Chevrolet Email lotto draws 
was conducted from an exclusive list of 25,000,000 Email all over the world. 
and your Email is to receive a cash prize of Four Crore Thirty Five Lakhs India 
Rupees (RS,4,35,000,000.00/INR) Email for moer info 

Best Regards,
Barr. Kenny White