Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'

[Junio C Hamano]

Jeff King  writes:

> FWIW, the code looks OK here. It is a shame to duplicate the policy
> found in git_config_parse_key(), though.
>
> I wonder if we could make a master version of that which canonicalizes
> in-place, and then just wrap it for the git_config_parse_key()
> interface. Actually, I guess the function you just wrote _is_ that inner
> function, as long as it learned about the "quiet" flag.

Hmm, I obviously missed an opportunity.  I thought about doing a
similar thing with the policy in parse-source, but that side didn't
seem worth doing, as the config-parse-source callgraph is quite a
mess (as it has to parse the .ini like format with line breaks and
comments, not the simple "[.]." thing, it
cannot quite avoid it), and it needs to take advantage of _some_
policy to parse the pieces.

We could loosen the policy implemented by config-parse-key interface
(e.g. change config-parse-source to let anything that begins with a
non-whitespace continue to be processed with get_value(), instead of
only allowing string that begins with isalpha(); similarly loosen
get_value() to allow any non-dot non-space string, not just
iskeychar() bytes) and first turn what is read into the simple
"[.]." format, and then reuse the new
"master" logic to validate.  That would allow us to update the
"master" logic to make it tighter or looser to some degree, but the
source parser still needs to hardcode _some_ policy (e.g. the first
level and the last level names begin with a non-space) that allows
it to guess what "" pieces the contents being parsed from
the .ini looking format meant to express.

But you are right.  config-parse-key does have the simpler string
that can just be given to the canonicalize thing and we should be
able to reuse it.

Re: [RFC][Git GUI] Make Commit message field in git GUI re sizable.

[Jessie Hernandez]

> On 2017-02-22 06:59 AM, Jessie Hernandez wrote:
>>> HI,
>>>
>>> the reason why it is fixed, is because commit messages should be
>>> wrapped at 76 characters to be used in mails. So it helps you with the
>>> wrapping.
>>>
>>> Bert
>>
>> Right ok. I understand.
>> Knowing this I think I might start writing my commit messages
>> differently
>> then.
>
> You can configure gui.commitMsgWidth if you don't like the default
> (which is 75, not 76).
>
>   M.

Hi Marc,

Yeah I did that in my local version and got my desired effect.
But knowing it was designed like this and and not a bug,
I am happy to use the designed behavior and adjust my way of working.

-
Jessie Hernandez

>
>
>> Thank you for this.
>>
>> Regards
>>
>> -
>> Jessie Hernandez
>>
>>>
>>>
>>> On Wed, Feb 22, 2017 at 10:27 AM, Jessie Hernandez
>>>  wrote:
 Hi all,

 I have been using git for a few years now and really like the
 software.
 I have a small annoyance and was wondering if I could get the
 communities
 view on this.

 When using git GUI I find it handy to be able to re-size the "Unstaged
 Changes" and the "Staged Changed" fields.

 I would like the same thing for the "Commit Message" field, or to have
 it
 re-size with the git GUI window.

 I can re-size the "Commit Message" vertically when making the
 "Modified"
 panel smaller.

 Does this make sense?
 I would be happy to get into more detail if that is necessary or if
 something is not clear.

 Thank you.

 -
 Jessie Hernandez


>>>
>>
>>
>

Re: [PATCH v2] config: reject invalid VAR in 'git -c VAR=VAL command'

[Jeff King]

On Tue, Feb 21, 2017 at 01:24:38PM -0800, Junio C Hamano wrote:

> The parsing of one-shot assignments of configuration variables that
> come from the command line historically was quite loose and allowed
> anything to pass.
> 
> The configuration variable names that come from files are validated
> in git_config_parse_source(), which uses get_base_var() that grabs
> the  (and subsection) while making sure that 
> consists of iskeychar() letters, the function itself that makes sure
> that the first letter in  is isalpha(), and get_value()
> that grabs the remainder of the  name while making sure
> that it consists of iskeychar() letters.
> 
> Perform an equivalent check in canonicalize_config_variable_name()
> to catch invalid configuration variable names that come from the
> command line.

FWIW, the code looks OK here. It is a shame to duplicate the policy
found in git_config_parse_key(), though.

I wonder if we could make a master version of that which canonicalizes
in-place, and then just wrap it for the git_config_parse_key()
interface. Actually, I guess the function you just wrote _is_ that inner
function, as long as it learned about the "quiet" flag.

-Peff

Re: git email From: parsing (was Re: [GIT PULL] Staging/IIO driver patches for 4.11-rc1)

[Jeff King]

On Thu, Feb 23, 2017 at 07:04:44AM +0100, Greg KH wrote:

> > Poor Simon Sandström.
> > 
> > Funnily enough, this only exists for one commit. You've got several
> > other commits from Simon that get his name right.
> > 
> > What happened?
> 
> I don't know what happened, I used git for this, I don't use quilt for
> "normal" patches accepted into my trees anymore, only for stable kernel
> work.
> 
> So either the mail is malformed, or git couldn't figure it out, I've
> attached the original message below, and cc:ed the git mailing list.
> 
> Also, Simon emailed me after this was committed saying something went
> wrong, but I couldn't go back and rebase my tree.  Simon, did you ever
> figure out if something was odd on your end?
> 
> Git developers, any ideas?

The problem isn't on the applying end, but rather on the generating end.
The From header in the attached mbox is:

  From: =?us-ascii?B?PT9VVEYtOD9xP1NpbW9uPTIwU2FuZHN0cj1DMz1CNm0/PQ==?= 


If you de-base64 that, you get:

  =?UTF-8?q?Simon=20Sandstr=C3=B6m?=

So something double-encoded it before it got to your mbox.

-Peff

git email From: parsing (was Re: [GIT PULL] Staging/IIO driver patches for 4.11-rc1)

[Greg KH]

On Wed, Feb 22, 2017 at 11:59:01AM -0800, Linus Torvalds wrote:
> On Wed, Feb 22, 2017 at 6:56 AM, Greg KH  wrote:
> >
> > =?UTF-8?q?Simon=20Sandstr=C3=B6m?= (1):
> >   staging: vt6656: Add missing identifier names
> 
> Wow, your scripts really screwed up that name.
> 
> I'm assuming this is quilt not doing proper character set handling..
> 
> Because if it's git, we need to get that fixed (but I'm pretty sure
> git gets this right - there are various tests for email header
> quoting).
> 
> Alternatively, somebody hand-edited some email and moved the From:
> header to the body without fixing up the RFC 1342 mail header quoting
> (which is very different from how quoting works in the *body* of an
> email).
> 
> Poor Simon Sandström.
> 
> Funnily enough, this only exists for one commit. You've got several
> other commits from Simon that get his name right.
> 
> What happened?

I don't know what happened, I used git for this, I don't use quilt for
"normal" patches accepted into my trees anymore, only for stable kernel
work.

So either the mail is malformed, or git couldn't figure it out, I've
attached the original message below, and cc:ed the git mailing list.

Also, Simon emailed me after this was committed saying something went
wrong, but I couldn't go back and rebase my tree.  Simon, did you ever
figure out if something was odd on your end?

Git developers, any ideas?

thanks,

greg k-h


messy_email.mbox
Description: application/mbox

Re: [PATCH] http(s): automatically try NTLM authentication first

[brian m. carlson]

On Thu, Feb 23, 2017 at 01:03:39AM +, David Turner wrote:
> So, I guess, this patch might be considered a security risk. But on the 
> other hand, even *without* this patch, and without http.allowempty at 
> all, I think a config which simply uses a https://  url without the magic :@
> would try SPNEGO.  As I understand it, the http.allowempty config just 
> makes the traditional :@ urls work. 

No, it's a bit different.  libcurl won't try to authenticate to a server
unless it has a username (and possibly password).  With the curl command
line client, you use a dummy value or -u: to force it to do auth anyway
(because you want, say, GSSAPI).  http.emptyAuth just sets that option
to “:” so libcurl will auth:

if (curl_empty_auth)
curl_easy_setopt(result, CURLOPT_USERPWD, ":");

I just use a dummy username for my URLs, but you can write :@ or any
other permutation to get it to work without emptyAuth.  As a
consequence, you have to opt-in to that on a per-URL (or per-domain)
basis, which is a bit more secure.

> Actually, though, I am not sure this is as bad as it seems, because gssapi
> might protect us.  When I locally tried a fake server, git (libcurl) refused 
> to 
> send my Kerberos credentials because "Server not found in Kerberos 
> database".  I don't have a machine set up with NTLM authentication 
> (because, apparently, that would be insane), so I don't know how to 
> confirm that gssapi would operate off of a whitelist for NTLM as well. 

Yup.  That's pretty much what I thought would happen, since the Kerberos
server has no HTTP/malicious.evil@yourrealm.tld service ticket.
Again, I don't know how NTLM does things, or if it's wrapped in a
suitable ticket format somehow.

Last I base64-decoded an NTLM SPNEGO response, it did not contain the
OID required by GSSAPI as a prefix; it instead contained an “NTLMSSP”
header, which isn't a valid OID.  I didn't delve much further, since I
was pretty sure I didn't want to know more.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH] http(s): automatically try NTLM authentication first

[Junio C Hamano]

Jeff King  writes:

> On Wed, Feb 22, 2017 at 11:34:19PM +, brian m. carlson wrote:
>
>> Browsers usually disable this feature by default, as it basically will
>> attempt to authenticate to any site that sends a 401.  For Kerberos
>> against a malicious site, the user will either not have a valid ticket
>> for that domain, or the user's Kerberos server will refuse to provide a
>> ticket to pass to the server, so there's no security risk involved.
>> 
>> I'm unclear how SPNEGO works with NTLM, so I can't speak for the
>> security of it.  From what I understand of NTLM and from RFC 4559, it
>> consists of a shared secret.  I'm unsure what security measures are in
>> place to not send that to an untrusted server.
>> 
>> As far as Kerberos, this is a desirable feature to have enabled, with
>> little downside.  I just don't know about the security of the NTLM part,
>> and I don't think we should take this patch unless we're sure we know
>> the consequences of it.
>
> Hmm. That would be a problem with my proposed patch 2 then, too, if only
> because it turns the feature on by default in more places.
>
> If it _is_ dangerous to turn on all the time, I'd think we should
> consider warning people in the http.emptyauth documentation.

Yeah, http..emptyAuth that knows where it is going may be a lot
safer but a blanket http.emptyAuth does sound bad.

Re: [PATCH 2/2] http: add an "auto" mode for http.emptyauth

[Jeff King]

On Thu, Feb 23, 2017 at 01:16:33AM +, David Turner wrote:

> I don't know enough about how libcurl handles authentication to know whether 
> these patches are a good idea, but I have a minor comment anyway.

As somebody who is using non-Basic auth, can you apply these patches and
show us the output of:

   GIT_TRACE_CURL=1 \
   git ls-remote https://your-server 2>&1 >/dev/null |
   egrep '(Send|Recv) header: (GET|HTTP|Auth)'

(without http.emptyauth turned on, obviously).

> > +* But only do so when this is _not_ our initial
> > +* request, as we would not then yet know what
> > +* methods are available.
> > +*/
> 
> Eliminate double-negative:
> 
> "But only do this when this is our second or subsequent request, 
> as by then we know what methods are available."

Yeah, that is clearer.

-Peff

Re: feature request: user email config per domain

[Igor Djordjevic BugA]

Hi Tushar,

On 22/02/2017 14:12, Tushar Kapila  wrote:
> So when remote URL has github.com push as tgkp...@search.com but for
> testing.abc.doman:8080 use tgkp...@test.xyz.com ?

I`m not sure if this is sensible, as authorship information is baked
into the commit at the time of committing, which (usually ;) happens
before you get to 'git push' to the other repo.

If possible, changing this info after the fact, on 'git push', would
influence the existing commit you`re trying to send over, so your
'git-push' would have a surprising consequence of not actually
pushing your desired commit at all, but creating a totally new commit
inside the other repo -- this new commit would be exactly the same
patch-wise (in regards to differences introduced), but because of the
changed user info it would be considered a different commit
nonetheless (different hash).

> ... I know I can over ride it per repository, but sometimes
> forget to do that. And even if I unset it, it inadvertantly gets set
> elsewhere when I make a repo and the site 'helps' me by showing me the
> commands to init and clone my new repo.

Otherwise, as you already stated that you find the current local (per
repo) user settings override logic inconvenient (error-prone), you
might be interested in approach described in this[1] Stack Overflow
post.

In short, it uses a template-injected 'post-checkout' hook (triggered
on 'git clone' as well) alongside '.gitconfig' (global) settings to
achieve what seems to be pretty similar to what you asked for (but
might be a bit more sensible), where you may fine-tune it further to
better suit your needs.

On 20/02/2017 21:12, Grant Humphries[2] wrote[1]:
> This answer is partially inspired by the post by @Saucier, but I was
> looking for an automated way to set user.name and user.email on a per
> repo basis, based on the remote, that was a little more light weight
> than the git-passport package that he developed. Also h/t to @John
> for the useConfigOnly setting. Here is my solution:
>
> .gitconfig changes:
>
>   [github]
>   name = 
>   email = 
>   [gitlab]
>   name = 
>   email = 
>   [init]
>   templatedir = ~/.git-templates
>   [user]
>   useConfigOnly = true
>
> post-checkout hook which should be saved to the following path:
> ~/.git-templates/hooks/post-checkout:
>
>   #!/usr/bin/env bash
>   
>   # make regex matching below case insensitive
>   shopt -s nocasematch
>   
>   # values in the services array should have a corresponding section in
>   # .gitconfig where the 'name' and 'email' for that service are specified
>   remote_url="$( git config --get --local remote.origin.url )"
>   services=(
>   'github'
>   'gitlab'
>   )
>   
>   set_local_user_config() {
>   local service="${1}"
>   local config="${2}"
>   local service_config="$( git config --get ${service}.${config} 
> )"
>   local local_config="$( git config --get --local user.${config} 
> )"
>   
>   if [[ "${local_config}" != "${service_config}" ]]; then
>   git config --local "user.${config}" "${service_config}"
>   echo "repo 'user.${config}' has been set to 
> '${service_config}'"
>   fi
>   }
>   
>   # if remote_url doesn't contain the any of the values in the services
>   # array the user name and email will remain unset and the
>   # user.useConfigOnly = true setting in .gitconfig will prompt for those
>   # credentials and prevent commits until they are defined
>   for s in "${services[@]}"; do
>   if [[ "${remote_url}" =~ "${s}" ]]; then
>   set_local_user_config "${s}" 'name'
>   set_local_user_config "${s}" 'email'
>   break
>   fi
>   done
>
> I use different credentials for github and gitlab, but those
> references in the code above could be replaced or augmented with any
> service that you use. In order to have the post-checkout hook
> automatically set the user name and email locally for a repo after a
> checkout make sure the service name appears in the remote url, add it
> to the services array in the post-checkout script and create a
> section for it in your .gitconfig that contains your user name and
> email for that service.
>
> If none of the service names appear in the remote url or the repo
> doesn't have a remote the user name and email will not be set
> locally. In these cases the user.useConfigOnly setting will be in
> play which will not allow you to make commits until the user name and
> email are set at the repo level, and will prompt the user to
> configure that information.

Regards,
Buga

*P.S.* For the purpose of completeness and archiving I copied the
Stack Overflow post[1] here as well, but all the credits 

RE: [PATCH 2/2] http: add an "auto" mode for http.emptyauth

[David Turner]

I don't know enough about how libcurl handles authentication to know whether 
these patches are a good idea, but I have a minor comment anyway.

