Re: [PATCH v3 6/6] Use the early config machinery to expand aliases

[Jeff King]

On Tue, Jun 13, 2017 at 11:26:06AM -0700, Brandon Williams wrote:

> So because I've been looking at the config machinery lately, I've
> noticed a lot of issues with how things are handled with respect to
> gitdir vs commondir.  Essentially the config resides at commondir/config
> always, and only at gitdir/config when not working with a worktree.
> Because of this, your patches point out a bug in how early config is
> handled.  I'll illustrate this using aliases.
> 
> Before this series (because aliases are read using the standard config
> machinery):
> 
>   > git init main
>   > git -C main config alias.test '!echo hello'
>   > git -C main test
> hello
>   > git -C main worktree add ../worktree
>   > git -C worktree test
> hello
> 
> After this series (using read_early_config()):
> 
>   > git init main
>   > git -C main config alias.test '!echo hello'
>   > git -C main test
> hello
>   > git -C main worktree add ../worktree
>   > git -C worktree test
> git: 'test' is not a git command. See 'git --help'.
> 
> The issue is that read_early_config passes the gitdir and not the
> commondir when reading the config.

Good catch.

> The solution would be to add a 'commondir' field to the config_options
> struct and populate that before reading the config.  I'm planning on
> fixing this in v2 of my config cleanup series which I'll hopefully have
> finished by the end of the day.

I think that read_early_config() really meant to set the commondir, as
it was always about actual config-file lookup. So it was sort-of buggy
already, though I suspect it was pretty hard to trigger.

But I agree that since include_by_gitdir now reads the same struct
field, swapping it out fixes the config-reading at the cost of breaking
that function. And we really need to pass both in.

I'm not sure whether we should care about this for Dscho's series or
not. I think his series _does_ make the problem easier to trigger. But
it's a minor enough bug that I think I'd be OK with letting your
solution proceed independently.

-Peff

[PATCH] configure.ac: loosen FREAD_READS_DIRECTORIES test program

[Jeff King]

On Wed, Jun 14, 2017 at 01:15:44AM -0400, Jeff King wrote:

> > I couldn't reproduce either with my usual build, but I don't usually use
> > autoconf. Running:
> > 
> >   make configure
> >   ./configure
> >   make
> >   (cd t && ./t1308-*)
> > 
> > does fail for me. The problem is that the generated config.mak.autogen
> > sets the wrong value for FREAD_READS_DIRECTORIES (and overrides the
> > default entry for Linux from config.mak.uname. So the configure script
> > needs to be fixed.
> 
> Actually, I'm not sure if configure.ac is wrong, or the new uses of
> FREAD_READS_DIRECTORIES. Because the test configure.ac actually checks:

Poking around more, I think the best thing is to just update the
configure script. The rationale is below.

-- >8 --
Subject: [PATCH] configure.ac: loosen FREAD_READS_DIRECTORIES test program

We added an FREAD_READS_DIRECTORIES Makefile knob long ago
in cba22528f (Add compat/fopen.c which returns NULL on
attempt to open directory, 2008-02-08) to handle systems
where reading from a directory returned garbage. This works
by catching the problem at the fopen() stage and returning
NULL.

More recently, we found that there is a class of systems
(including Linux) where fopen() succeeds but fread() fails.
Since the solution is the same (having fopen return NULL),
they use the same Makefile knob as of e2d90fd1c
(config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and
FreeBSD, 2017-05-03).

This works fine except for one thing: the autoconf test in
configure.ac to set FREAD_READS_DIRECTORIES actually checks
whether fread succeeds. Which means that on Linux systems,
the knob isn't set (and we even override the config.mak.uname
default). t1308 catches the failure.

We can fix this by tweaking the autoconf test to cover both
cases. In theory we might care about the distinction between
the traditional "fread reads directories" case and the new
"fopen opens directories". But since our solution catches
the problem at the fopen stage either way, we don't actually
need to know the difference. The "fopen" case is a superset.

This does mean the FREAD_READS_DIRECTORIES name is slightly
misleading. Probably FOPEN_OPENS_DIRECTORIES would be more
accurate. But it would be disruptive to simply change the
name (people's existing build configs would fail), and it's
not worth the complexity of handling both. Let's just add a
comment in the knob description.

Reported-by: Øyvind A. Holm 
Signed-off-by: Jeff King 
---
 Makefile | 3 ++-
 configure.ac | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 7c621f7f7..33b888730 100644
--- a/Makefile
+++ b/Makefile
@@ -19,7 +19,8 @@ all::
 # have been written to the final string if enough space had been available.
 #
 # Define FREAD_READS_DIRECTORIES if you are on a system which succeeds
-# when attempting to read from an fopen'ed directory.
+# when attempting to read from an fopen'ed directory (or even to fopen
+# it at all).
 #
 # Define NO_OPENSSL environment variable if you do not have OpenSSL.
 # This also implies BLK_SHA1.
diff --git a/configure.ac b/configure.ac
index deeb968da..602383ed1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -869,9 +869,9 @@ AC_CACHE_CHECK([whether system succeeds to read fopen'ed 
directory],
 [
 AC_RUN_IFELSE(
[AC_LANG_PROGRAM([AC_INCLUDES_DEFAULT],
-   [[char c;
+   [[
FILE *f = fopen(".", "r");
-   return f && fread(, 1, 1, f)]])],
+   return f)]])],
[ac_cv_fread_reads_directories=no],
[ac_cv_fread_reads_directories=yes])
 ])
-- 
2.13.1.766.g6bea926c5

Re: t1308-config-set.sh fails on current master

[Jeff King]

On Wed, Jun 14, 2017 at 01:02:15AM -0400, Jeff King wrote:

> On Wed, Jun 14, 2017 at 04:17:40AM +0200, Øyvind A. Holm wrote:
> 
> > > Interesting.  I'm not able to reproduce it, but of course that doesn't
> > > mean much.
> > 
> > I'll admit that I have a somewhat special build system, but it's been 
> > working great since I created it 7 months ago, and I run the test suite 
> > every time I install a new git. I'm using the Makefile located at
> > 
> >   https://gitlab.com/sunny256/src-other/blob/master/devel/git/Makefile
> > 
> > It's only doing regular stuff like "make configure", "./configure", etc, 
> > but I'm mentioning it in case the Makefile reveals something 
> > interesting. The git installation is in a non-standard location, the 
> > newest version of git I've installed is for example located under 
> > /usr/src-other/pool/git.master.v2.13.1-394-g41dd4330a121/ .
> 
> I couldn't reproduce either with my usual build, but I don't usually use
> autoconf. Running:
> 
>   make configure
>   ./configure
>   make
>   (cd t && ./t1308-*)
> 
> does fail for me. The problem is that the generated config.mak.autogen
> sets the wrong value for FREAD_READS_DIRECTORIES (and overrides the
> default entry for Linux from config.mak.uname. So the configure script
> needs to be fixed.

Actually, I'm not sure if configure.ac is wrong, or the new uses of
FREAD_READS_DIRECTORIES. Because the test configure.ac actually checks:

  FILE *f = fopen(".", "r");
  return f && fread(, 1, 1, f);

I.e., it sees that not only do we fopen() a directory, but we actually
read garbage from it. Whereas on Linux, we fopen the file and then the
read gets EISDIR.

So it's not true that FREAD_READS_DIRECTORIES; this is more like
FOPEN_OPENS_DIRECTORIES.

Just looking at how the macro is used, I think we want to handle both
cases the same (by doing an fstat check after fopen). So I think it
would be OK to continue to use FREAD_READS_DIRECTORIES for both cases,
and just fix the configure script. It may be worth updating the macro
name for clarity, though.

-Peff

Re: t1308-config-set.sh fails on current master

[Jeff King]

On Wed, Jun 14, 2017 at 04:17:40AM +0200, Øyvind A. Holm wrote:

> > Interesting.  I'm not able to reproduce it, but of course that doesn't
> > mean much.
> 
> I'll admit that I have a somewhat special build system, but it's been 
> working great since I created it 7 months ago, and I run the test suite 
> every time I install a new git. I'm using the Makefile located at
> 
>   https://gitlab.com/sunny256/src-other/blob/master/devel/git/Makefile
> 
> It's only doing regular stuff like "make configure", "./configure", etc, 
> but I'm mentioning it in case the Makefile reveals something 
> interesting. The git installation is in a non-standard location, the 
> newest version of git I've installed is for example located under 
> /usr/src-other/pool/git.master.v2.13.1-394-g41dd4330a121/ .

I couldn't reproduce either with my usual build, but I don't usually use
autoconf. Running:

  make configure
  ./configure
  make
  (cd t && ./t1308-*)

does fail for me. The problem is that the generated config.mak.autogen
sets the wrong value for FREAD_READS_DIRECTORIES (and overrides the
default entry for Linux from config.mak.uname. So the configure script
needs to be fixed.

-Peff

Re: [PATCH v2 4/6] config: don't implicitly use gitdir

[Jacob Keller]

On Tue, Jun 13, 2017 at 3:05 PM, Jonathan Nieder  wrote:
> Junio C Hamano wrote:
>> On Tue, Jun 13, 2017 at 2:51 PM, Jonathan Nieder  wrote:
>
>>> What is the next step, then?  You can find the notion ridiculous but
>>> it's how this project has worked in my experience (and how other
>>> projects with similar patch-based workflows work).
>>
>> Does "patch-based" have much to do with this? I agree that distributed
>> nature of the development would bring this issue, but I tend to think that
>> using merge/pull based workflow would not alleviate it--am I mistaken?
>
> Thanks, you're right.  Distributed is the relevant feature.
>
> The same issue can even come up when using a centralized version
> control system like Subversion or Perforce --- without attention to
> API compatibility, someone's change that was thoroughly reviewed and
> well tested locally in a developer's working directory can introduce
> subtle breakage once they run "svn commit", causing it to merge with
> the latest upstream changes.  The problem becomes more likely the more
> distributed a project is since each developer becomes less aware of
> the other changes that their modifications need to be compatible with.
>
> Jonathan

Which would be why early integration (pu) and a good test suite are
for. However, it's much easier to catch if you change the name when
the function no longer behaves the same. In this case I guess you
could argue that the part that changed wasn't part of the API...
However, I think I'd lean towards "we had code depending on it, so yes
it was".

It's ok to add new functionality only obtained by new flags or
similar,  but not ok to break potentially existing callers.

Thanks,
Jake

Re: [PATCH v3] doc: do not use `rm .git/index` when normalizing line endings

[Torsten Bögershausen]

On 14/06/17 00:15, Andreas Heiduk wrote:
Looks good to me, one minor typo below


When illustrating how to normalize the line endings, the
documentation in gitattributes tells the user to `rm .git/index`.

This is incorrect for two reasons:

  - Users shouldn't be instructed to mess around with the internal
implementation of Git using raw file system tools like `rm`.

  - Within a submodule or an additional working tree `.git` is just a
file containing a `gitdir: ` pointer into the real `.git`
directory.  Therefore `rm .git/index` does not work.

The purpose or `rm .git/index` instruction is to remove all entries

  ^^

from the index without touching the working tree.  The way to do this
with Git is to use `read-tree --empty`.

[]

Re: [PATCH] send-email: Add tocmd option to suppress-cc

[Viresh Kumar]

On 13-06-17, 10:23, Junio C Hamano wrote:
> Viresh Kumar  writes:
> 
> >> Going back to the core part of your change, i.e.
> >> 
> >> -  foreach my $entry (qw (cccmd cc author self sob body bodycc)) {
> >> +  foreach my $entry (qw (tocmd cccmd cc author self sob body bodycc)) {
> >> 
> >> to think about it a bit more, I notice that all the existing ones
> >> are about CC.  So I am not just not convinced that tocmd belongs to
> >> the same class.  I am inclined to say that it smells quite different
> >> from others.
> >
> > That's right but how do we solve this problem then?
> >
> > Add another optional argument like suppress-to ? I thought that it
> > would be better to override suppress-cc rather than attempting any
> > such thing.
> >
> > I am fine with any solution that address the concerns raised by this
> > patch though.
> 
> I am not sure what problem is being solved, quite honestly.  "I have
> tocmd configured and I want a way not to specify tocmd for a single
> invocation?"  Would something along the lines of 
> 
> git -c sendemail.tocmd=true send-email ...
> 
> how you do it in general?

Yeah, it works. I can use this instead.

This patch can be dropped now. Thanks.

-- 
viresh

Re: [PATCHv5 16/17] diff: buffer all output if asked to

[Stefan Beller]

On Tue, Jun 13, 2017 at 3:07 PM, Jonathan Tan  wrote:

>> +struct diff_line {
>
> Probably should be called diff_emission (or just emission), since these
> may not be full lines.

I think emitted_string would do as well?

>
> Also, can this definition be in the .c file? Callers should use the
> diff_emit_line() below, and not need to know how it is implemented
> internally.

done

>> +#define diff_line_INIT {NULL, NULL, NULL, 0, 0, 0}
>
> Should be DIFF_LINE_INIT (capitalization), and {NULL} is sufficient, I
> think.

done.

Re: t1308-config-set.sh fails on current master

[Øyvind A . Holm]

Hi, Jonathan, thanks for having a look at this.

On 2017-06-13 18:25:35, Jonathan Nieder wrote:
> Øyvind A. Holm wrote:
> > t1308-config-set.sh fails on current master 
> > (v2.13.1-449-g02a2850ad58e). The error is introduced by commit 
> > e2d90fd1c33a ("config.mak.uname: set FREAD_READS_DIRECTORIES for 
> > Linux and FreeBSD"). Reverting the commit results in a conflict, but 
> > the test works on the commit before, 02912f477586.
> >
> > Tested on
> >
> >   Debian GNU/Linux 8.8 (jessie)
> >   Linux Mint 18 Sarah
>
> Interesting.  I'm not able to reproduce it, but of course that doesn't
> mean much.

I'll admit that I have a somewhat special build system, but it's been 
working great since I created it 7 months ago, and I run the test suite 
every time I install a new git. I'm using the Makefile located at

  https://gitlab.com/sunny256/src-other/blob/master/devel/git/Makefile

It's only doing regular stuff like "make configure", "./configure", etc, 
but I'm mentioning it in case the Makefile reveals something 
interesting. The git installation is in a non-standard location, the 
newest version of git I've installed is for example located under 
/usr/src-other/pool/git.master.v2.13.1-394-g41dd4330a121/ .

> What is the output of the following command?
>
> ./t1308-config-set.sh --run=1,23 -x -v -i

Initialized empty Git repository in 
/home/sunny/src/git/src-other/devel/git/git/t/trash 
directory.t1308-config-set/.git/
expecting success: 
cat >.git/config <<-\EOF
[case]
penguin = very blue
Movie = BadPhysics
UPPERCASE = true
MixedCase = true
my =
foo
baz = sam
[Cores]
WhatEver = Second
baz = bar
[cores]
baz = bat
[CORES]
baz = ball
[my "Foo bAr"]
hi = mixed-case
[my "FOO BAR"]
hi = upper-case
[my "foo bar"]
hi = lower-case
[case]
baz = bat
baz = hask
[lamb]
chop = 65
head = none
[goat]
legs = 4
head = true
skin = false
nose = 1
horns
EOF

+ cat
ok 1 - setup default config

skipping test: get value for a simple key 
check_config get_value case.penguin "very blue"

ok 2 # skip get value for a simple key (--run)

skipping test: get value for a key with value as an empty string 
check_config get_value case.my ""

ok 3 # skip get value for a key with value as an empty string (--run)

skipping test: get value for a key with value as NULL 
check_config get_value case.foo "(NULL)"

ok 4 # skip get value for a key with value as NULL (--run)

skipping test: upper case key 
check_config get_value case.UPPERCASE "true" &&
check_config get_value case.uppercase "true"

ok 5 # skip upper case key (--run)

skipping test: mixed case key 
check_config get_value case.MixedCase "true" &&
check_config get_value case.MIXEDCASE "true" &&
check_config get_value case.mixedcase "true"

ok 6 # skip mixed case key (--run)

skipping test: key and value with mixed case 
check_config get_value case.Movie "BadPhysics"

ok 7 # skip key and value with mixed case (--run)

skipping test: key with case sensitive subsection 
check_config get_value "my.Foo bAr.hi" "mixed-case" &&
check_config get_value "my.FOO BAR.hi" "upper-case" &&
check_config get_value "my.foo bar.hi" "lower-case"

ok 8 # skip key with case sensitive subsection (--run)

skipping test: key with case insensitive section header 
check_config get_value cores.baz "ball" &&
check_config get_value Cores.baz "ball" &&
check_config get_value CORES.baz "ball" &&
check_config get_value coreS.baz "ball"

ok 9 # skip key with case insensitive section header (--run)

skipping test: key with case insensitive section header & variable 
check_config get_value CORES.BAZ "ball" &&
check_config get_value cores.baz "ball" &&
check_config get_value cores.BaZ "ball" &&
check_config get_value cOreS.bAz "ball"

ok 10 # skip key with case insensitive section header & variable (--run)

skipping test: find value with misspelled key 
check_config expect_code 1 get_value "my.fOo Bar.hi" "Value not found 
for \"my.fOo Bar.hi\""

ok 11 # skip find value with misspelled key (--run)

skipping test: find value with the highest priority 
check_config get_value case.baz "hask"

ok 12 # skip find value with the highest priority (--run)

skipping test: find integer value for a key 
check_config get_int lamb.chop 65

ok 13 # skip find integer value for a key (--run)

skipping test: find string value for a key 
check_config get_string case.baz hask &&

Re: t1308-config-set.sh fails on current master

[Jonathan Nieder]

Hi Øyvind,

Øyvind A. Holm wrote:

> t1308-config-set.sh fails on current master (v2.13.1-449-g02a2850ad58e). 
> The error is introduced by commit e2d90fd1c33a ("config.mak.uname: set 
> FREAD_READS_DIRECTORIES for Linux and FreeBSD"). Reverting the commit 
> results in a conflict, but the test works on the commit before, 
> 02912f477586.
>
> Tested on
>
>   Debian GNU/Linux 8.8 (jessie)
>   Linux Mint 18 Sarah

Interesting.  I'm not able to reproduce it, but of course that doesn't
mean much.

> Test output:
> 
>   $ ./t1308-config-set.sh
>   ok 1 - setup default config
>   ok 2 - get value for a simple key
>   ok 3 - get value for a key with value as an empty string
>   ok 4 - get value for a key with value as NULL
>   ok 5 - upper case key
>   ok 6 - mixed case key
>   ok 7 - key and value with mixed case
>   ok 8 - key with case sensitive subsection
>   ok 9 - key with case insensitive section header
>   ok 10 - key with case insensitive section header & variable
>   ok 11 - find value with misspelled key
>   ok 12 - find value with the highest priority
>   ok 13 - find integer value for a key
>   ok 14 - find string value for a key
>   ok 15 - check line error when NULL string is queried
>   ok 16 - find integer if value is non parse-able
>   ok 17 - find bool value for the entered key
>   ok 18 - find multiple values
>   ok 19 - find value from a configset
>   ok 20 - find value with highest priority from a configset
>   ok 21 - find value_list for a key from a configset
>   ok 22 - proper error on non-existent files
>   not ok 23 - proper error on directory "files"
>   #
>   #   echo "Error (-1) reading configuration file a-directory." 
> >expect &&
>   #   mkdir a-directory &&
>   #   test_expect_code 2 test-config configset_get_value foo.bar 
> a-directory 2>output &&
>   #   grep "^warning:" output &&
>   #   grep "^Error" output >actual &&
>   #   test_cmp expect actual
>   #
>   ok 24 - proper error on non-accessible files
>   ok 25 - proper error on error in default config files
>   ok 26 - proper error on error in custom config files
>   ok 27 - check line errors for malformed values
>   ok 28 - error on modifying repo config without repo
>   ok 29 - iteration shows correct origins
>   # failed 1 among 29 test(s)
>   1..29

What is the output of the following command?

./t1308-config-set.sh --run=1,23 -x -v -i

Other diagnostics:

- what is the output of "env"?
- cat ../GIT-BUILD-OPTIONS
- if you run that under 'strace -f -o /tmp/strace.out', does the
  strace.out say anything interesting?

Thanks,
Jonathan

Re: proposal for how to share other refs as part of refs/tracking/*

[Jacob Keller]

On Tue, Jun 13, 2017 at 8:55 AM, Marc Branchaud  wrote:
> On 2017-06-13 10:41 AM, Marc Branchaud wrote:
>>
>>
>> So I like your refs/tracking proposal, and hope that it aims for mirroring
>> a remote's refs, to eventually replace refs/remotes entirely.
>
>
> To be extra-clear:
>
> I think a refs/tracking hierarchy that starts with notes and maybe some
> other bits is a good first step.
>
> I *hope* such a first step can eventually mirror all of a remote's refs,
> including heads and tags.
>
> I in no way think that this effort should begin by tackling heads and tags.
>
> M.
>

As far as I understand it, mirroring all refs should be as simple as
adding something like

refs/*:refs/tracking//*

assuming that * begins working for multiple components..

Now actually making *use* of that feature would be different.

However, this was generally the direction I was thinking of heading.

Thanks,
Jake

t1308-config-set.sh fails on current master

[Øyvind A . Holm]

t1308-config-set.sh fails on current master (v2.13.1-449-g02a2850ad58e). 
The error is introduced by commit e2d90fd1c33a ("config.mak.uname: set 
FREAD_READS_DIRECTORIES for Linux and FreeBSD"). Reverting the commit 
results in a conflict, but the test works on the commit before, 
02912f477586.

Tested on

  Debian GNU/Linux 8.8 (jessie)
  Linux Mint 18 Sarah

Test output:

  $ ./t1308-config-set.sh
  ok 1 - setup default config
  ok 2 - get value for a simple key
  ok 3 - get value for a key with value as an empty string
  ok 4 - get value for a key with value as NULL
  ok 5 - upper case key
  ok 6 - mixed case key
  ok 7 - key and value with mixed case
  ok 8 - key with case sensitive subsection
  ok 9 - key with case insensitive section header
  ok 10 - key with case insensitive section header & variable
  ok 11 - find value with misspelled key
  ok 12 - find value with the highest priority
  ok 13 - find integer value for a key
  ok 14 - find string value for a key
  ok 15 - check line error when NULL string is queried
  ok 16 - find integer if value is non parse-able
  ok 17 - find bool value for the entered key
  ok 18 - find multiple values
  ok 19 - find value from a configset
  ok 20 - find value with highest priority from a configset
  ok 21 - find value_list for a key from a configset
  ok 22 - proper error on non-existent files
  not ok 23 - proper error on directory "files"
  #
  #   echo "Error (-1) reading configuration file a-directory." 
>expect &&
  #   mkdir a-directory &&
  #   test_expect_code 2 test-config configset_get_value foo.bar 
a-directory 2>output &&
  #   grep "^warning:" output &&
  #   grep "^Error" output >actual &&
  #   test_cmp expect actual
  #
  ok 24 - proper error on non-accessible files
  ok 25 - proper error on error in default config files
  ok 26 - proper error on error in custom config files
  ok 27 - check line errors for malformed values
  ok 28 - error on modifying repo config without repo
  ok 29 - iteration shows correct origins
  # failed 1 among 29 test(s)
  1..29
  $

Øyvind

N 60.376° E 5.3334°
OpenPGP fingerprint: A006 05D6 E676 B319 55E2  E77E FB0C BEE8 94A5 06E5
2daabdde-509d-11e7-a17a-db5caa6d21d3


signature.asc
Description: PGP signature

[ANNOUNCE] public-inbox.org/git/ search updates for diffs

[Eric Wong]

Hey all,

https://public-inbox.org/git/_/text/help has a few new prefixes
which might help improve searching:

dfn: match filename from diff
dfa: match diff removed (-) lines
dfb: match diff added (+) lines
dfhh:match diff hunk header context (usually a function name)
dfctx:   match diff context lines
dfpre:   match pre-image git blob ID
dfpost:  match post-image git blob ID
dfblob:  match either pre or post-image git blob ID

The blob ID searches should work down to 7 characters right now.

I updated the xapian indices in place ( "public-inbox-index --reindex" ),
so it should've happened without a service interruption.

Probably a couple more changes on the way this month depending
on my schedule and workload on other stuff.

Re: What's cooking in git.git (Jun 2017, #04; Tue, 13)

[Jonathan Nieder]

Jun 13, 2017 at 02:40:16PM -0700, Junio C Hamano wrote:

> * sb/submodule-blanket-recursive (2017-06-01) 9 commits
>   (merged to 'next' on 2017-06-04 at 418bb03032)
>  + builtin/fetch.c: respect 'submodule.recurse' option
>  + builtin/push.c: respect 'submodule.recurse' option
>  + builtin/grep.c: respect 'submodule.recurse' option
>  + Introduce 'submodule.recurse' option for worktree manipulators
>  + submodule loading: separate code path for .gitmodules and config overlay
>  + reset/checkout/read-tree: unify config callback for submodule recursion
>  + submodule test invocation: only pass additional arguments
>  + submodule recursing: do not write a config variable twice
>  + Merge branch 'ab/grep-preparatory-cleanup' into 
> sb/submodule-blanket-recursive
>
>  Many commands learned to pay attention to submodule.recurse
>  configuration.

Yay!

>  It is not known if a simple "yes/no" is sufficient in the longer
>  term, and what should happen when --recurse-submodules option starts
>  taking "recurse into them how?" parameter, though.

Any pointers for where this has been discussed, if anywhere (e.g. was
it in the thread
http://public-inbox.org/git/20170526191017.19155-1-sbel...@google.com)?
I'm hoping that we can make the defaults work well enough that a
simple "true/false" becomes sufficient.

Perhaps this is something that the documentation at
http://public-inbox.org/git/20170607185354.10050-1-sbel...@google.com
can go into, since it is an opinionated piece of documentation that
describes commonalities between submodule-related commands and how
they are meant to fit into a user's daily life.

[...]
> * bw/config-h (2017-06-13) 4 commits
>  - config: don't implicitly use gitdir
>  - config: don't include config.h by default
>  - config: remove git_config_iter
>  - config: create config.h
>
>  Code clean-up.

Patches 1-3 are good to go IMHO.

Patch 4 in pu is marked with my Reviewed-by.  I think it's getting
there but not there yet.  Did some script pull the tag from my reply
to the cover letter?  (I'm asking so that if so I can cooperate better
with such a script in the future and avoid false positive
Reviewed-bys.)

[...]
> * jk/warn-add-gitlink (2017-06-13) 2 commits
>  - t: move "git add submodule" into test blocks
>  - add: warn when adding an embedded repository
>
>  Using "git add d/i/r" when d/i/r is the top of the working tree of
>  a separate repository would create a gitlink in the index, which
>  would appear as a not-quite-initialized submodule to others.  We
>  learned to give warnings when this happens.

Note to self that we may want to put a note about this (and more
generally about the git-series style of caller that creates a GITLINK
entry that is not for a submodule) in the document being written at
http://public-inbox.org/git/20170607185354.10050-1-sbel...@google.com
or in some other document like gitrepository-layout.txt.

[...]
> * ls/github (2017-06-13) 1 commit
>  - Configure Git contribution guidelines for github.com
>
>  Help contributors that visit us at GitHub.
>
>  Will merge to 'next'.

\o/ Thank you.

[...]
> --
> [Stalled]
>
> * mg/status-in-progress-info (2017-05-10) 2 commits
>  - status --short --inprogress: spell it as --in-progress
>  - status: show in-progress info for short status
>
>  "git status" learns an option to report various operations
>  (e.g. "merging") that the user is in the middle of.
>
>  cf. 

Thanks for the poke.  This looks a quite nice change, but I agree with
you about its current state.

Regards,
Jonathan

Re: [PATCH] remote: drop free_refspecs() function

[Jonathan Nieder]

Jeff King wrote:

> Subject: [PATCH] remote: drop free_refspecs() function
>
> We already have free_refspec(), a public function which does
> the same thing as the static free_refspecs(). Let's just
> keep one.  There are two minor differences between the
> functions:
>
>   1. free_refspecs() is a noop when the refspec argument is
>  NULL. This probably doesn't matter in practice.  The
>  nr_refspec parameter would presumably be 0 in that
>  case, skipping the loop. And free(NULL) is explicitly
>  OK. But it doesn't hurt for us to port this extra
>  safety to free_refspec(), as one of the callers passes
>  a funny "i+1" count.
>
>   2. The order of arguments is reversed between the two
>  functions. This patch uses the already-public order of
>  free_refspec(), as it matches the argument order on the
>  parsing side.
>
> Signed-off-by: Jeff King 
> ---
>  remote.c | 28 ++--
>  1 file changed, 6 insertions(+), 22 deletions(-)

Reviewed-by: Jonathan Nieder 

Thanks.

Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch

[Jonathan Nieder]

Hi,

SZEDER Gábor wrote:

> The initial fetch during a clone doesn't transfer refs matching
> additional fetch refspecs given on the command line as configuration
> variables.  This contradicts the documentation stating that
> configuration variables specified via 'git clone -c = ...'
> "take effect immediately after the repository is initialized, but
> before the remote history is fetched" and the given example
[...]
> The reason for this is that the initial fetch is not a fully fledged
> 'git fetch' but a bunch of direct calls into the fetch/transport
> machinery with clone's own refs-to-refspec matching logic, which
> bypasses parts of 'git fetch' processing configured fetch refspecs.

Agh, subtle.

I'm hoping that longer term we can make fetch behave more like a
library and make the initial fetch into a fully fledged 'git fetch'
like thing again.  But this smaller change is the logical fix in the
meantime.

[...]
> diff --git a/remote.c b/remote.c
> index ad6c5424e..b8fd09dc9 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int nr_refspec, 
> const char **refspec)
>   return parse_refspec_internal(nr_refspec, refspec, 1, 0);
>  }
>  
> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)
> +{
> + struct refspec *rs;
> +
> + add_fetch_refspec(remote, refspec);
> + rs = parse_fetch_refspec(1, );
> + REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);
> + remote->fetch[remote->fetch_refspec_nr - 1] = *rs;
> +
> + /* Not free_refspecs(), as we copied its pointers above */
> + free(rs);
> +}
> +
>  static struct refspec *parse_push_refspec(int nr_refspec, const char 
> **refspec)
>  {
>   return parse_refspec_internal(nr_refspec, refspec, 0, 0);
> diff --git a/remote.h b/remote.h
> index 924881169..9ad8c1085 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -164,6 +164,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);
>  
>  int valid_fetch_refspec(const char *refspec);
>  struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);
> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec);
>  
>  void free_refspec(int nr_refspec, struct refspec *refspec);

I realize its neighbors don't have this, but can this function have a
brief comment explaining how it is meant to be used and what
guarantees it makes?

For example:

/** Adds a refspec to remote->fetch_refspec and remote->fetch. */
void add_and_parse_fetch_refspec(struct remote *remote, const char 
*refspec);

I'm tempted to say that this one should be named add_fetch_refspec (or
something like remote_add_refspec) --- this is the only way to add a
fetch refspec in the public remote API, and the fact that it parses is
an implementation detail.  The private add_fetch_refpsec that builds
the fetch_refspec as preparation for parsing them in a batch is not
part of the exported API.

Also, now that the API is appending to remote->fetch instead of
allocating it in one go, should it use the ALLOC_GROW heuristic /
fetch_refspec_alloc size?

The caller adds one refspec right after calling remote_get.  I'm
starting to wonder if this could be done more simply by having a
variant of remote_get that allows naming an additional refspec, so
that remote->fetch could be immutable after construction like it was
before.  What do you think?

[...]
> + /* Not free_refspecs(), as we copied its pointers above */
> + free(rs);

Allocating an array to put the parsed refspec in and then freeing it
seems wasteful.  Should parse_refspec_internal be changed to take an
output parameter so it can put the refspec into remote->fetch
directly?

[...]
> +++ b/builtin/clone.c
[...]
> @@ -848,16 +853,13 @@ int cmd_clone(int argc, const char **argv, const char 
> *prefix)
>   const struct ref *our_head_points_at;
>   struct ref *mapped_refs;
>   const struct ref *ref;
> - struct strbuf key = STRBUF_INIT, value = STRBUF_INIT;
> + struct strbuf key = STRBUF_INIT, default_refspec = STRBUF_INIT;

nit: since it's not part of a key, value pair like value,
default_refspec should probably go on its own line.

[...]
> --- a/t/t5611-clone-config.sh
> +++ b/t/t5611-clone-config.sh
> @@ -37,6 +37,50 @@ test_expect_success 'clone -c config is available during 
> clone' '
>   test_cmp expect child/file
>  '
>  
> +test_expect_success 'clone -c remote.origin.fetch= works' '
> + rm -rf child &&
> + git update-ref refs/grab/it refs/heads/master &&
> + git update-ref refs/leave/out refs/heads/master &&
> + git clone -c "remote.origin.fetch=+refs/grab/*:refs/grab/*" . child &&
> + git -C child for-each-ref --format="%(refname)" >actual &&
> + cat >expect <<-EOF &&
> + refs/grab/it
> + refs/heads/master
> + refs/remotes/origin/HEAD
> + refs/remotes/origin/master
> + EOF
> + test_cmp expect actual
> +'

Can use <<-\EOF to save the reviewer from having to look 

Re: [RFC/PATCH] builtin/blame: darken redundant line information

[Stefan Beller]

On Tue, Jun 13, 2017 at 4:42 PM, Jonathan Tan  wrote:
> On Mon, 12 Jun 2017 19:31:51 -0700
> Stefan Beller  wrote:
>
>> When using git-blame lots of lines contain redundant information, for
>> example in hunks that consist of multiple lines, the metadata (commit name,
>> author, timezone) are repeated. A reader may not be interested in those,
>> so darken them. The darkening is not just based on hunk, but actually
>> takes the previous lines content for that field to compare to.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>>
>>  Example output (blame of blame): http://i.imgur.com/0Y12p2f.png
>
> Looking at this image, how does blame decide what to dim? As it is, I see many
> identical timestamps (and also from the same commit) not being dimmed.
> (For example, see the very last line with "2013-01-05 ..." which is
> identical to the previous line, and I would expect that to be dimmed.)

The date dimming is broken (it is implemented separately as a hack :/)

> Also, my preference is to have all-or-nothing dimming (dim the whole
> line up to and including the time zone if nothing has changed, and dim
> nothing otherwise) but I know that this is a subjective issue.

ok, noted.

That is what I had as a very first design (except for dimming, blanking out
the lines with white spaces) and then went on "dimming even more".

I think this is also very similar to the discussion of boundary colors of
the moved blocks in the neighboring thread, this is finding "blocks"
(actually the finding part is easy as we are given the blocks) and then
applying some "dim middle, highlight boundaries" algorithm, which in
this particular case is a "highlight first line, dim the rest" for each field
separately.

Originally I dreamed more a Zebra-style non-boundary coloring,
the colors being temperature (https://en.wikipedia.org/wiki/Color_temperature)
and the recency dictating the temperature, such that you can easily
see which code is "hot" (i.e. modified recently)

Going by that I we could have different definitions of hotness:
* recency
* number of patches on a given line though that is hard to estimate
  when a line was changed vs newly-introduced
* number of different authors in a given function (= block of text with
  the same context hint in a virtual hunk header)

[BUG] b9c8e7f2fb6e breaks git bisect visualize

[Øyvind A . Holm]

Commit b9c8e7f2fb6e ("prefix_ref_iterator: don't trim too much") breaks 
git bisect visualize, this reproduces the bug:

  $ git bisect start
  $ git bisect bad
  $ git bisect good HEAD^^
  $ git bisect visualize
  fatal: BUG: attempt to trim too many characters
  $

Reverting b9c8e7f2fb6e makes git bisect visualize work again.

Tested on Debian GNU/Linux 8.8 (jessie).

Øyvind

N 60.37604° E 5.9°
OpenPGP fingerprint: A006 05D6 E676 B319 55E2  E77E FB0C BEE8 94A5 06E5
dcacbb24-5094-11e7-b7e4-db5caa6d21d3


signature.asc
Description: PGP signature

Re: [PATCHv5 04/17] diff: introduce more flexible emit function

[Jonathan Tan]

On Tue, 13 Jun 2017 16:41:57 -0700
Stefan Beller  wrote:

> On Tue, Jun 13, 2017 at 2:54 PM, Jonathan Tan  
> wrote:
> > - could this be called emit() instead?
> 
> Despite having good IDEs available some (including me)
> very much like working with raw text, and then having a function
> named as a common string doesn't help.
> 
> After this patch
> 
>   $ git grep emit_line |wc -l
>   16
>   # not all are this function, there is
>   emit_line_checked as well. But 16 is not too much.
> 
> But if renamed to emit():
> 
>   $ git grep emit -- diff.c |wc -l
>   60
> 
> You could argue I'd just have to grep
> for "emit (" instead, but that then I would have
> rely on correct whitespacing or use a regex already.
> Complexity which I would not like.
> 
> So I am not sure if this is helping a reader. (Not the casual
> reader, but the one grepping for this function)
> 
> Maybe we can settle on a different name though,
> such as emit_string which is not a prefix of a dozen
> different other functions?

emit_string sounds good to me.

Re: [RFC/PATCH] builtin/blame: darken redundant line information

[Jonathan Tan]

On Mon, 12 Jun 2017 19:31:51 -0700
Stefan Beller  wrote:

> When using git-blame lots of lines contain redundant information, for
> example in hunks that consist of multiple lines, the metadata (commit name,
> author, timezone) are repeated. A reader may not be interested in those,
> so darken them. The darkening is not just based on hunk, but actually
> takes the previous lines content for that field to compare to.
> 
> Signed-off-by: Stefan Beller 
> ---
> 
>  Example output (blame of blame): http://i.imgur.com/0Y12p2f.png

Looking at this image, how does blame decide what to dim? As it is, I see many
identical timestamps (and also from the same commit) not being dimmed.
(For example, see the very last line with "2013-01-05 ..." which is
identical to the previous line, and I would expect that to be dimmed.)

Also, my preference is to have all-or-nothing dimming (dim the whole
line up to and including the time zone if nothing has changed, and dim
nothing otherwise) but I know that this is a subjective issue.

Re: [PATCHv5 04/17] diff: introduce more flexible emit function

[Stefan Beller]

On Tue, Jun 13, 2017 at 2:54 PM, Jonathan Tan  wrote:
> On Wed, 24 May 2017 14:40:23 -0700
> Stefan Beller  wrote:
>
>> Currently, diff output is written either through the emit_line_0
>> function or through the FILE * in struct diff_options directly. To
>> make it easier to teach diff to buffer its output (which will be done
>> in a subsequent commit), introduce a more flexible emit_line() function.
>> In this commit, direct usages of emit_line_0() are replaced with
>> emit_line(); subsequent commits will also replace usages of the
>> FILE * with emit().
>
> Check the names of the functions in this paragraph.

Only the very last word needed replacement of /s/emit/emit_line/.

>
>> diff --git a/diff.c b/diff.c
>> index 2f9722b382..3569857818 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -516,36 +516,30 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t 
>> *mf2,
>>   ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
>>  }
>>
>> -static void emit_line_0(struct diff_options *o, const char *set, const char 
>> *reset,
>> - int first, const char *line, int len)
>> +static void emit_line(struct diff_options *o, const char *set, const char 
>> *reset,
>> +   int add_line_prefix, int sign, const char *line, int len)
>
> In the future, this function is going to be used even to emit partial
> lines

Yes.

> - could this be called emit() instead?

Despite having good IDEs available some (including me)
very much like working with raw text, and then having a function
named as a common string doesn't help.

After this patch

  $ git grep emit_line |wc -l
  16
  # not all are this function, there is
  emit_line_checked as well. But 16 is not too much.

But if renamed to emit():

  $ git grep emit -- diff.c |wc -l
  60

You could argue I'd just have to grep
for "emit (" instead, but that then I would have
rely on correct whitespacing or use a regex already.
Complexity which I would not like.

So I am not sure if this is helping a reader. (Not the casual
reader, but the one grepping for this function)

Maybe we can settle on a different name though,
such as emit_string which is not a prefix of a dozen
different other functions?

Thanks,
Stefan

Re: [PATCHv4 2/2] Documentation/clone: document ignored configuration variables

[Jonathan Nieder]

Hi,

SZEDER Gábor wrote:

> Due to limitations/bugs in the current implementation, some
> configuration variables specified via 'git clone -c var=val' (or 'git
> -c var=val clone') are ignored during the initial fetch and checkout.
>
> Let the users know which configuration variables are known to be
> ignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the
> documentation of 'git clone -c'.
>
> Signed-off-by: SZEDER Gábor 
> ---
>  Documentation/git-clone.txt | 4 
>  1 file changed, 4 insertions(+)

Makes sense.

> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index ec41d3d69..4f1e7d4ba 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -186,6 +186,10 @@ objects from the source repository into a pack in the 
> cloned repository.
>   values are given for the same key, each value will be written to
>   the config file. This makes it safe, for example, to add
>   additional fetch refspecs to the origin remote.
> + Note that due to limitations of the current implementation some
> + configuration variables don't take effect during the initial
> + fetch and checkout.  Configuration variables known to not take
> + effect are: `remote..mirror` and `remote..tagOpt`.
>  

Tiny nit: the paragraph of --config description is already a bit
overwhelming, and I think this additional note takes it over the edge
where I give up and stop reading.  Could it go in a separate
paragraph?

the config file. This makes it safe, for example, to add
additional fetch refspecs to the origin remote.
+
Due to limitations in the current implementation, some
configuration variables do not take effect until after the
initial fetch and checkout. Configuration variables known
not to take effect are `remote..mirror` and
`remote..tagOpt`.

Thanks,
Jonathan

Re: [PATCH 8/8] t0012: test "-h" with builtins

[Jonathan Nieder]

Hi,

Jeff King wrote:

> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -49,4 +49,16 @@ test_expect_success "--help does not work for guides" "
>   test_i18ncmp expect actual
>  "
> 
> +test_expect_success 'generate builtin list' '
> + git --list-builtins >builtins
> +'
> +
> +while read builtin
> +do
> + test_expect_success "$builtin can handle -h" '
> + test_expect_code 129 git $builtin -h >output 2>&1 &&
> + test_i18ngrep usage output
> + '
> +done 

Re: [PATCH] diff.c: color moved lines differently

[Jonathan Tan]

On Wed, 31 May 2017 17:24:29 -0700
Stefan Beller  wrote:

> When a patch consists mostly of moving blocks of code around, it can
> be quite tedious to ensure that the blocks are moved verbatim, and not
> undesirably modified in the move. To that end, color blocks that are
> moved within the same patch differently. For example (OM, del, add,
> and NM are different colors):

[snip]

Junio asks "are we happy with these changes" [1] and my answer is, in
general, yes - this seems like a very useful feature to have, and I'm OK
with the current design.

I do feel a bit of unease at how the emitted strings are collected
without many guarantees as to their contents (e.g. whether they are full
lines or even whether they originate from the text of a file), but this
is already true for the existing code. The potential danger is that we
are now relying more on the format of these strings, but we don't plan
to do anything other than to color them, so this seems fine.

I would also prefer if there was only one coloring method, to ease
testing, but I can tolerate the current multiplicity of options.

I think there was some discussion at trying to avoid indicating that
small blocks (consisting of few non-whitespace characters) are moved,
but I think that that can be added later.

[1] 
https://public-inbox.org/git/cagz79ky2z-fjyxczbzheu1hchlkkkdjecdmwsp-hkn0tjub...@mail.gmail.com/

> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -231,6 +231,38 @@ ifdef::git-diff[]
>  endif::git-diff[]
>   It is the same as `--color=never`.
>  
> +--color-moved[=]::
> + Moved lines of code are colored differently.
> +ifdef::git-diff[]
> + It can be changed by the `diff.colorMoved` configuration setting.
> +endif::git-diff[]
> + The  defaults to 'no' if the option is not given
> + and to 'adjacentbounds' if the option with no mode is given.
> + The mode must be one of:
> ++
> +--
> +no::
> + Moved lines are not highlighted.
> +nobounds::
> + Any line that is added in one location and was removed
> + in another location will be colored with 'color.diff.newmoved'.
> + Similarly 'color.diff.oldmoved' will be used for removed lines

Probably best to consistently capitalize newMoved and oldMoved.

> +static unsigned get_line_hash(struct diff_line *line, unsigned ignore_ws)
> +{
> + static struct strbuf sb = STRBUF_INIT;
> +
> + if (ignore_ws) {
> + strbuf_reset();
> + get_ws_cleaned_string(line, );
> + return memhash(sb.buf, sb.len);
> + } else {
> + return memhash(line->line, line->len);
> + }
> +}

Can be written without the "else".

> +test_expect_success 'move detection does not mess up colored words' '

Probably name this test "--color-moved has no effect when used with
--word-diff".

Re: [BUG] add_again() off-by-one error in custom format

[SZEDER Gábor]

[sorry for double post, forgot the mailing list...]

To throw in a fourth option, this one adjusts the expansions' cached
offsets when the magic makes it necessary.  It's not necessary for
'%-', because it only makes a difference when the expansion is empty,
and in that case

  - add_again() doesn't consider it cached,
  - and even if it did, the offset of a zero-length string wouldn't
matter.

 pretty.c | 32 +---
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/pretty.c b/pretty.c
index d0f86f5d8..69c4f2a21 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1066,9 +1066,17 @@ static size_t parse_padding_placeholder(struct strbuf 
*sb,
return 0;
 }
 
+enum format_commit_item_magic {
+   NO_MAGIC,
+   ADD_LF_BEFORE_NON_EMPTY,
+   DEL_LF_BEFORE_EMPTY,
+   ADD_SP_BEFORE_NON_EMPTY
+};
+
 static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
const char *placeholder,
-   void *context)
+   void *context,
+   enum format_commit_item_magic magic)
 {
struct format_commit_context *c = context;
const struct commit *commit = c->commit;
@@ -1155,6 +1163,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
 c->pretty_ctx->abbrev);
strbuf_addstr(sb, diff_get_color(c->auto_color, DIFF_RESET));
c->abbrev_commit_hash.len = sb->len - c->abbrev_commit_hash.off;
+   if (magic == ADD_LF_BEFORE_NON_EMPTY || magic == 
ADD_SP_BEFORE_NON_EMPTY)
+   c->abbrev_commit_hash.off++;
return 1;
case 'T':   /* tree hash */
strbuf_addstr(sb, oid_to_hex(>tree->object.oid));
@@ -1165,6 +1175,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
strbuf_add_unique_abbrev(sb, commit->tree->object.oid.hash,
 c->pretty_ctx->abbrev);
c->abbrev_tree_hash.len = sb->len - c->abbrev_tree_hash.off;
+   if (magic == ADD_LF_BEFORE_NON_EMPTY || magic == 
ADD_SP_BEFORE_NON_EMPTY)
+   c->abbrev_tree_hash.off++;
return 1;
case 'P':   /* parent hashes */
for (p = commit->parents; p; p = p->next) {
@@ -1184,6 +1196,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
}
c->abbrev_parent_hashes.len = sb->len -
  c->abbrev_parent_hashes.off;
+   if (magic == ADD_LF_BEFORE_NON_EMPTY || magic == 
ADD_SP_BEFORE_NON_EMPTY)
+   c->abbrev_parent_hashes.off++;
return 1;
case 'm':   /* left/right/bottom */
strbuf_addstr(sb, get_revision_mark(NULL, commit));
@@ -1314,7 +1328,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in 
UTF-8 */
 
 static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
const char *placeholder,
-   struct format_commit_context *c)
+   struct format_commit_context *c,
+   enum format_commit_item_magic magic)
 {
struct strbuf local_sb = STRBUF_INIT;
int total_consumed = 0, len, padding = c->padding;
@@ -1329,7 +1344,7 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* 
in UTF-8 */
}
while (1) {
int modifier = *placeholder == 'C';
-   int consumed = format_commit_one(_sb, placeholder, c);
+   int consumed = format_commit_one(_sb, placeholder, c, 
magic);
total_consumed += consumed;
 
if (!modifier)
@@ -1420,12 +1435,7 @@ static size_t format_commit_item(struct strbuf *sb, /* 
in UTF-8 */
 {
int consumed;
size_t orig_len;
-   enum {
-   NO_MAGIC,
-   ADD_LF_BEFORE_NON_EMPTY,
-   DEL_LF_BEFORE_EMPTY,
-   ADD_SP_BEFORE_NON_EMPTY
-   } magic = NO_MAGIC;
+   enum format_commit_item_magic magic = NO_MAGIC;
 
switch (placeholder[0]) {
case '-':
@@ -1445,9 +1455,9 @@ static size_t format_commit_item(struct strbuf *sb, /* in 
UTF-8 */
 
orig_len = sb->len;
if (((struct format_commit_context *)context)->flush_type != no_flush)
-   consumed = format_and_pad_commit(sb, placeholder, context);
+   consumed = format_and_pad_commit(sb, placeholder, context, 
magic);
else
-   consumed = format_commit_one(sb, placeholder, context);
+   consumed = format_commit_one(sb, placeholder, context, magic);
if (magic == NO_MAGIC)
return consumed;
 
-- 
2.13.1.438.gc638325b1

Re: What's cooking in git.git (Jun 2017, #03; Mon, 5)

[Stefan Beller]

On Wed, Jun 7, 2017 at 10:41 PM, Jacob Keller  wrote:
> On Mon, Jun 5, 2017 at 11:52 PM, Jacob Keller  wrote:
>>
>> I will try to find some time tomorrow to go over it in detail.
>>

https://public-inbox.org/git/20170613023151.9688-1-sbel...@google.com/
restarted the discussion on this feature, so I assembled a team of color experts
(my office mates), and we came to this conclusion more or less:

There are different ways to implement a move detection. Different
  people prefer different things, the main contenders are:

1)"Zebra mode" (in the last patch known as "alternate" mode.

This alternates between 2 colors for blocks of moved code.
This may or may not reset to the first color, but the main features are:
a) the whole block is in one color
b) adjacent blocks have different colors.

For this implementation we need 4 new colors (2 colors for +, 2 for -)

2)"Highlight bounds + dimming in between"

A block is generally in a dim color. Depending on further configuration
the boundaries are highlighted. There are different modes for that:
a) all boundaries are highlighted
b) bounds adjacent to other moved blocks of same sign are highlighted
c) no bounds highlighted, moved code is only dimmed.
(sucks for seeing blocks)
d) [NEW] only the first (last) border line.
This is consistent with the thread that started the discussion as
this is done in the RFC for blame.

  The configured colors "oldMoved" and "oldMovedAlternative"
  are assumed that one is highlighting and the other is the dim color.

  Depending on the exact implementation we'd need up to 6 new
  colors (dim, highlight, highlight alternative for each + and -)
  The user may configure these to be the same though, e.g.
  both dims could be 'context'.

