[PATCH] credential-libsecret: unlock locked secrets

[Dennis Kaarsemaker]

Credentials exposed by the secret service DBUS interface may be locked.
Setting the SECRET_SEARCH_UNLOCK flag will make the secret service
unlock these secrets, possibly prompting the user for credentials to do
so. Without this flag, the secret is simply not loaded.

Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
---
 contrib/credential/libsecret/git-credential-libsecret.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/credential/libsecret/git-credential-libsecret.c 
b/contrib/credential/libsecret/git-credential-libsecret.c
index 4c56979d8a..b4750c9ee8 100644
--- a/contrib/credential/libsecret/git-credential-libsecret.c
+++ b/contrib/credential/libsecret/git-credential-libsecret.c
@@ -104,7 +104,7 @@ static int keyring_get(struct credential *c)
items = secret_service_search_sync(service,
   SECRET_SCHEMA_COMPAT_NETWORK,
   attributes,
-  SECRET_SEARCH_LOAD_SECRETS,
+  SECRET_SEARCH_LOAD_SECRETS | 
SECRET_SEARCH_UNLOCK,
   NULL,
   );
g_hash_table_unref(attributes);
-- 
2.15.0-rc2-464-gb5de734

Re: git, isolation

[Dennis Kaarsemaker]

On Fri, 2017-11-03 at 17:33 +0100, Péter wrote:
> Hi,
> 
> If I do a "git commit", issue git operations, and at the end, issue a "rm 
> ", is there any guarantee that my 
> filesystem will be "clean", 

No.

> i.e. not polluted or otherwise modified by some git command? Are the git 
> operations 
> restricted to the repo-directory (and possibly remote places, over network)? 

No.

> Do the git-directory behaves as it were 
> chroot-ed or be a sandbox? (Yet another words: is the git-directory isolated 
> from the rest of the local filesystem (and 
> packaging system)?)

And no :)

Most git commands will not touch anything outside the main worktree and
the .git directory in there, but commands like 'git worktree' can be
used to create worktrees anywhere in the filesystem, and when you play
tricks with the GIT_WORK_TREE environment variable, you can do other
nasty things.

D.

Re: Git libsecret No Unlock Dialog Issue

[Dennis Kaarsemaker]

On Thu, 2017-11-02 at 11:35 -0700, Stefan Beller wrote:
> On Thu, Nov 2, 2017 at 9:00 AM, Yaroslav Sapozhnyk
>  wrote:
> > When using Git on Fedora with locked password store
> > credential-libsecret asks for username/password instead of displaying
> > the unlock dialog.
> 
> Git as packaged by Fedora or upstream Git (which version)?

Looking at the code: current upstream git. Looking at the documentation
for libsecret, this should fix it. I've not been able to test it
though.

diff --git a/contrib/credential/libsecret/git-credential-libsecret.c 
b/contrib/credential/libsecret/git-credential-libsecret.c
index 4c56979d8a..b4750c9ee8 100644
--- a/contrib/credential/libsecret/git-credential-libsecret.c
+++ b/contrib/credential/libsecret/git-credential-libsecret.c
@@ -104,7 +104,7 @@ static int keyring_get(struct credential *c)
items = secret_service_search_sync(service,
   SECRET_SCHEMA_COMPAT_NETWORK,
   attributes,
-  SECRET_SEARCH_LOAD_SECRETS,
+  SECRET_SEARCH_LOAD_SECRETS | 
SECRET_SEARCH_UNLOCK,
   NULL,
   );
g_hash_table_unref(attributes);

Re: Multiple paths in GIT_EXEC_PATH

[Dennis Kaarsemaker]

On Tue, 2017-10-17 at 18:21 +0300, Nikolay Yakimov wrote:
> For why I need that is another matter. Long story short, I need git to
> look for '.gitignore' in a particular non-standard location, since I
> have multiple git repositories in the same workdir (that workdir being
> $HOME and git repositories being stores for my different configs)

That is solvable without needing multiple directories in
$GIT_EXEC_PATH. vcsh, which also solves the 'multiple repos with same
workdir' problem in a very thin wrapper around git, does this by
setting core.excludesfile in the per repo config to a unique path.

hurricane:~$ vcsh dotfiles config core.excludesfile
.gitignore.d/dotfiles
hurricane:~$ vcsh secrets config core.excludesfile
.gitignore.d/secrets

D.

Re: Git diff --no-index and file descriptor inputs

[Dennis Kaarsemaker]

On Thu, 2017-09-07 at 15:55 -0400, Jason Pyeron wrote:
> > -Original Message-
> > From: Martin Ågren
> > Sent: Thursday, September 7, 2017 1:48 PM
> > 
> > On 7 September 2017 at 18:00, Jason Pyeron <jpye...@pdinc.us> wrote:
> > > 
> > > I was hoping to leverage --word-diff-regex, but the 
> > 
> > --no-index option 
> > > does not seem to work with <(...) notation.
> > > 
> > > I am I doing something wrong or is this a bug?
> > 
> > There were some patches floating around half a year ago, I 
> > don't know what happened to them.
> > 
> > From: Dennis Kaarsemaker
> > Sent: Friday, January 13, 2017 5:20 AM
> > Subject: [PATCH v2 0/2] diff --no-index: support symlinks and pipes
> > https://public-inbox.org/git/20170324213110.4331-1-den...@kaarsemaker.net/
> 
> I see, it goes back to 2016...
> 
> > From: Dennis Kaarsemaker
> > Subject: [RFC/PATCH 0/2] git diff <(command1) <(command2)
> > Date: Fri, 11 Nov 2016 21:19:56 +0100
> 
> https://public-inbox.org/git/2016201958.2175-1-den...@kaarsemaker.net/
> 
> I will read up on those threads weekend, then try to apply the patches and
> see what happens.
> 
> Thanks for the google fu help.

I hope to send a new version of this soon. I had almost no time to do
anything git related in the last year, trying to catch up with mailing
list traffic now.

D.

Re: git send-email does not work with Google anymore?!

[Dennis Kaarsemaker]

On Thu, 2017-10-05 at 12:52 +0200, Lars Schneider wrote:
> Hi,
> 
> I used to use the Google SMTP server to send my patches to the list with 
> the following config:

>8 ---

> Apparently that stopped working today. I get this error:
> 
> (mbox) Adding cc: Lars Schneider  from line 
> 'From: Lars Schneider '
> Password for 'smtp://larsxschnei...@gmail.com@smtp.gmail.com:587':
> 5.7.14  5.7.14 ...> Please log in via your web browser and
> 5.7.14 then try again.
> 5.7.14  Learn more at
> 5.7.14  https://support.google.com/mail/answer/78754 ... - gsmtp
> 
> Of couse I tried to log in via web browser etc. Does anyone else use 
> Google as SMTP server? If yes, does it work for you?

For 2fa-protected accounts, this seems to break quite often. I ended up
setting up a mail relay on my vps for this. If you want, you can use
git.seveas.net to send patches to the git mailing lists (doesn't work
for other recipients, I'm not quite stupid enough to run an open mail
relay...)

D.

Re: [PATCH v2] send-email: Net::SMTP::SSL is obsolete, use only when necessary

[Dennis Kaarsemaker]

On Thu, 2017-06-01 at 07:50 +0900, Junio C Hamano wrote:
> Dennis Kaarsemaker <den...@kaarsemaker.net> writes:
> 
> > Second ping. This problem is not going away, so if this solution is not
> > acceptable, I'd like to know what needs to be improved.
> 
> Perhaps you needed to actually test with older installation that
> people have, it seems, between pings.  Immediately after this was
> merged to 'master', we start getting bug reports X-<.
> 
> Eric Biggers' message 
> 
>     https://public-inbox.org/git/<20170531222455.gd72...@gmail.com>;
> 
> seems to indicate that we should cut off at 3.01 not 1.28?
> 
> Thanks.

Apologies for that. The version numbering of libnet is *weird*. The
libnet version this was fixed in *is* 1.28, but the Net::SMTP module in
 libnet version 1.28 has version 2.33.