> -Original Message-
> From: Jeff King [mailto:p...@peff.net]
> +static int curl_empty_auth_enabled(void) {
> + if (curl_empty_auth < 0) {
> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> + /*
> +  * In the automatic case, kick in the empty-auth
> +  * hack as long as we would potentially try some
> +  * method more exotic than "Basic".
> +  *
> +  * But only do so when this is _not_ our initial
> +  * request, as we would not then yet know what
> +  * methods are available.
> +  */

Eliminate double-negative:

"But only do this when this is our second or subsequent request, 
as by then we know what methods are available."

RE: [PATCH] http(s): automatically try NTLM authentication first

[David Turner]

> -Original Message-
> From: brian m. carlson [mailto:sand...@crustytoothpaste.net]
> 
> This is SPNEGO.  It will work with NTLM as well as Kerberos.
> 
> Browsers usually disable this feature by default, as it basically will 
> attempt to
> authenticate to any site that sends a 401.  For Kerberos against a malicious
> site, the user will either not have a valid ticket for that domain, or the 
> user's
> Kerberos server will refuse to provide a ticket to pass to the server, so
> there's no security risk involved.
> 
> I'm unclear how SPNEGO works with NTLM, so I can't speak for the security
> of it.  From what I understand of NTLM and from RFC 4559, it consists of a
> shared secret.  I'm unsure what security measures are in place to not send
> that to an untrusted server.
> 
> As far as Kerberos, this is a desirable feature to have enabled, with little
> downside.  I just don't know about the security of the NTLM part, and I don't
> think we should take this patch unless we're sure we know the
> consequences of it.

NTLM on its own is bad:

https://msdn.microsoft.com/en-us/library/windows/desktop/aa378749(v=vs.85).aspx
says:

"
1. (Interactive authentication only) A user accesses a client computer and 
provides a domain name, user name, and password. The client computes a 
cryptographic hash of the password and discards the actual password.
2. The client sends the user name to the server (in plaintext).
3. The server generates a 16-byte random number, called a challenge or 
nonce, and sends it to the client.
4. The client encrypts this challenge with the hash of the user's password 
and returns the result to the server. This is called the response.
..."

Wait, what?  If I'm a malicious server, I can get access to an offline oracle
for whether I've correctly guessed the user's password?  That doesn't 
sound secure at all!  Skimming the SPNEGO RFCs, there appears to be no
mitigation for this.  

So, I guess, this patch might be considered a security risk. But on the 
other hand, even *without* this patch, and without http.allowempty at 
all, I think a config which simply uses a https://  url without the magic :@
would try SPNEGO.  As I understand it, the http.allowempty config just 
makes the traditional :@ urls work. 

Actually, though, I am not sure this is as bad as it seems, because gssapi
might protect us.  When I locally tried a fake server, git (libcurl) refused to 
send my Kerberos credentials because "Server not found in Kerberos 
database".  I don't have a machine set up with NTLM authentication 
(because, apparently, that would be insane), so I don't know how to 
confirm that gssapi would operate off of a whitelist for NTLM as well. 

Re: feature request: user email config per domain

[Jeff King]

On Wed, Feb 22, 2017 at 06:42:02PM +0530, Tushar Kapila wrote:

> Feature request :  can we have a config for email per repo domain ?
> Something like:
> 
> git config --global domain.user.email tgkp...@test.xyz.com
> testing.abc.doman:8080
> 
> git config --global domain.user.email tgkp...@xyz.com abc.doman:80
> 
> git config --global domain.user.email tgkp...@search.com github.com
> 
> So when remote URL has github.com push as tgkp...@search.com but for
> testing.abc.doman:8080 use tgkp...@test.xyz.com ?

The idea of "the domain for this repo" doesn't really match Git's
distributed model. A repo may have no remotes at all, or multiple
remotes with different domains (or even remotes which do not have a
domain associated with them, like local files, or remote helpers which
obscure the domain name).

It sounds like you're assuming that the domain would come from looking
at remote.origin.url. Which I agree would work in the common case of
"git clone https://example.com;, but I'm not comfortable with the number
of corner cases the feature has.

I'd much rather see something based on the working tree path, like Duy's
conditional config includes. Then you write something in your
~/.gitconfig like:

  [include "gitdir:/home/peff/work/"]
  path = .gitconfig-work

  [include "gitdir:/home/peff/play/"]
  path = .gitconfig-play

and set user.email as appropriate in each.

You may also set user.useConfigOnly in your ~/.gitconfig. Then you'd
have to set user.email in each individual repository, but at least Git
would complain and remind you when you forget to do so, rather than
silently using the wrong email.

-Peff

Re: [PATCH] submodule init: warn about falling back to a local path

[Stefan Beller]

On Wed, Feb 22, 2017 at 7:01 AM, Philip Oakley  wrote:
>
> Would a note in the user docs about the fallback you list above also be
> useful?

duh! yes it would! Will look at the documentation and reroll.

Re: [PATCH] t: add multi-parent test of diff-tree --stdin

[brian m. carlson]

On Wed, Feb 22, 2017 at 03:42:25PM -0800, Junio C Hamano wrote:
> "brian m. carlson"  writes:
> 
> > We were lacking a test that covered the multi-parent case for commits.
> > Add a basic test to ensure we do not regress this behavior in the
> > future.
> >
> > Signed-off-by: brian m. carlson 
> > ---
> >  t/t4013-diff-various.sh | 19 +++
> >  1 file changed, 19 insertions(+)
> >
> > It's a little bit ugly to me that this test hard-codes so many values,
> > and I'm concerned that it may be unnecessarily brittle.  However, I
> > don't have a good idea of how to perform the kind of comprehensive test
> > we need otherwise.
> 
> Hmph, perhaps the expectation can be created out of the output from
> 'git diff-tree master^ master' or something?

Okay.  I'll try that in a reroll.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH] t: add multi-parent test of diff-tree --stdin

[Jeff King]

On Wed, Feb 22, 2017 at 03:42:25PM -0800, Junio C Hamano wrote:

> "brian m. carlson"  writes:
> 
> > We were lacking a test that covered the multi-parent case for commits.
> > Add a basic test to ensure we do not regress this behavior in the
> > future.
> >
> > Signed-off-by: brian m. carlson 
> > ---
> >  t/t4013-diff-various.sh | 19 +++
> >  1 file changed, 19 insertions(+)
> >
> > It's a little bit ugly to me that this test hard-codes so many values,
> > and I'm concerned that it may be unnecessarily brittle.  However, I
> > don't have a good idea of how to perform the kind of comprehensive test
> > we need otherwise.
> 
> Hmph, perhaps the expectation can be created out of the output from
> 'git diff-tree master^ master' or something?

I had a similar thought. It may also be worth testing that:

  echo "$(git rev-parse master) $(git rev-parse other)" | git diff-tree --stdin

shows the diff between "other" and "master", and not just "master^" and
"master" (i.e., it is not clear from the test expectation that the code
actually is parsing the second parent and not accidentally ignoring it).

For completeness, it would probably make sense to test the multi-parent
case, too.

-Peff

Re: [PATCH] t: add multi-parent test of diff-tree --stdin

[Junio C Hamano]

"brian m. carlson"  writes:

> We were lacking a test that covered the multi-parent case for commits.
> Add a basic test to ensure we do not regress this behavior in the
> future.
>
> Signed-off-by: brian m. carlson 
> ---
>  t/t4013-diff-various.sh | 19 +++
>  1 file changed, 19 insertions(+)
>
> It's a little bit ugly to me that this test hard-codes so many values,
> and I'm concerned that it may be unnecessarily brittle.  However, I
> don't have a good idea of how to perform the kind of comprehensive test
> we need otherwise.

Hmph, perhaps the expectation can be created out of the output from
'git diff-tree master^ master' or something?

>
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index d09acfe48e..e6c2a7a369 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -349,4 +349,23 @@ test_expect_success 'diff-tree --stdin with log 
> formatting' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success 'diff-tree --stdin with two parent commits' '
> + cat >expect <<-\EOF &&
> + c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
> + :04 04 da7a33fa77d8066d6698643940ce5860fe2d7fb3 
> f977ed46ae6873c1c30ab878e15a4accedc3618b M  dir
> + :100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 
> f4615da674c09df322d6ba8d6b21ecfb1b1ba510 M  file0
> + :00 100644  
> 7289e35bff32727c08dda207511bec138fdb9ea5 A  file3
> + 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
> + :04 04 847b63d013de168b2de618c5ff9720d5ab27e775 
> 65f5c9dd60ce3b2b3324b618ac7accf8d912c113 M  dir
> + :00 100644  
> b1e67221afe8461efd244b487afca22d46b95eb8 A  file1
> + 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
> + :04 04 da7a33fa77d8066d6698643940ce5860fe2d7fb3 
> 847b63d013de168b2de618c5ff9720d5ab27e775 M  dir
> + :100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 
> b414108e81e5091fe0974a1858b4d0d22b107f70 M  file0
> + :100644 00 01e79c32a8c99c557f0757da7cb6d65b3414466d 
>  D  file2
> + EOF
> + git rev-list --parents master | git diff-tree --stdin >actual &&
> + test_cmp expect actual
> +'
> +
> +
>  test_done

[PATCH 1/2] http: restrict auth methods to what the server advertises

[Jeff King]

By default, we tell curl to use CURLAUTH_ANY, which does not
limit its set of auth methods. However, this results in an
extra round-trip to the server when authentication is
required. After we've fed the credential to curl, it wants
to probe the server to find its list of available methods
before sending an Authorization header.

We can shortcut this by limiting our http_auth_methods by
what the server told us it supports. In some cases (such as
when the server only supports Basic), that lets curl skip
the extra probe request.

The end result should look the same to the user, but you can
use GIT_TRACE_CURL to verify the sequence of requests:

  GIT_TRACE_CURL=1 \
  git ls-remote https://example.com/repo.git \
  2>&1 >/dev/null |
  egrep '(Send|Recv) header: (GET|HTTP|Auth)'

Before this patch, hitting a Basic-only server like
github.com results in:

  Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  Recv header: HTTP/1.1 401 Authorization Required
  Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  Recv header: HTTP/1.1 401 Authorization Required
  Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  Send header: Authorization: Basic 
  Recv header: HTTP/1.1 200 OK

And after:

  Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  Recv header: HTTP/1.1 401 Authorization Required
  Send header: GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  Send header: Authorization: Basic 
  Recv header: HTTP/1.1 200 OK

The possible downsides are:

  - This only helps for a Basic-only server; for a server
with multiple auth options, curl may still send a probe
request to see which ones are available (IOW, there's no
way to say "don't probe, I already know what the server
will say").

  - The http_auth_methods variable is global, so this will
apply to all further requests. That's acceptable for
Git's usage of curl, though, which also treats the
credentials as global. I.e., in any given program
invocation we hit only one conceptual server (we may be
redirected at the outset, but in that case that's whose
auth_avail field we'd see).

Signed-off-by: Jeff King 
---
 http.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/http.c b/http.c
index 90a1c0f11..a05609766 100644
--- a/http.c
+++ b/http.c
@@ -1347,6 +1347,8 @@ static int handle_curl_result(struct slot_results 
*results)
} else {
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
http_auth_methods &= ~CURLAUTH_GSSNEGOTIATE;
+   if (results->auth_avail)
+   http_auth_methods &= results->auth_avail;
 #endif
return HTTP_REAUTH;
}
-- 
2.12.0.rc2.597.g959f68882

Re: [PATCH] http(s): automatically try NTLM authentication first

[Jeff King]

On Wed, Feb 22, 2017 at 11:34:19PM +, brian m. carlson wrote:

> Browsers usually disable this feature by default, as it basically will
> attempt to authenticate to any site that sends a 401.  For Kerberos
> against a malicious site, the user will either not have a valid ticket
> for that domain, or the user's Kerberos server will refuse to provide a
> ticket to pass to the server, so there's no security risk involved.
> 
> I'm unclear how SPNEGO works with NTLM, so I can't speak for the
> security of it.  From what I understand of NTLM and from RFC 4559, it
> consists of a shared secret.  I'm unsure what security measures are in
> place to not send that to an untrusted server.
> 
> As far as Kerberos, this is a desirable feature to have enabled, with
> little downside.  I just don't know about the security of the NTLM part,
> and I don't think we should take this patch unless we're sure we know
> the consequences of it.

Hmm. That would be a problem with my proposed patch 2 then, too, if only
because it turns the feature on by default in more places.

If it _is_ dangerous to turn on all the time, I'd think we should
consider warning people in the http.emptyauth documentation.

-Peff

[PATCH 2/2] http: add an "auto" mode for http.emptyauth

[Jeff King]

This variable needs to be specified to make some types of
non-basic authentication work, but ideally this would just
work out of the box for everyone.

However, simply setting it to "1" by default introduces an
extra round-trip for cases where it _isn't_ useful. We end
up sending a bogus empty credential that the server rejects.

Instead, let's introduce an automatic mode, that works like
this:

  1. We won't try to send the bogus credential on the first
 request. We'll wait to get an HTTP 401, as usual.

  2. After seeing an HTTP 401, the empty-auth hack will kick
 in only when we know there is an auth method beyond
 "Basic" to be tried.

That should make it work out of the box, without incurring
any extra round-trips for people hitting Basic-only servers.

This _does_ incur an extra round-trip if you really want to
use "Basic" but your server advertises other methods (the
emptyauth hack will kick in but fail, and then Git will
actually ask for a password).

The auto mode may incur an extra round-trip over setting
http.emptyauth=true, because part of the emptyauth hack is
to feed this blank password to curl even before we've made a
single request.

Signed-off-by: Jeff King 
---
My open questions are:

  - I don't have anything but a Basic server to test against. So it's
entirely possible that this doesn't actually work in the NTLM case.

  - what does a request log look like for somebody actually using NTLM?
It's possible if the initial request sends a restrict auth_avail,
that curl could get away without the extra probe request, and we'd
end up with the same number of requests for "auto" mode versus
http.emptyauth=true.

  - the whole "don't use this on the initial request" flag feels really
hacky. It's a side effect of how emptyauth tries to kick in even
before we have sent any requests. Probably it should have been
handled in the 401 code path originally, but I'm hesitant to change
it now. I suspect it is eliminating a round-trip in practice when it
is enabled.

  - I didn't test a server that advertises Basic and something else, but
really only takes Basic. So I'm just assuming that it incurs the
extra round-trip (actually probably two, one for curl's method
probe).

  - When your curl is too old to do CURLAUTH_ANY, I just left the
default to disable emptyauth. But it could easily be "1" if people
care.

 http.c | 38 ++
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/http.c b/http.c
index a05609766..ea70ec1ee 100644
--- a/http.c
+++ b/http.c
@@ -109,7 +109,7 @@ static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
 static int http_proactive_auth;
 static const char *user_agent;
-static int curl_empty_auth;
+static int curl_empty_auth = -1;
 
 enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
 
@@ -125,6 +125,7 @@ static struct credential cert_auth = CREDENTIAL_INIT;
 static int ssl_cert_password_required;
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
 static unsigned long http_auth_methods = CURLAUTH_ANY;
+static int http_auth_methods_restricted;
 #endif
 
 static struct curl_slist *pragma_header;
@@ -382,10 +383,37 @@ static int http_options(const char *var, const char 
*value, void *cb)
return git_default_config(var, value, cb);
 }
 
+static int curl_empty_auth_enabled(void)
+{
+   if (curl_empty_auth < 0) {
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+   /*
+* In the automatic case, kick in the empty-auth
+* hack as long as we would potentially try some
+* method more exotic than "Basic".
+*
+* But only do so when this is _not_ our initial
+* request, as we would not then yet know what
+* methods are available.
+*/
+   return http_auth_methods_restricted &&
+  http_auth_methods != CURLAUTH_BASIC;
+#else
+   /*
+* Our libcurl is too old to do AUTH_ANY in the first place;
+* just default to turning the feature off.
+*/
+   return 0;
+#endif
+   }
+
+   return curl_empty_auth;
+}
+
 static void init_curl_http_auth(CURL *result)
 {
if (!http_auth.username || !*http_auth.username) {
-   if (curl_empty_auth)
+   if (curl_empty_auth_enabled())
curl_easy_setopt(result, CURLOPT_USERPWD, ":");
return;
}
@@ -1079,7 +1107,7 @@ struct active_request_slot *get_active_slot(void)
 #ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
 #endif
-   if (http_auth.password || curl_empty_auth)
+   if (http_auth.password || curl_empty_auth_enabled())
init_curl_http_auth(slot->curl);
 
return slot;
@@ -1347,8 +1375,10 @@ static int 

[PATCH] t: add multi-parent test of diff-tree --stdin

[brian m. carlson]

We were lacking a test that covered the multi-parent case for commits.
Add a basic test to ensure we do not regress this behavior in the
future.

Signed-off-by: brian m. carlson 
---
 t/t4013-diff-various.sh | 19 +++
 1 file changed, 19 insertions(+)

It's a little bit ugly to me that this test hard-codes so many values,
and I'm concerned that it may be unnecessarily brittle.  However, I
don't have a good idea of how to perform the kind of comprehensive test
we need otherwise.

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index d09acfe48e..e6c2a7a369 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -349,4 +349,23 @@ test_expect_success 'diff-tree --stdin with log 
formatting' '
test_cmp expect actual
 '
 
+test_expect_success 'diff-tree --stdin with two parent commits' '
+   cat >expect <<-\EOF &&
+   c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a
+   :04 04 da7a33fa77d8066d6698643940ce5860fe2d7fb3 
f977ed46ae6873c1c30ab878e15a4accedc3618b M  dir
+   :100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 
f4615da674c09df322d6ba8d6b21ecfb1b1ba510 M  file0
+   :00 100644  
7289e35bff32727c08dda207511bec138fdb9ea5 A  file3
+   9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+   :04 04 847b63d013de168b2de618c5ff9720d5ab27e775 
65f5c9dd60ce3b2b3324b618ac7accf8d912c113 M  dir
+   :00 100644  
b1e67221afe8461efd244b487afca22d46b95eb8 A  file1
+   1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44
+   :04 04 da7a33fa77d8066d6698643940ce5860fe2d7fb3 
847b63d013de168b2de618c5ff9720d5ab27e775 M  dir
+   :100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 
b414108e81e5091fe0974a1858b4d0d22b107f70 M  file0
+   :100644 00 01e79c32a8c99c557f0757da7cb6d65b3414466d 
 D  file2
+   EOF
+   git rev-list --parents master | git diff-tree --stdin >actual &&
+   test_cmp expect actual
+'
+
+
 test_done

Re: [RFC 03/14] upload-pack: test negotiation with changing repo

[Junio C Hamano]

Jonathan Tan  writes:

>> This somehow looks like a good thing to do even in production.  Am I
>> mistaken?
>
> Yes, that's true. If this patch set stalls (for whatever reason), I'll
> spin this off into an independent patch.

... which may be needed.

As to the main goal of this topic, I think the "ask by refname (with
glob)" is very good thing to start the "client speaks first" v2
protocol, as it would allow us to reduce the size of the initial
advertisement for common cases (i.e. remote..fetch is likely
to list only refs/heads/* on the left hand side of a refspec).  And
adding this to v1 is probably a good first step to make sure the
code that is currently used by v1 protocol exchange that will be
shared with the v2 protocols are prepared to be driven by refname
without knowing the exact object name until the final round.

Re: [PATCH] http(s): automatically try NTLM authentication first

[brian m. carlson]

On Wed, Feb 22, 2017 at 09:04:14PM +, David Turner wrote:
> > -Original Message-
> > From: Junio C Hamano [mailto:jch2...@gmail.com] On Behalf Of Junio C
> > Hamano
> > Sent: Wednesday, February 22, 2017 3:20 PM
> > To: David Turner 
> > Cc: git@vger.kernel.org; sand...@crustytoothpaste.net; Johannes Schindelin
> > ; Eric Sunshine
> > ; Jeff King 
> > Subject: Re: [PATCH] http(s): automatically try NTLM authentication first
> > 
> > 
> > Some other possible worries we may have had I can think of are:
> > 
> >  - With this enabled unconditionally, would we leak some information?
> 
> I think "NTLM" is actually a misnomer here (I just copied Johannes's 
> commit message). The mechanism is actually SPNEGO, if I understand this 
> correctly. It seems to me that this is probably secure, since it is apparently
> widely implemented in browsers.

This is SPNEGO.  It will work with NTLM as well as Kerberos.

Browsers usually disable this feature by default, as it basically will
attempt to authenticate to any site that sends a 401.  For Kerberos
against a malicious site, the user will either not have a valid ticket
for that domain, or the user's Kerberos server will refuse to provide a
ticket to pass to the server, so there's no security risk involved.

I'm unclear how SPNEGO works with NTLM, so I can't speak for the
security of it.  From what I understand of NTLM and from RFC 4559, it
consists of a shared secret.  I'm unsure what security measures are in
place to not send that to an untrusted server.

As far as Kerberos, this is a desirable feature to have enabled, with
little downside.  I just don't know about the security of the NTLM part,
and I don't think we should take this patch unless we're sure we know
the consequences of it.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH] http(s): automatically try NTLM authentication first

[Jeff King]

On Wed, Feb 22, 2017 at 02:35:11PM -0800, Junio C Hamano wrote:

> A solution along your line would help Negotiate users OOB experience
> without hurting the servers that do not offer Negotiate, but until
> that materializes, users can set the lazier http.emptyAuth on
> (without selectively setting http..emptyAuth off for sites
> without Negotiate) and hurt the servers by throwing an empty auth
> anyway regardless of the default, so the flipping of the default is
> not fundamentally adding more harm in that sense.

I was hoping to materialize it today. :)

Here's what I came up with. I have a lot of questions about the second
patch which I'll outline there. But I think it may be a good start.

  [1/2]: http: restrict auth methods to what the server advertises
  [2/2]: http: add an "auto" mode for http.emptyauth

 http.c | 38 +++---
 1 file changed, 35 insertions(+), 3 deletions(-)

-Peff

Re: [PATCH v5 03/19] builtin/diff-tree: convert to struct object_id

[brian m. carlson]

On Wed, Feb 22, 2017 at 02:16:42PM -0500, Jeff King wrote:
> On Wed, Feb 22, 2017 at 10:50:21AM -0800, Junio C Hamano wrote:
> 
> > "brian m. carlson"  writes:
> > 
> > > Convert most leaf functions to struct object_id.  Change several
> > > hardcoded numbers to uses of parse_oid_hex.  In doing so, verify that we
> > > when we want two trees, we have exactly two trees.
> > >
> > > Finally, in stdin_diff_commit, avoid accessing the byte after the NUL.
> > > This will be a NUL as well, since the first NUL was a newline we
> > > overwrote.  However, with parse_oid_hex, we no longer need to increment
> > > the pointer directly, and can simply increment it as part of our check
> > > for the space character.
> > 
> > After reading the pre- and post-image twice, I think I convinced
> > myself that this is a faithful conersion and they do the same thing.
> 
> I think this is correct, too (but then, I think it largely comes from
> the patch I wrote the other night. So I did look at it carefully, but
> it's not exactly an independent review).

I did take that part, as well as other parts, from your patch.

I have a test, which I'll send in a minute, that verifies that they do
the same thing.  At first I introduced a segfault, and the test caught
it, so I feel confident that the patch does indeed function as the old
code did.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: feature request: user email config per domain

[Andrew Ardill]

On 23 February 2017 at 00:12, Tushar Kapila  wrote:
> I can set my email via:
> git config --global user.email tgkp...@xyz.dom
>
> this is dangerous when I use this my office or in a multi repository
> provider environment where my email is different for a few (like
> tgkp...@search.com for github and tus...@mycompany.com for my company
> private repo).

There has been a large discussion around this idea before, see this
thread for example [0].

The proposed idea was to have something set in your global config that
would only be turned on if you were within a given directory. This
would allow you to have two root directories, one for your work
projects and one for open source projects (for example) and any git
repositories within those folders would each would have their own
config options automatically applied based on their location.

There was a patch suggested, and it worked quite well, but nothing
further has been done to my knowledge.

[0] http://public-inbox.org/git/7vvc3d1o01@alter.siamese.dyndns.org/

Regards,

Andrew Ardill

Re: [PATCH] http(s): automatically try NTLM authentication first

[Junio C Hamano]

Jeff King  writes:

> On Wed, Feb 22, 2017 at 01:57:28PM -0800, Junio C Hamano wrote:
>
>> Jeff King  writes:
>> 
>> > On Wed, Feb 22, 2017 at 01:25:11PM -0800, Junio C Hamano wrote:
>> >> 
>> >> Thanks for your thoughts.  I'd think that we should take this change
>> >> and leave the optimization for later, then.  It's not like the
>> >> change of the default is making the normal situation any worse, it
>> >> seems.
>> >
>> > I'm not excited that it will start making known bogus-username requests
>> > by default to servers which do not even support Negotiate. I guess that
>> > is really the server-operators problem, but it feels pretty hacky.
>> 
>> I guess that's another valid concern.  The servers used to be able
>> to say "Ah, this repository needs auth and this request does not, so
>> reject it without asking the auth-db".  Now it must say "Ah, this
>> repository needs auth and this request does have one, but it is
>> empty so let's not even bother the auth-db" in order to reject a
>> useless "empty-auth" request with the same efficiency.
>> 
>> After the first request without auth (that fails), do we learn
>> anything useful from the server side (like "it knows Negotiate")
>> that we can use to flip the "empty-auth" bit to give a better
>> default to people from both worlds, I wonder...?
>
> Yes, that's exactly what I was trying to say in my first message.

I see.  I am still inclined to take this as-is for now to cook in
'next', though.  

A solution along your line would help Negotiate users OOB experience
without hurting the servers that do not offer Negotiate, but until
that materializes, users can set the lazier http.emptyAuth on
(without selectively setting http..emptyAuth off for sites
without Negotiate) and hurt the servers by throwing an empty auth
anyway regardless of the default, so the flipping of the default is
not fundamentally adding more harm in that sense.

Re: [PATCH] http(s): automatically try NTLM authentication first

[Jeff King]

On Wed, Feb 22, 2017 at 01:57:28PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > On Wed, Feb 22, 2017 at 01:25:11PM -0800, Junio C Hamano wrote:
> >> 
> >> Thanks for your thoughts.  I'd think that we should take this change
> >> and leave the optimization for later, then.  It's not like the
> >> change of the default is making the normal situation any worse, it
> >> seems.
> >
> > I'm not excited that it will start making known bogus-username requests
> > by default to servers which do not even support Negotiate. I guess that
> > is really the server-operators problem, but it feels pretty hacky.
> 
> I guess that's another valid concern.  The servers used to be able
> to say "Ah, this repository needs auth and this request does not, so
> reject it without asking the auth-db".  Now it must say "Ah, this
> repository needs auth and this request does have one, but it is
> empty so let's not even bother the auth-db" in order to reject a
> useless "empty-auth" request with the same efficiency.
> 
> After the first request without auth (that fails), do we learn
> anything useful from the server side (like "it knows Negotiate")
> that we can use to flip the "empty-auth" bit to give a better
> default to people from both worlds, I wonder...?

Yes, that's exactly what I was trying to say in my first message.

-Peff

Re: [PATCH] http(s): automatically try NTLM authentication first

[Junio C Hamano]

Jeff King  writes:

> On Wed, Feb 22, 2017 at 01:25:11PM -0800, Junio C Hamano wrote:
>> 
>> Thanks for your thoughts.  I'd think that we should take this change
>> and leave the optimization for later, then.  It's not like the
>> change of the default is making the normal situation any worse, it
>> seems.
>
> I'm not excited that it will start making known bogus-username requests
> by default to servers which do not even support Negotiate. I guess that
> is really the server-operators problem, but it feels pretty hacky.

I guess that's another valid concern.  The servers used to be able
to say "Ah, this repository needs auth and this request does not, so
reject it without asking the auth-db".  Now it must say "Ah, this
repository needs auth and this request does have one, but it is
empty so let's not even bother the auth-db" in order to reject a
useless "empty-auth" request with the same efficiency.

After the first request without auth (that fails), do we learn
anything useful from the server side (like "it knows Negotiate")
that we can use to flip the "empty-auth" bit to give a better
default to people from both worlds, I wonder...?

Re: [PATCH] http(s): automatically try NTLM authentication first

[Jeff King]

On Wed, Feb 22, 2017 at 01:25:11PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I don't think it incurs an extra round-trip now, because of the way
> > libcurl works. Though I think it _does_ make it harder for curl to later
> > optimize out that extra round-trip.
> > ...
> > In the current trace, you can see that libcurl insists on making a
> > second auth-less request after we've fed it credentials. I'm not sure
> > how to get rid of this useless extra round-trip, but it would be nice to
> > do so (IIRC, it is a probe request to find out the list of auth types
> > that the server supports, which are not remembered from the previous
> > request).
> > ...
> > With http.emptyauth, the second round-trip _isn't_ useless. It's trying
> > to send the empty credential.
> > 
> > So while curl isn't currently optimizing out the second call, I think
> > http.emptyauth makes it harder to do the right thing.
> > ...
> > I think that would keep it to 2 round-trips for the normal "Basic" case,
> > as well as for the GSSNegotiate case. It would be 3 requests when the
> > server offers GSSNegotiate but you can't use it (but you could set
> > http.emptyauth=false to optimize that out).
> 
> Thanks for your thoughts.  I'd think that we should take this change
> and leave the optimization for later, then.  It's not like the
> change of the default is making the normal situation any worse, it
> seems.

I'm not excited that it will start making known bogus-username requests
by default to servers which do not even support Negotiate. I guess that
is really the server-operators problem, but it feels pretty hacky.

-Peff

Re: [PATCH] http(s): automatically try NTLM authentication first

[Jeff King]

On Wed, Feb 22, 2017 at 01:16:33PM -0800, Junio C Hamano wrote:

> David Turner  writes:
> 
> > Always, no.  For failed authentication (or authorization), apparently, yes. 
> >  
> > I tested this by  setting the variable to false and then true, and trying 
> > to 
> > Push to a github repository which I didn't have write access to, with 
> > both an empty username (https://@:github.com/...) and no username 
> > (http://github.com/...).   I ran this under GIT_CURL_VERBOSE=1 and
> > I saw two 401 responses in the "http.emptyauth=true" case and one
> > in the false case.  I also tried with a repo that I did have access to 
> > (first
> > configuring the necessary tokens for HTTPS push access), and saw two
> > 401 responses in *both* cases.  
> 
> Thanks; that matches my observation.  I do not think we care about
> an extra roundtrip for the failure case, but as long as we do not
> increase the number of roundtrip in the normal case, we can declare
> that this is an improvement.  I am not quite sure where that extra
> 401 comes from in the normal case, and that might be an indication
> that we already are doing something wrong, though.

This patch drops the useless probe request:

diff --git a/http.c b/http.c
index 943e630ea..7b4c2db86 100644
--- a/http.c
+++ b/http.c
@@ -1663,6 +1663,9 @@ static int http_request(const char *url,
curlinfo_strbuf(slot->curl, CURLINFO_EFFECTIVE_URL,
options->effective_url);
 
+   if (results.auth_avail == CURLAUTH_BASIC)
+   http_auth_methods = CURLAUTH_BASIC;
+
curl_slist_free_all(headers);
strbuf_release();
 

but setting http.emptyauth adds back in the useless request. I think
that could be fixed by skipping the empty-auth thing when
http_auth_methods does not have CURLAUTH_NEGOTIATE in it (or perhaps
other methods need it to, so maybe skip it if _just_ BASIC is set).

I suspect the patch above could probably be generalized as:

  /* cut out methods we know the server doesn't support */
  http_auth_methods &= results.auth_avail;

and let curl figure it out from there.

-Peff

Re: [PATCH] http(s): automatically try NTLM authentication first

[Junio C Hamano]

Jeff King  writes:

> I don't think it incurs an extra round-trip now, because of the way
> libcurl works. Though I think it _does_ make it harder for curl to later
> optimize out that extra round-trip.
> ...
> In the current trace, you can see that libcurl insists on making a
> second auth-less request after we've fed it credentials. I'm not sure
> how to get rid of this useless extra round-trip, but it would be nice to
> do so (IIRC, it is a probe request to find out the list of auth types
> that the server supports, which are not remembered from the previous
> request).
> ...
> With http.emptyauth, the second round-trip _isn't_ useless. It's trying
> to send the empty credential.
> 
> So while curl isn't currently optimizing out the second call, I think
> http.emptyauth makes it harder to do the right thing.
> ...
> I think that would keep it to 2 round-trips for the normal "Basic" case,
> as well as for the GSSNegotiate case. It would be 3 requests when the
> server offers GSSNegotiate but you can't use it (but you could set
> http.emptyauth=false to optimize that out).

Thanks for your thoughts.  I'd think that we should take this change
and leave the optimization for later, then.  It's not like the
change of the default is making the normal situation any worse, it
seems.

Re: [PATCH] http(s): automatically try NTLM authentication first

[Junio C Hamano]

David Turner  writes:

> Always, no.  For failed authentication (or authorization), apparently, yes.  
> I tested this by  setting the variable to false and then true, and trying to 
> Push to a github repository which I didn't have write access to, with 
> both an empty username (https://@:github.com/...) and no username 
> (http://github.com/...).   I ran this under GIT_CURL_VERBOSE=1 and
> I saw two 401 responses in the "http.emptyauth=true" case and one
> in the false case.  I also tried with a repo that I did have access to (first
> configuring the necessary tokens for HTTPS push access), and saw two
> 401 responses in *both* cases.  

Thanks; that matches my observation.  I do not think we care about
an extra roundtrip for the failure case, but as long as we do not
increase the number of roundtrip in the normal case, we can declare
that this is an improvement.  I am not quite sure where that extra
401 comes from in the normal case, and that might be an indication
that we already are doing something wrong, though.

Re: [PATCH] http(s): automatically try NTLM authentication first

[Jeff King]

On Wed, Feb 22, 2017 at 12:19:56PM -0800, Junio C Hamano wrote:

> > It would be one thing if cURL would not let the user specify credentials
> > interactively after attempting NTLM authentication (i.e. login
> > credentials), but that is not the case.
> >
> > It would be another thing if attempting NTLM authentication was not
> > usually what users need to do when trying to authenticate via https://.
> > But that is also not the case.
> 
> Some other possible worries we may have had I can think of are:
> 
>  - With this enabled unconditionally, would we leak some information?
> 
>  - With this enabled unconditionally, would we always incur an extra
>roundtrip for people who are not running NTLM at all?
> 
> I do not think the former is the case, but what would I know (adding a
> few people involved in the original thread to CC: ;-)

I don't think it incurs an extra round-trip now, because of the way
libcurl works. Though I think it _does_ make it harder for curl to later
optimize out that extra round-trip.

The easiest way to see the difference is to run something like:

  GIT_CURL_VERBOSE=1 \
  git ls-remote https://example.com/repo-which-needs-auth.git 2>trace
  egrep '^>|^< HTTP|Authorization' trace

Before this patch, I get (this is against github.com, which only does
Basic auth):

  > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  < HTTP/1.1 401 Authorization Required
  > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  < HTTP/1.1 401 Authorization Required
  > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  Authorization: Basic 
  < HTTP/1.1 200 OK

And after I get:

  > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  < HTTP/1.1 401 Authorization Required
  > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  Authorization: Basic Og==
  < HTTP/1.1 401 Authorization Required
  > GET /repo.git/info/refs?service=git-upload-pack HTTP/1.1
  Authorization: Basic 
  < HTTP/1.1 200 OK

In the current trace, you can see that libcurl insists on making a
second auth-less request after we've fed it credentials. I'm not sure
how to get rid of this useless extra round-trip, but it would be nice to
do so (IIRC, it is a probe request to find out the list of auth types
that the server supports, which are not remembered from the previous
request).

With http.emptyauth, the second round-trip _isn't_ useless. It's trying
to send the empty credential.

So while curl isn't currently optimizing out the second call, I think
http.emptyauth makes it harder to do the right thing. That's probably
fixable if the logic ends up more like:

  - curl reports a 401 to us; actually look at the list of auth methods.

  - if there was gss-negotiate, then kick in the empty-auth magic
automatically.

  - if empty-auth failed (or if we decided not to try it), ask for a
credential and retry the request. Either way, tell curl that we want
to use "Basic" so it doesn't have to do the probe request (and
obviously if the server did not support Basic, then fail
immediately).

I think that would keep it to 2 round-trips for the normal "Basic" case,
as well as for the GSSNegotiate case. It would be 3 requests when the
server offers GSSNegotiate but you can't use it (but you could set
http.emptyauth=false to optimize that out).

That's all theoretical, though. I might not even be right about the
reason for the second request, and I certainly haven't written any code
(nor do I have a GSSNegotiate setup to test against).

-Peff

Re: [PATCH] Documentation: Link git-ls-files to core.quotePath variable.

[Andreas Heiduk]

@Phillip: Thanks.

@Junio: Don't bother, I'm about to fix other man-pages with the text
from ls-files. I will include that typo-fix and prepare a new patch
based on maint.


Am 22.02.2017 um 22:02 schrieb Junio C Hamano:
> "Philip Oakley"  writes:
>> s/option pathnamens/option, pathnames/# comma and spelling.
> 
> Thanks.  Will squash it in while queuing (I need to discard the new
> text added in the previous attempt in the preimage to make it apply,
> too).
>

RE: [PATCH] http(s): automatically try NTLM authentication first

[David Turner]

> -Original Message-
> From: Junio C Hamano [mailto:jch2...@gmail.com] On Behalf Of Junio C
> Hamano
> Sent: Wednesday, February 22, 2017 3:20 PM
> To: David Turner 
> Cc: git@vger.kernel.org; sand...@crustytoothpaste.net; Johannes Schindelin
> ; Eric Sunshine
> ; Jeff King 
> Subject: Re: [PATCH] http(s): automatically try NTLM authentication first
> 
> David Turner  writes:
> 
> > From: Johannes Schindelin 
> >
> > It is common in corporate setups to have permissions managed via a
> > domain account. That means that the user does not really have to log
> > in when accessing a central repository via https://, but that the
> > login credentials are used to authenticate with that repository.
> >
> > The common way to do that used to require empty credentials, i.e.
> > hitting Enter twice when being asked for user name and password, or by
> > using the very funny notation https://:@server/repository
> >
> > A recent commit (5275c3081c (http: http.emptyauth should allow empty
> > (not just NULL) usernames, 2016-10-04)) broke that usage, though, all
> > of a sudden requiring users to set http.emptyAuth = true.
> >
> > Which brings us to the bigger question why http.emptyAuth defaults to
> > false, to begin with.
> 
> This is a valid question, and and I do not see it explicitly asked in the 
> thread:
> 
> https://public-
> inbox.org/git/CAPig+cSphEu3iRJrkdBA+BRhi9HnopLJnKOHVuGhUqavtV1RXg
> @mail.gmail.com/#t
> 
> even though there is a hint of it already there.
> 
> > It would be one thing if cURL would not let the user specify
> > credentials interactively after attempting NTLM authentication (i.e.
> > login credentials), but that is not the case.
> >
> > It would be another thing if attempting NTLM authentication was not
> > usually what users need to do when trying to authenticate via https://.
> > But that is also not the case.
> 
> Some other possible worries we may have had I can think of are:
> 
>  - With this enabled unconditionally, would we leak some information?

I think "NTLM" is actually a misnomer here (I just copied Johannes's 
commit message). The mechanism is actually SPNEGO, if I understand this 
correctly. It seems to me that this is probably secure, since it is apparently
widely implemented in browsers.

>  - With this enabled unconditionally, would we always incur an extra
>roundtrip for people who are not running NTLM at all?
>
> I do not think the former is the case, but what would I know (adding a few
> people involved in the original thread to CC: ;-)

Always, no.  For failed authentication (or authorization), apparently, yes.  
I tested this by  setting the variable to false and then true, and trying to 
Push to a github repository which I didn't have write access to, with 
both an empty username (https://@:github.com/...) and no username 
(http://github.com/...).   I ran this under GIT_CURL_VERBOSE=1 and
I saw two 401 responses in the "http.emptyauth=true" case and one
in the false case.  I also tried with a repo that I did have access to (first
configuring the necessary tokens for HTTPS push access), and saw two
401 responses in *both* cases.  

Re: [PATCH v5 00/24] Remove submodule from files-backend.c

[Junio C Hamano]

Junio C Hamano  writes:

> Nguyễn Thái Ngọc Duy   writes:
>
>> v5 goes a bit longer than v4, basically:
>>
>>  - files_path() is broken down into three smaller functions,
>>files_{packed_refs,reflog,refname}_path().
>>
>>  - most of store-based api is added because..
>>
>>  - test-ref-store.c is added with t1405 and t1406 for some basic tests
>>I'm not aimimg for complete ref store coverage. But we can continue
>>to improve from there.
>>
>>  - refs_store_init() now takes a "permission" flag, like open().
>>Operations are allowed or forbidden based on this flag. The
>>submodule_allowed flag is killed. files_assert_main.. remains.
>>
>>  - get_*_ref_store() remain public api because it's used by
>>test-ref-store.c and pack-refs.c.
>>
>>  - files-backend.c should now make no function calls that implicitly
>>target the main store. But this will have to be tested more to be
>>sure. I'm tempted to add a tracing backend just for this purpose.
>
> OK.
>
>> Junio, if you take this on 'pu', you'll have to kick my other two
>> series out (they should not even compile). I'm not resending them
>> until I get a "looks mostly ok" from Michael. No point in updating
>> them when this series keeps moving.
>
> Thanks for a note.

As this round seems to have added conflicts with another topic that
is already in flight that the previous round did not conflict with,
I'll eject all three for now until this one solidifies a bit more.

Re: [PATCH] Documentation: Link git-ls-files to core.quotePath variable.

[Junio C Hamano]

"Philip Oakley"  writes:

>> -When `-z` option is not used, TAB, LF, and backslash characters
>> -in pathnames are represented as `\t`, `\n`, and `\\`,
>> -respectively. The path is also quoted according to the
>> -configuration variable `core.quotePath` (see linkgit:git-config[1]).
>> +Without the `-z` option pathnamens with "unusual" characters are
>
> s/option pathnamens/option, pathnames/# comma and spelling.

Thanks.  Will squash it in while queuing (I need to discard the new
text added in the previous attempt in the preimage to make it apply,
too).

-- >8 --
From: Andreas Heiduk 
Date: Wed, 22 Feb 2017 02:38:21 +0100
Subject: [PATCH] Documentation: clarify core.quotePath and update git-ls-files 
doc

Signed-off-by: Andreas Heiduk 
Signed-off-by: Junio C Hamano 
---
 Documentation/config.txt   | 22 --
 Documentation/git-ls-files.txt | 10 ++
 2 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f4721a048b..25e65aeb01 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -340,16 +340,18 @@ core.checkStat::
all fields, including the sub-second part of mtime and ctime.
 
 core.quotePath::
-   The commands that output paths (e.g. 'ls-files',
-   'diff'), when not given the `-z` option, will quote
-   "unusual" characters in the pathname by enclosing the
-   pathname in a double-quote pair and with backslashes the
-   same way strings in C source code are quoted.  If this
-   variable is set to false, the bytes higher than 0x80 are
-   not quoted but output as verbatim.  Note that double
-   quote, backslash and control characters are always
-   quoted without `-z` regardless of the setting of this
-   variable.
+   Commands that output paths (e.g. 'ls-files', 'diff'), will
+   quote "unusual" characters in the pathname by enclosing the
+   pathname in double-quotes and escaping those characters with
+   backslashes in the same way C escapes control characters (e.g.
+   `\t` for TAB, `\n` for LF, `\\` for backslash) or bytes with
+   values larger than 0x80 (e.g. octal `\265` for "micro").  If
+   this variable is set to false, bytes higher than 0x80 are not
+   considered "unusual" any more.  Double-quotes, backslash and
+   control characters are always escaped regardless of the
+   setting of this variable.  Many commands can output pathnames
+   completely verbatim using the `-z` option. The default value is
+   true.
 
 core.eol::
Sets the line ending type to use in the working directory for
diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 078b556665..a415223b45 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -76,7 +76,8 @@ OPTIONS
succeed.
 
 -z::
-   \0 line termination on output.
+   \0 line termination on output and do not quote filenames.
+   See OUTPUT below for more information.
 
 -x ::
 --exclude=::
@@ -192,9 +193,10 @@ the index records up to three such pairs; one from tree O 
in stage
 the user (or the porcelain) to see what should eventually be recorded at the
 path. (see linkgit:git-read-tree[1] for more information on state)
 
-When `-z` option is not used, TAB, LF, and backslash characters
-in pathnames are represented as `\t`, `\n`, and `\\`,
-respectively.
+Without the `-z` option, pathnames with "unusual" characters are
+quoted as explained for the configuration variable `core.quotePath`
+(see linkgit:git-config[1]).  Using `-z` the filename is output
+verbatim and the line is terminated by a NUL byte.
 
 
 Exclude Patterns
-- 
2.12.0-rc2-250-gd33575c7f2

Re: [PATCH] git add -i: replace \t with blanks in the help message

[Junio C Hamano]

Ralf Thielow  writes:

> Within the help message of 'git add -i', the 'diff' command uses one
> tab character and blanks to create the space between the name and the
> description while the others use blanks only.  So if the tab size is
> not at 4 characters, this description will not be in range.
> Replace the tab character with blanks.
>
> Signed-off-by: Ralf Thielow 
> ---
>  git-add--interactive.perl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, this was me being sloppy in the very first version.

Will queue.

>
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index cf6fc926a..982593c89 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1678,7 +1678,7 @@ status- show paths with changes
>  update- add working tree state to the staged set of changes
>  revert- revert staged set of changes back to the HEAD version
>  patch - pick hunks and update selectively
> -diff   - view diff between HEAD and index
> +diff  - view diff between HEAD and index
>  add untracked - add contents of untracked files to the staged set of changes
>  EOF
>  }

Re: [PATCH] submodule init: warn about falling back to a local path

[Philip Oakley]

From: "Stefan Beller" 
When a submodule is initialized, the config variable 
'submodule..url'

is set depending on the value of the same variable in the .gitmodules
file. When the URL indicates to be relative, then the url is computed
relative to its default remote. The default remote cannot be determined
accurately in all cases, such that it falls back to 'origin'.

The 'origin' remote may not exist, though. In that case we give up looking
for a suitable remote and we'll just assume it to be a local relative 
path.


This can be confusing to users as there is a lot of guessing involved,
which is not obvious to the user.


Would a note in the user docs about the fallback you list above also be 
useful?




So in the corner case of assuming a local autoritative truth, warn the
user to lessen the confusion.

This behavior was introduced in 4d6893200 (submodule add: allow relative
repository path even when no url is set, 2011-06-06), which shared the
code with submodule-init and then ported to C in 3604242f080a (submodule:
port init from shell to C, 2016-04-15).

In case of submodule-add, this behavior makes sense in some use cases[1],
however for submodule-init there does not seem to be an immediate obvious
use case to fall back to a local submodule. However there might be, so
warn instead of die here.

[1] e.g. 
http://stackoverflow.com/questions/8721984/git-ignore-files-for-public-repository-but-not-for-private

"store a secret locally in a submodule, with no intention to publish it"

Reported-by: Shawn Pearce 
Signed-off-by: Stefan Beller 
---
builtin/submodule--helper.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)


--
Philip 

Re: [PATCH] Documentation: Link git-ls-files to core.quotePath variable.

[Philip Oakley]

From: "Andreas Heiduk" 

[PATCH] Documentation: Clarify core.quotePath, remove cruft in
git-ls-files.

Signed-off-by: Andreas Heiduk 
---

I have merged the best parts about quoting into the core.quotePath
description and cleaned up the text in git-ls-files.txt regarding the
control characters.


Documentation/config.txt   | 22 --
Documentation/git-ls-files.txt | 11 ++-
2 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f4721a0..25e65ae 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -340,16 +340,18 @@ core.checkStat::
 all fields, including the sub-second part of mtime and ctime.

core.quotePath::
- The commands that output paths (e.g. 'ls-files',
- 'diff'), when not given the `-z` option, will quote
- "unusual" characters in the pathname by enclosing the
- pathname in a double-quote pair and with backslashes the
- same way strings in C source code are quoted.  If this
- variable is set to false, the bytes higher than 0x80 are
- not quoted but output as verbatim.  Note that double
- quote, backslash and control characters are always
- quoted without `-z` regardless of the setting of this
- variable.
+ Commands that output paths (e.g. 'ls-files', 'diff'), will
+ quote "unusual" characters in the pathname by enclosing the
+ pathname in double-quotes and escaping those characters with
+ backslashes in the same way C escapes control characters (e.g.
+ `\t` for TAB, `\n` for LF, `\\` for backslash) or bytes with
+ values larger than 0x80 (e.g. octal `\265` for "micro").  If
+ this variable is set to false, bytes higher than 0x80 are not
+ considered "unusual" any more.  Double-quotes, backslash and
+ control characters are always escaped regardless of the
+ setting of this variable.  Many commands can output pathnames
+ completely verbatim using the `-z` option. The default value is
+ true.

core.eol::
 Sets the line ending type to use in the working directory for
diff --git a/Documentation/git-ls-files.txt 
b/Documentation/git-ls-files.txt

index d2b17f2..88df561 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -76,7 +76,8 @@ OPTIONS
 succeed.

-z::
- \0 line termination on output.
+ \0 line termination on output and do not quote filenames.
+ See OUTPUT below for more information.

-x ::
--exclude=::
@@ -192,10 +193,10 @@ the index records up to three such pairs; one from
tree O in stage
the user (or the porcelain) to see what should eventually be recorded
at the
path. (see linkgit:git-read-tree[1] for more information on state)

-When `-z` option is not used, TAB, LF, and backslash characters
-in pathnames are represented as `\t`, `\n`, and `\\`,
-respectively. The path is also quoted according to the
-configuration variable `core.quotePath` (see linkgit:git-config[1]).
+Without the `-z` option pathnamens with "unusual" characters are


s/option pathnamens/option, pathnames/# comma and spelling.


+quoted as explained for the configuration variable `core.quotePath`
+(see linkgit:git-config[1]).  Using `-z` the filename is output
+verbatim and the line is terminated by a NUL byte.


--
Philip 

Re: [PATCH] http(s): automatically try NTLM authentication first

[Junio C Hamano]

David Turner  writes:

> From: Johannes Schindelin 
>
> It is common in corporate setups to have permissions managed via a
> domain account. That means that the user does not really have to log in
> when accessing a central repository via https://, but that the login
> credentials are used to authenticate with that repository.
>
> The common way to do that used to require empty credentials, i.e. hitting
> Enter twice when being asked for user name and password, or by using the
> very funny notation https://:@server/repository
>
> A recent commit (5275c3081c (http: http.emptyauth should allow empty (not
> just NULL) usernames, 2016-10-04)) broke that usage, though, all of a
> sudden requiring users to set http.emptyAuth = true.
>
> Which brings us to the bigger question why http.emptyAuth defaults to
> false, to begin with.

This is a valid question, and and I do not see it explicitly asked
in the thread:

https://public-inbox.org/git/capig+cspheu3irjrkdba+brhi9hnopljnkohvughuqavtv1...@mail.gmail.com/#t

even though there is a hint of it already there.

> It would be one thing if cURL would not let the user specify credentials
> interactively after attempting NTLM authentication (i.e. login
> credentials), but that is not the case.
>
> It would be another thing if attempting NTLM authentication was not
> usually what users need to do when trying to authenticate via https://.
> But that is also not the case.

Some other possible worries we may have had I can think of are:

 - With this enabled unconditionally, would we leak some information?

 - With this enabled unconditionally, would we always incur an extra
   roundtrip for people who are not running NTLM at all?

I do not think the former is the case, but what would I know (adding a
few people involved in the original thread to CC: ;-)

>  Documentation/config.txt | 3 ++-
>  http.c   | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index fc5a28a320..b0da64ed33 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1742,7 +1742,8 @@ http.emptyAuth::
>   Attempt authentication without seeking a username or password.  This
>   can be used to attempt GSS-Negotiate authentication without specifying
>   a username in the URL, as libcurl normally requires a username for
> - authentication.
> + authentication.  Default is true, since if this fails, git will fall
> + back to asking the user for their username/password.
>  
>  http.delegation::
>   Control GSSAPI credential delegation. The delegation is disabled
> diff --git a/http.c b/http.c
> index 90a1c0f113..943e630ea6 100644
> --- a/http.c
> +++ b/http.c
> @@ -109,7 +109,7 @@ static int curl_save_cookies;
>  struct credential http_auth = CREDENTIAL_INIT;
>  static int http_proactive_auth;
>  static const char *user_agent;
> -static int curl_empty_auth;
> +static int curl_empty_auth = 1;
>  
>  enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;

Re: [PATCH v5 03/19] builtin/diff-tree: convert to struct object_id

[Junio C Hamano]

Jeff King  writes:

> On Wed, Feb 22, 2017 at 10:50:21AM -0800, Junio C Hamano wrote:
>
>> What the function does appears somewhat iffy in the modern world.
>> We are relying on the fact that while Git is operating in this mode
>> of reading a tuple of commits per line and doing log-tree, that Git
>> itself will not do the history traversal (instead, another instance
>> of Git that feeds us via our standard input is walking the history)
>> for this piece of code to work correctly.  
>> 
>> Of course, the "diff-tree --stdin" command was meant to sit on the
>> downstream of "rev-list --parents", so the assumption the code makes
>> (i.e. the parents field of the in-core commit objects do not have to
>> be usable for history traversal) may be reasonable, but still...
>
> I'm not sure it's that weird. "diff-tree" is about diffing, not
> traversal. The only reason it touches commit->parents at all (and
> doesn't just kick off a diff between the arguments it gets) is that it's
> been stuck with pretty-printing the commits, which might ask to show the
> parents.

Yeah, I understand all that as 45392a648d ("git-diff-tree --stdin:
show all parents.", 2006-02-05) was mostly mine.  It's just I sense
that the recent trend is to take whatever existing code and see if
they are reusable in other contexts, and this is one of the things
that people might want to libify, but cannot be as-is.

Re: [PATCH v5 03/19] builtin/diff-tree: convert to struct object_id

[Jeff King]

On Wed, Feb 22, 2017 at 10:50:21AM -0800, Junio C Hamano wrote:

> "brian m. carlson"  writes:
> 
> > Convert most leaf functions to struct object_id.  Change several
> > hardcoded numbers to uses of parse_oid_hex.  In doing so, verify that we
> > when we want two trees, we have exactly two trees.
> >
> > Finally, in stdin_diff_commit, avoid accessing the byte after the NUL.
> > This will be a NUL as well, since the first NUL was a newline we
> > overwrote.  However, with parse_oid_hex, we no longer need to increment
> > the pointer directly, and can simply increment it as part of our check
> > for the space character.
> 
> After reading the pre- and post-image twice, I think I convinced
> myself that this is a faithful conersion and they do the same thing.

I think this is correct, too (but then, I think it largely comes from
the patch I wrote the other night. So I did look at it carefully, but
it's not exactly an independent review).

> What the function does appears somewhat iffy in the modern world.
> We are relying on the fact that while Git is operating in this mode
> of reading a tuple of commits per line and doing log-tree, that Git
> itself will not do the history traversal (instead, another instance
> of Git that feeds us via our standard input is walking the history)
> for this piece of code to work correctly.  
> 
> Of course, the "diff-tree --stdin" command was meant to sit on the
> downstream of "rev-list --parents", so the assumption the code makes
> (i.e. the parents field of the in-core commit objects do not have to
> be usable for history traversal) may be reasonable, but still...

I'm not sure it's that weird. "diff-tree" is about diffing, not
traversal. The only reason it touches commit->parents at all (and
doesn't just kick off a diff between the arguments it gets) is that it's
been stuck with pretty-printing the commits, which might ask to show the
parents.

-Peff

Re: url..insteadOf vs. submodules

[Stefan Beller]

On Wed, Feb 22, 2017 at 10:57 AM, Jeff King  wrote:
> On Wed, Feb 22, 2017 at 09:36:12AM -0800, Junio C Hamano wrote:
>
>> >> My gut feeling is that we should do the selective/filtered include
>> >> Peff mentioned when a repository is known to be used as a submodule
>> >> of somebody else.
>> >
>> > Does the management of these submodue-related config values
>> > become easier if, instead of placing them in .config, we
>> > place them in a git/.context file?
>>
>> Do you mean that Git users that use submodules adopt a convention
>> where a separate file in $GIT_DIR of the toplevel superproject holds
>> pieces of configuration that are meant to be shared between the
>> superproject and across all its submodules, and the $GIT_DIR/config
>> file in submodules and the superproject all include that shared one
>> via include.path mechanism?
>>
>> That may allow us to do without being responsible for sifting of
>> configuration variables into safe and unsafe bins.
>>
>> I dunno.
>
> Hmm. I certainly like that we punt on having to decide on the "should
> this be shared with submodules" decision. That makes the end result more
> flexible, and we don't have to get into a never-ending stream of
> "whitelist this config option" patches.
>
> My only concern is that it's not as discoverable. In the situation that
> kicked off this thread, somebody put url.X.insteadOf into their
> super-project .git/config, expecting it to work in the submodules. That
> _still_ wouldn't work with this proposal. They'd have to:
>
>   1. Put it in .git/context (or whatever we call it)
>
>   2. Maybe add include.path=context in .git/config if they want the
>  config shared with the super-project (or this could be automatic?)
>
> I guess it gives _a_ solution, which is more than we have now, but it
> doesn't feel very ergonomic.

Well, currently ".git/config" is the one and only blessed way to configure
a single repo and our documentation and user expectations reflect that.
Once git-worktree takes off (which has per working tree configuration files)
it doesn't feel as obscure anymore to have multiple config files.

The working trees will share the $GIT_COMMON_DIR/config file for
all working trees and have its own config file at $GIT_DIR/config.worktree
in its respective git directories. C.f.
https://public-inbox.org/git/20170110112524.12870-2-pclo...@gmail.com/

So I could imagine that we just introduce another config file
config.submodules which is source'd by the submodules.
Then the hard part becomes to decide which config value to put
in which config file. (We'd still be left to guess where to put some initial
new configuration value. config or config.submodules. Any update of a
value can just stay in its respective file. And I don't think we'd want
to invent a config option that tells us which policy we use where to
put config options. That sounds just scary.)

Re: url..insteadOf vs. submodules

[Jeff King]

On Wed, Feb 22, 2017 at 09:36:12AM -0800, Junio C Hamano wrote:

> >> My gut feeling is that we should do the selective/filtered include
> >> Peff mentioned when a repository is known to be used as a submodule
> >> of somebody else.
> >
> > Does the management of these submodue-related config values
> > become easier if, instead of placing them in .config, we
> > place them in a git/.context file?
> 
> Do you mean that Git users that use submodules adopt a convention
> where a separate file in $GIT_DIR of the toplevel superproject holds
> pieces of configuration that are meant to be shared between the
> superproject and across all its submodules, and the $GIT_DIR/config
> file in submodules and the superproject all include that shared one
> via include.path mechanism?
> 
> That may allow us to do without being responsible for sifting of
> configuration variables into safe and unsafe bins.
> 
> I dunno.

Hmm. I certainly like that we punt on having to decide on the "should
this be shared with submodules" decision. That makes the end result more
flexible, and we don't have to get into a never-ending stream of
"whitelist this config option" patches.

My only concern is that it's not as discoverable. In the situation that
kicked off this thread, somebody put url.X.insteadOf into their
super-project .git/config, expecting it to work in the submodules. That
_still_ wouldn't work with this proposal. They'd have to:

  1. Put it in .git/context (or whatever we call it)

  2. Maybe add include.path=context in .git/config if they want the
 config shared with the super-project (or this could be automatic?)

I guess it gives _a_ solution, which is more than we have now, but it
doesn't feel very ergonomic.

-Peff

Re: [PATCH v5 03/19] builtin/diff-tree: convert to struct object_id

[Junio C Hamano]

"brian m. carlson"  writes:

> Convert most leaf functions to struct object_id.  Change several
> hardcoded numbers to uses of parse_oid_hex.  In doing so, verify that we
> when we want two trees, we have exactly two trees.
>
> Finally, in stdin_diff_commit, avoid accessing the byte after the NUL.
> This will be a NUL as well, since the first NUL was a newline we
> overwrote.  However, with parse_oid_hex, we no longer need to increment
> the pointer directly, and can simply increment it as part of our check
> for the space character.

After reading the pre- and post-image twice, I think I convinced
myself that this is a faithful conersion and they do the same thing.

What the function does appears somewhat iffy in the modern world.
We are relying on the fact that while Git is operating in this mode
of reading a tuple of commits per line and doing log-tree, that Git
itself will not do the history traversal (instead, another instance
of Git that feeds us via our standard input is walking the history)
for this piece of code to work correctly.  

Of course, the "diff-tree --stdin" command was meant to sit on the
downstream of "rev-list --parents", so the assumption the code makes
(i.e. the parents field of the in-core commit objects do not have to
be usable for history traversal) may be reasonable, but still...

[PATCH] git add -i: replace \t with blanks in the help message

[Ralf Thielow]

Within the help message of 'git add -i', the 'diff' command uses one
tab character and blanks to create the space between the name and the
description while the others use blanks only.  So if the tab size is
not at 4 characters, this description will not be in range.
Replace the tab character with blanks.

Signed-off-by: Ralf Thielow 
---
 git-add--interactive.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index cf6fc926a..982593c89 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1678,7 +1678,7 @@ status- show paths with changes
 update- add working tree state to the staged set of changes
 revert- revert staged set of changes back to the HEAD version
 patch - pick hunks and update selectively
-diff - view diff between HEAD and index
+diff  - view diff between HEAD and index
 add untracked - add contents of untracked files to the staged set of changes
 EOF
 }
-- 
2.12.0.rc2.424.g63d3652c7

Re: [PATCH 03/15] lib-submodule-update.sh: define tests for recursing into submodules

[Stefan Beller]

On Thu, Feb 16, 2017 at 12:39 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Currently lib-submodule-update.sh provides 2 functions
>> test_submodule_switch and test_submodule_forced_switch that are used by a
>> variety of tests to ensure that submodules behave as expected. The current
>> expected behavior is that submodules are not touched at all (see
>> 42639d2317a for the exact setup).
>>
>> In the future we want to teach all these commands to properly recurse
>> into submodules. To do that, we'll add two testing functions to
>> submodule-update-lib.sh test_submodule_switch_recursing and
>> test_submodule_forced_switch_recursing.
>
> I'd remove "properly" and insert a colon after "add two ... to
> submodule-update-lib.sh" before the names of two functions.

ok

>
>> +reset_work_tree_to_interested () {
>> + reset_work_tree_to $1 &&
>> + # indicate we are interested in the submodule:
>> + git -C submodule_update config submodule.sub1.url "bogus" &&
>> + # also have it available:
>> + if ! test -d submodule_update/.git/modules/sub1
>> + then
>> + mkdir submodule_update/.git/modules &&
>
> Would we want "mkdir -p" here to be safe?

Yes I cannot think of a downside of being overly cautious here.

>
>> + cp -r submodule_update_repo/.git/modules/sub1 
>> submodule_update/.git/modules/sub1
>
> ... ahh, wouldn't matter that much, we checked that modules/sub1
> does not exist, and as long as nobody creates modules/ or 
> modules/somethingelse
> we are OK.

Well, I'll add the -p

>> +# Test that submodule contents are correctly updated when switching
>> +# between commits that change a submodule.
>> +# Test that the following transitions are correctly handled:
>> +# (These tests are also above in the case where we expect no change
>> +#  in the submodule)
>> +# - Updated submodule
>> +# - New submodule
>> +# - Removed submodule
>> +# - Directory containing tracked files replaced by submodule
>> +# - Submodule replaced by tracked files in directory
>> +# - Submodule replaced by tracked file with the same name
>> +# - tracked file replaced by submodule
>
> These should work without trouble only when the paths involved in
> the operation in the working tree are clean, right?  Just double
> checking.  If they are dirty we should expect a failure, instead of
> silent loss of information.

yes, I'll go over the tests again and add those cases if missing.


>> + command="$1"
>
> The dq-pair is not strictly needed on the RHS of the assignment, but
> it is a good way to signal that we considered that we might receive
> an argument with $IFS in it...
>
>> + $command add_sub1 &&
>
> ... and after doing so, not quoting $command here signals that we
> expect command line splitting to happen.  Am I reading it correctly?
> Without an actual caller appearing in this step, it is rather hard
> to judge.
>

I followed the existing code without thinking about these points, but they are
valid and exactly how we'd expect the code to behave.
$1 / $command will be e.g. "git checkout --recurse-submodules" in
this patch series; but later on we could also have functions.
C.f.  t4137 which defines a function

apply_3way () {
git diff --ignore-submodules=dirty "..$1" | git apply --3way -
}
test_submodule_switch "apply_3way"

We'd want to have a similar thing for the recursing part, e.g.

apply_3way_recursing () {
git diff --submodule=diff "..$1" | git apply
--recurse-submodules --3way -
}
test_submodule_switch_recursing "apply_3way_recursing"

>> + echo sub1 > .git/info/exclude
>
> ">.git/info/exclude"

ok

Re: [PATCH] Documentation: use brackets for optional arguments

[Junio C Hamano]

"brian m. carlson"  writes:

> The documentation for git blame used vertical bars for optional
> arguments to -M and -C, which is unusual and potentially confusing.
> Since most man pages use brackets for optional items, and that's
> consistent with how we document the same options for git diff and
> friends, use brackets here, too.

This seems to date back to April 2007 X-<.

Thanks.

>
> Signed-off-by: brian m. carlson 
> ---
>  Documentation/blame-options.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
> index 2669b87c9d..dc41957afa 100644
> --- a/Documentation/blame-options.txt
> +++ b/Documentation/blame-options.txt
> @@ -77,7 +77,7 @@ include::line-range-format.txt[]
>   terminal. Can't use `--progress` together with `--porcelain`
>   or `--incremental`.
>  
> --M||::
> +-M[]::
>   Detect moved or copied lines within a file. When a commit
>   moves or copies a block of lines (e.g. the original file
>   has A and then B, and the commit changes it to B and then
> @@ -93,7 +93,7 @@ alphanumeric characters that Git must detect as 
> moving/copying
>  within a file for it to associate those lines with the parent
>  commit. The default value is 20.
>  
> --C||::
> +-C[]::
>   In addition to `-M`, detect lines moved or copied from other
>   files that were modified in the same commit.  This is
>   useful when you reorganize your program and move code

Re: [PATCH v5 01/24] refs.h: add forward declaration for structs used in this file

[Stefan Beller]

On Wed, Feb 22, 2017 at 6:04 AM, Nguyễn Thái Ngọc Duy  wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  refs.h | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/refs.h b/refs.h
> index 9fbff90e7..c494b641a 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -1,6 +1,11 @@
>  #ifndef REFS_H
>  #define REFS_H
>
> +struct object_id;
> +struct ref_transaction;
> +struct strbuf;
> +struct string_list;
> +
>  /*
>   * Resolve a reference, recursively following symbolic refererences.
>   *
> @@ -144,7 +149,6 @@ int dwim_log(const char *str, int len, unsigned char 
> *sha1, char **ref);
>   * `ref_transaction_commit` is called.  So `ref_transaction_verify`
>   * won't report a verification failure until the commit is attempted.
>   */
> -struct ref_transaction;

Leaving the detailed comment about ref_transaction dangling?
I can understand if you don't want to move it with the declaration,
as you want all declarations terse in a few lines.
Maybe move the comment to be part of the first large comment
(The one that you can see in the first hunk, starting with
" * Resolve a reference, recursively following")

Maybe Michael has a better idea how to make this comment
more accessible to the casual refs-reader.

Thanks,
Stefan

Re: [PATCH] Documentation: correctly spell git worktree --detach

[Junio C Hamano]

"brian m. carlson"  writes:

> The option is “--detach”, but we accidentally spelled it “--detached” at
> one point in the man page.
>
> Signed-off-by: brian m. carlson 
> Reported-by: Casey Rodarmor 
> ---

Thanks, both.

>  Documentation/git-worktree.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index e257c19ebe..553cf8413f 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -52,7 +52,7 @@ is linked to the current repository, sharing everything 
> except working
>  directory specific files such as HEAD, index, etc. `-` may also be
>  specified as ``; it is synonymous with `@{-1}`.
>  +
> -If `` is omitted and neither `-b` nor `-B` nor `--detached` used,
> +If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
>  then, as a convenience, a new branch based at HEAD is created automatically,
>  as if `-b $(basename )` was specified.
>  

Re: url..insteadOf vs. submodules

[Junio C Hamano]

Jon Loeliger  writes:

> So, like, Junio C Hamano said:
>> Stefan Beller  writes:
>> 
>> > Do we want to invent a special value for url.*.insteadOf to mean
>> >   "look up in superproject, so I don't have to keep
>> >   a copy that may get stale" ?
>> 
>> My gut feeling is that we should do the selective/filtered include
>> Peff mentioned when a repository is known to be used as a submodule
>> of somebody else.
>
> Does the management of these submodue-related config values
> become easier if, instead of placing them in .config, we
> place them in a git/.context file?

Do you mean that Git users that use submodules adopt a convention
where a separate file in $GIT_DIR of the toplevel superproject holds
pieces of configuration that are meant to be shared between the
superproject and across all its submodules, and the $GIT_DIR/config
file in submodules and the superproject all include that shared one
via include.path mechanism?

That may allow us to do without being responsible for sifting of
configuration variables into safe and unsafe bins.

I dunno.

Re: [PATCH] Documentation: Link git-ls-files to core.quotePath variable.

[Junio C Hamano]

Andreas Heiduk  writes:

> [PATCH] Documentation: Clarify core.quotePath, remove cruft in
>  git-ls-files.
>
> Signed-off-by: Andreas Heiduk 
> ---
>
> I have merged the best parts about quoting into the core.quotePath
> description and cleaned up the text in git-ls-files.txt regarding the
> control characters.
>
>
>  Documentation/config.txt   | 22 --
>  Documentation/git-ls-files.txt | 11 ++-
>  2 files changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index f4721a0..25e65ae 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -340,16 +340,18 @@ core.checkStat::
>   all fields, including the sub-second part of mtime and ctime.
>
>  core.quotePath::
> - The commands that output paths (e.g. 'ls-files',
> - 'diff'), when not given the `-z` option, will quote
> - "unusual" characters in the pathname by enclosing the
> - pathname in a double-quote pair and with backslashes the
> - same way strings in C source code are quoted.  If this
> - variable is set to false, the bytes higher than 0x80 are
> - not quoted but output as verbatim.  Note that double
> - quote, backslash and control characters are always
> - quoted without `-z` regardless of the setting of this
> - variable.
> + Commands that output paths (e.g. 'ls-files', 'diff'), will
> + quote "unusual" characters in the pathname by enclosing the
> + pathname in double-quotes and escaping those characters with
> + backslashes in the same way C escapes control characters (e.g.
> + `\t` for TAB, `\n` for LF, `\\` for backslash) or bytes with
> + values larger than 0x80 (e.g. octal `\265` for "micro").  If
> + this variable is set to false, bytes higher than 0x80 are not
> + considered "unusual" any more.  Double-quotes, backslash and
> + control characters are always escaped regardless of the
> + setting of this variable.  Many commands can output pathnames
> + completely verbatim using the `-z` option. The default value is
> + true.

Even though I am not sure "\265 is micro" is a good example these
days, as "high-bit set" is primarily meant to catch UTF-8
multi-bytes, I find the above much easier to read than the original.

> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> index d2b17f2..88df561 100644
> --- a/Documentation/git-ls-files.txt
> +++ b/Documentation/git-ls-files.txt
> @@ -76,7 +76,8 @@ OPTIONS
>   succeed.
>
>  -z::
> - \0 line termination on output.
> + \0 line termination on output and do not quote filenames.
> + See OUTPUT below for more information.
>
>  -x ::
>  --exclude=::
> @@ -192,10 +193,10 @@ the index records up to three such pairs; one from
> tree O in stage
>  the user (or the porcelain) to see what should eventually be recorded
> at the
>  path. (see linkgit:git-read-tree[1] for more information on state)
>
> -When `-z` option is not used, TAB, LF, and backslash characters
> -in pathnames are represented as `\t`, `\n`, and `\\`,
> -respectively. The path is also quoted according to the
> -configuration variable `core.quotePath` (see linkgit:git-config[1]).
> +Without the `-z` option pathnamens with "unusual" characters are
> +quoted as explained for the configuration variable `core.quotePath`
> +(see linkgit:git-config[1]).  Using `-z` the filename is output
> +verbatim and the line is terminated by a NUL byte.

Yup, this looks much nicer than the original.

Thanks.

[PATCH] http(s): automatically try NTLM authentication first

[David Turner]

From: Johannes Schindelin 

It is common in corporate setups to have permissions managed via a
domain account. That means that the user does not really have to log in
when accessing a central repository via https://, but that the login
credentials are used to authenticate with that repository.

The common way to do that used to require empty credentials, i.e. hitting
Enter twice when being asked for user name and password, or by using the
very funny notation https://:@server/repository

A recent commit (5275c3081c (http: http.emptyauth should allow empty (not
just NULL) usernames, 2016-10-04)) broke that usage, though, all of a
sudden requiring users to set http.emptyAuth = true.

Which brings us to the bigger question why http.emptyAuth defaults to
false, to begin with.

It would be one thing if cURL would not let the user specify credentials
interactively after attempting NTLM authentication (i.e. login
credentials), but that is not the case.

It would be another thing if attempting NTLM authentication was not
usually what users need to do when trying to authenticate via https://.
But that is also not the case.

So let's just go ahead and change the default, and unbreak the NTLM
authentication. As a bonus, this also makes the "you need to hit Enter
twice" (which is hard to explain: why enter empty credentials when you
want to authenticate with your login credentials?) and the ":@" hack
(which is also pretty, pretty hard to explain to users) obsolete.

This fixes https://github.com/git-for-windows/git/issues/987

Signed-off-by: Johannes Schindelin 
Signed-off-by: David Turner 
---
This has been in git for Windows for a few months (without the
config.txt change).  We've also been using it internally.  So I think
it's time to merge back to upstream git.

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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index fc5a28a320..b0da64ed33 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1742,7 +1742,8 @@ http.emptyAuth::
Attempt authentication without seeking a username or password.  This
can be used to attempt GSS-Negotiate authentication without specifying
a username in the URL, as libcurl normally requires a username for
-   authentication.
+   authentication.  Default is true, since if this fails, git will fall
+   back to asking the user for their username/password.
 
 http.delegation::
Control GSSAPI credential delegation. The delegation is disabled
diff --git a/http.c b/http.c
index 90a1c0f113..943e630ea6 100644
--- a/http.c
+++ b/http.c
@@ -109,7 +109,7 @@ static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
 static int http_proactive_auth;
 static const char *user_agent;
-static int curl_empty_auth;
+static int curl_empty_auth = 1;
 
 enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
 
-- 
2.11.GIT

[PATCH v2 3/3] fetch-pack: add specific error for fetching an unadvertised object

[Matt McCutchen]

Enhance filter_refs (which decides whether a request for an unadvertised
object should be sent to the server) to record a new match status on the
"struct ref" when a request is not allowed, and have
report_unmatched_refs check for this status and print a special error
message, "Server does not allow request for unadvertised object".

Signed-off-by: Matt McCutchen 
---
 fetch-pack.c  | 42 +++---
 remote.h  |  9 +++--
 t/t5516-fetch-push.sh |  2 +-
 3 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 7c8d44c..f12bfcd 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -578,7 +578,7 @@ static void filter_refs(struct fetch_pack_args *args,
break; /* definitely do not have it */
else if (cmp == 0) {
keep = 1; /* definitely have it */
-   sought[i]->matched = 1;
+   sought[i]->match_status = REF_MATCHED;
}
i++;
}
@@ -598,22 +598,24 @@ static void filter_refs(struct fetch_pack_args *args,
}
 
/* Append unmatched requests to the list */
-   if ((allow_unadvertised_object_request &
-   (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
-   for (i = 0; i < nr_sought; i++) {
-   unsigned char sha1[20];
+   for (i = 0; i < nr_sought; i++) {
+   unsigned char sha1[20];
 
-   ref = sought[i];
-   if (ref->matched)
-   continue;
-   if (get_sha1_hex(ref->name, sha1) ||
-   ref->name[40] != '\0' ||
-   hashcmp(sha1, ref->old_oid.hash))
-   continue;
+   ref = sought[i];
+   if (ref->match_status != REF_NOT_MATCHED)
+   continue;
+   if (get_sha1_hex(ref->name, sha1) ||
+   ref->name[40] != '\0' ||
+   hashcmp(sha1, ref->old_oid.hash))
+   continue;
 
-   ref->matched = 1;
+   if ((allow_unadvertised_object_request &
+   (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
+   ref->match_status = REF_MATCHED;
*newtail = copy_ref(ref);
newtail = &(*newtail)->next;
+   } else {
+   ref->match_status = REF_UNADVERTISED_NOT_ALLOWED;
}
}
*refs = newlist;
@@ -1100,9 +1102,19 @@ int report_unmatched_refs(struct ref **sought, int 
nr_sought)
int i, ret = 0;
 
for (i = 0; i < nr_sought; i++) {
-   if (!sought[i] || sought[i]->matched)
+   if (!sought[i])
continue;
-   error(_("no such remote ref %s"), sought[i]->name);
+   switch (sought[i]->match_status) {
+   case REF_MATCHED:
+   continue;
+   case REF_NOT_MATCHED:
+   error(_("no such remote ref %s"), sought[i]->name);
+   break;
+   case REF_UNADVERTISED_NOT_ALLOWED:
+   error(_("Server does not allow request for unadvertised 
object %s"),
+ sought[i]->name);
+   break;
+   }
ret = 1;
}
return ret;
diff --git a/remote.h b/remote.h
index 9248811..0b9d8c4 100644
--- a/remote.h
+++ b/remote.h
@@ -89,8 +89,13 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
-   deletion:1,
-   matched:1;
+   deletion:1;
+
+   enum {
+   REF_NOT_MATCHED = 0, /* initial value */
+   REF_MATCHED,
+   REF_UNADVERTISED_NOT_ALLOWED
+   } match_status;
 
/*
 * Order is important here, as we write to FETCH_HEAD
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 0d13a45..78f3b8e 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1099,7 +1099,7 @@ test_expect_success 'fetch exact SHA1' '
 
# fetching the hidden object should fail by default
test_must_fail git fetch -v ../testrepo 
$the_commit:refs/heads/copy 2>err &&
-   test_i18ngrep "no such remote ref" err &&
+   test_i18ngrep "Server does not allow request for unadvertised 
object" err &&
test_must_fail git rev-parse --verify refs/heads/copy &&
 
# the server side can allow it to succeed
-- 
2.9.3

[PATCH v2 2/3] fetch_refs_via_pack: call report_unmatched_refs

[Matt McCutchen]

"git fetch" currently doesn't bother to check that it got all refs it
sought, because the common case of requesting a nonexistent ref triggers
a die() in get_fetch_map.  However, there's at least one case that
slipped through: "git fetch REMOTE SHA1" if the server doesn't allow
requests for unadvertised objects.  Make fetch_refs_via_pack (which is
on the "git fetch" code path) call report_unmatched_refs so that we at
least get an error message in that case.

Signed-off-by: Matt McCutchen 
---
 t/t5516-fetch-push.sh |  3 ++-
 transport.c   | 14 +-
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 26b2caf..0d13a45 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1098,7 +1098,8 @@ test_expect_success 'fetch exact SHA1' '
test_must_fail git cat-file -t $the_commit &&
 
# fetching the hidden object should fail by default
-   test_must_fail git fetch -v ../testrepo 
$the_commit:refs/heads/copy &&
+   test_must_fail git fetch -v ../testrepo 
$the_commit:refs/heads/copy 2>err &&
+   test_i18ngrep "no such remote ref" err &&
test_must_fail git rev-parse --verify refs/heads/copy &&
 
# the server side can allow it to succeed
diff --git a/transport.c b/transport.c
index 04e5d66..c377907 100644
--- a/transport.c
+++ b/transport.c
@@ -204,6 +204,7 @@ static struct ref *get_refs_via_connect(struct transport 
*transport, int for_pus
 static int fetch_refs_via_pack(struct transport *transport,
   int nr_heads, struct ref **to_fetch)
 {
+   int ret = 0;
struct git_transport_data *data = transport->data;
struct ref *refs;
char *dest = xstrdup(transport->url);
@@ -241,19 +242,22 @@ static int fetch_refs_via_pack(struct transport 
*transport,
  >pack_lockfile);
close(data->fd[0]);
close(data->fd[1]);
-   if (finish_connect(data->conn)) {
-   free_refs(refs);
-   refs = NULL;
-   }
+   if (finish_connect(data->conn))
+   ret = -1;
data->conn = NULL;
data->got_remote_heads = 0;
data->options.self_contained_and_connected =
args.self_contained_and_connected;
 
+   if (refs == NULL)
+   ret = -1;
+   if (report_unmatched_refs(to_fetch, nr_heads))
+   ret = -1;
+
free_refs(refs_tmp);
free_refs(refs);
free(dest);
-   return (refs ? 0 : -1);
+   return ret;
 }
 
 static int push_had_errors(struct ref *ref)
-- 
2.9.3

[PATCH v2 1/3] fetch-pack: move code to report unmatched refs to a function

[Matt McCutchen]

We're preparing to reuse this code in transport.c for "git fetch".

While I'm here, internationalize the existing error message.

Signed-off-by: Matt McCutchen 
---
 builtin/fetch-pack.c  |  7 +--
 fetch-pack.c  | 13 +
 fetch-pack.h  |  6 ++
 t/t5500-fetch-pack.sh |  6 +++---
 4 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index cfe9e44..2a1c1c2 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -219,12 +219,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
 * remote no-such-ref' would silently succeed without issuing
 * an error.
 */
-   for (i = 0; i < nr_sought; i++) {
-   if (!sought[i] || sought[i]->matched)
-   continue;
-   error("no such remote ref %s", sought[i]->name);
-   ret = 1;
-   }
+   ret |= report_unmatched_refs(sought, nr_sought);
 
while (ref) {
printf("%s %s\n",
diff --git a/fetch-pack.c b/fetch-pack.c
index 601f077..7c8d44c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1094,3 +1094,16 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
clear_shallow_info();
return ref_cpy;
 }
+
+int report_unmatched_refs(struct ref **sought, int nr_sought)
+{
+   int i, ret = 0;
+
+   for (i = 0; i < nr_sought; i++) {
+   if (!sought[i] || sought[i]->matched)
+   continue;
+   error(_("no such remote ref %s"), sought[i]->name);
+   ret = 1;
+   }
+   return ret;
+}
diff --git a/fetch-pack.h b/fetch-pack.h
index c912e3d..a2d46e6 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -45,4 +45,10 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
   struct sha1_array *shallow,
   char **pack_lockfile);
 
+/*
+ * Print an appropriate error message for each sought ref that wasn't
+ * matched.  Return 0 if all sought refs were matched, otherwise 1.
+ */
+int report_unmatched_refs(struct ref **sought, int nr_sought);
+
 #endif
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 505e1b4..b5865b3 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -484,7 +484,7 @@ test_expect_success 'test lonely missing ref' '
cd client &&
test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy
) >/dev/null 2>error-m &&
-   test_cmp expect-error error-m
+   test_i18ncmp expect-error error-m
 '
 
 test_expect_success 'test missing ref after existing' '
@@ -492,7 +492,7 @@ test_expect_success 'test missing ref after existing' '
cd client &&
test_must_fail git fetch-pack --no-progress .. refs/heads/A 
refs/heads/xyzzy
) >/dev/null 2>error-em &&
-   test_cmp expect-error error-em
+   test_i18ncmp expect-error error-em
 '
 
 test_expect_success 'test missing ref before existing' '
@@ -500,7 +500,7 @@ test_expect_success 'test missing ref before existing' '
cd client &&
test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy 
refs/heads/A
) >/dev/null 2>error-me &&
-   test_cmp expect-error error-me
+   test_i18ncmp expect-error error-me
 '
 
 test_expect_success 'test --all, --depth, and explicit head' '
-- 
2.9.3

Re: [PATCH 1/3] fetch-pack: move code to report unmatched refs to a function

[Matt McCutchen]

On Wed, 2017-02-22 at 09:11 -0800, Junio C Hamano wrote:
> Matt McCutchen  writes:
> 
> > We're preparing to reuse this code in transport.c for "git fetch".
> > 
> > While I'm here, internationalize the existing error message.
> > ---
> 
> Sounds good.  Please just say it is OK for me to forge your sign-off
> ;-)

Oops.  Given the other issue below, I'll just regenerate the patch
series.

> > diff --git a/fetch-pack.h b/fetch-pack.h
> > index c912e3d..fd4d80e 100644
> > --- a/fetch-pack.h
> > +++ b/fetch-pack.h
> > @@ -45,4 +45,13 @@ struct ref *fetch_pack(struct fetch_pack_args
> > *args,
> >        struct sha1_array *shallow,
> >        char **pack_lockfile);
> >  
> > +/*
> > + * Print an appropriate error message for each sought ref that
> > wasn't
> > + * matched.  Return 0 if all sought refs were matched, otherwise
> > 1.
> > + *
> > + * The type of "sought" should be "const struct ref *const *" but
> > for
> > + * http://stackoverflow.com/questions/5055655/double-pointer-const
> > -correctness-warnings-in-c .
> > + */
> 
> This is an unfinished sentence, but I wonder if we even need to have
> it here?  I'd be surprised if this function was unique in the
> codebase that takes an array pointer whose type is looser than
> necessary because of well-known language rules.

You're probably right.  I'm in the habit of documenting things that
were unknown to me, but I'll take your word for what's well-known to
the average git developer.  I'll remove the remark.

Matt

Re: [PATCH v5 00/24] Remove submodule from files-backend.c

[Junio C Hamano]

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

> v5 goes a bit longer than v4, basically:
>
>  - files_path() is broken down into three smaller functions,
>files_{packed_refs,reflog,refname}_path().
>
>  - most of store-based api is added because..
>
>  - test-ref-store.c is added with t1405 and t1406 for some basic tests
>I'm not aimimg for complete ref store coverage. But we can continue
>to improve from there.
>
>  - refs_store_init() now takes a "permission" flag, like open().
>Operations are allowed or forbidden based on this flag. The
>submodule_allowed flag is killed. files_assert_main.. remains.
>
>  - get_*_ref_store() remain public api because it's used by
>test-ref-store.c and pack-refs.c.
>
>  - files-backend.c should now make no function calls that implicitly
>target the main store. But this will have to be tested more to be
>sure. I'm tempted to add a tracing backend just for this purpose.

OK.

> Junio, if you take this on 'pu', you'll have to kick my other two
> series out (they should not even compile). I'm not resending them
> until I get a "looks mostly ok" from Michael. No point in updating
> them when this series keeps moving.

Thanks for a note.

Re: [RFC][Git GUI] Make Commit message field in git GUI re sizable.

[Marc Branchaud]

On 2017-02-22 06:59 AM, Jessie Hernandez wrote:

HI,

the reason why it is fixed, is because commit messages should be
wrapped at 76 characters to be used in mails. So it helps you with the
wrapping.

Bert


Right ok. I understand.
Knowing this I think I might start writing my commit messages differently
then.


You can configure gui.commitMsgWidth if you don't like the default 
(which is 75, not 76).


M.



Thank you for this.

Regards

-
Jessie Hernandez




On Wed, Feb 22, 2017 at 10:27 AM, Jessie Hernandez
 wrote:

Hi all,

I have been using git for a few years now and really like the software.
I have a small annoyance and was wondering if I could get the
communities
view on this.

When using git GUI I find it handy to be able to re-size the "Unstaged
Changes" and the "Staged Changed" fields.

I would like the same thing for the "Commit Message" field, or to have
it
re-size with the git GUI window.

I can re-size the "Commit Message" vertically when making the "Modified"
panel smaller.

Does this make sense?
I would be happy to get into more detail if that is necessary or if
something is not clear.

Thank you.

-
Jessie Hernandez

Re: `git show --oneline commit` does not do what's intuitively expected

[Linus Torvalds]

On Thu, Feb 16, 2017 at 8:05 PM, Jeff King  wrote:
> On Fri, Feb 17, 2017 at 02:51:36AM +0100, Luna Kid wrote:
>
>> tl;dr; --> Please add --no-diff (or --msg-only) to git show. We'll
>> love you for that. :)
>
> I think it is already spelled "--no-patch", or "-s" for short.

I think people should also learn about "--no-walk" (or the numberic
walk limiters) to the revision walking logic, because it can often be
useful.

IOW, if you only want the commit info, you can certainly use "git show
-s", but you can also use

git log --no-walk .. list of commits ..

or

git log -1 

to show a commit without any other details.

Basically, "git show" for a commit does what is mostly equivalent to

 git log --cc --no-walk

although "git show" then has other features too (ie it shows non-commits etc).

Linus

Re: [PATCH 1/3] fetch-pack: move code to report unmatched refs to a function

[Junio C Hamano]

Matt McCutchen  writes:

> We're preparing to reuse this code in transport.c for "git fetch".
>
> While I'm here, internationalize the existing error message.
> ---

Sounds good.  Please just say it is OK for me to forge your sign-off ;-)

> diff --git a/fetch-pack.h b/fetch-pack.h
> index c912e3d..fd4d80e 100644
> --- a/fetch-pack.h
> +++ b/fetch-pack.h
> @@ -45,4 +45,13 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
>  struct sha1_array *shallow,
>  char **pack_lockfile);
>  
> +/*
> + * Print an appropriate error message for each sought ref that wasn't
> + * matched.  Return 0 if all sought refs were matched, otherwise 1.
> + *
> + * The type of "sought" should be "const struct ref *const *" but for
> + * 
> http://stackoverflow.com/questions/5055655/double-pointer-const-correctness-warnings-in-c
>  .
> + */

This is an unfinished sentence, but I wonder if we even need to have
it here?  I'd be surprised if this function was unique in the
codebase that takes an array pointer whose type is looser than
necessary because of well-known language rules.

Re: [PATCH] fetch: print an error when declining to request an unadvertised object

[Junio C Hamano]

Matt McCutchen  writes:

> Anyway, I made a split series and will send it in a moment.  I don't
> know if all the commit messages include exactly the information you
> want; hopefully you're happy to edit them as desired.  Compared to the
> previous patch, there is one fix in the net result: fixing t5500-fetch-
> pack.sh to deal with the internationalized "no such remote ref"
> message.

Thanks for going an extra mile.  I think many developers in the
future who reads "git log" will thank you, too.  The changes,
especially the one in the last one, are very much more
understandable compared to the original, even if the end result is
the same.

[PATCH 1/3] fetch-pack: move code to report unmatched refs to a function

[Matt McCutchen]

We're preparing to reuse this code in transport.c for "git fetch".

While I'm here, internationalize the existing error message.
---
 builtin/fetch-pack.c  |  7 +--
 fetch-pack.c  | 13 +
 fetch-pack.h  |  9 +
 t/t5500-fetch-pack.sh |  6 +++---
 4 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index cfe9e44..2a1c1c2 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -219,12 +219,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
*prefix)
 * remote no-such-ref' would silently succeed without issuing
 * an error.
 */
-   for (i = 0; i < nr_sought; i++) {
-   if (!sought[i] || sought[i]->matched)
-   continue;
-   error("no such remote ref %s", sought[i]->name);
-   ret = 1;
-   }
+   ret |= report_unmatched_refs(sought, nr_sought);
 
while (ref) {
printf("%s %s\n",
diff --git a/fetch-pack.c b/fetch-pack.c
index 601f077..7c8d44c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1094,3 +1094,16 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
clear_shallow_info();
return ref_cpy;
 }
+
+int report_unmatched_refs(struct ref **sought, int nr_sought)
+{
+   int i, ret = 0;
+
+   for (i = 0; i < nr_sought; i++) {
+   if (!sought[i] || sought[i]->matched)
+   continue;
+   error(_("no such remote ref %s"), sought[i]->name);
+   ret = 1;
+   }
+   return ret;
+}
diff --git a/fetch-pack.h b/fetch-pack.h
index c912e3d..fd4d80e 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -45,4 +45,13 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
   struct sha1_array *shallow,
   char **pack_lockfile);
 
+/*
+ * Print an appropriate error message for each sought ref that wasn't
+ * matched.  Return 0 if all sought refs were matched, otherwise 1.
+ *
+ * The type of "sought" should be "const struct ref *const *" but for
+ * 
http://stackoverflow.com/questions/5055655/double-pointer-const-correctness-warnings-in-c
 .
+ */
+int report_unmatched_refs(struct ref **sought, int nr_sought);
+
 #endif
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 505e1b4..b5865b3 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -484,7 +484,7 @@ test_expect_success 'test lonely missing ref' '
cd client &&
test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy
) >/dev/null 2>error-m &&
-   test_cmp expect-error error-m
+   test_i18ncmp expect-error error-m
 '
 
 test_expect_success 'test missing ref after existing' '
@@ -492,7 +492,7 @@ test_expect_success 'test missing ref after existing' '
cd client &&
test_must_fail git fetch-pack --no-progress .. refs/heads/A 
refs/heads/xyzzy
) >/dev/null 2>error-em &&
-   test_cmp expect-error error-em
+   test_i18ncmp expect-error error-em
 '
 
 test_expect_success 'test missing ref before existing' '
@@ -500,7 +500,7 @@ test_expect_success 'test missing ref before existing' '
cd client &&
test_must_fail git fetch-pack --no-progress .. refs/heads/xyzzy 
refs/heads/A
) >/dev/null 2>error-me &&
-   test_cmp expect-error error-me
+   test_i18ncmp expect-error error-me
 '
 
 test_expect_success 'test --all, --depth, and explicit head' '
-- 
2.9.3

[PATCH 3/3] fetch-pack: add specific error for fetching an unadvertised object

[Matt McCutchen]

Enhance filter_refs (which decides whether a request for an unadvertised
object should be sent to the server) to record a new match status on the
"struct ref" when a request is not allowed, and have
report_unmatched_refs check for this status and print a special error
message, "Server does not allow request for unadvertised object".
---
 fetch-pack.c  | 42 +++---
 remote.h  |  9 +++--
 t/t5516-fetch-push.sh |  2 +-
 3 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 7c8d44c..f12bfcd 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -578,7 +578,7 @@ static void filter_refs(struct fetch_pack_args *args,
break; /* definitely do not have it */
else if (cmp == 0) {
keep = 1; /* definitely have it */
-   sought[i]->matched = 1;
+   sought[i]->match_status = REF_MATCHED;
}
i++;
}
@@ -598,22 +598,24 @@ static void filter_refs(struct fetch_pack_args *args,
}
 
/* Append unmatched requests to the list */
-   if ((allow_unadvertised_object_request &
-   (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
-   for (i = 0; i < nr_sought; i++) {
-   unsigned char sha1[20];
+   for (i = 0; i < nr_sought; i++) {
+   unsigned char sha1[20];
 
-   ref = sought[i];
-   if (ref->matched)
-   continue;
-   if (get_sha1_hex(ref->name, sha1) ||
-   ref->name[40] != '\0' ||
-   hashcmp(sha1, ref->old_oid.hash))
-   continue;
+   ref = sought[i];
+   if (ref->match_status != REF_NOT_MATCHED)
+   continue;
+   if (get_sha1_hex(ref->name, sha1) ||
+   ref->name[40] != '\0' ||
+   hashcmp(sha1, ref->old_oid.hash))
+   continue;
 
-   ref->matched = 1;
+   if ((allow_unadvertised_object_request &
+   (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
+   ref->match_status = REF_MATCHED;
*newtail = copy_ref(ref);
newtail = &(*newtail)->next;
+   } else {
+   ref->match_status = REF_UNADVERTISED_NOT_ALLOWED;
}
}
*refs = newlist;
@@ -1100,9 +1102,19 @@ int report_unmatched_refs(struct ref **sought, int 
nr_sought)
int i, ret = 0;
 
for (i = 0; i < nr_sought; i++) {
-   if (!sought[i] || sought[i]->matched)
+   if (!sought[i])
continue;
-   error(_("no such remote ref %s"), sought[i]->name);
+   switch (sought[i]->match_status) {
+   case REF_MATCHED:
+   continue;
+   case REF_NOT_MATCHED:
+   error(_("no such remote ref %s"), sought[i]->name);
+   break;
+   case REF_UNADVERTISED_NOT_ALLOWED:
+   error(_("Server does not allow request for unadvertised 
object %s"),
+ sought[i]->name);
+   break;
+   }
ret = 1;
}
return ret;
diff --git a/remote.h b/remote.h
index 9248811..0b9d8c4 100644
--- a/remote.h
+++ b/remote.h
@@ -89,8 +89,13 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
-   deletion:1,
-   matched:1;
+   deletion:1;
+
+   enum {
+   REF_NOT_MATCHED = 0, /* initial value */
+   REF_MATCHED,
+   REF_UNADVERTISED_NOT_ALLOWED
+   } match_status;
 
/*
 * Order is important here, as we write to FETCH_HEAD
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 0d13a45..78f3b8e 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1099,7 +1099,7 @@ test_expect_success 'fetch exact SHA1' '
 
# fetching the hidden object should fail by default
test_must_fail git fetch -v ../testrepo 
$the_commit:refs/heads/copy 2>err &&
-   test_i18ngrep "no such remote ref" err &&
+   test_i18ngrep "Server does not allow request for unadvertised 
object" err &&
test_must_fail git rev-parse --verify refs/heads/copy &&
 
# the server side can allow it to succeed
-- 
2.9.3

[PATCH 2/3] fetch_refs_via_pack: call report_unmatched_refs

[Matt McCutchen]

"git fetch" currently doesn't bother to check that it got all refs it
sought, because the common case of requesting a nonexistent ref triggers
a die() in get_fetch_map.  However, there's at least one case that
slipped through: "git fetch REMOTE SHA1" if the server doesn't allow
requests for unadvertised objects.  Make fetch_refs_via_pack (which is
on the "git fetch" code path) call report_unmatched_refs so that we at
least get an error message in that case.
---
 t/t5516-fetch-push.sh |  3 ++-
 transport.c   | 14 +-
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 26b2caf..0d13a45 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1098,7 +1098,8 @@ test_expect_success 'fetch exact SHA1' '
test_must_fail git cat-file -t $the_commit &&
 
# fetching the hidden object should fail by default
-   test_must_fail git fetch -v ../testrepo 
$the_commit:refs/heads/copy &&
+   test_must_fail git fetch -v ../testrepo 
$the_commit:refs/heads/copy 2>err &&
+   test_i18ngrep "no such remote ref" err &&
test_must_fail git rev-parse --verify refs/heads/copy &&
 
# the server side can allow it to succeed
diff --git a/transport.c b/transport.c
index 04e5d66..c377907 100644
--- a/transport.c
+++ b/transport.c
@@ -204,6 +204,7 @@ static struct ref *get_refs_via_connect(struct transport 
*transport, int for_pus
 static int fetch_refs_via_pack(struct transport *transport,
   int nr_heads, struct ref **to_fetch)
 {
+   int ret = 0;
struct git_transport_data *data = transport->data;
struct ref *refs;
char *dest = xstrdup(transport->url);
@@ -241,19 +242,22 @@ static int fetch_refs_via_pack(struct transport 
*transport,
  >pack_lockfile);
close(data->fd[0]);
close(data->fd[1]);
-   if (finish_connect(data->conn)) {
-   free_refs(refs);
-   refs = NULL;
-   }
+   if (finish_connect(data->conn))
+   ret = -1;
data->conn = NULL;
data->got_remote_heads = 0;
data->options.self_contained_and_connected =
args.self_contained_and_connected;
 
+   if (refs == NULL)
+   ret = -1;
+   if (report_unmatched_refs(to_fetch, nr_heads))
+   ret = -1;
+
free_refs(refs_tmp);
free_refs(refs);
free(dest);
-   return (refs ? 0 : -1);
+   return ret;
 }
 
 static int push_had_errors(struct ref *ref)
-- 
2.9.3

Re: [PATCH] fetch: print an error when declining to request an unadvertised object

[Matt McCutchen]

On Mon, 2017-02-20 at 22:36 -0800, Junio C Hamano wrote:
> Hmph, I would have expected this to be done as a three-patch series,
> 
>  * move the loop at the end of cmd_fetch_pack() to a separate helper
>    function report_unmatched_refs() and call it;
> 
>  * add a call to report_unmatched_refs() to the transport layer;
> 
>  * enhance report_unmatched_refs() by introducing match_status
>    field and adding new code to filter_refs() to diagnose other
>    kinds of errors.

Sure.

> The result looks reasonable from a cursory read, though.
> 
> Thanks for following it up to the completion.

This remark led me to believe you were satisfied with the single patch,
but the last "What's cooking in git.git" mail says "Expecting a split
series?".

Anyway, I made a split series and will send it in a moment.  I don't
know if all the commit messages include exactly the information you
want; hopefully you're happy to edit them as desired.  Compared to the
previous patch, there is one fix in the net result: fixing t5500-fetch-
pack.sh to deal with the internationalized "no such remote ref"
message.

Matt

Hello

[university]

Please kindly reply me if this your email is still in use.

Sincerely yours
Anessa...t

Re: url..insteadOf vs. submodules

[Jon Loeliger]

So, like, Junio C Hamano said:
> Stefan Beller  writes:
> 
> > Do we want to invent a special value for url.*.insteadOf to mean
> >   "look up in superproject, so I don't have to keep
> >   a copy that may get stale" ?
> 
> My gut feeling is that we should do the selective/filtered include
> Peff mentioned when a repository is known to be used as a submodule
> of somebody else.

Does the management of these submodue-related config values
become easier if, instead of placing them in .config, we
place them in a git/.context file?

jdl

Hello beautiful

[Wesley]

How you doing today? I hope you are doing well. My name is Wesley, from the US. 
I'm in Syria right now fighting ISIS. I want to get to know you better, if I 
may be so bold. I consider myself an easy-going man, and I am currently looking 
for a relationship in which I feel loved. Please tell me more about yourself, 
if you don't mind.

Hope to hear from you soon.

Regards,

Wesley.

[PATCH v5 22/24] t/helper: add test-ref-store to test ref-store functions

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Makefile|   1 +
 t/helper/.gitignore |   1 +
 t/helper/test-ref-store.c (new) | 274 
 3 files changed, 276 insertions(+)
 create mode 100644 t/helper/test-ref-store.c

diff --git a/Makefile b/Makefile
index 8e4081e06..d62d64623 100644
--- a/Makefile
+++ b/Makefile
@@ -624,6 +624,7 @@ TEST_PROGRAMS_NEED_X += test-parse-options
 TEST_PROGRAMS_NEED_X += test-path-utils
 TEST_PROGRAMS_NEED_X += test-prio-queue
 TEST_PROGRAMS_NEED_X += test-read-cache
+TEST_PROGRAMS_NEED_X += test-ref-store
 TEST_PROGRAMS_NEED_X += test-regex
 TEST_PROGRAMS_NEED_X += test-revision-walking
 TEST_PROGRAMS_NEED_X += test-run-command
diff --git a/t/helper/.gitignore b/t/helper/.gitignore
index d6e8b3679..5f68aa8f8 100644
--- a/t/helper/.gitignore
+++ b/t/helper/.gitignore
@@ -19,6 +19,7 @@
 /test-path-utils
 /test-prio-queue
 /test-read-cache
+/test-ref-store
 /test-regex
 /test-revision-walking
 /test-run-command
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
new file mode 100644
index 0..c4c670acd
--- /dev/null
+++ b/t/helper/test-ref-store.c
@@ -0,0 +1,274 @@
+#include "cache.h"
+#include "refs.h"
+
+static const char *notnull(const char *arg, const char *name)
+{
+   if (!arg)
+   die("%s required", name);
+   return arg;
+}
+
+static unsigned int arg_flags(const char *arg, const char *name)
+{
+   return atoi(notnull(arg, name));
+}
+
+static const char **get_store(const char **argv, struct ref_store **refs)
+{
+   const char *gitdir;
+
+   if (!argv[0]) {
+   die("ref store required");
+   } else if (!strcmp(argv[0], "main")) {
+   *refs = get_main_ref_store();
+   } else if (skip_prefix(argv[0], "submodule:", )) {
+   struct strbuf sb = STRBUF_INIT;
+   int ret;
+
+   ret = strbuf_git_path_submodule(, gitdir, "objects/");
+   if (ret)
+   die("strbuf_git_path_submodule failed: %d", ret);
+   add_to_alternates_memory(sb.buf);
+   strbuf_release();
+
+   *refs = get_submodule_ref_store(gitdir);
+   } else
+   die("unknown backend %s", argv[0]);
+
+   if (!*refs)
+   die("no ref store");
+
+   /* consume store-specific optional arguments if needed */
+
+   return argv + 1;
+}
+
+
+static int cmd_pack_refs(struct ref_store *refs, const char **argv)
+{
+   unsigned int flags = arg_flags(*argv++, "flags");
+
+   return refs_pack_refs(refs, flags);
+}
+
+static int cmd_peel_ref(struct ref_store *refs, const char **argv)
+{
+   const char *refname = notnull(*argv++, "refname");
+   unsigned char sha1[20];
+   int ret;
+
+   ret = refs_peel_ref(refs, refname, sha1);
+   if (!ret)
+   puts(sha1_to_hex(sha1));
+   return ret;
+}
+
+static int cmd_create_symref(struct ref_store *refs, const char **argv)
+{
+   const char *refname = notnull(*argv++, "refname");
+   const char *target = notnull(*argv++, "target");
+   const char *logmsg = *argv++;
+
+   return refs_create_symref(refs, refname, target, logmsg);
+}
+
+static int cmd_delete_refs(struct ref_store *refs, const char **argv)
+{
+   unsigned int flags = arg_flags(*argv++, "flags");
+   struct string_list refnames = STRING_LIST_INIT_NODUP;
+
+   while (*argv)
+   string_list_append(, *argv++);
+
+   return refs_delete_refs(refs, , flags);
+}
+
+static int cmd_rename_ref(struct ref_store *refs, const char **argv)
+{
+   const char *oldref = notnull(*argv++, "oldref");
+   const char *newref = notnull(*argv++, "newref");
+   const char *logmsg = *argv++;
+
+   return refs_rename_ref(refs, oldref, newref, logmsg);
+}
+
+static int each_ref(const char *refname, const struct object_id *oid,
+   int flags, void *cb_data)
+{
+   printf("%s %s 0x%x\n", oid_to_hex(oid), refname, flags);
+   return 0;
+}
+
+static int cmd_for_each_ref(struct ref_store *refs, const char **argv)
+{
+   const char *prefix = notnull(*argv++, "prefix");
+
+   return refs_for_each_ref_in(refs, prefix, each_ref, NULL);
+}
+
+static int cmd_resolve_ref(struct ref_store *refs, const char **argv)
+{
+   unsigned char sha1[20];
+   const char *refname = notnull(*argv++, "refname");
+   int resolve_flags = arg_flags(*argv++, "resolve-flags");
+   int flags;
+   const char *ref;
+
+   ref = refs_resolve_ref_unsafe(refs, refname, resolve_flags,
+ sha1, );
+   printf("%s %s 0x%x\n", sha1_to_hex(sha1), ref, flags);
+   return ref ? 0 : 1;
+}
+
+static int cmd_verify_ref(struct ref_store *refs, const char **argv)
+{
+   const char *refname = notnull(*argv++, "refname");
+   struct strbuf err = STRBUF_INIT;
+   int ret;
+
+   ret = 

[PATCH v5 24/24] t1406: new tests for submodule ref store

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/t1406-submodule-ref-store.sh (new +x) | 95 +
 1 file changed, 95 insertions(+)
 create mode 100755 t/t1406-submodule-ref-store.sh

diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
new file mode 100755
index 0..3b30ba62f
--- /dev/null
+++ b/t/t1406-submodule-ref-store.sh
@@ -0,0 +1,95 @@
+#!/bin/sh
+
+test_description='test submodule ref store api'
+
+. ./test-lib.sh
+
+RUN="test-ref-store submodule:sub"
+
+test_expect_success 'setup' '
+   git init sub &&
+   (
+   cd sub &&
+   test_commit first &&
+   git checkout -b new-master
+   )
+'
+
+test_expect_success 'pack_refs() not allowed' '
+   test_must_fail $RUN pack-refs 3
+'
+
+test_expect_success 'peel_ref(new-tag)' '
+   git -C sub rev-parse HEAD >expected &&
+   git -C sub tag -a -m new-tag new-tag HEAD &&
+   $RUN peel-ref refs/tags/new-tag >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'create_symref() not allowed' '
+   test_must_fail $RUN create-symref FOO refs/heads/master nothing
+'
+
+test_expect_success 'delete_refs() not allowed' '
+   test_must_fail $RUN delete-refs 0 FOO refs/tags/new-tag
+'
+
+test_expect_success 'rename_refs() not allowed' '
+   test_must_fail $RUN rename-ref refs/heads/master refs/heads/new-master
+'
+
+test_expect_success 'for_each_ref(refs/heads/)' '
+   $RUN for-each-ref refs/heads/ | cut -c 42- >actual &&
+   cat >expected <<-\EOF &&
+   master 0x0
+   new-master 0x0
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'resolve_ref(master)' '
+   SHA1=`git -C sub rev-parse master` &&
+   echo "$SHA1 refs/heads/master 0x0" >expected &&
+   $RUN resolve-ref refs/heads/master 0 >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'verify_ref(new-master)' '
+   $RUN verify-ref refs/heads/new-master
+'
+
+test_expect_success 'for_each_reflog()' '
+   $RUN for-each-reflog | cut -c 42- >actual &&
+   cat >expected <<-\EOF &&
+   refs/heads/master 0x0
+   refs/heads/new-master 0x0
+   HEAD 0x1
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'for_each_reflog_ent()' '
+   $RUN for-each-reflog-ent HEAD >actual && cat actual &&
+   head -n1 actual | grep first &&
+   tail -n2 actual | head -n1 | grep master.to.new
+'
+
+test_expect_success 'for_each_reflog_ent_reverse()' '
+   $RUN for-each-reflog-ent-reverse HEAD >actual &&
+   head -n1 actual | grep master.to.new &&
+   tail -n2 actual | head -n1 | grep first
+'
+
+test_expect_success 'reflog_exists(HEAD)' '
+   $RUN reflog-exists HEAD
+'
+
+test_expect_success 'delete_reflog() not allowed' '
+   test_must_fail $RUN delete-reflog HEAD
+'
+
+test_expect_success 'create-reflog() not allowed' '
+   test_must_fail $RUN create-reflog HEAD 1
+'
+
+test_done
-- 
2.11.0.157.gd943d85

[PATCH v5 23/24] t1405: some basic tests on main ref store

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/t1405-main-ref-store.sh (new +x) | 123 +
 1 file changed, 123 insertions(+)
 create mode 100755 t/t1405-main-ref-store.sh

diff --git a/t/t1405-main-ref-store.sh b/t/t1405-main-ref-store.sh
new file mode 100755
index 0..0999829f1
--- /dev/null
+++ b/t/t1405-main-ref-store.sh
@@ -0,0 +1,123 @@
+#!/bin/sh
+
+test_description='test main ref store api'
+
+. ./test-lib.sh
+
+RUN="test-ref-store main"
+
+test_expect_success 'pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE)' '
+   test_commit one &&
+   N=`find .git/refs -type f | wc -l` &&
+   test "$N" != 0 &&
+   $RUN pack-refs 3 &&
+   N=`find .git/refs -type f | wc -l`
+'
+
+test_expect_success 'peel_ref(new-tag)' '
+   git rev-parse HEAD >expected &&
+   git tag -a -m new-tag new-tag HEAD &&
+   $RUN peel-ref refs/tags/new-tag >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'create_symref(FOO, refs/heads/master)' '
+   $RUN create-symref FOO refs/heads/master nothing &&
+   echo refs/heads/master >expected &&
+   git symbolic-ref FOO >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'delete_refs(FOO, refs/tags/new-tag)' '
+   git rev-parse FOO -- &&
+   git rev-parse refs/tags/new-tag -- &&
+   $RUN delete-refs 0 FOO refs/tags/new-tag &&
+   test_must_fail git rev-parse FOO -- &&
+   test_must_fail git rev-parse refs/tags/new-tag --
+'
+
+test_expect_success 'rename_refs(master, new-master)' '
+   git rev-parse master >expected &&
+   $RUN rename-ref refs/heads/master refs/heads/new-master &&
+   git rev-parse new-master >actual &&
+   test_cmp expected actual &&
+   test_commit recreate-master
+'
+
+test_expect_success 'for_each_ref(refs/heads/)' '
+   $RUN for-each-ref refs/heads/ | cut -c 42- >actual &&
+   cat >expected <<-\EOF &&
+   master 0x0
+   new-master 0x0
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'resolve_ref(new-master)' '
+   SHA1=`git rev-parse new-master` &&
+   echo "$SHA1 refs/heads/new-master 0x0" >expected &&
+   $RUN resolve-ref refs/heads/new-master 0 >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'verify_ref(new-master)' '
+   $RUN verify-ref refs/heads/new-master
+'
+
+test_expect_success 'for_each_reflog()' '
+   $RUN for-each-reflog | cut -c 42- >actual &&
+   cat >expected <<-\EOF &&
+   refs/heads/master 0x0
+   refs/heads/new-master 0x0
+   HEAD 0x1
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'for_each_reflog_ent()' '
+   $RUN for-each-reflog-ent HEAD >actual &&
+   head -n1 actual | grep one &&
+   tail -n2 actual | head -n1 | grep recreate-master
+'
+
+test_expect_success 'for_each_reflog_ent_reverse()' '
+   $RUN for-each-reflog-ent-reverse HEAD >actual &&
+   head -n1 actual | grep recreate-master &&
+   tail -n2 actual | head -n1 | grep one
+'
+
+test_expect_success 'reflog_exists(HEAD)' '
+   $RUN reflog-exists HEAD
+'
+
+test_expect_success 'delete_reflog(HEAD)' '
+   $RUN delete-reflog HEAD &&
+   ! test -f .git/logs/HEAD
+'
+
+test_expect_success 'create-reflog(HEAD)' '
+   $RUN create-reflog HEAD 1 &&
+   test -f .git/logs/HEAD
+'
+
+test_expect_success 'delete_ref(refs/heads/foo)' '
+   git checkout -b foo &&
+   FOO_SHA1=`git rev-parse foo` &&
+   git checkout --detach &&
+   test_commit bar-commit &&
+   git checkout -b bar &&
+   BAR_SHA1=`git rev-parse bar` &&
+   $RUN update-ref updating refs/heads/foo $BAR_SHA1 $FOO_SHA1 0 &&
+   echo $BAR_SHA1 >expected &&
+   git rev-parse refs/heads/foo >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success 'delete_ref(refs/heads/foo)' '
+   SHA1=`git rev-parse foo` &&
+   git checkout --detach &&
+   $RUN delete-ref refs/heads/foo $SHA1 0 &&
+   test_must_fail git rev-parse refs/heads/foo --
+'
+
+test_done
-- 
2.11.0.157.gd943d85

[PATCH v5 21/24] refs: delete pack_refs() in favor of refs_pack_refs()

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

It only has one caller, not worth keeping just for convenience.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/pack-refs.c | 2 +-
 refs.c  | 5 -
 refs.h  | 1 -
 3 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/builtin/pack-refs.c b/builtin/pack-refs.c
index 39f9a55d1..b106a392a 100644
--- a/builtin/pack-refs.c
+++ b/builtin/pack-refs.c
@@ -17,5 +17,5 @@ int cmd_pack_refs(int argc, const char **argv, const char 
*prefix)
};
if (parse_options(argc, argv, prefix, opts, pack_refs_usage, 0))
usage_with_options(pack_refs_usage, opts);
-   return pack_refs(flags);
+   return refs_pack_refs(get_main_ref_store(), flags);
 }
diff --git a/refs.c b/refs.c
index 851b5e125..6efe5957d 100644
--- a/refs.c
+++ b/refs.c
@@ -1594,11 +1594,6 @@ int refs_pack_refs(struct ref_store *refs, unsigned int 
flags)
return refs->be->pack_refs(refs, flags);
 }
 
-int pack_refs(unsigned int flags)
-{
-   return refs_pack_refs(get_main_ref_store(), flags);
-}
-
 int refs_peel_ref(struct ref_store *refs, const char *refname,
  unsigned char *sha1)
 {
diff --git a/refs.h b/refs.h
index 342cecd23..1af41eaef 100644
--- a/refs.h
+++ b/refs.h
@@ -294,7 +294,6 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,
  * flags: Combination of the above PACK_REFS_* flags.
  */
 int refs_pack_refs(struct ref_store *refs, unsigned int flags);
-int pack_refs(unsigned int flags);
 
 /*
  * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.
-- 
2.11.0.157.gd943d85

[PATCH v5 20/24] files-backend: avoid ref api targetting main ref store

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

A small step towards making files-backend works as a non-main ref store
using the newly added store-aware API.

For the record, `join` and `nm` on refs.o and files-backend.o tell me
that files-backend no longer uses functions that defaults to
get_main_ref_store().

I'm not yet comfortable at the idea of removing
files_assert_main_repository() (or converting REF_STORE_MAIN to
REF_STORE_WRITE). More staring and testing is required before that can
happen. Well, except peel_ref(). I'm pretty sure that function is safe.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 85 ++--
 1 file changed, 49 insertions(+), 36 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index dafddefd3..09c280fd3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1836,8 +1836,6 @@ static int files_peel_ref(struct ref_store *ref_store,
int flag;
unsigned char base[20];
 
-   files_assert_main_repository(refs, "peel_ref");
-
if (current_ref_iter && current_ref_iter->refname == refname) {
struct object_id peeled;
 
@@ -1847,7 +1845,8 @@ static int files_peel_ref(struct ref_store *ref_store,
return 0;
}
 
-   if (read_ref_full(refname, RESOLVE_REF_READING, base, ))
+   if (refs_read_ref_full(ref_store, refname,
+  RESOLVE_REF_READING, base, ))
return -1;
 
/*
@@ -2017,15 +2016,15 @@ static struct ref_iterator *files_ref_iterator_begin(
  * on success. On error, write an error message to err, set errno, and
  * return a negative value.
  */
-static int verify_lock(struct ref_lock *lock,
+static int verify_lock(struct ref_store *ref_store, struct ref_lock *lock,
   const unsigned char *old_sha1, int mustexist,
   struct strbuf *err)
 {
assert(err);
 
-   if (read_ref_full(lock->ref_name,
- mustexist ? RESOLVE_REF_READING : 0,
- lock->old_oid.hash, NULL)) {
+   if (refs_read_ref_full(ref_store, lock->ref_name,
+  mustexist ? RESOLVE_REF_READING : 0,
+  lock->old_oid.hash, NULL)) {
if (old_sha1) {
int save_errno = errno;
strbuf_addf(err, "can't verify ref '%s'", 
lock->ref_name);
@@ -2094,8 +2093,9 @@ static struct ref_lock *lock_ref_sha1_basic(struct 
files_ref_store *refs,
resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
 
files_refname_path(refs, _file, refname);
-   resolved = !!resolve_ref_unsafe(refname, resolve_flags,
-   lock->old_oid.hash, type);
+   resolved = !!refs_resolve_ref_unsafe(>base,
+refname, resolve_flags,
+lock->old_oid.hash, type);
if (!resolved && errno == EISDIR) {
/*
 * we are trying to lock foo but we used to
@@ -2112,8 +2112,9 @@ static struct ref_lock *lock_ref_sha1_basic(struct 
files_ref_store *refs,
refname);
goto error_return;
}
-   resolved = !!resolve_ref_unsafe(refname, resolve_flags,
-   lock->old_oid.hash, type);
+   resolved = !!refs_resolve_ref_unsafe(>base,
+refname, resolve_flags,
+lock->old_oid.hash, type);
}
if (!resolved) {
last_errno = errno;
@@ -2151,7 +2152,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct 
files_ref_store *refs,
goto error_return;
}
 
-   if (verify_lock(lock, old_sha1, mustexist, err)) {
+   if (verify_lock(>base, lock, old_sha1, mustexist, err)) {
last_errno = errno;
goto error_return;
}
@@ -2406,7 +2407,7 @@ static void try_remove_empty_parents(struct 
files_ref_store *refs,
 }
 
 /* make sure nobody touched the ref, and unlink */
-static void prune_ref(struct ref_to_prune *r)
+static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)
 {
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
@@ -2414,7 +2415,7 @@ static void prune_ref(struct ref_to_prune *r)
if (check_refname_format(r->name, 0))
return;
 
-   transaction = ref_transaction_begin();
+   transaction = ref_store_transaction_begin(>base, );
if (!transaction ||
ref_transaction_delete(transaction, r->name, r->sha1,
   REF_ISPRUNING | REF_NODEREF, NULL, ) ||
@@ -2428,10 +2429,10 @@ static void prune_ref(struct ref_to_prune *r)
strbuf_release();
 }
 
-static void 

[PATCH v5 18/24] refs: add new ref-store api

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

This is not meant to cover all existing API. It adds enough to test ref
stores with the new test program test-ref-store, coming soon and to be
used by files-backend.c.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 235 +--
 refs.h   |  71 
 refs/files-backend.c |  13 +--
 refs/refs-internal.h |  31 +--
 4 files changed, 253 insertions(+), 97 deletions(-)

diff --git a/refs.c b/refs.c
index 7843d3085..9137ac283 100644
--- a/refs.c
+++ b/refs.c
@@ -185,13 +185,20 @@ struct ref_filter {
void *cb_data;
 };
 
-int read_ref_full(const char *refname, int resolve_flags, unsigned char *sha1, 
int *flags)
+int refs_read_ref_full(struct ref_store *refs, const char *refname,
+  int resolve_flags, unsigned char *sha1, int *flags)
 {
-   if (resolve_ref_unsafe(refname, resolve_flags, sha1, flags))
+   if (refs_resolve_ref_unsafe(refs, refname, resolve_flags, sha1, flags))
return 0;
return -1;
 }
 
+int read_ref_full(const char *refname, int resolve_flags, unsigned char *sha1, 
int *flags)
+{
+   return refs_read_ref_full(get_main_ref_store(), refname,
+ resolve_flags, sha1, flags);
+}
+
 int read_ref(const char *refname, unsigned char *sha1)
 {
return read_ref_full(refname, RESOLVE_REF_READING, sha1, NULL);
@@ -286,34 +293,52 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, 
const struct string_li
for_each_rawref(warn_if_dangling_symref, );
 }
 
+int refs_for_each_tag_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data)
+{
+   return refs_for_each_ref_in(refs, "refs/tags/", fn, cb_data);
+}
+
 int for_each_tag_ref(each_ref_fn fn, void *cb_data)
 {
-   return for_each_ref_in("refs/tags/", fn, cb_data);
+   return refs_for_each_tag_ref(get_main_ref_store(), fn, cb_data);
 }
 
 int for_each_tag_ref_submodule(const char *submodule, each_ref_fn fn, void 
*cb_data)
 {
-   return for_each_ref_in_submodule(submodule, "refs/tags/", fn, cb_data);
+   return refs_for_each_tag_ref(get_submodule_ref_store(submodule),
+fn, cb_data);
+}
+
+int refs_for_each_branch_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data)
+{
+   return refs_for_each_ref_in(refs, "refs/heads/", fn, cb_data);
 }
 
 int for_each_branch_ref(each_ref_fn fn, void *cb_data)
 {
-   return for_each_ref_in("refs/heads/", fn, cb_data);
+   return refs_for_each_branch_ref(get_main_ref_store(), fn, cb_data);
 }
 
 int for_each_branch_ref_submodule(const char *submodule, each_ref_fn fn, void 
*cb_data)
 {
-   return for_each_ref_in_submodule(submodule, "refs/heads/", fn, cb_data);
+   return refs_for_each_branch_ref(get_submodule_ref_store(submodule),
+   fn, cb_data);
+}
+
+int refs_for_each_remote_ref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data)
+{
+   return refs_for_each_ref_in(refs, "refs/remotes/", fn, cb_data);
 }
 
 int for_each_remote_ref(each_ref_fn fn, void *cb_data)
 {
-   return for_each_ref_in("refs/remotes/", fn, cb_data);
+   return refs_for_each_remote_ref(get_main_ref_store(), fn, cb_data);
 }
 
 int for_each_remote_ref_submodule(const char *submodule, each_ref_fn fn, void 
*cb_data)
 {
-   return for_each_ref_in_submodule(submodule, "refs/remotes/", fn, 
cb_data);
+   return refs_for_each_remote_ref(get_submodule_ref_store(submodule),
+   fn, cb_data);
 }
 
 int head_ref_namespaced(each_ref_fn fn, void *cb_data)
@@ -1120,14 +1145,17 @@ const char *find_descendant_ref(const char *dirname,
return NULL;
 }
 
-int rename_ref_available(const char *old_refname, const char *new_refname)
+int refs_rename_ref_available(struct ref_store *refs,
+ const char *old_refname,
+ const char *new_refname)
 {
struct string_list skip = STRING_LIST_INIT_NODUP;
struct strbuf err = STRBUF_INIT;
int ok;
 
string_list_insert(, old_refname);
-   ok = !verify_refname_available(new_refname, NULL, , );
+   ok = !refs_verify_refname_available(refs, new_refname,
+   NULL, , );
if (!ok)
error("%s", err.buf);
 
@@ -1168,10 +1196,9 @@ int head_ref(each_ref_fn fn, void *cb_data)
  * non-zero value, stop the iteration and return that value;
  * otherwise, return 0.
  */
-static int do_for_each_ref(const char *submodule, const char *prefix,
+static int do_for_each_ref(struct ref_store *refs, const char *prefix,
   each_ref_fn fn, int trim, int flags, void *cb_data)
 {
-   struct ref_store *refs = get_submodule_ref_store(submodule);
struct ref_iterator *iter;
 
if (!refs)
@@ -1183,19 +1210,30 @@ static int do_for_each_ref(const char *submodule, const 

[PATCH v5 16/24] files-backend: replace submodule_allowed check in files_downcast()

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

files-backend.c is unlearning submodules. Instead of having a specific
check for submodules to see what operation is allowed, files backend
now takes a set of flags at init. Each operation will check if the
required flags is present before performing.

For now we have four flags: read, write and odb access. Main ref store
has all flags, obviously, while submodule stores are read-only and have
access to odb (*).

The "main" flag stays because many functions in the backend calls
frontend ones without a ref store, so these functions always target the
main ref store. Ideally the flag should be gone after ref-store-aware
api is in place and used by backends.

(*) Submodule code needs for_each_ref. Try take REF_STORE_ODB flag
out. At least t3404 would fail. The "have access to odb" in submodule is
a bit hacky since we don't know from he whether add_submodule_odb() has
been called.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 15 ++---
 refs/files-backend.c | 86 +---
 refs/refs-internal.h |  9 +-
 3 files changed, 73 insertions(+), 37 deletions(-)

diff --git a/refs.c b/refs.c
index 67acae60c..2dc97891a 100644
--- a/refs.c
+++ b/refs.c
@@ -1416,7 +1416,8 @@ static struct ref_store *lookup_submodule_ref_store(const 
char *submodule)
  * Create, record, and return a ref_store instance for the specified
  * gitdir.
  */
-static struct ref_store *ref_store_init(const char *gitdir)
+static struct ref_store *ref_store_init(const char *gitdir,
+   unsigned int flags)
 {
const char *be_name = "files";
struct ref_storage_be *be = find_ref_storage_backend(be_name);
@@ -1425,7 +1426,7 @@ static struct ref_store *ref_store_init(const char 
*gitdir)
if (!be)
die("BUG: reference backend %s is unknown", be_name);
 
-   refs = be->init(gitdir);
+   refs = be->init(gitdir, flags);
return refs;
 }
 
@@ -1436,7 +1437,11 @@ struct ref_store *get_main_ref_store(void)
if (main_ref_store)
return main_ref_store;
 
-   refs = ref_store_init(get_git_dir());
+   refs = ref_store_init(get_git_dir(),
+ (REF_STORE_READ |
+  REF_STORE_WRITE |
+  REF_STORE_ODB |
+  REF_STORE_MAIN));
if (refs) {
if (main_ref_store)
die("BUG: main_ref_store initialized twice");
@@ -1485,7 +1490,9 @@ struct ref_store *get_ref_store(const char *submodule)
 
ret = submodule_to_gitdir(_sb, submodule);
if (!ret)
-   refs = ref_store_init(submodule_sb.buf);
+   /* pretend that add_submodule_odb() has been called */
+   refs = ref_store_init(submodule_sb.buf,
+ REF_STORE_READ | REF_STORE_ODB);
strbuf_release(_sb);
 
if (refs)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 37443369b..474d1027c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -916,6 +916,7 @@ struct packed_ref_cache {
  */
 struct files_ref_store {
struct ref_store base;
+   unsigned int store_flags;
 
char *gitdir;
char *gitcommondir;
@@ -976,13 +977,15 @@ static void clear_loose_ref_cache(struct files_ref_store 
*refs)
  * Create a new submodule ref cache and add it to the internal
  * set of caches.
  */
-static struct ref_store *files_ref_store_create(const char *gitdir)
+static struct ref_store *files_ref_store_create(const char *gitdir,
+   unsigned int flags)
 {
struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
struct ref_store *ref_store = (struct ref_store *)refs;
struct strbuf sb = STRBUF_INIT;
 
base_ref_store_init(ref_store, _be_files);
+   refs->store_flags = flags;
 
refs->gitdir = xstrdup(gitdir);
get_common_dir_noenv(, gitdir);
@@ -994,13 +997,17 @@ static struct ref_store *files_ref_store_create(const 
char *gitdir)
 }
 
 /*
- * Die if refs is for a submodule (i.e., not for the main repository).
- * caller is used in any necessary error messages.
+ * Die if refs is not the main ref store. caller is used in any
+ * necessary error messages.
  */
 static void files_assert_main_repository(struct files_ref_store *refs,
 const char *caller)
 {
-   /* This function is to be deleted in the next patch */
+   if (refs->store_flags & REF_STORE_MAIN)
+   return;
+
+   die("BUG: unallowed operation (%s), only works "
+   "on main ref store\n", caller);
 }
 
 /*
@@ -1009,9 +1016,9 @@ static void files_assert_main_repository(struct 
files_ref_store *refs,
  * files_ref_store is for a submodule (i.e., not for the main
  * repository). caller is used in any necessary error messages.
  */
-static 

[PATCH v5 19/24] refs: new transaction related ref-store api

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

The transaction struct now takes a ref store at creation and will
operate on that ref store alone.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 54 
 refs.h   |  8 
 refs/refs-internal.h |  1 +
 3 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/refs.c b/refs.c
index 9137ac283..851b5e125 100644
--- a/refs.c
+++ b/refs.c
@@ -618,16 +618,19 @@ static int delete_pseudoref(const char *pseudoref, const 
unsigned char *old_sha1
return 0;
 }
 
-int delete_ref(const char *refname, const unsigned char *old_sha1,
-  unsigned int flags)
+int refs_delete_ref(struct ref_store *refs, const char *refname,
+   const unsigned char *old_sha1,
+   unsigned int flags)
 {
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
 
-   if (ref_type(refname) == REF_TYPE_PSEUDOREF)
+   if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
+   assert(refs == get_main_ref_store());
return delete_pseudoref(refname, old_sha1);
+   }
 
-   transaction = ref_transaction_begin();
+   transaction = ref_store_transaction_begin(refs, );
if (!transaction ||
ref_transaction_delete(transaction, refname, old_sha1,
   flags, NULL, ) ||
@@ -642,6 +645,13 @@ int delete_ref(const char *refname, const unsigned char 
*old_sha1,
return 0;
 }
 
+int delete_ref(const char *refname, const unsigned char *old_sha1,
+  unsigned int flags)
+{
+   return refs_delete_ref(get_main_ref_store(), refname,
+  old_sha1, flags);
+}
+
 int copy_reflog_msg(char *buf, const char *msg)
 {
char *cp = buf;
@@ -801,11 +811,20 @@ int read_ref_at(const char *refname, unsigned int flags, 
unsigned long at_time,
return 1;
 }
 
-struct ref_transaction *ref_transaction_begin(struct strbuf *err)
+struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
+   struct strbuf *err)
 {
+   struct ref_transaction *tr;
assert(err);
 
-   return xcalloc(1, sizeof(struct ref_transaction));
+   tr = xcalloc(1, sizeof(struct ref_transaction));
+   tr->ref_store = refs;
+   return tr;
+}
+
+struct ref_transaction *ref_transaction_begin(struct strbuf *err)
+{
+   return ref_store_transaction_begin(get_main_ref_store(), err);
 }
 
 void ref_transaction_free(struct ref_transaction *transaction)
@@ -922,18 +941,20 @@ int update_ref_oid(const char *msg, const char *refname,
old_oid ? old_oid->hash : NULL, flags, onerr);
 }
 
-int update_ref(const char *msg, const char *refname,
-  const unsigned char *new_sha1, const unsigned char *old_sha1,
-  unsigned int flags, enum action_on_err onerr)
+int refs_update_ref(struct ref_store *refs, const char *msg,
+   const char *refname, const unsigned char *new_sha1,
+   const unsigned char *old_sha1, unsigned int flags,
+   enum action_on_err onerr)
 {
struct ref_transaction *t = NULL;
struct strbuf err = STRBUF_INIT;
int ret = 0;
 
if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
+   assert(refs == get_main_ref_store());
ret = write_pseudoref(refname, new_sha1, old_sha1, );
} else {
-   t = ref_transaction_begin();
+   t = ref_store_transaction_begin(refs, );
if (!t ||
ref_transaction_update(t, refname, new_sha1, old_sha1,
   flags, msg, ) ||
@@ -964,6 +985,15 @@ int update_ref(const char *msg, const char *refname,
return 0;
 }
 
+int update_ref(const char *msg, const char *refname,
+  const unsigned char *new_sha1,
+  const unsigned char *old_sha1,
+  unsigned int flags, enum action_on_err onerr)
+{
+   return refs_update_ref(get_main_ref_store(), msg, refname, new_sha1,
+  old_sha1, flags, onerr);
+}
+
 char *shorten_unambiguous_ref(const char *refname, int strict)
 {
int i;
@@ -1600,7 +1630,7 @@ int create_symref(const char *ref_target, const char 
*refs_heads_master,
 int ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err)
 {
-   struct ref_store *refs = get_main_ref_store();
+   struct ref_store *refs = transaction->ref_store;
 
return refs->be->transaction_commit(refs, transaction, err);
 }
@@ -1719,7 +1749,7 @@ int reflog_expire(const char *refname, const unsigned 
char *sha1,
 int initial_ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err)
 {
-   struct ref_store *refs = get_main_ref_store();
+   struct ref_store *refs = 

[PATCH v5 15/24] refs: move submodule code out of files-backend.c

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

files-backend is now initialized with a $GIT_DIR. Converting a submodule
path to where real submodule gitdir is located is done in get_ref_store().

This gives a slight performance improvement for submodules since we
don't convert submodule path to gitdir at every backend call like
before. We pay that once at ref-store creation.

More cleanup in files_downcast() and files_assert_main_repository()
follows shortly. It's separate to keep noises from this patch.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 20 ++--
 refs/files-backend.c | 24 ++--
 refs/refs-internal.h |  9 -
 3 files changed, 20 insertions(+), 33 deletions(-)

diff --git a/refs.c b/refs.c
index 562834fc0..67acae60c 100644
--- a/refs.c
+++ b/refs.c
@@ -9,6 +9,7 @@
 #include "refs/refs-internal.h"
 #include "object.h"
 #include "tag.h"
+#include "submodule.h"
 
 /*
  * List of all available backends
@@ -1413,9 +1414,9 @@ static struct ref_store *lookup_submodule_ref_store(const 
char *submodule)
 
 /*
  * Create, record, and return a ref_store instance for the specified
- * submodule (or the main repository if submodule is NULL).
+ * gitdir.
  */
-static struct ref_store *ref_store_init(const char *submodule)
+static struct ref_store *ref_store_init(const char *gitdir)
 {
const char *be_name = "files";
struct ref_storage_be *be = find_ref_storage_backend(be_name);
@@ -1424,7 +1425,7 @@ static struct ref_store *ref_store_init(const char 
*submodule)
if (!be)
die("BUG: reference backend %s is unknown", be_name);
 
-   refs = be->init(submodule);
+   refs = be->init(gitdir);
return refs;
 }
 
@@ -1435,7 +1436,7 @@ struct ref_store *get_main_ref_store(void)
if (main_ref_store)
return main_ref_store;
 
-   refs = ref_store_init(NULL);
+   refs = ref_store_init(get_git_dir());
if (refs) {
if (main_ref_store)
die("BUG: main_ref_store initialized twice");
@@ -1466,6 +1467,7 @@ struct ref_store *get_ref_store(const char *submodule)
 {
struct strbuf submodule_sb = STRBUF_INIT;
struct ref_store *refs;
+   int ret;
 
if (!submodule || !*submodule) {
return get_main_ref_store();
@@ -1476,8 +1478,14 @@ struct ref_store *get_ref_store(const char *submodule)
return refs;
 
strbuf_addstr(_sb, submodule);
-   if (is_nonbare_repository_dir(_sb))
-   refs = ref_store_init(submodule);
+   ret = is_nonbare_repository_dir(_sb);
+   strbuf_release(_sb);
+   if (!ret)
+   return refs;
+
+   ret = submodule_to_gitdir(_sb, submodule);
+   if (!ret)
+   refs = ref_store_init(submodule_sb.buf);
strbuf_release(_sb);
 
if (refs)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index d80c27837..37443369b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -917,12 +917,6 @@ struct packed_ref_cache {
 struct files_ref_store {
struct ref_store base;
 
-   /*
-* The name of the submodule represented by this object, or
-* NULL if it represents the main repository's reference
-* store:
-*/
-   const char *submodule;
char *gitdir;
char *gitcommondir;
char *packed_refs_path;
@@ -982,22 +976,14 @@ static void clear_loose_ref_cache(struct files_ref_store 
*refs)
  * Create a new submodule ref cache and add it to the internal
  * set of caches.
  */
-static struct ref_store *files_ref_store_create(const char *submodule)
+static struct ref_store *files_ref_store_create(const char *gitdir)
 {
struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
struct ref_store *ref_store = (struct ref_store *)refs;
struct strbuf sb = STRBUF_INIT;
-   const char *gitdir = get_git_dir();
 
base_ref_store_init(ref_store, _be_files);
 
-   if (submodule) {
-   refs->submodule = xstrdup(submodule);
-   refs->packed_refs_path = git_pathdup_submodule(
-   refs->submodule, "packed-refs");
-   return ref_store;
-   }
-
refs->gitdir = xstrdup(gitdir);
get_common_dir_noenv(, gitdir);
refs->gitcommondir = strbuf_detach(, NULL);
@@ -1014,8 +1000,7 @@ static struct ref_store *files_ref_store_create(const 
char *submodule)
 static void files_assert_main_repository(struct files_ref_store *refs,
 const char *caller)
 {
-   if (refs->submodule)
-   die("BUG: %s called for a submodule", caller);
+   /* This function is to be deleted in the next patch */
 }
 
 /*
@@ -1206,11 +1191,6 @@ static void files_refname_path(struct files_ref_store 
*refs,
   struct strbuf *sb,
   const char *refname)
 {
-   if (refs->submodule) {
-

[PATCH v5 14/24] path.c: move some code out of strbuf_git_path_submodule()

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

refs is learning to avoid path rewriting that is done by
strbuf_git_path_submodule(). Factor out this code so it could be reused
by refs*

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 path.c  | 34 +++---
 submodule.c | 31 +++
 submodule.h |  1 +
 3 files changed, 39 insertions(+), 27 deletions(-)

diff --git a/path.c b/path.c
index efcedafba..3451d2916 100644
--- a/path.c
+++ b/path.c
@@ -475,35 +475,16 @@ const char *worktree_git_path(const struct worktree *wt, 
const char *fmt, ...)
 static int do_submodule_path(struct strbuf *buf, const char *path,
 const char *fmt, va_list args)
 {
-   const char *git_dir;
struct strbuf git_submodule_common_dir = STRBUF_INIT;
struct strbuf git_submodule_dir = STRBUF_INIT;
-   const struct submodule *sub;
-   int err = 0;
+   int ret;
 
-   strbuf_addstr(buf, path);
-   strbuf_complete(buf, '/');
-   strbuf_addstr(buf, ".git");
-
-   git_dir = read_gitfile(buf->buf);
-   if (git_dir) {
-   strbuf_reset(buf);
-   strbuf_addstr(buf, git_dir);
-   }
-   if (!is_git_directory(buf->buf)) {
-   gitmodules_config();
-   sub = submodule_from_path(null_sha1, path);
-   if (!sub) {
-   err = SUBMODULE_PATH_ERR_NOT_CONFIGURED;
-   goto cleanup;
-   }
-   strbuf_reset(buf);
-   strbuf_git_path(buf, "%s/%s", "modules", sub->name);
-   }
-
-   strbuf_addch(buf, '/');
-   strbuf_addbuf(_submodule_dir, buf);
+   ret = submodule_to_gitdir(_submodule_dir, path);
+   if (ret)
+   goto cleanup;
 
+   strbuf_complete(_submodule_dir, '/');
+   strbuf_addbuf(buf, _submodule_dir);
strbuf_vaddf(buf, fmt, args);
 
if (get_common_dir_noenv(_submodule_common_dir, 
git_submodule_dir.buf))
@@ -514,8 +495,7 @@ static int do_submodule_path(struct strbuf *buf, const char 
*path,
 cleanup:
strbuf_release(_submodule_dir);
strbuf_release(_submodule_common_dir);
-
-   return err;
+   return ret;
 }
 
 char *git_pathdup_submodule(const char *path, const char *fmt, ...)
diff --git a/submodule.c b/submodule.c
index 3b98766a6..36b8d1d11 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1514,3 +1514,34 @@ void absorb_git_dir_into_superproject(const char *prefix,
strbuf_release();
}
 }
+
+int submodule_to_gitdir(struct strbuf *buf, const char *submodule)
+{
+   const struct submodule *sub;
+   const char *git_dir;
+   int ret = 0;
+
+   strbuf_reset(buf);
+   strbuf_addstr(buf, submodule);
+   strbuf_complete(buf, '/');
+   strbuf_addstr(buf, ".git");
+
+   git_dir = read_gitfile(buf->buf);
+   if (git_dir) {
+   strbuf_reset(buf);
+   strbuf_addstr(buf, git_dir);
+   }
+   if (!is_git_directory(buf->buf)) {
+   gitmodules_config();
+   sub = submodule_from_path(null_sha1, submodule);
+   if (!sub) {
+   ret = -1;
+   goto cleanup;
+   }
+   strbuf_reset(buf);
+   strbuf_git_path(buf, "%s/%s", "modules", sub->name);
+   }
+
+cleanup:
+   return ret;
+}
diff --git a/submodule.h b/submodule.h
index 05ab674f0..fc3d0303e 100644
--- a/submodule.h
+++ b/submodule.h
@@ -81,6 +81,7 @@ extern int push_unpushed_submodules(struct sha1_array 
*commits,
int dry_run);
 extern void connect_work_tree_and_git_dir(const char *work_tree, const char 
*git_dir);
 extern int parallel_submodules(void);
+int submodule_to_gitdir(struct strbuf *buf, const char *submodule);
 
 /*
  * Prepare the "env_array" parameter of a "struct child_process" for executing
-- 
2.11.0.157.gd943d85

[PATCH v5 17/24] refs: rename get_ref_store() to get_submodule_ref_store() and make it public

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

This function is intended to replace *_submodule() refs API. It provides
a ref store for a specific submodule, which can be operated on by a new
set of refs API.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 12 
 refs.h   | 11 +++
 refs/refs-internal.h | 12 
 3 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/refs.c b/refs.c
index 2dc97891a..7843d3085 100644
--- a/refs.c
+++ b/refs.c
@@ -1171,7 +1171,7 @@ int head_ref(each_ref_fn fn, void *cb_data)
 static int do_for_each_ref(const char *submodule, const char *prefix,
   each_ref_fn fn, int trim, int flags, void *cb_data)
 {
-   struct ref_store *refs = get_ref_store(submodule);
+   struct ref_store *refs = get_submodule_ref_store(submodule);
struct ref_iterator *iter;
 
if (!refs)
@@ -1344,10 +1344,10 @@ int resolve_gitlink_ref(const char *submodule, const 
char *refname,
/* We need to strip off one or more trailing slashes */
char *stripped = xmemdupz(submodule, len);
 
-   refs = get_ref_store(stripped);
+   refs = get_submodule_ref_store(stripped);
free(stripped);
} else {
-   refs = get_ref_store(submodule);
+   refs = get_submodule_ref_store(submodule);
}
 
if (!refs)
@@ -1468,13 +1468,17 @@ static void register_submodule_ref_store(struct 
ref_store *refs,
submodule);
 }
 
-struct ref_store *get_ref_store(const char *submodule)
+struct ref_store *get_submodule_ref_store(const char *submodule)
 {
struct strbuf submodule_sb = STRBUF_INIT;
struct ref_store *refs;
int ret;
 
if (!submodule || !*submodule) {
+   /*
+* FIXME: This case is ideally not allowed. But that
+* can't happen until we clean up all the callers.
+*/
return get_main_ref_store();
}
 
diff --git a/refs.h b/refs.h
index 29013cb7e..2efba6ba9 100644
--- a/refs.h
+++ b/refs.h
@@ -561,5 +561,16 @@ int reflog_expire(const char *refname, const unsigned char 
*sha1,
 int ref_storage_backend_exists(const char *name);
 
 struct ref_store *get_main_ref_store(void);
+/*
+ * Return the ref_store instance for the specified submodule. For the
+ * main repository, use submodule==NULL; such a call cannot fail. For
+ * a submodule, the submodule must exist and be a nonbare repository,
+ * otherwise return NULL. If the requested reference store has not yet
+ * been initialized, initialize it first.
+ *
+ * For backwards compatibility, submodule=="" is treated the same as
+ * submodule==NULL.
+ */
+struct ref_store *get_submodule_ref_store(const char *submodule);
 
 #endif /* REFS_H */
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 0cca280b5..f20dde39e 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -646,18 +646,6 @@ struct ref_store {
 void base_ref_store_init(struct ref_store *refs,
 const struct ref_storage_be *be);
 
-/*
- * Return the ref_store instance for the specified submodule. For the
- * main repository, use submodule==NULL; such a call cannot fail. For
- * a submodule, the submodule must exist and be a nonbare repository,
- * otherwise return NULL. If the requested reference store has not yet
- * been initialized, initialize it first.
- *
- * For backwards compatibility, submodule=="" is treated the same as
- * submodule==NULL.
- */
-struct ref_store *get_ref_store(const char *submodule);
-
 const char *resolve_ref_recursively(struct ref_store *refs,
const char *refname,
int resolve_flags,
-- 
2.11.0.157.gd943d85

[PATCH v5 12/24] refs.c: kill register_ref_store(), add register_submodule_ref_store()

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

This is the last function in this code (besides public API) that takes
submodule argument and handles both main/submodule cases. Break it down,
move main store registration in get_main_ref_store() and keep the rest
in register_submodule_ref_store().

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 50 ++
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/refs.c b/refs.c
index c284cb4f4..6adc38e42 100644
--- a/refs.c
+++ b/refs.c
@@ -1412,29 +1412,6 @@ static struct ref_store 
*lookup_submodule_ref_store(const char *submodule)
 }
 
 /*
- * Register the specified ref_store to be the one that should be used
- * for submodule (or the main repository if submodule is NULL). It is
- * a fatal error to call this function twice for the same submodule.
- */
-static void register_ref_store(struct ref_store *refs, const char *submodule)
-{
-   if (!submodule) {
-   if (main_ref_store)
-   die("BUG: main_ref_store initialized twice");
-
-   main_ref_store = refs;
-   } else {
-   if (!submodule_ref_stores.tablesize)
-   hashmap_init(_ref_stores, submodule_hash_cmp, 
0);
-
-   if (hashmap_put(_ref_stores,
-   alloc_submodule_hash_entry(submodule, refs)))
-   die("BUG: ref_store for submodule '%s' initialized 
twice",
-   submodule);
-   }
-}
-
-/*
  * Create, record, and return a ref_store instance for the specified
  * submodule (or the main repository if submodule is NULL).
  */
@@ -1448,7 +1425,6 @@ static struct ref_store *ref_store_init(const char 
*submodule)
die("BUG: reference backend %s is unknown", be_name);
 
refs = be->init(submodule);
-   register_ref_store(refs, submodule);
return refs;
 }
 
@@ -1460,9 +1436,32 @@ static struct ref_store *get_main_ref_store(void)
return main_ref_store;
 
refs = ref_store_init(NULL);
+   if (refs) {
+   if (main_ref_store)
+   die("BUG: main_ref_store initialized twice");
+
+   main_ref_store = refs;
+   }
return refs;
 }
 
+/*
+ * Register the specified ref_store to be the one that should be used
+ * for submodule. It is a fatal error to call this function twice for
+ * the same submodule.
+ */
+static void register_submodule_ref_store(struct ref_store *refs,
+const char *submodule)
+{
+   if (!submodule_ref_stores.tablesize)
+   hashmap_init(_ref_stores, submodule_hash_cmp, 0);
+
+   if (hashmap_put(_ref_stores,
+   alloc_submodule_hash_entry(submodule, refs)))
+   die("BUG: ref_store for submodule '%s' initialized twice",
+   submodule);
+}
+
 struct ref_store *get_ref_store(const char *submodule)
 {
struct strbuf submodule_sb = STRBUF_INIT;
@@ -1480,6 +1479,9 @@ struct ref_store *get_ref_store(const char *submodule)
if (is_nonbare_repository_dir(_sb))
refs = ref_store_init(submodule);
strbuf_release(_sb);
+
+   if (refs)
+   register_submodule_ref_store(refs, submodule);
return refs;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v5 13/24] refs.c: make get_main_ref_store() public and use it

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

get_ref_store() will soon be renamed to get_submodule_ref_store().
Together with future get_worktree_ref_store(), the three functions
provide an appropriate ref store for different operation modes. New APIs
will be added to operate directly on ref stores.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c   | 36 ++--
 refs.h   |  3 +++
 refs/files-backend.c |  2 +-
 3 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/refs.c b/refs.c
index 6adc38e42..562834fc0 100644
--- a/refs.c
+++ b/refs.c
@@ -1314,7 +1314,7 @@ const char *resolve_ref_recursively(struct ref_store 
*refs,
 /* backend functions */
 int refs_init_db(struct strbuf *err)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->init_db(refs, err);
 }
@@ -1322,7 +1322,7 @@ int refs_init_db(struct strbuf *err)
 const char *resolve_ref_unsafe(const char *refname, int resolve_flags,
   unsigned char *sha1, int *flags)
 {
-   return resolve_ref_recursively(get_ref_store(NULL), refname,
+   return resolve_ref_recursively(get_main_ref_store(), refname,
   resolve_flags, sha1, flags);
 }
 
@@ -1428,7 +1428,7 @@ static struct ref_store *ref_store_init(const char 
*submodule)
return refs;
 }
 
-static struct ref_store *get_main_ref_store(void)
+struct ref_store *get_main_ref_store(void)
 {
struct ref_store *refs;
 
@@ -1494,14 +1494,14 @@ void base_ref_store_init(struct ref_store *refs,
 /* backend functions */
 int pack_refs(unsigned int flags)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->pack_refs(refs, flags);
 }
 
 int peel_ref(const char *refname, unsigned char *sha1)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->peel_ref(refs, refname, sha1);
 }
@@ -1509,7 +1509,7 @@ int peel_ref(const char *refname, unsigned char *sha1)
 int create_symref(const char *ref_target, const char *refs_heads_master,
  const char *logmsg)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->create_symref(refs, ref_target, refs_heads_master,
   logmsg);
@@ -1518,7 +1518,7 @@ int create_symref(const char *ref_target, const char 
*refs_heads_master,
 int ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->transaction_commit(refs, transaction, err);
 }
@@ -1528,14 +1528,14 @@ int verify_refname_available(const char *refname,
 const struct string_list *skip,
 struct strbuf *err)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->verify_refname_available(refs, refname, extra, skip, 
err);
 }
 
 int for_each_reflog(each_ref_fn fn, void *cb_data)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
struct ref_iterator *iter;
 
iter = refs->be->reflog_iterator_begin(refs);
@@ -1546,7 +1546,7 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
 int for_each_reflog_ent_reverse(const char *refname, each_reflog_ent_fn fn,
void *cb_data)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->for_each_reflog_ent_reverse(refs, refname,
 fn, cb_data);
@@ -1555,14 +1555,14 @@ int for_each_reflog_ent_reverse(const char *refname, 
each_reflog_ent_fn fn,
 int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn,
void *cb_data)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->for_each_reflog_ent(refs, refname, fn, cb_data);
 }
 
 int reflog_exists(const char *refname)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->reflog_exists(refs, refname);
 }
@@ -1570,14 +1570,14 @@ int reflog_exists(const char *refname)
 int safe_create_reflog(const char *refname, int force_create,
   struct strbuf *err)
 {
-   struct ref_store *refs = get_ref_store(NULL);
+   struct ref_store *refs = get_main_ref_store();
 
return refs->be->create_reflog(refs, refname, force_create, err);
 }
 
 int delete_reflog(const char 

[PATCH v5 09/24] refs.c: introduce get_main_ref_store()

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 81b64b4ed..dab1a21ac 100644
--- a/refs.c
+++ b/refs.c
@@ -1456,15 +1456,23 @@ static struct ref_store *ref_store_init(const char 
*submodule)
return refs;
 }
 
+static struct ref_store *get_main_ref_store(void)
+{
+   struct ref_store *refs;
+
+   if (main_ref_store)
+   return main_ref_store;
+
+   refs = ref_store_init(NULL);
+   return refs;
+}
+
 struct ref_store *get_ref_store(const char *submodule)
 {
struct ref_store *refs;
 
if (!submodule || !*submodule) {
-   refs = lookup_ref_store(NULL);
-
-   if (!refs)
-   refs = ref_store_init(NULL);
+   return get_main_ref_store();
} else {
refs = lookup_ref_store(submodule);
 
-- 
2.11.0.157.gd943d85

[PATCH v5 10/24] refs: rename lookup_ref_store() to lookup_submodule_ref_store()

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

With get_main_ref_store() being used inside get_ref_store(),
lookup_ref_store() is only used for submodule code path. Rename to
reflect that and delete dead code.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 13 -
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/refs.c b/refs.c
index dab1a21ac..8b36d97c0 100644
--- a/refs.c
+++ b/refs.c
@@ -1395,17 +1395,13 @@ static struct ref_store *main_ref_store;
 static struct hashmap submodule_ref_stores;
 
 /*
- * Return the ref_store instance for the specified submodule (or the
- * main repository if submodule is NULL). If that ref_store hasn't
- * been initialized yet, return NULL.
+ * Return the ref_store instance for the specified submodule. If that
+ * ref_store hasn't been initialized yet, return NULL.
  */
-static struct ref_store *lookup_ref_store(const char *submodule)
+static struct ref_store *lookup_submodule_ref_store(const char *submodule)
 {
struct submodule_hash_entry *entry;
 
-   if (!submodule)
-   return main_ref_store;
-
if (!submodule_ref_stores.tablesize)
/* It's initialized on demand in register_ref_store(). */
return NULL;
@@ -1474,7 +1470,7 @@ struct ref_store *get_ref_store(const char *submodule)
if (!submodule || !*submodule) {
return get_main_ref_store();
} else {
-   refs = lookup_ref_store(submodule);
+   refs = lookup_submodule_ref_store(submodule);
 
if (!refs) {
struct strbuf submodule_sb = STRBUF_INIT;
@@ -1485,7 +1481,6 @@ struct ref_store *get_ref_store(const char *submodule)
strbuf_release(_sb);
}
}
-
return refs;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v5 11/24] refs.c: flatten get_ref_store() a bit

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

This helps the future changes in this code. And because get_ref_store()
is destined to become get_submodule_ref_store(), the "get main store"
code path will be removed eventually. After this the patch to delete
that code will be cleaner.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 19 +--
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/refs.c b/refs.c
index 8b36d97c0..c284cb4f4 100644
--- a/refs.c
+++ b/refs.c
@@ -1465,22 +1465,21 @@ static struct ref_store *get_main_ref_store(void)
 
 struct ref_store *get_ref_store(const char *submodule)
 {
+   struct strbuf submodule_sb = STRBUF_INIT;
struct ref_store *refs;
 
if (!submodule || !*submodule) {
return get_main_ref_store();
-   } else {
-   refs = lookup_submodule_ref_store(submodule);
+   }
 
-   if (!refs) {
-   struct strbuf submodule_sb = STRBUF_INIT;
+   refs = lookup_submodule_ref_store(submodule);
+   if (refs)
+   return refs;
 
-   strbuf_addstr(_sb, submodule);
-   if (is_nonbare_repository_dir(_sb))
-   refs = ref_store_init(submodule);
-   strbuf_release(_sb);
-   }
-   }
+   strbuf_addstr(_sb, submodule);
+   if (is_nonbare_repository_dir(_sb))
+   refs = ref_store_init(submodule);
+   strbuf_release(_sb);
return refs;
 }
 
-- 
2.11.0.157.gd943d85

[PATCH v5 05/24] files-backend: move "logs/" out of TMP_RENAMED_LOG

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

This makes reflog path building consistent, always in the form of

strbuf_git_path(sb, "logs/%s", refname);

It reduces the mental workload a bit in the next patch when that
function call is converted.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 435db1293..69946b0de 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2513,7 +2513,7 @@ static int files_delete_refs(struct ref_store *ref_store,
  * IOW, to avoid cross device rename errors, the temporary renamed log must
  * live into logs/refs.
  */
-#define TMP_RENAMED_LOG  "logs/refs/.tmp-renamed-log"
+#define TMP_RENAMED_LOG  "refs/.tmp-renamed-log"
 
 struct rename_cb {
const char *tmp_renamed_log;
@@ -2549,7 +2549,7 @@ static int rename_tmp_log(const char *newrefname)
int ret;
 
strbuf_git_path(, "logs/%s", newrefname);
-   strbuf_git_path(, TMP_RENAMED_LOG);
+   strbuf_git_path(, "logs/%s", TMP_RENAMED_LOG);
cb.tmp_renamed_log = tmp.buf;
ret = raceproof_create_file(path.buf, rename_tmp_log_callback, );
if (ret) {
@@ -2626,12 +2626,12 @@ static int files_rename_ref(struct ref_store *ref_store,
return 1;
 
strbuf_git_path(_oldref, "logs/%s", oldrefname);
-   strbuf_git_path(_renamed_log, TMP_RENAMED_LOG);
+   strbuf_git_path(_renamed_log, "logs/%s", TMP_RENAMED_LOG);
ret = log && rename(sb_oldref.buf, tmp_renamed_log.buf);
strbuf_release(_oldref);
strbuf_release(_renamed_log);
if (ret)
-   return error("unable to move logfile logs/%s to 
"TMP_RENAMED_LOG": %s",
+   return error("unable to move logfile logs/%s to 
logs/"TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
 
if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
@@ -2714,10 +2714,10 @@ static int files_rename_ref(struct ref_store *ref_store,
if (logmoved && rename(sb_newref.buf, sb_oldref.buf))
error("unable to restore logfile %s from %s: %s",
oldrefname, newrefname, strerror(errno));
-   strbuf_git_path(_renamed_log, TMP_RENAMED_LOG);
+   strbuf_git_path(_renamed_log, "logs/%s", TMP_RENAMED_LOG);
if (!logmoved && log &&
rename(tmp_renamed_log.buf, sb_oldref.buf))
-   error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
+   error("unable to restore logfile %s from 
logs/"TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
strbuf_release(_newref);
strbuf_release(_oldref);
-- 
2.11.0.157.gd943d85

[PATCH v5 03/24] files-backend: add and use files_packed_refs_path()

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

Keep repo-related path handling in one place. This will make it easier
to add submodule/multiworktree support later.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 29 ++---
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 1ebd59ec0..4676525de 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -923,6 +923,8 @@ struct files_ref_store {
 */
const char *submodule;
 
+   char *packed_refs_path;
+
struct ref_entry *loose;
struct packed_ref_cache *packed;
 };
@@ -985,7 +987,14 @@ static struct ref_store *files_ref_store_create(const char 
*submodule)
 
base_ref_store_init(ref_store, _be_files);
 
-   refs->submodule = xstrdup_or_null(submodule);
+   if (submodule) {
+   refs->submodule = xstrdup(submodule);
+   refs->packed_refs_path = git_pathdup_submodule(
+   refs->submodule, "packed-refs");
+   return ref_store;
+   }
+
+   refs->packed_refs_path = git_pathdup("packed-refs");
 
return ref_store;
 }
@@ -1153,19 +1162,18 @@ static void read_packed_refs(FILE *f, struct ref_dir 
*dir)
strbuf_release();
 }
 
+static const char *files_packed_refs_path(struct files_ref_store *refs)
+{
+   return refs->packed_refs_path;
+}
+
 /*
  * Get the packed_ref_cache for the specified files_ref_store,
  * creating it if necessary.
  */
 static struct packed_ref_cache *get_packed_ref_cache(struct files_ref_store 
*refs)
 {
-   char *packed_refs_file;
-
-   if (refs->submodule)
-   packed_refs_file = git_pathdup_submodule(refs->submodule,
-"packed-refs");
-   else
-   packed_refs_file = git_pathdup("packed-refs");
+   const char *packed_refs_file = files_packed_refs_path(refs);
 
if (refs->packed &&
!stat_validity_check(>packed->validity, packed_refs_file))
@@ -1184,7 +1192,6 @@ static struct packed_ref_cache 
*get_packed_ref_cache(struct files_ref_store *ref
fclose(f);
}
}
-   free(packed_refs_file);
return refs->packed;
 }
 
@@ -2160,7 +2167,7 @@ static int lock_packed_refs(struct files_ref_store *refs, 
int flags)
}
 
if (hold_lock_file_for_update_timeout(
-   , git_path("packed-refs"),
+   , files_packed_refs_path(refs),
flags, timeout_value) < 0)
return -1;
/*
@@ -2426,7 +2433,7 @@ static int repack_without_refs(struct files_ref_store 
*refs,
return 0; /* no refname exists in packed refs */
 
if (lock_packed_refs(refs, 0)) {
-   unable_to_lock_message(git_path("packed-refs"), errno, err);
+   unable_to_lock_message(files_packed_refs_path(refs), errno, 
err);
return -1;
}
packed = get_packed_refs(refs);
-- 
2.11.0.157.gd943d85

[PATCH v5 06/24] files-backend: add and use files_reflog_path()

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

Keep repo-related path handling in one place. This will make it easier
to add submodule/multiworktree support later.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 148 +++
 1 file changed, 89 insertions(+), 59 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 69946b0de..7b4ea4c56 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -165,7 +165,8 @@ static struct ref_entry *create_dir_entry(struct 
files_ref_store *ref_store,
  const char *dirname, size_t len,
  int incomplete);
 static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry);
-static int files_log_ref_write(const char *refname, const unsigned char 
*old_sha1,
+static int files_log_ref_write(struct files_ref_store *refs,
+  const char *refname, const unsigned char 
*old_sha1,
   const unsigned char *new_sha1, const char *msg,
   int flags, struct strbuf *err);
 
@@ -1167,6 +1168,18 @@ static const char *files_packed_refs_path(struct 
files_ref_store *refs)
return refs->packed_refs_path;
 }
 
+static void files_reflog_path(struct files_ref_store *refs,
+ struct strbuf *sb,
+ const char *refname)
+{
+   if (!refname) {
+   strbuf_git_path(sb, "logs");
+   return;
+   }
+
+   strbuf_git_path(sb, "logs/%s", refname);
+}
+
 /*
  * Get the packed_ref_cache for the specified files_ref_store,
  * creating it if necessary.
@@ -2316,7 +2329,9 @@ enum {
  * subdirs. flags is a combination of REMOVE_EMPTY_PARENTS_REF and/or
  * REMOVE_EMPTY_PARENTS_REFLOG.
  */
-static void try_remove_empty_parents(const char *refname, unsigned int flags)
+static void try_remove_empty_parents(struct files_ref_store *refs,
+const char *refname,
+unsigned int flags)
 {
struct strbuf buf = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
@@ -2348,7 +2363,7 @@ static void try_remove_empty_parents(const char *refname, 
unsigned int flags)
flags &= ~REMOVE_EMPTY_PARENTS_REF;
 
strbuf_reset();
-   strbuf_git_path(, "logs/%s", buf.buf);
+   files_reflog_path(refs, , buf.buf);
if ((flags & REMOVE_EMPTY_PARENTS_REFLOG) && rmdir(sb.buf))
flags &= ~REMOVE_EMPTY_PARENTS_REFLOG;
}
@@ -2541,15 +2556,15 @@ static int rename_tmp_log_callback(const char *path, 
void *cb_data)
}
 }
 
-static int rename_tmp_log(const char *newrefname)
+static int rename_tmp_log(struct files_ref_store *refs, const char *newrefname)
 {
struct strbuf path = STRBUF_INIT;
struct strbuf tmp = STRBUF_INIT;
struct rename_cb cb;
int ret;
 
-   strbuf_git_path(, "logs/%s", newrefname);
-   strbuf_git_path(, "logs/%s", TMP_RENAMED_LOG);
+   files_reflog_path(refs, , newrefname);
+   files_reflog_path(refs, , TMP_RENAMED_LOG);
cb.tmp_renamed_log = tmp.buf;
ret = raceproof_create_file(path.buf, rename_tmp_log_callback, );
if (ret) {
@@ -2609,7 +2624,7 @@ static int files_rename_ref(struct ref_store *ref_store,
int log, ret;
struct strbuf err = STRBUF_INIT;
 
-   strbuf_git_path(_oldref, "logs/%s", oldrefname);
+   files_reflog_path(refs, _oldref, oldrefname);
log = !lstat(sb_oldref.buf, );
strbuf_release(_oldref);
if (log && S_ISLNK(loginfo.st_mode))
@@ -2625,8 +2640,8 @@ static int files_rename_ref(struct ref_store *ref_store,
if (!rename_ref_available(oldrefname, newrefname))
return 1;
 
-   strbuf_git_path(_oldref, "logs/%s", oldrefname);
-   strbuf_git_path(_renamed_log, "logs/%s", TMP_RENAMED_LOG);
+   files_reflog_path(refs, _oldref, oldrefname);
+   files_reflog_path(refs, _renamed_log, TMP_RENAMED_LOG);
ret = log && rename(sb_oldref.buf, tmp_renamed_log.buf);
strbuf_release(_oldref);
strbuf_release(_renamed_log);
@@ -2667,7 +2682,7 @@ static int files_rename_ref(struct ref_store *ref_store,
}
}
 
-   if (log && rename_tmp_log(newrefname))
+   if (log && rename_tmp_log(refs, newrefname))
goto rollback;
 
logmoved = log;
@@ -2709,12 +2724,12 @@ static int files_rename_ref(struct ref_store *ref_store,
log_all_ref_updates = flag;
 
  rollbacklog:
-   strbuf_git_path(_newref, "logs/%s", newrefname);
-   strbuf_git_path(_oldref, "logs/%s", oldrefname);
+   files_reflog_path(refs, _newref, newrefname);
+   files_reflog_path(refs, _oldref, oldrefname);
if (logmoved && rename(sb_newref.buf, sb_oldref.buf))
error("unable to restore logfile %s 

[PATCH v5 07/24] files-backend: add and use files_refname_path()

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

Keep repo-related path handling in one place. This will make it easier
to add submodule/multiworktree support later.

This automatically adds the "if submodule then use the submodule version
of git_path" to other call sites too. But it does not mean those
operations are sumodule-ready. Not yet.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 45 +
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7b4ea4c56..72f4e1746 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1180,6 +1180,18 @@ static void files_reflog_path(struct files_ref_store 
*refs,
strbuf_git_path(sb, "logs/%s", refname);
 }
 
+static void files_refname_path(struct files_ref_store *refs,
+  struct strbuf *sb,
+  const char *refname)
+{
+   if (refs->submodule) {
+   strbuf_git_path_submodule(sb, refs->submodule, "%s", refname);
+   return;
+   }
+
+   strbuf_git_path(sb, "%s", refname);
+}
+
 /*
  * Get the packed_ref_cache for the specified files_ref_store,
  * creating it if necessary.
@@ -1251,10 +1263,7 @@ static void read_loose_refs(const char *dirname, struct 
ref_dir *dir)
size_t path_baselen;
int err = 0;
 
-   if (refs->submodule)
-   err = strbuf_git_path_submodule(, refs->submodule, "%s", 
dirname);
-   else
-   strbuf_git_path(, "%s", dirname);
+   files_refname_path(refs, , dirname);
path_baselen = path.len;
 
if (err) {
@@ -1396,10 +1405,7 @@ static int files_read_raw_ref(struct ref_store 
*ref_store,
*type = 0;
strbuf_reset(_path);
 
-   if (refs->submodule)
-   strbuf_git_path_submodule(_path, refs->submodule, "%s", 
refname);
-   else
-   strbuf_git_path(_path, "%s", refname);
+   files_refname_path(refs, _path, refname);
 
path = sb_path.buf;
 
@@ -1587,7 +1593,7 @@ static int lock_raw_ref(struct files_ref_store *refs,
*lock_p = lock = xcalloc(1, sizeof(*lock));
 
lock->ref_name = xstrdup(refname);
-   strbuf_git_path(_file, "%s", refname);
+   files_refname_path(refs, _file, refname);
 
 retry:
switch (safe_create_leading_directories(ref_file.buf)) {
@@ -2059,7 +2065,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct 
files_ref_store *refs,
if (flags & REF_DELETING)
resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
 
-   strbuf_git_path(_file, "%s", refname);
+   files_refname_path(refs, _file, refname);
resolved = !!resolve_ref_unsafe(refname, resolve_flags,
lock->old_oid.hash, type);
if (!resolved && errno == EISDIR) {
@@ -2358,7 +2364,7 @@ static void try_remove_empty_parents(struct 
files_ref_store *refs,
strbuf_setlen(, q - buf.buf);
 
strbuf_reset();
-   strbuf_git_path(, "%s", buf.buf);
+   files_refname_path(refs, , buf.buf);
if ((flags & REMOVE_EMPTY_PARENTS_REF) && rmdir(sb.buf))
flags &= ~REMOVE_EMPTY_PARENTS_REF;
 
@@ -2668,7 +2674,7 @@ static int files_rename_ref(struct ref_store *ref_store,
struct strbuf path = STRBUF_INIT;
int result;
 
-   strbuf_git_path(, "%s", newrefname);
+   files_refname_path(refs, , newrefname);
result = remove_empty_directories();
strbuf_release();
 
@@ -3901,7 +3907,7 @@ static int files_transaction_commit(struct ref_store 
*ref_store,
update->type & REF_ISSYMREF) {
/* It is a loose reference. */
strbuf_reset();
-   strbuf_git_path(, "%s", lock->ref_name);
+   files_refname_path(refs, , lock->ref_name);
if (unlink_or_msg(sb.buf, err)) {
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
@@ -4201,25 +4207,24 @@ static int files_reflog_expire(struct ref_store 
*ref_store,
 
 static int files_init_db(struct ref_store *ref_store, struct strbuf *err)
 {
+   struct files_ref_store *refs =
+   files_downcast(ref_store, 0, "init_db");
struct strbuf sb = STRBUF_INIT;
 
-   /* Check validity (but we don't need the result): */
-   files_downcast(ref_store, 0, "init_db");
-
/*
 * Create .git/refs/{heads,tags}
 */
-   strbuf_git_path(, "refs/heads");
+   files_refname_path(refs, , "refs/heads");
safe_create_dir(sb.buf, 1);
strbuf_reset();
-   strbuf_git_path(, "refs/tags");
+   files_refname_path(refs, , "refs/tags");

[PATCH v5 08/24] files-backend: remove the use of git_path()

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

Given $GIT_DIR and $GIT_COMMON_DIR, files-backend is now in charge of
deciding what goes where (*). The end goal is to pass $GIT_DIR only. A
refs "view" of a linked worktree is a logical ref store that combines
two files backends together.

(*) Not entirely true since strbuf_git_path_submodule() still does path
translation underneath. But that's for another patch.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 43 ++-
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 72f4e1746..a390eaadf 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -923,7 +923,8 @@ struct files_ref_store {
 * store:
 */
const char *submodule;
-
+   char *gitdir;
+   char *gitcommondir;
char *packed_refs_path;
 
struct ref_entry *loose;
@@ -985,6 +986,8 @@ static struct ref_store *files_ref_store_create(const char 
*submodule)
 {
struct files_ref_store *refs = xcalloc(1, sizeof(*refs));
struct ref_store *ref_store = (struct ref_store *)refs;
+   struct strbuf sb = STRBUF_INIT;
+   const char *gitdir = get_git_dir();
 
base_ref_store_init(ref_store, _be_files);
 
@@ -995,7 +998,11 @@ static struct ref_store *files_ref_store_create(const char 
*submodule)
return ref_store;
}
 
-   refs->packed_refs_path = git_pathdup("packed-refs");
+   refs->gitdir = xstrdup(gitdir);
+   get_common_dir_noenv(, gitdir);
+   refs->gitcommondir = strbuf_detach(, NULL);
+   strbuf_addf(, "%s/packed-refs", refs->gitcommondir);
+   refs->packed_refs_path = strbuf_detach(, NULL);
 
return ref_store;
 }
@@ -1173,11 +1180,26 @@ static void files_reflog_path(struct files_ref_store 
*refs,
  const char *refname)
 {
if (!refname) {
-   strbuf_git_path(sb, "logs");
+   /*
+* FIXME: of course this is wrong in multi worktree
+* setting. To be fixed real soon.
+*/
+   strbuf_addf(sb, "%s/logs", refs->gitcommondir);
return;
}
 
-   strbuf_git_path(sb, "logs/%s", refname);
+   switch (ref_type(refname)) {
+   case REF_TYPE_PER_WORKTREE:
+   case REF_TYPE_PSEUDOREF:
+   strbuf_addf(sb, "%s/logs/%s", refs->gitdir, refname);
+   break;
+   case REF_TYPE_NORMAL:
+   strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname);
+   break;
+   default:
+   die("BUG: unknown ref type %d of ref %s",
+   ref_type(refname), refname);
+   }
 }
 
 static void files_refname_path(struct files_ref_store *refs,
@@ -1189,7 +1211,18 @@ static void files_refname_path(struct files_ref_store 
*refs,
return;
}
 
-   strbuf_git_path(sb, "%s", refname);
+   switch (ref_type(refname)) {
+   case REF_TYPE_PER_WORKTREE:
+   case REF_TYPE_PSEUDOREF:
+   strbuf_addf(sb, "%s/%s", refs->gitdir, refname);
+   break;
+   case REF_TYPE_NORMAL:
+   strbuf_addf(sb, "%s/%s", refs->gitcommondir, refname);
+   break;
+   default:
+   die("BUG: unknown ref type %d of ref %s",
+   ref_type(refname), refname);
+   }
 }
 
 /*
-- 
2.11.0.157.gd943d85

[PATCH v5 04/24] files-backend: convert git_path() to strbuf_git_path()

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

git_path() and friends are going to be killed in files-backend.c in near
future. And because there's a risk with overwriting buffer in
git_path(), let's convert them all to strbuf_git_path(). We'll have
easier time killing/converting strbuf_git_path() then because we won't
have to worry about memory management again.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 139 +++
 1 file changed, 106 insertions(+), 33 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 4676525de..435db1293 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2319,6 +2319,7 @@ enum {
 static void try_remove_empty_parents(const char *refname, unsigned int flags)
 {
struct strbuf buf = STRBUF_INIT;
+   struct strbuf sb = STRBUF_INIT;
char *p, *q;
int i;
 
@@ -2340,14 +2341,19 @@ static void try_remove_empty_parents(const char 
*refname, unsigned int flags)
if (q == p)
break;
strbuf_setlen(, q - buf.buf);
-   if ((flags & REMOVE_EMPTY_PARENTS_REF) &&
-   rmdir(git_path("%s", buf.buf)))
+
+   strbuf_reset();
+   strbuf_git_path(, "%s", buf.buf);
+   if ((flags & REMOVE_EMPTY_PARENTS_REF) && rmdir(sb.buf))
flags &= ~REMOVE_EMPTY_PARENTS_REF;
-   if ((flags & REMOVE_EMPTY_PARENTS_REFLOG) &&
-   rmdir(git_path("logs/%s", buf.buf)))
+
+   strbuf_reset();
+   strbuf_git_path(, "logs/%s", buf.buf);
+   if ((flags & REMOVE_EMPTY_PARENTS_REFLOG) && rmdir(sb.buf))
flags &= ~REMOVE_EMPTY_PARENTS_REFLOG;
}
strbuf_release();
+   strbuf_release();
 }
 
 /* make sure nobody touched the ref, and unlink */
@@ -2509,11 +2515,16 @@ static int files_delete_refs(struct ref_store 
*ref_store,
  */
 #define TMP_RENAMED_LOG  "logs/refs/.tmp-renamed-log"
 
-static int rename_tmp_log_callback(const char *path, void *cb)
+struct rename_cb {
+   const char *tmp_renamed_log;
+   int true_errno;
+};
+
+static int rename_tmp_log_callback(const char *path, void *cb_data)
 {
-   int *true_errno = cb;
+   struct rename_cb *cb = cb_data;
 
-   if (rename(git_path(TMP_RENAMED_LOG), path)) {
+   if (rename(cb->tmp_renamed_log, path)) {
/*
 * rename(a, b) when b is an existing directory ought
 * to result in ISDIR, but Solaris 5.8 gives ENOTDIR.
@@ -2521,7 +2532,7 @@ static int rename_tmp_log_callback(const char *path, void 
*cb)
 * but report EISDIR to raceproof_create_file() so
 * that it knows to retry.
 */
-   *true_errno = errno;
+   cb->true_errno = errno;
if (errno == ENOTDIR)
errno = EISDIR;
return -1;
@@ -2532,20 +2543,26 @@ static int rename_tmp_log_callback(const char *path, 
void *cb)
 
 static int rename_tmp_log(const char *newrefname)
 {
-   char *path = git_pathdup("logs/%s", newrefname);
-   int ret, true_errno;
+   struct strbuf path = STRBUF_INIT;
+   struct strbuf tmp = STRBUF_INIT;
+   struct rename_cb cb;
+   int ret;
 
-   ret = raceproof_create_file(path, rename_tmp_log_callback, _errno);
+   strbuf_git_path(, "logs/%s", newrefname);
+   strbuf_git_path(, TMP_RENAMED_LOG);
+   cb.tmp_renamed_log = tmp.buf;
+   ret = raceproof_create_file(path.buf, rename_tmp_log_callback, );
if (ret) {
if (errno == EISDIR)
-   error("directory not empty: %s", path);
+   error("directory not empty: %s", path.buf);
else
error("unable to move logfile %s to %s: %s",
- git_path(TMP_RENAMED_LOG), path,
- strerror(true_errno));
+ tmp.buf, path.buf,
+ strerror(cb.true_errno));
}
 
-   free(path);
+   strbuf_release();
+   strbuf_release();
return ret;
 }
 
@@ -2586,9 +2603,15 @@ static int files_rename_ref(struct ref_store *ref_store,
int flag = 0, logmoved = 0;
struct ref_lock *lock;
struct stat loginfo;
-   int log = !lstat(git_path("logs/%s", oldrefname), );
+   struct strbuf sb_oldref = STRBUF_INIT;
+   struct strbuf sb_newref = STRBUF_INIT;
+   struct strbuf tmp_renamed_log = STRBUF_INIT;
+   int log, ret;
struct strbuf err = STRBUF_INIT;
 
+   strbuf_git_path(_oldref, "logs/%s", oldrefname);
+   log = !lstat(sb_oldref.buf, );
+   strbuf_release(_oldref);
if (log && S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldrefname);
 
@@ -2602,7 +2625,12 @@ static int files_rename_ref(struct 

[PATCH v5 02/24] files-backend: make files_log_ref_write() static

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

Created in 5f3c3a4e6f (files_log_ref_write: new function - 2015-11-10)
but probably never used outside refs-internal.c

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs/files-backend.c | 3 +++
 refs/refs-internal.h | 4 
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index db3bd42a9..1ebd59ec0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -165,6 +165,9 @@ static struct ref_entry *create_dir_entry(struct 
files_ref_store *ref_store,
  const char *dirname, size_t len,
  int incomplete);
 static void add_entry_to_dir(struct ref_dir *dir, struct ref_entry *entry);
+static int files_log_ref_write(const char *refname, const unsigned char 
*old_sha1,
+  const unsigned char *new_sha1, const char *msg,
+  int flags, struct strbuf *err);
 
 static struct ref_dir *get_ref_dir(struct ref_entry *entry)
 {
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index fa93c9a32..f732473e1 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -228,10 +228,6 @@ struct ref_transaction {
enum ref_transaction_state state;
 };
 
-int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
-   const unsigned char *new_sha1, const char *msg,
-   int flags, struct strbuf *err);
-
 /*
  * Check for entries in extras that are within the specified
  * directory, where dirname is a reference directory name including
-- 
2.11.0.157.gd943d85

[PATCH v5 01/24] refs.h: add forward declaration for structs used in this file

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.h | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/refs.h b/refs.h
index 9fbff90e7..c494b641a 100644
--- a/refs.h
+++ b/refs.h
@@ -1,6 +1,11 @@
 #ifndef REFS_H
 #define REFS_H
 
+struct object_id;
+struct ref_transaction;
+struct strbuf;
+struct string_list;
+
 /*
  * Resolve a reference, recursively following symbolic refererences.
  *
@@ -144,7 +149,6 @@ int dwim_log(const char *str, int len, unsigned char *sha1, 
char **ref);
  * `ref_transaction_commit` is called.  So `ref_transaction_verify`
  * won't report a verification failure until the commit is attempted.
  */
-struct ref_transaction;
 
 /*
  * Bit values set in the flags argument passed to each_ref_fn() and
-- 
2.11.0.157.gd943d85

[PATCH v5 00/24] Remove submodule from files-backend.c

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

v5 goes a bit longer than v4, basically:

 - files_path() is broken down into three smaller functions,
   files_{packed_refs,reflog,refname}_path().

 - most of store-based api is added because..

 - test-ref-store.c is added with t1405 and t1406 for some basic tests
   I'm not aimimg for complete ref store coverage. But we can continue
   to improve from there.

 - refs_store_init() now takes a "permission" flag, like open().
   Operations are allowed or forbidden based on this flag. The
   submodule_allowed flag is killed. files_assert_main.. remains.

 - get_*_ref_store() remain public api because it's used by
   test-ref-store.c and pack-refs.c.

 - files-backend.c should now make no function calls that implicitly
   target the main store. But this will have to be tested more to be
   sure. I'm tempted to add a tracing backend just for this purpose.

Junio, if you take this on 'pu', you'll have to kick my other two
series out (they should not even compile). I'm not resending them
until I get a "looks mostly ok" from Michael. No point in updating
them when this series keeps moving.

This series is also available on my github repo. branch
files-backend-git-dir-2.

Nguyễn Thái Ngọc Duy (24):
  refs.h: add forward declaration for structs used in this file
  files-backend: make files_log_ref_write() static
  files-backend: add and use files_packed_refs_path()
  files-backend: convert git_path() to strbuf_git_path()
  files-backend: move "logs/" out of TMP_RENAMED_LOG
  files-backend: add and use files_reflog_path()
  files-backend: add and use files_refname_path()
  files-backend: remove the use of git_path()
  refs.c: introduce get_main_ref_store()
  refs: rename lookup_ref_store() to lookup_submodule_ref_store()
  refs.c: flatten get_ref_store() a bit
  refs.c: kill register_ref_store(), add register_submodule_ref_store()
  refs.c: make get_main_ref_store() public and use it
  path.c: move some code out of strbuf_git_path_submodule()
  refs: move submodule code out of files-backend.c
  files-backend: replace submodule_allowed check in files_downcast()
  refs: rename get_ref_store() to get_submodule_ref_store() and make it public
  refs: add new ref-store api
  refs: new transaction related ref-store api
  files-backend: avoid ref api targetting main ref store
  refs: delete pack_refs() in favor of refs_pack_refs()
  t/helper: add test-ref-store to test ref-store functions
  t1405: some basic tests on main ref store
  t1406: new tests for submodule ref store

 Makefile|   1 +
 builtin/pack-refs.c |   2 +-
 path.c  |  34 +--
 refs.c  | 411 ++
 refs.h  | 100 ++-
 refs/files-backend.c| 509 +---
 refs/refs-internal.h|  64 +---
 submodule.c |  31 ++
 submodule.h |   1 +
 t/helper/.gitignore |   1 +
 t/helper/test-ref-store.c (new) | 274 +
 t/t1405-main-ref-store.sh (new +x)  | 123 
 t/t1406-submodule-ref-store.sh (new +x) |  95 ++
 13 files changed, 1269 insertions(+), 377 deletions(-)
 create mode 100644 t/helper/test-ref-store.c
 create mode 100755 t/t1405-main-ref-store.sh
 create mode 100755 t/t1406-submodule-ref-store.sh

-- 
2.11.0.157.gd943d85

Re: feature request: user email config per domain

[Pranit Bauva]

Hey Tushar,

When you run `git config --global user.email a...@xyz.com` it writes
out this setting in ~/.gitconfig which is then considered to be
"global" ie. this same email will be used for every repo, but ...

You can always override this. Let's say there is a repo named "foo" in
which you want the email "x...@abc.com", then you go inside that folder
and type `git config user.email x...@abc.com`. Note: the option
`--global` is skipped here. This creates a file `.gitconfig` in the
folder "foo" and now whenever you commit, the .gitconfig file inside
the repo will get the first preference and then the global
~/.gitconfig.

This will work for you assuming that you have different repos for your
company and for your open source work. Will this solve your problem?

Regards,
Pranit Bauva

feature request: user email config per domain

[Tushar Kapila]

I can set my email via:
git config --global user.email tgkp...@xyz.dom

this is dangerous when I use this my office or in a multi repository
provider environment where my email is different for a few (like
tgkp...@search.com for github and tus...@mycompany.com for my company
private repo). I know I can over ride it per repository, but sometimes
forget to do that. And even if I unset it, it inadvertantly gets set
elsewhere when I make a repo and the site 'helps' me by showing me the
commands to init and clone my new repo.
I did an analysis on a bunch of company git repositories using jgit
(only master branch), and we have 57 emails out of 346 which are not
the company email. Also in there are cases when name is the same but
some commits are by email 1 and others by email 2, because of this
global config. As some of us work on open source and company repos on
the same computer.

Feature request :  can we have a config for email per repo domain ?
Something like:

git config --global domain.user.email tgkp...@test.xyz.com
testing.abc.doman:8080

git config --global domain.user.email tgkp...@xyz.com abc.doman:80

git config --global domain.user.email tgkp...@search.com github.com

So when remote URL has github.com push as tgkp...@search.com but for
testing.abc.doman:8080 use tgkp...@test.xyz.com ?

For me one name is enough. But can do the same for name if others need it?

Thank you.

Regards
Tushar Kapila

[PATCH] Documentation: correctly spell git worktree --detach

[brian m. carlson]

The option is “--detach”, but we accidentally spelled it “--detached” at
one point in the man page.

Signed-off-by: brian m. carlson 
Reported-by: Casey Rodarmor 
---
 Documentation/git-worktree.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index e257c19ebe..553cf8413f 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -52,7 +52,7 @@ is linked to the current repository, sharing everything 
except working
 directory specific files such as HEAD, index, etc. `-` may also be
 specified as ``; it is synonymous with `@{-1}`.
 +
-If `` is omitted and neither `-b` nor `-B` nor `--detached` used,
+If `` is omitted and neither `-b` nor `-B` nor `--detach` used,
 then, as a convenience, a new branch based at HEAD is created automatically,
 as if `-b $(basename )` was specified.
 

Re: Fwd: Typo in worktree man page

[brian m. carlson]

On Tue, Feb 21, 2017 at 04:52:11PM -0800, Casey Rodarmor wrote:
> Hi there,
> 
> The git worktree man page mentions the `--detached` flag in the
> section on `add`, but the flag is actually called `--detach`.

Thanks for reporting this.  I'll send a patch.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

[PATCH] Documentation: use brackets for optional arguments

[brian m. carlson]

The documentation for git blame used vertical bars for optional
arguments to -M and -C, which is unusual and potentially confusing.
Since most man pages use brackets for optional items, and that's
consistent with how we document the same options for git diff and
friends, use brackets here, too.

Signed-off-by: brian m. carlson 
---
 Documentation/blame-options.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 2669b87c9d..dc41957afa 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -77,7 +77,7 @@ include::line-range-format.txt[]
terminal. Can't use `--progress` together with `--porcelain`
or `--incremental`.
 
--M||::
+-M[]::
Detect moved or copied lines within a file. When a commit
moves or copies a block of lines (e.g. the original file
has A and then B, and the commit changes it to B and then
@@ -93,7 +93,7 @@ alphanumeric characters that Git must detect as moving/copying
 within a file for it to associate those lines with the parent
 commit. The default value is 20.
 
--C||::
+-C[]::
In addition to `-M`, detect lines moved or copied from other
files that were modified in the same commit.  This is
useful when you reorganize your program and move code