3) Combine 1) and 2)

The algorithm would look like this:
a) Find moved blocks
b) Take one option from 2) to highlight the bounds. (all bounds
  or adjacent bounds are the only serious contenders IMHO)

  This needs 8 new colors configured:
  for each of (new, old):
for each of (Zebra black, Zebra white):
  for each of (dim, highlight):
define_color()

The color experts agreed that (3) might be the best solution
as this gives most flexibility:

"I would be happy as I can configure the bounds highlighting
to not exist, it would degenerate to a pure Zebra, which is
very simple to understand. Junio seemed to like (2) a lot, so
he would configure both dim colors to be 'context', but configure
the highlight colors to be attention drawing. So everybody would
be happy. It is also not too many colors, we are good at for loops."

I hope to have summarized this adequately.

Thanks,
Stefan

[PATCH v3] doc: do not use `rm .git/index` when normalizing line endings

[Andreas Heiduk]

When illustrating how to normalize the line endings, the
documentation in gitattributes tells the user to `rm .git/index`.

This is incorrect for two reasons:

 - Users shouldn't be instructed to mess around with the internal
   implementation of Git using raw file system tools like `rm`.

 - Within a submodule or an additional working tree `.git` is just a
   file containing a `gitdir: ` pointer into the real `.git`
   directory.  Therefore `rm .git/index` does not work.

The purpose or `rm .git/index` instruction is to remove all entries
from the index without touching the working tree.  The way to do this
with Git is to use `read-tree --empty`.

Signed-off-by: Andreas Heiduk 
Helped-by: Junio C Hamano 
Helped-by: Torsten Bögershausen 
---
 Documentation/gitattributes.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 473648386..2a2d7e2a4 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -229,7 +229,7 @@ From a clean working directory:
 
 -
 $ echo "* text=auto" >.gitattributes
-$ rm .git/index # Remove the index to re-scan the working directory
+$ git read-tree --empty   # Clean index, force re-scan of working directory
 $ git add .
 $ git status# Show files that will be normalized
 $ git commit -m "Introduce end-of-line normalization"
-- 
2.13.0

Re: [PATCHv5 16/17] diff: buffer all output if asked to

[Jonathan Tan]

On Wed, 24 May 2017 14:40:35 -0700
Stefan Beller  wrote:

> diff --git a/diff.h b/diff.h
> index 85948ed65a..fad1258556 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -115,6 +115,42 @@ enum diff_submodule_format {
>   DIFF_SUBMODULE_INLINE_DIFF
>  };
>  
> +/*
> + * This struct is used when we need to buffer the output of the diff output.
> + *
> + * NEEDSWORK: Instead of storing a copy of the line, add an offset pointer
> + * into the pre/post image file. This pointer could be a union with the
> + * line pointer. By storing an offset into the file instead of the literal 
> line,
> + * we can decrease the memory footprint for the buffered output. At first we
> + * may want to only have indirection for the content lines, but we could also
> + * enhance the state for emitting prefabricated lines, e.g. the similarity
> + * score line or hunk/file headers would only need to store a number or path
> + * and then the output can be constructed later on depending on state.
> + */
> +struct diff_line {

Probably should be called diff_emission (or just emission), since these
may not be full lines.

Also, can this definition be in the .c file? Callers should use the
diff_emit_line() below, and not need to know how it is implemented
internally.

> + const char *set;
> + const char *reset;
> + const char *line;
> + int len;
> + int sign;
> + int add_line_prefix;
> + enum {
> + /*
> +  * Emits [lineprefix][set][sign][reset] and then calls
> +  * ws_check_emit which will output "line", marked up
> +  * according to ws_rule.
> +  */
> + DIFF_LINE_WS,
> +
> + /* Emits [lineprefix][set][sign] line [reset] */
> + DIFF_LINE_ASIS,
> +
> + /* Reloads the ws_rule; line contains the file name */
> + DIFF_LINE_RELOAD_WS_RULE
> + } state;
> +};
> +#define diff_line_INIT {NULL, NULL, NULL, 0, 0, 0}

Should be DIFF_LINE_INIT (capitalization), and {NULL} is sufficient, I
think.

Re: [PATCH v2 4/6] config: don't implicitly use gitdir

[Jonathan Nieder]

Junio C Hamano wrote:
> On Tue, Jun 13, 2017 at 2:51 PM, Jonathan Nieder  wrote:

>> What is the next step, then?  You can find the notion ridiculous but
>> it's how this project has worked in my experience (and how other
>> projects with similar patch-based workflows work).
>
> Does "patch-based" have much to do with this? I agree that distributed
> nature of the development would bring this issue, but I tend to think that
> using merge/pull based workflow would not alleviate it--am I mistaken?

Thanks, you're right.  Distributed is the relevant feature.

The same issue can even come up when using a centralized version
control system like Subversion or Perforce --- without attention to
API compatibility, someone's change that was thoroughly reviewed and
well tested locally in a developer's working directory can introduce
subtle breakage once they run "svn commit", causing it to merge with
the latest upstream changes.  The problem becomes more likely the more
distributed a project is since each developer becomes less aware of
the other changes that their modifications need to be compatible with.

Jonathan

Re: [PATCH v2 3/6] config: don't include config.h by default

[Jonathan Nieder]

Brandon Williams wrote:
> Stop including config.h by default in cache.h.  Instead only include
> config.h in those files which require use of the config system.
>
> Signed-off-by: Brandon Williams 
> ---
[...]
>  145 files changed, 145 insertions(+), 1 deletion(-)

Reviewed-by: Jonathan Nieder 

Thanks.

Re: [PATCH v2 4/6] config: don't implicitly use gitdir

[Junio C Hamano]

On Tue, Jun 13, 2017 at 2:51 PM, Jonathan Nieder  wrote:
>
> What is the next step, then?  You can find the notion ridiculous but
> it's how this project has worked in my experience (and how other
> projects with similar patch-based workflows work).

Does "patch-based" have much to do with this? I agree that distributed
nature of the development would bring this issue, but I tend to think that
using merge/pull based workflow would not alleviate it--am I mistaken?

Re: [PATCHv5 04/17] diff: introduce more flexible emit function

[Jonathan Tan]

On Wed, 24 May 2017 14:40:23 -0700
Stefan Beller  wrote:

> Currently, diff output is written either through the emit_line_0
> function or through the FILE * in struct diff_options directly. To
> make it easier to teach diff to buffer its output (which will be done
> in a subsequent commit), introduce a more flexible emit_line() function.
> In this commit, direct usages of emit_line_0() are replaced with
> emit_line(); subsequent commits will also replace usages of the
> FILE * with emit().

Check the names of the functions in this paragraph.

> diff --git a/diff.c b/diff.c
> index 2f9722b382..3569857818 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -516,36 +516,30 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t 
> *mf2,
>   ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
>  }
>  
> -static void emit_line_0(struct diff_options *o, const char *set, const char 
> *reset,
> - int first, const char *line, int len)
> +static void emit_line(struct diff_options *o, const char *set, const char 
> *reset,
> +   int add_line_prefix, int sign, const char *line, int len)

In the future, this function is going to be used even to emit partial
lines - could this be called emit() instead?

Re: [PATCH v2 4/6] config: don't implicitly use gitdir

[Jonathan Nieder]

Brandon Williams wrote:
> On 06/13, Jonathan Nieder wrote:
>> Brandon Williams wrote:

>>> Commit 2185fde56 (config: handle conditional include when $GIT_DIR is
>>> not set up) added a 'git_dir' field to the config_options struct.  Let's
>>> use this option field explicitly all the time instead of occasionally
>>> falling back to calling 'git_pathdup("config")' to get the path to the
>>> local repository configuration.  This allows 'do_git_config_sequence()'
>>> to not implicitly rely on global repository state.
>>>
>>> Signed-off-by: Brandon Williams 
>>> ---
>>>  builtin/config.c | 2 ++
>>>  config.c | 6 ++
>>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> The same comments as before still apply:
>>
>> - this changes API to make opts->git_dir mandatory, which is error prone
>>   and easily avoidable, e.g. by making git_dir an argument to
>>   git_config_with_options
>
> I still don't agree with this.  I have looked at all callers and ensured
> that 'git_dir' will be set when appropriate in the 'config_options'
> struct.  I find the notion ridiculous that I would need to change a
> function's name or arguments every time the internals of the function
> are adjusted or when an options struct obtains a new field.  Plus, there
> is already an aptly named parameter of type 'config_options' with which
> to hold options for the config machinery.  This struct is also added to
> in a later patch to include commondir so that the gitdir vs commondir
> issue can be resolved.

What is the next step, then?  You can find the notion ridiculous but
it's how this project has worked in my experience (and how other
projects with similar patch-based workflows work).

I also don't really understand why it is so bad to have to care about
API compatibility when it is so simple to do --- change the function
signature or change the function name.  That's all it takes.

>> - the commit message doesn't say anything about to git dir vs common dir
>>   change.  It needs to, or even better, the switch to use common dir
>>   instead of git dir can happen as a separate patch.
>
> There really isn't any switching in this patch.  One of the following
> patches in this series addresses this problem in more detail though.

There is, because of the gitdir: behavior change.

Jonathan

What's cooking in git.git (Jun 2017, #04; Tue, 13)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to "master"]

* jc/diff-tree-stale-comment (2017-06-02) 1 commit
  (merged to 'next' on 2017-06-04 at bffae281d2)
 + diff-tree: update stale in-code comments

 Comment fix.


* jc/noent-notdir (2017-05-30) 2 commits
  (merged to 'next' on 2017-06-04 at 7cb4efbc3c)
 + treewide: use is_missing_file_error() where ENOENT and ENOTDIR are checked
 + compat-util: is_missing_file_error()

 Our code often opens a path to an optional file, to work on its
 contents when we can successfully open it.  We can ignore a failure
 to open if such an optional file does not exist, but we do want to
 report a failure in opening for other reasons (e.g. we got an I/O
 error, or the file is there, but we lack the permission to open).

 The exact errors we need to ignore are ENOENT (obviously) and
 ENOTDIR (less obvious).  Instead of repeating comparison of errno
 with these two constants, introduce a helper function to do so.


* jk/pack-idx-corruption-safety (2017-06-07) 1 commit
  (merged to 'next' on 2017-06-07 at 31f94e174d)
 + t5313: make extended-table test more deterministic

 A flaky test has been corrected.


* nd/fopen-errors (2017-06-02) 13 commits
  (merged to 'next' on 2017-06-04 at 7a755e73bb)
 + mingw_fopen: report ENOENT for invalid file names
 + mingw: verify that paths are not mistaken for remote nicknames
 + log: fix memory leak in open_next_file()
 + rerere.c: move error_errno() closer to the source system call
 + print errno when reporting a system call error
 + wrapper.c: make warn_on_inaccessible() static
 + wrapper.c: add and use fopen_or_warn()
 + wrapper.c: add and use warn_on_fopen_errors()
 + config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too
 + config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
 + clone: use xfopen() instead of fopen()
 + use xfopen() in more places
 + git_fopen: fix a sparse 'not declared' warning

 We often try to open a file for reading whose existence is
 optional, and silently ignore errors from open/fopen; report such
 errors if they are not due to missing files.


* rf/completion (2017-06-02) 6 commits
  (merged to 'next' on 2017-06-04 at dde1e34703)
 + completion: add git config credentialCache.ignoreSIGHUP
 + completion: add git config credential completions
 + completion: add git config advice completions
 + completion: add git config am.threeWay completion
 + completion: add git config core completions
 + completion: add git config gc completions

 Completion updates.


* sb/submodule-blanket-recursive (2017-06-01) 9 commits
  (merged to 'next' on 2017-06-04 at 418bb03032)
 + builtin/fetch.c: respect 'submodule.recurse' option
 + builtin/push.c: respect 'submodule.recurse' option
 + builtin/grep.c: respect 'submodule.recurse' option
 + Introduce 'submodule.recurse' option for worktree manipulators
 + submodule loading: separate code path for .gitmodules and config overlay
 + reset/checkout/read-tree: unify config callback for submodule recursion
 + submodule test invocation: only pass additional arguments
 + submodule recursing: do not write a config variable twice
 + Merge branch 'ab/grep-preparatory-cleanup' into 
sb/submodule-blanket-recursive

 Many commands learned to pay attention to submodule.recurse
 configuration.

 It is not known if a simple "yes/no" is sufficient in the longer
 term, and what should happen when --recurse-submodules option starts
 taking "recurse into them how?" parameter, though.

--
[New Topics]

* js/alias-early-config (2017-06-13) 6 commits
 - Use the early config machinery to expand aliases
 - t7006: demonstrate a problem with aliases in subdirectories
 - t1308: relax the test verifying that empty alias values are disallowed
 - help: use early config when autocorrecting aliases
 - config: report correct line number upon error
 - discover_git_directory(): avoid setting invalid git_dir

 The code to pick up and execute command alias definition from the
 configuration used to switch to the top of the working tree and
 then come back when the expanded alias was executed, which was
 unnecessarilyl complex.  Attempt to simplify the logic by using the
 early-config mechanism that does not chdir around.

 Waiting for discussion to settle.


* pc/dir-count-slashes (2017-06-12) 1 commit
 - dir: create function count_slashes()

 Three instances of the same helper function have been consolidated
 to one.

 Will merge to 'next'.


* sb/t4005-modernize (2017-06-10) 1 commit
 - t4005: modernize 

Re: [PATCH v2 4/6] config: don't implicitly use gitdir

[Brandon Williams]

On 06/13, Jonathan Nieder wrote:
> Brandon Williams wrote:
> 
> > Commit 2185fde56 (config: handle conditional include when $GIT_DIR is
> > not set up) added a 'git_dir' field to the config_options struct.  Let's
> > use this option field explicitly all the time instead of occasionally
> > falling back to calling 'git_pathdup("config")' to get the path to the
> > local repository configuration.  This allows 'do_git_config_sequence()'
> > to not implicitly rely on global repository state.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  builtin/config.c | 2 ++
> >  config.c | 6 ++
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> The same comments as before still apply:
> 
> - this changes API to make opts->git_dir mandatory, which is error prone
>   and easily avoidable, e.g. by making git_dir an argument to
>   git_config_with_options

I still don't agree with this.  I have looked at all callers and ensured
that 'git_dir' will be set when appropriate in the 'config_options'
struct.  I find the notion ridiculous that I would need to change a
function's name or arguments every time the internals of the function
are adjusted or when an options struct obtains a new field.  Plus, there
is already an aptly named parameter of type 'config_options' with which
to hold options for the config machinery.  This struct is also added to
in a later patch to include commondir so that the gitdir vs commondir
issue can be resolved.

> - the commit message doesn't say anything about to git dir vs common dir
>   change.  It needs to, or even better, the switch to use common dir
>   instead of git dir can happen as a separate patch.

There really isn't any switching in this patch.  One of the following
patches in this series addresses this problem in more detail though.

-- 
Brandon Williams

Re: [BUG] add_again() off-by-one error in custom format

[Junio C Hamano]

René Scharfe  writes:

> The difference is about the same as the one between:
>
>   $ time git log --format="" >/dev/null
>
>   real0m0.463s
>   user0m0.448s
>   sys 0m0.012s
>
> and:
>
>   $ time git log --format="%h" >/dev/null
>
>   real0m1.062s
>   user0m0.636s
>   sys 0m0.416s
>
> With caching duplicates are basically free and without it short
> hashes have to be looked up again.  Other placeholders may reduce
> the relative slowdown, depending on how expensive they are.

I think the real question is how likely people use more than one
occurrence of the same thing in their custom format, and how deeply
they care that --format='%h %h' costs more than --format='%h'.  The
cost won't of course be double (because the main traversal costs
without any output), but it would be rather unreasonable to expect
that --format='%h %h %h %h %h' to cost the same as --format='%h';
after all, Git is doing more for them ;-)

So in that sense, I am actually OK if we decide to remove the caching.

> Forgot a third option, probably because it's not a particularly good
> idea: Replacing the caching in pretty.c with a short static cache in
> find_unique_abbrev_r().

Indeed.

Re: [PATCH v2 2/6] config: remove git_config_iter

[Jonathan Nieder]

Brandon Williams wrote:

> Since there is no implementation of the function 'git_config_iter' lets
> stop exporting it and remove the prototype from config.h.
>
> Signed-off-by: Brandon Williams 
> ---
>  config.h | 1 -
>  1 file changed, 1 deletion(-)

Language nit:

s/' lets/', let's/

That is, there is a comma and an apostrophe missing.

With or without that tweak,
Reviewed-by: Jonathan Nieder 

Re: [PATCH v2 1/6] config: create config.h

[Jonathan Nieder]

Hi,

Brandon Williams wrote:

> Move all config related declarations from cache.h to a new config.h
> header file.  This makes cache.h smaller and allows for the opportunity
> in a following patch to only include config.h when needed.
>
> Signed-off-by: Brandon Williams 
> ---
>  cache.h  | 190 +
>  config.h | 194 
> +++
>  2 files changed, 195 insertions(+), 189 deletions(-)
>  create mode 100644 config.h

'git diff HEAD^:cache.h config.h' confirms that this is
straightforward code movement and didn't introduce any unintentional
changes along the way.  Thanks for writing it.

Reviewed-by: Jonathan Nieder 

Re: [PATCH v2 4/6] config: don't implicitly use gitdir

[Jonathan Nieder]

Brandon Williams wrote:

> Commit 2185fde56 (config: handle conditional include when $GIT_DIR is
> not set up) added a 'git_dir' field to the config_options struct.  Let's
> use this option field explicitly all the time instead of occasionally
> falling back to calling 'git_pathdup("config")' to get the path to the
> local repository configuration.  This allows 'do_git_config_sequence()'
> to not implicitly rely on global repository state.
>
> Signed-off-by: Brandon Williams 
> ---
>  builtin/config.c | 2 ++
>  config.c | 6 ++
>  2 files changed, 4 insertions(+), 4 deletions(-)

The same comments as before still apply:

- this changes API to make opts->git_dir mandatory, which is error prone
  and easily avoidable, e.g. by making git_dir an argument to
  git_config_with_options

- the commit message doesn't say anything about to git dir vs common dir
  change.  It needs to, or even better, the switch to use common dir
  instead of git dir can happen as a separate patch.

Thanks,
Jonathan

Re: [RFC/PATCH] submodules: overhaul documentation

[Stefan Beller]

Adding two native speakers as we start word smithing.

On Tue, Jun 13, 2017 at 12:29 PM, Junio C Hamano  wrote:

>> +
>> +A submodule is another Git repository tracked in a subdirectory of your
>> +repository. The tracked repository has its own history, which does not
>> +interfere with the history of the current repository.
>
> "tracked in a subdirectory" sounds as if your top-level superproject
> has a dedicated submodules/ directory and in it there live a bunch
> of submodules.  Which obviously is not what you meant.  If phrased
> "tracked as a subdirectory", I think the sentence makes sense.

Given this explanation "as a" also sounds wrong[1], maybe we need to
separate (1) where it is put/mounted and (2) the fact that is tracked,
i.e. the superproject has an idea of what should be there at a given
revision. (I shortly thought about /s/as a/using/ in the above, but):

  A submodule is another Git repository at an arbitrary place inside
  the working tree, and also tracked. The tracked repository has its
  own history, which does not interfere with the history of the current
  repository.

[1] http://www.thesaurus.com/browse/as

>
> While "which does not interfere" may be technically correct, I am
> not sure what the value of saying that is.

I think we can drop it here. When writing I wanted to separate it from
subtrees, but this is the wrong place for that.

>
>> +Submodules are composed from a so-called `gitlink` tree entry
>> +in the main repository that refers to a particular commit object
>> +within the inner repository.
>
> Correct, but it may be unclear to the readers why we do so.  Perhaps
>
> ... and this way, the tree of each commit in the main repository
> "knows" which commit from the submodule's history is "tied" to it.
>
> or something like that?

sounds good to me.

>
>> +Additionally to the gitlink entry the `.gitmodules` file (see
>> +linkgit:gitmodules[5]) at the root of the source tree contains
>> +information needed for submodules.
>
> Is that really true?  Each submodule do not *need* what is in
> .gitmodules; the top-level superproject needs to learn about
> its submodules from the contents of that file, though.

Ha! The ediled words in my mind were:

 ... information needed for submodules [to work in the superproject].

But maybe we need to reword that as

  Additionally to the gitlink entry the `.gitmodules` file (see
  linkgit:gitmodules[5]) at the root of the source tree contains
  information on how to handle submodules.

I'd like to keep this part short and not go into detail.

>
>> +The only required information
>> +is the path setting, which estabishes a logical name for the submodule.
>
> The phrase "the path setting" feels a bit unfortunate.  Is that
> "only" thing we need?  Without URL we have no way to populate it,
> no?

git config -f .gitmodules submodule.foo.path foo
git config submodule.foo.url example.org/foo
git submodule update --init

ought to work just fine. It is not the recommended way of working,
but it should work.

I think (in the far future) we actually should only have the path information
in-tree and *any* other information outside the tree, which includes the URL,

See[2], where I state how I'd like to shape the future:

  $ cat .gitmodules
  [submodule "sub42"]
path = foo
  # path only in tree!

  $ cat .git/config
  ...
  [submodule]