I did test with some older perl versions, but not far enough back it
seems :(

D.

Re: persistent-https, url insteadof, and `git submodule`

[Dennis Kaarsemaker]

On Fri, 2017-05-19 at 23:43 +0200, Dennis Kaarsemaker wrote:
> On Fri, 2017-05-19 at 14:57 -0500, Elliott Cable wrote:
> > Set up `persistent-https` as described in the [README][]; including the
> > ‘rewrite https urls’ feature in `.gitconfig`:
> > 
> > [url "persistent-https"]
> > insteadof = https
> > [url "persistent-http"]
> > insteadof = http
> > 
> > Unfortunately, this breaks `git submodule add`:
> > 
> > > git submodule add https://github.com/nodenv/nodenv.git \
> > ./Vendor/nodenv
> > Cloning into '/Users/ec/Library/System Repo/Vendor/nodenv'...
> > fatal: transport 'persistent-https' not allowed
> > fatal: clone of 'https://github.com/nodenv/nodenv.git' into
> > submodule path '/Users/ec/Library/System Repo/Vendor/nodenv' failed
> > 
> > Presumably this isn't intended behaviour?
> 
> It actually is. git-submodule sets GIT_PROTOCOL_FROM_USER to 0, which
> makes git not trust any urls except http(s), git, ssh and file urls
> unless you explicitely configure git to allow it. See the
> GIT_ALLOW_PROTOCOL section in man git and the git-config section it
> links to.

33cfccbbf3 (submodule: allow only certain protocols for submodule
fetches, 2015-09-16) says:

submodule: allow only certain protocols for submodule fetches

Some protocols (like git-remote-ext) can execute arbitrary
code found in the URL. The URLs that submodules use may come
from arbitrary sources (e.g., .gitmodules files in a remote
repository). Let's restrict submodules to fetching from a
known-good subset of protocols.

Note that we apply this restriction to all submodule
commands, whether the URL comes from .gitmodules or not.
This is more restrictive than we need to be; for example, in
the tests we run:

  git submodule add ext::...

which should be trusted, as the URL comes directly from the
command line provided by the user. But doing it this way is
simpler, and makes it much less likely that we would miss a
case. And since such protocols should be an exception
(especially because nobody who clones from them will be able
to update the submodules!), it's not likely to inconvenience
anyone in practice.


D.

Re: persistent-https, url insteadof, and `git submodule`

[Dennis Kaarsemaker]

On Fri, 2017-05-19 at 14:57 -0500, Elliott Cable wrote:
> Set up `persistent-https` as described in the [README][]; including the
> ‘rewrite https urls’ feature in `.gitconfig`:
> 
> [url "persistent-https"]
> insteadof = https
> [url "persistent-http"]
> insteadof = http
> 
> Unfortunately, this breaks `git submodule add`:
> 
> > git submodule add https://github.com/nodenv/nodenv.git \
> ./Vendor/nodenv
> Cloning into '/Users/ec/Library/System Repo/Vendor/nodenv'...
> fatal: transport 'persistent-https' not allowed
> fatal: clone of 'https://github.com/nodenv/nodenv.git' into
> submodule path '/Users/ec/Library/System Repo/Vendor/nodenv' failed
> 
> Presumably this isn't intended behaviour?

It actually is. git-submodule sets GIT_PROTOCOL_FROM_USER to 0, which
makes git not trust any urls except http(s), git, ssh and file urls
unless you explicitely configure git to allow it. See the
GIT_ALLOW_PROTOCOL section in man git and the git-config section it
links to.

D.

Re: [PATCH v2] send-email: Net::SMTP::SSL is obsolete, use only when necessary

[Dennis Kaarsemaker]

Second ping. This problem is not going away, so if this solution is not
acceptable, I'd like to know what needs to be improved.

On Thu, 2017-05-04 at 09:01 +0200, Dennis Kaarsemaker wrote:
> Ping. It's a little over a month since I sent this, but I haven't seen
> any comments. Is this commit good to go?
> 
> On Fri, 2017-03-24 at 22:37 +0100, Dennis Kaarsemaker wrote:
> > Net::SMTP itself can do the necessary SSL and STARTTLS bits just fine
> > since version 1.28, and Net::SMTP::SSL is now deprecated. Since 1.28
> > isn't that old yet, keep the old code in place and use it when
> > necessary.
> > 
> > While we're in the area, mark some messages for translation that were
> > not yet marked as such.
> > 
> > Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
> > ---
> >  git-send-email.perl | 54 
> > ++---
> >  1 file changed, 35 insertions(+), 19 deletions(-)
> > 
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > index eea0a517f7..0d90439d9a 100755
> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -1353,10 +1353,12 @@ EOF
> > die __("The required SMTP server is not properly 
> > defined.")
> > }
> >  
> > +   require Net::SMTP;
> > +   my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < 
> > version->parse("1.28");
> > +   $smtp_domain ||= maildomain();
> > +
> > if ($smtp_encryption eq 'ssl') {
> > $smtp_server_port ||= 465; # ssmtp
> > -   require Net::SMTP::SSL;
> > -   $smtp_domain ||= maildomain();
> > require IO::Socket::SSL;
> >  
> > # Suppress "variable accessed once" warning.
> > @@ -1368,34 +1370,48 @@ EOF
> > # Net::SMTP::SSL->new() does not forward any SSL options
> > IO::Socket::SSL::set_client_defaults(
> > ssl_verify_params());
> > -   $smtp ||= Net::SMTP::SSL->new($smtp_server,
> > - Hello => $smtp_domain,
> > - Port => $smtp_server_port,
> > - Debug => $debug_net_smtp);
> > +
> > +   if ($use_net_smtp_ssl) {
> > +   require Net::SMTP::SSL;
> > +   $smtp ||= Net::SMTP::SSL->new($smtp_server,
> > + Hello => 
> > $smtp_domain,
> > + Port => 
> > $smtp_server_port,
> > + Debug => 
> > $debug_net_smtp);
> > +   }
> > +   else {
> > +   $smtp ||= Net::SMTP->new($smtp_server,
> > +Hello => $smtp_domain,
> > +Port => 
> > $smtp_server_port,
> > +Debug => 
> > $debug_net_smtp,
> > +SSL => 1);
> > +   }
> > }
> > else {
> > -   require Net::SMTP;
> > -   $smtp_domain ||= maildomain();
> > $smtp_server_port ||= 25;
> > $smtp ||= Net::SMTP->new($smtp_server,
> >  Hello => $smtp_domain,
> >  Debug => $debug_net_smtp,
> >  Port => $smtp_server_port);
> > if ($smtp_encryption eq 'tls' && $smtp) {
> > -   require Net::SMTP::SSL;
> > -   $smtp->command('STARTTLS');
> > -   $smtp->response();
> > -   if ($smtp->code == 220) {
> > +   if ($use_net_smtp_ssl) {
> > +   $smtp->command('STARTTLS');
> > +   $smtp->response();
> > +   if ($smtp->code != 220) {
> > +   die sprintf(__("Server does not 
>

Re: Bug report: Git Stash; spelling mistake of stash followed by a yes prints character 'y' infinite times.

[Dennis Kaarsemaker]

On Wed, 2017-05-03 at 14:51 +0530, Rashmi Pai wrote:

> I am a corporate user of git. I noticed that when you switch between
> the branches and do a git stash ( I miss spelled it as git stahs). Git
> asked if i meant git stash. and i entered yes. and git printed the
> character y infinite times.

Hi Rashmi,

While git asks the question 'Did you mean stash', that question is
merely rhetorical, it does not expect an answer but exits. So what
you're seeing is the output of the shell builting 'yes', not output
from git.

D.

Re: Script to rebase branches

[Dennis Kaarsemaker]

On Sat, 2017-05-06 at 12:23 +0200, Lars Schneider wrote:
> Hi,
> 
> I am about to write a bash/sh script that helps me to rebase a bunch of 
> branches (e.g. select branches based on prefix, conflict resolution/
> rerere support, ...).
> 
> I wonder if anyone has such a script already and is willing to share it.

Dscho's git garden shears perhaps?

https://github.com/git-for-windows/build-extra/blob/master/shears.sh

D.

Re: [PATCH v2] send-email: Net::SMTP::SSL is obsolete, use only when necessary

[Dennis Kaarsemaker]

Ping. It's a little over a month since I sent this, but I haven't seen
any comments. Is this commit good to go?

On Fri, 2017-03-24 at 22:37 +0100, Dennis Kaarsemaker wrote:
> Net::SMTP itself can do the necessary SSL and STARTTLS bits just fine
> since version 1.28, and Net::SMTP::SSL is now deprecated. Since 1.28
> isn't that old yet, keep the old code in place and use it when
> necessary.
> 
> While we're in the area, mark some messages for translation that were
> not yet marked as such.
> 
> Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
> ---
>  git-send-email.perl | 54 
> ++---
>  1 file changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index eea0a517f7..0d90439d9a 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1353,10 +1353,12 @@ EOF
>   die __("The required SMTP server is not properly 
> defined.")
>   }
>  
> + require Net::SMTP;
> + my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < 
> version->parse("1.28");
> + $smtp_domain ||= maildomain();
> +
>   if ($smtp_encryption eq 'ssl') {
>   $smtp_server_port ||= 465; # ssmtp
> - require Net::SMTP::SSL;
> - $smtp_domain ||= maildomain();
>   require IO::Socket::SSL;
>  
>   # Suppress "variable accessed once" warning.
> @@ -1368,34 +1370,48 @@ EOF
>   # Net::SMTP::SSL->new() does not forward any SSL options
>   IO::Socket::SSL::set_client_defaults(
>   ssl_verify_params());
> - $smtp ||= Net::SMTP::SSL->new($smtp_server,
> -   Hello => $smtp_domain,
> -   Port => $smtp_server_port,
> -   Debug => $debug_net_smtp);
> +
> + if ($use_net_smtp_ssl) {
> + require Net::SMTP::SSL;
> + $smtp ||= Net::SMTP::SSL->new($smtp_server,
> +   Hello => 
> $smtp_domain,
> +   Port => 
> $smtp_server_port,
> +   Debug => 
> $debug_net_smtp);
> + }
> + else {
> + $smtp ||= Net::SMTP->new($smtp_server,
> +  Hello => $smtp_domain,
> +  Port => 
> $smtp_server_port,
> +  Debug => 
> $debug_net_smtp,
> +  SSL => 1);
> + }
>   }
>   else {
> - require Net::SMTP;
> - $smtp_domain ||= maildomain();
>   $smtp_server_port ||= 25;
>   $smtp ||= Net::SMTP->new($smtp_server,
>Hello => $smtp_domain,
>Debug => $debug_net_smtp,
>Port => $smtp_server_port);
>   if ($smtp_encryption eq 'tls' && $smtp) {
> - require Net::SMTP::SSL;
> - $smtp->command('STARTTLS');
> - $smtp->response();
> - if ($smtp->code == 220) {
> + if ($use_net_smtp_ssl) {
> + $smtp->command('STARTTLS');
> + $smtp->response();
> + if ($smtp->code != 220) {
> + die sprintf(__("Server does not 
> support STARTTLS! %s"), $smtp->message);
> + }
> + require Net::SMTP::SSL;
>   $smtp = Net::SMTP::SSL->start_SSL($smtp,
> 
> ssl_verify_params())
> - or die "STARTTLS failed! 
> ".IO::Socket::SSL::errstr();
> - $smtp_encrypti

Re: ttk error when starting git gui

[Dennis Kaarsemaker]

On Thu, 2017-03-30 at 15:54 -0400, Peter van der Does wrote:

> It looks like the git gui needs TCL/TK 8.6.0 or higher. Since that
> version the command 'ttk::style theme use' has been changed, which
> allows the command to be run without an argument and then returning the
> current theme used.
> I believe RHEL6 use Tk-8.5.7 but I can't be 100% sure.

It does.

$ rpm -q tk
tk-8.5.7-5.el6.x86_64

D.

Re: [PATCH v4 1/2] diff --no-index: optionally follow symlinks

[Dennis Kaarsemaker]

On Fri, 2017-03-24 at 15:56 -0700, Junio C Hamano wrote:

> diff --git a/diff.c b/diff.c
> > index be11e4ef2b..2afecfb939 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s, 
> > unsigned int flags)
> > s->size = xsize_t(st.st_size);
> > if (!s->size)
> > goto empty;
> > -   if (S_ISLNK(st.st_mode)) {
> > +   if (S_ISLNK(s->mode)) {
> 
> This change is conceptually wrong.  s->mode (often) comes from the
> index but in this codepath, after finding that s->oid is not valid
> or we want to read from the working tree instead (several lines
> before this part), we are committed to read from the working tree
> and check things with st.st_* fields, not s->mode, when we decide
> what to do with the thing we find on the filesystem, no?

Hmm, true. It just accidentally does the right thing because s->mode
happens to always match the expectations of this code. I will pass on
more information into diff_populate_filespec so an explicit check can
be done here.

> > @@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s, 
> > unsigned int flags)
> > s->should_free = 1;
> > return 0;
> > }
> > +   if (S_ISLNK(st.st_mode)) {
> > +   stat(s->path, );
> > +   s->size = xsize_t(st.st_size);
> > +   }
> > if (size_only)
> > return 0;
> > if ((flags & CHECK_BINARY) &&
> 
> I suspect that this would conflict with a recent topic.  

Possibly. I used the same base commit for the newer versions as that
seems to be your preference. If there is a merge conflict, do you want
me to rebase against current master?

> But more importantly, this inserted code feels doubly wrong.
> 
>  - what allows us to unconditionally do "ah, symbolic link on the
>disk--find the target of the link, not the symbolic link itself"?
>We do not seem to be checking '--dereference' around here.

The implicit check above (which you already noted is faulty) allows us
to do this. So fixing the check above will also involve fixing this.

>  - does this code do a reasonable thing when the path is a symbolic
>link that points at a directory?  what does it mean to grab
>st.st_size for such a thing (and then go on to open() and xmmap()
>it)?

No, it does something entirely unreasonable. I hadn't even thought of
testing with symlinks to directories, as my ulterior motive was the
next commit that makes it work with pipes. This will be fixed.

Thanks very much for the thoroughness of your review!

D.

[PATCH v2] send-email: Net::SMTP::SSL is obsolete, use only when necessary

[Dennis Kaarsemaker]

Net::SMTP itself can do the necessary SSL and STARTTLS bits just fine
since version 1.28, and Net::SMTP::SSL is now deprecated. Since 1.28
isn't that old yet, keep the old code in place and use it when
necessary.

While we're in the area, mark some messages for translation that were
not yet marked as such.

Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
---
 git-send-email.perl | 54 ++---
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index eea0a517f7..0d90439d9a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1353,10 +1353,12 @@ EOF
die __("The required SMTP server is not properly 
defined.")
}
 
+   require Net::SMTP;
+   my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < 
version->parse("1.28");
+   $smtp_domain ||= maildomain();
+
if ($smtp_encryption eq 'ssl') {
$smtp_server_port ||= 465; # ssmtp
-   require Net::SMTP::SSL;
-   $smtp_domain ||= maildomain();
require IO::Socket::SSL;
 
# Suppress "variable accessed once" warning.
@@ -1368,34 +1370,48 @@ EOF
# Net::SMTP::SSL->new() does not forward any SSL options
IO::Socket::SSL::set_client_defaults(
ssl_verify_params());
-   $smtp ||= Net::SMTP::SSL->new($smtp_server,
- Hello => $smtp_domain,
- Port => $smtp_server_port,
- Debug => $debug_net_smtp);
+
+   if ($use_net_smtp_ssl) {
+   require Net::SMTP::SSL;
+   $smtp ||= Net::SMTP::SSL->new($smtp_server,
+ Hello => 
$smtp_domain,
+ Port => 
$smtp_server_port,
+ Debug => 
$debug_net_smtp);
+   }
+   else {
+   $smtp ||= Net::SMTP->new($smtp_server,
+Hello => $smtp_domain,
+Port => 
$smtp_server_port,
+Debug => 
$debug_net_smtp,
+SSL => 1);
+   }
}
else {
-   require Net::SMTP;
-   $smtp_domain ||= maildomain();
$smtp_server_port ||= 25;
$smtp ||= Net::SMTP->new($smtp_server,
 Hello => $smtp_domain,
 Debug => $debug_net_smtp,
 Port => $smtp_server_port);
if ($smtp_encryption eq 'tls' && $smtp) {
-   require Net::SMTP::SSL;
-   $smtp->command('STARTTLS');
-   $smtp->response();
-   if ($smtp->code == 220) {
+   if ($use_net_smtp_ssl) {
+   $smtp->command('STARTTLS');
+   $smtp->response();
+   if ($smtp->code != 220) {
+   die sprintf(__("Server does not 
support STARTTLS! %s"), $smtp->message);
+   }
+   require Net::SMTP::SSL;
$smtp = Net::SMTP::SSL->start_SSL($smtp,
  
ssl_verify_params())
-   or die "STARTTLS failed! 
".IO::Socket::SSL::errstr();
-   $smtp_encryption = '';
-   # Send EHLO again to receive fresh
-   # supported commands
-   $smtp->hello($smtp_domain);
-   } else {
-   die sprintf(__("Server does not support 
STARTTLS! %s"), $smtp->message);
+   or die sprintf(__("STARTTLS 
failed! %s"

[PATCH v4 0/2] diff --no-index: support symlinks and pipes

[Dennis Kaarsemaker]

git diff <(command1) <(command2) is less useful than it could be, all it 
outputs is:

diff --git a/dev/fd/63 b/dev/fd/62
index 9e6542b297..9f7b2c291b 12
--- a/dev/fd/63
+++ b/dev/fd/62
@@ -1 +1 @@
-pipe:[464811685]
\ No newline at end of file
+pipe:[464811687]
\ No newline at end of file

Normal diff provides arguably better output: the diff of the output of the
commands. This series makes it possible for git diff --no-index to follow
symlinks and read from pipes, mimicking the behaviour of normal diff.

v1: http://public-inbox.org/git/2016201958.2175-1-den...@kaarsemaker.net/
v2: http://public-inbox.org/git/20170113102021.6054-1-den...@kaarsemaker.net/
v3: http://public-inbox.org/git/20170318210038.22638-1-den...@kaarsemaker.net/

Changes since v3:
Using the --dereference option without being in explicit or implicit no-index
mode is no longer silently ignored, but an error. A test has been added for
this behaviour.

Dennis Kaarsemaker (2):
  diff --no-index: optionally follow symlinks
  diff --no-index: support reading from pipes

 Documentation/diff-options.txt |  9 +++
 builtin/diff.c |  2 ++
 diff-no-index.c| 16 ++---
 diff.c | 30 +++
 diff.h |  2 +-
 t/t4011-diff-symlink.sh|  6 +
 t/t4053-diff-no-index.sh   | 54 ++
 t/test-lib.sh  |  4 
 8 files changed, 115 insertions(+), 8 deletions(-)

-- 
2.12.0-488-gd3584ba

[PATCH v4 1/2] diff --no-index: optionally follow symlinks

[Dennis Kaarsemaker]

Git's diff machinery does not follow symlinks, which makes sense as git
itself also does not, but stores the symlink destination.

In --no-index mode however, it is useful for diff to be able to follow
symlinks, matching the behaviour of ordinary diff. A new --dereference
(name copied from diff) option has been added to enable this behaviour.
--no-dereference can be used to disable it again.

Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
---
 Documentation/diff-options.txt |  9 +
 builtin/diff.c |  2 ++
 diff-no-index.c|  7 ---
 diff.c | 12 ++--
 diff.h |  2 +-
 t/t4011-diff-symlink.sh|  6 ++
 t/t4053-diff-no-index.sh   | 44 ++
 7 files changed, 76 insertions(+), 6 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 2d77a19626..5a9d58b701 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -216,6 +216,15 @@ any of those replacements occurred.
commit range.  Defaults to `diff.submodule` or the 'short' format
if the config option is unset.
 
+ifdef::git-diff[]
+--dereference::
+--no-dereference::
+   Normally, "git diff --no-index" will compare symlinks by comparing what
+   they point to. The `--dereference` option will make it compare the 
content
+   of the linked files. The `--no-dereference` option disables an earlier
+   `--dereference`.
+endif::git-diff[]
+
 --color[=]::
Show colored diff.
`--color` (i.e. without '=') is the same as `--color=always`.
diff --git a/builtin/diff.c b/builtin/diff.c
index 7f91f6d226..09e646060e 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -360,6 +360,8 @@ int cmd_diff(int argc, const char **argv, const char 
*prefix)
if (nongit)
die(_("Not a git repository"));
argc = setup_revisions(argc, argv, , NULL);
+   if (DIFF_OPT_TST(, DEREFERENCE))
+   die(_("--dereference can only be used together with 
--no-index"));
if (!rev.diffopt.output_format) {
rev.diffopt.output_format = DIFF_FORMAT_PATCH;
diff_setup_done();
diff --git a/diff-no-index.c b/diff-no-index.c
index f420786039..fe48f32ddd 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -40,7 +40,7 @@ static int read_directory_contents(const char *path, struct 
string_list *list)
  */
 static const char file_from_standard_input[] = "-";
 
-static int get_mode(const char *path, int *mode)
+static int get_mode(const char *path, int *mode, int dereference)
 {
struct stat st;
 
@@ -52,7 +52,7 @@ static int get_mode(const char *path, int *mode)
 #endif
else if (path == file_from_standard_input)
*mode = create_ce_mode(0666);
-   else if (lstat(path, ))
+   else if (dereference ? stat(path, ) : lstat(path, ))
return error("Could not access '%s'", path);
else
*mode = st.st_mode;
@@ -93,7 +93,8 @@ static int queue_diff(struct diff_options *o,
 {
int mode1 = 0, mode2 = 0;
 
-   if (get_mode(name1, ) || get_mode(name2, ))
+   if (get_mode(name1, , DIFF_OPT_TST(o, DEREFERENCE)) ||
+   get_mode(name2, , DIFF_OPT_TST(o, DEREFERENCE)))
return -1;
 
if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
diff --git a/diff.c b/diff.c
index be11e4ef2b..2afecfb939 100644
--- a/diff.c
+++ b/diff.c
@@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
s->size = xsize_t(st.st_size);
if (!s->size)
goto empty;
-   if (S_ISLNK(st.st_mode)) {
+   if (S_ISLNK(s->mode)) {
struct strbuf sb = STRBUF_INIT;
 
if (strbuf_readlink(, s->path, s->size))
@@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
s->should_free = 1;
return 0;
}
+   if (S_ISLNK(st.st_mode)) {
+   stat(s->path, );
+   s->size = xsize_t(st.st_size);
+   }
if (size_only)
return 0;
if ((flags & CHECK_BINARY) &&
@@ -3884,7 +3888,11 @@ int diff_opt_parse(struct diff_options *options,
else if (!strcmp(arg, "--no-follow")) {
DIFF_OPT_CLR(options, FOLLOW_RENAMES);
DIFF_OPT_CLR(options, DEFAULT_FOLLOW_RENAMES);
-   } else if (!strcmp(arg, "--color"))
+   } else if (!strcmp(arg, "--dereference"))
+   DIFF_OPT_SET(options, DEREFERENCE);
+   else if (!strcmp(arg, "--no-dereference")

[PATCH v4 2/2] diff --no-index: support reading from pipes

[Dennis Kaarsemaker]

diff <(command1) <(command2) provides useful output, let's make it
possible for git to do the same.

Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
---
 diff-no-index.c  |  9 +
 diff.c   | 18 --
 t/t4053-diff-no-index.sh | 10 ++
 t/test-lib.sh|  4 
 4 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index fe48f32ddd..1262a587e5 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -83,6 +83,15 @@ static struct diff_filespec *noindex_filespec(const char 
*name, int mode)
name = "/dev/null";
s = alloc_filespec(name);
fill_filespec(s, null_sha1, 0, mode);
+   /*
+* In --no-index mode, we support reading from pipes. canon_mode, 
called by
+* fill_filespec, gets confused by this and thinks we now have 
subprojects.
+* To help the rest of the diff machinery along, we now override what
+* canon_mode says. This is done here instead of in canon_mode, because 
the
+* rest of git does not (and should not) support pipes.
+*/
+   if (S_ISFIFO(mode))
+   s->mode = S_IFREG | ce_permissions(mode);
if (name == file_from_standard_input)
populate_from_stdin(s);
return s;
diff --git a/diff.c b/diff.c
index 2afecfb939..4f74a54d74 100644
--- a/diff.c
+++ b/diff.c
@@ -2765,6 +2765,11 @@ static int diff_populate_gitlink(struct diff_filespec 
*s, int size_only)
return 0;
 }
 
+static int should_mmap_file_contents(struct stat *st)
+{
+   return S_ISREG(st->st_mode);
+}
+
 /*
  * While doing rename detection and pickaxe operation, we may need to
  * grab the data for the blob (or file) for our own in-core comparison.
@@ -2839,9 +2844,18 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
fd = open(s->path, O_RDONLY);
if (fd < 0)
goto err_empty;
-   s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
+   if (!should_mmap_file_contents()) {
+   struct strbuf sb = STRBUF_INIT;
+   strbuf_read(, fd, 0);
+   s->size = sb.len;
+   s->data = strbuf_detach(, NULL);
+   s->should_free = 1;
+   }
+   else {
+   s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, 
fd, 0);
+   s->should_munmap = 1;
+   }
close(fd);
-   s->should_munmap = 1;
 
/*
 * Convert from working tree format to canonical git format
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 8c87bffb34..2d9b322315 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -171,4 +171,14 @@ test_expect_success SYMLINKS 'diff --no-index 
--no-dereference does not follow s
test_cmp expect actual
 '
 
+test_expect_success PROCESS_SUBSTITUTION 'diff --no-index works on fifos' '
+   cat >expect <<-EOF &&
+   @@ -1 +1 @@
+   -1
+   +2
+   EOF
+   test_expect_code 1 git diff --no-index --dereference <(echo 1) <(echo 
2) | tail -n +5 > actual &&
+   test_cmp expect actual
+'
+
 test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11562bde10..78f3d24651 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1128,3 +1128,7 @@ build_option () {
 test_lazy_prereq LONG_IS_64BIT '
test 8 -le "$(build_option sizeof-long)"
 '
+
+test_lazy_prereq PROCESS_SUBSTITUTION '
+   eval "foo=<(echo test)" 2>/dev/null
+'
-- 
2.12.0-488-gd3584ba

Re: blame --line-porcelain is providing me with funny output

[Dennis Kaarsemaker]

On Wed, 2017-03-22 at 18:32 -0600, Edmundo Carmona Antoranz wrote:
> 
> $ git blame --no-progress -w --line-porcelain -L 72,72
> author-mail <somem...@gmail.com>
> 
> $ git cat-file -p 3290fe6dd2a7e2bb35ac760443335dec58802ff1
> author Stefan Beller <somem...@google.com> 1484160452 -0800
> 
> Committer mails are matching, however author mail does not match
> between line-porcelain and cat-file. Is there a reason for that?

The commit object has Stefan's Google address, but git.git's  mailmap
maps that to his gmail address.

git blame actually does this mapping, where git cat-file does not.
-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

Re: [PATCH v3 0/2] diff --no-index: support symlinks and pipes

[Dennis Kaarsemaker]

On Sun, 2017-03-19 at 15:08 -0700, Junio C Hamano wrote:
> Dennis Kaarsemaker <den...@kaarsemaker.net> writes:
> 
> > Normal diff provides arguably better output: the diff of the output of the
> > commands. This series makes it possible for git diff --no-index to follow
> > symlinks and read from pipes, mimicking the behaviour of normal diff.
> > 
> > v1: 
> > http://public-inbox.org/git/2016201958.2175-1-den...@kaarsemaker.net/
> > v2: 
> > http://public-inbox.org/git/20170113102021.6054-1-den...@kaarsemaker.net/
> > 
> > Changes since v2, prompted by feedback from Junio:
> > 
> > - A --derefence option was added and the default is no longer to dereference
> >   symlinks.
> 
> I do agree that it makes sense to have --[no-]dereference options,
> but I do not think it was my feedback and suggestion to make it
> optional (not default) to dereference, so please do not blame me for
> that choice.

Then I misinterpreted your message at 
http://public-inbox.org/git/xmqqk29yedkv....@gitster.mtv.corp.google.com/
No blame inteded, my apologies for coming across as blaming.

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

Re: [PATCH v3 1/2] diff --no-index: optionally follow symlinks

[Dennis Kaarsemaker]

On Sun, 2017-03-19 at 15:14 -0700, Junio C Hamano wrote:
> Dennis Kaarsemaker <den...@kaarsemaker.net> writes:
> 
> > diff --git a/diff.c b/diff.c
> > index be11e4ef2b..2afecfb939 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s, 
> > unsigned int flags)
> > s->size = xsize_t(st.st_size);
> > if (!s->size)
> > goto empty;
> > -   if (S_ISLNK(st.st_mode)) {
> > +   if (S_ISLNK(s->mode)) {
> > struct strbuf sb = STRBUF_INIT;
> >  
> > if (strbuf_readlink(, s->path, s->size))
> > @@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s, 
> > unsigned int flags)
> > s->should_free = 1;
> > return 0;
> > }
> > +   if (S_ISLNK(st.st_mode)) {
> > +   stat(s->path, );
> > +   s->size = xsize_t(st.st_size);
> 
> Doesn't this affect --no-index mode?  We never need to do a wasteful
> stat() after lstat() and we are penalizing the normal codepath with
> this change, no?

the S_ISLNK(s->mode) conditional above is for the normal codepath,
which returns early. So the stat I added is only done for symlinks in
no_index mode.

> > @@ -3884,7 +3888,11 @@ int diff_opt_parse(struct diff_options *options,
> > else if (!strcmp(arg, "--no-follow")) {
> > DIFF_OPT_CLR(options, FOLLOW_RENAMES);
> > DIFF_OPT_CLR(options, DEFAULT_FOLLOW_RENAMES);
> > -   } else if (!strcmp(arg, "--color"))
> > +   } else if (!strcmp(arg, "--dereference"))
> > +   DIFF_OPT_SET(options, DEREFERENCE);
> > +   else if (!strcmp(arg, "--no-dereference"))
> > +   DIFF_OPT_CLR(options, DEREFERENCE);
> > +   else if (!strcmp(arg, "--color"))
> > options->use_color = 1;
> 
> Also shouldn't be some code to detect --[no-]dereference options
> given when --no-index is not in effect and error out?  As the patch
> title says, this change should be a no-op for normal codepath and
> only affect the no-index hack.

But erroring out isn't a no-op. With the current patch you can do 
--dereference without --no-index and it simply wouldn't affect
anything. 

I don't mind either way, so I'll make it error out.
-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

Re: [PATCH] send-email: Net::SMTP::SSL is obsolete, use only when necessary

[Dennis Kaarsemaker]

On Sat, 2017-03-18 at 23:47 +0100, Ævar Arnfjörð Bjarmason wrote:

> On Sat, Mar 18, 2017 at 11:23 PM, Dennis Kaarsemaker
> <den...@kaarsemaker.net> wrote:
>
> > +   require Net::SMTP;
> > +   my $use_net_smtp_ssl = $Net::SMTP::VERSION lt "1.28";
> > +   $smtp_domain ||= maildomain();
> > +
> 
> While Net::SMTP is unlikely to change its versioning scheme, let's use
> comparisons via the version module here in case they do change it to
> something silly, and this ends up introducing a bug.

ok.

> > [...]
> > +   if ($smtp->code != 220) {
> > +   die sprintf(__("Server does 
> > not support STARTTLS! %s"), $smtp->message);
> 
> Here a new message you're adding gets __(), makes sense.

Didn't add it, it just moved from a bit further below :)

> > +   else {
> > +   $smtp->starttls(ssl_verify_params())
> > +   or die "STARTTLS failed! 
> > ".IO::Socket::SSL::errstr();
> > +   }
> 
> I see you just copied that from above but I wonder if it makes sense
> to just mark both occurrences with __() too while we're at it.

ok.

D.

Re: How do I make 'git diff --no-index' follow symlinks?

[Dennis Kaarsemaker]

On Sat, 2017-03-18 at 12:30 -0700, Junio C Hamano wrote:
> Sounds like
> 
> https://public-inbox.org/git/2016201958.2175-1-den...@kaarsemaker.net/
> 
> to me.  A key message in the thread may be:
> 
> 
> https://public-inbox.org/git/alpine.DEB.2.20.1611121106110.3746@virtualbox/

Sorry for the delay in sending v3. I've had a serious case of
Lennonitis (Life is what happens to you while you're busy making other
plans).

D.

[PATCH] send-email: Net::SMTP::SSL is obsolete, use only when necessary

[Dennis Kaarsemaker]

Net::SMTP itself can do the necessary SSL and STARTTLS bits just fine
since version 1.28, and Net::SMTP::SSL is now deprecated. Since 1.28
isn't that old yet, keep the old code in place and use it when
necessary.

Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
---
 Note: I've only been able to test the starttls bits. None of the smtp servers
 I use actually use ssl, only starttls.

 git-send-email.perl | 52 ++--
 1 file changed, 34 insertions(+), 18 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index eea0a517f7..e247ea39dd 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1353,10 +1353,12 @@ EOF
die __("The required SMTP server is not properly 
defined.")
}
 
+   require Net::SMTP;
+   my $use_net_smtp_ssl = $Net::SMTP::VERSION lt "1.28";
+   $smtp_domain ||= maildomain();
+
if ($smtp_encryption eq 'ssl') {
$smtp_server_port ||= 465; # ssmtp
-   require Net::SMTP::SSL;
-   $smtp_domain ||= maildomain();
require IO::Socket::SSL;
 
# Suppress "variable accessed once" warning.
@@ -1368,34 +1370,48 @@ EOF
# Net::SMTP::SSL->new() does not forward any SSL options
IO::Socket::SSL::set_client_defaults(
ssl_verify_params());
-   $smtp ||= Net::SMTP::SSL->new($smtp_server,
- Hello => $smtp_domain,
- Port => $smtp_server_port,
- Debug => $debug_net_smtp);
+
+   if ($use_net_smtp_ssl) {
+   require Net::SMTP::SSL;
+   $smtp ||= Net::SMTP::SSL->new($smtp_server,
+ Hello => 
$smtp_domain,
+ Port => 
$smtp_server_port,
+ Debug => 
$debug_net_smtp);
+   }
+   else {
+   $smtp ||= Net::SMTP->new($smtp_server,
+Hello => $smtp_domain,
+Port => 
$smtp_server_port,
+Debug => 
$debug_net_smtp,
+SSL => 1);
+   }
}
else {
-   require Net::SMTP;
-   $smtp_domain ||= maildomain();
$smtp_server_port ||= 25;
$smtp ||= Net::SMTP->new($smtp_server,
 Hello => $smtp_domain,
 Debug => $debug_net_smtp,
 Port => $smtp_server_port);
if ($smtp_encryption eq 'tls' && $smtp) {
-   require Net::SMTP::SSL;
-   $smtp->command('STARTTLS');
-   $smtp->response();
-   if ($smtp->code == 220) {
+   if ($use_net_smtp_ssl) {
+   $smtp->command('STARTTLS');
+   $smtp->response();
+   if ($smtp->code != 220) {
+   die sprintf(__("Server does not 
support STARTTLS! %s"), $smtp->message);
+   }
+   require Net::SMTP::SSL;
$smtp = Net::SMTP::SSL->start_SSL($smtp,
  
ssl_verify_params())
or die "STARTTLS failed! 
".IO::Socket::SSL::errstr();
-   $smtp_encryption = '';
-   # Send EHLO again to receive fresh
-   # supported commands
-   $smtp->hello($smtp_domain);
-   } else {
-   die sprintf(__("Server does not support 
STARTTLS! %s"), $smtp->message);
}
+   else {
+ 

[PATCH v3 1/2] diff --no-index: optionally follow symlinks

[Dennis Kaarsemaker]

Git's diff machinery does not follow symlinks, which makes sense as git
itself also does not, but stores the symlink destination.

In --no-index mode however, it is useful for diff to be able to follow
symlinks, matching the behaviour of ordinary diff. A new --dereference
(name copied from diff) option has been added to enable this behaviour.
--no-dereference can be used to disable it again.

Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
---
 Documentation/diff-options.txt |  9 +
 diff-no-index.c|  7 ---
 diff.c | 12 ++--
 diff.h |  2 +-
 t/t4053-diff-no-index.sh   | 44 ++
 5 files changed, 68 insertions(+), 6 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 2d77a19626..5a9d58b701 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -216,6 +216,15 @@ any of those replacements occurred.
commit range.  Defaults to `diff.submodule` or the 'short' format
if the config option is unset.
 
+ifdef::git-diff[]
+--dereference::
+--no-dereference::
+   Normally, "git diff --no-index" will compare symlinks by comparing what
+   they point to. The `--dereference` option will make it compare the 
content
+   of the linked files. The `--no-dereference` option disables an earlier
+   `--dereference`.
+endif::git-diff[]
+
 --color[=]::
Show colored diff.
`--color` (i.e. without '=') is the same as `--color=always`.
diff --git a/diff-no-index.c b/diff-no-index.c
index f420786039..fe48f32ddd 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -40,7 +40,7 @@ static int read_directory_contents(const char *path, struct 
string_list *list)
  */
 static const char file_from_standard_input[] = "-";
 
-static int get_mode(const char *path, int *mode)
+static int get_mode(const char *path, int *mode, int dereference)
 {
struct stat st;
 
@@ -52,7 +52,7 @@ static int get_mode(const char *path, int *mode)
 #endif
else if (path == file_from_standard_input)
*mode = create_ce_mode(0666);
-   else if (lstat(path, ))
+   else if (dereference ? stat(path, ) : lstat(path, ))
return error("Could not access '%s'", path);
else
*mode = st.st_mode;
@@ -93,7 +93,8 @@ static int queue_diff(struct diff_options *o,
 {
int mode1 = 0, mode2 = 0;
 
-   if (get_mode(name1, ) || get_mode(name2, ))
+   if (get_mode(name1, , DIFF_OPT_TST(o, DEREFERENCE)) ||
+   get_mode(name2, , DIFF_OPT_TST(o, DEREFERENCE)))
return -1;
 
if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
diff --git a/diff.c b/diff.c
index be11e4ef2b..2afecfb939 100644
--- a/diff.c
+++ b/diff.c
@@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
s->size = xsize_t(st.st_size);
if (!s->size)
goto empty;
-   if (S_ISLNK(st.st_mode)) {
+   if (S_ISLNK(s->mode)) {
struct strbuf sb = STRBUF_INIT;
 
if (strbuf_readlink(, s->path, s->size))
@@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
s->should_free = 1;
return 0;
}
+   if (S_ISLNK(st.st_mode)) {
+   stat(s->path, );
+   s->size = xsize_t(st.st_size);
+   }
if (size_only)
return 0;
if ((flags & CHECK_BINARY) &&
@@ -3884,7 +3888,11 @@ int diff_opt_parse(struct diff_options *options,
else if (!strcmp(arg, "--no-follow")) {
DIFF_OPT_CLR(options, FOLLOW_RENAMES);
DIFF_OPT_CLR(options, DEFAULT_FOLLOW_RENAMES);
-   } else if (!strcmp(arg, "--color"))
+   } else if (!strcmp(arg, "--dereference"))
+   DIFF_OPT_SET(options, DEREFERENCE);
+   else if (!strcmp(arg, "--no-dereference"))
+   DIFF_OPT_CLR(options, DEREFERENCE);
+   else if (!strcmp(arg, "--color"))
options->use_color = 1;
else if (skip_prefix(arg, "--color=", )) {
int value = git_config_colorbool(NULL, arg);
diff --git a/diff.h b/diff.h
index 25ae60d5ff..db33dc67f6 100644
--- a/diff.h
+++ b/diff.h
@@ -69,7 +69,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct 
diff_options *opt, void *data)
 #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)
 #define DIFF_OPT_FOLLOW_RENAMES  (1 <<  7)
 #define DIFF_OPT_RENAME_EMPTY(1 <<  8)
-/* (1 <<  9) unused */
+#define DIFF_OPT_DEREFERENCE (1 &

[PATCH v3 0/2] diff --no-index: support symlinks and pipes

[Dennis Kaarsemaker]

git diff <(command1) <(command2) is less useful than it could be, all it 
outputs is:

diff --git a/dev/fd/63 b/dev/fd/62
index 9e6542b297..9f7b2c291b 12
--- a/dev/fd/63
+++ b/dev/fd/62
@@ -1 +1 @@
-pipe:[464811685]
\ No newline at end of file
+pipe:[464811687]
\ No newline at end of file

Normal diff provides arguably better output: the diff of the output of the
commands. This series makes it possible for git diff --no-index to follow
symlinks and read from pipes, mimicking the behaviour of normal diff.

v1: http://public-inbox.org/git/2016201958.2175-1-den...@kaarsemaker.net/
v2: http://public-inbox.org/git/20170113102021.6054-1-den...@kaarsemaker.net/

Changes since v2, prompted by feedback from Junio:

- A --derefence option was added and the default is no longer to dereference
  symlinks.
- Instead of looking at what canon_mode returns, use the original mode of 
  files to override behaviour for pipes.
- Turn the !S_ISREG(...) check into a should_mmap_file_contents helper.

Dennis Kaarsemaker (2):
  diff --no-index: optionally follow symlinks
  diff --no-index: support reading from pipes

 Documentation/diff-options.txt |  9 +++
 diff-no-index.c| 16 ++---
 diff.c | 30 +++
 diff.h |  2 +-
 t/t4053-diff-no-index.sh   | 54 ++
 t/test-lib.sh  |  4 
 6 files changed, 107 insertions(+), 8 deletions(-)

-- 
2.12.0-437-g0cc2799

[PATCH v3 2/2] diff --no-index: support reading from pipes

[Dennis Kaarsemaker]

diff <(command1) <(command2) provides useful output, let's make it
possible for git to do the same.

Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
---
 diff-no-index.c  |  9 +
 diff.c   | 18 --
 t/t4053-diff-no-index.sh | 10 ++
 t/test-lib.sh|  4 
 4 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index fe48f32ddd..1262a587e5 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -83,6 +83,15 @@ static struct diff_filespec *noindex_filespec(const char 
*name, int mode)
name = "/dev/null";
s = alloc_filespec(name);
fill_filespec(s, null_sha1, 0, mode);
+   /*
+* In --no-index mode, we support reading from pipes. canon_mode, 
called by
+* fill_filespec, gets confused by this and thinks we now have 
subprojects.
+* To help the rest of the diff machinery along, we now override what
+* canon_mode says. This is done here instead of in canon_mode, because 
the
+* rest of git does not (and should not) support pipes.
+*/
+   if (S_ISFIFO(mode))
+   s->mode = S_IFREG | ce_permissions(mode);
if (name == file_from_standard_input)
populate_from_stdin(s);
return s;
diff --git a/diff.c b/diff.c
index 2afecfb939..4f74a54d74 100644
--- a/diff.c
+++ b/diff.c
@@ -2765,6 +2765,11 @@ static int diff_populate_gitlink(struct diff_filespec 
*s, int size_only)
return 0;
 }
 
+static int should_mmap_file_contents(struct stat *st)
+{
+   return S_ISREG(st->st_mode);
+}
+
 /*
  * While doing rename detection and pickaxe operation, we may need to
  * grab the data for the blob (or file) for our own in-core comparison.
@@ -2839,9 +2844,18 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
fd = open(s->path, O_RDONLY);
if (fd < 0)
goto err_empty;
-   s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
+   if (!should_mmap_file_contents()) {
+   struct strbuf sb = STRBUF_INIT;
+   strbuf_read(, fd, 0);
+   s->size = sb.len;
+   s->data = strbuf_detach(, NULL);
+   s->should_free = 1;
+   }
+   else {
+   s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, 
fd, 0);
+   s->should_munmap = 1;
+   }
close(fd);
-   s->should_munmap = 1;
 
/*
 * Convert from working tree format to canonical git format
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 8c87bffb34..2d9b322315 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -171,4 +171,14 @@ test_expect_success SYMLINKS 'diff --no-index 
--no-dereference does not follow s
test_cmp expect actual
 '
 
+test_expect_success PROCESS_SUBSTITUTION 'diff --no-index works on fifos' '
+   cat >expect <<-EOF &&
+   @@ -1 +1 @@
+   -1
+   +2
+   EOF
+   test_expect_code 1 git diff --no-index --dereference <(echo 1) <(echo 
2) | tail -n +5 > actual &&
+   test_cmp expect actual
+'
+
 test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11562bde10..78f3d24651 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1128,3 +1128,7 @@ build_option () {
 test_lazy_prereq LONG_IS_64BIT '
test 8 -le "$(build_option sizeof-long)"
 '
+
+test_lazy_prereq PROCESS_SUBSTITUTION '
+   eval "foo=<(echo test)" 2>/dev/null
+'
-- 
2.12.0-437-g0cc2799

Re: [PATCH 4/4] ident: do not ignore empty config name/email

[Dennis Kaarsemaker]

On Thu, 2017-02-23 at 23:18 -0500, Jeff King wrote:
> On Thu, Feb 23, 2017 at 08:11:11PM -0800, Junio C Hamano wrote:
> 
> > > So I dunno. I could really go either way on it. Feel free to drop it, or
> > > even move it into a separate topic to be cooked longer.
> > 
> > If it were 5 years ago, it would have been different, but I do not
> > think cooking it longer in 'next' would smoke out breakages in
> > obscure scripts any longer.  Git is used by too many people who have
> > never seen its source these days.
> 
> Yeah, I have noticed that, too. I wonder if it would be interesting to
> cut "weeklies" or something of "master" or even "next" that people could
> install with a single click.
> 
> Of course it's not like we have a binary installer in the first place,
> so I guess that's a prerequisite.

I provide daily[*] snapshots of git's master and next tree as packages
for Ubuntu, Debian, Fedora and CentOS on launchpad and SuSE's
openbuildservice. If there's sufficient interest in this (I know of
only a few users), I can try to put more effort into this.

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

[*]When the tooling isn't broken for some reason.

[PATCH v2 2/2] diff --no-index: support reading from pipes

[Dennis Kaarsemaker]

diff <(command1) <(command2) provides useful output, let's make it
possible for git to do the same.

Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
---
 diff-no-index.c  |  8 
 diff.c   | 13 +++--
 t/t4053-diff-no-index.sh | 10 ++
 t/test-lib.sh|  4 
 4 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index 826fe97ffc..2123da435b 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -83,6 +83,14 @@ static struct diff_filespec *noindex_filespec(const char 
*name, int mode)
name = "/dev/null";
s = alloc_filespec(name);
fill_filespec(s, null_sha1, 0, mode);
+   /*
+* In --no-index mode, we support reading from pipes. canon_mode, 
called by
+* fill_filespec, gets confused by this and thinks we now have 
subprojects.
+* Detect this and tell the rest of the diff machinery to treat pipes as
+* normal files.
+*/
+   if (S_ISGITLINK(s->mode))
+   s->mode = S_IFREG | ce_permissions(mode);
if (name == file_from_standard_input)
populate_from_stdin(s);
return s;
diff --git a/diff.c b/diff.c
index 2fc0226338..bb04eab331 100644
--- a/diff.c
+++ b/diff.c
@@ -2839,9 +2839,18 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
fd = open(s->path, O_RDONLY);
if (fd < 0)
goto err_empty;
-   s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
+   if (!S_ISREG(st.st_mode)) {
+   struct strbuf sb = STRBUF_INIT;
+   strbuf_read(, fd, 0);
+   s->size = sb.len;
+   s->data = strbuf_detach(, NULL);
+   s->should_free = 1;
+   }
+   else {
+   s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, 
fd, 0);
+   s->should_munmap = 1;
+   }
close(fd);
-   s->should_munmap = 1;
 
/*
 * Convert from working tree format to canonical git format
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index c6046fef19..ba343566c0 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -157,4 +157,14 @@ test_expect_success SYMLINKS 'diff --no-index 
--no-dereference does not follow s
test_cmp expect actual
 '
 
+test_expect_success PROCESS_SUBSTITUTION 'diff --no-index works on fifos' '
+   cat >expect <<\EOF
+   @@ -1 +1 @@
+   -1
+   +2
+   EOF
+   test_expect_code 1 git diff --no-index <(echo 1) <(echo 2) | tail -n +5 
> actual &&
+   test_cmp expect actual
+'
+
 test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11562bde10..78f3d24651 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1128,3 +1128,7 @@ build_option () {
 test_lazy_prereq LONG_IS_64BIT '
test 8 -le "$(build_option sizeof-long)"
 '
+
+test_lazy_prereq PROCESS_SUBSTITUTION '
+   eval "foo=<(echo test)" 2>/dev/null
+'
-- 
2.11.0-234-gaf85957

[PATCH v2 0/2] diff --no-index: support symlinks and pipes

[Dennis Kaarsemaker]

git diff <(command1) <(command2) is less useful than it could be, all it 
outputs is:

diff --git a/dev/fd/63 b/dev/fd/62
index 9e6542b297..9f7b2c291b 12
--- a/dev/fd/63
+++ b/dev/fd/62
@@ -1 +1 @@
-pipe:[464811685]
\ No newline at end of file
+pipe:[464811687]
\ No newline at end of file

Normal diff provides arguably better output: the diff of the output of the
commands. This series makes it possible for git diff --no-index to follow
symlinks and read from pipes, mimicking the behaviour of normal diff.

v1: http://public-inbox.org/git/2016201958.2175-1-den...@kaarsemaker.net/

Changes since the RFC/v1 patch:
- Following symlinks is now the default. I think an accurate summary of the
  discussion on v1 is that this behaviour is useful enough to be the default,
  but to add an escape hatch. That escape hatch is named --no-dereference, name
  stolen from gnu diff.
- Added tests and documentation

Specifically not changed:
These changes affect only diff --no-index. Using --no-dereference is an error
without --no-index.

Dennis Kaarsemaker (2):
  diff --no-index: follow symlinks
  diff --no-index: support reading from pipes

 Documentation/diff-options.txt |  7 +++
 diff-no-index.c| 15 ---
 diff.c | 23 +++
 diff.h |  2 +-
 t/t4053-diff-no-index.sh   | 40 
 t/test-lib.sh  |  4 
 6 files changed, 83 insertions(+), 8 deletions(-)

-- 
2.11.0-234-gaf85957

[PATCH v2 1/2] diff --no-index: follow symlinks

[Dennis Kaarsemaker]

Git's diff machinery does not follow symlinks, which makes sense as git
itself also does not, but stores the symlink destination.

In --no-index mode however, it is useful for diff to to follow symlinks,
matching the behaviour of ordinary diff. A new --no-dereference (name
copied from diff) option has been added to disable this behaviour.

Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
---
 Documentation/diff-options.txt |  7 +++
 diff-no-index.c|  7 ---
 diff.c | 10 --
 diff.h |  2 +-
 t/t4053-diff-no-index.sh   | 30 ++
 5 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 2d77a19626..48bcf3cc5e 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -216,6 +216,13 @@ any of those replacements occurred.
commit range.  Defaults to `diff.submodule` or the 'short' format
if the config option is unset.
 
+ifdef::git-diff[]
+--no-dereference::
+   Normally, "git diff --no-index" will dereference symlinks and compare
+   the contents of the linked files, mimicking ordinary diff. This
+   option disables that behaviour.
+endif::git-diff[]
+
 --color[=]::
Show colored diff.
`--color` (i.e. without '=') is the same as `--color=always`.
diff --git a/diff-no-index.c b/diff-no-index.c
index f420786039..826fe97ffc 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -40,7 +40,7 @@ static int read_directory_contents(const char *path, struct 
string_list *list)
  */
 static const char file_from_standard_input[] = "-";
 
-static int get_mode(const char *path, int *mode)
+static int get_mode(const char *path, int *mode, int no_dereference)
 {
struct stat st;
 
@@ -52,7 +52,7 @@ static int get_mode(const char *path, int *mode)
 #endif
else if (path == file_from_standard_input)
*mode = create_ce_mode(0666);
-   else if (lstat(path, ))
+   else if (no_dereference ? lstat(path, ) : stat(path, ))
return error("Could not access '%s'", path);
else
*mode = st.st_mode;
@@ -93,7 +93,8 @@ static int queue_diff(struct diff_options *o,
 {
int mode1 = 0, mode2 = 0;
 
-   if (get_mode(name1, ) || get_mode(name2, ))
+   if (get_mode(name1, , DIFF_OPT_TST(o, NO_DEREFERENCE)) ||
+   get_mode(name2, , DIFF_OPT_TST(o, NO_DEREFERENCE)))
return -1;
 
if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
diff --git a/diff.c b/diff.c
index be11e4ef2b..2fc0226338 100644
--- a/diff.c
+++ b/diff.c
@@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
s->size = xsize_t(st.st_size);
if (!s->size)
goto empty;
-   if (S_ISLNK(st.st_mode)) {
+   if (S_ISLNK(s->mode)) {
struct strbuf sb = STRBUF_INIT;
 
if (strbuf_readlink(, s->path, s->size))
@@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
s->should_free = 1;
return 0;
}
+   if (S_ISLNK(st.st_mode)) {
+   stat(s->path, );
+   s->size = xsize_t(st.st_size);
+   }
if (size_only)
return 0;
if ((flags & CHECK_BINARY) &&
@@ -3884,7 +3888,9 @@ int diff_opt_parse(struct diff_options *options,
else if (!strcmp(arg, "--no-follow")) {
DIFF_OPT_CLR(options, FOLLOW_RENAMES);
DIFF_OPT_CLR(options, DEFAULT_FOLLOW_RENAMES);
-   } else if (!strcmp(arg, "--color"))
+   } else if (!strcmp(arg, "--no-dereference"))
+   DIFF_OPT_SET(options, NO_DEREFERENCE);
+   else if (!strcmp(arg, "--color"))
options->use_color = 1;
else if (skip_prefix(arg, "--color=", )) {
int value = git_config_colorbool(NULL, arg);
diff --git a/diff.h b/diff.h
index 25ae60d5ff..74883db1eb 100644
--- a/diff.h
+++ b/diff.h
@@ -69,7 +69,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct 
diff_options *opt, void *data)
 #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)
 #define DIFF_OPT_FOLLOW_RENAMES  (1 <<  7)
 #define DIFF_OPT_RENAME_EMPTY(1 <<  8)
-/* (1 <<  9) unused */
+#define DIFF_OPT_NO_DEREFERENCE  (1 <<  9)
 #define DIFF_OPT_HAS_CHANGES (1 << 10)
 #define DIFF_OPT_QUICK   (1 << 11)
 #define DIFF_OPT_NO_INDEX(1 << 12)
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 453e

Re: "git fsck" not detecting garbage at the end of blob object files...

[Dennis Kaarsemaker]

On Sat, 2017-01-07 at 07:50 -0500, John Szakmeister wrote:
> I was perusing StackOverflow this morning and ran across this
> question: 
> http://stackoverflow.com/questions/41521143/git-fsck-full-only-checking-directories/
> 
> It was a simple question about why "checking objects" was not
> appearing, but in it was another issue.  The user purposefully
> corrupted a blob object file to see if `git fsck` would catch it by
> tacking extra data on at the end.  `git fsck` happily said everything
> was okay, but when I played with things locally I found out that `git
> gc` does not like that extra garbage.  I'm not sure what the trade-off
> needs to be here, but my expectation is that if `git fsck` says
> everything is okay, then all operations using that object (file)
> should work too.
> 
> Is that unreasonable?  What would be the impact of fixing this issue?

If you do this with a commit object or tree object, fsck does complain.
I think it's sensible to do so for blob objects as well.

Editing blob object:

hurricane:/tmp/moo (master)$ hexer 
.git/objects/a1/b3ebb97f10ff8d85a9472bcba50cb575dbd485 
hurricane:/tmp/moo (master)$ git status
On branch master
nothing to commit, working tree clean
hurricane:/tmp/moo (master)$ git fsck
Checking object directories: 100% (256/256), done.
hurricane:/tmp/moo (master)$ git gc
Counting objects: 3, done.
error: garbage at end of loose object 'a1b3ebb97f10ff8d85a9472bcba50cb575dbd485'
fatal: loose object a1b3ebb97f10ff8d85a9472bcba50cb575dbd485 (stored in 
.git/objects/a1/b3ebb97f10ff8d85a9472bcba50cb575dbd485) is corrupt
error: failed to run repack

Editing tree object:

hurricane:/tmp/moo (master)$ hexer 
.git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20
hurricane:/tmp/moo (master +)$ git status
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in 
.git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in 
.git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt
hurricane:/tmp/moo (master +)$ git fsck
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in 
.git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt
error: garbage at end of loose object 'd4eda486f02e3e862e23f6eb3739a25a2ca43f20'
fatal: loose object d4eda486f02e3e862e23f6eb3739a25a2ca43f20 (stored in 
.git/objects/d4/eda486f02e3e862e23f6eb3739a25a2ca43f20) is corrupt

Editing commit object:

hurricane:/tmp/moo (master)$ echo test >> 
.git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0 
hurricane:/tmp/moo (master +)$ git status
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in 
.git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in 
.git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt
!(128) hurricane:/tmp/moo (master +)$ git fsck
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in 
.git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt
error: garbage at end of loose object '4759a693f7e8362c724d3365fe6df398083fafa0'
fatal: loose object 4759a693f7e8362c724d3365fe6df398083fafa0 (stored in 
.git/objects/47/59a693f7e8362c724d3365fe6df398083fafa0) is corrupt

D.

Re: [PATCH v2 3/6] update_unicode.sh: pin the uniset repo to a known good commit

[Dennis Kaarsemaker]

On Wed, 2016-12-14 at 00:31 +0100, Beat Bolli wrote:
> +   ( cd uniset && git checkout 4b186196dd )

Micronit, but this is perhaps better written as

git -C uniset checkout 4b186196dd

to avoid the subshell and cd.

D.

Re: git-daemon regression: 650c449250d7 common-main: call git_extract_argv0_path()

[Dennis Kaarsemaker]

Hi Mike,

On Sat, 2016-11-26 at 15:03 +0100, Mike Galbraith wrote:
> Greetings,
> 
> git-daemon went broke on me post v2.9.3 due to binaries being installed
> in /usr/lib/git, which is not in PATH.  Reverting 650c449250d7 fixes it
> up, as does ln -s /usr/lib/git/git-daemon /usr/bin/git-daemon 'course,
> but thought I should report it, since it used to work without that.

I don't know how you usually install git, but git-daemon is not
supposed to be in $PATH, the correct way to invoke the git daemon is
'git daemon' not 'git-daemon'

Having all subcommands of git as separate binaries in your $PATH is an
ancient git.git practice that stopped being used/supported quite a
while ago.

I don't know why this patch broke that obsolete practice, but hopefully
this can help you forward.

D.

Re: [PATCH 2/2] difftool: add a feature flag for the builtin vs scripted version

[Dennis Kaarsemaker]

On Tue, 2016-11-22 at 18:01 +0100, Johannes Schindelin wrote:
> The original idea was to use an environment variable
> GIT_USE_BUILTIN_DIFFTOOL, but the test suite resets those variables, and
> we do want to use that feature flag to run the tests with, and without,
> the feature flag.
> 
> Besides, the plan is to add an opt-in flag in Git for Windows'
> installer. If we implemented the feature flag as an environment
> variable, we would have to modify the user's environment, in order to
> make the builtin difftool the default when called from Git Bash, Git CMD
> or third-party tools.

Hi Johannes,

Why is this not a normal configuration variable (as in git config
difftool.builtin true or something)? It doesn't make much sense to me
to introduce a way of configuring git by introducing magic files, when
a normal configuration variable would do just fine, and the GfW
installer can also set such variables, like it does for the crlf config
I believe.

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

Re: Feature request - show result of URL rewrites

[Dennis Kaarsemaker]

On Sat, 2016-11-12 at 18:49 +0100, Git User wrote:
> Hello
> 
> Hopefully this is the right place to submit feature requests - let me
> know if there's somewhere else I should use!
> 
> Git lets you rewrite URLs using "url..insteadOf"
> 
> https://stackoverflow.com/a/11383587
> https://git-scm.com/docs/git-config
> 
> Can you add a git-config option to show the result of this rewriting
> whenever this occurs, as debugging more complicated rules can be
> difficult/wasn't obvious without Wireshark.
> 
> E.g. you could have the option 'url.printRewrites [True/False]' which
> would print the line "Rewrote url 'git://github.com/git/git' to 'http
> s://github.com/git/git'" to terminal/stdout  when set to True.

Such a configuration would be superfluous, the GIT_TRACE and
GIT_CURL_VERBOSE environment variables already provide all the
debugging information you need here.

D.

Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)

[Dennis Kaarsemaker]

On Fri, 2016-11-11 at 13:27 -0800, Junio C Hamano wrote:
> Dennis Kaarsemaker <den...@kaarsemaker.net> writes:
> 
> > No tests or documentation updates yet, and I'm not sure whether
> > --follow-symlinks in other modes than --no-index should be supported, 
> > ignored
> > (as it is now) or cause an error, but I'm leaning towards the third option.
> 
> 
> My knee-jerk reaction is:
> 
>  * The --no-index mode should default to your --follow-symlinks
>behaviour, without any option to turn it on or off.

ok.

>  * If normal "diff" that follows symlinks by default has an option
>to disable it, then it is OK to also add --no-follow-symlinks
>that is only valid in the --no-index mode, so that we can mimick
>it better (I do not think this is the case, though).

It does not, so no new option.

>  * Other modes should not follow symbolic links ever, no need for a
>new option.

Makes sense.

> In any case, I'd advise you not to reroll this too quickly and
> frequently until the end of this cycle.  During a feature freeze, I
> won't take new topics in 'pu' as that would add more things I need
> to worry about, and if you reroll in too quick succession, it will
> become harder to identify the latest set and queue after the
> release.

I'm in no hurry, so I'll sit on this until v2.11 is done.

D.

[PATCH 1/2] diff --no-index: add option to follow symlinks

[Dennis Kaarsemaker]

Git's diff machinery does not follow symlinks, which makes sense as git
itself also does not, but stores the symlink destination.

In --no-index mode however, it is useful for diff to be able to follow
symlinks, matching the behaviour of ordinary diff.

Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
---
 diff-no-index.c |  7 ---
 diff.c  | 10 --
 diff.h  |  2 +-
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index f420786..15811c2 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -40,7 +40,7 @@ static int read_directory_contents(const char *path, struct 
string_list *list)
  */
 static const char file_from_standard_input[] = "-";
 
-static int get_mode(const char *path, int *mode)
+static int get_mode(const char *path, int *mode, int follow_symlinks)
 {
struct stat st;
 
@@ -52,7 +52,7 @@ static int get_mode(const char *path, int *mode)
 #endif
else if (path == file_from_standard_input)
*mode = create_ce_mode(0666);
-   else if (lstat(path, ))
+   else if (follow_symlinks ? stat(path, ) : lstat(path, ))
return error("Could not access '%s'", path);
else
*mode = st.st_mode;
@@ -93,7 +93,8 @@ static int queue_diff(struct diff_options *o,
 {
int mode1 = 0, mode2 = 0;
 
-   if (get_mode(name1, ) || get_mode(name2, ))
+   if (get_mode(name1, , DIFF_OPT_TST(o, FOLLOW_SYMLINKS)) ||
+   get_mode(name2, , DIFF_OPT_TST(o, FOLLOW_SYMLINKS)))
return -1;
 
if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
diff --git a/diff.c b/diff.c
index be11e4e..1eaf604 100644
--- a/diff.c
+++ b/diff.c
@@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
s->size = xsize_t(st.st_size);
if (!s->size)
goto empty;
-   if (S_ISLNK(st.st_mode)) {
+   if (S_ISLNK(s->mode)) {
struct strbuf sb = STRBUF_INIT;
 
if (strbuf_readlink(, s->path, s->size))
@@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
s->should_free = 1;
return 0;
}
+   if (S_ISLNK(st.st_mode)) {
+   stat(s->path, );
+   s->size = xsize_t(st.st_size);
+   }
if (size_only)
return 0;
if ((flags & CHECK_BINARY) &&
@@ -3884,7 +3888,9 @@ int diff_opt_parse(struct diff_options *options,
else if (!strcmp(arg, "--no-follow")) {
DIFF_OPT_CLR(options, FOLLOW_RENAMES);
DIFF_OPT_CLR(options, DEFAULT_FOLLOW_RENAMES);
-   } else if (!strcmp(arg, "--color"))
+   } else if (!strcmp(arg, "--follow-symlinks"))
+   DIFF_OPT_SET(options, FOLLOW_SYMLINKS);
+   else if (!strcmp(arg, "--color"))
options->use_color = 1;
else if (skip_prefix(arg, "--color=", )) {
int value = git_config_colorbool(NULL, arg);
diff --git a/diff.h b/diff.h
index 25ae60d..22b0c5a 100644
--- a/diff.h
+++ b/diff.h
@@ -69,7 +69,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct 
diff_options *opt, void *data)
 #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)
 #define DIFF_OPT_FOLLOW_RENAMES  (1 <<  7)
 #define DIFF_OPT_RENAME_EMPTY(1 <<  8)
-/* (1 <<  9) unused */
+#define DIFF_OPT_FOLLOW_SYMLINKS (1 <<  9)
 #define DIFF_OPT_HAS_CHANGES (1 << 10)
 #define DIFF_OPT_QUICK   (1 << 11)
 #define DIFF_OPT_NO_INDEX(1 << 12)
-- 
2.10.1-449-gab0f84c

[RFC/PATCH 0/2] git diff <(command1) <(command2)

[Dennis Kaarsemaker]

Today on #git, a user asked why git diff <(command1) <(command2) gave only some
gibberish about pipes as output. The answer is fairly simple: git diff gets as
arguments /dev/fd/62 and /dev/fd/63, which are symlinks. So git simply
readlink()s them and gets pipe:[123456] as destination of that link which it
then outputs.

Given that 'normal' diff provides arguably better output in this case (a diff
of the output of the two commands), I wanted to look at what it would take for
git to handle this. Surprisingly: not much. 1/2 adds support for
--follow-symlinks to git diff --no-index (and only the --no-index variant) and
2/2 adds support for reading from pipes.

No tests or documentation updates yet, and I'm not sure whether
--follow-symlinks in other modes than --no-index should be supported, ignored
(as it is now) or cause an error, but I'm leaning towards the third option.

Dennis Kaarsemaker (2):
  diff --no-index: add option to follow symlinks
  diff --no-index: support reading from pipes

 diff-no-index.c | 15 ---
 diff.c  | 23 +++
 diff.h  |  2 +-
 3 files changed, 32 insertions(+), 8 deletions(-)

-- 
2.10.1-449-gab0f84c

[PATCH 2/2] diff --no-index: support reading from pipes

[Dennis Kaarsemaker]

diff <(command1) <(command2) provides useful output, let's make it
possible for git to do the same.

Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
---
 diff-no-index.c |  8 
 diff.c  | 13 +++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index 15811c2..487dbf5 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -83,6 +83,14 @@ static struct diff_filespec *noindex_filespec(const char 
*name, int mode)
name = "/dev/null";
s = alloc_filespec(name);
fill_filespec(s, null_sha1, 0, mode);
+   /*
+* In --no-index mode, we support reading from pipes. canon_mode, 
called by
+* fill_filespec, gets confused by this and thinks we now have 
subprojects.
+* Detect this and tell the rest of the diff machinery to treat pipes as
+* normal files.
+*/
+   if (S_ISGITLINK(s->mode))
+   s->mode = S_IFREG | ce_permissions(mode);
if (name == file_from_standard_input)
populate_from_stdin(s);
return s;
diff --git a/diff.c b/diff.c
index 1eaf604..c599efb 100644
--- a/diff.c
+++ b/diff.c
@@ -2839,9 +2839,18 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
fd = open(s->path, O_RDONLY);
if (fd < 0)
goto err_empty;
-   s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
+   if (!S_ISREG(st.st_mode)) {
+   struct strbuf sb = STRBUF_INIT;
+   strbuf_read(, fd, 0);
+   s->size = sb.len;
+   s->data = strbuf_detach(, NULL);
+   s->should_free = 1;
+   }
+   else {
+   s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, 
fd, 0);
+   s->should_munmap = 1;
+   }
close(fd);
-   s->should_munmap = 1;
 
/*
 * Convert from working tree format to canonical git format
-- 
2.10.1-449-gab0f84c

Re: [RFC] Add way to make Git credentials accessible from clean/smudge filter

[Dennis Kaarsemaker]

On Fri, 2016-11-11 at 10:40 +0100, Lars Schneider wrote:
> On 11 Nov 2016, at 10:31, Jeff King  wrote:
> 
> > On Fri, Nov 11, 2016 at 10:28:56AM +0100, Lars Schneider wrote:
> > 
> > > > Yeah, that is the solution I was going to suggest. The credentials are
> > > > totally orthogonal to the filters, and I would rather not shove them
> > > > into the protocol. It's an extra process, but with the new multi-use
> > > > smudge filter, it's one per git invocation, not one per file.
> > > 
> > > 
> > > The trouble with "git credential" is that it works only if the credential 
> > > helper is setup correctly. Although I assume that most people have setup 
> > > this, 
> > > I have also worked with a number of people who prefer to enter their 
> > > passwords 
> > > every time Git makes a network connection.
> > 
> > 
> > Are you sure about that? If I do:
> > 
> >  echo url=https://example.com/repo.git |
> >  git credential fill
> > 
> > I get prompted for a username and password.
> 
> 
> Hm.. either I don't understand you or I expressed myself unclear. 
> 
> Let's say a user runs:
> 
> $ git clone https://myrepo.git
> 
> If no credential helper is setup, then Git asks the user for credentials.
> Afterwards Git starts downloading stuff. At some point Git will run my
> smudge filter on some files and in my case the smudge filter needs the
> Git credentials. AFAIK, the smudge filter has no way to get the credentials 
> from Git at this point - not even by invoking "git credential". 
> Is this correct?

I think that's correct, but the same argument goes both ways: unless I
use a credential helper, or explicitely give a filter application my
credentials, I don't want a helper to be able to get to those
credentials. I'd consider that a security bug.

D.

Re: Forbid access to /gitweb but authorize the sub projets

[Dennis Kaarsemaker]

On Mon, 2016-11-07 at 14:07 +0100, Alexandre Duplaix wrote:
> 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 ?

My favourite way of doing this is abusing the fact that gitweb.conf is
perl code that's loaded with do $filename.

This makes it easy to override such things. Try this in gitweb.conf for example:

sub no_index {
die_error(403, "No access to the repository list");
}
$actions{project_list} = \_index;
$actions{project_index} = \_index;
$actions{opml} = \_index;

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

Re: [PATCH] push: do not use potentially ambiguous default refspec

[Dennis Kaarsemaker]

On Fri, Oct 28, 2016 at 12:25:30PM -0700, Junio C Hamano wrote:

>  * It is appreciated if somebody with spare cycles can add a test or
>two for this in t/t5523-push-upstream.sh or somewhere nearby.

5523 is for push --set-upstream-to, 5528 seemed more appropriate. Here's
something squashable that fails before your patch and succeeds after.

>8
Subject: [PATCH] push: test pushing ambiguously named branches

Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
---
 t/t5528-push-default.sh | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index 73f4bb6..ac103ce 100755
--- a/t/t5528-push-default.sh
+++ b/t/t5528-push-default.sh
@@ -98,6 +98,16 @@ test_expect_success 'push from/to new branch with upstream, 
matching and simple'
test_push_failure upstream
 '
 
+test_expect_success 'push ambiguously named branch with upstream, matching and 
simple' '
+   git checkout -b ambiguous &&
+   test_config branch.ambiguous.remote parent1 &&
+   test_config branch.ambiguous.merge refs/heads/ambiguous &&
+   git tag ambiguous &&
+   test_push_success simple ambiguous &&
+   test_push_success matching ambiguous &&
+   test_push_success upstream ambiguous
+'
+
 test_expect_success 'push from/to new branch with current creates remote 
branch' '
test_config branch.new-branch.remote repo1 &&
git checkout new-branch &&
-- 
2.10.1-449-gab0f84c

Re: password forgot

[Dennis Kaarsemaker]

That is asking for your local computer password. This uninstaller also
has nothing to do with git itself, but with the third-party git package
you installed. The git project does not ship an 'uninstall.sh' 

On Tue, 2016-10-25 at 13:23 -0300, Luciano Schillagi wrote:
> sorry, I'm a little confused
> 
> and this? 
> 
> 
> 
> 
> 2016-10-25 13:16 GMT-03:00 Dennis Kaarsemaker <den...@kaarsemaker.net>:
> > On Tue, 2016-10-25 at 12:52 -0300, Luciano Schillagi wrote:
> > > Hi,
> > >
> > > I forgot my password in git, such as resetting?
> > 
> > Hi Luciano,
> > 
> > Git itself doesn't do any authentication, so I assume you lost the
> > password for an account on a hosted git solution such as gitlab or
> > github.
> > 
> > You should contact the support team of whatever hoster you use, the git
> > developers cannot help you here.
> > 
> > D.
> > 
> 
> 
> 

Re: password forgot

[Dennis Kaarsemaker]

On Tue, 2016-10-25 at 12:52 -0300, Luciano Schillagi wrote:
> Hi,
> 
> I forgot my password in git, such as resetting?

Hi Luciano,

Git itself doesn't do any authentication, so I assume you lost the
password for an account on a hosted git solution such as gitlab or
github.

You should contact the support team of whatever hoster you use, the git
developers cannot help you here.

D.

Re: Drastic jump in the time required for the test suite

[Dennis Kaarsemaker]

On Thu, 2016-10-20 at 08:31 -0400, Jeff King wrote:

> I'm also not entirely convinced that the test suite being a shell script
> is the main culprit for its slowness. We run git a lot of times, and
> that's inherent in testing it. I ran the whole test suite under
> "strace -f -e execve". There are ~335K execs. Here's the breakdown of
> the top ones:

You're measuring execve's, but fork (well, fork emulation. There's no
actual fork) is also expensive on windows iirc, so subshells add a lot
to this cost. That said, strace -eclone says that a 'make test' forks
~408k times, and while this is significantly more than the amount of
execs in your example, this does include cvs and svn tests and it's
still in the same ballpark.

D.

Re: [PATCH] rev-list: use hdr_termination instead of a always using a newline

[Dennis Kaarsemaker]

On Thu, 2016-10-20 at 11:19 -0700, Jacob Keller wrote:
> Here's my solution, with an updated test using a helper function based
> on using sed (which I think is more portable than tail -n1 ?). The
> change actually is very simple. I ran the test suite and it appears to
> be not breaking anyone else since the normal case is
> hdr_termination="\n" except in the cases where it needs to be NUL.
> 
> Thanks for the bug report!

I like both improvements, and both 'make test' and gitweb are happy
with this version. Thanks for the quick fix.

D.

Re: [PATCH] rev-list: restore the NUL commit separator in --header mode

[Dennis Kaarsemaker]

On Wed, 2016-10-19 at 15:41 -0700, Junio C Hamano wrote:
> Dennis Kaarsemaker <den...@kaarsemaker.net> writes:
> 
> > +   touch expect &&
> > +   printf "\0" > expect &&
> 
> 
> What's the point of that "touch", especially if you are going to
> overwrite it immediately after?

Leftover debugging crud. I tried various ways of generating an
actual/expect to compare.

> > +   git rev-list --header --max-count=1 HEAD | tail -n1 >actual &&
> 
> 
> As "tail" is a tool for text files, it is likely unportable to use
> "tail -n1" to grab the "last incomplete line that happens to contain
> a single NUL".
> 
> > +   test_cmp_bin expect actual
> > +'

Yeah, I was fearing that. I didn't find anything in the testsuite that
helps answering the question "does this file end with a NUL" and would
appreciate a hint :)

D.

Re: [PATCH] rev-list: restore the NUL commit separator in --header mode

[Dennis Kaarsemaker]

On Wed, 2016-10-19 at 15:39 -0700, Junio C Hamano wrote:
> Jacob Keller <jacob.kel...@gmail.com> writes:
> 
> > Hi,
> > 
> > On Wed, Oct 19, 2016 at 2:04 PM, Dennis Kaarsemaker
> > <den...@kaarsemaker.net> wrote:
> > > Commit 660e113 (graph: add support for --line-prefix on all graph-aware
> > > output) changed the way commits were shown. Unfortunately this dropped
> > > the NUL between commits in --header mode. Restore the NUL and add a test
> > > for this feature.
> > > 
> > 
> > 
> > Oops! Thanks for the bug fix.
> > 
> > > Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
> > > ---
> > >  builtin/rev-list.c   | 4 
> > >  t/t6000-rev-list-misc.sh | 7 +++
> > >  2 files changed, 11 insertions(+)
> > > 
> > > diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> > > index 8479f6e..cfa6a7d 100644
> > > --- a/builtin/rev-list.c
> > > +++ b/builtin/rev-list.c
> > > @@ -157,6 +157,10 @@ static void show_commit(struct commit *commit, void 
> > > *data)
> > > if (revs->commit_format == CMIT_FMT_ONELINE)
> > > putchar('\n');
> > > }
> > > +   if (revs->commit_format == CMIT_FMT_RAW) {
> > > +   putchar(info->hdr_termination);
> > > +   }
> > > +
> > 
> > 
> > This seems right to me. My one concern is that we make sure we restore
> > it for every case (in case it needs to be there for other formats?)
> > I'm not entirely sure about whether other non-raw modes need this or
> > not?
> 
> 
> Right.  The original didn't do anything special for CMIT_FMT_RAW,
> and 660e113 did not remove anything special for CMIT_FMT_RAW, so it
> isn't immediately obvious why this patch is sufficient.  
> 
> Dennis, care to elaborate?

The original logic was (best seen with git show -w 660e113):

if(showing graphs) {
 do pretty things
}
else {
 just print the buffer and the header terminator
}

660e113 changed that to

do pretty things

Given that the 'do pretty things part' works for other uses of git rev-
list, it made sense that the \0 should only be added back in
CMIT_FMT_RAW mode. Changing the first putchar('\n') as Jacob proposes
(that mail arrived while I'm typing this) might work too, I haven't
tested it.

D.

[PATCH] rev-list: restore the NUL commit separator in --header mode

[Dennis Kaarsemaker]

Commit 660e113 (graph: add support for --line-prefix on all graph-aware
output) changed the way commits were shown. Unfortunately this dropped
the NUL between commits in --header mode. Restore the NUL and add a test
for this feature.

Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
---
 builtin/rev-list.c   | 4 
 t/t6000-rev-list-misc.sh | 7 +++
 2 files changed, 11 insertions(+)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 8479f6e..cfa6a7d 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -157,6 +157,10 @@ static void show_commit(struct commit *commit, void *data)
if (revs->commit_format == CMIT_FMT_ONELINE)
putchar('\n');
}
+   if (revs->commit_format == CMIT_FMT_RAW) {
+   putchar(info->hdr_termination);
+   }
+
strbuf_release();
} else {
if (graph_show_remainder(revs->graph))
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 3e752ce..a2acff3 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -100,4 +100,11 @@ test_expect_success '--bisect and --first-parent can not 
be combined' '
test_must_fail git rev-list --bisect --first-parent HEAD
 '
 
+test_expect_success '--header shows a NUL after each commit' '
+   touch expect &&
+   printf "\0" > expect &&
+   git rev-list --header --max-count=1 HEAD | tail -n1 >actual &&
+   test_cmp_bin expect actual
+'
+
 test_done
-- 
2.10.1-449-gab0f84c


-- 
Dennis Kaarsemaker <den...@kaarsemaker.net>
http://twitter.com/seveas

Re: [PATCH v12 3/8] graph: add support for --line-prefix on all graph-aware output

[Dennis Kaarsemaker]

On Wed, 2016-08-31 at 16:27 -0700, Jacob Keller wrote:
> From: Jacob Keller 
> 
> Add an extension to git-diff and git-log (and any other graph-aware
> displayable output) such that "--line-prefix=" will print the
> additional line-prefix on every line of output.

This patch breaks git rev-list --header, also breaking gitweb.

The NUL between commits has gone missing, causing gitweb to interpret
the output of git rev-list as one commit.

Sorry for not catching this earlier, I actually encountered this early
september but thought it was caused by us running an ancient gitweb
with a modern git. Finally managed to upgrade gitweb today, and the bug
didn't go away. git bisect says 660e113ce is the culprit. Checking out
'next' and reverting this single patch makes the problem disappear.

Haven't yet tried to fix the bug, but this hunk looks suspicious:

-   if (revs->commit_format != CMIT_FMT_USERFORMAT ||
-   buf.len) {
-   fwrite(buf.buf, 1, buf.len, stdout);
-   putchar(info->hdr_termination);
-   }
+   /*
+* If the message buffer is empty, just show
+* the rest of the graph output for this
+* commit.
+*/
+   if (graph_show_remainder(revs->graph))
+   putchar('\n');
+   if (revs->commit_format == CMIT_FMT_ONELINE)
+  

D.

[PATCH] worktree: allow the main brach of a bare repository to be checked out

[Dennis Kaarsemaker]

In bare repositories, get_worktrees() still returns the main repository,
so git worktree list can show it. ignore it in find_shared_symref so we
can still check out the main branch.

Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
---
 t/t2025-worktree-add.sh | 8 
 worktree.c  | 2 ++
 2 files changed, 10 insertions(+)

On Sun, 2016-10-09 at 17:52 +0700, Duy Nguyen wrote:
> On Sun, Oct 9, 2016 at 2:51 PM, Dennis Kaarsemaker > <den...@kaarsemaker.net> 
> wrote:
> > On Sat, 2016-10-08 at 19:30 -0500, Michael Tutty wrote:
> > > 
> > > The only exception seems to be merging to master. When I do git
> > > worktree add /tmp/path/to/worktree master I get an error:
> > > 
> > > [fatal: 'master' is already checked out at '/path/to/bare/repo']
> > > 
> > 
> > The worktree code treats the base repo as a worktree, even if it's
> > bare. For the purpose of being able to do a checkout of the main branch
> > of a bare repo, this patch should do:
> > 
> --snip--
> 
> You're fast :) I'm still studying  8d9fdd7 (worktree.c: check whether
> branch is rebased in another worktree - 2016-04-22). But yeah that
> should fix it.

OK, so here it is as a proper patch.

D.

diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 4bcc335..2996c38 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -138,6 +138,14 @@ test_expect_success 'checkout from a bare repo without 
"add"' '
)
 '
 
++test_expect_success '"add" default branch of a bare repo' '
+   (
+   git clone --bare . bare2 &&
+   cd bare2 &&
+   git worktree add ../there3 master
+   )
+'
+
 test_expect_success 'checkout with grafts' '
test_when_finished rm .git/info/grafts &&
test_commit abc &&
diff --git a/worktree.c b/worktree.c
index 5acfe4c..35e95b7 100644
--- a/worktree.c
+++ b/worktree.c
@@ -345,6 +345,8 @@ const struct worktree *find_shared_symref(const char 
*symref,
 
for (i = 0; worktrees[i]; i++) {
struct worktree *wt = worktrees[i];
+   if(wt->is_bare)
+   continue;
 
    if (wt->is_detached && !strcmp(symref, "HEAD")) {
if (is_worktree_being_rebased(wt, target)) {
-- 
2.10.1-356-g947a599


-- 
Dennis Kaarsemaker <den...@kaarsemaker.net>
http://twitter.com/seveas

Re: [PATCH] contrib: add credential helper for libsecret

[Dennis Kaarsemaker]

On Tue, 2016-10-11 at 13:13 -0700, Junio C Hamano wrote:
> Dennis Kaarsemaker <den...@kaarsemaker.net> writes:
> 
> > On Tue, 2016-10-11 at 22:48 +0300, Mantas Mikulėnas wrote:
> > > On 2016-10-11 22:36, Junio C Hamano wrote:
> > > > Thanks for a review.  I'll wait until one of (1) a squashable patch
> > > > to address the "we do not want unconditional overwrite" issue, (2) a
> > > > reroll from Mantas to do the same, or (3) a counter-argument from
> > > > somebody to explain why unconditional overwrite is a good idea here
> > > > (but not in the original) appears.
> > > 
> > > 
> > > 
> > > I overlooked that. I can write a patch, but it shouldn't make any
> > > difference in practice – if c->username *was* set, then it would also
> > > get added to the search attribute list, therefore the search couldn't
> > > possibly return any results with a different username anyway.
> > 
> > 
> > Makes sense, so a (3) it is.
> 
> 
> So... does it mean the gnome-keyring one needs a bugfix?

I'd say both behaviours are correct.

When you do a search without a username, both helpers fill in the
username returned by the actual credential storage. When you do a
search with a username, the gnome-keyring helper won't overwrite the
value passed in and the libsecret helper overwrites it with the same
value, as the search can only return matches that have the same value.

D.

Re: [PATCH] contrib: add credential helper for libsecret

[Dennis Kaarsemaker]

On Tue, 2016-10-11 at 22:48 +0300, Mantas Mikulėnas wrote:
> On 2016-10-11 22:36, Junio C Hamano wrote:
> > Thanks for a review.  I'll wait until one of (1) a squashable patch
> > to address the "we do not want unconditional overwrite" issue, (2) a
> > reroll from Mantas to do the same, or (3) a counter-argument from
> > somebody to explain why unconditional overwrite is a good idea here
> > (but not in the original) appears.
> 
> 
> I overlooked that. I can write a patch, but it shouldn't make any
> difference in practice – if c->username *was* set, then it would also
> get added to the search attribute list, therefore the search couldn't
> possibly return any results with a different username anyway.

Makes sense, so a (3) it is.

D.

Re: [PATCH] contrib: add credential helper for libsecret

[Dennis Kaarsemaker]

On Mon, 2016-10-10 at 16:46 -0400, Jeff King wrote:
> On Mon, Oct 10, 2016 at 10:20:50PM +0200, Dennis Kaarsemaker wrote:
> 
> > On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:
> > > This is based on the existing gnome-keyring helper, but instead of
> > > libgnome-keyring (which was specific to GNOME and is deprecated), it
> > > uses libsecret which can support other implementations of XDG Secret
> > > Service API.
> > > 
> > > Passes t0303-credential-external.sh.
> > 
> > When setting credential.helper to this helper, I get the following output:
> > 
> > $ git clone https://private-repo-url-removed private
> > Cloning into 'private'...
> > /home/dennis/code/git/contrib/credential/libsecret/ get: 1: 
> > /home/dennis/code/git/contrib/credential/libsecret/ get: 
> > /home/dennis/code/git/contrib/credential/libsecret/: Permission denied
> > 
> > Looks suboptimal. Am I holding it wrong?
> 
> That looks like a directory name in your error message. How did you set
> up credential.helper?

Doh.

I had done git config --global credential.helper 
~/code/git/contrib/credential/libsecret/

After doing it properly (actual path to the binary), it works just
fine. The error message above is slightly suboptimal, but there's no
solution for pebkac.

D.

Re: [PATCH] contrib: add credential helper for libsecret

[Dennis Kaarsemaker]

On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:

> + s = g_hash_table_lookup(attributes, "user");
> + if (s) {
> + g_free(c->username);
> + c->username = g_strdup(s);
> + }

This always overwrites c->username, the original gnome-keyring version
only does that when the username isn't set. Other than that it looks
good to me.

Reviewed-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
Tested-by: Dennis Kaarsemaker <den...@kaarsemaker.net>

D.

Re: [PATCH] contrib: add credential helper for libsecret

[Dennis Kaarsemaker]

On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:
> This is based on the existing gnome-keyring helper, but instead of
> libgnome-keyring (which was specific to GNOME and is deprecated), it
> uses libsecret which can support other implementations of XDG Secret
> Service API.
> 
> Passes t0303-credential-external.sh.

When setting credential.helper to this helper, I get the following output:

$ git clone https://private-repo-url-removed private
Cloning into 'private'...
/home/dennis/code/git/contrib/credential/libsecret/ get: 1: 
/home/dennis/code/git/contrib/credential/libsecret/ get: 
/home/dennis/code/git/contrib/credential/libsecret/: Permission denied

Looks suboptimal. Am I holding it wrong?

D.

Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL

[Dennis Kaarsemaker]

[And now with CC to the list, sorry Stefan]

On Mon, 2016-10-10 at 10:56 -0700, Stefan Beller wrote:
> Before 63e95beb0 (2016-04-15, submodule: port resolve_relative_url from
> shell to C), it did not matter if the superprojects URL had a trailing
> slash or not. It was just chopped off as one of the first steps
> (The "remoteurl=${remoteurl%/}" near the beginning of
> resolve_relative_url(), which was removed in said commit).
> 
> When porting this to the C version, an off-by-one error was introduced
> and we did not check the actual last character to be a slash, but the
> NULL delimiter.
> 
> Reintroduce the behavior from before 63e95beb0, to ignore the trailing
> slash.

Looks good to me, and fixes my simple testcase and cloning epiphany
with trailing slash. Thanks!

D.

Re: Problem with submodules

[Dennis Kaarsemaker]

On Sun, 2016-10-09 at 16:41 +0200, ven...@gmail.com wrote:
> Hi, I want to report a regression.
> 
> After cloning for example https://git.gnome.org/browse/epiphany with
> git 2.10 and running ./autogen.sh I get the following errors:
> http://pastebin.com/93AunRhu
> 
> The developer told me that it is probably not an issue caused by
> epiphany and I should try an older git version. I installed 2.7.2 and
> it works perfectly. So I think theres a bug in git 2.10.

I can't reproduce the problem with git 2.10.1

Can you share the .git/config file in your clone of epiphany please.

D.

Re: Bug? git worktree fails with master on bare repo

[Dennis Kaarsemaker]

On Sat, 2016-10-08 at 19:30 -0500, Michael Tutty wrote:
> Hey all,
> I'm working on some server-side software to do a merge. By using git
> worktree it's possible to check out a given branch for a bare repo and
> merge another branch into it. It's very fast, even with large
> repositories.
> 
> The only exception seems to be merging to master. When I do git
> worktree add /tmp/path/to/worktree master I get an error:
> 
> [fatal: 'master' is already checked out at '/path/to/bare/repo']
> 
> But this is clearly not true, git worktree list gives:
> 
> [/path/to/bare/repo (bare)]
> 
> ...and of course, there is no work tree at that path, just the bare
> repo files you'd expect.

The worktree code treats the base repo as a worktree, even if it's
bare. For the purpose of being able to do a checkout of the main branch
of a bare repo, this patch should do:

diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 4bcc335..b618d6b 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -138,6 +138,14 @@ test_expect_success 'checkout from a bare repo without 
"add"' '
)
 '
 
+test_expect_success '"add" default branch of a bare repo' '
+   (
+   git clone --bare . bare2 &&
+   cd bare2 &&
+   git worktree add ../there3 master
+   )
+'
+
 test_expect_success 'checkout with grafts' '
test_when_finished rm .git/info/grafts &&
test_commit abc &&
diff --git a/worktree.c b/worktree.c
index 5acfe4c..35e95b7 100644
--- a/worktree.c
+++ b/worktree.c
@@ -345,6 +345,8 @@ const struct worktree *find_shared_symref(const char 
*symref,
 
for (i = 0; worktrees[i]; i++) {
struct worktree *wt = worktrees[i];
+   if(wt->is_bare)
+   continue;
 
if (wt->is_detached && !strcmp(symref, "HEAD")) {
if (is_worktree_being_rebased(wt, target)) {


But I'm wondering why the worktree code does this. A bare repo isn't a
worktree and I think it shouldn't treat it as one. A patch that rips
out this feature and updates the tests to match would look like this:


diff --git a/builtin/worktree.c b/builtin/worktree.c
index 5c4854d..3600530 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -382,15 +382,11 @@ static int add(int ac, const char **av, const char 
*prefix)
 static void show_worktree_porcelain(struct worktree *wt)
 {
    printf("worktree %s\n", wt->path);
-   if (wt->is_bare)
-   printf("bare\n");
-   else {
-   printf("HEAD %s\n", sha1_to_hex(wt->head_sha1));
-   if (wt->is_detached)
-   printf("detached\n");
-   else
-   printf("branch %s\n", wt->head_ref);
-   }
+   printf("HEAD %s\n", sha1_to_hex(wt->head_sha1));
+   if (wt->is_detached)
+   printf("detached\n");
+   else
+   printf("branch %s\n", wt->head_ref);
    printf("\n");
 }
 
@@ -401,16 +397,12 @@ static void show_worktree(struct worktree *wt, int 
path_maxlen, int abbrev_len)
    int path_adj = cur_path_len - utf8_strwidth(wt->path);
 
    strbuf_addf(, "%-*s ", 1 + path_maxlen + path_adj, wt->path);
-   if (wt->is_bare)
-   strbuf_addstr(, "(bare)");
-   else {
-   strbuf_addf(, "%-*s ", abbrev_len,
-   find_unique_abbrev(wt->head_sha1, 
DEFAULT_ABBREV));
-   if (!wt->is_detached)
-   strbuf_addf(, "[%s]", 
shorten_unambiguous_ref(wt->head_ref, 0));
-   else
-   strbuf_addstr(, "(detached HEAD)");
-   }
+   strbuf_addf(, "%-*s ", abbrev_len,
+   find_unique_abbrev(wt->head_sha1, DEFAULT_ABBREV));
+   if (!wt->is_detached)
+   strbuf_addf(, "[%s]", shorten_unambiguous_ref(wt->head_ref, 
0));
+   else
+   strbuf_addstr(, "(detached HEAD)");
    printf("%s\n", sb.buf);
 
    strbuf_release();
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 4bcc335..b618d6b 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -138,6 +138,14 @@ test_expect_success 'checkout from a bare repo without 
"add"' '
    )
 '
 
+test_expect_success '"add" default branch of a bare repo' '
+   (
+   git clone --bare . bare2 &&
+   cd bare2 &&
+   git worktree add ../there3 master
+   )
+'
+
 test_expect_success 'checkout with grafts' '
    test_when_finished rm .git/info/grafts &&
    test_commit abc &&
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 1b1b65a..842e9d9 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -62,9 +62,8 @@ test_expect_success 'bare repo setup' '
 
 test_expect_success '"list" all worktrees from bare main' '
    test_when_finished "rm -rf there && git -C bare1 worktree prune" &&
-   git -C bare1 worktree add 

Re: Repeatable Extraction

[Dennis Kaarsemaker]

On di, 2016-09-27 at 20:14 -0700, chris king wrote:
> Hello, first off thanks for such a wonderful tool! I have a general
> question and I hope this is an appropriate spot to ask it.
> 
> Is there a way automate extraction that will repeatably generate the
> same files? Currently, each time I extract git portable many of the
> binaries change slightly. For example, if I extract twice using
> 
> PortableGit-2.10.0-32-bit.7z.exe -y -gm2
> 
> then Beyond Compare tells me that many of the files in usr\bin have
> changed at offset 0x88 and 0x89. Why is that?

Hi Chris,

That file is specific to the git for windows project, not git itself.
While you may be able to find somebody on the git@vger mailinglist who
knows the answer, git for windows also has a separate mailinglist at
https://groups.google.com/forum/#!forum/git-for-windows and a chat room
at https://gitter.im/git-for-windows/git -- these may be of more
assistance in this case.

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

Re: Bug

[Dennis Kaarsemaker]

On Tue, 2016-09-13 at 13:18 -0400, Mike Hawes wrote:
> To whom this may concern,
>
> I found a bug in git while trying to push my website.
> I redid the process and it happened again.
> I also tried it on another computer and it happened again.
> I was wondering how to claim a bug?

Hi Mike,

When you think git does not behave as you expect, please do not stop
your bug report with just "git does not work".  "I used git in this
way, but it did not work" is not much better, neither is "I used git
in this way, and X happend, which is broken".  It often is that git is
correct to cause X happen in such a case, and it is your expectation
that is broken. People would not know what other result Y you expected
to see instead of X, if you left it unsaid.

Please remember to always state

 - what you wanted to achieve;

 - what you did (the version of git and the command sequence to reproduce
   the behavior);

 - what you saw happen (X above);

 - what you expected to see (Y above); and

 - how the last two are different.

See http://www.chiark.greenend.org.uk/~sgtatham/bugs.html for further
hints.

(The above was shamelessly copied from the "A note from the maintainer" mails)

D.

Re: Git Ignore Exception bug

[Dennis Kaarsemaker]

On vr, 2016-09-09 at 15:39 -0600, Nathan Williams wrote:
> it ignore doesn't seem to be working properly when adding exceptions.

>8 -- snip testcase

> Expected results
> % git st
> On branch master
> Untracked files:
>   (use "git add ..." to include in what will be committed)
> 
> foo/bar/

That expectation is wrong, it should show foo/. And indeed it does
(tested with 2.9.0 and 2.10.0-rc1)

$ sh -x testscript
+ rm -rf repo
+ mkdir repo
+ cd repo
+ git init
Initialized empty Git repository in /home/dennis/code/git/repo/.git/
+ echo foo/*
+ echo !foo/bar
+ git add .gitignore
+ git commit -m Ignore file with exceptions
[master (root-commit) 7e1b82a] Ignore file with exceptions
 1 file changed, 2 insertions(+)
 create mode 100644 .gitignore
+ mkdir foo
+ mkdir foo/bar
+ touch foo/1
+ touch foo/2
+ touch foo/bar/a
+ touch foo/bar/b
+ git status
On branch master
Untracked files:
  (use "git add ..." to include in what will be committed)

foo/

nothing added to commit but untracked files present (use "git add" to track)
-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

Re: [PATCH 0/16] fix config-reading in non-repos

[Dennis Kaarsemaker]

On ma, 2016-09-12 at 23:22 -0400, Jeff King wrote:

> The motivation for this series is to fix the regression in v2.9 where
> core.logallrefupdates is sometimes not set properly in a newly
> initialized repository, as described in this thread:
> 
>   http://public-inbox.org/git/c46d36ef-3c2e-374f-0f2e-ffe31104e...@gmx.de/T/#u
> 
> The root of the problem is that we are overly eager to read and use
> config from ".git/config", even when we have not established that it is
> part of a repository. This is especially bad for git-init, which would
> not want to read anything until we've created the new repo.
> 
> So the two interesting parts of the fix are:
> 
>   1. We stop blindly reading ".git/config" when we don't know there's an
>  actual git directory. This is in patch 14, and is actually enough
>  to fix the v2.9 regression.
> 
>   2. We are more thorough about dropping any cached config values when
>  we move into the new repository in git-init (patch 16).
> 
>  I didn't dig into when this was broken, but it was probably when we
>  switched git_config() to use cached values in the v2.2.0
>  time-frame.
> 
> Doing (1) required fixing up some builtins that depended on the blind
> .git/config thing, as the tests demonstrated. But I think this is a sign
> that we are moving in the right direction, because each one of those
> programs could easily be demonstrated to be broken in scenarios only
> slightly more exotic than the test scripts (e.g., see patch 3 for one of
> the simplest cases).
> 
> So I think notwithstanding their use as prep for patch 14, patches 1-13
> fix useful bugs.
> 
> I won't be surprised if there are other fallouts that were not caught by
> the test suite (i.e., programs that expect to read config, don't do
> RUN_SETUP, but aren't covered well by tests). I poked around the list of
> builtins in git.c that do not use RUN_SETUP, and they seem to correctly
> end up in setup_git_directory_gently() before reading config. But it's
> possible I missed a case.
> 
> So this is definitely a bit larger than I'd hope for a regression-fix to
> maint. But anything that doesn't address this issue at the config layer
> is going to end up as a bit of a hack, and I'd rather not pile up hacks
> if we can avoid it.

Agreed with all of the above, this is much better than just fixing the
symptom on the mailinglist thread that started this.

> I've cc'd Dennis, who helped investigate solutions in the thread
> mentioned above, and Duy, because historically he has been the one most
> willing and able to battle the dragon of our setup code. :)
> 
>   [01/16]: t1007: factor out repeated setup
>   [02/16]: hash-object: always try to set up the git repository
>   [03/16]: patch-id: use RUN_SETUP_GENTLY
>   [04/16]: diff: skip implicit no-index check when given --no-index
>   [05/16]: diff: handle --no-index prefixes consistently
>   [06/16]: diff: always try to set up the repository
>   [07/16]: pager: remove obsolete comment
>   [08/16]: pager: stop loading git_default_config()
>   [09/16]: pager: make pager_program a file-local static
>   [10/16]: pager: use callbacks instead of configset
>   [11/16]: pager: handle early config
>   [12/16]: t1302: use "git -C"
>   [13/16]: test-config: setup git directory
>   [14/16]: config: only read .git/config from configured repos
>   [15/16]: init: expand comments explaining config trickery
>   [16/16]: init: reset cached config when entering new repo

Couldn't find anything to comment on, and I've tested that this does
indeed fix the symptoms we saw.

Reviewed-by: Dennis Kaarsemaker <den...@kaarsemaker.net>

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

Re: [PATCH v2] rebase -i: improve advice on bad instruction lines

[Dennis Kaarsemaker]

Hi Ralf,

There are quite a few patch series in flight these days around
interactive rebase. Have you checked for conflicts with those?

On di, 2016-09-06 at 20:59 +0200, Ralf Thielow wrote:
> If we found bad instruction lines in the instruction sheet
> of interactive rebase, we give the user advice on how to
> fix it.  However, we don't tell the user what to do afterwards.
> Give the user advice to run 'git rebase --continue' after
> the fix.
> 
> Signed-off-by: Ralf Thielow 
> ---
> Changes in v2:
> - adjust tests
> 
>  git-rebase--interactive.sh| 2 +-
>  t/t3404-rebase-interactive.sh | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index b1ba21c..029594e 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -1041,7 +1041,7 @@ The possible behaviours are: ignore, warn,
> error.")"
>   # placed before the commit of the next action
>   checkout_onto
>  
> - warn "$(gettext "You can fix this with 'git rebase
> --edit-todo'.")"
> + warn "$(gettext "You can fix this with 'git rebase
> --edit-todo' and then run 'git rebase --continue'.")"
>   die "$(gettext "Or you can abort the rebase with
> 'git rebase --abort'.")"
>   fi
>  }
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-
> interactive.sh
> index 597e94e..e38e296 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1195,7 +1195,7 @@ To avoid this message, use "drop" to explicitly
> remove a commit.
>  Use 'git config rebase.missingCommitsCheck' to change the level of
> warnings.
>  The possible behaviours are: ignore, warn, error.
>  
> -You can fix this with 'git rebase --edit-todo'.
> +You can fix this with 'git rebase --edit-todo' and then run 'git
> rebase --continue'.
>  Or you can abort the rebase with 'git rebase --abort'.
>  EOF
>  
> @@ -1219,7 +1219,7 @@ cat >expect <  Warning: the command isn't recognized in the following line:
>   - badcmd $(git rev-list --oneline -1 master~1)
>  
> -You can fix this with 'git rebase --edit-todo'.
> +You can fix this with 'git rebase --edit-todo' and then run 'git
> rebase --continue'.
>  Or you can abort the rebase with 'git rebase --abort'.
>  EOF
>  
> @@ -1254,7 +1254,7 @@ cat >expect <  Warning: the SHA-1 is missing or isn't a commit in the following
> line:
>   - edit XXX False commit
>  
> -You can fix this with 'git rebase --edit-todo'.
> +You can fix this with 'git rebase --edit-todo' and then run 'git
> rebase --continue'.
>  Or you can abort the rebase with 'git rebase --abort'.
>  EOF
>  

Re: [PATCH 6/9] rebase -i: check for missing commits in the rebase--helper

[Dennis Kaarsemaker]

On vr, 2016-09-02 at 18:23 +0200, Johannes Schindelin wrote:
> In particular on Windows, where shell scripts are even more expensive
> than on MacOSX or Linux, it makes sense to move a loop that forks
> Git at least once for every line in the todo list into a builtin.

Heh, this was the one thing that made me hesitate sending the
suggestion about rebase-helper --edit-todo, but with this bit already
moved, I think rebase-helper --edit-todo makes even more sense to do.

D.

Re: [PATCH 4/9] rebase -i: also expand/collapse the SHA-1s via the rebase--helper

[Dennis Kaarsemaker]

On vr, 2016-09-02 at 18:23 +0200, Johannes Schindelin wrote:
> This is crucial to improve performance on Windows, as the speed is now
> mostly dominated by the SHA-1 transformation (because it spawns a new
> rev-parse process for *every* line, and spawning processes is pretty
> slow from Git for Windows' MSYS2 Bash).

I see these functions only used as part of an shorten-edit-expand
sequence. Why not do a git rebase-helper --edit-todo instead? Saves
another few process spawnings.

Something for yet another later followup patch?

D.

Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands

[Dennis Kaarsemaker]

On vr, 2016-09-02 at 16:22 +0200, Johannes Schindelin wrote:
> I hope this clarifies why I am not so concerned about some issues
> such as translation, or commit messages, or grammar, and more so
> about others, such as incorrect code.

It does, thanks!

D.

Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands

[Dennis Kaarsemaker]

On vr, 2016-09-02 at 09:13 +0200, Johannes Schindelin wrote:

> As Git for Windows does not ship with translations (for multiple
> reasons), it would not be a regression.

I'm confused, how does "git for windows does not ship with
translations" translate to "this is not a regression"? Is this patch
series only meant to be for git for windows and not go into git.git
itself?

D.

Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository

[Dennis Kaarsemaker]

On vr, 2016-09-02 at 04:04 -0400, Jeff King wrote:
> On Wed, Aug 31, 2016 at 05:32:33PM +0200, Dennis Kaarsemaker wrote:
> 
> > 
> > > 
> > > We may need to do something like turn off the
> > > need_shared_repository_from_config in init-db, since I think it would
> > > not want to ever read from the default config sources in most of its
> > > code-paths (OTOH, it should in theory respect core.sharedRepository
> > > in ~/.gitconfig, so maybe there is another more elegant way of
> > > handling this).
> > I would go even further and say that git init should completely ignore
> > the config of a repository you happen to be in when creating a new
> > repository.
> Hmm. I'd think we would already be avoiding that, because we shouldn't
> be calling setup_git_directory(). But some of the lazy-loaded setup is a
> bit overzealous, and we blindly look at ".git/config". If we try the
> same operation from a subdir of an existing repo, we _don't_ end up
> confused. Eek.

Yikes. Didnt' dig that deep, but that sounds wrong :)

> So I actually wonder if that is the root of the bug. In your patch, you
> disable config reading when we chdir to the new repo:
> 
> > 
> > diff --git a/builtin/init-db.c b/builtin/init-db.c
> > index 3a45f0b..d0fd3dc 100644
> > --- a/builtin/init-db.c
> > +++ b/builtin/init-db.c
> > @@ -493,6 +493,12 @@ int cmd_init_db(int argc, const char **argv, const 
> > char *prefix)
> >     int mkdir_tried = 0;
> >     retry:
> >     if (chdir(argv[0]) < 0) {
> > +   /*
> > +    * We're creating a new repository. If we're already in 
> > another
> > +    * repository, ignore its config
> > +    */
> > +   ignore_repo_config = 1;
> > +   git_config_clear();
> But I think we should go further and avoid ever looking at the original
> repository in the first place. I.e., I would say that "git init" should
> never ever behave differently if run in an existing repo versus outside
> of one.

Well, 'git init' is a valid operation to run inside an existing repo to
reinitialize some bits, so we definitely need to not ignore the config
once we're sure we're not creating a new repo.

> So really we ought to be setting ignore_repo_config from the very start
> of cmd_init(), and then re-enabling it once we are "inside" the new
> repo.  The git_config_clear() should in theory come once we are
> "inside", as well; we may have cached system/global config, and
> need to flush so we read them anew along with the new local config.

That's why I git_config_clear() twice.

> OTOH, since there shouldn't be anything interesting in the new
> repo-level config, I'm not sure that's really necessary. The rest of
> "init" can probably proceed without caring.

Except when running 'git init' to re-init existing repo.

> I also wonder if there are other things besides config which might
> accidentally read from .git (because they call git_pathdup(), and it
> just blindly looks in ".git" if nobody called setup_git_directory()). So
> it would be nice to have some flag for "do not ever lazy-call
> setup_git_env; we do not care about any repository".  But I think that's
> ahrd; functions like git_pathdup() are always expected to return _some_
> value, so what would they say? The best we could do is return
> "/does-not-exist/" or something, but that is awfully hacky.
> 
> > 
> > @@ -500,7 +506,6 @@ int cmd_init_db(int argc, const char **argv, const char 
> > *prefix)
> >      * and we know shared_repository should always 
> > be 0;
> >      * but just in case we play safe.
> >      */
> > -   saved = get_shared_repository();
> >     set_shared_repository(0);
> >     switch 
> > (safe_create_leading_directories_const(argv[0])) {
> >     case SCLD_OK:
> I don't know if anybody cares about being able to set core.sharedRepository
> from ~/.gitconfig. It didn't work until v2.9.0 anyway (when I moved it
> out of the repository-format check), but it seems like you _should_ be
> able to set it and have it Just Work.
> 
> And in that case, this "we know shared_repository should always be 0" is
> not true, and we would want to keep doing the save/set-to-0/restore
> dance here.

We don't need to save if we throw away the cache below.

> > @@ -524,6 +528,11 @@ int cmd_init_db(int argc, const char **argv, c

Re: [PATCH] make dist: allow using an installed version of git

[Dennis Kaarsemaker]

On do, 2016-09-01 at 10:43 -0700, Junio C Hamano wrote:
> Dennis Kaarsemaker <den...@kaarsemaker.net> writes:
> 
> > b1de9de2 back in 2005 ensured that we could create a tarball with 'make
> > dist' even if git wasn't installed yet. These days however, chances are
> > higher that a git version is available. Add a config.mak knob to allow
> > people to choose to use the installed version of git to create the
> > tarball and avoid the overhead of building git-archive.
> 
> Thanks, but not interested.

Pity. Would save me quite a bit of tarball build time.

> We do not know what vintage of "git" happens to be installed on the
> platform, but we know how "git archive" we ship with the source
> ought to behave.

That's why I didn't want to make it the default, but merely make it an
available option for saving some build time for people who know their
git is up-to-date enough.

D.

Re: `make profile-install` fails in 2.9.3

[Dennis Kaarsemaker]

On do, 2016-09-01 at 18:08 +0200, Jan Keromnes wrote:

> However, this fails (and has failed in previous versions), because it
> runs the whole test-suite to get the profile, but bails out if there
> were test failures (which happens often).

Working around failing tests is fixing a symptom, not the root cause. I
run the testsuite for master, next and pu very regularly and test
failures on master are extremely rare. I think I've seen one or so in
the last year, might even be 0. So let's focus on that instead.

> Related problem: `t3700-add.sh` currently fails in 2.9.3. I can
> provide more debug information if you don't already know this problem.

It should not fail, and for me does not fail on ubuntu 16.04. Please
run that test in verbose mode and share its output, as well as your
build configuration.

D.

Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands

[Dennis Kaarsemaker]

On do, 2016-09-01 at 17:17 +0200, Johannes Schindelin wrote:
> Hi Dennis,
> 
> On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:
> 
> > 
> > On wo, 2016-08-31 at 10:54 +0200, Johannes Schindelin wrote:
> > 
> > > 
> > > +static int is_fixup(enum todo_command command)
> > > +{
> > > + return command == TODO_FIXUP || command == TODO_SQUASH;
> > > +}
> > It sounds wrong to have a function named is_fixup return true when
> > the
> > command isn't a fixup but a squash. Maybe name it
> > changes_previous_commit or something?
> I can see how that may sound confusing, unless you understand that a
> squash is a fixup that lets the user edit the commit message, too. So
> essentially squash = fixup + edit, if you will.
> 
> Maybe the name is more appropriate in that light?

Kinda makes sense. It's not how I use fixup/squash as a user of rebase
-i though. But we can't go there, that's bikeshed country :)

> > > +static const char *nth_for_number(int n)
> > > +{
> > > + int n1 = n % 10, n10 = n % 100;
> > > +
> > > + if (n1 == 1 && n10 != 11)
> > > + return "st";
> > > + if (n1 == 2 && n10 != 12)
> > > + return "nd";
> > > + if (n1 == 3 && n10 != 13)
> > > + return "rd";
> > > + return "th";
> > > +}
> > > 
> > > 8---
> > > 
> > > + if (command == TODO_SQUASH) {
> > > + unlink(rebase_path_fixup_msg());
> > > + strbuf_addf(, "\n%c This is the %d%s commit
> > > message:\n\n%s",
> > > + comment_line_char,
> > > + count, nth_for_number(count), body);
> > > + }
> > > + else if (command == TODO_FIXUP) {
> > > + strbuf_addf(,
> > > + "\n%c The %d%s commit message will be
> > > skipped:\n\n",
> > > + comment_line_char, count,
> > > nth_for_number(count));
> > > + strbuf_add_commented_lines(, body,
> > > strlen(body));
> > > + }
> > This way of handling numbers is not translatable, and I really
> > think we
> > should mark these strings for translation, like they are in the .sh
> > version.
> Ah, this is the risk of working on something as big as rebase
> --helper.
> Back when I started with it, the relevant code in git-rebase
> --interactive
> read like this:
> 
>   nth_string () {
>   case "$1" in
>   *1[0-9]|*[04-9]) echo "$1"th;;
>   *1) echo "$1"st;;
>   *2) echo "$1"nd;;
>   *3) echo "$1"rd;;
>   esac
>   }
> 
> I merely did a faithful translation of that...
> 
> Now, I see that git-rebase--interactive was switched to use
> eval_gettext,
> which in turn is handled in git-sh-i18n whose code is quite
> convoluted. In
> the absence of gettext, it uses git-sh-i18n--envsubst, which has no C
> API
> whatsoever.
> 
> And I see that the beautiful ordinal computation was given up in
> favor of
> a lousy "#1", "#2", "#3", etc (it used to be "1st", "2nd", "3rd"
> etc).
> 
> In any case, translation is not my main concern until v2.10.0, so
> I'll
> take care of this after that release.

Hmm, not sure if I agree with that. I'd see it as a regression to lose
the i18n there.

D.

Re: [PATCH 27/34] sequencer (rebase -i): differentiate between comments and 'noop'

[Dennis Kaarsemaker]

On do, 2016-09-01 at 17:32 +0200, Johannes Schindelin wrote:
> Hi Dennis,
> 
> On Thu, 1 Sep 2016, Dennis Kaarsemaker wrote:
> 
> > 
> > On wo, 2016-08-31 at 10:56 +0200, Johannes Schindelin wrote:
> > > 
> > > diff --git a/sequencer.c b/sequencer.c
> > > index 51c2f76..4c902e5 100644
> > > --- a/sequencer.c
> > > +++ b/sequencer.c
> > > @@ -763,7 +763,8 @@ enum todo_command {
> > > TODO_SQUASH,
> > > TODO_EXEC,
> > > TODO_NOOP,
> > > -   TODO_DROP
> > > +   TODO_DROP,
> > > +   TODO_COMMENT
> > >  };
> > (picking a random commit that touches this enum)
> > 
> > In a few places you now make comparisons like "< TODO_NOOP", so I
> > think
> > it would be good to have a comment near the definition of this enum
> > that says that ordering matters and why, so people don't attempt to
> > add
> > a new TODO_FOOBAR at the end.
> True.
> 
> It does not seem that we have a precedent for that. The closest is
> what I
> had in an early iteration of the fsck message IDs, and subsequently
> things
> were refactored so that it is not the order, but a flag, that
> determines
> what the command does.
> 
> Not sure how to do this elegantly. Maybe like this?
> 
>   enum todo_command {
>   TODO_PICK_COMMANDS = 0,
>   TODO_PICK = TODO_PICK_COMMANDS,
>   TODO_SQUASH,
> 
>   TODO_NON_PICK_COMMANDS,
>   TODO_EXEC = TODO_NON_PICK_COMMANDS,
> 
>   TODO_NOOP_COMMANDS,
>   TODO_NOOP = TODO_NOOP_COMMANDS,
>   TODO_DROP
>   TODO_DROP,
> 
>   TODO_LAST_COMMAND,
>   TODO_COMMENT = TODO_LAST_COMMAND
>   };
> 
> But that is so god-awful to read.

Agreed, that sure is awful.

How about something like

/*
 * Note that ordering matters in this enum. Not only must it match the
 * mapping below, it is also divided into several sections that matter.
 * When adding new commands, make sure you add it in the right section.
 */
enum todo_command {
/* All commands that handle commits */
TODO_PICK,
...
/* All commands that do something else than pick */
TODO_EXEC,
...
/* All commands that do nothing but are counted for reporting progress 
*/
TODO_NOOP,
...
/* Comments, which are not counted
TODO_COMMENT
}

Re: [PATCH 27/34] sequencer (rebase -i): differentiate between comments and 'noop'

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 10:56 +0200, Johannes Schindelin wrote:
> diff --git a/sequencer.c b/sequencer.c
> index 51c2f76..4c902e5 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -763,7 +763,8 @@ enum todo_command {
> TODO_SQUASH,
> TODO_EXEC,
> TODO_NOOP,
> -   TODO_DROP
> +   TODO_DROP,
> +   TODO_COMMENT
>  };

(picking a random commit that touches this enum)

In a few places you now make comparisons like "< TODO_NOOP", so I think
it would be good to have a comment near the definition of this enum
that says that ordering matters and why, so people don't attempt to add
a new TODO_FOOBAR at the end.

D. 

Re: [PATCH 32/34] sequencer (rebase -i): show the progress

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 10:56 +0200, Johannes Schindelin wrote:
> The interactive rebase keeps the user informed about its progress.
> If the sequencer wants to do the grunt work of the interactive
> rebase, it also needs to show that progress.
> 
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 33 +
>  1 file changed, 33 insertions(+)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 89fd625..e8c6daf 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1218,6 +1218,7 @@ struct todo_list {
>   struct strbuf buf;
>   struct todo_item *items;
>   int nr, alloc, current;
> + int done_nr, total_nr;
>  };
>  
>  #define TODO_LIST_INIT { STRBUF_INIT }
> @@ -1329,6 +1330,17 @@ static int parse_insn_buffer(char *buf, struct 
> todo_list *todo_list)
>   return res;
>  }
>  
> +static int count_commands(struct todo_list *todo_list)
> +{
> + int count = 0, i;
> +
> + for (i = 0; i < todo_list->nr; i++)
> + if (todo_list->items[i].command != TODO_COMMENT)
> + count++;
> +
> + return count;
> +}
> +
>  static int read_populate_todo(struct todo_list *todo_list,
>   struct replay_opts *opts)
>  {
> @@ -1355,6 +1367,22 @@ static int read_populate_todo(struct todo_list 
> *todo_list,
>   if (!todo_list->nr &&
>   (!is_rebase_i(opts) || !file_exists(rebase_path_done(
>   return error(_("No commits parsed."));
> +
> + if (is_rebase_i(opts)) {
> + struct todo_list done = TODO_LIST_INIT;
> +
> + if (strbuf_read_file(, rebase_path_done(), 0) > 0 &&
> + !parse_insn_buffer(done.buf.buf, ))
> + todo_list->done_nr = count_commands();
> + else
> + todo_list->done_nr = 0;
> +
> + todo_list->total_nr = todo_list->done_nr
> + + count_commands(todo_list);
> +
> + todo_list_release();
> + }
> +
>   return 0;
>  }
>  
> @@ -1900,6 +1928,11 @@ static int pick_commits(struct todo_list *todo_list, 
> struct replay_opts *opts)
>   if (save_todo(todo_list, opts))
>   return -1;
>   if (is_rebase_i(opts)) {
> + if (item->command != TODO_COMMENT)
> + fprintf(stderr, "Rebasing (%d/%d)%s",
> + ++(todo_list->done_nr),
> + todo_list->total_nr,
> + opts->verbose ? "\n" : "\r");
>   unlink(rebase_path_message());
>   unlink(rebase_path_author_script());
>   unlink(rebase_path_stopped_sha());

(picking a random commit that shows this 'symptom')

You're sprinking a lot of is_rebase_i's around sequencer.c to make sure
there are no changes in behaviour. I wonder if the right balance has
been struck between 'no changes in behaviour' and 'common behaviour'.
For instance, in this case, maybe it would be a better idea for non-
rebase uses of the sequencer to also show progress.

D.

Re: [PATCH 07/34] sequencer (rebase -i): add support for the 'fixup' and 'squash' commands

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 10:54 +0200, Johannes Schindelin wrote:

> +static int is_fixup(enum todo_command command)
> +{
> + return command == TODO_FIXUP || command == TODO_SQUASH;
> +}

It sounds wrong to have a function named is_fixup return true when the
command isn't a fixup but a squash. Maybe name it
changes_previous_commit or something?

> +static const char *nth_for_number(int n)
> +{
> + int n1 = n % 10, n10 = n % 100;
> +
> + if (n1 == 1 && n10 != 11)
> + return "st";
> + if (n1 == 2 && n10 != 12)
> + return "nd";
> + if (n1 == 3 && n10 != 13)
> + return "rd";
> + return "th";
> +}

>8---

> + if (command == TODO_SQUASH) {
> + unlink(rebase_path_fixup_msg());
> + strbuf_addf(, "\n%c This is the %d%s commit message:\n\n%s",
> + comment_line_char,
> + count, nth_for_number(count), body);
> + }
> + else if (command == TODO_FIXUP) {
> + strbuf_addf(,
> + "\n%c The %d%s commit message will be skipped:\n\n",
> + comment_line_char, count, nth_for_number(count));
> + strbuf_add_commented_lines(, body, strlen(body));
> + }

This way of handling numbers is not translatable, and I really think we
should mark these strings for translation, like they are in the .sh
version.

D.

Re: [PATCH 20/34] sequencer (rebase -i): copy commit notes at end

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 10:55 +0200, Johannes Schindelin wrote:
> +   if (!stat(rebase_path_rewritten_list(), ) &&
> +   st.st_size > 0) {
> +   struct child_process child = CHILD_PROCESS_INIT;
> +
> +   child.in = open(rebase_path_rewritten_list(), 
> O_RDONLY);
> +   child.git_cmd = 1;
> +   argv_array_push(, "notes");
> +   argv_array_push(, "copy");
> +   argv_array_push(, "--for-rewrite=rebase");
> +   /* we don't care if this copying failed */
> +   run_command();
> +   }

I know this is a strict port of git-rebase--interactive.sh, but
shouldn't we at least warn the user that the copy failed?

D.

Re: [PATCH 06/34] sequencer (rebase -i): write the 'done' file

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 10:54 +0200, Johannes Schindelin wrote:
> In the interactive rebase, commands that were successfully processed are
> not simply discarded, but appended to the 'done' file instead. This is
> used e.g. to display the current state to the user in the output of
> `git status` or the progress.

Wouldn't it make more sense to have this patch before the ones that
implement the actual rebase commands?

Hmm, and after reading more of this series, I think the same applies to
some other patches too, e.g. 08/34 and 14/34, so I'm probably missing
something. So before I make a fool of myself and suggest that the
implementation of the actual commands should come at the end, maybe you
could tell me what I'm missing :) 

D.

Re: [PATCH 15/34] sequencer (rebase -i): leave a patch upon error

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 10:55 +0200, Johannes Schindelin wrote:
> Just like the interactive rebase, we want to leave a 'patch' file for
> further inspection by the user (even if we never tried to actually apply
> that patch, since we're cherry-picking instead).
> 
> Signed-off-by: Johannes Schindelin 

This commit message confuses me. Did you mean s/Just like the/When
doing an/? 

D.

Re: [PATCH 00/34] Teach the sequencer to act as rebase -i's backend

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 10:53 +0200, Johannes Schindelin wrote:
> This marks the count down to '3': two more patch series after this
> (really tiny ones) and we have a faster rebase -i.

I got to 16/34 (and skipped 07/34), will continue tomorrow. I hope the
comments are useful.

D.

Re: [PATCH 05/34] sequencer (rebase -i): learn about the 'verbose' mode

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 10:54 +0200, Johannes Schindelin wrote:

> +   if (file_exists(rebase_path_verbose()))
> +   opts->verbose = 1;

I don't see anything in this series that creates this file, will that
be part of a later series?

D.

Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 06:48 -0400, Jeff King wrote:
> On Wed, Aug 31, 2016 at 11:12:26AM +0200, Dennis Kaarsemaker wrote:
> 
> > 
> > That is indeed a bug. git reads the config of t1 and then thinks a
> > template config has set that value, so it won't override it.
> This is a regression in v2.9.0 due to my ae5f677 (lazily load
> core.sharedrepository, 2016-03-11).
> 
> > 
> > This is caused by git init reading the config via
> > get_shared_repository. The comment above it indicates that this may
> > not
> > be needed, and indeed not doing it makes this bug go away.
> Hrm. I'm not sure if that will work, though, because we may call
> get_shared_repository() from other code-paths (e.g., anything that
> calls
> adjust_shared_perm(), like safe_create_leading_directories).

Agreed, the diff was more to point out what triggered this (so I could
send that and run away to spend the day with jr.) than an attempt at a
patch.

> We may need to do something like turn off the
> need_shared_repository_from_config in init-db, since I think it would
> not want to ever read from the default config sources in most of its
> code-paths (OTOH, it should in theory respect core.sharedRepository
> in ~/.gitconfig, so maybe there is another more elegant way of
> handling this).

I would go even further and say that git init should completely ignore
the config of a repository you happen to be in when creating a new
repository.

> I'm out of time for the day, so it will be a while before I can dig
> further. Please feel free to figure it out while I am sleeping. :)
> 
> -Peff

I hope you slept well :)

This is what I came up with to implement my suggestion above, comments
welcome.

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 3a45f0b..d0fd3dc 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -493,6 +493,12 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
    int mkdir_tried = 0;
    retry:
    if (chdir(argv[0]) < 0) {
+   /*
+    * We're creating a new repository. If we're already in 
another
+    * repository, ignore its config
+    */
+   ignore_repo_config = 1;
+   git_config_clear();
    if (!mkdir_tried) {
    int saved;
    /*
@@ -500,7 +506,6 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
     * and we know shared_repository should always 
be 0;
     * but just in case we play safe.
     */
-   saved = get_shared_repository();
    set_shared_repository(0);
    switch 
(safe_create_leading_directories_const(argv[0])) {
    case SCLD_OK:
@@ -513,7 +518,6 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
    die_errno(_("cannot mkdir %s"), 
argv[0]);
    break;
    }
-   set_shared_repository(saved);
    if (mkdir(argv[0], 0777) < 0)
    die_errno(_("cannot mkdir %s"), 
argv[0]);
    mkdir_tried = 1;
@@ -524,6 +528,11 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
    } else if (0 < argc) {
    usage(init_db_usage[0]);
    }
+
+   need_shared_repository_from_config = 1;
+   ignore_repo_config = 0;
+   git_config_clear();
+
    if (is_bare_repository_cfg == 1) {
    char *cwd = xgetcwd();
    setenv(GIT_DIR_ENVIRONMENT, cwd, argc > 0);
diff --git a/cache.h b/cache.h
index f30a441..1be16fc 100644
--- a/cache.h
+++ b/cache.h
@@ -640,6 +640,8 @@ extern int hold_locked_index(struct lock_file *, int);
 extern void set_alternate_index_output(const char *);
 
 /* Environment bits from configuration mechanism */
+extern int ignore_repo_config;
+extern int need_shared_repository_from_config;
 extern int trust_executable_bit;
 extern int trust_ctime;
 extern int check_stat;
diff --git a/config.c b/config.c
index 0dfed68..2df0189 100644
--- a/config.c
+++ b/config.c
@@ -1304,7 +1304,7 @@ static int do_git_config_sequence(config_fn_t fn, void 
*data)
    ret += git_config_from_file(fn, user_config, data);
 
    current_parsing_scope = CONFIG_SCOPE_REPO;
-   if (repo_config && !access_or_die(repo_config, R_OK, 0))
+   if (repo_config && !ignore_repo_config && !access_or_die(repo_config, 
R_OK, 0))
    ret += git_config_from_file(fn, repo_config, data);
 
 

Re: bug: 'core.logallrefupdates' is not set by default in non-bare repository

[Dennis Kaarsemaker]

That is indeed a bug. git reads the config of t1 and then thinks a
template config has set that value, so it won't override it.

This is caused by git init reading the config via
get_shared_repository. The comment above it indicates that this may not
be needed, and indeed not doing it makes this bug go away.

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 3a45f0b..b2883a6 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -494,14 +494,6 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
retry:
if (chdir(argv[0]) < 0) {
if (!mkdir_tried) {
-   int saved;
-   /*
-* At this point we haven't read any 
configuration,
-* and we know shared_repository should always 
be 0;
-* but just in case we play safe.
-*/
-   saved = get_shared_repository();
-   set_shared_repository(0);
switch 
(safe_create_leading_directories_const(argv[0])) {
case SCLD_OK:
case SCLD_PERMS:
@@ -513,7 +505,6 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
die_errno(_("cannot mkdir %s"), 
argv[0]);
break;
}
-   set_shared_repository(saved);
if (mkdir(argv[0], 0777) < 0)
die_errno(_("cannot mkdir %s"), 
argv[0]);
                                mkdir_tried = 1; 

On wo, 2016-08-31 at 10:09 +0200, doak wrote:
> Hi there,
> 
> If 'git init /path/to/repo' is called inside another repository with
> 'core.logallrefupdates' set to true, it will be not set by default in
> the created repository.
> This seems to be a bug.
> I am using Git v2.9.3 (Arch).
> 
> Steps to reproduce:
> -
> git init t1
> cd t1
> # 'core.logallrefupdates' will not be set for 't2'.
> git init ../t2
> -
> 
> Stated from 'git-config(1)':
> -
> core.logAllRefUpdates
> [...]
> This value is true by default in a repository that has a
> working directory associated with it, and false by default in a bare
> repository.
> -
> 
> 
> With best regards,
> doak
> 
> 

Re: git blame [was: Reducing CPU load on git server]

[Dennis Kaarsemaker]

On wo, 2016-08-31 at 01:42 -0400, Jeff King wrote:
> On Tue, Aug 30, 2016 at 12:46:20PM +0200, Jakub Narębski wrote:
> 
> > I wonder if having support for 'git blame ' in Git core would
> > be something interesting to Git users.  I once tried to implement it,
> > but it went nowhere.  Would it be hard to implement?
> I think there's some interest; I have received a few off-list emails
> over the years about it. There was some preliminary discussion long ago:
> 
>8 
>
> Here's a snippet from an off-list conversation I had with Dennis (cc'd)
> in 2014 (I think he uses that blame-tree code as part of a custom git
> web interface):

I still do, but haven't worked on that webinterface in a while. I also
still build git with blame-tree merged in (which does occasionally
require a rebase with some tinkering).

> That's all I could dig out of my archives. I'd be happy if somebody> wanted 
> to pick it up and run with it. Polishing for upstream has been on
> my list for several years now, but there's usually something more
> important (or interesting) to work on at any given moment.

Same here. I've been meaning to pick this up, but it touches some parts
of git I'm still not too familiar with and there are always other
things to do :)

D.

Re: Reducing CPU load on git server

[Dennis Kaarsemaker]

On ma, 2016-08-29 at 13:57 -0700, W. David Jarvis wrote:

> >  * If you do need branches consider archiving stale tags/branches
> > after some time. I implemented this where I work, we just have a
> > $REPO-archive.git with every tag/branch ever created for a given
> > $REPO.git, and delete refs after a certain time.
> 
> This is something else that we're actively considering. Why did your
> company implement this -- was it to reduce load, or just to clean up
> your repositories? Did you notice any change in server load?

At 50k refs, ref negotiation gets expensive, especially over http

D. 

Re: [PATCH 00/22] Prepare the sequencer for the upcoming rebase -i patches

[Dennis Kaarsemaker]

On ma, 2016-08-29 at 10:03 +0200, Johannes Schindelin wrote:

> Therefore I would be most grateful for every in-depth review.

Tried to do that, but could come up only with a few nits. I think the
approach is sensible.

D.

Re: [PATCH 20/22] sequencer: remember do_recursive_merge()'s return value

[Dennis Kaarsemaker]

On ma, 2016-08-29 at 10:06 +0200, Johannes Schindelin wrote:

> The return value of do_recursive_merge() may be positive (indicating merge
> conflicts), se let's OR later error conditions so as not to overwrite them
> with 0.

s/se/so/?

D.

Re: [PATCH 15/22] sequencer: introduce a helper to read files written by scripts

[Dennis Kaarsemaker]

On ma, 2016-08-29 at 10:06 +0200, Johannes Schindelin wrote:
> +   if (strbuf_read_file(buf, path, 0) < 0) {
> +   warning_errno("could not read '%s'", path);
> +   return 0;
> +   }
> +
> +   if (buf->len > orig_len && buf->buf[buf->len - 1] == '\n') {
> +   if (--buf->len > orig_len && buf->buf[buf->len - 1]
> == '\r')
> +   --buf->len;
> +   buf->buf[buf->len] = '\0';
> +   }

Why not use open + strbuf_getline instead of hand-rolling a newline
eradicator?

D.

Re: [PATCH 12/22] sequencer: refactor the code to obtain a short commit name

[Dennis Kaarsemaker]

On ma, 2016-08-29 at 10:05 +0200, Johannes Schindelin wrote:



I fail to see the point of this patch, would you mind enlightening me?

D.

Re: [PATCH 04/22] sequencer: future-proof remove_sequencer_state()

[Dennis Kaarsemaker]

On ma, 2016-08-29 at 10:04 +0200, Johannes Schindelin wrote:

> +   if (read_and_refresh_cache(opts))
> +   return -1;
> +

This doesn't seem to be related to the get_dir changes?

D.

Re: [PATCH 01/22] sequencer: use static initializers for replay_opts

[Dennis Kaarsemaker]

On ma, 2016-08-29 at 10:03 +0200, Johannes Schindelin wrote:

> +#define REPLAY_OPTS_INIT { -1, -1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, NULL, NULL, 
> NULL, 0, 0, NULL }

This looked off to me, as it replaces memset(..., 0, ...) so is not
100% equivalent. But the changed functions both set opts.action and
call parse_args which sets opts.subcommand.

D.

Re: [Ksummit-discuss] checkkpatch (in)sanity ?

[Dennis Kaarsemaker]

On zo, 2016-08-28 at 12:52 -0700, Joe Perches wrote:
> On Sun, 2016-08-28 at 11:59 +0200, Julia Lawall wrote:
> > 
> > On Sun, 28 Aug 2016, Alexey Dobriyan wrote:
> []
> > 
> > > 
> > > The problem is that c-h.pl generates noise in the commit history
> > > and
> > > makes git-blame less useful than it can be.
> > Could it be that this is a problem with git blame, rather than with
> > checkpatch?  Last year there was a discussion on this list about
> > how there
> > is an option to git blame that will cause it to step through the
> > history,
> > and not show only the most recent patch that has modified a given
> > line.
> It is more or less an ease-of-use limitation of git blame.
> 
> There are some that want an ncurses only version of git blame
> that could
> use arrow-key style navigation for historical commit
> line-ranges.
> 
> git gui blame kind of works, but it's not ncurses/text based.
> git-cola kind of works too, but it's not text based either.
> 
> Are there other existing tools for blame history viewing?

tig has a neat way of doing blame history digging: do a `tig blame
filename`, select a line and hit the comma key to re-blame from the
parent of the commit that was blamed for that line.

(hat tip to Jeff King who pointed out this trick recently)

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

[PATCH] make dist: allow using an installed version of git

[Dennis Kaarsemaker]

b1de9de2 back in 2005 ensured that we could create a tarball with 'make
dist' even if git wasn't installed yet. These days however, chances are
higher that a git version is available. Add a config.mak knob to allow
people to choose to use the installed version of git to create the
tarball and avoid the overhead of building git-archive.

Signed-off-by: Dennis Kaarsemaker <den...@kaarsemaker.net>
---
 Makefile | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index d96ecb7..3dabb75 100644
--- a/Makefile
+++ b/Makefile
@@ -378,6 +378,9 @@ all::
 #
 # to say "export LESS=FRX (and LV=-c) if the environment variable
 # LESS (and LV) is not set, respectively".
+#
+# Define USE_INSTALLED_GIT_ARCHIVE if you don't want to build git-archive as
+# part of 'make dist', but are happy to rely on a git version on you $PATH
 
 GIT-VERSION-FILE: FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -2423,8 +2426,15 @@ quick-install-html:
 ### Maintainer's dist rules
 
 GIT_TARNAME = git-$(GIT_VERSION)
-dist: git-archive$(X) configure
-   ./git-archive --format=tar \
+ifndef USE_INSTALLED_GIT_ARCHIVE
+   GIT_ARCHIVE = ./git-archive$(X)
+   GIT_ARCHIVE_DEP = git-archive$(X)
+else
+   GIT_ARCHIVE = git archive
+   GIT_ARCHIVE_DEP =
+endif
+dist: $(GIT_ARCHIVE_DEP) configure
+   $(GIT_ARCHIVE) --format=tar \
--prefix=$(GIT_TARNAME)/ HEAD^{tree} > $(GIT_TARNAME).tar
@mkdir -p $(GIT_TARNAME)
@cp configure $(GIT_TARNAME)
-- 
2.10.0-rc1-230-g8efea0f
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Https password present in git output

[Dennis Kaarsemaker]

On wo, 2016-07-13 at 20:26 +0300, ervion wrote:
> One possibility for this in git is to save remote in the 
> https://username:passw...@domain.com/repo.git format.

This is not recommended. Git has credential helpers to help you store
passwords outside the git configuration.

Which then makes your original problem go away :)

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

Re: Dependencies required for offline installation

[Dennis Kaarsemaker]

On di, 2016-07-05 at 14:06 -0400, Kevin Paxton wrote:
> Thank you for the response.
> 
> I apologize. RHEL 6.5, not 5.5.

That's less ancient, but still not recommended. When using RHEL, try to
stay with the latest point release so you get security updates.

> Would the same version be applicable to 6.5 as well as the
> dependencies that you mentioned?

Red hat actually ships a version of git with RHEL 6.  So the 
version will be different (I believe it's a 1.7.something). The
dependencies should be similar, if not the same.

> On Tue, Jul 5, 2016 at 1:53 PM, Dennis Kaarsemaker
> <den...@kaarsemaker.net> wrote:
> > 
> > On di, 2016-07-05 at 07:45 -0400, Kevin Paxton wrote:
> > > 
> > > Hi,
> > > 
> > > I’m looking to install git on a separate network that is running
> > > Redhat 5.5.
> > That's ancient and unsupported. If you insist on using rhel 5, at
> > least
> > do 5.11 so you get the security updates.
> > 
> > > 
> > > I need to know what is the list of packages that I need to
> > > download to be able to install git-all? I plan on using git-svn
> > > to
> > > migrate an existing svn repo over to git as well. Svn version we
> > > have
> > > installed is 1.9.3.
> > There are rpms for git 1.8 in EPEL. git-all is probably overkill,
> > but
> > you'll need at least git, perl-Git, perl-Git-SVN and perl-Error.
> > 
> > > 
> > > Does the tarball contain all dependencies already? Should I go
> > > that
> > > route? Or should I try and find all the rpm's required?
> > The source tarball of git contains no dependencies. Also be aware
> > that
> > building git from source requires even more dependencies.
> > 
> > D.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] Refactor recv_sideband()

[Dennis Kaarsemaker]

On vr, 2016-06-24 at 14:14 -0400, Jeff King wrote:

> On Fri, Jun 24, 2016 at 07:45:04PM +0200, Johannes Schindelin wrote:
>> Do we *actually* send color via the sideband, like, ever?

> We don't, but remember that we forward arbitrary output from hooks.
> If the consensus is "nah, it is probably crazy and nobody is doing it
> today" then I am fine with that.

It's crazy, so obviously we're doing it :)

Though losing this would not be a big deal for us.

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

Re: [BUG REPORT] git 2.9.0 clone --recursive fails on cloning a submodule

[Dennis Kaarsemaker]

On zo, 2016-06-19 at 18:09 -0700, Stefan Beller wrote:

> How often do we see a depth != 1 in practice?

Travis clones with --depth=50

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

Re: [bug] assertion in 2.8.4 triggering on old-ish worktree

[Dennis Kaarsemaker]

On do, 2016-06-16 at 17:02 +1200, Chris Packham wrote:
> On Thu, Jun 16, 2016 at 4:59 PM, Chris Packham  om> wrote:
> > 
> > Hi All,
> > 
> > I have the git-sh-prompt configured in my .bashrc today I visited
> > an
> > old worktree that I haven't really touched in a few years (sorry
> > can't
> > remember the git version I was using back then). I received the
> > following output when changing to the directory
> > 
> > git: pathspec.c:317: prefix_pathspec: Assertion `item-
> > >nowildcard_len
> > <= item->len && item->prefix <= item->len' failed.
> > 
> > I assume it's one of the git invocations in git-sh-prompt that's
> > hitting the assertion. Any thoughts on what might be triggering it?
> > Any debug I can gather?
> A bit more info. The directory in question is a uninitialised
> submodule. It doesn't trigger in the root of the parent project.

That very much smells like a class of bugs we've seen before, with git
getting confused around submodules. See also for example

https://www.mail-archive.com/git@vger.kernel.org/msg68447.html

I don't think an accepted fix exists yet.

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