Re: [PATCH v9 02/20] ref-filter: include reference to 'used_atom' within 'atom_value'

[Karthik Nayak]

On Wed, Dec 28, 2016 at 2:29 AM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>> From: Karthik Nayak 
>>
>> Ensure that each 'atom_value' has a reference to its corresponding
>> 'used_atom'. This let's us use values within 'used_atom' in the
>
> s/let's us use/lets us use/;
>

Noted, will change.

-- 
Regards,
Karthik Nayak

Re: [PATCH v9 03/20] ref-filter: implement %(if:equals=) and %(if:notequals=)

[Karthik Nayak]

On Wed, Dec 28, 2016 at 2:30 AM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>> This is done by introducing 'if_atom_parser()' which parses the given
>> %(if) atom and then stores the data in used_atom which is later passed
>> on to the used_atom of the %(then) atom, so that it can do the required
>> comparisons.
>>
>> Add tests and Documentation for the same.
>
> s/Documentation/documentation/

Thanks, will change.

-- 
Regards,
Karthik Nayak

[RFH] gpg --import entropy while running tests

[Jeff King]

I noticed a few of our test scripts taking a long time to run, even
though they used to be quick. Here's one:

  $ time ./t7612-merge-verify-signatures.sh 
  ok 1 - create signed commits
  ok 2 - merge unsigned commit with verification
  ok 3 - merge commit with bad signature with verification
  ok 4 - merge commit with untrusted signature with verification
  ok 5 - merge signed commit with verification
  ok 6 - merge commit with bad signature without verification
  # passed all 6 test(s)
  1..6

  real0m12.285s
  user0m0.048s
  sys 0m0.044s


That's a lot of time not using any CPU. What's going on? Running with
"sh -x" shows that we spend most of the time in this line from
lib-gpg.sh:

  gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
  "$TEST_DIRECTORY"/lib-gpg/keyring.gpg

And running gpg with "--debug-level guru" shows that we are blocking
while waiting for entropy. Has anybody else seen this? I feel like I
noticed it starting a few weeks ago, and indeed dropping back to gpg
2.0.26 (from 2.1.17) makes the problem go away.

Is it a bug in gpg (oddly, the kernel reports lots of entropy available,
and generating the signatures themselves is quite fast)? Or is the new
version doing something special in the import process that we need to
work around or disable?

The reason we use --import at all is to handle differences in the .gnupg
format between versions 1 and 2. So the nuclear option would be to just
carry pre-made .gnupg directories for _both_ versions in our test suite,
and pick the appropriate one based on the output of "gpg --version".
That feels like a hack, but it gives us a lot of control.

I'd love it if we could figure out a way of making --import reliably
fast, though.

-Peff

Re: [PATCH v9 01/20] ref-filter: implement %(if), %(then), and %(else) atoms

[Karthik Nayak]

On Wed, Dec 28, 2016 at 2:28 AM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>>
>> +Some atoms like %(align) and %(if) always require a matching %(end).
>> +We call them "opening atoms" and sometimes denote them as %($open).
>> +
>> +When a scripting language specific quoting is in effect, everything
>> +between a top-level opening atom and its matching %(end) is evaluated
>> +according to the semantics of the opening atom and its result is
>> +quoted.
>
> What is unsaid in the last paragraph is that you assume "is
> evaluated according to the semantics of the opening atom" does not
> involve quoting and only the result from the top-level is quoted.  I
> am not sure if that is clear to the first-time readers.
>

How about being a little more explicit about that?

When a scripting language specific quoting is in effect, everything
between a top-level opening atom and its matching %(end) is evaluated
according to the semantics of the opening atom and only its result
from the top-level is quoted.

>>
>>  EXAMPLES
>>  
>> @@ -273,6 +291,22 @@ eval=`git for-each-ref --shell --format="$fmt" \
>>  eval "$eval"
>>  
>> ...
>> +
>> +git for-each-ref --format="%(refname)%(if)%(authorname)%(then) 
>> %(color:red)Authored by: %(authorname)%(end)"
>> +
>
> This makes readers wonder how "red"ness is reset, but that is not
> something this example is interested in demonstrating.  Let's drop
> the %(color:red) bit to avoid distracting readers.

Sure, will do :)

-- 
Regards,
Karthik Nayak

Re: Gitview Shell Injection Vulnerability

[Jeff King]

On Tue, Dec 27, 2016 at 10:45:58AM -0800, Stefan Beller wrote:

> > I expect that things that start their life in the contrib/ area
> > to graduate out of contrib/ once they mature, either by becoming
> > projects on their own, or moving to the toplevel directory.  On
> > the other hand, I expect I'll be proposing removal of disused
> > and inactive ones from time to time.
> 
> Maybe it's time for a spring cleanup and remove some old (dead?)
> projects from contrib?

Yeah, that was my thought on reading the vulnerability report: "that's
still around?". Looks like it hasn't had a substantive commit since
2007. But I dunno, maybe it's just finished, and doesn't need any more
changes. :)

-Peff

Re: HowTo distribute a hook with the reposity.

[Jeff King]

On Tue, Dec 27, 2016 at 09:32:22PM -0800, Jacob Keller wrote:

> On Tue, Dec 27, 2016 at 5:34 PM, John P. Hartmann  
> wrote:
> > I would like a hook in .got/hooks to be made available to all who clone or
> > pull a particular project.  I'd also like the hook to be under git control
> > (changes committed ).  I added a hook, but git status does not show it.
> > Presumably git excludes its files in .git/ from version control lest there
> > be a chiken-and-egg situation.
> >
> > Is there a way to achieve this?  Or a better way to do it?
> >
> > Thanks,  j.
> 
> Best way I found, was add a script with an "installme" shell script or
> similar that you tell all users of the repository that they are
> expected to run this to install the scripts. You can' make it happen
> automatically.

I agree that is the best way to do it.

I didn't dig up previous discussions, but the gist is usually:

  1. Cloning a repository should not run arbitrary code from the remote
 without the user on the cloning side taking some further affirmative
 action.

 This is for security reasons. Obviously the next step is quite often
 to run "make" which does run arbitrary code, but that counts as an
 action.

  2. We could write a feature in git that manages hooks (or config, etc).
 But ultimately you would still be running "git clone
 --trust-remote-hooks" or something to satisfy point (1).

  3. There's not much point in doing point (2), because you can just
 spell it as "git clone && cd clone && ./install-hooks" and then git
 does not have to care at all about your scripts.

  4. A hook (or config) management system could do fancy things like
 merging your custom local config, picking up changes from the
 remote, etc. But all of that can happen outside of Git totally (and
 quite often you want to manage things in contributors setups
 _besides_ Git data anyway).

 An example system is:

   https://github.com/Autodesk/enterprise-config-for-git

 (with the disclaimer that I've never used it myself, so I have no
 idea how good it is).

I think you probably know all that, Jake, but I am laying it out for the
benefit of the OP and the list. :)

-Peff

Re: [PATCH] pathspec: give better message for submodule related pathspec error

[Jeff King]

On Tue, Dec 27, 2016 at 04:05:59PM -0800, Stefan Beller wrote:

> Every once in a while someone complains to the mailing list to have
> run into this weird assertion[1].

If people are running into it, it definitely should not be an assert,
nor a die("BUG"). It should be a regular die(), which your patch does.
So this is definitely a good step, even if the ultimate goal may be to
handle the case more gracefully (I say that without having even read the
background, or knowing what the right handling would be).

But...

> diff --git a/pathspec.c b/pathspec.c
> index 22ca74a126..d522f43331 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -313,8 +313,11 @@ static unsigned prefix_pathspec(struct pathspec_item 
> *item,
>   }
>  
>   /* sanity checks, pathspec matchers assume these are sane */
> - assert(item->nowildcard_len <= item->len &&
> -item->prefix <= item->len);
> + if (item->nowildcard_len <= item->len &&
> + item->prefix <= item->len)
> + die (_("Path leads inside submodule '%s', but the submodule "
> +"was not recognized, i.e. not initialized or deleted"),
> +ce->name);

Don't you need to flip the logic here? An assert() triggers when the
condition is not true, but an "if" does the opposite. So "assert(X)"
should always become "if (!X) die(...)".

-Peff

Re: HowTo distribute a hook with the reposity.

[Jacob Keller]

On Tue, Dec 27, 2016 at 5:34 PM, John P. Hartmann  wrote:
> I would like a hook in .got/hooks to be made available to all who clone or
> pull a particular project.  I'd also like the hook to be under git control
> (changes committed ).  I added a hook, but git status does not show it.
> Presumably git excludes its files in .git/ from version control lest there
> be a chiken-and-egg situation.
>
> Is there a way to achieve this?  Or a better way to do it?
>
> Thanks,  j.

Best way I found, was add a script with an "installme" shell script or
similar that you tell all users of the repository that they are
expected to run this to install the scripts. You can' make it happen
automatically.

Thanks,
Jake

Re: [PATCH v9 15/20] ref-filter: modify the 'lstrip=' option to work with negative ''

[Jacob Keller]

On Tue, Dec 27, 2016 at 1:11 PM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>> Currently the 'lstrip=' option only takes a positive value ''
>> and strips '' slash-separated path components from the left. Modify
>> the 'lstrip' option to also take a negative number '' which would
>> only _leave_ behind 'N' slash-separated path components from the left.
>
> "would only leave behind N components from the left" sounds as if
> the result is A/B, when you are given A/B/C/D/E and asked to
> lstrip:-2.  Given these two tests added by the patch ...
>
>> +test_atom head refname:lstrip=-1 master
>> +test_atom head refname:lstrip=-2 heads/master
>
> ... I somehow think that is not what you wanted to say.  Instead,
> you strip from the left as many as necessary and leave -N
> components that appear at the right-most end, no?
>
>> --- a/Documentation/git-for-each-ref.txt
>> +++ b/Documentation/git-for-each-ref.txt
>> @@ -98,7 +98,8 @@ refname::
>>   abbreviation mode. If `lstrip=` is appended, strips ``
>>   slash-separated path components from the front of the refname
>>   (e.g., `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo`.
>> - `` must be a positive integer.
>> + if `` is a negative number, then only `` path components
>> + are left behind.
>
> I think positive  is so obvious not to require an example but it
> is good that you have one.  The negative  case needs illustration
> more than the positive case.  Perhaps something like:
>
> (e.g. %(refname:lstrip=-1) strips components of refs/tags/frotz
> from the left to leave only one component, i.e. 'frotz').
>
> Would %(refname:lstrip=-4) attempt to strip components of
> refs/tags/frotz from the left to leave only four components, and
> because the original does not have that many components, it ends
> with refs/tags/frotz?
>
> I am debating myself if we need something like "When the ref does
> not have enough components, the result becomes an empty string if
> stripping with positive , or it becomes the full refname if
> stripping with negative .  Neither is an error." is necessary
> here.  Or is it too obvious?

I do not think it hurts to have, and makes this obvious.

Thanks,
Jake

HowTo distribute a hook with the reposity.

[John P. Hartmann]

I would like a hook in .got/hooks to be made available to all who clone 
or pull a particular project.  I'd also like the hook to be under git 
control (changes committed ).  I added a hook, but git status does not 
show it.  Presumably git excludes its files in .git/ from version 
control lest there be a chiken-and-egg situation.


Is there a way to achieve this?  Or a better way to do it?

Thanks,  j.

[PATCH] pathspec: give better message for submodule related pathspec error

[Stefan Beller]

Every once in a while someone complains to the mailing list to have
run into this weird assertion[1].

The usual response from the mailing list is link to old discussions[2],
and acknowledging the problem stating it is known.

For now just improve the user visible error message.

[1] https://www.google.com/search?q=item-%3Enowildcard_len
[2] 
http://git.661346.n2.nabble.com/assert-failed-in-submodule-edge-case-td7628687.html
https://www.spinics.net/lists/git/msg249473.html

Signed-off-by: Stefan Beller 
---

If you were following the mailing list closely today, you may sense
that I am cleaning up stalled branches. :)

I think such a hot fix is warranted given how often we had reports
on the mailing list.

Thanks,
Stefan

 pathspec.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 22ca74a126..d522f43331 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -313,8 +313,11 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
}
 
/* sanity checks, pathspec matchers assume these are sane */
-   assert(item->nowildcard_len <= item->len &&
-  item->prefix <= item->len);
+   if (item->nowildcard_len <= item->len &&
+   item->prefix <= item->len)
+   die (_("Path leads inside submodule '%s', but the submodule "
+  "was not recognized, i.e. not initialized or deleted"),
+  ce->name);
return magic;
 }
 
-- 
2.11.0.196.gee862f456e.dirty

[PATCH 2/3] submodule update documentation: don't repeat ourselves

[Stefan Beller]

The documentation for the `git submodule update` command, repeats itself
for each update option, "This is done when  is given, or no
option is given and `submodule..update` is set to .

Avoid these repetitive clauses by stating the command line options take
precedence over configured options.

Also add 'none' to the list of options instead of mentioning it in the
following running text and split the list into two parts, one that is
accessible via the command line and one that is only reachable via the
configuration variables.

Signed-off-by: Stefan Beller 
---
 Documentation/git-submodule.txt | 30 ++
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index f9da0d9e91..7e03ccc444 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -153,13 +153,13 @@ Update the registered submodules to match what the 
superproject
 expects by cloning missing submodules and updating the working tree of
 the submodules. The "updating" can be done in several ways depending
 on command line options and the value of `submodule..update`
-configuration variable. Supported update procedures are:
+configuration variable. The command line option takes precedence over
+the configuration variable. if neither is given, a checkout is performed.
+update procedures supported both from the command line as well as setting
+`submodule..update`:
 
checkout;; the commit recorded in the superproject will be
-   checked out in the submodule on a detached HEAD. This is
-   done when `--checkout` option is given, or no option is
-   given, and `submodule..update` is unset, or if it is
-   set to 'checkout'.
+   checked out in the submodule on a detached HEAD.
 +
 If `--force` is specified, the submodule will be checked out (using
 `git checkout --force` if appropriate), even if the commit specified
@@ -167,23 +167,21 @@ in the index of the containing repository already matches 
the commit
 checked out in the submodule.
 
rebase;; the current branch of the submodule will be rebased
-   onto the commit recorded in the superproject. This is done
-   when `--rebase` option is given, or no option is given, and
-   `submodule..update` is set to 'rebase'.
+   onto the commit recorded in the superproject.
 
merge;; the commit recorded in the superproject will be merged
-   into the current branch in the submodule. This is done
-   when `--merge` option is given, or no option is given, and
-   `submodule..update` is set to 'merge'.
+   into the current branch in the submodule.
+
+The following procedures are only available via the `submodule..update`
+configuration variable:
 
custom command;; arbitrary shell command that takes a single
argument (the sha1 of the commit recorded in the
-   superproject) is executed. This is done when no option is
-   given, and `submodule..update` has the form of
-   '!command'.
+   superproject) is executed. When `submodule..update`
+   is set to '!command', the remainder after the exclamation mark
+   is the custom command.
 
-When no option is given and `submodule..update` is set to 'none',
-the submodule is not updated.
+   none;; the submodule is not updated.
 
 If the submodule is not yet initialized, and you just want to use the
 setting as stored in .gitmodules, you can automatically initialize the
-- 
2.11.0.196.gee862f456e.dirty

[PATCH 0/3] Update submodule documentation

[Stefan Beller]

I think I sent out the first 2 patches once upon a time, but the review
or me resending them stalled. They improve the man page for the git-submodule
command.

The third patch is RFC-ish and adds a background story to submodules
Similar to gitattributes, gitnamespaces, gittutorial it doesn't describe
a particular command but e.g. explains design.

Thanks,
Stefan

Stefan Beller (3):
  submodule documentation: add options to the subcommand
  submodule update documentation: don't repeat ourselves
  submodules: add a background story

 Documentation/git-submodule.txt |  93 +++--
 Documentation/gitsubmodules.txt | 176 
 2 files changed, 206 insertions(+), 63 deletions(-)
 create mode 100644 Documentation/gitsubmodules.txt

-- 
2.11.0.196.gee862f456e.dirty

[PATCH 3/3] submodules: add a background story

[Stefan Beller]

Just like gitattributes, gitcredentials, gitnamespaces, gittutorial,
gitmodules, we'd like to provide some background on submodules,
which is not specific to the `submodule` command, but elaborates
on the background and its intended usage.

Signed-off-by: Stefan Beller 
---
 Documentation/git-submodule.txt |  36 ++--
 Documentation/gitsubmodules.txt | 176 
 2 files changed, 181 insertions(+), 31 deletions(-)
 create mode 100644 Documentation/gitsubmodules.txt

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 7e03ccc444..c99920aceb 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -23,37 +23,7 @@ DESCRIPTION
 ---
 Inspects, updates and manages submodules.
 
-A submodule allows you to keep another Git repository in a subdirectory
-of your repository. The other repository has its own history, which does not
-interfere with the history of the current repository. This can be used to
-have external dependencies such as third party libraries for example.
-
-When cloning or pulling a repository containing submodules however,
-these will not be checked out by default; the 'init' and 'update'
-subcommands will maintain submodules checked out and at
-appropriate revision in your working tree.
-
-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 that is completely separate.
-A record in the `.gitmodules` (see linkgit:gitmodules[5]) file at the
-root of the source tree assigns a logical name to the submodule and
-describes the default URL the submodule shall be cloned from.
-The logical name can be used for overriding this URL within your
-local repository configuration (see 'submodule init').
-
-Submodules are not to be confused with remotes, which are other
-repositories of the same project; submodules are meant for
-different projects you would like to make part of your source tree,
-while the history of the two projects still stays completely
-independent and you cannot modify the contents of the submodule
-from within the main project.
-If you want to merge the project histories and want to treat the
-aggregated whole as a single project from then on, you may want to
-add a remote for the other project and use the 'subtree' merge strategy,
-instead of treating the other project as a submodule. Directories
-that come from both projects can be cloned and checked out as a whole
-if you choose to go that route.
+For more information about submodules, see linkgit:gitsubmodules[5]
 
 COMMANDS
 
@@ -405,6 +375,10 @@ This file should be formatted in the same way as 
`$GIT_DIR/config`. The key
 to each submodule url is "submodule.$name.url".  See linkgit:gitmodules[5]
 for details.
 
+SEE ALSO
+
+linkgit:gitsubmodules[1], linkgit:gitmodules[1].
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/Documentation/gitsubmodules.txt b/Documentation/gitsubmodules.txt
new file mode 100644
index 00..aa5fa25608
--- /dev/null
+++ b/Documentation/gitsubmodules.txt
@@ -0,0 +1,176 @@
+gitsubmodules(5)
+
+
+NAME
+
+gitsubmodules - information about submodules
+
+SYNOPSIS
+
+$GIT_DIR/config, .gitmodules
+
+--
+git submodule
+--
+
+DESCRIPTION
+---
+
+A submodule allows you to keep another Git repository in a subdirectory
+of your repository. The other repository has its own history, which does not
+interfere with the history of the current repository. This can be used to
+have external dependencies such as third party libraries for example.
+
+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 that is completely separate.
+A record in the `.gitmodules` (see linkgit:gitmodules[5]) file at the
+root of the source tree assigns a logical name to the submodule and
+describes the default URL the submodule shall be cloned from.
+The logical name can be used for overriding this URL within your
+local repository configuration (see 'submodule init').
+
+Submodules are not to be confused with remotes, which are other
+repositories of the same project; submodules are meant for
+different projects you would like to make part of your source tree,
+while the history of the two projects still stays completely
+independent and you cannot modify the contents of the submodule
+from within the main project.
+If you want to merge the project histories and want to treat the
+aggregated whole as a single project from then on, you may want to
+add a remote for the other project and use the 'subtree' merge strategy,
+instead of treating the other project as a submodule. Directories
+that come from both projects can be cloned and checked out as a whole
+if you choose to go that route.
+
+When cloning or pulling a 

[PATCH 1/3] submodule documentation: add options to the subcommand

[Stefan Beller]

When reading up on a subcommand of `git submodule `,
it is convenient to have its options nearby and not just at the
top of the man page.  Add the options to each subcommand.

While at it, also document the `--checkout` option for `update`.

Signed-off-by: Stefan Beller 
---
 Documentation/git-submodule.txt | 27 +++
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index d841573475..f9da0d9e91 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -9,17 +9,12 @@ git-submodule - Initialize, update or inspect submodules
 SYNOPSIS
 
 [verse]
-'git submodule' [--quiet] add [-b ] [-f|--force] [--name ]
- [--reference ] [--depth ] [--]  
[]
+'git submodule' [--quiet] add [] [--]  []
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
 'git submodule' [--quiet] init [--] [...]
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...)
-'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
- [--[no-]recommend-shallow] [-f|--force] [--rebase|--merge]
- [--reference ] [--depth ] [--recursive]
- [--jobs ] [--] [...]
-'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
- [commit] [--] [...]
+'git submodule' [--quiet] update [] [--] [...]
+'git submodule' [--quiet] summary [] [--] [...]
 'git submodule' [--quiet] foreach [--recursive] 
 'git submodule' [--quiet] sync [--recursive] [--] [...]
 
@@ -62,7 +57,7 @@ if you choose to go that route.
 
 COMMANDS
 
-add::
+add [-b ] [-f|--force] [--name ] [--reference ] 
[--depth ] [--]  []::
Add the given repository as a submodule at the given path
to the changeset to be committed next to the current
project: the current project is termed the "superproject".
@@ -103,7 +98,7 @@ together in the same relative location, and only the
 superproject's URL needs to be provided: git-submodule will correctly
 locate the submodule using the relative URL in .gitmodules.
 
-status::
+status [--cached] [--recursive] [--] [...]::
Show the status of the submodules. This will print the SHA-1 of the
currently checked out commit for each submodule, along with the
submodule path and the output of 'git describe' for the
@@ -120,7 +115,7 @@ submodules with respect to the commit recorded in the index 
or the HEAD,
 linkgit:git-status[1] and linkgit:git-diff[1] will provide that information
 too (and can also report changes to a submodule's work tree).
 
-init::
+init [--] [...]::
Initialize the submodules recorded in the index (which were
added and committed elsewhere) by copying submodule
names and urls from .gitmodules to .git/config.
@@ -135,7 +130,7 @@ init::
the explicit 'init' step if you do not intend to customize
any submodule locations.
 
-deinit::
+deinit [-f|--force] (--all|[--] ...)::
Unregister the given submodules, i.e. remove the whole
`submodule.$name` section from .git/config together with their work
tree. Further calls to `git submodule update`, `git submodule foreach`
@@ -151,7 +146,7 @@ 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.
 
-update::
+update [--init] [--remote] [-N|--no-fetch] [--[no-]recommend-shallow] 
[-f|--force] [--checkout|--rebase|--merge] [--reference ] [--depth 
] [--recursive] [--jobs ] [--] [...]::
 +
 --
 Update the registered submodules to match what the superproject
@@ -197,7 +192,7 @@ submodule with the `--init` option.
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and update any nested submodules within.
 --
-summary::
+summary [--cached|--files] [(-n|--summary-limit) ] [commit] [--] 
[...]::
Show commit summary between the given commit (defaults to HEAD) and
working tree/index. For a submodule in question, a series of commits
in the submodule between the given super project commit and the
@@ -210,7 +205,7 @@ summary::
 Using the `--submodule=log` option with linkgit:git-diff[1] will provide that
 information too.
 
-foreach::
+foreach [--recursive] ::
Evaluates an arbitrary shell command in each checked out submodule.
The command has access to the variables $name, $path, $sha1 and
$toplevel:
@@ -231,7 +226,7 @@ As an example, +git submodule foreach \'echo $path 
{backtick}git
 rev-parse HEAD{backtick}'+ will show the path and currently checked out
 commit for each submodule.
 
-sync::
+sync [--recursive] [--] [...]::
Synchronizes submodules' remote URL configuration setting
to the value specified in .gitmodules. It will only affect those
submodules which already have a URL entry in .git/config (that is the
-- 

What's cooking in git.git (Dec 2016, #08; Tue, 27)

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

This will be the last issue of the "What's cooking" report for the
year.  I will be travelling and expect to be slow to respond for a
week or so.  Hence there is no "short-term" summary; the topics
marked as "Will merge to 'master'" may stay in 'next' until the
second week of next month.

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

* bw/transport-protocol-policy (2016-12-15) 6 commits
  (merged to 'next' on 2016-12-19 at 166168205c)
 + http: respect protocol.*.allow=user for http-alternates
 + transport: add from_user parameter to is_transport_allowed
 + http: create function to get curl allowed protocols
 + transport: add protocol policy config option
 + http: always warn if libcurl version is too old
 + lib-proto-disable: variable name fix

 Finer-grained control of what protocols are allowed for transports
 during clone/fetch/push have been enabled via a new configuration
 mechanism.


* cp/merge-continue (2016-12-15) 4 commits
  (merged to 'next' on 2016-12-19 at 8ba0094f45)
 + merge: mark usage error strings for translation
 + merge: ensure '--abort' option takes no arguments
 + completion: add --continue option for merge
 + merge: add '--continue' option as a synonym for 'git commit'

 "git merge --continue" has been added as a synonym to "git commit"
 to conclude a merge that has stopped due to conflicts.


* gv/p4-multi-path-commit-fix (2016-12-19) 1 commit
  (merged to 'next' on 2016-12-21 at f7ba714387)
 + git-p4: fix multi-path changelist empty commits

 "git p4" that tracks multile p4 paths imported a single changelist
 that touches files in these multiple paths as one commit, followed
 by many empty commits.  This has been fixed.


* jc/lock-report-on-error (2016-12-27) 1 commit
  (merged to 'next' on 2016-12-27 at ab2ae230f4)
 + lockfile: move REPORT_ON_ERROR bit elsewhere

 Hotfix for a topic already merged to 'master'.


* jc/push-default-explicit (2016-10-31) 2 commits
  (merged to 'next' on 2016-12-05 at d63f3777af)
 + push: test pushing ambiguously named branches
 + push: do not use potentially ambiguous default refspec

 Originally merged to 'next' on 2016-11-01

 A lazy "git push" without refspec did not internally use a fully
 specified refspec to perform 'current', 'simple', or 'upstream'
 push, causing unnecessary "ambiguous ref" errors.


* jk/difftool-in-subdir (2016-12-11) 4 commits
  (merged to 'next' on 2016-12-21 at c1056e014b)
 + difftool: rename variables for consistency
 + difftool: chdir as early as possible
 + difftool: sanitize $workdir as early as possible
 + difftool: fix dir-diff index creation when in a subdirectory

 Even though an fix was attempted in Git 2.9.3 days, but running
 "git difftool --dir-diff" from a subdirectory never worked. This
 has been fixed.


* js/mingw-isatty (2016-12-22) 3 commits
  (merged to 'next' on 2016-12-22 at 5be6c1a083)
 + mingw: replace isatty() hack
 + mingw: fix colourization on Cygwin pseudo terminals
 + mingw: adjust is_console() to work with stdin

 Update the isatty() emulation for Windows by updating the previous
 hack that depended on internals of (older) MSVC runtime.


* ld/p4-compare-dir-vs-symlink (2016-12-18) 1 commit
  (merged to 'next' on 2016-12-20 at f58fed9ef8)
 + git-p4: avoid crash adding symlinked directory

 "git p4" misbehaved when swapping a directory and a symbolic link.


* ls/filter-process (2016-12-18) 2 commits
  (merged to 'next' on 2016-12-19 at 5ed29656db)
 + t0021: fix flaky test
  (merged to 'next' on 2016-12-12 at 8ed1f9eb02)
 + docs: warn about possible '=' in clean/smudge filter process values

 Doc update.


* ls/p4-lfs (2016-12-20) 1 commit
  (merged to 'next' on 2016-12-22 at 0759f94c65)
 + git-p4: add diff/merge properties to .gitattributes for GitLFS files

 Update GitLFS integration with "git p4".


* lt/shortlog-by-committer (2016-12-20) 3 commits
  (merged to 'next' on 2016-12-21 at c72e6e7f76)
 + t4201: make tests work with and without the MINGW prerequiste
  (merged to 'next' on 2016-12-19 at 555976fc0a)
 + shortlog: test and document --committer option
 + shortlog: group by committer information

 "git shortlog" learned "--committer" option to group commits by
 committer, instead of author.


* mk/mingw-winansi-ttyname-termination-fix (2016-12-20) 1 commit
  (merged to 'next' on 2016-12-21 at 1e8e994605)
 + mingw: consider that UNICODE_STRING::Length counts bytes

 A potential but unlikely buffer overflow in Windows port has been
 fixed.


* sb/submodule-config-cleanup (2016-11-22) 3 commits
  (merged to 'next' on 2016-12-05 at 

Re: [PATCH] worktree: initialize return value for submodule_uses_worktrees

[Junio C Hamano]

Stefan Beller  writes:

> On Tue, Dec 27, 2016 at 2:12 PM, Junio C Hamano  wrote:
>
>> I'm planning to disappear until early next
>> year
>
> Safe travels!

Thanks.

> I assume there is no interim maintainer for such a short
> period of time (which also is not as busy).

I plan to take my Flip with me (the primary development box for me
these days is Google's datacentre, and I only need a chromebook to
use as a SSH terminal or two) and expect to have a usable network
connection, but given the number of topics labelled as "Waiting for
review to conclude", I expect that people have plenty to chew while
I am away and the public git.git tree does not replace the in-flight
topics for a week or so ;-)

Re: [PATCH] worktree: initialize return value for submodule_uses_worktrees

[Stefan Beller]

On Tue, Dec 27, 2016 at 2:12 PM, Junio C Hamano  wrote:

>
> or even make it a helper function "is_empty_directory(const char *)".

This sounds like the way to go IMHO.

> I'm planning to disappear until early next
> year

Safe travels!
I assume there is no interim maintainer for such a short
period of time (which also is not as busy).

Thanks,
Stefan

Re: git-apply: warn/fail on *changed* end of line (eol) *only*?

[Igor Djordjevic BugA]

On 27/12/2016 18:27, Junio C Hamano wrote:

> To see the problem with "check existing lines", it probably is
> easier to extend the above example to start from a file with one
> more line, like this:
>
> 1 
> 3 
> 4 
> 5 
>
> and extend all the example patches to remove "4 " line as well,
> where they remove "3 ", making the first example patch like
> so:
>
>  1 
> -3 
> -4 
> +three 
>  5 
>
> Now, if you take "three " to be replacing "3 ", then you
> may feel that not warning on the CRLF would be the right thing, but
> there is no reason (other than the fact you, a human, understand
> what 'three' means) to choose "3 " over "4 " as the
> original.  If you take "three " to be replacing "4 ", you
> would need to warn.

Hmm, but why do you keep insisting on knowing what the lines are about,
and still making a 'per line' end of line comparison? Besides, your
example falls within the (only?) special case I mentioned as the one Git
probably shouldn`t be acting upon, as both  and  are present
among the the old hunk lines already (lines starting with '-').