active = .
active = :(exclude)Irrelevant/submodules/for/my/usecase/*
  # note how this is user centric

  $ git show refs/meta/magic/for/refs/heads/master:.gitmodules
  [submodule "sub42"]
url = https://example.org/foo
branch = .
  # Note how this is neither centering on the in-tree
  # contents, nor the user. Instead it focuses on the
  # project or group. It is *workflow* centric.
  # Workflows may change over time, e.g. the url could
  # be repointed to k.org or an in-house mirror without tree
  # changes.

Jonathan pointed out the ref name is chosen poorly, but conceptually
I would want to keep the URL setting outside the tree. The URL may
change over time, independently from the history currently checked out
(think of bisect, that includes an "submodule update --init" to bisect across
a fully populated superproject 'at the time')

[2] 
https://public-inbox.org/git/cagz79kbbtwqicvkrs51fv91r_7zhdtc+fr8z-sqzrpf2cjf...@mail.gmail.com/




>
>> +The usual git configuration (see linkgit:git-config[1]) can be used to
>> +override settings given by the `.gitmodules` file.
>> +
>> +Submodules can be used for two different use cases:
>> +
>> +1. Using another project that stands on its own.
>> +  When you want to use a third party library, submodules allow you to
>> +  have a clean history for your own project as well as for the library.
>> +  This also allows for updating the third party library as needed.
>> +
>> +2. Artificially split a (logically single) project into multiple
>> +   repositories and tying them back together. This can be used to
>> +   

[PATCH v2 4/4] sha1_file, fsck: add missing blob support

[Jonathan Tan]

Currently, Git does not support repos with very large numbers of blobs
or repos that wish to minimize manipulation of certain blobs (for
example, because they are very large) very well, even if the user
operates mostly on part of the repo, because Git is designed on the
assumption that every blob referenced by a tree object is available
somewhere in the repo storage.

As a first step to reducing this problem, add rudimentary support for
missing blobs by teaching sha1_file to invoke a hook whenever a blob is
requested and unavailable but registered to be missing, and by updating
fsck to tolerate such blobs.  The hook is a shell command that can be
configured through "git config"; this hook takes in a list of hashes and
writes (if successful) the corresponding objects to the repo's local
storage.

This commit does not include support for generating such a repo; neither
has any command (other than fsck) been modified to either tolerate
missing blobs (without invoking the hook) or be more efficient in
invoking the missing blob hook. Only a fallback is provided in the form
of sha1_file invoking the missing blob hook when necessary.

In order to determine the code changes in sha1_file.c necessary, I
investigated the following:
 (1) functions in sha1_file that take in a hash, without the user
 regarding how the object is stored (loose or packed)
 (2) functions in sha1_file that operate on packed objects (because I
 need to check callers that know about the loose/packed distinction
 and operate on both differently, and ensure that they can handle
 the concept of objects that are neither loose nor packed)

(1) is handled by the modification to sha1_object_info_extended().

For (2), I looked through the same functions as in (1) and also
for_each_packed_object. The ones that are relevant are:
 - parse_pack_index
   - http - indirectly from http_get_info_packs
 - find_pack_entry_one
   - this searches a single pack that is provided as an argument; the
 caller already knows (through other means) that the sought object
 is in a specific pack
 - find_sha1_pack
   - fast-import - appears to be an optimization to not store a
 file if it is already in a pack
   - http-walker - to search through a struct alt_base
   - http-push - to search through remote packs
 - has_sha1_pack
   - builtin/fsck - fixed in this commit
   - builtin/count-objects - informational purposes only (check if loose
 object is also packed)
   - builtin/prune-packed - check if object to be pruned is packed (if
 not, don't prune it)
   - revision - used to exclude packed objects if requested by user
   - diff - just for optimization
 - for_each_packed_object
   - reachable - only to find recent objects
   - builtin/fsck - fixed in this commit
   - builtin/cat-file - see below

As described in the list above, builtin/fsck has been updated. I have
left builtin/cat-file alone; this means that cat-file
--batch-all-objects will only operate on objects physically in the repo.

An alternative design that I considered but rejected:

 - Adding a hook whenever a packed blob is requested, not on any blob.
   That is, whenever we attempt to search the packfiles for a blob, if
   it is missing (from the packfiles and from the loose object storage),
   to invoke the hook (which must then store it as a packfile), open the
   packfile the hook generated, and report that the blob is found in
   that new packfile. This reduces the amount of analysis needed (in
   that we only need to look at how packed blobs are handled), but
   requires that the hook generate packfiles (or for sha1_file to pack
   whatever loose objects are generated), creating one packfile for each
   missing blob and potentially very many packfiles that must be
   linearly searched. This may be tolerable now for repos that only have
   a few missing blobs (for example, repos that only want to exclude
   large blobs), and might be tolerable in the future if we have
   batching support for the most commonly used commands, but is not
   tolerable now for repos that exclude a large amount of blobs.

Signed-off-by: Jonathan Tan 
---
 Documentation/config.txt |  10 +++
 builtin/fsck.c   |   7 +++
 cache.h  |   6 ++
 sha1_file.c  | 156 ++-
 t/t3907-missing-blob.sh  |  69 +
 5 files changed, 233 insertions(+), 15 deletions(-)
 create mode 100755 t/t3907-missing-blob.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index dd4beec39..10da5fde1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -390,6 +390,16 @@ The default is false, except linkgit:git-clone[1] or 
linkgit:git-init[1]
 will probe and set core.ignoreCase true if appropriate when the repository
 is created.
 
+core.missingBlobCommand::
+   If set, whenever a blob in the local repo is attempted to be
+   read but is missing, invoke this shell 

[PATCH v2 0/4] Improvements to sha1_file

[Jonathan Tan]

Peff suggested putting in a new field in struct object_info for the
object contents; I tried it and it seems to work out quite well.

Patch 1 is unmodified from the previous version. Patches 2-3 have been
rewritten, and patch 4 is similar except that the missing-lookup change
is made to sha1_object_info_extended() instead of the now gone
get_object().

As before, I would like review on patches 1-3 to go into the tree.
(Patch 4 is a work in progress, and is here just to demonstrate the
effectiveness of the refactoring.)

Jonathan Tan (4):
  sha1_file: teach packed_object_info about typename
  sha1_file: move delta base cache code up
  sha1_file: consolidate storage-agnostic object fns
  sha1_file, fsck: add missing blob support

 Documentation/config.txt |  10 +
 builtin/fsck.c   |   7 +
 cache.h  |  13 ++
 sha1_file.c  | 506 +--
 t/t3907-missing-blob.sh  |  69 +++
 5 files changed, 418 insertions(+), 187 deletions(-)
 create mode 100755 t/t3907-missing-blob.sh

-- 
2.13.1.518.g3df882009-goog

[PATCH v2 3/4] sha1_file: consolidate storage-agnostic object fns

[Jonathan Tan]

In sha1_file.c, there are a few functions that provide information on an
object regardless of its storage (cached, loose, or packed). Looking
through all non-static functions in sha1_file.c that take in an unsigned
char * pointer, the relevant ones are:
 - sha1_object_info_extended
 - sha1_object_info (auto-fixed by sha1_object_info_extended)
 - read_sha1_file_extended (uses read_object)
 - read_object_with_reference (auto-fixed by read_sha1_file_extended)
 - has_sha1_file_with_flags
 - assert_sha1_type (auto-fixed by sha1_object_info)

Looking at the 3 primary functions (sha1_object_info_extended,
read_object, has_sha1_file_with_flags), they independently implement
mechanisms such as object replacement, retrying the packed store after
failing to find the object in the packed store then the loose store, and
being able to mark a packed object as bad and then retrying the whole
process. Consolidating these mechanisms would be a great help to
maintainability.

Therefore, consolidate all 3 functions by extending
sha1_object_info_extended() to support the functionality needed by all 3
functions, and then modifying the other 2 to use
sha1_object_info_extended().

Note that has_sha1_file_with_flags() does not try cached storage,
whereas the other 2 functions do - this functionality is preserved.

Signed-off-by: Jonathan Tan 
---
 cache.h |   7 +++
 sha1_file.c | 143 +++-
 2 files changed, 81 insertions(+), 69 deletions(-)

diff --git a/cache.h b/cache.h
index 4d92aae0e..3c85867c3 100644
--- a/cache.h
+++ b/cache.h
@@ -1835,6 +1835,7 @@ struct object_info {
off_t *disk_sizep;
unsigned char *delta_base_sha1;
struct strbuf *typename;
+   void **contentp;
 
/* Response */
enum {
@@ -1866,6 +1867,12 @@ struct object_info {
  */
 #define OBJECT_INFO_INIT {NULL}
 
+/*
+ * sha1_object_info_extended() supports the LOOKUP_ flags and the OBJECT_INFO_
+ * flags.
+ */
+#define OBJECT_INFO_QUICK 4
+#define OBJECT_INFO_SKIP_CACHED 8
 extern int sha1_object_info_extended(const unsigned char *, struct object_info 
*, unsigned flags);
 extern int packed_object_info(struct packed_git *pack, off_t offset, struct 
object_info *);
 
diff --git a/sha1_file.c b/sha1_file.c
index a158907d1..98086e21e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2005,19 +2005,6 @@ int parse_sha1_header(const char *hdr, unsigned long 
*sizep)
return parse_sha1_header_extended(hdr, , LOOKUP_REPLACE_OBJECT);
 }
 
-static void *unpack_sha1_file(void *map, unsigned long mapsize, enum 
object_type *type, unsigned long *size, const unsigned char *sha1)
-{
-   int ret;
-   git_zstream stream;
-   char hdr[8192];
-
-   ret = unpack_sha1_header(, map, mapsize, hdr, sizeof(hdr));
-   if (ret < Z_OK || (*type = parse_sha1_header(hdr, size)) < 0)
-   return NULL;
-
-   return unpack_sha1_rest(, hdr, *size, sha1);
-}
-
 unsigned long get_size_from_delta(struct packed_git *p,
  struct pack_window **w_curs,
  off_t curpos)
@@ -2326,8 +2313,10 @@ static void *cache_or_unpack_entry(struct packed_git *p, 
off_t base_offset,
if (!ent)
return unpack_entry(p, base_offset, type, base_size);
 
-   *type = ent->type;
-   *base_size = ent->size;
+   if (type)
+   *type = ent->type;
+   if (base_size)
+   *base_size = ent->size;
return xmemdupz(ent->data, ent->size);
 }
 
@@ -2388,9 +2377,16 @@ int packed_object_info(struct packed_git *p, off_t 
obj_offset,
 * We always get the representation type, but only convert it to
 * a "real" type later if the caller is interested.
 */
-   type = unpack_object_header(p, _curs, , );
+   if (oi->contentp) {
+   *oi->contentp = cache_or_unpack_entry(p, obj_offset, oi->sizep,
+ );
+   if (!*oi->contentp)
+   type = OBJ_BAD;
+   } else {
+   type = unpack_object_header(p, _curs, , );
+   }
 
-   if (oi->sizep) {
+   if (!oi->contentp && oi->sizep) {
if (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {
off_t tmp_pos = curpos;
off_t base_offset = get_delta_base(p, _curs, _pos,
@@ -2685,8 +2681,10 @@ void *unpack_entry(struct packed_git *p, off_t 
obj_offset,
free(external_base);
}
 
-   *final_type = type;
-   *final_size = size;
+   if (final_type)
+   *final_type = type;
+   if (final_size)
+   *final_size = size;
 
unuse_pack(_curs);
 
@@ -2920,6 +2918,7 @@ static int sha1_loose_object_info(const unsigned char 
*sha1,
git_zstream stream;
char hdr[32];
struct strbuf hdrbuf = STRBUF_INIT;
+   unsigned long size_scratch;
 

[PATCH v2 1/4] sha1_file: teach packed_object_info about typename

[Jonathan Tan]

In commit 46f0344 ("sha1_file: support reading from a loose object of
unknown type", 2015-05-06), "struct object_info" gained a "typename"
field that could represent a type name from a loose object file, whether
valid or invalid, as opposed to the existing "typep" which could only
represent valid types. Some relatively complex manipulations were added
to avoid breaking packed_object_info() without modifying it, but it is
much easier to just teach packed_object_info() about the new field.
Therefore, teach packed_object_info() as described above.

Signed-off-by: Jonathan Tan 
---
 sha1_file.c | 29 -
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 59a4ed2ed..a52b27541 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2277,9 +2277,18 @@ int packed_object_info(struct packed_git *p, off_t 
obj_offset,
*oi->disk_sizep = revidx[1].offset - obj_offset;
}
 
-   if (oi->typep) {
-   *oi->typep = packed_to_object_type(p, obj_offset, type, 
_curs, curpos);
-   if (*oi->typep < 0) {
+   if (oi->typep || oi->typename) {
+   enum object_type ptot;
+   ptot = packed_to_object_type(p, obj_offset, type, _curs,
+curpos);
+   if (oi->typep)
+   *oi->typep = ptot;
+   if (oi->typename) {
+   const char *tn = typename(ptot);
+   if (tn)
+   strbuf_addstr(oi->typename, tn);
+   }
+   if (ptot < 0) {
type = OBJ_BAD;
goto out;
}
@@ -2960,7 +2969,6 @@ int sha1_object_info_extended(const unsigned char *sha1, 
struct object_info *oi,
struct cached_object *co;
struct pack_entry e;
int rtype;
-   enum object_type real_type;
const unsigned char *real = lookup_replace_object_extended(sha1, flags);
 
co = find_cached_object(real);
@@ -2992,18 +3000,9 @@ int sha1_object_info_extended(const unsigned char *sha1, 
struct object_info *oi,
return -1;
}
 
-   /*
-* packed_object_info() does not follow the delta chain to
-* find out the real type, unless it is given oi->typep.
-*/
-   if (oi->typename && !oi->typep)
-   oi->typep = _type;
-
rtype = packed_object_info(e.p, e.offset, oi);
if (rtype < 0) {
mark_bad_packed_object(e.p, real);
-   if (oi->typep == _type)
-   oi->typep = NULL;
return sha1_object_info_extended(real, oi, 0);
} else if (in_delta_base_cache(e.p, e.offset)) {
oi->whence = OI_DBCACHED;
@@ -3014,10 +3013,6 @@ int sha1_object_info_extended(const unsigned char *sha1, 
struct object_info *oi,
oi->u.packed.is_delta = (rtype == OBJ_REF_DELTA ||
 rtype == OBJ_OFS_DELTA);
}
-   if (oi->typename)
-   strbuf_addstr(oi->typename, typename(*oi->typep));
-   if (oi->typep == _type)
-   oi->typep = NULL;
 
return 0;
 }
-- 
2.13.1.518.g3df882009-goog

[PATCH v2 2/4] sha1_file: move delta base cache code up

[Jonathan Tan]

In a subsequent patch, packed_object_info() will be modified to use the
delta base cache, so move the relevant code to before
packed_object_info().

Signed-off-by: Jonathan Tan 
---
 sha1_file.c | 226 +++-
 1 file changed, 116 insertions(+), 110 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index a52b27541..a158907d1 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2239,116 +2239,6 @@ static enum object_type packed_to_object_type(struct 
packed_git *p,
goto out;
 }
 
-int packed_object_info(struct packed_git *p, off_t obj_offset,
-  struct object_info *oi)
-{
-   struct pack_window *w_curs = NULL;
-   unsigned long size;
-   off_t curpos = obj_offset;
-   enum object_type type;
-
-   /*
-* We always get the representation type, but only convert it to
-* a "real" type later if the caller is interested.
-*/
-   type = unpack_object_header(p, _curs, , );
-
-   if (oi->sizep) {
-   if (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {
-   off_t tmp_pos = curpos;
-   off_t base_offset = get_delta_base(p, _curs, _pos,
-  type, obj_offset);
-   if (!base_offset) {
-   type = OBJ_BAD;
-   goto out;
-   }
-   *oi->sizep = get_size_from_delta(p, _curs, tmp_pos);
-   if (*oi->sizep == 0) {
-   type = OBJ_BAD;
-   goto out;
-   }
-   } else {
-   *oi->sizep = size;
-   }
-   }
-
-   if (oi->disk_sizep) {
-   struct revindex_entry *revidx = find_pack_revindex(p, 
obj_offset);
-   *oi->disk_sizep = revidx[1].offset - obj_offset;
-   }
-
-   if (oi->typep || oi->typename) {
-   enum object_type ptot;
-   ptot = packed_to_object_type(p, obj_offset, type, _curs,
-curpos);
-   if (oi->typep)
-   *oi->typep = ptot;
-   if (oi->typename) {
-   const char *tn = typename(ptot);
-   if (tn)
-   strbuf_addstr(oi->typename, tn);
-   }
-   if (ptot < 0) {
-   type = OBJ_BAD;
-   goto out;
-   }
-   }
-
-   if (oi->delta_base_sha1) {
-   if (type == OBJ_OFS_DELTA || type == OBJ_REF_DELTA) {
-   const unsigned char *base;
-
-   base = get_delta_base_sha1(p, _curs, curpos,
-  type, obj_offset);
-   if (!base) {
-   type = OBJ_BAD;
-   goto out;
-   }
-
-   hashcpy(oi->delta_base_sha1, base);
-   } else
-   hashclr(oi->delta_base_sha1);
-   }
-
-out:
-   unuse_pack(_curs);
-   return type;
-}
-
-static void *unpack_compressed_entry(struct packed_git *p,
-   struct pack_window **w_curs,
-   off_t curpos,
-   unsigned long size)
-{
-   int st;
-   git_zstream stream;
-   unsigned char *buffer, *in;
-
-   buffer = xmallocz_gently(size);
-   if (!buffer)
-   return NULL;
-   memset(, 0, sizeof(stream));
-   stream.next_out = buffer;
-   stream.avail_out = size + 1;
-
-   git_inflate_init();
-   do {
-   in = use_pack(p, w_curs, curpos, _in);
-   stream.next_in = in;
-   st = git_inflate(, Z_FINISH);
-   if (!stream.avail_out)
-   break; /* the payload is larger than it should be */
-   curpos += stream.next_in - in;
-   } while (st == Z_OK || st == Z_BUF_ERROR);
-   git_inflate_end();
-   if ((st != Z_STREAM_END) || stream.total_out != size) {
-   free(buffer);
-   return NULL;
-   }
-
-   return buffer;
-}
-
 static struct hashmap delta_base_cache;
 static size_t delta_base_cached;
 
@@ -2486,6 +2376,122 @@ static void add_delta_base_cache(struct packed_git *p, 
off_t base_offset,
hashmap_add(_base_cache, ent);
 }
 
+int packed_object_info(struct packed_git *p, off_t obj_offset,
+  struct object_info *oi)
+{
+   struct pack_window *w_curs = NULL;
+   unsigned long size;
+   off_t curpos = obj_offset;
+   enum object_type type;
+
+   /*
+* We always get the representation type, but only convert it to
+* a "real" type later if the 

[PATCH v2 6/6] config: respect commondir

[Brandon Williams]

Worktrees present an interesting problem when it comes to the config.
Historically we could assume that the per-repository config lives at
'gitdir/config', but since worktrees were introduced this isn't the case
anymore.  There is currently no way to specify per-worktree
configuration, and as such the repository config is shared with all
worktrees and is located at 'commondir/config'.

Many users of the config machinery correctly set
'config_options.git_dir' with the repository's commondir, allowing the
config to be properly loaded when operating in a worktree.  But other's,
like 'read_early_config()', set 'config_options.git_dir' with the
repository's gitdir which can be incorrect when using worktrees.

To fix this issue, and to make things less ambiguous, lets add a
'commondir' field to the 'config_options' struct and have all callers
properly set both the 'git_dir' and 'commondir' fields so that the
config machinery is able to properly find the repository's config.

Signed-off-by: Brandon Williams 
---
 builtin/config.c |  6 --
 config.c | 18 --
 config.h |  1 +
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index f06a8dff2..8b6e227c5 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -539,8 +539,10 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
config_options.respect_includes = !given_config_source.file;
else
config_options.respect_includes = respect_includes_opt;
-   if (!nongit)
-   config_options.git_dir = get_git_common_dir();
+   if (!nongit) {
+   config_options.commondir = get_git_common_dir();
+   config_options.git_dir = get_git_dir();
+   }
 
if (end_null) {
term = '\0';
diff --git a/config.c b/config.c
index 9aa9b9715..81151fb54 100644
--- a/config.c
+++ b/config.c
@@ -1544,8 +1544,8 @@ static int do_git_config_sequence(const struct 
config_options *opts,
char *user_config = expand_user_path("~/.gitconfig", 0);
char *repo_config;
 
-   if (opts->git_dir)
-   repo_config = mkpathdup("%s/config", opts->git_dir);
+   if (opts->commondir)
+   repo_config = mkpathdup("%s/config", opts->commondir);
else
repo_config = NULL;
 
@@ -1609,8 +1609,11 @@ static void git_config_raw(config_fn_t fn, void *data)
struct config_options opts = {0};
 
opts.respect_includes = 1;
-   if (have_git_dir())
-   opts.git_dir = get_git_common_dir();
+   if (have_git_dir()) {
+   opts.commondir = get_git_common_dir();
+   opts.git_dir = get_git_dir();
+   }
+
if (git_config_with_options(fn, data, NULL, ) < 0)
/*
 * git_config_with_options() normally returns only
@@ -1657,7 +1660,8 @@ void read_early_config(config_fn_t cb, void *data)
 
opts.respect_includes = 1;
 
-   if (have_git_dir())
+   if (have_git_dir()) {
+   opts.commondir = get_git_common_dir();
opts.git_dir = get_git_dir();
/*
 * When setup_git_directory() was not yet asked to discover the
@@ -1667,8 +1671,10 @@ void read_early_config(config_fn_t cb, void *data)
 * notably, the current working directory is still the same after the
 * call).
 */
-   else if (discover_git_directory(, ))
+   } else if (discover_git_directory(, )) {
+   opts.commondir = commondir.buf;
opts.git_dir = gitdir.buf;
+   }
 
git_config_with_options(cb, data, NULL, );
 
diff --git a/config.h b/config.h
index c70599bd5..63b92784c 100644
--- a/config.h
+++ b/config.h
@@ -30,6 +30,7 @@ enum config_origin_type {
 
 struct config_options {
unsigned int respect_includes : 1;
+   const char *commondir;
const char *git_dir;
 };
 
-- 
2.13.1.518.g3df882009-goog

[PATCH v2 3/6] config: don't include config.h by default

[Brandon Williams]

Stop including config.h by default in cache.h.  Instead only include
config.h in those files which require use of the config system.

Signed-off-by: Brandon Williams 
---
 advice.c | 1 +
 alias.c  | 1 +
 apply.c  | 1 +
 archive-tar.c| 1 +
 archive-zip.c| 1 +
 archive.c| 1 +
 attr.c   | 1 +
 bisect.c | 1 +
 branch.c | 1 +
 builtin/add.c| 1 +
 builtin/am.c | 1 +
 builtin/blame.c  | 2 ++
 builtin/branch.c | 1 +
 builtin/cat-file.c   | 1 +
 builtin/check-attr.c | 1 +
 builtin/check-ignore.c   | 1 +
 builtin/check-mailmap.c  | 1 +
 builtin/checkout-index.c | 1 +
 builtin/checkout.c   | 1 +
 builtin/clean.c  | 1 +
 builtin/clone.c  | 1 +
 builtin/column.c | 1 +
 builtin/commit-tree.c| 1 +
 builtin/commit.c | 1 +
 builtin/config.c | 1 +
 builtin/count-objects.c  | 1 +
 builtin/describe.c   | 1 +
 builtin/diff-files.c | 1 +
 builtin/diff-index.c | 1 +
 builtin/diff-tree.c  | 1 +
 builtin/diff.c   | 1 +
 builtin/difftool.c   | 1 +
 builtin/fast-export.c| 1 +
 builtin/fetch.c  | 1 +
 builtin/fmt-merge-msg.c  | 1 +
 builtin/for-each-ref.c   | 1 +
 builtin/fsck.c   | 1 +
 builtin/gc.c | 1 +
 builtin/grep.c   | 1 +
 builtin/hash-object.c| 1 +
 builtin/help.c   | 1 +
 builtin/index-pack.c | 1 +
 builtin/init-db.c| 1 +
 builtin/log.c| 1 +
 builtin/ls-files.c   | 1 +
 builtin/ls-tree.c| 1 +
 builtin/merge-base.c | 1 +
 builtin/merge-file.c | 1 +
 builtin/merge.c  | 1 +
 builtin/mv.c | 1 +
 builtin/name-rev.c   | 1 +
 builtin/notes.c  | 1 +
 builtin/pack-objects.c   | 1 +
 builtin/patch-id.c   | 1 +
 builtin/pull.c   | 1 +
 builtin/push.c   | 1 +
 builtin/read-tree.c  | 1 +
 builtin/rebase--helper.c | 1 +
 builtin/receive-pack.c   | 1 +
 builtin/reflog.c | 1 +
 builtin/remote.c | 1 +
 builtin/repack.c | 1 +
 builtin/replace.c| 1 +
 builtin/rerere.c | 1 +
 builtin/reset.c  | 1 +
 builtin/rev-list.c   | 1 +
 builtin/rev-parse.c  | 1 +
 builtin/revert.c | 1 +
 builtin/rm.c | 1 +
 builtin/send-pack.c  | 1 +
 builtin/shortlog.c   | 1 +
 builtin/show-branch.c| 1 +
 builtin/stripspace.c | 1 +
 builtin/submodule--helper.c  | 1 +
 builtin/symbolic-ref.c   | 1 +
 builtin/tag.c| 1 +
 builtin/unpack-file.c| 1 +
 builtin/unpack-objects.c | 1 +
 builtin/update-index.c   | 1 +
 builtin/update-ref.c | 1 +
 builtin/update-server-info.c | 1 +
 builtin/var.c| 1 +
 builtin/verify-commit.c  | 1 +
 builtin/verify-pack.c| 1 +
 builtin/verify-tag.c | 1 +
 builtin/worktree.c   | 1 +
 builtin/write-tree.c | 1 +
 cache.h  | 1 -
 color.c  | 1 +
 column.c | 1 +
 config.c | 1 +
 connect.c| 1 +
 convert.c| 1 +
 credential-cache--daemon.c   | 1 +
 credential.c | 1 +
 daemon.c | 1 +
 diff.c   | 1 +
 dir.c| 1 +
 environment.c| 1 +
 fast-import.c| 1 +
 fetch-pack.c | 1 +
 git.c| 1 +
 gpg-interface.c  | 1 +
 graph.c  | 1 +
 grep.c   | 1 +
 help.c   | 1 +
 http-backend.c   | 1 +
 http-fetch.c | 1 +
 http.c   | 1 +
 ident.c  | 1 +
 imap-send.c  | 1 +
 ll-merge.c   | 1 +
 log-tree.c   | 1 +
 mailinfo.c   | 1 +
 merge-recursive.c| 1 +
 notes-utils.c| 1 +
 notes.c  | 1 +
 pager.c  | 1 +
 parse-options.c  | 1 +
 pathspec.c   | 1 +
 

[PATCH v2 4/6] config: don't implicitly use gitdir

[Brandon Williams]

Commit 2185fde56 (config: handle conditional include when $GIT_DIR is
not set up) added a 'git_dir' field to the config_options struct.  Let's
use this option field explicitly all the time instead of occasionally
falling back to calling 'git_pathdup("config")' to get the path to the
local repository configuration.  This allows 'do_git_config_sequence()'
to not implicitly rely on global repository state.

Signed-off-by: Brandon Williams 
---
 builtin/config.c | 2 ++
 config.c | 6 ++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index 753c40a5c..f06a8dff2 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -539,6 +539,8 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
config_options.respect_includes = !given_config_source.file;
else
config_options.respect_includes = respect_includes_opt;
+   if (!nongit)
+   config_options.git_dir = get_git_common_dir();
 
if (end_null) {
term = '\0';
diff --git a/config.c b/config.c
index 2390f98e3..4e2842689 100644
--- a/config.c
+++ b/config.c
@@ -219,8 +219,6 @@ static int include_by_gitdir(const struct config_options 
*opts,
 
if (opts->git_dir)
git_dir = opts->git_dir;
-   else if (have_git_dir())
-   git_dir = get_git_dir();
else
goto done;
 
@@ -1548,8 +1546,6 @@ static int do_git_config_sequence(const struct 
config_options *opts,
 
if (opts->git_dir)
repo_config = mkpathdup("%s/config", opts->git_dir);
-   else if (have_git_dir())
-   repo_config = git_pathdup("config");
else
repo_config = NULL;
 
@@ -1613,6 +1609,8 @@ static void git_config_raw(config_fn_t fn, void *data)
struct config_options opts = {0};
 
opts.respect_includes = 1;
+   if (have_git_dir())
+   opts.git_dir = get_git_common_dir();
if (git_config_with_options(fn, data, NULL, ) < 0)
/*
 * git_config_with_options() normally returns only
-- 
2.13.1.518.g3df882009-goog

[PATCH v2 2/6] config: remove git_config_iter

[Brandon Williams]

Since there is no implementation of the function 'git_config_iter' lets
stop exporting it and remove the prototype from config.h.

Signed-off-by: Brandon Williams 
---
 config.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/config.h b/config.h
index f7f8b66c5..c70599bd5 100644
--- a/config.h
+++ b/config.h
@@ -165,7 +165,6 @@ extern int git_configset_get_pathname(struct config_set 
*cs, const char *key, co
 extern int git_config_get_value(const char *key, const char **value);
 extern const struct string_list *git_config_get_value_multi(const char *key);
 extern void git_config_clear(void);
-extern void git_config_iter(config_fn_t fn, void *data);
 extern int git_config_get_string_const(const char *key, const char **dest);
 extern int git_config_get_string(const char *key, char **dest);
 extern int git_config_get_int(const char *key, int *dest);
-- 
2.13.1.518.g3df882009-goog

[PATCH v2 5/6] setup: teach discover_git_directory to respect the commondir

[Brandon Williams]

Currently 'discover_git_directory' only looks at the gitdir to determine
if a git directory was discovered.  This causes a problem in the event
that the gitdir which was discovered was in fact a per-worktree git
directory and not the common git directory.  This is because the
repository config, which is checked to verify the repository's format,
is stored in the commondir and not in the per-worktree gitdir.  Correct
this behavior by checking the config stored in the commondir.

It will also be of use for callers to have access to the commondir, so
lets also return that upon successfully discovering a git directory.

Signed-off-by: Brandon Williams 
---
 cache.h  |  3 ++-
 config.c | 10 ++
 setup.c  |  9 +++--
 3 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/cache.h b/cache.h
index fd45b8c55..a4176436d 100644
--- a/cache.h
+++ b/cache.h
@@ -530,7 +530,8 @@ extern void setup_work_tree(void);
  * appended to gitdir. The return value is either NULL if no repository was
  * found, or pointing to the path inside gitdir's buffer.
  */
-extern const char *discover_git_directory(struct strbuf *gitdir);
+extern const char *discover_git_directory(struct strbuf *commondir,
+ struct strbuf *gitdir);
 extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);
 extern char *prefix_path(const char *prefix, int len, const char *path);
diff --git a/config.c b/config.c
index 4e2842689..9aa9b9715 100644
--- a/config.c
+++ b/config.c
@@ -1652,7 +1652,8 @@ static void configset_iter(struct config_set *cs, 
config_fn_t fn, void *data)
 void read_early_config(config_fn_t cb, void *data)
 {
struct config_options opts = {0};
-   struct strbuf buf = STRBUF_INIT;
+   struct strbuf commondir = STRBUF_INIT;
+   struct strbuf gitdir = STRBUF_INIT;
 
opts.respect_includes = 1;
 
@@ -1666,12 +1667,13 @@ void read_early_config(config_fn_t cb, void *data)
 * notably, the current working directory is still the same after the
 * call).
 */
-   else if (discover_git_directory())
-   opts.git_dir = buf.buf;
+   else if (discover_git_directory(, ))
+   opts.git_dir = gitdir.buf;
 
git_config_with_options(cb, data, NULL, );
 
-   strbuf_release();
+   strbuf_release();
+   strbuf_release();
 }
 
 static void git_config_check_init(void);
diff --git a/setup.c b/setup.c
index e99a82cbe..7bbb8736f 100644
--- a/setup.c
+++ b/setup.c
@@ -946,10 +946,12 @@ static enum discovery_result 
setup_git_directory_gently_1(struct strbuf *dir,
}
 }
 
-const char *discover_git_directory(struct strbuf *gitdir)
+const char *discover_git_directory(struct strbuf *commondir,
+  struct strbuf *gitdir)
 {
struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
size_t gitdir_offset = gitdir->len, cwd_len;
+   size_t commondir_offset = commondir->len;
struct repository_format candidate;
 
if (strbuf_getcwd())
@@ -974,8 +976,10 @@ const char *discover_git_directory(struct strbuf *gitdir)
strbuf_insert(gitdir, gitdir_offset, dir.buf, dir.len);
}
 
+   get_common_dir(commondir, gitdir->buf + gitdir_offset);
+
strbuf_reset();
-   strbuf_addf(, "%s/config", gitdir->buf + gitdir_offset);
+   strbuf_addf(, "%s/config", commondir->buf + commondir_offset);
read_repository_format(, dir.buf);
strbuf_release();
 
@@ -983,6 +987,7 @@ const char *discover_git_directory(struct strbuf *gitdir)
warning("ignoring git dir '%s': %s",
gitdir->buf + gitdir_offset, err.buf);
strbuf_release();
+   strbuf_setlen(commondir, commondir_offset);
return NULL;
}
 
-- 
2.13.1.518.g3df882009-goog

[PATCH v2 0/6] config.h

[Brandon Williams]

Changes in v2:
 * Fix a small nit in builtin/config.c that Jonathan pointed out.
 * Added two patches which ensure that the repository wide config is properly
   read by providing 'commondir' as a field in the 'config_options' struct.

Brandon Williams (6):
  config: create config.h
  config: remove git_config_iter
  config: don't include config.h by default
  config: don't implicitly use gitdir
  setup: teach discover_git_directory to respect the commondir
  config: respect commondir

--- interdiff with 'origin/bw/config-h'

diff --git a/builtin/config.c b/builtin/config.c
index 90f49a6ee..8b6e227c5 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -539,8 +539,10 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
config_options.respect_includes = !given_config_source.file;
else
config_options.respect_includes = respect_includes_opt;
-   if (have_git_dir())
-   config_options.git_dir = get_git_common_dir();
+   if (!nongit) {
+   config_options.commondir = get_git_common_dir();
+   config_options.git_dir = get_git_dir();
+   }
 
if (end_null) {
term = '\0';
diff --git a/cache.h b/cache.h
index fd45b8c55..a4176436d 100644
--- a/cache.h
+++ b/cache.h
@@ -530,7 +530,8 @@ extern void setup_work_tree(void);
  * appended to gitdir. The return value is either NULL if no repository was
  * found, or pointing to the path inside gitdir's buffer.
  */
-extern const char *discover_git_directory(struct strbuf *gitdir);
+extern const char *discover_git_directory(struct strbuf *commondir,
+ struct strbuf *gitdir);
 extern const char *setup_git_directory_gently(int *);
 extern const char *setup_git_directory(void);
 extern char *prefix_path(const char *prefix, int len, const char *path);
diff --git a/config.c b/config.c
index 4e2842689..81151fb54 100644
--- a/config.c
+++ b/config.c
@@ -1544,8 +1544,8 @@ static int do_git_config_sequence(const struct 
config_options *opts,
char *user_config = expand_user_path("~/.gitconfig", 0);
char *repo_config;
 
-   if (opts->git_dir)
-   repo_config = mkpathdup("%s/config", opts->git_dir);
+   if (opts->commondir)
+   repo_config = mkpathdup("%s/config", opts->commondir);
else
repo_config = NULL;
 
@@ -1609,8 +1609,11 @@ static void git_config_raw(config_fn_t fn, void *data)
struct config_options opts = {0};
 
opts.respect_includes = 1;
-   if (have_git_dir())
-   opts.git_dir = get_git_common_dir();
+   if (have_git_dir()) {
+   opts.commondir = get_git_common_dir();
+   opts.git_dir = get_git_dir();
+   }
+
if (git_config_with_options(fn, data, NULL, ) < 0)
/*
 * git_config_with_options() normally returns only
@@ -1652,11 +1655,13 @@ static void configset_iter(struct config_set *cs, 
config_fn_t fn, void *data)
 void read_early_config(config_fn_t cb, void *data)
 {
struct config_options opts = {0};
-   struct strbuf buf = STRBUF_INIT;
+   struct strbuf commondir = STRBUF_INIT;
+   struct strbuf gitdir = STRBUF_INIT;
 
opts.respect_includes = 1;
 
-   if (have_git_dir())
+   if (have_git_dir()) {
+   opts.commondir = get_git_common_dir();
opts.git_dir = get_git_dir();
/*
 * When setup_git_directory() was not yet asked to discover the
@@ -1666,12 +1671,15 @@ void read_early_config(config_fn_t cb, void *data)
 * notably, the current working directory is still the same after the
 * call).
 */
-   else if (discover_git_directory())
-   opts.git_dir = buf.buf;
+   } else if (discover_git_directory(, )) {
+   opts.commondir = commondir.buf;
+   opts.git_dir = gitdir.buf;
+   }
 
git_config_with_options(cb, data, NULL, );
 
-   strbuf_release();
+   strbuf_release();
+   strbuf_release();
 }
 
 static void git_config_check_init(void);
diff --git a/config.h b/config.h
index c70599bd5..63b92784c 100644
--- a/config.h
+++ b/config.h
@@ -30,6 +30,7 @@ enum config_origin_type {
 
 struct config_options {
unsigned int respect_includes : 1;
+   const char *commondir;
const char *git_dir;
 };
 
diff --git a/setup.c b/setup.c
index e99a82cbe..7bbb8736f 100644
--- a/setup.c
+++ b/setup.c
@@ -946,10 +946,12 @@ static enum discovery_result 
setup_git_directory_gently_1(struct strbuf *dir,
}
 }
 
-const char *discover_git_directory(struct strbuf *gitdir)
+const char *discover_git_directory(struct strbuf *commondir,
+  struct strbuf *gitdir)
 {
struct strbuf dir = STRBUF_INIT, err = STRBUF_INIT;
size_t gitdir_offset = gitdir->len, cwd_len;
+   size_t commondir_offset = commondir->len;
struct 

[PATCH v2 1/6] config: create config.h

[Brandon Williams]

Move all config related declarations from cache.h to a new config.h
header file.  This makes cache.h smaller and allows for the opportunity
in a following patch to only include config.h when needed.

Signed-off-by: Brandon Williams 
---
 cache.h  | 190 +
 config.h | 194 +++
 2 files changed, 195 insertions(+), 189 deletions(-)
 create mode 100644 config.h

diff --git a/cache.h b/cache.h
index 4d92aae0e..ed56f8818 100644
--- a/cache.h
+++ b/cache.h
@@ -11,6 +11,7 @@
 #include "string-list.h"
 #include "pack-revindex.h"
 #include "hash.h"
+#include "config.h"
 
 #ifndef platform_SHA_CTX
 /*
@@ -1872,188 +1873,9 @@ extern int packed_object_info(struct packed_git *pack, 
off_t offset, struct obje
 /* Dumb servers support */
 extern int update_server_info(int);
 
-/* git_config_parse_key() returns these negated: */
-#define CONFIG_INVALID_KEY 1
-#define CONFIG_NO_SECTION_OR_NAME 2
-/* git_config_set_gently(), git_config_set_multivar_gently() return the above 
or these: */
-#define CONFIG_NO_LOCK -1
-#define CONFIG_INVALID_FILE 3
-#define CONFIG_NO_WRITE 4
-#define CONFIG_NOTHING_SET 5
-#define CONFIG_INVALID_PATTERN 6
-#define CONFIG_GENERIC_ERROR 7
-
-#define CONFIG_REGEX_NONE ((void *)1)
-
-struct git_config_source {
-   unsigned int use_stdin:1;
-   const char *file;
-   const char *blob;
-};
-
-enum config_origin_type {
-   CONFIG_ORIGIN_BLOB,
-   CONFIG_ORIGIN_FILE,
-   CONFIG_ORIGIN_STDIN,
-   CONFIG_ORIGIN_SUBMODULE_BLOB,
-   CONFIG_ORIGIN_CMDLINE
-};
-
-struct config_options {
-   unsigned int respect_includes : 1;
-   const char *git_dir;
-};
-
-typedef int (*config_fn_t)(const char *, const char *, void *);
-extern int git_default_config(const char *, const char *, void *);
-extern int git_config_from_file(config_fn_t fn, const char *, void *);
-extern int git_config_from_mem(config_fn_t fn, const enum config_origin_type,
-   const char *name, const char *buf, 
size_t len, void *data);
-extern int git_config_from_blob_sha1(config_fn_t fn, const char *name,
-const unsigned char *sha1, void *data);
-extern void git_config_push_parameter(const char *text);
-extern int git_config_from_parameters(config_fn_t fn, void *data);
-extern void read_early_config(config_fn_t cb, void *data);
-extern void git_config(config_fn_t fn, void *);
-extern int git_config_with_options(config_fn_t fn, void *,
-  struct git_config_source *config_source,
-  const struct config_options *opts);
-extern int git_parse_ulong(const char *, unsigned long *);
-extern int git_parse_maybe_bool(const char *);
-extern int git_config_int(const char *, const char *);
-extern int64_t git_config_int64(const char *, const char *);
-extern unsigned long git_config_ulong(const char *, const char *);
-extern ssize_t git_config_ssize_t(const char *, const char *);
-extern int git_config_bool_or_int(const char *, const char *, int *);
-extern int git_config_bool(const char *, const char *);
-extern int git_config_maybe_bool(const char *, const char *);
-extern int git_config_string(const char **, const char *, const char *);
-extern int git_config_pathname(const char **, const char *, const char *);
-extern int git_config_set_in_file_gently(const char *, const char *, const 
char *);
-extern void git_config_set_in_file(const char *, const char *, const char *);
-extern int git_config_set_gently(const char *, const char *);
-extern void git_config_set(const char *, const char *);
-extern int git_config_parse_key(const char *, char **, int *);
-extern int git_config_key_is_valid(const char *key);
-extern int git_config_set_multivar_gently(const char *, const char *, const 
char *, int);
-extern void git_config_set_multivar(const char *, const char *, const char *, 
int);
-extern int git_config_set_multivar_in_file_gently(const char *, const char *, 
const char *, const char *, int);
-extern void git_config_set_multivar_in_file(const char *, const char *, const 
char *, const char *, int);
-extern int git_config_rename_section(const char *, const char *);
-extern int git_config_rename_section_in_file(const char *, const char *, const 
char *);
-extern const char *git_etc_gitconfig(void);
-extern int git_env_bool(const char *, int);
-extern unsigned long git_env_ulong(const char *, unsigned long);
-extern int git_config_system(void);
-extern int config_error_nonbool(const char *);
-#if defined(__GNUC__)
-#define config_error_nonbool(s) (config_error_nonbool(s), const_error())
-#endif
 extern const char *get_log_output_encoding(void);
 extern const char *get_commit_output_encoding(void);
 
-extern int git_config_parse_parameter(const char *, config_fn_t fn, void 
*data);
-
-enum config_scope {
-   CONFIG_SCOPE_UNKNOWN = 0,
-   

Re: [PATCH v4 3/5] stash: add test for stashing in a detached state

[Junio C Hamano]

Joel Teichroeb  writes:

>>> +test_expect_success 'create in a detached state' '
>>> + test_when_finished "git checkout master" &&
>>> + git checkout HEAD~1 &&
>>> + >foo &&
>>> + git add foo &&
>>> + STASH_ID=$(git stash create) &&
>>> + HEAD_ID=$(git rev-parse --short HEAD) &&
>>> + echo "WIP on (no branch): ${HEAD_ID} initial" >expect &&
>>> + git show --pretty=%s -s ${STASH_ID} >actual &&
>>> + test_cmp expect actual
>>> +'
>>
>> Hmph.  Is the title automatically given to the stash the
>> only/primary thing that is of interest to us in this test?  I think
>> we care more about that we record the right thing in the resulting
>> stash and also after creating the stash the working tree and the
>> index becomes clean.  Shouldn't we be testing that?
>
> In this case, the title is really what I wanted to test. There are
> other tests already to make sure that stash create works, but there
> were no tests to ensure that a stash was created with the correct
> title when not on a branch.

Ah, OK.

Thanks.

Re: [BUG] add_again() off-by-one error in custom format

[René Scharfe]

Am 13.06.2017 um 20:29 schrieb Junio C Hamano:

René Scharfe  writes:


Indeed, a very nice bug report!


I think the call to format_commit_one() needs to be taught to pass
'magic' through, and then add_again() mechanism needs to be told not
to cache when magic is in effect, or something like that.

Perhaps something along this line...?

   pretty.c | 64 
++--
   1 file changed, 38 insertions(+), 26 deletions(-)


That looks quite big.  You even invent a way to classify magic.


Hmph, by "a way to classify", do you mean the enum?  That thing was
there from before, just moved.


Oh, yes, sorry.  I didn't even get that far into the patch.  (I'll
better go to bed after hitting send..)


Also I think we do not have to change add_again() at all, because
chunk->off tolerates being given a garbage value as long as
chunk->len stays 0, and add_again() does not change chunk->len at
all.

Which cuts the diffstat down to

  pretty.c | 40 +---
  1 file changed, 25 insertions(+), 15 deletions(-)


Does the caching feature justify the added complexity?


That's a good question.  I thought about your second alternative
while adding the "don't cache" thing, but if we can measure and find
out that we are not gaining much from caching, certainly that sounds
much simpler.


The difference is about the same as the one between:

$ time git log --format="" >/dev/null

real0m0.463s
user0m0.448s
sys 0m0.012s

and:

$ time git log --format="%h" >/dev/null

real0m1.062s
user0m0.636s
sys 0m0.416s

With caching duplicates are basically free and without it short
hashes have to be looked up again.  Other placeholders may reduce
the relative slowdown, depending on how expensive they are.

Forgot a third option, probably because it's not a particularly good
idea: Replacing the caching in pretty.c with a short static cache in
find_unique_abbrev_r().

René

Re: [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch

[Joel Teichroeb]

On Tue, Jun 13, 2017 at 12:40 PM, Junio C Hamano  wrote:
> Joel Teichroeb  writes:
>
>> If the return value of merge recurisve is not checked, the stash could end
>> up being dropped even though it was not applied properly
>
> s/recurisve/recursive/
>
>> Signed-off-by: Joel Teichroeb 
>> ---
>>  t/t3903-stash.sh | 14 ++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
>> index cc923e6335..5399fb05ca 100755
>> --- a/t/t3903-stash.sh
>> +++ b/t/t3903-stash.sh
>> @@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the 
>> stash if the branch exists
>>   git rev-parse stash@{0} --
>>  '
>>
>> +test_expect_success 'stash branch should not drop the stash if the apply 
>> fails' '
>> + git stash clear &&
>> + git reset HEAD~1 --hard &&
>> + echo foo >file &&
>> + git add file &&
>> + git commit -m initial &&
>
> It's not quite intuitive to call a non-root commit "initial" ;-)
>
>> + echo bar >file &&
>> + git stash &&
>> + echo baz >file &&
>
> OK, so 'file' has 'foo' in HEAD, 'bar' in the stash@{0}.
>
>> + test_when_finished "git checkout master" &&
>> + test_must_fail git stash branch new_branch stash@{0} &&
>
> Hmph.  Do we blindly checkout new_branch out of stash@{0}^1 and
> unstash, but because 'file' in the working tree is dirty, we fail to
> apply the stash and stop?
>
> This sounds like a bug to me.  Shouldn't we be staying on 'master',
> and fail without even creating 'new_branch', when this happens?

Good point. The existing behavior is to create new_branch and check it
out. I'm not sure what the correct state should be then. Create
new_branch, checkout new_branch, fail to apply, checkout master?
Should it then delete new_branch? Is there a way instead to test
applying the stash before creating the branch without actually
applying it? Something like putting merge_recursive into some kind of
dry-run mode?

>
> In any case we should be testing what branch we are on after this
> step.  What branch should we be on after "git stash branch" fails?
>
>> + git rev-parse stash@{0} --
>> +'
>> +
>>  test_expect_success 'stash apply shows status same as git status (relative 
>> to current directory)' '
>>   git stash clear &&
>>   echo 1 >subdir/subfile1 &&

Re: [PATCH v4 3/5] stash: add test for stashing in a detached state

[Joel Teichroeb]

On Tue, Jun 13, 2017 at 12:45 PM, Junio C Hamano  wrote:
> Joel Teichroeb  writes:
>
>> Signed-off-by: Joel Teichroeb 
>> ---
>>  t/t3903-stash.sh | 12 
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
>> index 5399fb05ca..ce4c8fe3d6 100755
>> --- a/t/t3903-stash.sh
>> +++ b/t/t3903-stash.sh
>> @@ -822,6 +822,18 @@ test_expect_success 'create with multiple arguments for 
>> the message' '
>>   test_cmp expect actual
>>  '
>>
>> +test_expect_success 'create in a detached state' '
>> + test_when_finished "git checkout master" &&
>> + git checkout HEAD~1 &&
>> + >foo &&
>> + git add foo &&
>> + STASH_ID=$(git stash create) &&
>> + HEAD_ID=$(git rev-parse --short HEAD) &&
>> + echo "WIP on (no branch): ${HEAD_ID} initial" >expect &&
>> + git show --pretty=%s -s ${STASH_ID} >actual &&
>> + test_cmp expect actual
>> +'
>
> Hmph.  Is the title automatically given to the stash the
> only/primary thing that is of interest to us in this test?  I think
> we care more about that we record the right thing in the resulting
> stash and also after creating the stash the working tree and the
> index becomes clean.  Shouldn't we be testing that?

In this case, the title is really what I wanted to test. There are
other tests already to make sure that stash create works, but there
were no tests to ensure that a stash was created with the correct
title when not on a branch. That being said though, I'll add more
validation as more validation is always better.

>
> If "git stash create" fails to make the working tree and the index
> clean, then "git checkout master" run by when-finished will carry
> the local modifications with us, which probably is not what you
> meant.  You'd need "reset --hard" there, too, perhaps?

Agreed.

>
>>  test_expect_success 'stash --  stashes and restores the file' '
>>   >foo &&
>>   >bar &&

Re: [PATCH v4 4/5] merge: close the index lock when not writing the new index

[Junio C Hamano]

Joel Teichroeb  writes:

> If the merge does not have anything to do, it does not unlock the index,
> causing any further index operations to fail. Thus, always unlock the index
> regardless of outcome.
>
> Signed-off-by: Joel Teichroeb 
> ---

This one makes sense.  

So far, nobody who calls this function performs further index
manipulations and letting the atexit handlers automatically release
the lock was sufficient.  This allows new callers to do more work on
the index after a merge finishes.


>  merge-recursive.c | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index ae5238d82c..16bb5512ef 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2145,9 +2145,12 @@ int merge_recursive_generic(struct merge_options *o,
>   if (clean < 0)
>   return clean;
>  
> - if (active_cache_changed &&
> - write_locked_index(_index, lock, COMMIT_LOCK))
> - return err(o, _("Unable to write index."));
> + if (active_cache_changed) {
> + if (write_locked_index(_index, lock, COMMIT_LOCK))
> + return err(o, _("Unable to write index."));
> + } else {
> + rollback_lock_file(lock);
> + }
>  
>   return clean ? 0 : 1;
>  }

Re: [PATCH v4 3/5] stash: add test for stashing in a detached state

[Junio C Hamano]

Joel Teichroeb  writes:

> Signed-off-by: Joel Teichroeb 
> ---
>  t/t3903-stash.sh | 12 
>  1 file changed, 12 insertions(+)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 5399fb05ca..ce4c8fe3d6 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -822,6 +822,18 @@ test_expect_success 'create with multiple arguments for 
> the message' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success 'create in a detached state' '
> + test_when_finished "git checkout master" &&
> + git checkout HEAD~1 &&
> + >foo &&
> + git add foo &&
> + STASH_ID=$(git stash create) &&
> + HEAD_ID=$(git rev-parse --short HEAD) &&
> + echo "WIP on (no branch): ${HEAD_ID} initial" >expect &&
> + git show --pretty=%s -s ${STASH_ID} >actual &&
> + test_cmp expect actual
> +'

Hmph.  Is the title automatically given to the stash the
only/primary thing that is of interest to us in this test?  I think
we care more about that we record the right thing in the resulting
stash and also after creating the stash the working tree and the
index becomes clean.  Shouldn't we be testing that?

If "git stash create" fails to make the working tree and the index
clean, then "git checkout master" run by when-finished will carry
the local modifications with us, which probably is not what you
meant.  You'd need "reset --hard" there, too, perhaps?

>  test_expect_success 'stash --  stashes and restores the file' '
>   >foo &&
>   >bar &&

Re: [PATCH v4 2/5] stash: Add a test for when apply fails during stash branch

[Junio C Hamano]

Joel Teichroeb  writes:

> If the return value of merge recurisve is not checked, the stash could end
> up being dropped even though it was not applied properly

s/recurisve/recursive/

> Signed-off-by: Joel Teichroeb 
> ---
>  t/t3903-stash.sh | 14 ++
>  1 file changed, 14 insertions(+)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index cc923e6335..5399fb05ca 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -656,6 +656,20 @@ test_expect_success 'stash branch should not drop the 
> stash if the branch exists
>   git rev-parse stash@{0} --
>  '
>  
> +test_expect_success 'stash branch should not drop the stash if the apply 
> fails' '
> + git stash clear &&
> + git reset HEAD~1 --hard &&
> + echo foo >file &&
> + git add file &&
> + git commit -m initial &&

It's not quite intuitive to call a non-root commit "initial" ;-)

> + echo bar >file &&
> + git stash &&
> + echo baz >file &&

OK, so 'file' has 'foo' in HEAD, 'bar' in the stash@{0}.

> + test_when_finished "git checkout master" &&
> + test_must_fail git stash branch new_branch stash@{0} &&

Hmph.  Do we blindly checkout new_branch out of stash@{0}^1 and
unstash, but because 'file' in the working tree is dirty, we fail to
apply the stash and stop?

This sounds like a bug to me.  Shouldn't we be staying on 'master',
and fail without even creating 'new_branch', when this happens?

In any case we should be testing what branch we are on after this
step.  What branch should we be on after "git stash branch" fails?

> + git rev-parse stash@{0} --
> +'
> +
>  test_expect_success 'stash apply shows status same as git status (relative 
> to current directory)' '
>   git stash clear &&
>   echo 1 >subdir/subfile1 &&

Re: [PATCH v4 1/5] stash: add test for stash create with no files

[Junio C Hamano]

Joel Teichroeb  writes:

> Ensure the command gives the correct return code

OK.  When you know what the correct return code is, we'd prefer to
see it spelled out, i.e.

Ensure that the command succeeds.

Or did you mean that the command outputs nothing?

The test itself looks obviously correct ;-)

> Signed-off-by: Joel Teichroeb 
> ---
>  t/t3903-stash.sh | 8 
>  1 file changed, 8 insertions(+)
>
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 3b4bed5c9a..cc923e6335 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -444,6 +444,14 @@ test_expect_failure 'stash file to directory' '
>   test foo = "$(cat file/file)"
>  '
>  
> +test_expect_success 'stash create - no changes' '
> + git stash clear &&
> + test_when_finished "git reset --hard HEAD" &&
> + git reset --hard &&
> + git stash create >actual &&
> + test_must_be_empty actual
> +'
> +
>  test_expect_success 'stash branch - no stashes on stack, stash-like 
> argument' '
>   git stash clear &&
>   test_when_finished "git reset --hard HEAD" &&

Re: [RFC/PATCH] submodules: overhaul documentation

[Junio C Hamano]

Stefan Beller  writes:

> @@ -149,15 +119,17 @@ deinit [-f|--force] (--all|[--] ...)::
>   tree. Further calls to `git submodule update`, `git submodule foreach`
>   and `git submodule sync` will skip any unregistered submodules until
>   they are initialized again, so use this command if you don't want to
> - have a local checkout of the submodule in your working tree anymore. If
> - you really want to remove a submodule from the repository and commit
> - that use linkgit:git-rm[1] instead.
> + have a local checkout of the submodule in your working tree anymore.
>  +
>  When the command is run without pathspec, it errors out,
>  instead of deinit-ing everything, to prevent mistakes.
>  +
>  If `--force` is specified, the submodule's working tree will
>  be removed even if it contains local modifications.
> ++
> +If you really want to remove a submodule from the repository and commit
> +that use linkgit:git-rm[1] instead. See linkgit:gitsubmodules[7] for removal
> +options.

Good reorganization.

> diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt
> new file mode 100644
> index 00..2bf3149b68
> --- /dev/null
> +++ b/Documentation/gitsubmodules.txt
> @@ -0,0 +1,214 @@
> +gitsubmodules(7)
> +
> +
> +NAME
> +
> +gitsubmodules - mounting one repository inside another
> +
> +SYNOPSIS
> +
> +.gitmodules, $GIT_DIR/config
> +--
> +git submodule
> +git  --recurse-submodules
> +--
> +
> +DESCRIPTION
> +---
> +
> +A submodule is another Git repository tracked in a subdirectory of your
> +repository. The tracked repository has its own history, which does not
> +interfere with the history of the current repository.

"tracked in a subdirectory" sounds as if your top-level superproject
has a dedicated submodules/ directory and in it there live a bunch
of submodules.  Which obviously is not what you meant.  If phrased
"tracked as a subdirectory", I think the sentence makes sense.

While "which does not interfere" may be technically correct, I am
not sure what the value of saying that is.

> +Submodules are composed from a so-called `gitlink` tree entry
> +in the main repository that refers to a particular commit object
> +within the inner repository.

Correct, but it may be unclear to the readers why we do so.  Perhaps

... and this way, the tree of each commit in the main repository
"knows" which commit from the submodule's history is "tied" to it.

or something like that?

> +Additionally to the gitlink entry the `.gitmodules` file (see
> +linkgit:gitmodules[5]) at the root of the source tree contains
> +information needed for submodules.

Is that really true?  Each submodule do not *need* what is in
.gitmodules; the top-level superproject needs to learn about
its submodules from the contents of that file, though.

> +The only required information
> +is the path setting, which estabishes a logical name for the submodule.

The phrase "the path setting" feels a bit unfortunate.  Is that
"only" thing we need?  Without URL we have no way to populate it,
no?

> +The usual git configuration (see linkgit:git-config[1]) can be used to
> +override settings given by the `.gitmodules` file.
> +
> +Submodules can be used for two different use cases:
> +
> +1. Using another project that stands on its own.
> +  When you want to use a third party library, submodules allow you to
> +  have a clean history for your own project as well as for the library.
> +  This also allows for updating the third party library as needed.
> +
> +2. Artificially split a (logically single) project into multiple
> +   repositories and tying them back together. This can be used to
> +   overcome deficiences in the data model of Git, such as:

s/deficiences in the data model/current limitations/ perhaps?

> +* To have finer grained access control.
> +  The design principles of Git do not allow for partial repositories to be
> +  checked out or transferred. A repository is the smallest unit that a user
> +  can be given access to. Submodules are separate repositories, such that
> +  you can restrict access to parts of your project via the use of submodules.

Some servers implement per-branch access control that seems to work
rather well.  Given that "shallow history" is possible (i.e. you
could give one commit without exposing older parts of the history),
I think the limitation this paragrah refers to is that "a tree is
the smallest unit that the user can be given access to."

> +* In its current form Git scales up poorly for very large repositories that
> +  change a lot, as the history grows very large. For that you may want to 
> look
> +  at shallow clone, sparse checkout, or git-LFS.
> +  However you can also use submodules to e.g. hold large binary assets
> +  and these repositories are then shallowly cloned such that you do not
> +  have a large history locally.

This is why I suggest 

Re: [BUG] add_again() off-by-one error in custom format

[Junio C Hamano]

René Scharfe  writes:

> Indeed, a very nice bug report!
>
>> I think the call to format_commit_one() needs to be taught to pass
>> 'magic' through, and then add_again() mechanism needs to be told not
>> to cache when magic is in effect, or something like that.
>>
>> Perhaps something along this line...?
>>
>>   pretty.c | 64 
>> ++--
>>   1 file changed, 38 insertions(+), 26 deletions(-)
>
> That looks quite big.  You even invent a way to classify magic. 

Hmph, by "a way to classify", do you mean the enum?  That thing was
there from before, just moved.

Also I think we do not have to change add_again() at all, because
chunk->off tolerates being given a garbage value as long as
chunk->len stays 0, and add_again() does not change chunk->len at
all.

Which cuts the diffstat down to

 pretty.c | 40 +---
 1 file changed, 25 insertions(+), 15 deletions(-)

> Does the caching feature justify the added complexity?

That's a good question.  I thought about your second alternative
while adding the "don't cache" thing, but if we can measure and find
out that we are not gaining much from caching, certainly that sounds
much simpler.

>
> - Don't cache anymore, now that we have placeholders that change output
>   on top of the original append-only ones.  Yields a net code reduction.
>   Duplicated %h, %t and %p will have to be resolved at each occurrence.
>
> - Provide some space for the cache instead of reusing the output, like
>   a strbuf for each placeholder.  Adds a memory allocation to each
>   first occurrence of %h, %t and %p.  Such a cache could be reused for
>   multiple commits by truncating it instead of releasing; not sure how
>   to pass it in nicely for it to survive a sequence of calls, though.
>
> René

Re: [PATCH v3 6/6] Use the early config machinery to expand aliases

[Brandon Williams]

On 06/13, Johannes Schindelin wrote:
> Instead of discovering the .git/ directory, read the config and then
> trying to painstakingly reset all the global state if we did not find a
> matching alias, let's use the early config machinery instead.
> 
> It may look like unnecessary work to discover the .git/ directory in the
> early config machinery and then call setup_git_directory_gently() in the
> case of a shell alias, repeating the very same discovery *again*.
> However, we have to do this as the early config machinery takes pains
> *not* to touch any global state, while shell aliases expect a possibly
> changed working directory and at least the GIT_PREFIX and GIT_DIR
> variables to be set.
> 
> Also, one might be tempted to streamline the code in alias_lookup() to
> *not* use a strbuf for the key. However, if the config reports an error,
> it is far superior to tell the user that the `alias.xyz` key had a
> problem than to claim that it was the `xyz` key.
> 
> This change also fixes a known issue where Git tried to read the pager
> config from an incorrect path in a subdirectory of a Git worktree if an
> alias expanded to a shell command.
> 
> Signed-off-by: Johannes Schindelin 

So because I've been looking at the config machinery lately, I've
noticed a lot of issues with how things are handled with respect to
gitdir vs commondir.  Essentially the config resides at commondir/config
always, and only at gitdir/config when not working with a worktree.
Because of this, your patches point out a bug in how early config is
handled.  I'll illustrate this using aliases.

Before this series (because aliases are read using the standard config
machinery):

  > git init main
  > git -C main config alias.test '!echo hello'
  > git -C main test
hello
  > git -C main worktree add ../worktree
  > git -C worktree test
hello

After this series (using read_early_config()):

  > git init main
  > git -C main config alias.test '!echo hello'
  > git -C main test
hello
  > git -C main worktree add ../worktree
  > git -C worktree test
git: 'test' is not a git command. See 'git --help'.

The issue is that read_early_config passes the gitdir and not the
commondir when reading the config.

The solution would be to add a 'commondir' field to the config_options
struct and populate that before reading the config.  I'm planning on
fixing this in v2 of my config cleanup series which I'll hopefully have
finished by the end of the day.

> ---
>  alias.c  | 31 ---
>  git.c| 55 ---
>  t/t7006-pager.sh |  2 +-
>  3 files changed, 29 insertions(+), 59 deletions(-)
> 
> diff --git a/alias.c b/alias.c
> index 3b90397a99d..6bdc9363037 100644
> --- a/alias.c
> +++ b/alias.c
> @@ -1,14 +1,31 @@
>  #include "cache.h"
>  
> +struct config_alias_data
> +{
> + struct strbuf key;
> + char *v;
> +};
> +
> +static int config_alias_cb(const char *key, const char *value, void *d)
> +{
> + struct config_alias_data *data = d;
> +
> + if (!strcmp(key, data->key.buf))
> + return git_config_string((const char **)>v, key, value);
> +
> + return 0;
> +}
> +
>  char *alias_lookup(const char *alias)
>  {
> - char *v = NULL;
> - struct strbuf key = STRBUF_INIT;
> - strbuf_addf(, "alias.%s", alias);
> - if (git_config_key_is_valid(key.buf))
> - git_config_get_string(key.buf, );
> - strbuf_release();
> - return v;
> + struct config_alias_data data = { STRBUF_INIT, NULL };
> +
> + strbuf_addf(, "alias.%s", alias);
> + if (git_config_key_is_valid(data.key.buf))
> + read_early_config(config_alias_cb, );
> + strbuf_release();
> +
> + return data.v;
>  }
>  
>  #define SPLIT_CMDLINE_BAD_ENDING 1
> diff --git a/git.c b/git.c
> index 8ff44f081d4..58ef570294d 100644
> --- a/git.c
> +++ b/git.c
> @@ -16,50 +16,6 @@ const char git_more_info_string[] =
>  "to read about a specific subcommand or concept.");
>  
>  static int use_pager = -1;
> -static char *orig_cwd;
> -static const char *env_names[] = {
> - GIT_DIR_ENVIRONMENT,
> - GIT_WORK_TREE_ENVIRONMENT,
> - GIT_IMPLICIT_WORK_TREE_ENVIRONMENT,
> - GIT_PREFIX_ENVIRONMENT
> -};
> -static char *orig_env[4];
> -static int save_restore_env_balance;
> -
> -static void save_env_before_alias(void)
> -{
> - int i;
> -
> - assert(save_restore_env_balance == 0);
> - save_restore_env_balance = 1;
> - orig_cwd = xgetcwd();
> - for (i = 0; i < ARRAY_SIZE(env_names); i++) {
> - orig_env[i] = getenv(env_names[i]);
> - orig_env[i] = xstrdup_or_null(orig_env[i]);
> - }
> -}
> -
> -static void restore_env(int external_alias)
> -{
> - int i;
> -
> - assert(save_restore_env_balance == 1);
> - save_restore_env_balance = 0;
> - if (!external_alias && orig_cwd && chdir(orig_cwd))
> - die_errno("could not 

Re: [BUG] add_again() off-by-one error in custom format

[René Scharfe]

Am 13.06.2017 um 00:49 schrieb Junio C Hamano:

Michael Giuffrida  writes:


For the abbreviated commit hash placeholder ('h'), pretty.c uses
add_again() to cache the result of the expansion, and then uses that
result in future expansions. This causes problems when the expansion
includes whitespace:

 $ git log -n 1 --pretty='format:newline:%+h%nno_newline:%h'
 newline:
 a0b1c2d
 no_newline:
 a0b1c2

The second expansion of the hash added an unwanted newline and removed
the final character. It seemingly used the cached expansion *starting
from the inserted newline*.

The expected output is:

 $ git log -n 1 --pretty='format:newline:%+h%nno_newline:%h'
 newline:
 a0b1c2d
 no_newline:a0b1c2d


Nicely explained.  The add_again() mechanism caches an earlier
result by remembering the offset and the length in the strbuf the
formatted string is being accumulated to, but because %+x (and
probably %-x) magic placeholders futz with the result of
format_commit_one() in place, the cache goes out of sync, of course.


Indeed, a very nice bug report!


I think the call to format_commit_one() needs to be taught to pass
'magic' through, and then add_again() mechanism needs to be told not
to cache when magic is in effect, or something like that.

Perhaps something along this line...?

  pretty.c | 64 ++--
  1 file changed, 38 insertions(+), 26 deletions(-)


That looks quite big.  You even invent a way to classify magic. Does the
caching feature justify the added complexity?  Alternatives:

- Don't cache anymore, now that we have placeholders that change output
  on top of the original append-only ones.  Yields a net code reduction.
  Duplicated %h, %t and %p will have to be resolved at each occurrence.

- Provide some space for the cache instead of reusing the output, like
  a strbuf for each placeholder.  Adds a memory allocation to each
  first occurrence of %h, %t and %p.  Such a cache could be reused for
  multiple commits by truncating it instead of releasing; not sure how
  to pass it in nicely for it to survive a sequence of calls, though.

René

Re: [PATCH/RFC] branch: add tests for new copy branch feature

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:
> On Tue, Jun 13 2017, Jonathan Nieder jotted:
> > Ævar Arnfjörð Bjarmason wrote:

>>> My understanding of that last part is that Jonathan/someone (see
>>> reported-by in that patch) had some script which was renaming
>>> branches, and it was easier for whatever reason to just make it no-op
>>> if the rename would have yielded the same result as doing nothing at
>>> all.
>>>
>>> Most likely your implementation will consist of just re-using the
>>> logic in rename_branch() (and renaming it to e.g.
>>> copy_or_rename_branch() ...) so you could just re-use the no-op
>>> behavior we use for -m, or if there's some reason not to no-op and
>>> error instead for -c we could just do that, but in any case this case
>>> of `git branch -c master master` or `git branch -c currentbranch`
>>> should be tested for.
>>
>> I may be missing some context, but notice that the above mentioned
>> commit is about -M, not -m.
>
> The context was just that that commit added a change in how -M
> interacted when clobbering the current HEAD, and that -C should have a
> test for that behavior, which the patch now submitted to the list has:
>
> +test_expect_success 'git branch -C master master should work when master 
> is checked out' '
> +   git checkout master &&
> +   git branch -C master master
> +'

Perfect, thanks.  Carry on. :)

Jonathan

Re: [RFC/PATCH] builtin/blame: darken redundant line information

[Junio C Hamano]

Stefan Beller  writes:

> On Tue, Jun 13, 2017 at 10:33 AM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> But you do not want to (yet)? The goal is not to tell you where the bounds
>>> are, but the goal is to point out that extra care is required for review of
>>> these particular 3 lines.
>>
>> And when you _can_ help users in that "extra care" by pointing out
>> where the boundary is, what is the justification for hiding that
>> information?
> ...
>
> And how many colors would be confusing for the user?

Well, having two "dimmed" for moved lines are not needed if we use
the same as context, so we can reduce the colors to just two
highlights times two (i.e. old and new).

Having said that,

> I would think we want to start with a simple model ...

would be OK.  Let me configure the old-moved and new-moved to the
same as context and see if I can live with just a single highlight
color.

Thanks.

Re: [PATCH/RFC] branch: add tests for new copy branch feature

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 13 2017, Jonathan Nieder jotted:

> Hi,
>
> Ævar Arnfjörð Bjarmason wrote:
>
>> So the reason we have this for -m is:
>>
>> commit 3f59481e33
>> Author: Jonathan Nieder 
>> Date:   Fri Nov 25 20:30:02 2011 -0600
>>
>> branch: allow a no-op "branch -M  HEAD"
>>
>> Overwriting the current branch with a different commit is forbidden, as 
>> it
>> will make the status recorded in the index and the working tree out of
>> sync with respect to the HEAD. There however is no reason to forbid it if
>> the current branch is renamed to itself, which admittedly is something
>> only an insane user would do, but is handy for scripts.
>>
>> My understanding of that last part is that Jonathan/someone (see
>> reported-by in that patch) had some script which was renaming
>> branches, and it was easier for whatever reason to just make it no-op
>> if the rename would have yielded the same result as doing nothing at
>> all.
>>
>> Most likely your implementation will consist of just re-using the
>> logic in rename_branch() (and renaming it to e.g.
>> copy_or_rename_branch() ...) so you could just re-use the no-op
>> behavior we use for -m, or if there's some reason not to no-op and
>> error instead for -c we could just do that, but in any case this case
>> of `git branch -c master master` or `git branch -c currentbranch`
>> should be tested for.
>
> I may be missing some context, but notice that the above mentioned
> commit is about -M, not -m.

The context was just that that commit added a change in how -M
interacted when clobbering the current HEAD, and that -C should have a
test for that behavior, which the patch now submitted to the list has:

+test_expect_success 'git branch -C master master should work when master 
is checked out' '
+   git checkout master &&
+   git branch -C master master
+'

Re: [RFC/PATCH] builtin/blame: darken redundant line information

[Stefan Beller]

On Tue, Jun 13, 2017 at 10:48 AM, Junio C Hamano  wrote:
>
> I never said "start and end" (you did).  I just wanted the boundary
> of A and B and C clear, so I'd be perfectly happy with:
>
>  context
> +A  dim
> +A  dim
> +A  highlight #1
> +C  highlight #2
> +B  highlight #1
> +B  dim
> +B  dim
>  context
>
> You can do that still with only two highlight colors, no?

So to put it into an algorithm:

1) detect blocks
2) if blocks are adjacent, their bounds are eligible for highlighting
3) the highlighting is implemented using the "alternate" strategy
  in that any line highlighted belonging to a different block flips
  the highlighting, such that:

  context
 +A  dim
 +A  dim
 +A  highlight #1
 +B  highlight #2
 +B  dim
 +B  dim
  context

So if we go this way, we would need indeed 6 colors:

  Dimmed, Highlighted, HighlightedAlternative

color-moved modes:
nobounds::
  uses dimmed only
allbounds::
adjacentbounds::
  See algorithm above, using dimmed for inside the block and
  both highlights for bounds, making sure adjacent block bounds
  alternate the highlighting color.
alternate::
  Uses only highlighting colors, complete block is colored with
  one of the highlights

I think that is reasonable to implement. But I do still wonder if
we really want to add so many new colors.
I'll give it a try after my next submodule series.

Thanks,
Stefan

Re: [PATCH/RFC] branch: add tests for new copy branch feature

[Jonathan Nieder]

Hi,

Ævar Arnfjörð Bjarmason wrote:

> So the reason we have this for -m is:
>
> commit 3f59481e33
> Author: Jonathan Nieder 
> Date:   Fri Nov 25 20:30:02 2011 -0600
>
> branch: allow a no-op "branch -M  HEAD"
>
> Overwriting the current branch with a different commit is forbidden, as it
> will make the status recorded in the index and the working tree out of
> sync with respect to the HEAD. There however is no reason to forbid it if
> the current branch is renamed to itself, which admittedly is something
> only an insane user would do, but is handy for scripts.
>
> My understanding of that last part is that Jonathan/someone (see
> reported-by in that patch) had some script which was renaming
> branches, and it was easier for whatever reason to just make it no-op
> if the rename would have yielded the same result as doing nothing at
> all.
>
> Most likely your implementation will consist of just re-using the
> logic in rename_branch() (and renaming it to e.g.
> copy_or_rename_branch() ...) so you could just re-use the no-op
> behavior we use for -m, or if there's some reason not to no-op and
> error instead for -c we could just do that, but in any case this case
> of `git branch -c master master` or `git branch -c currentbranch`
> should be tested for.

I may be missing some context, but notice that the above mentioned
commit is about -M, not -m.

Thanks and hope that helps,
Jonathan

Re: [PATCH 2/3] branch: add test for -m renaming multiple config sections

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 13 2017, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason  writes:
>
>> On Tue, Jun 13, 2017 at 7:10 PM, Junio C Hamano  wrote:
>>> Indenting using <<- would make it easier to read.  I.e.
>>>
>>> cat >config.branch <<-\EOF &&
>>> ;; Comment for ...
>>> [branch "source"]
>>> ;; Comment for ...
>>> ...
>>> EOF
>>
>> I should have added a comment for that, I can't find any portable (but
>> suggestions welcome) way to do that and preserve the indentation, so
>> the test_cmp would still succeed if the moving/renaming function
>> munged all leading whitespace in the config with -\EOF as opposed to
>> \EOF.
>
> Ah, I see why it is done that way.  You could indent the lines in
> the configuration file with SPs (<<- strips only HTs, no?)

Sure, we'll do that for v2.

>> It's just a sufficiently large number, I thought -A was portable
>> enough after grepping the test suite, but on closer inspection it
>> turns out those were all git-grep invocations, oops. Yeah all I need
>> here is all lines after a line matching a given string, so that sed
>> command works, will fix it up to use that.
>
> Oops, indeed ;-)  Thanks.

Re: [RFC/PATCH] builtin/blame: darken redundant line information

[Junio C Hamano]

Stefan Beller  writes:

> On Tue, Jun 13, 2017 at 10:33 AM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> But you do not want to (yet)? The goal is not to tell you where the bounds
>>> are, but the goal is to point out that extra care is required for review of
>>> these particular 3 lines.
>>
>> And when you _can_ help users in that "extra care" by pointing out
>> where the boundary is, what is the justification for hiding that
>> information?
>
> It is very complicated and confusing. Consider this:
>
>>  context
>> -C
>>  context
>> -B
>> -B
>> -B
>> -A
>> -A
>> -A
>>  context
>> +A
>> +A
>> +A
>> +C
>> +B
>> +B
>> +B
>>  context
>
> So from your emails I understood you want to markup
> block starts and ends, but in this case C is *both* start
> and end of a block, and has also different blocks around.

I never said "start and end" (you did).  I just wanted the boundary
of A and B and C clear, so I'd be perfectly happy with:

 context
+A  dim
+A  dim
+A  highlight #1
+C  highlight #2
+B  highlight #1
+B  dim
+B  dim
 context

You can do that still with only two highlight colors, no?

Re: [RFC/PATCH] builtin/blame: darken redundant line information

[Stefan Beller]

On Tue, Jun 13, 2017 at 10:33 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> But you do not want to (yet)? The goal is not to tell you where the bounds
>> are, but the goal is to point out that extra care is required for review of
>> these particular 3 lines.
>
> And when you _can_ help users in that "extra care" by pointing out
> where the boundary is, what is the justification for hiding that
> information?

It is very complicated and confusing. Consider this:

>  context
> -C
>  context
> -B
> -B
> -B
> -A
> -A
> -A
>  context
> +A
> +A
> +A
> +C
> +B
> +B
> +B
>  context

So from your emails I understood you want to markup
block starts and ends, but in this case C is *both* start
and end of a block, and has also different blocks around.

So ideally we could tell the user this

>  context
> _context C goes to after +A
> -C
> _context C goes to before +B
>  context
> _context -B goes to after +C
> -B
> -B
> -B
> _context -B goes to before contextB
> _context -A goes to after contextA
> -A
> -A
> -A
> _context -A goes to after contextA
>  context
> _context +A comes from after -B
> +A
> +A
> +A
> _context +A comes from before contextA
> _context +C comes from after contextC
> +C
> _context +C comes from before contextC
> _context +B comes from after contextB
> +B
> +B
> +B
> _context +B comes from after -A
>  context

(show the context of where the move is coming from/going to,
maybe just one or two lines)

And how many colors would be confusing for the user?

I would think we want to start with a simple model and if a
niche is not good (read: people think it can be improved easily
compared to the usefulness they get out of it) enough we fix
it up later.

I thought that adding 4 new colors is already maybe too much,
as Git users were happy with a handful for 10 years, so I am very
opposed to add more than 4 colors unless there is a very good
reason. And I'd think that this is not adding a lot of useful information
for a reviewer?

Thanks,
Stefan

Re: [PATCH 2/3] branch: add test for -m renaming multiple config sections

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Tue, Jun 13, 2017 at 7:10 PM, Junio C Hamano  wrote:
>> Indenting using <<- would make it easier to read.  I.e.
>>
>> cat >config.branch <<-\EOF &&
>> ;; Comment for ...
>> [branch "source"]
>> ;; Comment for ...
>> ...
>> EOF
>
> I should have added a comment for that, I can't find any portable (but
> suggestions welcome) way to do that and preserve the indentation, so
> the test_cmp would still succeed if the moving/renaming function
> munged all leading whitespace in the config with -\EOF as opposed to
> \EOF.

Ah, I see why it is done that way.  You could indent the lines in
the configuration file with SPs (<<- strips only HTs, no?)

> It's just a sufficiently large number, I thought -A was portable
> enough after grepping the test suite, but on closer inspection it
> turns out those were all git-grep invocations, oops. Yeah all I need
> here is all lines after a line matching a given string, so that sed
> command works, will fix it up to use that.

Oops, indeed ;-)  Thanks.

Re: [RFC/PATCH] builtin/blame: darken redundant line information

[Junio C Hamano]

Stefan Beller  writes:

> But you do not want to (yet)? The goal is not to tell you where the bounds
> are, but the goal is to point out that extra care is required for review of
> these particular 3 lines.

And when you _can_ help users in that "extra care" by pointing out
where the boundary is, what is the justification for hiding that
information?

Re: [PATCH 2/3] branch: add test for -m renaming multiple config sections

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 13, 2017 at 7:10 PM, Junio C Hamano  wrote:
> Sahil Dua  writes:
>
>> + cat >expect <<-\EOF &&
>> + branch.dest.key1=value1
>> + some.gar.b=age
>> + branch.dest.key2=value2
>> + EOF
>> + cat >config.branch <<\EOF &&
>> +;; Comment for source
>> +[branch "source"]
>> + ;; Comment for the source value
>> + key1 = value1
>> + ;; Comment for some.gar
>> +[some "gar"]
>> + ;; Comment for the some.gar value
>> + b = age
>> + ;; Comment for source, again
>> +[branch "source"]
>> + ;; Comment for the source value, again
>> + key2 = value2
>> +EOF
>
> Indenting using <<- would make it easier to read.  I.e.
>
> cat >config.branch <<-\EOF &&
> ;; Comment for ...
> [branch "source"]
> ;; Comment for ...
> ...
> EOF

I should have added a comment for that, I can't find any portable (but
suggestions welcome) way to do that and preserve the indentation, so
the test_cmp would still succeed if the moving/renaming function
munged all leading whitespace in the config with -\EOF as opposed to
\EOF.

>> + cat config.branch >>.git/config &&
>> + git branch -m source dest &&
>> + git config -f .git/config -l | grep -F -e source -e dest -e some.gar 
>> >actual &&
>> + test_cmp expect actual &&
>> +
>> + # ...and that the comments for those sections are also
>> + # preserved.
>> + cat config.branch | sed "s/\"source\"/\"dest\"/" >expect &&
>> + grep -A 9001 "Comment for source" .git/config >actual &&
>
> Where does 9001 come from?  Is that just "an arbitrary large
> number"?
>
> Besides, "grep -A" is quite unportable.  Would
>
> sed -n -e "/Comment for source/,$p" .git/config >actual
>
> work equally well?

It's just a sufficiently large number, I thought -A was portable
enough after grepping the test suite, but on closer inspection it
turns out those were all git-grep invocations, oops. Yeah all I need
here is all lines after a line matching a given string, so that sed
command works, will fix it up to use that.

>> + test_cmp expect actual
>> +'
>> +
>>  test_expect_success 'deleting a symref' '
>>   git branch target &&
>>   git symbolic-ref refs/heads/symref refs/heads/target &&
>>
>> --
>> https://github.com/git/git/pull/363

Re: [PATCH 3/3] branch: add a --copy (-c) option to go with --move (-m)

[Junio C Hamano]

Junio C Hamano  writes:

> Sahil Dua  writes:
>
>> Add the ability to --copy a branch and its reflog and configuration,
>> this uses the same underlying machinery as the --move (-m) option
>> except the reflog and configuration is copied instead of being moved.
>>
>> This is useful for e.g. copying a topic branch to a new version,
>> e.g. work to work-2 after submitting the work topic to the list, while
>> preserving all the tracking info and other configuration that goes
>> with the branch, and unlike --move keeping the other already-submitted
>> branch around for reference.
>>
>> Like --move, when the source branch is the currently checked out
>> branch the HEAD is moved to the destination branch. In the case of
>> --move we don't really have a choice (other than remaining on a
>> detached HEAD), but it makes sense to do the same for --copy.
>
> I strongly disagree with this "it makes sense to do the same".  It
> would equally (if not more) make sense to keep the HEAD pointing at
> the same.
>
> Personally, I may use this feature if it didn't move HEAD, but I
> wouldn't if HEAD gets moved.  But that may be just me.

Ah, that came out to be stronger than I intended.

While I do prefer "the HEAD is not moved by this command---if you
want to move to the newly created branch after copying, check it out
yourself" a lot better than what the patch does, I do not think I'd
care so strongly that I'd reject this patch series unless the
behaviour is changed.

But I do react strongly to an unsubstantiated claim "it makes sense
to do the same".  I can buy "We anticipate that in 50% of the case
users would find this branch switching annoying and in the other 50%
of the case, users would find it useful; since we need to pick one,
we just randomly decide to do the same as --move", though.

Re: [RFC/PATCH] builtin/blame: darken redundant line information

[Stefan Beller]

On Tue, Jun 13, 2017 at 10:19 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Here is what currently happens:
>>
>>>
>>>  context
>>> -B  dim  oldMoved
>>> -B  dim  oldMoved
>>> -B  highlight oldMovedAlternative
>>> -A  highlight oldMovedAlternative
>>> -A  dim  oldMoved
>>> -A  dim  oldMoved
>>>  context
>>> +A  dim  newMoved
>>> +A  dim  newMoved
>>> +A  highlight  newMovedAlternative
>>> +B  highlight  newMovedAlternative
>>> +B  dim  newMoved
>>> +B  dim  newMoved
>>>  context
>>>
>>
>> So the there is only one "highlight" color in each block.
>> There is no separate hightligh-for-ending-block and
>> highlight-for-new-block respectively.
>
> I think the adjacentbounds mode is simply broken if that is the
> design.

ok. Going by this reasoning, would you claim that allbounds would
also be broken design:

> git show --color-moved=allbounds:
>  context
> -B  oldMovedAlternative
> -B  oldMoved
> -B  oldMovedAlternative
> -A  oldMovedAlternative
> -A  oldMoved
> -A  oldMovedAlternative
>  context
> +A  newMovedAlternative
> +A  newMoved
> +A  newMovedAlternative
> +B  newMovedAlternative
> +B  newMoved
> +B  newMovedAlternative
>  context


>
> In the above simplified case, you can get away with only a single
> "highlight" color, but you cannot tell where the boundaries are when
> three or more lines are shuffled, no?

But you do not want to (yet)? The goal is not to tell you where the bounds
are, but the goal is to point out that extra care is required for review of
these particular 3 lines.

So IMHO this feature helps for drawing reviewer attention, but not for
explaining blocks.

In an extreme alternative design, we would have just annotated
each hunk in the context lines for example telling that there are
n out of m new lines. But that information by itself is not useful for
review

Instead this alternative moved line detection could have an impact
on diff stats.

Re: [PATCH] send-email: Add tocmd option to suppress-cc

[Junio C Hamano]

Viresh Kumar  writes:

>> Going back to the core part of your change, i.e.
>> 
>> -foreach my $entry (qw (cccmd cc author self sob body bodycc)) {
>> +foreach my $entry (qw (tocmd cccmd cc author self sob body bodycc)) {
>> 
>> to think about it a bit more, I notice that all the existing ones
>> are about CC.  So I am not just not convinced that tocmd belongs to
>> the same class.  I am inclined to say that it smells quite different
>> from others.
>
> That's right but how do we solve this problem then?
>
> Add another optional argument like suppress-to ? I thought that it
> would be better to override suppress-cc rather than attempting any
> such thing.
>
> I am fine with any solution that address the concerns raised by this
> patch though.

I am not sure what problem is being solved, quite honestly.  "I have
tocmd configured and I want a way not to specify tocmd for a single
invocation?"  Would something along the lines of 

git -c sendemail.tocmd=true send-email ...

how you do it in general?  Do you want a prettier-looking
equivalent, e.g.

git send-email --tocmd=true ...

or something like that?

Re: [RFC/PATCH] builtin/blame: darken redundant line information

[Junio C Hamano]

Stefan Beller  writes:

> Here is what currently happens:
>
>>
>>  context
>> -B  dim  oldMoved
>> -B  dim  oldMoved
>> -B  highlight oldMovedAlternative
>> -A  highlight oldMovedAlternative
>> -A  dim  oldMoved
>> -A  dim  oldMoved
>>  context
>> +A  dim  newMoved
>> +A  dim  newMoved
>> +A  highlight  newMovedAlternative
>> +B  highlight  newMovedAlternative
>> +B  dim  newMoved
>> +B  dim  newMoved
>>  context
>>
>
> So the there is only one "highlight" color in each block.
> There is no separate hightligh-for-ending-block and
> highlight-for-new-block respectively.

I think the adjacentbounds mode is simply broken if that is the
design.

In the above simplified case, you can get away with only a single
"highlight" color, but you cannot tell where the boundaries are when
three or more lines are shuffled, no?

Re: [PATCH 1/2] add: warn when adding an embedded repository

[Junio C Hamano]

Jeff King  writes:

> I also waffled on whether we should ask the submodule code
> whether it knows about a particular path. Technically:
>
>   git config submodule.foo.path foo
>   git config submodule.foo.url git://...
>   git add foo
>
> is legal, but would still warn with this patch. 

Did you mean "git config -f .gitmodules" for the first two?  If so,
I think I agree that (1) it should be legal and (2) we should be
able to add the check on top of this patch.

Or do you really mean the case in which these are in .git/config?

Re: [PATCH 1/2] add: warn when adding an embedded repository

[Brandon Williams]

On 06/13, Stefan Beller wrote:
> On Tue, Jun 13, 2017 at 2:24 AM, Jeff King  wrote:
> 
> > There is a config knob that can disable the (long) hint. But
> > I intentionally omitted a config knob to disable the warning
> > entirely. Whether the warning is sensible or not is
> > generally about context, not about the user's preferences.
> > If there's a tool or workflow that adds gitlinks without
> > matching .gitmodules, it should probably be taught about the
> > new command-line option, rather than blanket-disabling the
> > warning.
> >
> > Signed-off-by: Jeff King 
> > ---
> > The check for "is this a gitlink" is done by looking for a
> > trailing "/" in the added path. This feels kind of hacky,
> > but actually seems to work well in practice.
> 
> This whole "slash at the end" thing comes from extensive use
> of shell completion adding the slash at the end of a directory
> IMHO. (cf. PATHSPEC_STRIP_SUBMODULE_SLASH_* is
> the same underlying hack.)

I got rid of PATHSPEC_STRIP_SUBMODULE_SLASH_* recently, just an fyi.

-- 
Brandon Williams

Re: [PATCH 2/2] t: move "git add submodule" into test blocks

[Stefan Beller]

On Tue, Jun 13, 2017 at 2:24 AM, Jeff King  wrote:
> Some submodule tests do some setup outside of a test_expect
> block. This is bad because we won't actually check the
> outcome of those commands. But it's doubly so because "git
> add submodule" now produces a warning to stderr, which is
> not suppressed by the test scripts in non-verbose mode.

Makes sense.

> This patch does the minimal to fix the annoying warnings.
> All three of these scripts could use more cleanup of related
> setup.

agreed.

>
> Signed-off-by: Jeff King 

Reviewed-by: Stefan Beller 

Re: [RFC/PATCH] builtin/blame: darken redundant line information

[Stefan Beller]

On Tue, Jun 13, 2017 at 10:00 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> * As you have an individual color setup, maybe you can fix this
>>   for you by setting the appropriate slots to your perception of
>>   dimmed?
>
> I do not think it is possible with only {new,old}{,alternative} 4
> colors.
>
> Consider this diff:
>
>  context
> -B
> -B
> -B
> -A
> -A
> -A
>  context
> +A
> +A
> +A
> +B
> +B
> +B
>  context
>
> Two blocks (A and B) that are adjacent are moved but swapped to form
> a pair of new adjacent blocks.
>
> We would like the boundary between the last "-B" and the first "-A"
> to be highlighted differently; all other "-A" and "-B" lines do not
> disappear but go elsewhere, so they want to be dimmed.
>
> The newly added 6 lines are actually moved from elsewhere, and we
> would like the boundary between the last "+A" and the first "+B" to
> be highlighted differently, and others are dimmed.
>
> So I'd think you would need at least two kinds of highlight colors
> plus a dimmed color.

Here is what currently happens:

>
>  context
> -B  dim  oldMoved
> -B  dim  oldMoved
> -B  highlight oldMovedAlternative
> -A  highlight oldMovedAlternative
> -A  dim  oldMoved
> -A  dim  oldMoved
>  context
> +A  dim  newMoved
> +A  dim  newMoved
> +A  highlight  newMovedAlternative
> +B  highlight  newMovedAlternative
> +B  dim  newMoved
> +B  dim  newMoved
>  context
>

So the there is only one "highlight" color in each block.
There is no separate hightligh-for-ending-block and
highlight-for-new-block respectively.

> If old_moved and old_moved_alternative are meant for highlighting
> "-B" and "-A" above differently, while new_moved and
> new_moved_alternative are for highlighting "+A" and "+B"
> differently, you'd need a way to specify "dim" for old and new moved
> lines, which seems to be impossible with only 4 new colors.

The standard non alternative was dim in my mind for the
adjacentbounds mode.

If you prefer to have the alternate mode, you rather want no dim,
no highlight, but just 2 alternating colors of equal attention-drawing.

Re: [PATCH 2/3] branch: add test for -m renaming multiple config sections

[Junio C Hamano]

Sahil Dua  writes:

> + cat >expect <<-\EOF &&
> + branch.dest.key1=value1
> + some.gar.b=age
> + branch.dest.key2=value2
> + EOF
> + cat >config.branch <<\EOF &&
> +;; Comment for source
> +[branch "source"]
> + ;; Comment for the source value
> + key1 = value1
> + ;; Comment for some.gar
> +[some "gar"]
> + ;; Comment for the some.gar value
> + b = age
> + ;; Comment for source, again
> +[branch "source"]
> + ;; Comment for the source value, again
> + key2 = value2
> +EOF

Indenting using <<- would make it easier to read.  I.e.

cat >config.branch <<-\EOF &&
;; Comment for ...
[branch "source"]
;; Comment for ...
...
EOF

> + cat config.branch >>.git/config &&
> + git branch -m source dest &&
> + git config -f .git/config -l | grep -F -e source -e dest -e some.gar 
> >actual &&
> + test_cmp expect actual &&
> +
> + # ...and that the comments for those sections are also
> + # preserved.
> + cat config.branch | sed "s/\"source\"/\"dest\"/" >expect &&
> + grep -A 9001 "Comment for source" .git/config >actual &&

Where does 9001 come from?  Is that just "an arbitrary large
number"?

Besides, "grep -A" is quite unportable.  Would

sed -n -e "/Comment for source/,$p" .git/config >actual

work equally well?

> + test_cmp expect actual
> +'
> +
>  test_expect_success 'deleting a symref' '
>   git branch target &&
>   git symbolic-ref refs/heads/symref refs/heads/target &&
>
> --
> https://github.com/git/git/pull/363

Re: [PATCH 1/3] config: create a function to format section headers

[Ævar Arnfjörð Bjarmason]

On Tue, Jun 13, 2017 at 6:17 PM, Sahil Dua  wrote:
> Factor out the logic which creates section headers in the config file,
> e.g. the 'branch.foo' key will be turned into '[branch "foo"]'.

+CC Junio: This is to be applied, Sahil is using submitgit which
apparently doesn't CC you by default (I don't know if there's some
option for that he missed).

Also +CC Lars who hacked up -m initially, Alex who hacked on some of
the logic of git_config_rename_section(), and Johannes who also hacked
on some of the guts of that code.

Also, in the interest of full disclosure. Sahil's a co-worker of mine
who I've been mentoring on how to submit changes to git.git, after I
added a note to the internal company announcement saying I'd upgraded
git advertising that I'd be happy to help anyone who's interested
contribute to the project. That explains the off-list review & Sahil
submitting a patch of mine first seen as part of this series.

Re: [PATCH 1/2] add: warn when adding an embedded repository

[Stefan Beller]

On Tue, Jun 13, 2017 at 2:24 AM, Jeff King  wrote:
> It's an easy mistake to add a repository inside another
> repository, like:
>
>   git clone $url
>   git add .
>
> The resulting entry is a gitlink, but there's no matching
> .gitmodules entry. Trying to use "submodule init" (or clone
> with --recursive) doesn't do anything useful. Prior to
> v2.13, such an entry caused git-submodule to barf entirely.
> In v2.13, the entry is considered "inactive" and quietly
> ignored. Either way, no clone of your repository can do
> anything useful with the gitlink without the user manually
> adding the submodule config.
>
> In most cases, the user probably meant to either add a real
> submodule, or they forgot to put the embedded repository in
> their .gitignore file.
>
> Let's issue a warning when we see this case. There are a few
> things to note:
>
>   - the warning will go in the git-add porcelain; anybody
> wanting to do low-level manipulation of the index is
> welcome to create whatever funny states they want.
>
>   - we detect the case by looking for a newly added gitlink;
> updates via "git add submodule" are perfectly reasonable,
> and this avoids us having to investigate .gitmodules
> entirely
>
>   - there's a command-line option to suppress the warning.
> This is needed for git-submodule itself (which adds the
> entry before adding any submodule config), but also
> provides a mechanism for other scripts doing
> submodule-like things.
>
> We could make this a hard error instead of a warning.
> However, we do add lots of sub-repos in our test suite. It's
> not _wrong_ to do so. It just creates a state where users
> may be surprised. Pointing them in the right direction with
> a gentle hint is probably the best option.

Sounds good up to here (and right).

> There is a config knob that can disable the (long) hint. But
> I intentionally omitted a config knob to disable the warning
> entirely. Whether the warning is sensible or not is
> generally about context, not about the user's preferences.
> If there's a tool or workflow that adds gitlinks without
> matching .gitmodules, it should probably be taught about the
> new command-line option, rather than blanket-disabling the
> warning.
>
> Signed-off-by: Jeff King 
> ---
> The check for "is this a gitlink" is done by looking for a
> trailing "/" in the added path. This feels kind of hacky,
> but actually seems to work well in practice.

This whole "slash at the end" thing comes from extensive use
of shell completion adding the slash at the end of a directory
IMHO. (cf. PATHSPEC_STRIP_SUBMODULE_SLASH_* is
the same underlying hack.)

> We've already
> expanded the pathspecs to real filenames via dir.c, and that
> omits trees. So anything with a trailing slash must be a
> gitlink.

Oh!

>
> And I really didn't want to incur any extra cost in the
> common case here (e.g., checking for "path/.git"). We could
> do it at zero-cost by pushing the check much further down
> (i.e., when we'd realize anyway that it's a gitlink), but I
> didn't want to pollute read-cache.c with what is essentially
> a porcelain warning. The actual check done there seems to be
> checking S_ISDIR, but I didn't even want to incur an extra
> stat per-file.

makes sense.

>
> I also waffled on whether we should ask the submodule code
> whether it knows about a particular path. Technically:
>
>   git config submodule.foo.path foo
>   git config submodule.foo.url git://...
>   git add foo
>
> is legal, but would still warn with this patch. I don't know
> how much we should care (it would also be easy to do on
> top).

And here I was thinking this is not legal, because you may override
anything *except* submodule.*.path in the config. That is because
all the other settings (such as url, active flag, branch,
shallow recommendation) are dependent on the use case, the user,
changes to the environment (url) or such. The name<->path mapping
however is only to be changed via changes to the tracked content.
That is why it would make sense to disallow overriding the path
outside the tracked content.

In my ideal dream world of submodules we would have the following:

  $ cat .gitmodules
  [submodule "sub42"]
path = foo
  # path only in tree!

  $ cat .git/config
  ...
  [submodule]
active = .
active = :(exclude)Irrelevant/submodules/for/my/usecase/*
  # note how this is user centric

  $ git show refs/meta/magic/for/refs/heads/master:.gitmodules
  [submodule "sub42"]
url = https://example.org/foo
branch = .
  # Note how this is neither centering on the in-tree
  # contents, nor the user. Instead it focuses on the
  # project or group. It is *workflow* centric.
  # Workflows may change over time, e.g. the url could
  # be repointed to k.org or an in-house mirror without tree
  # changes.


But back to reviewing this patch.

>
>  Documentation/config.txt  |  3 +++
>  Documentation/git-add.txt |  7 +++
>  

Re: [PATCH 1/3] config: create a function to format section headers

[Junio C Hamano]

Sahil Dua  writes:

> Factor out the logic which creates section headers in the config file,
> e.g. the 'branch.foo' key will be turned into '[branch "foo"]'.
>
> This introduces no function changes, but is needed for a later change
> which adds support for copying branch sections in the config file.
>
> Signed-off-by: Sahil Dua 
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>  config.c | 13 +++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/config.c b/config.c
> index 146cb3452adab..d5bb69e925dac 100644
> --- a/config.c
> +++ b/config.c
> @@ -2169,10 +2169,10 @@ static int write_error(const char *filename)
>   return 4;
>  }
>  
> -static int store_write_section(int fd, const char *key)
> +struct strbuf store_create_section(const char *key)
>  {
>   const char *dot;
> - int i, success;
> + int i;
>   struct strbuf sb = STRBUF_INIT;
>  
>   dot = memchr(key, '.', store.baselen);
> @@ -2188,6 +2188,15 @@ static int store_write_section(int fd, const char *key)
>   strbuf_addf(, "[%.*s]\n", store.baselen, key);
>   }
>  
> + return sb;
> +}
> +
> +static int store_write_section(int fd, const char *key)
> +{
> + int success;
> +
> + struct strbuf sb = store_create_section(key);
> +
>   success = write_in_full(fd, sb.buf, sb.len) == sb.len;
>   strbuf_release();
>  
>
> --
> https://github.com/git/git/pull/363

Makes sense.

Re: [PATCH 4/4] config: don't implicitly use gitdir

[Jonathan Nieder]

Jeff King wrote:
> On Mon, Jun 12, 2017 at 06:38:17PM -0700, Jonathan Nieder wrote:
>> Brandon Williams wrote:
>>> On 06/12, Jonathan Nieder wrote:

 Alternatively, could this patch rename git_config_with_options?  That
 way any other patch in flight that calls git_config_with_options would
 conflict with this patch, giving us an opportunity to make sure it
 also sets git_dir.  As another nice side benefit it would make it easy
 for someone reading the patch to verify it didn't miss any callers.
>> [...]
>>> And I don't know if I agree with renaming a function just to rename it.
>>
>> I forgot to say: another way to accomplish the same thing can be to
>> reorder the function's arguments.  The relevant thing is to make code
>> that calls the function without being aware of the new requirements
>> fail to compile.
>
> If the parameter is now required, then it might make sense for it to
> become an actual function parameter instead of being stuffed into the
> config_options struct. That would give you your breaking change, plus
> make it more obvious to the reader that it is not optional.

I like it.  I also like that in your proposed patch the no longer
necessary local git_dir goes away.

What's the next step?  The discussion of get_git_common_dir() reveals
that switching from git_path() to mkpathdup() loses the
adjust_git_path() step and everything becomes complicated.  So perhaps
adding opts.git_common_dir and using that instead of git_path should
happen as a preliminary patch, making this patch afterward a more
straightforward refactoring.

Thanks,
Jonathan

Re: [PATCH 3/3] branch: add a --copy (-c) option to go with --move (-m)

[Junio C Hamano]

Sahil Dua  writes:

> Add the ability to --copy a branch and its reflog and configuration,
> this uses the same underlying machinery as the --move (-m) option
> except the reflog and configuration is copied instead of being moved.
>
> This is useful for e.g. copying a topic branch to a new version,
> e.g. work to work-2 after submitting the work topic to the list, while
> preserving all the tracking info and other configuration that goes
> with the branch, and unlike --move keeping the other already-submitted
> branch around for reference.
>
> Like --move, when the source branch is the currently checked out
> branch the HEAD is moved to the destination branch. In the case of
> --move we don't really have a choice (other than remaining on a
> detached HEAD), but it makes sense to do the same for --copy.

I strongly disagree with this "it makes sense to do the same".  It
would equally (if not more) make sense to keep the HEAD pointing at
the same.

Personally, I may use this feature if it didn't move HEAD, but I
wouldn't if HEAD gets moved.  But that may be just me.

Re: [RFC/PATCH] builtin/blame: darken redundant line information

[Junio C Hamano]

Stefan Beller  writes:

> * As you have an individual color setup, maybe you can fix this
>   for you by setting the appropriate slots to your perception of
>   dimmed?

I do not think it is possible with only {new,old}{,alternative} 4
colors.

Consider this diff:

 context
-B
-B
-B
-A
-A
-A
 context
+A
+A
+A
+B
+B
+B
 context

Two blocks (A and B) that are adjacent are moved but swapped to form
a pair of new adjacent blocks.  

We would like the boundary between the last "-B" and the first "-A"
to be highlighted differently; all other "-A" and "-B" lines do not
disappear but go elsewhere, so they want to be dimmed.

The newly added 6 lines are actually moved from elsewhere, and we
would like the boundary between the last "+A" and the first "+B" to
be highlighted differently, and others are dimmed.  

So I'd think you would need at least two kinds of highlight colors
plus a dimmed color.

 context
-B  dim
-B  dim
-B  highlight
-A  highlight
-A  dim
-A  dim
 context
+A  dim
+A  dim
+A  highlight
+B  highlight
+B  dim
+B  dim
 context

If old_moved and old_moved_alternative are meant for highlighting
"-B" and "-A" above differently, while new_moved and
new_moved_alternative are for highlighting "+A" and "+B"
differently, you'd need a way to specify "dim" for old and new moved
lines, which seems to be impossible with only 4 new colors.

Re: [PATCH v2 7/8] alias_lookup(): optionally return top-level directory

[Junio C Hamano]

Jeff King  writes:

> On Tue, Jun 13, 2017 at 01:42:02PM +0200, Johannes Schindelin wrote:
>
>> > As you probably guessed, I had tried that first and then figured that if
>> > I needed to keep the config_key_is_valid() test anyway, I could just as
>> > well keep the strbuf around for later use.
>> > 
>> > Will change the code,
>> 
>> Alas, I won't change the code after all.
>> 
>> It is really tempting to avoid the extra strbuf, but then the error
>> message would change from
>> 
>>  error: missing value for 'alias.br'
>> 
>> to
>> 
>>  error: missing value for 'br'
>> 
>> which is of course no good at all.
>> 
>> And since I already have to keep that strbuf, I'll simply keep the
>> config_key_is_valid() guard, too (because why not).
>
> Oof, yeah, that is definitely worse. I'm fine with keeping both parts.

When you replace Dscho's "compare 'var' with 'alias.br' that is in
strbuf naively with the "skip-prefix and compare with br" without
changing anything else, i.e.

if (skip_prefix(var, "alias.", ) && !strcmp(key, data->key))
return git_config_string((const char **)>v, key, value);

it would cause the "br" to be fed to git_config_string() and result
in problem reported for "br", not "alias.br".  

But this can be trivially fixed by passing "var" instead of "key" to
git_config_string(), no?  Am I mistaken?

Re: [PATCH v3 6/6] Use the early config machinery to expand aliases

[Junio C Hamano]

Johannes Schindelin  writes:

> Instead of discovering the .git/ directory, read the config and then
> trying to painstakingly reset all the global state if we did not find a
> matching alias, let's use the early config machinery instead.

s/read//, I think.  My reading hiccupped while trying to figure
out what two alternative approaches are being compared.

> It may look like unnecessary work to discover the .git/ directory in the
> early config machinery and then call setup_git_directory_gently() in the
> case of a shell alias, repeating the very same discovery *again*.
> However, we have to do this as the early config machinery takes pains
> *not* to touch any global state, while shell aliases expect a possibly
> changed working directory and at least the GIT_PREFIX and GIT_DIR
> variables to be set.

Makes sense.  Nicely explained.

> Also, one might be tempted to streamline the code in alias_lookup() to
> *not* use a strbuf for the key. However, if the config reports an error,
> it is far superior to tell the user that the `alias.xyz` key had a
> problem than to claim that it was the `xyz` key.

The mention of "streamline" is puzzling to me.  When we are trying
"git xyz", "alias.xyz" is the key we would look up, not "xyz"; it is
not clear to anybody who didn't read the discussion on v2 (which
includes the readers of "git log" in a few months) what kind of flawed
streamlining could look up "xyz" and result in a bad configuration
reported on it.

>  alias.c  | 31 ---
>  git.c| 55 ---
>  t/t7006-pager.sh |  2 +-
>  3 files changed, 29 insertions(+), 59 deletions(-)

Happy to see the deletion of all the save/restore-env stuff.

Except for the puzzlement in one paragraph in the log, looks very
good.  Thanks for a pleasant reading.

Re: [RFC/PATCH] builtin/blame: darken redundant line information

[Stefan Beller]

On Tue, Jun 13, 2017 at 8:25 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> When using git-blame lots of lines contain redundant information, for
>> example in hunks that consist of multiple lines, the metadata (commit name,
>> author, timezone) are repeated. A reader may not be interested in those,
>> so darken them. The darkening is not just based on hunk, but actually
>> takes the previous lines content for that field to compare to.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>
> Not about "blame", but I was trying the --color-moved stuff on
> Brandon's "create config.h" patch and found its behaviour somewhat
> different from what I recall we discussed.

We discussed several things and we did not come to an agreement,
what is best, so I implemented different things there.

> I thought that the
> adjacentbounds mode was invented to dim (i.e. not attract undue
> attention) to most of the moved lines, but highlight only the
> boundary of moved blocks,

I agree up to this part. And when you use the standard color scheme,
the new/old moved is dimmed according to my perception of colors.

If you use an individual setup for colors, you need to adjust the four
color.diff.{old, new}Moved[Alternative] slots.

> so I expected most of the new and old
> lines in that patch would be shown in the "context" color,

So instead of a special color in this mode you expected "context"
for color.diff.{old, new}Moved and "highlighted" for the alternative slots.

> except
> for the boundary between two blocks of removed lines that have gone
> to different location (and similarly two blocks of new lines that
> have come from different location) would be painted in oldmoved and
> newmoved colors and their alternatives.  Instead I see all old and
> new lines that are moved painted in these colors, without any
> dimming.

http://i.imgur.com/36DMRBl.png is what I see (regular colors,
"git show --color-moved f2f1da5348ff2f297b43b") and
that is what I would expect.

>
> Is my expectation off?

Maybe? It sounds as if you expected 'context' color to be used
in the adjacentbounds.

* Do you expect 'context' to be used in other modes as well?

* Do you expect this as code/algorithm change or would rather
  a change of default colors (default {old/new}Moved = 'context')
  be sufficient to meet that expectation?

* As you have an individual color setup, maybe you can fix this
  for you by setting the appropriate slots to your perception of
  dimmed?

Thanks,
Stefan

[PATCH 3/3] branch: add a --copy (-c) option to go with --move (-m)

[Sahil Dua]

Add the ability to --copy a branch and its reflog and configuration,
this uses the same underlying machinery as the --move (-m) option
except the reflog and configuration is copied instead of being moved.

This is useful for e.g. copying a topic branch to a new version,
e.g. work to work-2 after submitting the work topic to the list, while
preserving all the tracking info and other configuration that goes
with the branch, and unlike --move keeping the other already-submitted
branch around for reference.

Like --move, when the source branch is the currently checked out
branch the HEAD is moved to the destination branch. In the case of
--move we don't really have a choice (other than remaining on a
detached HEAD), but it makes sense to do the same for --copy.

The most common usage of this feature is expected to be moving to a
new topic branch which is a copy of the current one, in that case
moving to the target branch is what the user wants, and doesn't
unexpectedly behave differently than --move would.

One outstanding caveat of this implementation is that:

git checkout maint &&
git checkout master &&
git branch -c topic &&
git checkout -

Will check out 'maint' instead of 'master'. This is because the @{-N}
feature (or its -1 shorthand "-") relies on HEAD reflogs created by
the checkout command, so in this case we'll checkout maint instead of
master, as the user might expect. What to do about that is left to a
future change.

Helped-by: Ævar Arnfjörð Bjarmason 
Signed-off-by: Sahil Dua 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-branch.txt |  14 ++-
 builtin/branch.c |  67 ++
 cache.h  |   2 +
 config.c | 102 +
 refs.c   |  11 +++
 refs.h   |   9 +-
 refs/files-backend.c |  46 --
 refs/refs-internal.h |   4 +
 t/t3200-branch.sh| 211 +++
 9 files changed, 420 insertions(+), 46 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 81bd0a7b7741f..94fd89ddc5ab9 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -18,6 +18,7 @@ SYNOPSIS
 'git branch' (--set-upstream-to= | -u ) []
 'git branch' --unset-upstream []
 'git branch' (-m | -M) [] 
+'git branch' (-c | -C) [] 
 'git branch' (-d | -D) [-r] ...
 'git branch' --edit-description []
 
@@ -64,6 +65,10 @@ If  had a corresponding reflog, it is renamed to 
match
 renaming. If  exists, -M must be used to force the rename
 to happen.
 
+The `-c` and `-C` options have the exact same semantics as `-m` and
+`-M`, except instead of the branch being renamed it along with its
+config and reflog will be copied to a new name.
+
 With a `-d` or `-D` option, `` will be deleted.  You may
 specify more than one branch for deletion.  If the branch currently
 has a reflog then the reflog will also be deleted.
@@ -104,7 +109,7 @@ OPTIONS
In combination with `-d` (or `--delete`), allow deleting the
branch irrespective of its merged status. In combination with
`-m` (or `--move`), allow renaming the branch even if the new
-   branch name already exists.
+   branch name already exists, the same applies for `-c` (or `--copy`).
 
 -m::
 --move::
@@ -113,6 +118,13 @@ OPTIONS
 -M::
Shortcut for `--move --force`.
 
+-c::
+--copy::
+   Copy a branch and the corresponding reflog.
+
+-C::
+   Shortcut for `--copy --force`.
+
 --color[=]::
Color branches to highlight current, local, and
remote-tracking branches.
diff --git a/builtin/branch.c b/builtin/branch.c
index 83fcda43dceec..89f64f4123a5e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -27,6 +27,7 @@ static const char * const builtin_branch_usage[] = {
N_("git branch [] [-l] [-f]  []"),
N_("git branch [] [-r] (-d | -D) ..."),
N_("git branch [] (-m | -M) [] "),
+   N_("git branch [] (-c | -C) [] "),
N_("git branch [] [-r | -a] [--points-at]"),
N_("git branch [] [-r | -a] [--format]"),
NULL
@@ -449,15 +450,19 @@ static void reject_rebase_or_bisect_branch(const char 
*target)
free_worktrees(worktrees);
 }
 
-static void rename_branch(const char *oldname, const char *newname, int force)
+static void copy_or_rename_branch(const char *oldname, const char *newname, 
int copy, int force)
 {
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
STRBUF_INIT;
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
int recovery = 0;
int clobber_head_ok;
 
-   if (!oldname)
-   die(_("cannot rename the current branch while not on any."));
+   if (!oldname) {
+   if (copy)
+   die(_("cannot copy the current branch while not on 
any."));
+   

[PATCH 2/3] branch: add test for -m renaming multiple config sections

[Sahil Dua]

From: Ævar Arnfjörð Bjarmason 

Add a test for how 'git branch -m' handles the renaming of multiple
config sections existing for one branch.

The config format we use is hybrid machine/human editable, and we do
our best to preserve the likes of comments and formatting when editing
the file with git-config.

This adds a test for the currently expected semantics in the face of
some rather obscure edge cases which are unlikely to occur in
practice.

Helped-by: Sahil Dua 
Signed-off-by: Ævar Arnfjörð Bjarmason 
Signed-off-by: Sahil Dua 
---
 t/t3200-branch.sh | 37 +
 1 file changed, 37 insertions(+)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 10f8f026ffb4b..28c02ffeadb4f 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -341,6 +341,43 @@ test_expect_success 'config information was renamed, too' '
test_must_fail git config branch.s/s.dummy
 '
 
+test_expect_success 'git branch -m correctly renames multiple config sections' 
'
+   test_when_finished "git checkout master" &&
+   git checkout -b source master &&
+
+   # Assert that a config file with multiple config sections has
+   # those sections preserved...
+   cat >expect <<-\EOF &&
+   branch.dest.key1=value1
+   some.gar.b=age
+   branch.dest.key2=value2
+   EOF
+   cat >config.branch <<\EOF &&
+;; Comment for source
+[branch "source"]
+   ;; Comment for the source value
+   key1 = value1
+   ;; Comment for some.gar
+[some "gar"]
+   ;; Comment for the some.gar value
+   b = age
+   ;; Comment for source, again
+[branch "source"]
+   ;; Comment for the source value, again
+   key2 = value2
+EOF
+   cat config.branch >>.git/config &&
+   git branch -m source dest &&
+   git config -f .git/config -l | grep -F -e source -e dest -e some.gar 
>actual &&
+   test_cmp expect actual &&
+
+   # ...and that the comments for those sections are also
+   # preserved.
+   cat config.branch | sed "s/\"source\"/\"dest\"/" >expect &&
+   grep -A 9001 "Comment for source" .git/config >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'deleting a symref' '
git branch target &&
git symbolic-ref refs/heads/symref refs/heads/target &&

--
https://github.com/git/git/pull/363

[PATCH 1/3] config: create a function to format section headers

[Sahil Dua]

Factor out the logic which creates section headers in the config file,
e.g. the 'branch.foo' key will be turned into '[branch "foo"]'.

This introduces no function changes, but is needed for a later change
which adds support for copying branch sections in the config file.

Signed-off-by: Sahil Dua 
Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 config.c | 13 +++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 146cb3452adab..d5bb69e925dac 100644
--- a/config.c
+++ b/config.c
@@ -2169,10 +2169,10 @@ static int write_error(const char *filename)
return 4;
 }
 
-static int store_write_section(int fd, const char *key)
+struct strbuf store_create_section(const char *key)
 {
const char *dot;
-   int i, success;
+   int i;
struct strbuf sb = STRBUF_INIT;
 
dot = memchr(key, '.', store.baselen);
@@ -2188,6 +2188,15 @@ static int store_write_section(int fd, const char *key)
strbuf_addf(, "[%.*s]\n", store.baselen, key);
}
 
+   return sb;
+}
+
+static int store_write_section(int fd, const char *key)
+{
+   int success;
+
+   struct strbuf sb = store_create_section(key);
+
success = write_in_full(fd, sb.buf, sb.len) == sb.len;
strbuf_release();
 

--
https://github.com/git/git/pull/363

Re: proposal for how to share other refs as part of refs/tracking/*

[Marc Branchaud]

On 2017-06-13 10:41 AM, Marc Branchaud wrote:


So I like your refs/tracking proposal, and hope that it aims for 
mirroring a remote's refs, to eventually replace refs/remotes entirely.


To be extra-clear:

I think a refs/tracking hierarchy that starts with notes and maybe some 
other bits is a good first step.


I *hope* such a first step can eventually mirror all of a remote's refs, 
including heads and tags.


I in no way think that this effort should begin by tackling heads and tags.

M.

Re: [PATCH] git-stash: fix pushing stash with pathspec from subdir

[Junio C Hamano]

Patrick Steinhardt  writes:

> The `git stash push` command recently gained the ability to get a
> pathspec as its argument to only stash matching files. Calling this
> command from a subdirectory does not work, though, as one of the first
> things we do is changing to the top level directory without keeping
> track of the prefix from which the command is being run.
>
> Fix the shortcoming by storing the prefix previous to the call to
> `cd_to_toplevel` and then subsequently using `git rev-parse --prefix` to
> correctly resolve the pathspec. Add a test to catch future breakage of
> this usecase.

It sounds more like a simple bug than "shortcoming" ;-), and the
right approach to fix it is to add the original prefix before the
pathspecs before using them, which is exactly what your patch does.
Looks good.

I suspect that "rev-parse --prefix" needs a bit of tweak to make it
truly usable for pathspecs with magic, but that is a totally
separate issue.

Thanks.

> Signed-off-by: Patrick Steinhardt 
> ---
>  git-stash.sh |  3 +++
>  t/t3903-stash.sh | 16 
>  2 files changed, 19 insertions(+)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 2fb651b2b..e7b85932d 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -19,6 +19,7 @@ OPTIONS_SPEC=
>  START_DIR=$(pwd)
>  . git-sh-setup
>  require_work_tree
> +prefix=$(git rev-parse --show-prefix) || exit 1
>  cd_to_toplevel
>  
>  TMP="$GIT_DIR/.git-stash.$$"
> @@ -273,6 +274,8 @@ push_stash () {
>   shift
>   done
>  
> + eval "set $(git rev-parse --sq --prefix "$prefix" -- "$@")"
> +
>   if test -n "$patch_mode" && test -n "$untracked"
>   then
>   die "$(gettext "Can't use --patch and --include-untracked or 
> --all at the same time")"
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 3b4bed5c9..4046817d7 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -812,6 +812,22 @@ test_expect_success 'stash --  stashes and 
> restores the file' '
>   test_path_is_file bar
>  '
>  
> +test_expect_success 'stash --  stashes in subdirectory' '
> + mkdir sub &&
> + >foo &&
> + >bar &&
> + git add foo bar &&
> + (
> + cd sub &&
> + git stash push -- ../foo
> + ) &&
> + test_path_is_file bar &&
> + test_path_is_missing foo &&
> + git stash pop &&
> + test_path_is_file foo &&
> + test_path_is_file bar
> +'
> +
>  test_expect_success 'stash with multiple pathspec arguments' '
>   >foo &&
>   >bar &&

Re: [RFC/PATCH] builtin/blame: darken redundant line information

[Junio C Hamano]

Stefan Beller  writes:

> When using git-blame lots of lines contain redundant information, for
> example in hunks that consist of multiple lines, the metadata (commit name,
> author, timezone) are repeated. A reader may not be interested in those,
> so darken them. The darkening is not just based on hunk, but actually
> takes the previous lines content for that field to compare to.
>
> Signed-off-by: Stefan Beller 
> ---

Not about "blame", but I was trying the --color-moved stuff on
Brandon's "create config.h" patch and found its behaviour somewhat
different from what I recall we discussed.  I thought that the
adjacentbounds mode was invented to dim (i.e. not attract undue
attention) to most of the moved lines, but highlight only the
boundary of moved blocks, so I expected most of the new and old
lines in that patch would be shown in the "context" color, except
for the boundary between two blocks of removed lines that have gone
to different location (and similarly two blocks of new lines that
have come from different location) would be painted in oldmoved and
newmoved colors and their alternatives.  Instead I see all old and
new lines that are moved painted in these colors, without any
dimming.

Is my expectation off?

Re: [PATCH 4/4] config: don't implicitly use gitdir

[Brandon Williams]

On 06/13, Jeff King wrote:
> On Mon, Jun 12, 2017 at 10:52:43PM -0700, Brandon Williams wrote:
> 
> > > >> curious: Why get_git_common_dir() instead of get_git_dir()?
> > > >
> > > > Needs to be commondir since the config is stored in the common git
> > > > directory and not a per worktree git directory.
> > > 
> > > *puzzled* Why wasn't this needed before, then?  The rest of the patch
> > > should result in no functional change, but this part seems different.
> > 
> > there is no functional change, this is what always happened.
> > git_path("config") will replace gitdir with commondir under the hood.
> 
> Of the two callsites you removed, one is git_pathdup(), and the other
> is get_git_dir(). So they weren't matched, though I suspect the one in
> include_by_gitdir probably ought to have been commondir?

Good point, I hope my reply on the other part of the thread answers
this.

-- 
Brandon Williams