The logic should be simple, what I tried to describe in the previous
message:

  1. Examine all '-' lines line endings.

  1.1. If not all line endings are the same (like in your example, no
   matter the line content), do nothing.

  1.2. If all line endings _are_ the same within the old hunk ('-'
   lines), check if any of the '+' lines (new hunk) has a different
   line ending (still no matter the line content).

  1.2.1. If no different line endings among '+' lines in comparison to
 unique line ending found in '-' lines, do nothing.

  1.2.2. If _any_ of the '+' lines _has_ a different line ending in
 comparison to unique line ending found in '-' lines, then do
 warn/fail.

This even might seem as the most sensible approach, no matter the
overall project end of line setting, which is exactly why I find it
interesting - it seems to 'just work', being beginner friendly _and_
also watching your back on unexpected end of line changes.

> A totally uninteresting special case is when the original is all
> , but in that case, as you already said in the original
> message, the project wants  and you can configure it as such.
> Then  in the line-end  won't be mistaken as if it is a
> whitespace character  at the end of a line terminated with .

But this is exactly the most confusing case, especially for beginners,
where project configuration is incorrect, and you get warned about
'whitespace errors' where all line endings (old/new) are in fact still
the same (confusing).

Yet worse, you don`t get warned of different line endings being applied,
as these are considered 'correct' due to current (incorrect) setting, no
matter the whole file is actually different, which you don`t get warned
about in the first place, and you may discover the file line ending
breakage only when other contributors start complaining.

Also, would it be sensible to account for a possible file inside the
project having different line endings than project in general...? This
would help there as well, without unintentionally breaking the file.

Regards, BugA

P.S. I guess you don`t need me to tell you that, but please feel free
to drop out of this discussion at your own discretion, even though I
enjoy sharing thoughts (and learning new stuff!), I do kind of feel I`m
wasting your time for something that might not be that important, if at
all, otherwise someone would have probably addressed it so far :/

Re: [PATCH] worktree: initialize return value for submodule_uses_worktrees

[Junio C Hamano]

Stefan Beller  writes:

> When the worktrees directory is empty, the `ret` will be returned
> uninitialized. Fix it by initializing the value.
>
> Signed-off-by: Stefan Beller 
> ---
>
> This goes on top of 1a248cf (origin/sb/submodule-embed-gitdir);
> ideally to be squashed, but as it is in next already, as a separate
> patch.
>
> Thanks,
> Stefan

If you initialize it at the definition site, it would be more
consistent if these "return 0" we see earlier parts of the function
also returned "ret" instead of "0".  A better alternative would be
to initialize it to 0 before it starts to matter, i.e. immediately
before the

while (readdir()) {
if (is_dot_or_dotdot())
continue;
ret = 1;
break;
}   

loop.  I also wonder if that loop is easier to read 

for (has_paths = 0; !has_paths && (d = readdir(dir)) != NULL; ) {
if (is_dot_or_dotdot())
continue;
has_paths = 1;
}

or even make it a helper function "is_empty_directory(const char *)".

Having said that, I'll queue this as-is and will merge to 'master'
by the end of the day, as I'm planning to disappear until early next
year, so please do not "reroll" this to add yet another integration
cycle to my day.

Thanks.


>  worktree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/worktree.c b/worktree.c
> index d4606aa8cd..828fd7a0ad 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -387,7 +387,7 @@ int submodule_uses_worktrees(const char *path)
>   struct strbuf sb = STRBUF_INIT;
>   DIR *dir;
>   struct dirent *d;
> - int ret;
> + int ret = 0;
>   struct repository_format format;
>  
>   submodule_gitdir = git_pathdup_submodule(path, "%s", "");

Re: [PATCHv5 4/4] rm: absorb a submodules git dir before deletion

[Junio C Hamano]

Stefan Beller  writes:

 @@ -358,9 +331,6 @@ int cmd_rm(int argc, const char **argv, const char 
 *prefix)
   oidclr();
   if (check_local_mod(, index_only))
   exit(1);
 - } else if (!index_only) {
 - if (check_submodules_use_gitfiles())
 - exit(1);
   }

>>>
>>> Hmph.  It may be a bit strange to see an "index-only" remove to
>>> touch working tree, no?  Yet submodules_absorb_gitdir_if_needed() is
>>> unconditionally called above, which feels somewhat unexpected.
>> ...
> Well scratch that.
> is_staging_gitmodules_ok only checks for the .gitmodules file and not
> for the submodule itself (the submodule is not an argument to that function)
>
> The actual answer is found in check_local_mod called via
>
> if (!force) {
> struct object_id oid;
> if (get_oid("HEAD", ))
> oidclr();
> if (check_local_mod(, index_only))
> exit(1);
> }

OK, that happens way before this loop starts finding submodules and
removing them, so we can say that the latter is well protected.

Thanks.

Re: [PATCH v9 15/20] ref-filter: modify the 'lstrip=' option to work with negative ''

[Junio C Hamano]

Karthik Nayak  writes:

> Currently the 'lstrip=' option only takes a positive value ''
> and strips '' slash-separated path components from the left. Modify
> the 'lstrip' option to also take a negative number '' which would
> only _leave_ behind 'N' slash-separated path components from the left.

"would only leave behind N components from the left" sounds as if
the result is A/B, when you are given A/B/C/D/E and asked to
lstrip:-2.  Given these two tests added by the patch ...

> +test_atom head refname:lstrip=-1 master
> +test_atom head refname:lstrip=-2 heads/master

... I somehow think that is not what you wanted to say.  Instead,
you strip from the left as many as necessary and leave -N
components that appear at the right-most end, no?

> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -98,7 +98,8 @@ refname::
>   abbreviation mode. If `lstrip=` is appended, strips ``
>   slash-separated path components from the front of the refname
>   (e.g., `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo`.
> - `` must be a positive integer.
> + if `` is a negative number, then only `` path components
> + are left behind.

I think positive  is so obvious not to require an example but it
is good that you have one.  The negative  case needs illustration
more than the positive case.  Perhaps something like:

(e.g. %(refname:lstrip=-1) strips components of refs/tags/frotz 
from the left to leave only one component, i.e. 'frotz').

Would %(refname:lstrip=-4) attempt to strip components of
refs/tags/frotz from the left to leave only four components, and
because the original does not have that many components, it ends
with refs/tags/frotz?

I am debating myself if we need something like "When the ref does
not have enough components, the result becomes an empty string if
stripping with positive , or it becomes the full refname if
stripping with negative .  Neither is an error." is necessary
here.  Or is it too obvious?

Re: [PATCH v9 11/20] ref-filter: introduce refname_atom_parser()

[Junio C Hamano]

Karthik Nayak  writes:

> +symref::
> + The ref which the given symbolic ref refers to. If not a
> + symbolic ref, nothing is printed. Respects the `:short` and
> + `:strip` options in the same way as `refname` above.
> +

I am slightly unhappy with this name.  If we had an atom that lets
you ask "Is this a symref?" and yields "" or "->", it could also be
called symref, and we would name it "is_symref" or something to
disambiguate it.  Then it is only fair to give this one that lets
you ask "What does this symref point at?" a bit more specific name,
like "symref_target" or something.

But probably I am worried too much.  "is_symref", if necessary, can
be written as "%(if:notequals=)%(symref)%(then)...%(else)...%(end)"
and it is not likely that it would be used often, so let's keep it
as-is.

Re: [PATCH v9 03/20] ref-filter: implement %(if:equals=) and %(if:notequals=)

[Junio C Hamano]

Karthik Nayak  writes:

> This is done by introducing 'if_atom_parser()' which parses the given
> %(if) atom and then stores the data in used_atom which is later passed
> on to the used_atom of the %(then) atom, so that it can do the required
> comparisons.
>
> Add tests and Documentation for the same.

s/Documentation/documentation/

Re: [PATCH v9 19/20] branch: use ref-filter printing APIs

[Junio C Hamano]

Karthik Nayak  writes:

>  static char branch_colors[][COLOR_MAXLEN] = {
> - GIT_COLOR_RESET,
> - GIT_COLOR_NORMAL,   /* PLAIN */
> - GIT_COLOR_RED,  /* REMOTE */
> - GIT_COLOR_NORMAL,   /* LOCAL */
> - GIT_COLOR_GREEN,/* CURRENT */
> - GIT_COLOR_BLUE, /* UPSTREAM */
> + "%(color:reset)",
> + "%(color:reset)",   /* PLAIN */
> + "%(color:red)", /* REMOTE */
> + "%(color:reset)",   /* LOCAL */
> + "%(color:green)",   /* CURRENT */
> + "%(color:blue)",/* UPSTREAM */
>  };

The contents of this table is no longer tied to COLOR_MAXLEN.  The
above entries used by default happen to be shorter, but it is
misleading.

static const char *branch_colors[] = {
"%(color:reset)",
...
};

perhaps?

More importantly, does this re-definition of the branch_colors[]
work with custom colors filled in git_branch_config() by calling
e.g. color_parse("red", branch_colors[BRANCH_COLOR_REMOTE]), where
"red" and "remote" come from an existing configuration file?

[color "branch"]
remote = red

It obviously would not work if you changed the type of branch_colors[]
because the color_parse() wants the caller to have allocated space
of COLOR_MAXLEN.  

But if filling these ANSI escape sequence into the format works OK,
then doesn't it tell us that we do not need to have this change to
use "%(color:reset)" etc. as the new default values?

Re: [PATCH v9 02/20] ref-filter: include reference to 'used_atom' within 'atom_value'

[Junio C Hamano]

Karthik Nayak  writes:

> From: Karthik Nayak 
>
> Ensure that each 'atom_value' has a reference to its corresponding
> 'used_atom'. This let's us use values within 'used_atom' in the

s/let's us use/lets us use/;

> 'handler' function.

Re: [PATCH v9 01/20] ref-filter: implement %(if), %(then), and %(else) atoms

[Junio C Hamano]

Karthik Nayak  writes:

>  
> +Some atoms like %(align) and %(if) always require a matching %(end).
> +We call them "opening atoms" and sometimes denote them as %($open).
> +
> +When a scripting language specific quoting is in effect, everything
> +between a top-level opening atom and its matching %(end) is evaluated
> +according to the semantics of the opening atom and its result is
> +quoted.

What is unsaid in the last paragraph is that you assume "is
evaluated according to the semantics of the opening atom" does not
involve quoting and only the result from the top-level is quoted.  I
am not sure if that is clear to the first-time readers.

>  
>  EXAMPLES
>  
> @@ -273,6 +291,22 @@ eval=`git for-each-ref --shell --format="$fmt" \
>  eval "$eval"
>  
> ...
> +
> +git for-each-ref --format="%(refname)%(if)%(authorname)%(then) 
> %(color:red)Authored by: %(authorname)%(end)"
> +

This makes readers wonder how "red"ness is reset, but that is not
something this example is interested in demonstrating.  Let's drop
the %(color:red) bit to avoid distracting readers.

[PATCH 2/2] t7411: test lookup of uninitialized submodules

[Stefan Beller]

Sometimes we need to lookup information of uninitialized submodules. Make
sure that works.

Signed-off-by: Stefan Beller 
---
 t/t7411-submodule-config.sh | 21 +
 1 file changed, 21 insertions(+)

diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 3646f28b40..b40df6a4c1 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -126,6 +126,27 @@ test_expect_success 'reading of local configuration' '
)
 '
 
+cat >super/expect_url super/expect_fetchrecurse_die.err <

[PATCH 1/2] t7411: quote URLs

[Stefan Beller]

The variables may contain white spaces, so we need to quote them.
By not quoting the variables we'd end up passing multiple arguments to
git config, which doesn't fail for two arguments as value.

Signed-off-by: Stefan Beller 
---
 t/t7411-submodule-config.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh
index 47562ce465..3646f28b40 100755
--- a/t/t7411-submodule-config.sh
+++ b/t/t7411-submodule-config.sh
@@ -120,8 +120,8 @@ test_expect_success 'reading of local configuration' '
"" submodule \
>actual &&
test_cmp expect_local_path actual &&
-   git config submodule.a.url $old_a &&
-   git config submodule.submodule.url $old_submodule &&
+   git config submodule.a.url "$old_a" &&
+   git config submodule.submodule.url "$old_submodule" &&
git config --unset submodule.a.path c
)
 '
-- 
2.11.0.rc2.50.g8bda6b2.dirty

[PATCH 0/2] submodule config test cleanup

[Stefan Beller]

A test cleanup and an additional test for the submodule configuration.

Stefan Beller (2):
  t7411: quote URLs
  t7411: test lookup of uninitialized submodules

 t/t7411-submodule-config.sh | 25 +++--
 1 file changed, 23 insertions(+), 2 deletions(-)

-- 
2.11.0.rc2.50.g8bda6b2.dirty

Re: Intermittent failure of t0021

[Michael Haggerty]

On 12/27/2016 08:21 PM, Michael Haggerty wrote:
> I'm seeing intermittent failures of t0021. [...]

Just after submitting this, I saw that the problem has already been
reported and a fix proposed:

http://public-inbox.org/git/dd8decbc-f856-4f68-6d77-7ea9d5f9d...@ramsayjones.plus.com/

Sorry for the noise.

Michael

Intermittent failure of t0021

[Michael Haggerty]

Hi,

I'm seeing intermittent failures of t0021. If I run it as follows (using
a ramdisk as temporary space, and `EXPENSIVE` turned off), it fails on
average about every 40 attempts:

$ make -j8 O=0 && (cd t && for i in $(seq 200); do if ./t0021-*.sh ;
then echo "passed attempt $i"; else echo "failed on attempt $i"; break;
fi ; done )

(I've also seen it fail while running the whole test suite, so it is not
the tight test loop that triggers the problem.)

The output when it fails is

Initialized empty Git repository in
/home/mhagger/self/proj/git/git/t/trash
directory.t0021-conversion/repo/.git/
[master (root-commit) 56d459b] test commit 1
 Author: A U Thor 
 1 file changed, 1 insertion(+)
 create mode 100644 .gitattributes
[master c30c7fc] test commit 2
 Author: A U Thor 
 4 files changed, 5 insertions(+)
 create mode 100644 test.r
 create mode 100644 test2.r
 create mode 100644 test4-empty.r
 create mode 100644 testsubdir/test3 'sq',$x.r
sort: cannot read: rot13-filter.log: No such file or directory
--- expected.log2016-12-27 17:58:18.017505012 +
+++ rot13-filter.log2016-12-27 17:58:18.017505012 +
@@ -1,7 +0,0 @@
-x IN: clean test.r 57 [OK] -- OUT: 57 . [OK]
-x IN: clean test2.r 14 [OK] -- OUT: 14 . [OK]
-x IN: clean test4-empty.r 0 [OK] -- OUT: 0  [OK]
-x IN: clean testsubdir/test3 'sq',$x.r 49 [OK] -- OUT: 49 . [OK]
-  1 START
-  1 STOP
-  1 init handshake complete
not ok 15 - required process filter should filter data

The failure bisects to

9c48b4f "t0021: minor filter process test cleanup", 2016-12-04

If I test the two parts of that commit separately, it is the removal of
the `.` arguments from `git commit` that triggers the failures (i.e.,
not the removal of the creation of `.gitignore`).

It's possible that the test was unreliable or racy even before this
change, but I have run the same test 1000+ times against the parent
commit (b18f6a00662524443cfb82f5fed7d3b54524c8ab) under the same
conditions without seeing a single failure. So *something* seems to have
been changed for the worse by this commit.

I don't plan to look into it any further, but if you have trouble
reproducing the problem don't hesitate to contact me.

Michael

Re: [PATCH v3 21/21] Documentation/git-update-index: explain splitIndex.*

[Junio C Hamano]

Christian Couder  writes:

>  --split-index::
>  --no-split-index::
> - Enable or disable split index mode. If enabled, the index is
> - split into two files, $GIT_DIR/index and $GIT_DIR/sharedindex..
> - Changes are accumulated in $GIT_DIR/index while the shared
> - index file contains all index entries stays unchanged. If
> - split-index mode is already enabled and `--split-index` is
> - given again, all changes in $GIT_DIR/index are pushed back to
> - the shared index file. This mode is designed for very large
> - indexes that take a significant amount of time to read or write.
> + Enable or disable split index mode. If split-index mode is
> + already enabled and `--split-index` is given again, all
> + changes in $GIT_DIR/index are pushed back to the shared index
> + file.

In the world after this series that introduced the percentage-based
auto consolidation, it smells strange, or even illogical, that index
is un-split after doing this:

$ git update-index --split-index
$ git update-index --split-index

Before this series, it may have been a quick and dirty way to force
consolidating the split index without totally disabling the feature,
i.e. it would have looked more like

$ git update-index --split-index
... work work work to accumulate more modifications
... consolidate into one --- there was no other way than
... disabling it temporarily
$ git update-index --no-split-index
... but the user likes the feature so re-enable it.
$ git update-index --split-index

which I guess is where this strange behaviour comes from.

It may be something we need to fix to unconfuse the end-users after
this series lands.  Even though "first disable and then re-enable"
takes two commands (as opposed to one), it is far more logical.  And
the percentage-based auto consolidation feature is meant to reduce
the need for manual consolidation, so it probably makes sense to
remove this illogical feature.

> @@ -394,6 +390,31 @@ Although this bit looks similar to assume-unchanged bit, 
> its goal is
>  different from assume-unchanged bit's. Skip-worktree also takes
>  precedence over assume-unchanged bit when both are set.
>  
> +Split index
> +---
> +
> +This mode is designed for very large indexes that take a significant
> +amount of time to read or write.

This is not a new problem, but it probably is incorrect to say "to
read or write".  It saves time by not rewriting the whole thing but
instead write out only the updated bits.  You'd still read the whole
thing while populating the in-core index from the disk, and if
anything, you'd probably spend _more_ cycles because you'd essentially
be reading the base and then reading the delta to apply on top.

> +To avoid deleting a shared index file that is still used, its mtime is
> +updated to the current time everytime a new split index based on the
> +shared index file is either created or read from.


The same comment on the mention of "mtime" in another patch applies
here as well.

Re: [PATCH v3 20/21] Documentation/config: add splitIndex.sharedIndexExpire

[Junio C Hamano]

Christian Couder  writes:

> Signed-off-by: Christian Couder 
> ---
>  Documentation/config.txt | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index e0f5a77980..24e771d22e 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2786,6 +2786,17 @@ splitIndex.maxPercentChange::
>   than 20 percent of the total number of entries.
>   See linkgit:git-update-index[1].
>  
> +splitIndex.sharedIndexExpire::
> + When the split index feature is used, shared index files with
> + a mtime older than this time will be removed when a new shared

As end-user facing documentation, it would be much better if we can
rephrase it for those who do not know what a 'mtime' is, and it
would be even better if we can do so without losing precision.

I think "shared index files that were not modified since the time
this variable specifies will be removed" would be understandable and
correct enough?

> + index file is created. The value "now" expires all entries
> + immediately, and "never" suppresses expiration altogether.
> + The default value is "one.week.ago".
> + Note that each time a new split-index file is created, the
> + mtime of the related shared index file is updated to the
> + current time.

To match the above suggestion, "Note that a shared index file is
considered modified (for the purpose of expiration) each time a new
split-index file is created based on it."?

> + See linkgit:git-update-index[1].
> +
>  status.relativePaths::
>   By default, linkgit:git-status[1] shows paths relative to the
>   current directory. Setting this variable to `false` shows paths

Re: [PATCH v3 08/21] Documentation/git-update-index: talk about core.splitIndex config var

[Junio C Hamano]

Christian Couder  writes:

> Signed-off-by: Christian Couder 
> ---
>  Documentation/git-update-index.txt | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/git-update-index.txt 
> b/Documentation/git-update-index.txt
> index 7386c93162..e091b2a409 100644
> --- a/Documentation/git-update-index.txt
> +++ b/Documentation/git-update-index.txt
> @@ -171,6 +171,12 @@ may not support it yet.
>   given again, all changes in $GIT_DIR/index are pushed back to
>   the shared index file. This mode is designed for very large
>   indexes that take a significant amount of time to read or write.
> ++
> +These options take effect whatever the value of the `core.splitIndex`
> +configuration variable (see linkgit:git-config[1]).

Doesn't the "whatever..." clause in this sentence lack its verb
(presumably, "is", right after "variable")?

> +emitted when the change goes against the configured value, as the
> +configured value will take effect next time the index is read and this
> +will remove the intended effect of the option.

It feels somewhat strange to see a warning in this case. 

If the user configures the system to usually do X, and issues a
command that explicitly does Y (or "not-X"), we do not warn for the
command invocation if it is a single-shot operation.  That's the
usual "command line overrides configuration" an end-user expects.

I think you made this deviate from the usual end-user expectation
because it is unpredictable when the configuration kicks in the next
time to undo the effect of this command line request.  And I agree
that it is a very un-nice thing to do to the users if we did not
warn here, if we are to keep that unpredictableness.

But stepping back a bit, we know the user's intention is that with
"--split-index" the user wants to use the split index, even though
the user may have configuration not to use the split index.  Don't
we want to honor that intention?  In order to honor that intention
without getting confused by the configured variable to tell us not
to split, another way is to destroy that unpredictableness.  For
that, shouldn't we automatically remove the configuration that says
"do not split" (by perhaps set it to "do split")?  Doing so still
may need a warning that says "we disabled your previously configured
setting while at it", but it would be much better than a warning
message that essentially says "we do it once, but we will disobey
you at an unpredictable time", I would think.

Re: [PATCH v3 10/21] read-cache: regenerate shared index if necessary

[Junio C Hamano]

Christian Couder  writes:

> + case 0:
> + return 1; /* 0% means always write a new shared index */
> + case 100:
> + return 0; /* 100% means never write a new shared index */
> + default:
> + ; /* do nothing: just use the configured value */
> + }

Just like you did in 04/21, write "break" to avoid mistakes made in
the future, i.e.

default:
break; /* just use the configured value */

> +
> + /* Count not shared entries */
> + for (i = 0; i < istate->cache_nr; i++) {
> + struct cache_entry *ce = istate->cache[i];
> + if (!ce->index)
> + not_shared++;
> + }
> +
> + return istate->cache_nr * max_split < not_shared * 100;

On a 32-bit arch with 2G int and more than 20 million paths in the
index, multiplying by max_split that can come close to 100 can
theoretically cause integer overflow, but in practice it probably
does not matter.  Or does it?

Re: [PATCH v3 06/21] t1700: add tests for core.splitIndex

[Junio C Hamano]

Christian Couder  writes:

> +test_expect_success 'set core.splitIndex config variable to true' '
> + git config core.splitIndex true &&
> + : >three &&
> + git update-index --add three &&
> + git ls-files --stage >ls-files.actual &&
> + cat >ls-files.expect < +100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0one
> +100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0three
> +100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0two
> +EOF
> + test_cmp ls-files.expect ls-files.actual &&

It does not add much value to follow the "existing" outdated style
like this when you are only adding new tests.  Write these like

cat >ls-files.expect <<-\EOF &&
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0   one
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0   three
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0   two
EOF

which would give incentive to others (or yourself) to update the
style of the existing mess ;-).

Re: [PATCH v3 14/21] read-cache: touch shared index files when used

[Junio C Hamano]

Christian Couder  writes:

> +/*
> + * Signal that the shared index is used by updating its mtime.
> + *
> + * This way, shared index can be removed if they have not been used
> + * for some time. It's ok to fail to update the mtime if we are on a
> + * read only file system.
> + */
> +void freshen_shared_index(char *base_sha1_hex)
> +{
> + const char *shared_index = git_path("sharedindex.%s", base_sha1_hex);
> + check_and_freshen_file(shared_index, 1);

What happens when this call fails?  The function returns 0 if the
file did not even exist.  It also returns 0 if you cannot update its
timestamp.

Shouldn't the series be exposing freshen_file() instead _and_ taking
its error return value seriously?

> +}
> +
>  int read_index_from(struct index_state *istate, const char *path)
>  {
>   struct split_index *split_index;
> @@ -2273,6 +2286,8 @@ int write_locked_index(struct index_state *istate, 
> struct lock_file *lock,
>   int ret = write_shared_index(istate, lock, flags);
>   if (ret)
>   return ret;
> + } else {
> + freshen_shared_index(sha1_to_hex(si->base_sha1));
>   }
>  
>   return write_split_index(istate, lock, flags);

Re: [PATCH v3 13/21] sha1_file: make check_and_freshen_file() non static

[Junio C Hamano]

Christian Couder  writes:

> This function will be used in a commit soon, so let's make
> it available globally.

See comment on 14/21; I am not convinced that this is the function
you would want to borrow.

[PATCHv6] rm: absorb a submodules git dir before deletion

[Stefan Beller]

When deleting a submodule, we need to keep the actual git directory around,
such that we do not lose local changes in there and at a later checkout
of the submodule we don't need to clone it again.

Now that the functionality is available to absorb the git directory of a
submodule, rewrite the checking in git-rm to not complain, but rather
relocate the git directories inside the superproject.

An alternative solution was discussed to have a function
`depopulate_submodule`. That would couple the check for its git directory
and possible relocation before the the removal, such that it is less
likely to miss the check in the future.  But the indirection with such
a function added seemed also complex. The reason for that was that this
possible move of the git directory was also implemented in
`ok_to_remove_submodule`, such that this function could truthfully
answer whether it is ok to remove the submodule.

The solution proposed here defers all these checks to the caller.

Signed-off-by: Stefan Beller 
---

This replaces the last patch of the series 
"git-rm absorbs submodule git directory before deletion".
named "rm: absorb a submodules git dir before deletion".

The only change is a guard (!index_only) for
submodules_absorb_gitdir_if_needed(prefix);

Junio, the other concern you raised about not checking
for local modifications in the submodule has been answered
as a reply there; we do check for local modifications there,
though it is not obvious.

Thanks,
Stefan

 builtin/rm.c  | 80 ++-
 t/t3600-rm.sh | 39 +++--
 2 files changed, 34 insertions(+), 85 deletions(-)

diff --git a/builtin/rm.c b/builtin/rm.c
index 5a5a66272b..20635dca94 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -59,27 +59,9 @@ static void print_error_files(struct string_list *files_list,
}
 }
 
-static void error_removing_concrete_submodules(struct string_list *files, int 
*errs)
-{
-   print_error_files(files,
- Q_("the following submodule (or one of its nested "
-"submodules)\n"
-"uses a .git directory:",
-"the following submodules (or one of their nested "
-"submodules)\n"
-"use a .git directory:", files->nr),
- _("\n(use 'rm -rf' if you really want to remove "
-   "it including all of its history)"),
- errs);
-   string_list_clear(files, 0);
-}
-
-static int check_submodules_use_gitfiles(void)
+static void submodules_absorb_gitdir_if_needed(const char *prefix)
 {
int i;
-   int errs = 0;
-   struct string_list files = STRING_LIST_INIT_NODUP;
-
for (i = 0; i < list.nr; i++) {
const char *name = list.entry[i].name;
int pos;
@@ -99,12 +81,9 @@ static int check_submodules_use_gitfiles(void)
continue;
 
if (!submodule_uses_gitfile(name))
-   string_list_append(, name);
+   absorb_git_dir_into_superproject(prefix, name,
+   ABSORB_GITDIR_RECURSE_SUBMODULES);
}
-
-   error_removing_concrete_submodules(, );
-
-   return errs;
 }
 
 static int check_local_mod(struct object_id *head, int index_only)
@@ -120,7 +99,6 @@ static int check_local_mod(struct object_id *head, int 
index_only)
int errs = 0;
struct string_list files_staged = STRING_LIST_INIT_NODUP;
struct string_list files_cached = STRING_LIST_INIT_NODUP;
-   struct string_list files_submodule = STRING_LIST_INIT_NODUP;
struct string_list files_local = STRING_LIST_INIT_NODUP;
 
no_head = is_null_oid(head);
@@ -219,13 +197,8 @@ static int check_local_mod(struct object_id *head, int 
index_only)
else if (!index_only) {
if (staged_changes)
string_list_append(_cached, name);
-   if (local_changes) {
-   if (S_ISGITLINK(ce->ce_mode) &&
-   !submodule_uses_gitfile(name))
-   string_list_append(_submodule, 
name);
-   else
-   string_list_append(_local, name);
-   }
+   if (local_changes)
+   string_list_append(_local, name);
}
}
print_error_files(_staged,
@@ -247,8 +220,6 @@ static int check_local_mod(struct object_id *head, int 
index_only)
  );
string_list_clear(_cached, 0);
 
-   error_removing_concrete_submodules(_submodule, );
-
print_error_files(_local,
  Q_("the following file has local modifications:",

Re: [PATCH v3 12/21] Documentation/config: add splitIndex.maxPercentChange

[Junio C Hamano]

Christian Couder  writes:

> Signed-off-by: Christian Couder 
> ---
>  Documentation/config.txt | 13 +
>  1 file changed, 13 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 221c5982c0..e0f5a77980 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2773,6 +2773,19 @@ showbranch.default::
>   The default set of branches for linkgit:git-show-branch[1].
>   See linkgit:git-show-branch[1].
>  
> +splitIndex.maxPercentChange::
> + When the split index feature is used, this specifies the
> + percent of entries the split index can contain compared to the
> + whole number of entries in both the split index and the shared

s/whole/total/ to match the last sentence of this section, perhaps?
"The number of all entries" would also be OK, but "the whole number
of entries" sounds a bit strange.

Re: Gitview Shell Injection Vulnerability

[Stefan Beller]

+cc the author of gitview

On Tue, Dec 27, 2016 at 12:29 AM, Javantea  wrote:
> I have found a shell injection vulnerability in contrib/gitview/gitview.
>
> Gitview Shell Injection Vulnerability
>
> Versions affected: 8cb711c8a5-1d1bdafd64 (<=2.11.0)
>
> Gitview executes shell commands using string concatenation with user supplied 
> data, filenames and branch names. Running Gitview and interacting with the 
> user interface with a malicious filename or branch name in the current 
> repository results in malicious commands being executed as the current user.
>
> AnnotateWindow.add_file_data(self, filename, commit_sha1, line_num):
> fp = os.popen("git cat-file blob " + commit_sha1 +":"+filename)
>
> AnnotateWindow.annotate(self, filename, commit_sha1, line_num):
> fp = os.popen("git ls-tree "+ commit_sha1 + " -- " + filename)
> fp = os.popen("git blame --incremental -C -C -- " + filename + " " + 
> commit_sha1)
>
> GitView.set_branch(self, args):
> fp = os.popen("git rev-parse --sq --default HEAD " + list_to_string(args, 
> 1))
> fp = os.popen("git rev-list  --header --topo-order --parents " + 
> git_rev_list_cmd)
>
> The program also has other uses of os.popen but none use values that the user 
> can manipulate. However, the fix should definitely replace these instances so 
> that the code might one day pass pylint and manual code review easier.
>
> The function os.popen has been replaced by safer functions in the subprocess 
> module. The code can be improved easily because it requires very little 
> change to convert the code to work with arrays of strings instead of strings.
>
> If you have any questions or would like a patch, please let me know.
>

I guess you could send a patch to fix it. It is unclear to me
how the patch submission process for these work, though.
Please see contrib/README to see why it is unclear to me.

> I expect that things that start their life in the contrib/ area
> to graduate out of contrib/ once they mature, either by becoming
> projects on their own, or moving to the toplevel directory.  On
> the other hand, I expect I'll be proposing removal of disused
> and inactive ones from time to time.

Maybe it's time for a spring cleanup and remove some old (dead?)
projects from contrib?

Thanks,
Stefan

Re: [PATCHv5 4/4] rm: absorb a submodules git dir before deletion

[Stefan Beller]

On Tue, Dec 27, 2016 at 10:17 AM, Stefan Beller  wrote:
> On Mon, Dec 26, 2016 at 5:10 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> @@ -342,6 +313,8 @@ int cmd_rm(int argc, const char **argv, const char 
>>> *prefix)
>>>   exit(0);
>>>   }
>>>
>>> + submodules_absorb_gitdir_if_needed(prefix);
>>> +
>>>   /*
>>>* If not forced, the file, the index and the HEAD (if exists)
>>>* must match; but the file can already been removed, since
>>> @@ -358,9 +331,6 @@ int cmd_rm(int argc, const char **argv, const char 
>>> *prefix)
>>>   oidclr();
>>>   if (check_local_mod(, index_only))
>>>   exit(1);
>>> - } else if (!index_only) {
>>> - if (check_submodules_use_gitfiles())
>>> - exit(1);
>>>   }
>>>
>>
>> Hmph.  It may be a bit strange to see an "index-only" remove to
>> touch working tree, no?  Yet submodules_absorb_gitdir_if_needed() is
>> unconditionally called above, which feels somewhat unexpected.
>
> I agree. In a reroll I'll protect the call with
>
> if (!index_only)
> submodules_absorb_gitdir_if_needed(prefix);
>
>>
>>> @@ -389,32 +359,20 @@ int cmd_rm(int argc, const char **argv, const char 
>>> *prefix)
>>>*/
>>>   if (!index_only) {
>>>   int removed = 0, gitmodules_modified = 0;
>>>   for (i = 0; i < list.nr; i++) {
>>>   const char *path = list.entry[i].name;
>>>   if (list.entry[i].is_submodule) {
>>> + struct strbuf buf = STRBUF_INIT;
>>> +
>>> + strbuf_addstr(, path);
>>> + if (remove_dir_recursively(, 0))
>>> + die(_("could not remove '%s'"), path);
>>> + strbuf_release();
>>> +
>>> + removed = 1;
>>> + if (!remove_path_from_gitmodules(path))
>>> + gitmodules_modified = 1;
>>> + continue;
>>>   }
>>
>> I do not see any behaviour change from the original (not quoted
>> here), but it is somewhat surprising that "git rm ./submodule" does
>> not really check if the submodule has local modifications and files
>> that are not even added before remove_dir_recursively() is called.
>>
>> Or perhaps I am reading the code incorrectly and such a check is
>> done elsewhere?
>
> When determining if the entry is a submodule (i.e. setting
> the is_submodule bit) we have
>
> list.entry[list.nr].is_submodule = S_ISGITLINK(ce->ce_mode);
> if (list.entry[list.nr++].is_submodule &&
>   !is_staging_gitmodules_ok())
> die (_("Please stage ..."));
>

Well scratch that.
is_staging_gitmodules_ok only checks for the .gitmodules file and not
for the submodule itself (the submodule is not an argument to that function)

The actual answer is found in check_local_mod called via

if (!force) {
struct object_id oid;
if (get_oid("HEAD", ))
oidclr();
if (check_local_mod(, index_only))
exit(1);
}

check_local_mod was touched in the previous patch as it has:

/*
* Is the index different from the file in the work tree?
* If it's a submodule, is its work tree modified?
*/
if (ce_match_stat(ce, , 0) ||
  (S_ISGITLINK(ce->ce_mode) &&
   bad_to_remove_submodule(ce->name,
SUBMODULE_REMOVAL_DIE_ON_ERROR |
SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED)))
local_changes = 1;

Re: What's cooking in git.git (Dec 2016, #05; Mon, 19)

[Michael Haggerty]

On 12/27/2016 06:16 PM, Junio C Hamano wrote:
> Michael Haggerty  writes:
> 
>> Sorry I didn't notice this earlier, but the `LOCK_REPORT_ON_ERROR`
>> constant introduced by
>>
>> 3f061bf "lockfile: LOCK_REPORT_ON_ERROR", 2016-12-07
>>
>> sets that constant to the value 2,...
> 
> Sorry I didn't notice this earlier, either.  Thanks for spotting.
> 
> -- >8 --
> From: Junio C Hamano 
> Date: Tue, 27 Dec 2016 09:12:09 -0800
> Subject: [PATCH] lockfile: move REPORT_ON_ERROR bit elsewhere
> 
> There was LOCK_NO_DEREF defined as 2 = 1<<1 with the same value,
> which was missed due to a huge comment block.  Deconflict by moving
> the new one to 4 = 1<<2 for now.

The fix is obviously correct. I'm not sure I like it that comments are
being blamed for mistakes :-P

Perhaps defining these constants within an `enum` would make it clearer
that they are a group and help prevent problems like this in the future
(though that could be done later rather than as part of this hot fix).

Michael

> Signed-off-by: Junio C Hamano 
> ---
>  lockfile.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lockfile.h b/lockfile.h
> index 16775a7d79..7b715f9e77 100644
> --- a/lockfile.h
> +++ b/lockfile.h
> @@ -137,7 +137,7 @@ struct lock_file {
>   * ... this flag can be passed instead to return -1 and give the usual
>   * error message upon an error.
>   */
> -#define LOCK_REPORT_ON_ERROR 2
> +#define LOCK_REPORT_ON_ERROR 4
>  
>  /*
>   * Usually symbolic links in the destination path are resolved. This
> 

Re: [PATCHv5 4/4] rm: absorb a submodules git dir before deletion

[Stefan Beller]

On Mon, Dec 26, 2016 at 5:10 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> @@ -342,6 +313,8 @@ int cmd_rm(int argc, const char **argv, const char 
>> *prefix)
>>   exit(0);
>>   }
>>
>> + submodules_absorb_gitdir_if_needed(prefix);
>> +
>>   /*
>>* If not forced, the file, the index and the HEAD (if exists)
>>* must match; but the file can already been removed, since
>> @@ -358,9 +331,6 @@ int cmd_rm(int argc, const char **argv, const char 
>> *prefix)
>>   oidclr();
>>   if (check_local_mod(, index_only))
>>   exit(1);
>> - } else if (!index_only) {
>> - if (check_submodules_use_gitfiles())
>> - exit(1);
>>   }
>>
>
> Hmph.  It may be a bit strange to see an "index-only" remove to
> touch working tree, no?  Yet submodules_absorb_gitdir_if_needed() is
> unconditionally called above, which feels somewhat unexpected.

I agree. In a reroll I'll protect the call with

if (!index_only)
submodules_absorb_gitdir_if_needed(prefix);

>
>> @@ -389,32 +359,20 @@ int cmd_rm(int argc, const char **argv, const char 
>> *prefix)
>>*/
>>   if (!index_only) {
>>   int removed = 0, gitmodules_modified = 0;
>>   for (i = 0; i < list.nr; i++) {
>>   const char *path = list.entry[i].name;
>>   if (list.entry[i].is_submodule) {
>> + struct strbuf buf = STRBUF_INIT;
>> +
>> + strbuf_addstr(, path);
>> + if (remove_dir_recursively(, 0))
>> + die(_("could not remove '%s'"), path);
>> + strbuf_release();
>> +
>> + removed = 1;
>> + if (!remove_path_from_gitmodules(path))
>> + gitmodules_modified = 1;
>> + continue;
>>   }
>
> I do not see any behaviour change from the original (not quoted
> here), but it is somewhat surprising that "git rm ./submodule" does
> not really check if the submodule has local modifications and files
> that are not even added before remove_dir_recursively() is called.
>
> Or perhaps I am reading the code incorrectly and such a check is
> done elsewhere?

When determining if the entry is a submodule (i.e. setting
the is_submodule bit) we have

list.entry[list.nr].is_submodule = S_ISGITLINK(ce->ce_mode);
if (list.entry[list.nr++].is_submodule &&
  !is_staging_gitmodules_ok())
die (_("Please stage ..."));

I think for clarity we could drop the is_submodule bit and only
and directly do

if (S_ISGITLINK(ce->ce_mode) &&
  !is_staging_gitmodules_ok())
die (_("Please stage ..."));

and below (the code that you quoted) also just check via
S_ISGITLINK(ce->ce_mode)

Thanks,
Stefan

Re: [PATCHv5 1/4] submodule.h: add extern keyword to functions

[Stefan Beller]

On Mon, Dec 26, 2016 at 5:13 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> As the upcoming series will add a lot of functions to the submodule
>> header, let's first make the header consistent to the rest of the project
>> by adding the extern keyword to functions.
>
> This may be the right thing to do in the longer term but a patch
> like this adds a lot of unnecessary work on the integrator when
> there are _other_ topics that are in flight.  I'll see how bad the
> conflicts are while merging the topic to 'pu'.
>
> Thanks.

Maybe we can roll this patch on its own,
so in case the latter patches need rerolls we still have this one and can
build on top?

Thanks,
Stefan

Re: [PATCHv5 3/4] submodule: rename and add flags to ok_to_remove_submodule

[Stefan Beller]

On Mon, Dec 26, 2016 at 4:53 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> As only 0 is understood as false, rename the function to invert the
>> meaning, i.e. the return code of 0 signals the removal of the submodule
>> is fine, and other values can be used to return a more precise answer
>> what went wrong.
>
> Makes sense to rename it as that will catch all the callers that
> depend on the old semantics and name.
>
>> - if (start_command())
>> - die(_("could not run 'git status --porcelain -u 
>> --ignore-submodules=none' in submodule %s"), path);
>> + if (start_command()) {
>> + if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
>> + die(_("could not start 'git status in submodule '%s'"),
>> + path);
>> + ret = -1;
>> + goto out;
>> + }
>
> This new codepath that does not die will not leak anything, as
> a failed start_command() should release its argv[] and env[].
>
>>   len = strbuf_read(, cp.out, 1024);
>>   if (len > 2)
>> - ok_to_remove = 0;
>> + ret = 1;
>
> Not a new problem but is it obvious why the comparison of "len" is
> against "2"?  This may deserve a one-liner comment.
>
> Otherwise looks good to me.

I took the check "len > 2" from the other occurrence in submodule.c.
When thinking about it (and inspecting the man page for
porcelain status), I think just checking != 0 is sufficient.

So in a reroll I'll change that to just

if (len)
...

I'll look at the other occurrences and see if we can reuse code
as well.

Thanks,
Stefan

[PATCH] worktree: initialize return value for submodule_uses_worktrees

[Stefan Beller]

When the worktrees directory is empty, the `ret` will be returned
uninitialized. Fix it by initializing the value.

Signed-off-by: Stefan Beller 
---

This goes on top of 1a248cf (origin/sb/submodule-embed-gitdir);
ideally to be squashed, but as it is in next already, as a separate
patch.

Thanks,
Stefan

 worktree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/worktree.c b/worktree.c
index d4606aa8cd..828fd7a0ad 100644
--- a/worktree.c
+++ b/worktree.c
@@ -387,7 +387,7 @@ int submodule_uses_worktrees(const char *path)
struct strbuf sb = STRBUF_INIT;
DIR *dir;
struct dirent *d;
-   int ret;
+   int ret = 0;
struct repository_format format;
 
submodule_gitdir = git_pathdup_submodule(path, "%s", "");
-- 
2.11.0.rc2.50.g8bda6b2.dirty

Re: Making it possible to do “git push origin” instead of “git push origin ”, without having to one-time prepare each branch for it

[Stefan Monov]

Thanks guys!

Re: git-apply: warn/fail on *changed* end of line (eol) *only*?

[Junio C Hamano]

Junio C Hamano  writes:

> Imagine that the project wants LF line endings, i.e. it considers
> that a line with CRLF ending has an unwanted "whitespace" at the
> end.  Now, you start from this source file:
>
> 1 
> 3 
> 5 
>
> and a patch like this comes in:
>
>  1 
> -3 
> +three 
>  5 
>
> You think that "3 " was replaced by "three ", and the
> claim is "the 'previous' contents already had  ending, so the
> change is not making things worse".

To see the problem with "check existing lines", it probably is
easier to extend the above example to start from a file with one
more line, like this:

1 
3 
4 
5 

and extend all the example patches to remove "4 " line as well,
where they remove "3 ", making the first example patch like
so:

 1 
-3 
-4 
+three 
 5 

Now, if you take "three " to be replacing "3 ", then you
may feel that not warning on the CRLF would be the right thing, but
there is no reason (other than the fact you, a human, understand
what 'three' means) to choose "3 " over "4 " as the
original.  If you take "three " to be replacing "4 ", you
would need to warn.

A totally uninteresting special case is when the original is all
, but in that case, as you already said in the original
message, the project wants  and you can configure it as such.
Then  in the line-end  won't be mistaken as if it is a
whitespace character  at the end of a line terminated with .

Re: What's cooking in git.git (Dec 2016, #05; Mon, 19)

[Junio C Hamano]

Michael Haggerty  writes:

> Sorry I didn't notice this earlier, but the `LOCK_REPORT_ON_ERROR`
> constant introduced by
>
> 3f061bf "lockfile: LOCK_REPORT_ON_ERROR", 2016-12-07
>
> sets that constant to the value 2,...

Sorry I didn't notice this earlier, either.  Thanks for spotting.

-- >8 --
From: Junio C Hamano 
Date: Tue, 27 Dec 2016 09:12:09 -0800
Subject: [PATCH] lockfile: move REPORT_ON_ERROR bit elsewhere

There was LOCK_NO_DEREF defined as 2 = 1<<1 with the same value,
which was missed due to a huge comment block.  Deconflict by moving
the new one to 4 = 1<<2 for now.

Signed-off-by: Junio C Hamano 
---
 lockfile.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lockfile.h b/lockfile.h
index 16775a7d79..7b715f9e77 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -137,7 +137,7 @@ struct lock_file {
  * ... this flag can be passed instead to return -1 and give the usual
  * error message upon an error.
  */
-#define LOCK_REPORT_ON_ERROR 2
+#define LOCK_REPORT_ON_ERROR 4
 
 /*
  * Usually symbolic links in the destination path are resolved. This
-- 
2.11.0-449-gc01fa73926

Re: What's cooking in git.git (Dec 2016, #05; Mon, 19)

[Michael Haggerty]

On 12/20/2016 01:21 AM, Junio C Hamano wrote:
> 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.
> 
> The second (rather large) batch of topics have been merged to
> 'master'.  Please test and catch possible regressions early.
> 
> 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/lock-report-on-error (2016-12-07) 3 commits
>   (merged to 'next' on 2016-12-13 at cb6c07ee92)
>  + lockfile: LOCK_REPORT_ON_ERROR
>  + hold_locked_index(): align error handling with hold_lockfile_for_update()
>  + wt-status: implement opportunisitc index update correctly
> 
>  Git 2.11 had a minor regression in "merge --ff-only" that competed
>  with another process that simultanously attempted to update the
>  index. We used to explain what went wrong with an error message,
>  but the new code silently failed.  The error message has been
>  resurrected.

Sorry I didn't notice this earlier, but the `LOCK_REPORT_ON_ERROR`
constant introduced by

3f061bf "lockfile: LOCK_REPORT_ON_ERROR", 2016-12-07

sets that constant to the value 2, which is the same value set for the
existing constant `LOCK_NO_DEREF`. Both constants define bits that can
be set in the `flags` argument of `hold_lock_file_for_update()`, so one
of these values needs to be changed.

Michael

[PATCH v9 11/20] ref-filter: introduce refname_atom_parser()

[Karthik Nayak]

From: Karthik Nayak 

Using refname_atom_parser_internal(), introduce refname_atom_parser()
which will parse the %(symref) and %(refname) atoms. Store the parsed
information into the 'used_atom' structure based on the modifiers used
along with the atoms.

Now the '%(symref)' atom supports the ':strip' atom modifier. Update the
Documentation and tests to reflect this.

Helped-by: Jeff King 
Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt |  5 +++
 ref-filter.c   | 73 +-
 t/t6300-for-each-ref.sh|  9 +
 3 files changed, 54 insertions(+), 33 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index b17273e..0fa5818 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -170,6 +170,11 @@ if::
the value between the %(if:...) and %(then) atoms with the
given string.
 
+symref::
+   The ref which the given symbolic ref refers to. If not a
+   symbolic ref, nothing is printed. Respects the `:short` and
+   `:strip` options in the same way as `refname` above.
+
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
 be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index c1ebc40..7bfd656 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -187,6 +187,11 @@ static void objectname_atom_parser(struct used_atom *atom, 
const char *arg)
die(_("unrecognized %%(objectname) argument: %s"), arg);
 }
 
+static void refname_atom_parser(struct used_atom *atom, const char *arg)
+{
+   return refname_atom_parser_internal(>u.refname, arg, atom->name);
+}
+
 static align_type parse_align_position(const char *s)
 {
if (!strcmp(s, "right"))
@@ -257,7 +262,7 @@ static struct {
cmp_type cmp_type;
void (*parser)(struct used_atom *atom, const char *arg);
 } valid_atom[] = {
-   { "refname" },
+   { "refname" , FIELD_STR, refname_atom_parser },
{ "objecttype" },
{ "objectsize", FIELD_ULONG },
{ "objectname", FIELD_STR, objectname_atom_parser },
@@ -287,7 +292,7 @@ static struct {
{ "contents", FIELD_STR, contents_atom_parser },
{ "upstream", FIELD_STR, remote_ref_atom_parser },
{ "push", FIELD_STR, remote_ref_atom_parser },
-   { "symref" },
+   { "symref", FIELD_STR, refname_atom_parser },
{ "flag" },
{ "HEAD" },
{ "color", FIELD_STR, color_atom_parser },
@@ -1082,21 +1087,16 @@ static inline char *copy_advance(char *dst, const char 
*src)
return dst;
 }
 
-static const char *strip_ref_components(const char *refname, const char 
*nr_arg)
+static const char *strip_ref_components(const char *refname, unsigned int len)
 {
-   char *end;
-   long nr = strtol(nr_arg, , 10);
-   long remaining = nr;
+   long remaining = len;
const char *start = refname;
 
-   if (nr < 1 || *end != '\0')
-   die(_(":strip= requires a positive integer argument"));
-
while (remaining) {
switch (*start++) {
case '\0':
-   die(_("ref '%s' does not have %ld components to 
:strip"),
-   refname, nr);
+   die(_("ref '%s' does not have %ud components to 
:strip"),
+   refname, len);
case '/':
remaining--;
break;
@@ -1105,6 +1105,16 @@ static const char *strip_ref_components(const char 
*refname, const char *nr_arg)
return start;
 }
 
+static const char *show_ref(struct refname_atom *atom, const char *refname)
+{
+   if (atom->option == R_SHORT)
+   return shorten_unambiguous_ref(refname, warn_ambiguous_refs);
+   else if (atom->option == R_STRIP)
+   return strip_ref_components(refname, atom->strip);
+   else
+   return refname;
+}
+
 static void fill_remote_ref_details(struct used_atom *atom, const char 
*refname,
struct branch *branch, const char **s)
 {
@@ -1177,6 +1187,21 @@ char *get_head_description(void)
return strbuf_detach(, NULL);
 }
 
+static const char *get_symref(struct used_atom *atom, struct ref_array_item 
*ref)
+{
+   if (!ref->symref)
+   return "";
+   else
+   return show_ref(>u.refname, ref->symref);
+}
+
+static const char *get_refname(struct used_atom *atom, struct ref_array_item 
*ref)
+{
+   if (ref->kind & FILTER_REFS_DETACHED_HEAD)
+   return get_head_description();
+   return show_ref(>u.refname, ref->refname);
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
@@ -1205,7 +1230,6 @@ static void populate_value(struct 

[PATCH v9 08/20] ref-filter: add support for %(upstream:track,nobracket)

[Karthik Nayak]

From: Karthik Nayak 

Add support for %(upstream:track,nobracket) which will print the
tracking information without the brackets (i.e. "ahead N, behind M").
This is needed when we port branch.c to use ref-filter's printing APIs.

Add test and documentation for the same.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt | 10 --
 ref-filter.c   | 67 +-
 t/t6300-for-each-ref.sh|  2 ++
 3 files changed, 53 insertions(+), 26 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 24a1679..b17273e 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -120,9 +120,13 @@ upstream::
`refname` above.  Additionally respects `:track` to show
"[ahead N, behind M]" and `:trackshort` to show the terse
version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
-   or "=" (in sync).  Has no effect if the ref does not have
-   tracking information associated with it. `:track` also prints
-   "[gone]" whenever unknown upstream ref is encountered.
+   or "=" (in sync). `:track` also prints "[gone]" whenever
+   unknown upstream ref is encountered. Append `:track,nobracket`
+   to show tracking information without brackets (i.e "ahead N,
+   behind M").  Has no effect if the ref does not have tracking
+   information associated with it.  All the options apart from
+   `nobracket` are mutually exclusive, but if used together the
+   last option is selected.
 
 push::
The name of a local ref which represents the `@{push}` location
diff --git a/ref-filter.c b/ref-filter.c
index 9989918..6de0927 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -48,8 +48,10 @@ static struct used_atom {
union {
char color[COLOR_MAXLEN];
struct align align;
-   enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
-   remote_ref;
+   struct {
+   enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT } 
option;
+   unsigned int nobracket : 1;
+   } remote_ref;
struct {
enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, 
C_SUB, C_TRAILERS } option;
unsigned int nlines;
@@ -77,16 +79,33 @@ static void color_atom_parser(struct used_atom *atom, const 
char *color_value)
 
 static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
 {
-   if (!arg)
-   atom->u.remote_ref = RR_NORMAL;
-   else if (!strcmp(arg, "short"))
-   atom->u.remote_ref = RR_SHORTEN;
-   else if (!strcmp(arg, "track"))
-   atom->u.remote_ref = RR_TRACK;
-   else if (!strcmp(arg, "trackshort"))
-   atom->u.remote_ref = RR_TRACKSHORT;
-   else
-   die(_("unrecognized format: %%(%s)"), atom->name);
+   struct string_list params = STRING_LIST_INIT_DUP;
+   int i;
+
+   if (!arg) {
+   atom->u.remote_ref.option = RR_NORMAL;
+   return;
+   }
+
+   atom->u.remote_ref.nobracket = 0;
+   string_list_split(, arg, ',', -1);
+
+   for (i = 0; i < params.nr; i++) {
+   const char *s = params.items[i].string;
+
+   if (!strcmp(s, "short"))
+   atom->u.remote_ref.option = RR_SHORTEN;
+   else if (!strcmp(s, "track"))
+   atom->u.remote_ref.option = RR_TRACK;
+   else if (!strcmp(s, "trackshort"))
+   atom->u.remote_ref.option = RR_TRACKSHORT;
+   else if (!strcmp(s, "nobracket"))
+   atom->u.remote_ref.nobracket = 1;
+   else
+   die(_("unrecognized format: %%(%s)"), atom->name);
+   }
+
+   string_list_clear(, 0);
 }
 
 static void body_atom_parser(struct used_atom *atom, const char *arg)
@@ -1069,25 +1088,27 @@ static void fill_remote_ref_details(struct used_atom 
*atom, const char *refname,
struct branch *branch, const char **s)
 {
int num_ours, num_theirs;
-   if (atom->u.remote_ref == RR_SHORTEN)
+   if (atom->u.remote_ref.option == RR_SHORTEN)
*s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
-   else if (atom->u.remote_ref == RR_TRACK) {
+   else if (atom->u.remote_ref.option == RR_TRACK) {
if (stat_tracking_info(branch, _ours,
   _theirs, NULL)) {
-   *s = "[gone]";
-   return;
-   }
-
-   if (!num_ours && !num_theirs)
+   *s = xstrdup("gone");

[PATCH v9 10/20] ref-filter: introduce refname_atom_parser_internal()

[Karthik Nayak]

From: Karthik Nayak 

Since there are multiple atoms which print refs ('%(refname)',
'%(symref)', '%(push)', '%(upstream)'), it makes sense to have a common
ground for parsing them. This would allow us to share implementations of
the atom modifiers between these atoms.

Introduce refname_atom_parser_internal() to act as a common parsing
function for ref printing atoms. This would eventually be used to
introduce refname_atom_parser() and symref_atom_parser() and also be
internally used in remote_ref_atom_parser().

Helped-by: Jeff King 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/ref-filter.c b/ref-filter.c
index e98ef4b..c1ebc40 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -32,6 +32,11 @@ struct if_then_else {
condition_satisfied : 1;
 };
 
+struct refname_atom {
+   enum { R_NORMAL, R_SHORT, R_STRIP } option;
+   unsigned int strip;
+};
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().
@@ -64,6 +69,7 @@ static struct used_atom {
enum { O_FULL, O_LENGTH, O_SHORT } option;
unsigned int length;
} objectname;
+   struct refname_atom refname;
} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -77,6 +83,21 @@ static void color_atom_parser(struct used_atom *atom, const 
char *color_value)
die(_("unrecognized color: %%(color:%s)"), color_value);
 }
 
+static void refname_atom_parser_internal(struct refname_atom *atom,
+const char *arg, const char *name)
+{
+   if (!arg)
+   atom->option = R_NORMAL;
+   else if (!strcmp(arg, "short"))
+   atom->option = R_SHORT;
+   else if (skip_prefix(arg, "strip=", )) {
+   atom->option = R_STRIP;
+   if (strtoul_ui(arg, 10, >strip) || atom->strip <= 0)
+   die(_("positive value expected refname:strip=%s"), arg);
+   } else
+   die(_("unrecognized %%(%s) argument: %s"), name, arg);
+}
+
 static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
 {
struct string_list params = STRING_LIST_INIT_DUP;
-- 
2.10.2

[PATCH v9 13/20] ref-filter: rename the 'strip' option to 'lstrip'

[Karthik Nayak]

In preparation for the upcoming patch, where we introduce the 'rstrip'
option. Rename the 'strip' option to 'lstrip' to remove ambiguity.

Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt | 10 +-
 builtin/tag.c  |  4 ++--
 ref-filter.c   | 20 ++--
 t/t6300-for-each-ref.sh| 22 +++---
 4 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index e7cdff6..46b4583 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -95,9 +95,9 @@ refname::
The name of the ref (the part after $GIT_DIR/).
For a non-ambiguous short name of the ref append `:short`.
The option core.warnAmbiguousRefs is used to select the strict
-   abbreviation mode. If `strip=` is appended, strips ``
+   abbreviation mode. If `lstrip=` is appended, strips ``
slash-separated path components from the front of the refname
-   (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
+   (e.g., `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo`.
`` must be a positive integer.  If a displayed ref has fewer
components than ``, the command aborts with an error.
 
@@ -116,7 +116,7 @@ objectname::
 
 upstream::
The name of a local ref which can be considered ``upstream''
-   from the displayed ref. Respects `:short` and `:strip` in the
+   from the displayed ref. Respects `:short` and `:lstrip` in the
same way as `refname` above.  Additionally respects `:track`
to show "[ahead N, behind M]" and `:trackshort` to show the
terse version: ">" (ahead), "<" (behind), "<>" (ahead and
@@ -130,7 +130,7 @@ upstream::
 
 push::
The name of a local ref which represents the `@{push}`
-   location for the displayed ref. Respects `:short`, `:strip`,
+   location for the displayed ref. Respects `:short`, `:lstrip`,
`:track`, and `:trackshort` options as `upstream`
does. Produces an empty string if no `@{push}` ref is
configured.
@@ -174,7 +174,7 @@ if::
 symref::
The ref which the given symbolic ref refers to. If not a
symbolic ref, nothing is printed. Respects the `:short` and
-   `:strip` options in the same way as `refname` above.
+   `:lstrip` options in the same way as `refname` above.
 
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
diff --git a/builtin/tag.c b/builtin/tag.c
index 73df728..b4789ce 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -45,11 +45,11 @@ static int list_tags(struct ref_filter *filter, struct 
ref_sorting *sorting, con
if (!format) {
if (filter->lines) {
to_free = xstrfmt("%s %%(contents:lines=%d)",
- "%(align:15)%(refname:strip=2)%(end)",
+ 
"%(align:15)%(refname:lstrip=2)%(end)",
  filter->lines);
format = to_free;
} else
-   format = "%(refname:strip=2)";
+   format = "%(refname:lstrip=2)";
}
 
verify_ref_format(format);
diff --git a/ref-filter.c b/ref-filter.c
index 9140539..e0015c6 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -33,8 +33,8 @@ struct if_then_else {
 };
 
 struct refname_atom {
-   enum { R_NORMAL, R_SHORT, R_STRIP } option;
-   unsigned int strip;
+   enum { R_NORMAL, R_SHORT, R_LSTRIP } option;
+   unsigned int lstrip;
 };
 
 /*
@@ -91,10 +91,10 @@ static void refname_atom_parser_internal(struct 
refname_atom *atom,
atom->option = R_NORMAL;
else if (!strcmp(arg, "short"))
atom->option = R_SHORT;
-   else if (skip_prefix(arg, "strip=", )) {
-   atom->option = R_STRIP;
-   if (strtoul_ui(arg, 10, >strip) || atom->strip <= 0)
-   die(_("positive value expected refname:strip=%s"), arg);
+   else if (skip_prefix(arg, "lstrip=", )) {
+   atom->option = R_LSTRIP;
+   if (strtoul_ui(arg, 10, >lstrip) || atom->lstrip <= 0)
+   die(_("positive value expected refname:lstrip=%s"), 
arg);
} else
die(_("unrecognized %%(%s) argument: %s"), name, arg);
 }
@@ -1091,7 +1091,7 @@ static inline char *copy_advance(char *dst, const char 
*src)
return dst;
 }
 
-static const char *strip_ref_components(const char *refname, unsigned int len)
+static const char *lstrip_ref_components(const char *refname, unsigned int len)
 {
long remaining = len;
const char *start = refname;
@@ -1099,7 +1099,7 @@ static const char *strip_ref_components(const char 
*refname, unsigned int len)
 

[PATCH v9 16/20] ref-filter: add an 'rstrip=' option to atoms which deal with refnames

[Karthik Nayak]

Complimenting the existing 'lstrip=' option, add an 'rstrip='
option which strips `` slash-separated path components from the end
of the refname (e.g., `%(refname:rstrip=2)` turns `refs/tags/foo` into
`refs`).

Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt | 40 -
 ref-filter.c   | 41 --
 t/t6300-for-each-ref.sh| 19 ++
 3 files changed, 80 insertions(+), 20 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 9055ba0..81db67d 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -95,10 +95,12 @@ refname::
The name of the ref (the part after $GIT_DIR/).
For a non-ambiguous short name of the ref append `:short`.
The option core.warnAmbiguousRefs is used to select the strict
-   abbreviation mode. If `lstrip=` is appended, strips ``
-   slash-separated path components from the front of the refname
-   (e.g., `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo`.
-   if `` is a negative number, then only `` path components
+   abbreviation mode. The `lstrip=` or `rstrip=` option can
+   be appended to strip `` slash-separated path components
+   from the left or right of the refname respectively (e.g.,
+   `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo` and
+   `%(refname:rstrip=2)` turns `refs/tags/foo` into `refs`).  if
+   `` is a negative number, then only `` path components
are left behind.
 
 objecttype::
@@ -116,22 +118,23 @@ objectname::
 
 upstream::
The name of a local ref which can be considered ``upstream''
-   from the displayed ref. Respects `:short` and `:lstrip` in the
-   same way as `refname` above.  Additionally respects `:track`
-   to show "[ahead N, behind M]" and `:trackshort` to show the
-   terse version: ">" (ahead), "<" (behind), "<>" (ahead and
-   behind), or "=" (in sync). `:track` also prints "[gone]"
-   whenever unknown upstream ref is encountered. Append
-   `:track,nobracket` to show tracking information without
-   brackets (i.e "ahead N, behind M").  Has no effect if the ref
-   does not have tracking information associated with it.  All
-   the options apart from `nobracket` are mutually exclusive, but
-   if used together the last option is selected.
+   from the displayed ref. Respects `:short`, `:lstrip` and
+   `:rstrip` in the same way as `refname` above.  Additionally
+   respects `:track` to show "[ahead N, behind M]" and
+   `:trackshort` to show the terse version: ">" (ahead), "<"
+   (behind), "<>" (ahead and behind), or "=" (in sync). `:track`
+   also prints "[gone]" whenever unknown upstream ref is
+   encountered. Append `:track,nobracket` to show tracking
+   information without brackets (i.e "ahead N, behind M").  Has
+   no effect if the ref does not have tracking information
+   associated with it.  All the options apart from `nobracket`
+   are mutually exclusive, but if used together the last option
+   is selected.
 
 push::
The name of a local ref which represents the `@{push}`
location for the displayed ref. Respects `:short`, `:lstrip`,
-   `:track`, and `:trackshort` options as `upstream`
+   `:rstrip`, `:track`, and `:trackshort` options as `upstream`
does. Produces an empty string if no `@{push}` ref is
configured.
 
@@ -173,8 +176,9 @@ if::
 
 symref::
The ref which the given symbolic ref refers to. If not a
-   symbolic ref, nothing is printed. Respects the `:short` and
-   `:lstrip` options in the same way as `refname` above.
+   symbolic ref, nothing is printed. Respects the `:short`,
+   `:lstrip` and `:rstrip` options in the same way as `refname`
+   above.
 
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
diff --git a/ref-filter.c b/ref-filter.c
index 1cd92ea..dd7e751 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -33,8 +33,8 @@ struct if_then_else {
 };
 
 struct refname_atom {
-   enum { R_NORMAL, R_SHORT, R_LSTRIP } option;
-   int lstrip;
+   enum { R_NORMAL, R_SHORT, R_LSTRIP, R_RSTRIP } option;
+   int lstrip, rstrip;
 };
 
 /*
@@ -95,6 +95,10 @@ static void refname_atom_parser_internal(struct refname_atom 
*atom,
atom->option = R_LSTRIP;
if (strtol_i(arg, 10, >lstrip))
die(_("Integer value expected refname:lstrip=%s"), arg);
+   } else if (skip_prefix(arg, "rstrip=", )) {
+   atom->option = R_RSTRIP;
+   if (strtol_i(arg, 10, >rstrip))
+   die(_("Integer value expected refname:rstrip=%s"), arg);
} else

[PATCH v9 05/20] ref-filter: move get_head_description() from branch.c

[Karthik Nayak]

From: Karthik Nayak 

Move the implementation of get_head_description() from branch.c to
ref-filter.  This gives a description of the HEAD ref if called. This
is used as the refname for the HEAD ref whenever the
FILTER_REFS_DETACHED_HEAD option is used. Make it public because we
need it to calculate the length of the HEAD refs description in
branch.c:calc_maxwidth() when we port branch.c to use ref-filter
APIs.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 builtin/branch.c | 33 -
 ref-filter.c | 38 --
 ref-filter.h |  2 ++
 3 files changed, 38 insertions(+), 35 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 9d30f55..6423ebc 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -364,39 +364,6 @@ static void add_verbose_info(struct strbuf *out, struct 
ref_array_item *item,
strbuf_release();
 }
 
-static char *get_head_description(void)
-{
-   struct strbuf desc = STRBUF_INIT;
-   struct wt_status_state state;
-   memset(, 0, sizeof(state));
-   wt_status_get_state(, 1);
-   if (state.rebase_in_progress ||
-   state.rebase_interactive_in_progress)
-   strbuf_addf(, _("(no branch, rebasing %s)"),
-   state.branch);
-   else if (state.bisect_in_progress)
-   strbuf_addf(, _("(no branch, bisect started on %s)"),
-   state.branch);
-   else if (state.detached_from) {
-   if (state.detached_at)
-   /* TRANSLATORS: make sure this matches
-  "HEAD detached at " in wt-status.c */
-   strbuf_addf(, _("(HEAD detached at %s)"),
-   state.detached_from);
-   else
-   /* TRANSLATORS: make sure this matches
-  "HEAD detached from " in wt-status.c */
-   strbuf_addf(, _("(HEAD detached from %s)"),
-   state.detached_from);
-   }
-   else
-   strbuf_addstr(, _("(no branch)"));
-   free(state.branch);
-   free(state.onto);
-   free(state.detached_from);
-   return strbuf_detach(, NULL);
-}
-
 static void format_and_print_ref_item(struct ref_array_item *item, int 
maxwidth,
  struct ref_filter *filter, const char 
*remote_prefix)
 {
diff --git a/ref-filter.c b/ref-filter.c
index 385fc04..5511a20 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -14,6 +14,7 @@
 #include "git-compat-util.h"
 #include "version.h"
 #include "trailer.h"
+#include "wt-status.h"
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 typedef enum { COMPARE_EQUAL, COMPARE_UNEQUAL, COMPARE_NONE } cmp_status;
@@ -1101,6 +1102,37 @@ static void fill_remote_ref_details(struct used_atom 
*atom, const char *refname,
*s = refname;
 }
 
+char *get_head_description(void)
+{
+   struct strbuf desc = STRBUF_INIT;
+   struct wt_status_state state;
+   memset(, 0, sizeof(state));
+   wt_status_get_state(, 1);
+   if (state.rebase_in_progress ||
+   state.rebase_interactive_in_progress)
+   strbuf_addf(, _("(no branch, rebasing %s)"),
+   state.branch);
+   else if (state.bisect_in_progress)
+   strbuf_addf(, _("(no branch, bisect started on %s)"),
+   state.branch);
+   else if (state.detached_from) {
+   /* TRANSLATORS: make sure these match _("HEAD detached at ")
+  and _("HEAD detached from ") in wt-status.c */
+   if (state.detached_at)
+   strbuf_addf(, _("(HEAD detached at %s)"),
+   state.detached_from);
+   else
+   strbuf_addf(, _("(HEAD detached from %s)"),
+   state.detached_from);
+   }
+   else
+   strbuf_addstr(, _("(no branch)"));
+   free(state.branch);
+   free(state.onto);
+   free(state.detached_from);
+   return strbuf_detach(, NULL);
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
@@ -1140,9 +1172,11 @@ static void populate_value(struct ref_array_item *ref)
name++;
}
 
-   if (starts_with(name, "refname"))
+   if (starts_with(name, "refname")) {
refname = ref->refname;
-   else if (starts_with(name, "symref"))
+   if (ref->kind & FILTER_REFS_DETACHED_HEAD)
+   refname = get_head_description();
+   } else if (starts_with(name, "symref"))
refname = ref->symref ? ref->symref : "";

[PATCH v9 12/20] ref-filter: make remote_ref_atom_parser() use refname_atom_parser_internal()

[Karthik Nayak]

From: Karthik Nayak 

Use the recently introduced refname_atom_parser_internal() within
remote_ref_atom_parser(), this provides a common base for all the ref
printing atoms, allowing %(upstream) and %(push) to also use the
':strip' option.

The atoms '%(push)' and '%(upstream)' will retain the ':track' and
':trackshort' atom modifiers to themselves as they have no meaning in
context to the '%(refname)' and '%(symref)' atoms.

Update the documentation and tests to reflect the same.

Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt | 31 ---
 ref-filter.c   | 26 +++---
 t/t6300-for-each-ref.sh|  2 ++
 3 files changed, 33 insertions(+), 26 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 0fa5818..e7cdff6 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -116,23 +116,24 @@ objectname::
 
 upstream::
The name of a local ref which can be considered ``upstream''
-   from the displayed ref. Respects `:short` in the same way as
-   `refname` above.  Additionally respects `:track` to show
-   "[ahead N, behind M]" and `:trackshort` to show the terse
-   version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
-   or "=" (in sync). `:track` also prints "[gone]" whenever
-   unknown upstream ref is encountered. Append `:track,nobracket`
-   to show tracking information without brackets (i.e "ahead N,
-   behind M").  Has no effect if the ref does not have tracking
-   information associated with it.  All the options apart from
-   `nobracket` are mutually exclusive, but if used together the
-   last option is selected.
+   from the displayed ref. Respects `:short` and `:strip` in the
+   same way as `refname` above.  Additionally respects `:track`
+   to show "[ahead N, behind M]" and `:trackshort` to show the
+   terse version: ">" (ahead), "<" (behind), "<>" (ahead and
+   behind), or "=" (in sync). `:track` also prints "[gone]"
+   whenever unknown upstream ref is encountered. Append
+   `:track,nobracket` to show tracking information without
+   brackets (i.e "ahead N, behind M").  Has no effect if the ref
+   does not have tracking information associated with it.  All
+   the options apart from `nobracket` are mutually exclusive, but
+   if used together the last option is selected.
 
 push::
-   The name of a local ref which represents the `@{push}` location
-   for the displayed ref. Respects `:short`, `:track`, and
-   `:trackshort` options as `upstream` does. Produces an empty
-   string if no `@{push}` ref is configured.
+   The name of a local ref which represents the `@{push}`
+   location for the displayed ref. Respects `:short`, `:strip`,
+   `:track`, and `:trackshort` options as `upstream`
+   does. Produces an empty string if no `@{push}` ref is
+   configured.
 
 HEAD::
'*' if HEAD matches current ref (the checked out branch), ' '
diff --git a/ref-filter.c b/ref-filter.c
index 7bfd656..9140539 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -54,7 +54,8 @@ static struct used_atom {
char color[COLOR_MAXLEN];
struct align align;
struct {
-   enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT } 
option;
+   enum { RR_REF, RR_TRACK, RR_TRACKSHORT } option;
+   struct refname_atom refname;
unsigned int nobracket : 1;
} remote_ref;
struct {
@@ -104,7 +105,9 @@ static void remote_ref_atom_parser(struct used_atom *atom, 
const char *arg)
int i;
 
if (!arg) {
-   atom->u.remote_ref.option = RR_NORMAL;
+   atom->u.remote_ref.option = RR_REF;
+   refname_atom_parser_internal(>u.remote_ref.refname,
+arg, atom->name);
return;
}
 
@@ -114,16 +117,17 @@ static void remote_ref_atom_parser(struct used_atom 
*atom, const char *arg)
for (i = 0; i < params.nr; i++) {
const char *s = params.items[i].string;
 
-   if (!strcmp(s, "short"))
-   atom->u.remote_ref.option = RR_SHORTEN;
-   else if (!strcmp(s, "track"))
+   if (!strcmp(s, "track"))
atom->u.remote_ref.option = RR_TRACK;
else if (!strcmp(s, "trackshort"))
atom->u.remote_ref.option = RR_TRACKSHORT;
else if (!strcmp(s, "nobracket"))
atom->u.remote_ref.nobracket = 1;
-   else
-   die(_("unrecognized format: %%(%s)"), atom->name);
+   else {
+   

[PATCH v9 19/20] branch: use ref-filter printing APIs

[Karthik Nayak]

From: Karthik Nayak 

Port branch.c to use ref-filter APIs for printing. This clears out
most of the code used in branch.c for printing and replaces them with
calls made to the ref-filter library.

Introduce build_format() which gets the format required for printing
of refs. Make amendments to print_ref_list() to reflect these changes.

The strings included in build_format() may not be safely quoted for
inclusion (i.e. it might contain '%' which needs to be escaped with an
additional '%'). Introduce quote_literal_for_format() as a helper
function which takes a string and returns a version of the string that
is safely quoted to be used in the for-each-ref format which is built
in build_format().

Change calc_maxwidth() to also account for the length of HEAD ref, by
calling ref-filter:get_head_discription().

Also change the test in t6040 to reflect the changes.

Before this patch, all cross-prefix symrefs weren't shortened. Since
we're using ref-filter APIs, we shorten all symrefs by default. We also
allow the user to change the format if needed with the introduction of
the '--format' option in the next patch.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Helped-by: Junio C Hamano 
Helped-by: Jeff King 
Helped-by: Ramsay Jones 
Signed-off-by: Karthik Nayak 
---
 builtin/branch.c | 249 ---
 t/t3203-branch-output.sh |   2 +-
 t/t6040-tracking-info.sh |   2 +-
 3 files changed, 88 insertions(+), 165 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 34cd61c..2887545 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -36,12 +36,12 @@ static unsigned char head_sha1[20];
 
 static int branch_use_color = -1;
 static char branch_colors[][COLOR_MAXLEN] = {
-   GIT_COLOR_RESET,
-   GIT_COLOR_NORMAL,   /* PLAIN */
-   GIT_COLOR_RED,  /* REMOTE */
-   GIT_COLOR_NORMAL,   /* LOCAL */
-   GIT_COLOR_GREEN,/* CURRENT */
-   GIT_COLOR_BLUE, /* UPSTREAM */
+   "%(color:reset)",
+   "%(color:reset)",   /* PLAIN */
+   "%(color:red)", /* REMOTE */
+   "%(color:reset)",   /* LOCAL */
+   "%(color:green)",   /* CURRENT */
+   "%(color:blue)",/* UPSTREAM */
 };
 enum color_branch {
BRANCH_COLOR_RESET = 0,
@@ -280,180 +280,88 @@ static int delete_branches(int argc, const char **argv, 
int force, int kinds,
return(ret);
 }
 
-static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
-   int show_upstream_ref)
+static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
 {
-   int ours, theirs;
-   char *ref = NULL;
-   struct branch *branch = branch_get(branch_name);
-   const char *upstream;
-   struct strbuf fancy = STRBUF_INIT;
-   int upstream_is_gone = 0;
-   int added_decoration = 1;
-
-   if (stat_tracking_info(branch, , , ) < 0) {
-   if (!upstream)
-   return;
-   upstream_is_gone = 1;
-   }
-
-   if (show_upstream_ref) {
-   ref = shorten_unambiguous_ref(upstream, 0);
-   if (want_color(branch_use_color))
-   strbuf_addf(, "%s%s%s",
-   branch_get_color(BRANCH_COLOR_UPSTREAM),
-   ref, 
branch_get_color(BRANCH_COLOR_RESET));
-   else
-   strbuf_addstr(, ref);
-   }
+   int i, max = 0;
+   for (i = 0; i < refs->nr; i++) {
+   struct ref_array_item *it = refs->items[i];
+   const char *desc = it->refname;
+   int w;
 
-   if (upstream_is_gone) {
-   if (show_upstream_ref)
-   strbuf_addf(stat, _("[%s: gone]"), fancy.buf);
-   else
-   added_decoration = 0;
-   } else if (!ours && !theirs) {
-   if (show_upstream_ref)
-   strbuf_addf(stat, _("[%s]"), fancy.buf);
-   else
-   added_decoration = 0;
-   } else if (!ours) {
-   if (show_upstream_ref)
-   strbuf_addf(stat, _("[%s: behind %d]"), fancy.buf, 
theirs);
-   else
-   strbuf_addf(stat, _("[behind %d]"), theirs);
+   skip_prefix(it->refname, "refs/heads/", );
+   skip_prefix(it->refname, "refs/remotes/", );
+   if (it->kind == FILTER_REFS_DETACHED_HEAD) {
+   char *head_desc = get_head_description();
+   w = utf8_strwidth(head_desc);
+   free(head_desc);
+   } else
+   w = utf8_strwidth(desc);
 
-   } else if (!theirs) {
-   if 

[PATCH v9 14/20] ref-filter: Do not abruptly die when using the 'lstrip=' option

[Karthik Nayak]

Currently when we use the 'lstrip=' option, if 'N' is greater than
the number of components available in the refname, we abruptly end
program execution by calling die().

This behavior is undesired since a single refname with few components
could end program execution. To avoid this, return an empty string
whenever the value 'N' is greater than the number of components
available, instead of calling die().

Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt | 3 +--
 ref-filter.c   | 3 +--
 t/t6300-for-each-ref.sh| 4 
 3 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 46b4583..82c202a 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -98,8 +98,7 @@ refname::
abbreviation mode. If `lstrip=` is appended, strips ``
slash-separated path components from the front of the refname
(e.g., `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo`.
-   `` must be a positive integer.  If a displayed ref has fewer
-   components than ``, the command aborts with an error.
+   `` must be a positive integer.
 
 objecttype::
The type of the object (`blob`, `tree`, `commit`, `tag`).
diff --git a/ref-filter.c b/ref-filter.c
index e0015c6..76553eb 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1099,8 +1099,7 @@ static const char *lstrip_ref_components(const char 
*refname, unsigned int len)
while (remaining) {
switch (*start++) {
case '\0':
-   die(_("ref '%s' does not have %ud components to 
:lstrip"),
-   refname, len);
+   return "";
case '/':
remaining--;
break;
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 5eb013c..d3d1a97 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -147,10 +147,6 @@ test_expect_success 'arguments to :lstrip must be positive 
integers' '
test_must_fail git for-each-ref --format="%(refname:lstrip=foo)"
 '
 
-test_expect_success 'stripping refnames too far gives an error' '
-   test_must_fail git for-each-ref --format="%(refname:lstrip=3)"
-'
-
 test_expect_success 'Check format specifiers are ignored in naming date atoms' 
'
git for-each-ref --format="%(authordate)" refs/heads &&
git for-each-ref --format="%(authordate:default) %(authordate)" 
refs/heads &&
-- 
2.10.2

[PATCH v9 18/20] branch, tag: use porcelain output

[Karthik Nayak]

From: Karthik Nayak 

Call ref-filter's setup_ref_filter_porcelain_msg() to enable
translated messages for the %(upstream:tack) atom. Although branch.c
doesn't currently use ref-filter's printing API's, this will ensure
that when it does in the future patches, we do not need to worry about
translation.

Written-by: Matthieu Moy 
Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 builtin/branch.c | 2 ++
 builtin/tag.c| 2 ++
 2 files changed, 4 insertions(+)

diff --git a/builtin/branch.c b/builtin/branch.c
index 6423ebc..34cd61c 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -649,6 +649,8 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPT_END(),
};
 
+   setup_ref_filter_porcelain_msg();
+
memset(, 0, sizeof(filter));
filter.kind = FILTER_REFS_BRANCHES;
filter.abbrev = -1;
diff --git a/builtin/tag.c b/builtin/tag.c
index b4789ce..8a1a476 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -375,6 +375,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_END()
};
 
+   setup_ref_filter_porcelain_msg();
+
git_config(git_tag_config, sorting_tail);
 
memset(, 0, sizeof(opt));
-- 
2.10.2

[PATCH v9 06/20] ref-filter: introduce format_ref_array_item()

[Karthik Nayak]

From: Karthik Nayak 

To allow column display, we will need to first render the output in a
string list to allow print_columns() to compute the proper size of
each column before starting the actual output. Introduce the function
format_ref_array_item() that does the formatting of a ref_array_item
to an strbuf.

show_ref_array_item() is kept as a convenience wrapper around it which
obtains the strbuf and prints it the standard output.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c | 16 
 ref-filter.h |  3 +++
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 5511a20..47b521c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1833,10 +1833,10 @@ static void append_literal(const char *cp, const char 
*ep, struct ref_formatting
}
 }
 
-void show_ref_array_item(struct ref_array_item *info, const char *format, int 
quote_style)
+void format_ref_array_item(struct ref_array_item *info, const char *format,
+  int quote_style, struct strbuf *final_buf)
 {
const char *cp, *sp, *ep;
-   struct strbuf *final_buf;
struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
 
state.quote_style = quote_style;
@@ -1866,9 +1866,17 @@ void show_ref_array_item(struct ref_array_item *info, 
const char *format, int qu
}
if (state.stack->prev)
die(_("format: %%(end) atom missing"));
-   final_buf = >output;
-   fwrite(final_buf->buf, 1, final_buf->len, stdout);
+   strbuf_addbuf(final_buf, >output);
pop_stack_element();
+}
+
+void show_ref_array_item(struct ref_array_item *info, const char *format, int 
quote_style)
+{
+   struct strbuf final_buf = STRBUF_INIT;
+
+   format_ref_array_item(info, format, quote_style, _buf);
+   fwrite(final_buf.buf, 1, final_buf.len, stdout);
+   strbuf_release(_buf);
putchar('\n');
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index f78323d..630e7c2 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -100,6 +100,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep);
 int verify_ref_format(const char *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
+/*  Based on the given format and quote_style, fill the strbuf */
+void format_ref_array_item(struct ref_array_item *info, const char *format,
+  int quote_style, struct strbuf *final_buf);
 /*  Print the ref using the given format and quote_style */
 void show_ref_array_item(struct ref_array_item *info, const char *format, int 
quote_style);
 /*  Callback function for parsing the sort option */
-- 
2.10.2

[PATCH v9 07/20] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams

[Karthik Nayak]

From: Karthik Nayak 

Borrowing from branch.c's implementation print "[gone]" whenever an
unknown upstream ref is encountered instead of just ignoring it.

This makes sure that when branch.c is ported over to using ref-filter
APIs for printing, this feature is not lost.

Make changes to t/t6300-for-each-ref.sh and
Documentation/git-for-each-ref.txt to reflect this change.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Helped-by : Jacob Keller 
Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt | 3 ++-
 ref-filter.c   | 4 +++-
 t/t6300-for-each-ref.sh| 2 +-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index c2cc03c..24a1679 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -121,7 +121,8 @@ upstream::
"[ahead N, behind M]" and `:trackshort` to show the terse
version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
or "=" (in sync).  Has no effect if the ref does not have
-   tracking information associated with it.
+   tracking information associated with it. `:track` also prints
+   "[gone]" whenever unknown upstream ref is encountered.
 
 push::
The name of a local ref which represents the `@{push}` location
diff --git a/ref-filter.c b/ref-filter.c
index 47b521c..9989918 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1073,8 +1073,10 @@ static void fill_remote_ref_details(struct used_atom 
*atom, const char *refname,
*s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
else if (atom->u.remote_ref == RR_TRACK) {
if (stat_tracking_info(branch, _ours,
-  _theirs, NULL))
+  _theirs, NULL)) {
+   *s = "[gone]";
return;
+   }
 
if (!num_ours && !num_theirs)
*s = "";
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index e67c694..a2e3f55 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -382,7 +382,7 @@ test_expect_success 'Check that :track[short] cannot be 
used with other atoms' '
 
 test_expect_success 'Check that :track[short] works when upstream is invalid' '
cat >expected <<-\EOF &&
-
+   [gone]
 
EOF
test_when_finished "git config branch.master.merge refs/heads/master" &&
-- 
2.10.2

[PATCH v9 09/20] ref-filter: make "%(symref)" atom work with the ':short' modifier

[Karthik Nayak]

From: Karthik Nayak 

The "%(symref)" atom doesn't work when used with the ':short' modifier
because we strictly match only 'symref' for setting the 'need_symref'
indicator. Fix this by comparing with the valid_atom rather than the
used_atom.

Add tests for %(symref) and %(symref:short) while we're here.

Helped-by: Junio C Hamano 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c|  2 +-
 t/t6300-for-each-ref.sh | 24 
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index 6de0927..e98ef4b 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -352,7 +352,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
valid_atom[i].parser(_atom[at], arg);
if (*atom == '*')
need_tagged = 1;
-   if (!strcmp(used_atom[at].name, "symref"))
+   if (!strcmp(valid_atom[i].name, "symref"))
need_symref = 1;
return at;
 }
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index af76dc5..7663a36 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -38,6 +38,7 @@ test_atom() {
case "$1" in
head) ref=refs/heads/master ;;
 tag) ref=refs/tags/testtag ;;
+sym) ref=refs/heads/sym ;;
   *) ref=$1 ;;
esac
printf '%s\n' "$3" >expected
@@ -566,6 +567,7 @@ test_expect_success 'Verify sort with multiple keys' '
test_cmp expected actual
 '
 
+
 test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '
test_when_finished "git checkout master" &&
git for-each-ref --format="%(HEAD) %(refname:short)" refs/heads/ 
>actual &&
@@ -600,4 +602,26 @@ test_expect_success 'basic atom: head contents:trailers' '
test_cmp expect actual.clean
 '
 
+test_expect_success 'Add symbolic ref for the following tests' '
+   git symbolic-ref refs/heads/sym refs/heads/master
+'
+
+cat >expected expected 

[PATCH v9 04/20] ref-filter: modify "%(objectname:short)" to take length

[Karthik Nayak]

From: Karthik Nayak 

Add support for %(objectname:short=) which would print the
abbreviated unique objectname of given length. When no length is
specified, the length is 'DEFAULT_ABBREV'. The minimum length is
'MINIMUM_ABBREV'. The length may be exceeded to ensure that the
provided object name is unique.

Add tests and documentation for the same.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Helped-by: Jacob Keller 
Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt |  3 +++
 ref-filter.c   | 25 +++--
 t/t6300-for-each-ref.sh| 10 ++
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 39aab09..c2cc03c 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -110,6 +110,9 @@ objectsize::
 objectname::
The object name (aka SHA-1).
For a non-ambiguous abbreviation of the object name append `:short`.
+   For an abbreviation of the object name with desired length append
+   `:short=`, where the minimum length is MINIMUM_ABBREV. The
+   length may be exceeded to ensure unique object names.
 
 upstream::
The name of a local ref which can be considered ``upstream''
diff --git a/ref-filter.c b/ref-filter.c
index e002629..385fc04 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -57,7 +57,10 @@ static struct used_atom {
cmp_status cmp_status;
const char *str;
} if_then_else;
-   enum { O_FULL, O_SHORT } objectname;
+   struct {
+   enum { O_FULL, O_LENGTH, O_SHORT } option;
+   unsigned int length;
+   } objectname;
} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -129,10 +132,17 @@ static void contents_atom_parser(struct used_atom *atom, 
const char *arg)
 static void objectname_atom_parser(struct used_atom *atom, const char *arg)
 {
if (!arg)
-   atom->u.objectname = O_FULL;
+   atom->u.objectname.option = O_FULL;
else if (!strcmp(arg, "short"))
-   atom->u.objectname = O_SHORT;
-   else
+   atom->u.objectname.option = O_SHORT;
+   else if (skip_prefix(arg, "short=", )) {
+   atom->u.objectname.option = O_LENGTH;
+   if (strtoul_ui(arg, 10, >u.objectname.length) ||
+   atom->u.objectname.length == 0)
+   die(_("positive value expected objectname:short=%s"), 
arg);
+   if (atom->u.objectname.length < MINIMUM_ABBREV)
+   atom->u.objectname.length = MINIMUM_ABBREV;
+   } else
die(_("unrecognized %%(objectname) argument: %s"), arg);
 }
 
@@ -606,12 +616,15 @@ static int grab_objectname(const char *name, const 
unsigned char *sha1,
   struct atom_value *v, struct used_atom *atom)
 {
if (starts_with(name, "objectname")) {
-   if (atom->u.objectname == O_SHORT) {
+   if (atom->u.objectname.option == O_SHORT) {
v->s = xstrdup(find_unique_abbrev(sha1, 
DEFAULT_ABBREV));
return 1;
-   } else if (atom->u.objectname == O_FULL) {
+   } else if (atom->u.objectname.option == O_FULL) {
v->s = xstrdup(sha1_to_hex(sha1));
return 1;
+   } else if (atom->u.objectname.option == O_LENGTH) {
+   v->s = xstrdup(find_unique_abbrev(sha1, 
atom->u.objectname.length));
+   return 1;
} else
die("BUG: unknown %%(objectname) option");
}
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index aea1dfc..e67c694 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -60,6 +60,8 @@ test_atom head objecttype commit
 test_atom head objectsize 171
 test_atom head objectname $(git rev-parse refs/heads/master)
 test_atom head objectname:short $(git rev-parse --short refs/heads/master)
+test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master)
+test_atom head objectname:short=10 $(git rev-parse --short=10 
refs/heads/master)
 test_atom head tree $(git rev-parse refs/heads/master^{tree})
 test_atom head parent ''
 test_atom head numparent 0
@@ -99,6 +101,8 @@ test_atom tag objecttype tag
 test_atom tag objectsize 154
 test_atom tag objectname $(git rev-parse refs/tags/testtag)
 test_atom tag objectname:short $(git rev-parse --short refs/tags/testtag)
+test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master)
+test_atom head objectname:short=10 $(git rev-parse --short=10 
refs/heads/master)
 

[PATCH v9 20/20] branch: implement '--format' option

[Karthik Nayak]

From: Karthik Nayak 

Implement the '--format' option provided by 'ref-filter'. This lets the
user list branches as per desired format similar to the implementation
in 'git for-each-ref'.

Add tests and documentation for the same.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 Documentation/git-branch.txt |  7 ++-
 builtin/branch.c | 14 +-
 t/t3203-branch-output.sh | 14 ++
 3 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 5516a47..1fae4ee 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -12,7 +12,7 @@ SYNOPSIS
[--list] [-v [--abbrev= | --no-abbrev]]
[--column[=] | --no-column]
[(--merged | --no-merged | --contains) []] [--sort=]
-   [--points-at ] [...]
+   [--points-at ] [--format=] [...]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f]  
[]
 'git branch' (--set-upstream-to= | -u ) []
 'git branch' --unset-upstream []
@@ -250,6 +250,11 @@ start-point is either a local or remote-tracking branch.
 --points-at ::
Only list branches of the given object.
 
+--format ::
+   A string that interpolates `%(fieldname)` from the object
+   pointed at by a ref being shown.  The format is the same as
+   that of linkgit:git-for-each-ref[1].
+
 Examples
 
 
diff --git a/builtin/branch.c b/builtin/branch.c
index 2887545..4051a18 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -28,6 +28,7 @@ static const char * const builtin_branch_usage[] = {
N_("git branch [] [-r] (-d | -D) ..."),
N_("git branch [] (-m | -M) [] "),
N_("git branch [] [-r | -a] [--points-at]"),
+   N_("git branch [] [-r | -a] [--format]"),
NULL
 };
 
@@ -364,14 +365,14 @@ static char *build_format(struct ref_filter *filter, int 
maxwidth, const char *r
return strbuf_detach(, NULL);
 }
 
-static void print_ref_list(struct ref_filter *filter, struct ref_sorting 
*sorting)
+static void print_ref_list(struct ref_filter *filter, struct ref_sorting 
*sorting, const char *format)
 {
int i;
struct ref_array array;
int maxwidth = 0;
const char *remote_prefix = "";
struct strbuf out = STRBUF_INIT;
-   char *format;
+   char *to_free = NULL;
 
/*
 * If we are listing more than just remote branches,
@@ -388,7 +389,8 @@ static void print_ref_list(struct ref_filter *filter, 
struct ref_sorting *sortin
if (filter->verbose)
maxwidth = calc_maxwidth(, strlen(remote_prefix));
 
-   format = build_format(filter, maxwidth, remote_prefix);
+   if (!format)
+   format = to_free = build_format(filter, maxwidth, 
remote_prefix);
verify_ref_format(format);
 
ref_array_sort(sorting, );
@@ -407,7 +409,7 @@ static void print_ref_list(struct ref_filter *filter, 
struct ref_sorting *sortin
}
 
ref_array_clear();
-   free(format);
+   free(to_free);
 }
 
 static void reject_rebase_or_bisect_branch(const char *target)
@@ -528,6 +530,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
struct ref_filter filter;
int icase = 0;
static struct ref_sorting *sorting = NULL, **sorting_tail = 
+   const char *format = NULL;
 
struct option options[] = {
OPT_GROUP(N_("Generic options")),
@@ -569,6 +572,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
N_("print only branches of the object"), 0, 
parse_opt_object_name
},
OPT_BOOL('i', "ignore-case", , N_("sorting and filtering 
are case insensitive")),
+   OPT_STRING(  0 , "format", , N_("format"), N_("format to 
use for the output")),
OPT_END(),
};
 
@@ -641,7 +645,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
if (!sorting)
sorting = ref_default_sorting();
sorting->ignore_case = icase;
-   print_ref_list(, sorting);
+   print_ref_list(, sorting, format);
print_columns(, colopts, NULL);
string_list_clear(, 0);
return 0;
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 4521328..5778c0a 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -225,4 +225,18 @@ test_expect_success 'sort branches, ignore case' '
)
 '
 
+test_expect_success 'git branch --format option' '
+   cat >expect <<-\EOF &&
+   Refname is (HEAD detached from fromtag)
+   Refname is refs/heads/ambiguous
+   Refname is refs/heads/branch-one
+   Refname is refs/heads/branch-two
+   Refname is refs/heads/master
+

[PATCH v9 01/20] ref-filter: implement %(if), %(then), and %(else) atoms

[Karthik Nayak]

From: Karthik Nayak 

Implement %(if), %(then) and %(else) atoms. Used as
%(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the
format string between %(if) and %(then) expands to an empty string, or
to only whitespaces, then the whole %(if)...%(end) expands to the string
following %(then). Otherwise, it expands to the string following
%(else), if any. Nesting of this construct is possible.

This is in preparation for porting over `git branch -l` to use
ref-filter APIs for printing.

Add Documentation and tests regarding the same.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt |  34 ++
 ref-filter.c   | 134 +++--
 t/t6302-for-each-ref-filter.sh |  76 +
 3 files changed, 237 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index abe13f3..5e80c34 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -149,6 +149,16 @@ align::
quoted, but if nested then only the topmost level performs
quoting.
 
+if::
+   Used as %(if)...%(then)...%(end) or
+   %(if)...%(then)...%(else)...%(end).  If there is an atom with
+   value or string literal after the %(if) then everything after
+   the %(then) is printed, else if the %(else) atom is used, then
+   everything after %(else) is printed. We ignore space when
+   evaluating the string before %(then), this is useful when we
+   use the %(HEAD) atom which prints either "*" or " " and we
+   want to apply the 'if' condition only on the 'HEAD' ref.
+
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
 be used to specify the value in the header field.
@@ -186,6 +196,14 @@ As a special case for the date-type fields, you may 
specify a format for
 the date by adding `:` followed by date format name (see the
 values the `--date` option to linkgit:git-rev-list[1] takes).
 
+Some atoms like %(align) and %(if) always require a matching %(end).
+We call them "opening atoms" and sometimes denote them as %($open).
+
+When a scripting language specific quoting is in effect, everything
+between a top-level opening atom and its matching %(end) is evaluated
+according to the semantics of the opening atom and its result is
+quoted.
+
 
 EXAMPLES
 
@@ -273,6 +291,22 @@ eval=`git for-each-ref --shell --format="$fmt" \
 eval "$eval"
 
 
+
+An example to show the usage of %(if)...%(then)...%(else)...%(end).
+This prefixes the current branch with a star.
+
+
+git for-each-ref --format="%(if)%(HEAD)%(then)* %(else)  
%(end)%(refname:short)" refs/heads/
+
+
+
+An example to show the usage of %(if)...%(then)...%(end).
+This prints the authorname, if present.
+
+
+git for-each-ref --format="%(refname)%(if)%(authorname)%(then) 
%(color:red)Authored by: %(authorname)%(end)"
+
+
 SEE ALSO
 
 linkgit:git-show-ref[1]
diff --git a/ref-filter.c b/ref-filter.c
index 1a97840..0a57872 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -22,6 +22,12 @@ struct align {
unsigned int width;
 };
 
+struct if_then_else {
+   unsigned int then_atom_seen : 1,
+   else_atom_seen : 1,
+   condition_satisfied : 1;
+};
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().
@@ -214,6 +220,9 @@ static struct {
{ "color", FIELD_STR, color_atom_parser },
{ "align", FIELD_STR, align_atom_parser },
{ "end" },
+   { "if" },
+   { "then" },
+   { "else" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -221,7 +230,7 @@ static struct {
 struct ref_formatting_stack {
struct ref_formatting_stack *prev;
struct strbuf output;
-   void (*at_end)(struct ref_formatting_stack *stack);
+   void (*at_end)(struct ref_formatting_stack **stack);
void *at_end_data;
 };
 
@@ -354,13 +363,14 @@ static void pop_stack_element(struct ref_formatting_stack 
**stack)
*stack = prev;
 }
 
-static void end_align_handler(struct ref_formatting_stack *stack)
+static void end_align_handler(struct ref_formatting_stack **stack)
 {
-   struct align *align = (struct align *)stack->at_end_data;
+   struct ref_formatting_stack *cur = *stack;
+   struct align *align = (struct align *)cur->at_end_data;
struct strbuf s = STRBUF_INIT;
 
-   strbuf_utf8_align(, align->position, align->width, stack->output.buf);
-   strbuf_swap(>output, );
+   strbuf_utf8_align(, align->position, align->width, cur->output.buf);
+   strbuf_swap(>output, );
strbuf_release();
 }
 
@@ -374,6 

[PATCH v9 03/20] ref-filter: implement %(if:equals=) and %(if:notequals=)

[Karthik Nayak]

From: Karthik Nayak 

Implement %(if:equals=) wherein the if condition is only
satisfied if the value obtained between the %(if:...) and %(then) atom
is the same as the given ''.

Similarly, implement (if:notequals=) wherein the if condition
is only satisfied if the value obtained between the %(if:...) and
%(then) atom is different from the given ''.

This is done by introducing 'if_atom_parser()' which parses the given
%(if) atom and then stores the data in used_atom which is later passed
on to the used_atom of the %(then) atom, so that it can do the required
comparisons.

Add tests and Documentation for the same.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt |  3 +++
 ref-filter.c   | 46 +-
 t/t6302-for-each-ref-filter.sh | 18 +++
 3 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 5e80c34..39aab09 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -158,6 +158,9 @@ if::
evaluating the string before %(then), this is useful when we
use the %(HEAD) atom which prints either "*" or " " and we
want to apply the 'if' condition only on the 'HEAD' ref.
+   Append ":equals=" or ":notequals=" to compare
+   the value between the %(if:...) and %(then) atoms with the
+   given string.
 
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
diff --git a/ref-filter.c b/ref-filter.c
index f31c4b6..e002629 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -16,6 +16,7 @@
 #include "trailer.h"
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
+typedef enum { COMPARE_EQUAL, COMPARE_UNEQUAL, COMPARE_NONE } cmp_status;
 
 struct align {
align_type position;
@@ -23,6 +24,8 @@ struct align {
 };
 
 struct if_then_else {
+   cmp_status cmp_status;
+   const char *str;
unsigned int then_atom_seen : 1,
else_atom_seen : 1,
condition_satisfied : 1;
@@ -50,6 +53,10 @@ static struct used_atom {
enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, 
C_SUB, C_TRAILERS } option;
unsigned int nlines;
} contents;
+   struct {
+   cmp_status cmp_status;
+   const char *str;
+   } if_then_else;
enum { O_FULL, O_SHORT } objectname;
} u;
 } *used_atom;
@@ -179,6 +186,21 @@ static void align_atom_parser(struct used_atom *atom, 
const char *arg)
string_list_clear(, 0);
 }
 
+static void if_atom_parser(struct used_atom *atom, const char *arg)
+{
+   if (!arg) {
+   atom->u.if_then_else.cmp_status = COMPARE_NONE;
+   return;
+   } else if (skip_prefix(arg, "equals=", >u.if_then_else.str)) {
+   atom->u.if_then_else.cmp_status = COMPARE_EQUAL;
+   } else if (skip_prefix(arg, "notequals=", >u.if_then_else.str)) {
+   atom->u.if_then_else.cmp_status = COMPARE_UNEQUAL;
+   } else {
+   die(_("unrecognized %%(if) argument: %s"), arg);
+   }
+}
+
+
 static struct {
const char *name;
cmp_type cmp_type;
@@ -220,7 +242,7 @@ static struct {
{ "color", FIELD_STR, color_atom_parser },
{ "align", FIELD_STR, align_atom_parser },
{ "end" },
-   { "if" },
+   { "if", FIELD_STR, if_atom_parser },
{ "then" },
{ "else" },
 };
@@ -422,6 +444,9 @@ static void if_atom_handler(struct atom_value *atomv, 
struct ref_formatting_stat
struct ref_formatting_stack *new;
struct if_then_else *if_then_else = xcalloc(sizeof(struct 
if_then_else), 1);
 
+   if_then_else->str = atomv->atom->u.if_then_else.str;
+   if_then_else->cmp_status = atomv->atom->u.if_then_else.cmp_status;
+
push_stack_element(>stack);
new = state->stack;
new->at_end = if_then_else_handler;
@@ -453,10 +478,17 @@ static void then_atom_handler(struct atom_value *atomv, 
struct ref_formatting_st
die(_("format: %%(then) atom used after %%(else)"));
if_then_else->then_atom_seen = 1;
/*
-* If there exists non-empty string between the 'if' and
-* 'then' atom then the 'if' condition is satisfied.
+* If the 'equals' or 'notequals' attribute is used then
+* perform the required comparison. If not, only non-empty
+* strings satisfy the 'if' condition.
 */
-   if (cur->output.len && !is_empty(cur->output.buf))
+   if (if_then_else->cmp_status == COMPARE_EQUAL) {
+   if (!strcmp(if_then_else->str, cur->output.buf))
+  

[PATCH v9 02/20] ref-filter: include reference to 'used_atom' within 'atom_value'

[Karthik Nayak]

From: Karthik Nayak 

Ensure that each 'atom_value' has a reference to its corresponding
'used_atom'. This let's us use values within 'used_atom' in the
'handler' function.

Hence we can get the %(align) atom's parameters directly from the
'used_atom' therefore removing the necessity of passing %(align) atom's
parameters to 'atom_value'.

This also acts as a preparatory patch for the upcoming patch where we
introduce %(if:equals=) and %(if:notequals=).

Signed-off-by: Karthik Nayak 
---
 ref-filter.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 0a57872..f31c4b6 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -241,11 +241,9 @@ struct ref_formatting_state {
 
 struct atom_value {
const char *s;
-   union {
-   struct align align;
-   } u;
void (*handler)(struct atom_value *atomv, struct ref_formatting_state 
*state);
unsigned long ul; /* used for sorting when not FIELD_STR */
+   struct used_atom *atom;
 };
 
 /*
@@ -381,7 +379,7 @@ static void align_atom_handler(struct atom_value *atomv, 
struct ref_formatting_s
push_stack_element(>stack);
new = state->stack;
new->at_end = end_align_handler;
-   new->at_end_data = >u.align;
+   new->at_end_data = >atom->u.align;
 }
 
 static void if_then_else_handler(struct ref_formatting_stack **stack)
@@ -1090,6 +1088,7 @@ static void populate_value(struct ref_array_item *ref)
struct branch *branch = NULL;
 
v->handler = append_atom;
+   v->atom = atom;
 
if (*name == '*') {
deref = 1;
@@ -1154,7 +1153,6 @@ static void populate_value(struct ref_array_item *ref)
v->s = " ";
continue;
} else if (starts_with(name, "align")) {
-   v->u.align = atom->u.align;
v->handler = align_atom_handler;
continue;
} else if (!strcmp(name, "end")) {
-- 
2.10.2

[PATCH v9 00/20] port branch.c to use ref-filter's printing options

[Karthik Nayak]

This is part of unification of the commands 'git tag -l, git branch -l
and git for-each-ref'. This ports over branch.c to use ref-filter's
printing options.

Initially posted here: $(gmane/279226). It was decided that this series
would follow up after refactoring ref-filter parsing mechanism, which
is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c).

v1 can be found here: $(gmane/288342)
v2 can be found here: $(gmane/288863)
v3 can be found here: $(gmane/290299)
v4 can be found here: $(gmane/291106)
v5b can be found here: $(gmane/292467)
v6 can be found here: http://marc.info/?l=git=146330914118766=2
v7 can be found here: http://marc.info/?l=git=147863593317362=2
v8 can be found here: http://marc.info/?l=git=148112502029302=2

Changes in this version:
1. A few formatting errors.
2. Made quote_literal_for_format() static.
3. lstrip and rstrip doesn't die on less components.

Thanks Jacob, Junio, Jeff, Ramsay for their suggestions and help.

Karthik Nayak (20):
  ref-filter: implement %(if), %(then), and %(else) atoms
  ref-filter: include reference to 'used_atom' within 'atom_value'
  ref-filter: implement %(if:equals=) and
%(if:notequals=)
  ref-filter: modify "%(objectname:short)" to take length
  ref-filter: move get_head_description() from branch.c
  ref-filter: introduce format_ref_array_item()
  ref-filter: make %(upstream:track) prints "[gone]" for invalid
upstreams
  ref-filter: add support for %(upstream:track,nobracket)
  ref-filter: make "%(symref)" atom work with the ':short' modifier
  ref-filter: introduce refname_atom_parser_internal()
  ref-filter: introduce refname_atom_parser()
  ref-filter: make remote_ref_atom_parser() use
refname_atom_parser_internal()
  ref-filter: rename the 'strip' option to 'lstrip'
  ref-filter: Do not abruptly die when using the 'lstrip=' option
  ref-filter: modify the 'lstrip=' option to work with negative ''
  ref-filter: add an 'rstrip=' option to atoms which deal with
refnames
  ref-filter: allow porcelain to translate messages in the output
  branch, tag: use porcelain output
  branch: use ref-filter printing APIs
  branch: implement '--format' option

 Documentation/git-branch.txt   |   7 +-
 Documentation/git-for-each-ref.txt |  86 +--
 builtin/branch.c   | 290 +++---
 builtin/tag.c  |   6 +-
 ref-filter.c   | 490 +++--
 ref-filter.h   |   7 +
 t/t3203-branch-output.sh   |  16 +-
 t/t6040-tracking-info.sh   |   2 +-
 t/t6300-for-each-ref.sh|  88 ++-
 t/t6302-for-each-ref-filter.sh |  94 +++
 10 files changed, 780 insertions(+), 306 deletions(-)

Interdiff:

diff --git a/builtin/branch.c b/builtin/branch.c
index 6393c3c..4051a18 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -306,9 +306,9 @@ static int calc_maxwidth(struct ref_array *refs, int 
remote_bonus)
return max;
 }

-const char *quote_literal_for_format(const char *s)
+static const char *quote_literal_for_format(const char *s)
 {
-   struct strbuf buf = STRBUF_INIT;
+   static struct strbuf buf = STRBUF_INIT;

strbuf_reset();
while (*s) {
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -76,10 +77,10 @@ static struct used_atom {
struct {
enum { RR_REF, RR_TRACK, RR_TRACKSHORT } option;
struct refname_atom refname;
-   unsigned int nobracket: 1;
+   unsigned int nobracket : 1;
} remote_ref;
@@ -1106,7 +1126,8 @@ static const char *lstrip_ref_components(const char 
*refname, int len)
const char *p = refname;

/* Find total no of '/' separated path-components */
-   for (i = 0; p[i]; p[i] == '/' ? i++ : *p++);
+   for (i = 0; p[i]; p[i] == '/' ? i++ : *p++)
+   ;
/*
 * The number of components we need to strip is now
 * the total minus the components to be left (Plus one
@@ -1116,11 +1137,10 @@ static const char *lstrip_ref_components(const char 
*refname, int len)
remaining = i + len + 1;
}

-   while (remaining) {
+   while (remaining > 0) {
switch (*start++) {
case '\0':
-   die(_("ref '%s' does not have %d components to 
:lstrip"),
-   refname, len);
+   return "";
case '/':
remaining--;
break;
@@ -1140,7 +1160,8 @@ static const char *rstrip_ref_components(const char 
*refname, int len)
const char *p = refname;

/* Find total no of '/' separated path-components */
-   for (i = 0; p[i]; p[i] == '/' ? i++ : *p++);
+   for (i = 0; p[i]; p[i] == '/' ? i++ : *p++)
+ 

Gitview Shell Injection Vulnerability

[Javantea]

I have found a shell injection vulnerability in contrib/gitview/gitview.

Gitview Shell Injection Vulnerability

Versions affected: 8cb711c8a5-1d1bdafd64 (<=2.11.0)

Gitview executes shell commands using string concatenation with user supplied 
data, filenames and branch names. Running Gitview and interacting with the user 
interface with a malicious filename or branch name in the current repository 
results in malicious commands being executed as the current user.

AnnotateWindow.add_file_data(self, filename, commit_sha1, line_num):
fp = os.popen("git cat-file blob " + commit_sha1 +":"+filename)

AnnotateWindow.annotate(self, filename, commit_sha1, line_num):
fp = os.popen("git ls-tree "+ commit_sha1 + " -- " + filename)
fp = os.popen("git blame --incremental -C -C -- " + filename + " " + 
commit_sha1)

GitView.set_branch(self, args):
fp = os.popen("git rev-parse --sq --default HEAD " + list_to_string(args, 
1))
fp = os.popen("git rev-list  --header --topo-order --parents " + 
git_rev_list_cmd)

The program also has other uses of os.popen but none use values that the user 
can manipulate. However, the fix should definitely replace these instances so 
that the code might one day pass pylint and manual code review easier.

The function os.popen has been replaced by safer functions in the subprocess 
module. The code can be improved easily because it requires very little change 
to convert the code to work with arrays of strings instead of strings.

If you have any questions or would like a patch, please let me know.

Regards,
Javantea