[PATCHv2 1/2] builtin/commit.c: drop use snprintf via dynamic allocation

[Elia Pinto]

In general snprintf is bad because it may silently truncate results if we're
wrong. In this patch where we use PATH_MAX, we'd want to handle larger
paths anyway, so we switch to dynamic allocation.

Helped-by: Jeff King 
Signed-off-by: Elia Pinto 
---
This is the second version of the patch.

I have split the original commit in two, as discussed here
http://public-inbox.org/git/20161213132717.42965-1-gitter.spi...@gmail.com/.

 builtin/commit.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 0ed634b26..09bcc0f13 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -960,15 +960,16 @@ static int prepare_to_commit(const char *index_file, 
const char *prefix,
return 0;
 
if (use_editor) {
-   char index[PATH_MAX];
-   const char *env[2] = { NULL };
-   env[0] =  index;
-   snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-   if (launch_editor(git_path_commit_editmsg(), NULL, env)) {
+   struct argv_array env = ARGV_ARRAY_INIT;
+
+   argv_array_pushf(, "GIT_INDEX_FILE=%s", index_file);
+   if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
fprintf(stderr,
_("Please supply the message using either -m or -F 
option.\n"));
+   argv_array_clear();
exit(1);
}
+   argv_array_clear();
}
 
if (!no_verify &&
@@ -1557,23 +1558,22 @@ static int run_rewrite_hook(const unsigned char 
*oldsha1,
 
 int run_commit_hook(int editor_is_used, const char *index_file, const char 
*name, ...)
 {
-   const char *hook_env[3] =  { NULL };
-   char index[PATH_MAX];
+   struct argv_array hook_env = ARGV_ARRAY_INIT;
va_list args;
int ret;
 
-   snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-   hook_env[0] = index;
+   argv_array_pushf(_env, "GIT_INDEX_FILE=%s", index_file);
 
/*
 * Let the hook know that no editor will be launched.
 */
if (!editor_is_used)
-   hook_env[1] = "GIT_EDITOR=:";
+   argv_array_push(_env, "GIT_EDITOR=:");
 
va_start(args, name);
-   ret = run_hook_ve(hook_env, name, args);
+   ret = run_hook_ve(hook_env.argv,name, args);
va_end(args);
+   argv_array_clear(_env);
 
return ret;
 }
-- 
2.11.0.154.g5f5f154

[PATCHv2 2/2] builtin/commit.c: drop use snprintf via dynamic allocation

[Elia Pinto]

In general snprintf is bad because it may silently truncate results
if we're wrong. In this patch, instead of using xnprintf, which asserts
that we don't truncate, we are switching to dynamic allocation, so we can
avoid dealing with magic numbers in the code.

Helped-by: Jeff King  
Signed-off-by: Elia Pinto 
---
This is the second version of the patch.

I have split the original commit in two, as discussed here
http://public-inbox.org/git/20161213132717.42965-1-gitter.spi...@gmail.com/.

 builtin/commit.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 09bcc0f13..37228330c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1526,12 +1526,10 @@ static int git_commit_config(const char *k, const char 
*v, void *cb)
 static int run_rewrite_hook(const unsigned char *oldsha1,
const unsigned char *newsha1)
 {
-   /* oldsha1 SP newsha1 LF NUL */
-   static char buf[2*40 + 3];
+   char *buf;
struct child_process proc = CHILD_PROCESS_INIT;
const char *argv[3];
int code;
-   size_t n;
 
argv[0] = find_hook("post-rewrite");
if (!argv[0])
@@ -1547,11 +1545,11 @@ static int run_rewrite_hook(const unsigned char 
*oldsha1,
code = start_command();
if (code)
return code;
-   n = snprintf(buf, sizeof(buf), "%s %s\n",
-sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
+   buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
sigchain_push(SIGPIPE, SIG_IGN);
-   write_in_full(proc.in, buf, n);
+   write_in_full(proc.in, buf, strlen(buf));
close(proc.in);
+   free(buf);
sigchain_pop(SIGPIPE);
return finish_command();
 }
-- 
2.11.0.154.g5f5f154

Re: [PATCH 2/2] diff: document the pattern format for diff.orderFile

[Junio C Hamano]

Richard Hansen  writes:

> I was looking at the code to see how the two file formats differed and
> noticed that match_order() doesn't set the WM_PATHNAME flag when it
> calls wildmatch().  That's unintentional (a bug), right?

It has been that way from day one IIRC even before we introduced
wildmatch()---IOW it may be intentional that the current code that
uses wildmatch() does not use WM_PATHNAME.

[PATCH v2 0/2] diff orderfile documentation improvements

[Richard Hansen]

Changes from v1:
  * Don't reference gitignore for the file format because they're not
quite the same.

Richard Hansen (2):
  diff: document behavior of relative diff.orderFile
  diff: document the format of the -O (diff.orderFile) file

 Documentation/diff-config.txt  |  7 +++---
 Documentation/diff-options.txt | 54 --
 2 files changed, 56 insertions(+), 5 deletions(-)

-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v2 2/2] diff: document the format of the -O (diff.orderFile) file

[Richard Hansen]

Signed-off-by: Richard Hansen 
---
 Documentation/diff-config.txt  |  5 ++--
 Documentation/diff-options.txt | 54 --
 2 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 875212045..9e4111320 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -99,11 +99,10 @@ diff.noprefix::
If set, 'git diff' does not show any source or destination prefix.
 
 diff.orderFile::
-   File indicating how to order files within a diff, using
-   one shell glob pattern per line.
+   File indicating how to order files within a diff.
+   See the '-O' option to linkgit:git-diff[1] for details.
If `diff.orderFile` is a relative pathname, it is treated as
relative to the top of the work tree.
-   Can be overridden by the '-O' option to linkgit:git-diff[1].
 
 diff.renameLimit::
The number of files to consider when performing the copy/rename
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index e6215c372..e57e9f810 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -466,11 +466,61 @@ information.
 endif::git-format-patch[]
 
 -O::
-   Output the patch in the order specified in the
-   , which has one shell glob pattern per line.
+   Control the order in which files appear in the output.
This overrides the `diff.orderFile` configuration variable
(see linkgit:git-config[1]).  To cancel `diff.orderFile`,
use `-O/dev/null`.
++
+The output order is determined by the order of glob patterns in
+.
+All files with pathnames that match the first pattern are output
+first, all files with pathnames that match the second pattern (but not
+the first) are output next, and so on.
+All files with pathnames that do not match any pattern are output
+last, as if there was an implicit match-all pattern at the end of the
+file.
+If multiple pathnames have the same rank, their output order relative
+to each other is the normal order.
++
+ is parsed as follows:
++
+--
+ - Blank lines are ignored, so they can be used as separators for
+   readability.
+
+ - Lines starting with a hash ("`#`") are ignored, so they can be used
+   for comments.  Add a backslash ("`\`") to the beginning of the
+   pattern if it starts with a hash.
+
+ - Each other line contains a single pattern.
+--
++
+Patterns have the same syntax and semantics as patterns used for
+fnmantch(3) with the FNM_PATHNAME flag, except multiple consecutive
+unescaped asterisks (e.g., "`**`") have a special meaning:
++
+--
+ - A pattern beginning with "`**/`" means match in all directories.
+   For example, "`**/foo`" matches filename "`foo`" anywhere, and
+   "`**/foo/bar`" matches filename "`bar`" anywhere that is directly
+   under directory "`foo`".
+
+ - A pattern ending with "`/**`" matches everything inside a
+   directory, with infinite depth.  For example, "`abc/**`" matches
+   "`abc/def/ghi`" but not "`foo/abc/def`".
+
+ - A slash followed by two consecutive asterisks then a slash
+   ("`/**/`") matches zero or more directory components.  For example,
+   "`a/**/b`" matches "`a/b`", "`a/x/b`", "`a/x/y/b`" and so on.
+
+ - A pattern with more than one consecutive unescaped asterisk is
+   invalid.
+--
++
+In addition, a pathname matches a pattern if the pathname with any
+number of its final pathname components removed matches the pattern.
+For example, the pattern "`foo/*bar`" matches "`foo/asdfbar`" and
+"`foo/bar/baz`" but not "`foo/barx`".
 
 ifndef::git-format-patch[]
 -R::
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v2 1/2] diff: document behavior of relative diff.orderFile

[Richard Hansen]

Document that a relative pathname for diff.orderFile is interpreted as
relative to the top-level work directory.

Signed-off-by: Richard Hansen 
---
 Documentation/diff-config.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 58f4bd6af..875212045 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -101,6 +101,8 @@ diff.noprefix::
 diff.orderFile::
File indicating how to order files within a diff, using
one shell glob pattern per line.
+   If `diff.orderFile` is a relative pathname, it is treated as
+   relative to the top of the work tree.
Can be overridden by the '-O' option to linkgit:git-diff[1].
 
 diff.renameLimit::
-- 
2.11.0.390.gc69c2f50cf-goog

Re: [PATCH 2/2] diff: document the pattern format for diff.orderFile

[Richard Hansen]

On 2017-01-10 15:14, Junio C Hamano wrote:

Richard Hansen  writes:


Document the format of the patterns used for the diff.orderFile
setting and diff's '-O' option by referring the reader to the
gitignore[5] page.

Signed-off-by: Richard Hansen 
---
 Documentation/diff-config.txt  | 3 ++-
 Documentation/diff-options.txt | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 875212045..a35ecdd6b 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -100,7 +100,8 @@ diff.noprefix::

 diff.orderFile::
File indicating how to order files within a diff, using
-   one shell glob pattern per line.
+   one glob pattern per line.
+   See linkgit:gitignore[5] for the pattern format.



I do not think it is wise to suggest referring to gitignore, as the
logic of matching is quite different, other than the fact that they
both use wildmatch() internally.  Also, unlike gitignore, orderfile
does not allow any negative matching i.e. "!".


I was looking at the code to see how the two file formats differed and 
noticed that match_order() doesn't set the WM_PATHNAME flag when it 
calls wildmatch().  That's unintentional (a bug), right?


-Richard





If `diff.orderFile` is a relative pathname, it is treated as
relative to the top of the work tree.
Can be overridden by the '-O' option to linkgit:git-diff[1].
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index e6215c372..dc6b1af71 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -467,7 +467,8 @@ endif::git-format-patch[]

 -O::
Output the patch in the order specified in the
-   , which has one shell glob pattern per line.
+   , which has one glob pattern per line.
+   See linkgit:gitignore[5] for the pattern format.
This overrides the `diff.orderFile` configuration variable
(see linkgit:git-config[1]).  To cancel `diff.orderFile`,
use `-O/dev/null`.

Re: What's cooking in git.git (Jan 2017, #01; Tue, 10)

[Stefan Beller]

On Tue, Jan 10, 2017 at 3:48 PM, Junio C Hamano  wrote:
> Here are the topics that have been cooking.

These two are not included:

A bug fix (regression from rewriting submodule stuff in C)
http://public-inbox.org/git/20170107001953.3196-1-sbel...@google.com/

And another cleanup series
http://public-inbox.org/git/20161227193605.12413-1-sbel...@google.com

I just assume you're still back-logged due to your travel around new year,

Thanks,
Stefan

Re: git cat-file on a submodule

[Stefan Beller]

On Tue, Jan 10, 2017 at 4:11 PM, David Turner  wrote:
> Why does git cat-file -t $sha:foo, where foo is a submodule, not work?
>
> git rev-parse $sha:foo works.
>
> By "why", I mean "would anyone complain if I fixed it?"

$ git log -- builtin/cat-file.c |grep -i -e gitlink -e submodule
$ # no result

I think nobody cared so far. Go for it!

> FWIW, I think
> -p should just return the submodule's sha.

That sounds right as the sha1 is also printed for the tree already, i.e.
in Gerrit you can get the submodules via
$ git cat-file -p HEAD:plugins/
100644 blob c6bb7f182440d6ab860bbcfadc9901b0d94d1ee3 BUCK
16 commit 9b163e113de9f3a49219a02d388f7f46ea2559d3
commit-message-length-validator
16 commit 69b8f9f413ce83a71593a4068a3b8e81f684cbad cookbook-plugin
16 commit 7b41f3a413b46140b050ae5324cbbcdd467d2b3a download-commands
16 commit 3acc14d10d26678eae6489038fe0d4dad644a9b4 hooks
16 commit c5123d6a5604cc740d6f42485235c0d3ec141c4e replication
16 commit 3f3d572e9618f268b19cc54856deee4c96180e4c reviewnotes
16 commit 3ca1167edda713f4bfdcecd9c0e2626797d7027f singleusergroup

"commit " is the correct answer already :)

Thanks,
Stefan

git cat-file on a submodule

[David Turner]

Why does git cat-file -t $sha:foo, where foo is a submodule, not work? 

git rev-parse $sha:foo works.  

By "why", I mean "would anyone complain if I fixed it?"  FWIW, I think
-p should just return the submodule's sha.

What's cooking in git.git (Jan 2017, #01; Tue, 10)

[Junio C Hamano]

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

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

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

--
[Graduated to "master"]

* bw/grep-recurse-submodules (2016-12-22) 12 commits
  (merged to 'next' on 2016-12-22 at 1ede815b8d)
 + grep: search history of moved submodules
 + grep: enable recurse-submodules to work on  objects
 + grep: optionally recurse into submodules
 + grep: add submodules as a grep source type
 + submodules: load gitmodules file from commit sha1
 + submodules: add helper to determine if a submodule is initialized
 + submodules: add helper to determine if a submodule is populated
  (merged to 'next' on 2016-12-22 at fea8fa870f)
 + real_path: canonicalize directory separators in root parts
 + real_path: have callers use real_pathdup and strbuf_realpath
 + real_path: create real_pathdup
 + real_path: convert real_path_internal to strbuf_realpath
 + real_path: resolve symlinks by hand
 (this branch is tangled with bw/realpath-wo-chdir.)

 "git grep" learns to optionally recurse into submodules.


* dt/smart-http-detect-server-going-away (2016-11-18) 2 commits
  (merged to 'next' on 2016-12-05 at 3ea70d01af)
 + upload-pack: optionally allow fetching any sha1
 + remote-curl: don't hang when a server dies before any output

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

 When the http server gives an incomplete response to a smart-http
 rpc call, it could lead to client waiting for a full response that
 will never come.  Teach the client side to notice this condition
 and abort the transfer.

 An improvement counterproposal has failed.
 cf. <20161114194049.mktpsvgdhex2f...@sigill.intra.peff.net>


* jc/abbrev-autoscale-config (2016-12-22) 1 commit
  (merged to 'next' on 2016-12-27 at 631e4200e2)
 + config.abbrev: document the new default that auto-scales

 Recent update to the default abbreviation length that auto-scales
 lacked documentation update, which has been corrected.


* jc/compression-config (2016-11-15) 1 commit
  (merged to 'next' on 2016-12-05 at 323769ca07)
 + compression: unify pack.compression configuration parsing

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

 Compression setting for producing packfiles were spread across
 three codepaths, one of which did not honor any configuration.
 Unify these so that all of them honor core.compression and
 pack.compression variables the same way.


* jc/git-open-cloexec (2016-11-02) 3 commits
  (merged to 'next' on 2016-12-27 at 487682eb6e)
 + sha1_file: stop opening files with O_NOATIME
 + git_open_cloexec(): use fcntl(2) w/ FD_CLOEXEC fallback
 + git_open(): untangle possible NOATIME and CLOEXEC interactions

 The codeflow of setting NOATIME and CLOEXEC on file descriptors Git
 opens has been simplified.
 We may want to drop the tip one, but we'll see.


* jc/latin-1 (2016-09-26) 2 commits
  (merged to 'next' on 2016-12-05 at fb549caa12)
 + utf8: accept "latin-1" as ISO-8859-1
 + utf8: refactor code to decide fallback encoding

 Originally merged to 'next' on 2016-09-28

 Some platforms no longer understand "latin-1" that is still seen in
 the wild in e-mail headers; replace them with "iso-8859-1" that is
 more widely known when conversion fails from/to it.


* jc/retire-compaction-heuristics (2016-12-23) 1 commit
  (merged to 'next' on 2016-12-27 at c69c2f50cf)
 + diff: retire "compaction" heuristics

 "git diff" and its family had two experimental heuristics to shift
 the contents of a hunk to make the patch easier to read.  One of
 them turns out to be better than the other, so leave only the
 "--indent-heuristic" option and remove the other one.


* jt/fetch-no-redundant-tag-fetch-map (2016-11-11) 1 commit
  (merged to 'next' on 2016-12-05 at 432f9469a7)
 + fetch: do not redundantly calculate tag refmap

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

 Code cleanup to avoid using redundant refspecs while fetching with
 the --tags option.


* mh/fast-import-notes-fix-new (2016-12-20) 1 commit
  (merged to 'next' on 2016-12-27 at b63805e6f6)
 + fast-import: properly fanout notes when tree is imported

 "git fast-import" sometimes mishandled while rebalancing notes
 tree, which has been fixed.


* mm/gc-safety-doc (2016-11-16) 1 commit
  (merged to 'next' on 2016-12-05 at 031ecc1886)
 + git-gc.txt: expand discussion of races with other processes

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

 Doc update.


* mm/push-social-engineering-attack-doc (2016-11-14) 1 commit
  (merged to 'next' on 2016-12-05 at 9a2b5bd1a9)
 + doc: mention transfer data leaks in more places

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

 Doc update on fetching and pushing.


* nd/config-misc-fixes 

Re: [PATCH v1] convert: add "status=delayed" to filter process protocol

[Taylor Blau]

On Tue, Jan 10, 2017 at 11:11:01PM +0100, Jakub Narębski wrote:
> W dniu 09.01.2017 o 00:42, Junio C Hamano pisze:
> > larsxschnei...@gmail.com writes:
> >> From: Lars Schneider 
> >>
> >> Some `clean` / `smudge` filters might require a significant amount of
> >> time to process a single blob. During this process the Git checkout
> >> operation is blocked and Git needs to wait until the filter is done to
> >> continue with the checkout.
>
> Lars, what is expected use case for this feature; that is when do you
> think this problem may happen?  Is it something that happened IRL?
>
> >>
> >> Teach the filter process protocol (introduced in edcc858) to accept the
> >> status "delayed" as response to a filter request. Upon this response Git
> >> continues with the checkout operation and asks the filter to process the
> >> blob again after all other blobs have been processed.
> >
> > Hmm, I would have expected that the basic flow would become
> >
> > for each paths to be processed:
> > convert-to-worktree to buf
> > if not delayed:
> > do the caller's thing to use buf
> > else:
> > remember path
> >
> > for each delayed paths:
> > ensure filter process finished processing for path
> > fetch the thing to buf from the process
> > do the caller's thing to use buf
>
> I would expect here to have a kind of event loop, namely
>
> while there are delayed paths:
> get path that is ready from filter
> fetch the thing to buf (supporting "delayed")
> if path done
> do the caller's thing to use buf
> (e.g. finish checkout path, eof convert, etc.)
>
> We can either trust filter process to tell us when it finished sending
> delayed paths, or keep list of paths that are being delayed in Git.

This makes a lot of sense to me. The "get path that is ready from filter" should
block until the filter has data that it is ready to send. This way Git isn't
wasting time in a busy-loop asking whether the filter has data ready to be sent.
It also means that if the filter has one large chunk that it's ready to write,
Git can work on that while the filter continues to process more data,
theoretically improving the performance of checkouts with many large delayed
objects.

>
> >
> > and that would make quite a lot of sense.  However, what is actually
> > implemented is a bit disappointing from that point of view.  While
> > its first part is the same as above, the latter part instead does:
> >
> > for each delayed paths:
> > checkout the path
> >
> > Presumably, checkout_entry() does the "ensure that the process is
> > done converting" (otherwise the result is simply buggy), but what
> > disappoints me is that this does not allow callers that call
> > "convert-to-working-tree", whose interface is obtain the bytestream
> > in-core in the working tree representation, given an object in the
> > object-db representation in an in-core buffer, to _use_ the result
> > of the conversion.  The caller does not have a chance to even see
> > the result as it is written straight to the filesystem, once it
> > calls checkout_delayed_entries().
> >
>

In addition to the above, I'd also like to investigate adding a "no more items"
message into the filter protocol. This would be useful for filters that
batch delayed items into groups. In particular, if the batch size is `N`, and 
Git
sends `2N-1` items, the second batch will be under-filled. The filter on the
other end needs some mechanism to send the second batch, even though it hasn't
hit max capacity.

Specifically, this is how Git LFS implements object transfers for data it does
not have locally, but I'm sure that this sort of functionality would be useful
for other filter implementations as well.

--
Thanks,
Taylor Blau

ttayl...@github.com

Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`

[Junio C Hamano]

Jeff King  writes:

> On Sat, Jan 07, 2017 at 02:03:30PM -0800, Junio C Hamano wrote:
>
>> Is that a longer way to say that the claim "... is designed as a
>> book" is false?
>> 
>> > So I dunno. I really do think "article" is conceptually the most
>> > appropriate style, but I agree that there are some book-like things
>> > (like appendices).
>> 
>> ... Yeah, I should have read forward first before starting to insert
>> my comments.
>
> To be honest, I'm not sure whether "book" versus "article" was really
> considered in the original writing. I think we can call it whichever
> produces the output we find most pleasing. I was mostly just pointing at
> there are some tradeoffs in the end result in flipping the type.

I understand.  

And I tend to agree that the silliness you observed (like a t-o-c
for a one-section "chapter") is not quite welcome.

For now I queued only 2/2 which looked good.  I won't object if
somebody else rerolls 1/2 to appease AsciiDoctor, but let's take an
obviously good bit first.

Thanks.

Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`

[Junio C Hamano]

"brian m. carlson"  writes:

>> [1] I think we've also traditionally considered asciidoc to be the
>> definitive toolchain, and people using asciidoctor are free to
>> submit patches to make things work correctly in both places. I'm not
>> opposed to changing that attitude, as it seems like asciidoctor is
>> faster and more actively maintained these days. But I suspect our
>> build chain would need some improvements. Last time I tried building
>> with AsciiDoctor it involved a lot manual tweaking of Makefile
>> variables. It sounds like Dscho is doing it regularly, though. It
>> should probably work out of the box (with something like
>> USE_ASCIIDOCTOR=Yes) if we expect people to actually rely on it.
>
> Yes, that would probably be beneficial.  I'll see if I can come up with
> some patches based on Dscho's work.

Thanks.

Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile

[Junio C Hamano]

Richard Hansen  writes:

>> A related tangent.
>>
>> I wonder if anything that uses git_config_pathname() should be
>> relative to GIT_DIR when it is not absolute.
>
> I think so.  (For bare repositories anyway; non-bare should be
> relative to GIT_WORK_TREE.)  Perhaps git_config_pathname() itself
> should convert relative paths to absolute so that every pathname
> setting automatically works without changing any calling code.

Yes, that was what I was alluding to.  We might have to wait until
major version boundary to do so, but I think that it is the sensible
way forward in the longer term to convert relative to absolute in
git_config_pathname().

Thanks.

Re: [PATCH v1] convert: add "status=delayed" to filter process protocol

[Jakub Narębski]

W dniu 09.01.2017 o 00:42, Junio C Hamano pisze:
> larsxschnei...@gmail.com writes:
>> From: Lars Schneider 
>>
>> Some `clean` / `smudge` filters might require a significant amount of
>> time to process a single blob. During this process the Git checkout
>> operation is blocked and Git needs to wait until the filter is done to
>> continue with the checkout.

Lars, what is expected use case for this feature; that is when do you
think this problem may happen?  Is it something that happened IRL?

>>
>> Teach the filter process protocol (introduced in edcc858) to accept the
>> status "delayed" as response to a filter request. Upon this response Git
>> continues with the checkout operation and asks the filter to process the
>> blob again after all other blobs have been processed.
> 
> Hmm, I would have expected that the basic flow would become
> 
>   for each paths to be processed:
>   convert-to-worktree to buf
>   if not delayed:
>   do the caller's thing to use buf
>   else:
>   remember path
> 
>   for each delayed paths:
>   ensure filter process finished processing for path
>   fetch the thing to buf from the process
>   do the caller's thing to use buf

I would expect here to have a kind of event loop, namely

while there are delayed paths:
get path that is ready from filter
fetch the thing to buf (supporting "delayed")
if path done
do the caller's thing to use buf 
(e.g. finish checkout path, eof convert, etc.)

We can either trust filter process to tell us when it finished sending
delayed paths, or keep list of paths that are being delayed in Git.

> 
> and that would make quite a lot of sense.  However, what is actually
> implemented is a bit disappointing from that point of view.  While
> its first part is the same as above, the latter part instead does:
> 
>   for each delayed paths:
>   checkout the path
> 
> Presumably, checkout_entry() does the "ensure that the process is
> done converting" (otherwise the result is simply buggy), but what
> disappoints me is that this does not allow callers that call
> "convert-to-working-tree", whose interface is obtain the bytestream 
> in-core in the working tree representation, given an object in the
> object-db representation in an in-core buffer, to _use_ the result
> of the conversion.  The caller does not have a chance to even see
> the result as it is written straight to the filesystem, once it
> calls checkout_delayed_entries().
> 

Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile

[Richard Hansen]

On 2017-01-10 15:23, Junio C Hamano wrote:

Junio C Hamano  writes:


Richard Hansen  writes:


On 2017-01-10 01:58, Jeff King wrote:
...

What happens in a bare repository?

I'm guessing it's relative to the top-level of the repository,


I just tried it out and it's relative to $PWD.


That is understandable.  When the user says

$ cmd -O $file

with any option -O that takes a filename, it is most natural if we
used $PWD/$file when $file is not absolute path.


Ahh, ignore me in the above.  The discussion is about the
configuration variable, and I agree that being relative to GIT_DIR
would have made more sense.  In fact taking it as relative to PWD
does not make any sense.


I'll stay silent regarding bare repositories then.



We should have been a lot more careful when we added 6d8940b562
("diff: add diff.orderfile configuration variable", 2013-12-18), but
it is too late to complain now.

A related tangent.

I wonder if anything that uses git_config_pathname() should be
relative to GIT_DIR when it is not absolute.


I think so.  (For bare repositories anyway; non-bare should be relative 
to GIT_WORK_TREE.)  Perhaps git_config_pathname() itself should convert 
relative paths to absolute so that every pathname setting automatically 
works without changing any calling code.


-Richard

Re: [PATCH 2/4] t1000: modernize style

[Junio C Hamano]

Stefan Beller  writes:

> On Tue, Jan 10, 2017 at 12:37 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> The preferred style in tests seems to be
>>
>> s/seems to be/is/;
>
> If this is the only nit, mind to fix up the commit message locally?

Certainly.  It wasn't even meant as a "nitpick".  I was just
confirming your observation.

Re: [PATCHv2 4/5] unpack-trees: factor file removal out of check_updates

[Junio C Hamano]

Stefan Beller  writes:

> As far as I understand the operations now, a working tree operation
> is done like this:
>
> * First compute the new index, how the world would look like
>   (don't touch a thing in the working tree, it is like a complete
>   dry run, that just checks for potential loss of data, e.g.
>   untracked files, merge conflicts etc)
> * remove all files to be marked for deletion
> * add and update all files that are new or change content.
>
> check_updates does the last two things listed here, which in the
> grand scheme of things looked odd to me.

In the grand scheme of things, the flow has always been understood
more like this two-step process:

- compute the result in core (rejecting an inconsistent result
  before touching the working tree)

- reflect the result to the working tree

and the latter is done by check_updates().  

Removing gone entries before instanciating possibly new entries is
done in order to make room where we can create a new path D/F in the
result, after removing an old path D [*1*].  But it is merely an
implementation detail of the latter, i.e. "reflect the result to the
working tree.

It is arguably true that check_updates() is not about "checking" but
is about "doing", hence is misnamed.

[Footnote]

*1* If the result computed in-core wants to create D/F, it must have
removal of D---otherwise the result is inconsistent.

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

[Junio C Hamano]

Karthik Nayak  writes:

> index 81db67d74..08be8462c 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -95,13 +95,17 @@ 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` and
> + `%(refname:rstrip=2)` turns `refs/tags/foo` into `refs`).

I hiccupped while reading this, as the (e.g.) example talks about rstrip
that is not mentioned in the main text that is enhanced by the
example.

If `lstrip=` (`rstrip=`) is appended, strips ``
slash-separated path components from the front (tail) of the
refname (e.g. `%(refname:lstrip=2)` ...

perhaps?

> + if `` is a negative number, then only `` path components
> + are left behind.

Begin the sentence with cap?  I'd rephrase it a bit while at it if I
were doing this:

If `` is a negative number, strip as many path components
as necessary from the specified end to leave `-` path
components.

Other than the above, looks very good to me.

Thanks.

Re: [PATCHv2 4/5] unpack-trees: factor file removal out of check_updates

[Stefan Beller]

On Tue, Jan 10, 2017 at 12:05 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> This makes check_updates shorter and easier to understand.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>
> I agree that 3/5 made it easier to understand by ejecting a block
> that is not essential to the functionality of the function out of
> it, making the remainder of the fuction about "removing gone files
> and then write out the modified files".
>
> The ejecting of the first half of these two operations, both are
> what this function is about, done by this step feels backwards.  If
> anything, the "only do the actual working tree manipulation when not
> doing a dry-run and told to update" logic that must be in both are
> spread in two helper functions after step 5/5, and with the added
> boilerplate for these two helpers, the end result becomes _longer_
> to understand what is really going on when check_updates() is
> called.
>
> Is the original after step 3/5 too long and hard to understand?

It is ok to understand for the current state of the world,
so please drop 4 and 5 for now.

In a future, when
* working tree operations would learn about threading or
* working tree operations were aware of submodules
then steps of 4 and 5 would be longer, such that the check_updates
function may require further splitting up.

As far as I understand the operations now, a working tree operation
is done like this:

* First compute the new index, how the world would look like
  (don't touch a thing in the working tree, it is like a complete
  dry run, that just checks for potential loss of data, e.g.
  untracked files, merge conflicts etc)
* remove all files to be marked for deletion
* add and update all files that are new or change content.

check_updates does the last two things listed here, which in the
grand scheme of things looked odd to me.

When introducing e.g. threaded checkout, then each of the functions
introduced in patch 4&5 would be one compartment, i.e. the function
remove_workingtree_files would need to have all its tasks finished
before we even queue up tasks for updating files, such that we
do not run into D/F conflicts racily.

So yeah, maybe this is too much future-visionary thinking, so please
drop 4/5; I can revive them if needed. I think I mainly did them
for my own clarity and understanding.

Thanks,
Stefan

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

[Junio C Hamano]

Karthik Nayak  writes:

> + if_then_else->condition_satisfied = 1;
> + } else  if (if_then_else->cmp_status == COMPARE_UNEQUAL) {

Please, no space before tabs (locally fixed--no need to resend).

[PATCH v5 06/14] t7610: don't rely on state from previous test

[Richard Hansen]

If the repository must be in a particular state (beyond what is
already done by the 'setup' test case) before the test can run, make
the necessary repository changes in the test script even if it means
duplicating some lines of code from the previous test case.

This is a step toward making the tests more independent so that if one
test fails it doesn't cause subsequent tests to fail.

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index f62ceffdc..2d92a2646 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -181,8 +181,12 @@ test_expect_success 'mergetool in subdir' '
 '
 
 test_expect_success 'mergetool on file in parent dir' '
+   git reset --hard &&
+   git submodule update -N &&
(
cd subdir &&
+   test_must_fail git merge master >/dev/null 2>&1 &&
+   ( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file2 ../spaced\ name >/dev/null 
2>&1 ) &&
( yes "" | git mergetool ../both >/dev/null 2>&1 ) &&
@@ -651,6 +655,8 @@ test_expect_success 'mergetool -Oorder-file is honored' '
test_config diff.orderFile order-file &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
test_config mergetool.myecho.trustExitCode true &&
+   echo b >order-file &&
+   echo a >>order-file &&
test_must_fail git merge order-file-side1 &&
cat >expect <<-\EOF &&
Merging:
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v5 03/14] t7610: update branch names to match test number

[Richard Hansen]

Rename the testNN branches so that NN matches the test number.  This
should make it easier to troubleshoot test issues.  Use $test_count to
keep this future-proof.

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 82 ++--
 1 file changed, 41 insertions(+), 41 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 6d9f21511..14090739f 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -94,7 +94,7 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'custom mergetool' '
-   git checkout -b test1 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -113,7 +113,7 @@ test_expect_success 'custom mergetool' '
 
 test_expect_success 'mergetool crlf' '
test_config core.autocrlf true &&
-   git checkout -b test2 branch1 &&
+   git checkout -b test$test_count branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
@@ -134,7 +134,7 @@ test_expect_success 'mergetool crlf' '
 '
 
 test_expect_success 'mergetool in subdir' '
-   git checkout -b test3 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
cd subdir &&
@@ -161,7 +161,7 @@ test_expect_success 'mergetool on file in parent dir' '
 '
 
 test_expect_success 'mergetool skips autoresolved' '
-   git checkout -b test4 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
@@ -192,7 +192,7 @@ test_expect_success 'mergetool merges all from subdir' '
 test_expect_success 'mergetool skips resolved paths when rerere is active' '
test_config rerere.enabled true &&
rm -rf .git/rr-cache &&
-   git checkout -b test5 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "l" | git mergetool --no-prompt submod >/dev/null 2>&1 ) &&
@@ -233,7 +233,7 @@ test_expect_success 'conflicted stash sets up rerere'  '
 test_expect_success 'mergetool takes partial path' '
git reset --hard &&
test_config rerere.enabled false &&
-   git checkout -b test12 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
 
@@ -308,12 +308,12 @@ test_expect_success 'mergetool keeps tempfiles when 
aborting delete/delete' '
 '
 
 test_expect_success 'deleted vs modified submodule' '
-   git checkout -b test6 branch1 &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
mv submod submod-movedaside &&
git rm --cached submod &&
git commit -m "Submodule deleted from branch" &&
-   git checkout -b test6.a test6 &&
+   git checkout -b test$test_count.a test$test_count &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 
>/dev/null 2>&1 ) &&
@@ -329,7 +329,7 @@ test_expect_success 'deleted vs modified submodule' '
git commit -m "Merge resolved by keeping module" &&
 
mv submod submod-movedaside &&
-   git checkout -b test6.b test6 &&
+   git checkout -b test$test_count.b test$test_count &&
git submodule update -N &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
@@ -343,9 +343,9 @@ test_expect_success 'deleted vs modified submodule' '
git commit -m "Merge resolved by deleting module" &&
 
mv submod-movedaside submod &&
-   git checkout -b test6.c master &&
+   git checkout -b test$test_count.c master &&
git submodule update -N &&
-   test_must_fail git merge test6 &&
+   test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 
>/dev/null 2>&1 ) &&
( yes "" | git mergetool both >/dev/null 2>&1 ) &&
@@ -359,9 +359,9 @@ test_expect_success 'deleted vs modified submodule' '
git commit -m "Merge resolved by deleting module" &&
mv submod.orig submod &&
 
-   git checkout -b test6.d master &&
+   git checkout -b test$test_count.d master &&
git submodule update -N &&
-   test_must_fail git merge test6 &&
+   test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 
>/dev/null 2>&1 ) &&
( yes "" | git mergetool both >/dev/null 

[PATCH v5 04/14] t7610: Move setup code to the 'setup' test case.

[Richard Hansen]

Multiple test cases depend on these hunks, so move them to the 'setup'
test case.  This is a step toward making the tests more independent so
that if one test fails it doesn't cause subsequent tests to fail.

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 65 
 1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 14090739f..550838a1c 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -55,6 +55,22 @@ test_expect_success 'setup' '
git rm file12 &&
git commit -m "branch1 changes" &&
 
+   git checkout -b delete-base branch1 &&
+   mkdir -p a/a &&
+   (echo one; echo two; echo 3; echo 4) >a/a/file.txt &&
+   git add a/a/file.txt &&
+   git commit -m"base file" &&
+   git checkout -b move-to-b delete-base &&
+   mkdir -p b/b &&
+   git mv a/a/file.txt b/b/file.txt &&
+   (echo one; echo two; echo 4) >b/b/file.txt &&
+   git commit -a -m"move to b" &&
+   git checkout -b move-to-c delete-base &&
+   mkdir -p c/c &&
+   git mv a/a/file.txt c/c/file.txt &&
+   (echo one; echo two; echo 3) >c/c/file.txt &&
+   git commit -a -m"move to c" &&
+
git checkout -b stash1 master &&
echo stash1 change file11 >file11 &&
git add file11 &&
@@ -86,6 +102,23 @@ test_expect_success 'setup' '
git rm file11 &&
git commit -m "master updates" &&
 
+   git clean -fdx &&
+   git checkout -b order-file-start master &&
+   echo start >a &&
+   echo start >b &&
+   git add a b &&
+   git commit -m start &&
+   git checkout -b order-file-side1 order-file-start &&
+   echo side1 >a &&
+   echo side1 >b &&
+   git add a b &&
+   git commit -m side1 &&
+   git checkout -b order-file-side2 order-file-start &&
+   echo side2 >a &&
+   echo side2 >b &&
+   git add a b &&
+   git commit -m side2 &&
+
git config merge.tool mytool &&
git config mergetool.mytool.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
git config mergetool.mytool.trustExitCode true &&
@@ -244,21 +277,7 @@ test_expect_success 'mergetool takes partial path' '
 '
 
 test_expect_success 'mergetool delete/delete conflict' '
-   git checkout -b delete-base branch1 &&
-   mkdir -p a/a &&
-   (echo one; echo two; echo 3; echo 4) >a/a/file.txt &&
-   git add a/a/file.txt &&
-   git commit -m"base file" &&
-   git checkout -b move-to-b delete-base &&
-   mkdir -p b/b &&
-   git mv a/a/file.txt b/b/file.txt &&
-   (echo one; echo two; echo 4) >b/b/file.txt &&
-   git commit -a -m"move to b" &&
-   git checkout -b move-to-c delete-base &&
-   mkdir -p c/c &&
-   git mv a/a/file.txt c/c/file.txt &&
-   (echo one; echo two; echo 3) >c/c/file.txt &&
-   git commit -a -m"move to c" &&
+   git checkout move-to-c &&
test_must_fail git merge move-to-b &&
echo d | git mergetool a/a/file.txt &&
! test -f a/a/file.txt &&
@@ -607,26 +626,12 @@ test_expect_success MKTEMP 'temporary filenames are used 
with mergetool.writeToT
 '
 
 test_expect_success 'diff.orderFile configuration is honored' '
+   git checkout order-file-side2 &&
test_config diff.orderFile order-file &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
test_config mergetool.myecho.trustExitCode true &&
echo b >order-file &&
echo a >>order-file &&
-   git checkout -b order-file-start master &&
-   echo start >a &&
-   echo start >b &&
-   git add a b &&
-   git commit -m start &&
-   git checkout -b order-file-side1 order-file-start &&
-   echo side1 >a &&
-   echo side1 >b &&
-   git add a b &&
-   git commit -m side1 &&
-   git checkout -b order-file-side2 order-file-start &&
-   echo side2 >a &&
-   echo side2 >b &&
-   git add a b &&
-   git commit -m side2 &&
test_must_fail git merge order-file-side1 &&
cat >expect <<-\EOF &&
Merging:
-- 
2.11.0.390.gc69c2f50cf-goog

Re: [PATCH 2/4] t1000: modernize style

[Stefan Beller]

On Tue, Jan 10, 2017 at 12:37 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> The preferred style in tests seems to be
>
> s/seems to be/is/;

If this is the only nit, mind to fix up the commit message locally?
(I was even unsure if we want to have this patch as part
of a larger series, as it is just refactoring for the sake of refactoring,
i.e. t1000 doesn't see a new test in this series, only t1001 does)

>
>>
>> test_expect_success 'short description, ended by 2 single quotes' '
>>   here comes the test &&
>>   and chains over many lines &&
>>   ended by a quote
>> '
>
> Thanks.  This is way overdue.  During the time the script has been
> dormant for more than two years, we should have done this.

agreed.

[PATCH v5 02/14] rev-parse doc: pass "--" to rev-parse in the --prefix example

[Richard Hansen]

The "--" argument avoids "ambiguous argument: unknown revision or
path not in the working tree" errors when a pathname argument refers
to a non-existent file.

The "--" passed explicitly to set was removed because rev-parse
outputs the "--" argument that it is given.

Signed-off-by: Richard Hansen 
---
 Documentation/git-rev-parse.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index b6c6326cd..7241e9689 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -91,7 +91,8 @@ repository.  For example:
 
 prefix=$(git rev-parse --show-prefix)
 cd "$(git rev-parse --show-toplevel)"
-eval "set -- $(git rev-parse --sq --prefix "$prefix" "$@")"
+# rev-parse provides the -- needed for 'set'
+eval "set $(git rev-parse --sq --prefix "$prefix" -- "$@")"
 
 
 --verify::
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v5 05/14] t7610: use test_when_finished for cleanup tasks

[Richard Hansen]

This is a step toward making the tests more independent so that if one
test fails it doesn't cause subsequent tests to fail.

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 71 +++-
 1 file changed, 37 insertions(+), 34 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 550838a1c..f62ceffdc 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -145,6 +145,11 @@ test_expect_success 'custom mergetool' '
 '
 
 test_expect_success 'mergetool crlf' '
+   test_when_finished "git reset --hard" &&
+   # This test_config line must go after the above reset line so that
+   # core.autocrlf is unconfigured before reset runs.  (The
+   # test_config command uses test_when_finished internally and
+   # test_when_finished is LIFO.)
test_config core.autocrlf true &&
git checkout -b test$test_count branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
@@ -161,9 +166,7 @@ test_expect_success 'mergetool crlf' '
test "$(printf x | cat subdir/file3 -)" = "$(printf "master new 
sub\r\nx")" &&
git submodule update -N &&
test "$(cat submod/bar)" = "master submodule" &&
-   git commit -m "branch1 resolved with mergetool - autocrlf" &&
-   test_config core.autocrlf false &&
-   git reset --hard
+   git commit -m "branch1 resolved with mergetool - autocrlf"
 '
 
 test_expect_success 'mergetool in subdir' '
@@ -194,6 +197,7 @@ test_expect_success 'mergetool on file in parent dir' '
 '
 
 test_expect_success 'mergetool skips autoresolved' '
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
@@ -202,8 +206,7 @@ test_expect_success 'mergetool skips autoresolved' '
( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
output="$(git mergetool --no-prompt)" &&
-   test "$output" = "No files need merging" &&
-   git reset --hard
+   test "$output" = "No files need merging"
 '
 
 test_expect_success 'mergetool merges all from subdir' '
@@ -223,6 +226,7 @@ test_expect_success 'mergetool merges all from subdir' '
 '
 
 test_expect_success 'mergetool skips resolved paths when rerere is active' '
+   test_when_finished "git reset --hard" &&
test_config rerere.enabled true &&
rm -rf .git/rr-cache &&
git checkout -b test$test_count branch1 &&
@@ -232,8 +236,7 @@ test_expect_success 'mergetool skips resolved paths when 
rerere is active' '
( yes "d" "d" | git mergetool --no-prompt >/dev/null 2>&1 ) &&
git submodule update -N &&
output="$(yes "n" | git mergetool --no-prompt)" &&
-   test "$output" = "No files need merging" &&
-   git reset --hard
+   test "$output" = "No files need merging"
 '
 
 test_expect_success 'conflicted stash sets up rerere'  '
@@ -264,6 +267,7 @@ test_expect_success 'conflicted stash sets up rerere'  '
 '
 
 test_expect_success 'mergetool takes partial path' '
+   test_when_finished "git reset --hard" &&
git reset --hard &&
test_config rerere.enabled false &&
git checkout -b test$test_count branch1 &&
@@ -272,11 +276,11 @@ test_expect_success 'mergetool takes partial path' '
 
( yes "" | git mergetool subdir ) &&
 
-   test "$(cat subdir/file3)" = "master new sub" &&
-   git reset --hard
+   test "$(cat subdir/file3)" = "master new sub"
 '
 
 test_expect_success 'mergetool delete/delete conflict' '
+   test_when_finished "git reset --hard HEAD" &&
git checkout move-to-c &&
test_must_fail git merge move-to-b &&
echo d | git mergetool a/a/file.txt &&
@@ -288,29 +292,30 @@ test_expect_success 'mergetool delete/delete conflict' '
git reset --hard HEAD &&
test_must_fail git merge move-to-b &&
! echo a | git mergetool a/a/file.txt &&
-   ! test -f a/a/file.txt &&
-   git reset --hard HEAD
+   ! test -f a/a/file.txt
 '
 
 test_expect_success 'mergetool produces no errors when keepBackup is used' '
+   test_when_finished "git reset --hard HEAD" &&
test_config mergetool.keepBackup true &&
test_must_fail git merge move-to-b &&
: >expect &&
echo d | git mergetool a/a/file.txt 2>actual &&
test_cmp expect actual &&
-   ! test -d a &&
-   git reset --hard HEAD
+   ! test -d a
 '
 
 test_expect_success 'mergetool honors tempfile config for deleted files' '
+   test_when_finished "git reset --hard HEAD" &&
test_config mergetool.keepTemporaries false &&
test_must_fail git merge move-to-b &&
echo d | git mergetool a/a/file.txt &&
-   ! test -d a &&
-   git reset --hard HEAD
+   ! test -d a
 '
 
 test_expect_success 'mergetool keeps tempfiles when aborting 

[PATCH v5 10/14] t7610: don't assume the checked-out commit

[Richard Hansen]

Always check out the required commit at the beginning of the test so
that a failure in a previous test does not cause the test to work off
of the wrong commit.

This is a step toward making the tests more independent so that if one
test fails it doesn't cause subsequent tests to fail.

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index efcf5c3f1..54164a320 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -184,7 +184,7 @@ test_expect_success 'mergetool in subdir' '
 
 test_expect_success 'mergetool on file in parent dir' '
test_when_finished "git reset --hard" &&
-   git checkout -b test$test_count &&
+   git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
cd subdir &&
@@ -218,7 +218,7 @@ test_expect_success 'mergetool skips autoresolved' '
 
 test_expect_success 'mergetool merges all from subdir' '
test_when_finished "git reset --hard" &&
-   git checkout -b test$test_count &&
+   git checkout -b test$test_count branch1 &&
test_config rerere.enabled false &&
(
cd subdir &&
@@ -306,7 +306,7 @@ test_expect_success 'mergetool delete/delete conflict' '
 
 test_expect_success 'mergetool produces no errors when keepBackup is used' '
test_when_finished "git reset --hard HEAD" &&
-   git checkout -b test$test_count &&
+   git checkout -b test$test_count move-to-c &&
test_config mergetool.keepBackup true &&
test_must_fail git merge move-to-b &&
: >expect &&
@@ -317,7 +317,7 @@ test_expect_success 'mergetool produces no errors when 
keepBackup is used' '
 
 test_expect_success 'mergetool honors tempfile config for deleted files' '
test_when_finished "git reset --hard HEAD" &&
-   git checkout -b test$test_count &&
+   git checkout -b test$test_count move-to-c &&
test_config mergetool.keepTemporaries false &&
test_must_fail git merge move-to-b &&
echo d | git mergetool a/a/file.txt &&
@@ -327,7 +327,7 @@ test_expect_success 'mergetool honors tempfile config for 
deleted files' '
 test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
test_when_finished "git reset --hard HEAD" &&
test_when_finished "git clean -fdx" &&
-   git checkout -b test$test_count &&
+   git checkout -b test$test_count move-to-c &&
test_config mergetool.keepTemporaries true &&
test_must_fail git merge move-to-b &&
! (echo a; echo n) | git mergetool a/a/file.txt &&
@@ -663,7 +663,7 @@ test_expect_success 'diff.orderFile configuration is 
honored' '
 '
 test_expect_success 'mergetool -Oorder-file is honored' '
test_when_finished "git reset --hard >/dev/null 2>&1" &&
-   git checkout -b test$test_count &&
+   git checkout -b test$test_count order-file-side2 &&
test_config diff.orderFile order-file &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
test_config mergetool.myecho.trustExitCode true &&
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v5 08/14] t7610: delete some now-unnecessary 'git reset --hard' lines

[Richard Hansen]

Tests now always run 'git reset --hard' at the end (even if they
fail), so it's no longer necessary to run 'git reset --hard' at the
beginning of a test.

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 55587504e..7d5e1df88 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -184,7 +184,6 @@ test_expect_success 'mergetool in subdir' '
 
 test_expect_success 'mergetool on file in parent dir' '
test_when_finished "git reset --hard" &&
-   git reset --hard &&
git submodule update -N &&
(
cd subdir &&
@@ -277,7 +276,6 @@ test_expect_success 'conflicted stash sets up rerere'  '
 
 test_expect_success 'mergetool takes partial path' '
test_when_finished "git reset --hard" &&
-   git reset --hard &&
test_config rerere.enabled false &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v5 13/14] mergetool: take the "-O" out of $orderfile

[Richard Hansen]

This will make it easier for a future commit to convert a relative
orderfile pathname to either absolute or relative to the top-level
directory.  It also improves code readability.

Signed-off-by: Richard Hansen 
---
 git-mergetool.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index e52b4e4f2..b506896dc 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -421,7 +421,7 @@ main () {
prompt=true
;;
-O*)
-   orderfile="$1"
+   orderfile="${1#-O}"
;;
--)
shift
@@ -465,7 +465,7 @@ main () {
 
files=$(git -c core.quotePath=false \
diff --name-only --diff-filter=U \
-   ${orderfile:+"$orderfile"} -- "$@")
+   ${orderfile:+"-O$orderfile"} -- "$@")
 
cd_to_toplevel
 
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v5 11/14] t7610: spell 'git reset --hard' consistently

[Richard Hansen]

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 37 +++--
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 54164a320..c031ecd9e 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -289,23 +289,23 @@ test_expect_success 'mergetool takes partial path' '
 '
 
 test_expect_success 'mergetool delete/delete conflict' '
-   test_when_finished "git reset --hard HEAD" &&
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count move-to-c &&
test_must_fail git merge move-to-b &&
echo d | git mergetool a/a/file.txt &&
! test -f a/a/file.txt &&
-   git reset --hard HEAD &&
+   git reset --hard &&
test_must_fail git merge move-to-b &&
echo m | git mergetool a/a/file.txt &&
test -f b/b/file.txt &&
-   git reset --hard HEAD &&
+   git reset --hard &&
test_must_fail git merge move-to-b &&
! echo a | git mergetool a/a/file.txt &&
! test -f a/a/file.txt
 '
 
 test_expect_success 'mergetool produces no errors when keepBackup is used' '
-   test_when_finished "git reset --hard HEAD" &&
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count move-to-c &&
test_config mergetool.keepBackup true &&
test_must_fail git merge move-to-b &&
@@ -316,7 +316,7 @@ test_expect_success 'mergetool produces no errors when 
keepBackup is used' '
 '
 
 test_expect_success 'mergetool honors tempfile config for deleted files' '
-   test_when_finished "git reset --hard HEAD" &&
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count move-to-c &&
test_config mergetool.keepTemporaries false &&
test_must_fail git merge move-to-b &&
@@ -325,7 +325,7 @@ test_expect_success 'mergetool honors tempfile config for 
deleted files' '
 '
 
 test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
-   test_when_finished "git reset --hard HEAD" &&
+   test_when_finished "git reset --hard" &&
test_when_finished "git clean -fdx" &&
git checkout -b test$test_count move-to-c &&
test_config mergetool.keepTemporaries true &&
@@ -342,7 +342,7 @@ test_expect_success 'mergetool keeps tempfiles when 
aborting delete/delete' '
 '
 
 test_expect_success 'deleted vs modified submodule' '
-   test_when_finished "git reset --hard HEAD" &&
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
mv submod submod-movedaside &&
@@ -560,7 +560,7 @@ test_expect_success 'directory vs modified submodule' '
test "$(cat submod/file16)" = "not a submodule" &&
rm -rf submod.orig &&
 
-   git reset --hard >/dev/null 2>&1 &&
+   git reset --hard &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
test ! -e submod.orig &&
@@ -572,7 +572,8 @@ test_expect_success 'directory vs modified submodule' '
( cd submod && git clean -f && git reset --hard ) &&
git submodule update -N &&
test "$(cat submod/bar)" = "master submodule" &&
-   git reset --hard >/dev/null 2>&1 && rm -rf submod-movedaside &&
+   git reset --hard &&
+   rm -rf submod-movedaside &&
 
git checkout -b test$test_count.c master &&
git submodule update -N &&
@@ -582,7 +583,7 @@ test_expect_success 'directory vs modified submodule' '
git submodule update -N &&
test "$(cat submod/bar)" = "master submodule" &&
 
-   git reset --hard >/dev/null 2>&1 &&
+   git reset --hard &&
git submodule update -N &&
test_must_fail git merge test$test_count &&
test -n "$(git ls-files -u)" &&
@@ -590,13 +591,13 @@ test_expect_success 'directory vs modified submodule' '
( yes "r" | git mergetool submod ) &&
test "$(cat submod/file16)" = "not a submodule" &&
 
-   git reset --hard master >/dev/null 2>&1 &&
+   git reset --hard master &&
( cd submod && git clean -f && git reset --hard ) &&
git submodule update -N
 '
 
 test_expect_success 'file with no base' '
-   test_when_finished "git reset --hard master >/dev/null 2>&1" &&
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool mybase -- both &&
@@ -605,7 +606,7 @@ test_expect_success 'file with no base' '
 '
 
 test_expect_success 'custom commands override built-ins' '
-   test_when_finished "git reset --hard master >/dev/null 2>&1" &&
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
test_config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
test_config 

[PATCH v5 07/14] t7610: run 'git reset --hard' after each test to clean up

[Richard Hansen]

Use test_when_finished to run 'git reset --hard' after each test so
that the repository is left in a saner state for the next test.

This is a step toward making the tests more independent so that if one
test fails it doesn't cause subsequent tests to fail.

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 8 
 1 file changed, 8 insertions(+)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 2d92a2646..55587504e 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -127,6 +127,7 @@ test_expect_success 'setup' '
 '
 
 test_expect_success 'custom mergetool' '
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
@@ -170,6 +171,7 @@ test_expect_success 'mergetool crlf' '
 '
 
 test_expect_success 'mergetool in subdir' '
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
@@ -181,6 +183,7 @@ test_expect_success 'mergetool in subdir' '
 '
 
 test_expect_success 'mergetool on file in parent dir' '
+   test_when_finished "git reset --hard" &&
git reset --hard &&
git submodule update -N &&
(
@@ -214,6 +217,7 @@ test_expect_success 'mergetool skips autoresolved' '
 '
 
 test_expect_success 'mergetool merges all from subdir' '
+   test_when_finished "git reset --hard" &&
test_config rerere.enabled false &&
(
cd subdir &&
@@ -244,6 +248,7 @@ test_expect_success 'mergetool skips resolved paths when 
rerere is active' '
 '
 
 test_expect_success 'conflicted stash sets up rerere'  '
+   test_when_finished "git reset --hard" &&
test_config rerere.enabled true &&
git checkout stash1 &&
echo "Conflicting stash content" >file11 &&
@@ -403,6 +408,7 @@ test_expect_success 'deleted vs modified submodule' '
 '
 
 test_expect_success 'file vs modified submodule' '
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
mv submod submod-movedaside &&
@@ -474,6 +480,7 @@ test_expect_success 'file vs modified submodule' '
 '
 
 test_expect_success 'submodule in subdirectory' '
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
git submodule update -N &&
(
@@ -535,6 +542,7 @@ test_expect_success 'submodule in subdirectory' '
 '
 
 test_expect_success 'directory vs modified submodule' '
+   test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
mv submod submod-movedaside &&
git rm --cached submod &&
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v5 14/14] mergetool: fix running in subdir when rerere enabled

[Richard Hansen]

The pathnames output by the 'git rerere remaining' command are
relative to the top-level directory but the 'git diff --name-only'
command expects its pathname arguments to be relative to the current
working directory.  Run cd_to_toplevel before running 'git diff
--name-only' and adjust any relative pathnames so that 'git mergetool'
does not fail when run from a subdirectory with rerere enabled.

This fixes a regression introduced in
57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).

Based-on-patch-by: Junio C Hamano 
Signed-off-by: Richard Hansen 
---
 git-mergetool.sh | 17 +++--
 t/t7610-mergetool.sh |  7 ++-
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index b506896dc..c062e3de3 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -454,6 +454,17 @@ main () {
merge_keep_backup="$(git config --bool mergetool.keepBackup || echo 
true)"
merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries 
|| echo false)"
 
+   prefix=$(git rev-parse --show-prefix) || exit 1
+   cd_to_toplevel
+
+   if test -n "$orderfile"
+   then
+   orderfile=$(
+   git rev-parse --prefix "$prefix" -- "$orderfile" |
+   sed -e 1d
+   )
+   fi
+
if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
then
set -- $(git rerere remaining)
@@ -461,14 +472,16 @@ main () {
then
print_noop_and_exit
fi
+   elif test $# -ge 0
+   then
+   # rev-parse provides the -- needed for 'set'
+   eval "set $(git rev-parse --sq --prefix "$prefix" -- "$@")"
fi
 
files=$(git -c core.quotePath=false \
diff --name-only --diff-filter=U \
${orderfile:+"-O$orderfile"} -- "$@")
 
-   cd_to_toplevel
-
if test -z "$files"
then
print_noop_and_exit
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index b36fde1c0..820fc8597 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -234,7 +234,7 @@ test_expect_success 'mergetool merges all from subdir 
(rerere disabled)' '
)
 '
 
-test_expect_failure 'mergetool merges all from subdir (rerere enabled)' '
+test_expect_success 'mergetool merges all from subdir (rerere enabled)' '
test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
test_config rerere.enabled true &&
@@ -677,6 +677,11 @@ test_expect_success 'diff.orderFile configuration is 
honored' '
b
a
EOF
+
+   # make sure "order-file" that is ambiguous between
+   # rev and path is understood correctly.
+   git branch order-file HEAD &&
+
git mergetool --no-prompt --tool myecho >output &&
git grep --no-index -h -A2 Merging: output >actual &&
test_cmp expect actual
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v5 12/14] t7610: add test case for rerere+mergetool+subdir bug

[Richard Hansen]

If rerere is enabled and mergetool is run from a subdirectory,
mergetool always prints "No files need merging".  Add an expected
failure test case for this situation.

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 21 -
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index c031ecd9e..b36fde1c0 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -216,7 +216,7 @@ test_expect_success 'mergetool skips autoresolved' '
test "$output" = "No files need merging"
 '
 
-test_expect_success 'mergetool merges all from subdir' '
+test_expect_success 'mergetool merges all from subdir (rerere disabled)' '
test_when_finished "git reset --hard" &&
git checkout -b test$test_count branch1 &&
test_config rerere.enabled false &&
@@ -234,6 +234,25 @@ test_expect_success 'mergetool merges all from subdir' '
)
 '
 
+test_expect_failure 'mergetool merges all from subdir (rerere enabled)' '
+   test_when_finished "git reset --hard" &&
+   git checkout -b test$test_count branch1 &&
+   test_config rerere.enabled true &&
+   rm -rf .git/rr-cache &&
+   (
+   cd subdir &&
+   test_must_fail git merge master &&
+   ( yes "r" | git mergetool ../submod ) &&
+   ( yes "d" "d" | git mergetool --no-prompt ) &&
+   test "$(cat ../file1)" = "master updated" &&
+   test "$(cat ../file2)" = "master new" &&
+   test "$(cat file3)" = "master new sub" &&
+   ( cd .. && git submodule update -N ) &&
+   test "$(cat ../submod/bar)" = "master submodule" &&
+   git commit -m "branch2 resolved by mergetool from subdir"
+   )
+'
+
 test_expect_success 'mergetool skips resolved paths when rerere is active' '
test_when_finished "git reset --hard" &&
test_config rerere.enabled true &&
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v5 00/14] fix mergetool+rerere+subdir regression

[Richard Hansen]

If rerere is enabled, no pathnames are given, and mergetool is run
from a subdirectory, mergetool always prints "No files need merging".
Fix the bug.

This regression was introduced in
57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).

Changes since v4:
  * Revert to Junio's original proposal for 14/14.  (Junio:  Please
feel free to take author credit on 14/4; it's now 100% your code.)
  * Do:
# rev-parse provides the -- needed for 'set'
eval "set $(git rev-parse --sq -- "$@")"
instead of:
eval "set -- $(git rev-parse --sq -- "$@")"
shift
for both 02/14 and 14/14.

Richard Hansen (14):
  .mailmap: Use my personal email address as my canonical
  rev-parse doc: pass "--" to rev-parse in the --prefix example
  t7610: update branch names to match test number
  t7610: Move setup code to the 'setup' test case.
  t7610: use test_when_finished for cleanup tasks
  t7610: don't rely on state from previous test
  t7610: run 'git reset --hard' after each test to clean up
  t7610: delete some now-unnecessary 'git reset --hard' lines
  t7610: always work on a test-specific branch
  t7610: don't assume the checked-out commit
  t7610: spell 'git reset --hard' consistently
  t7610: add test case for rerere+mergetool+subdir bug
  mergetool: take the "-O" out of $orderfile
  mergetool: fix running in subdir when rerere enabled

 .mailmap|   2 +
 Documentation/git-rev-parse.txt |   3 +-
 git-mergetool.sh|  21 ++-
 t/t7610-mergetool.sh| 281 
 4 files changed, 187 insertions(+), 120 deletions(-)

-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v5 01/14] .mailmap: Use my personal email address as my canonical

[Richard Hansen]

When I changed employers my work address changed from rhan...@bbn.com
to hans...@google.com.  Rather than map my old work address to my new,
map them both to my permanent personal email address.  (I will still
use my work address in commits I submit so that my employer gets some
credit.)

Signed-off-by: Richard Hansen 
---
 .mailmap | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/.mailmap b/.mailmap
index 9cc33e925..9c87a3840 100644
--- a/.mailmap
+++ b/.mailmap
@@ -192,6 +192,8 @@ Philippe Bruhat 
 Ralf Thielow  
 Ramsay Jones  
 René Scharfe  
+Richard Hansen  
+Richard Hansen  
 Robert Fitzsimons 
 Robert Shearman  
 Robert Zeh 
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH v5 09/14] t7610: always work on a test-specific branch

[Richard Hansen]

Create and use a test-specific branch when the test might create a
commit.  This is not always necessary for correctness, but it improves
debuggability by ensuring a commit created by test #N shows up on the
testN branch, not the branch for test #N-1.

Signed-off-by: Richard Hansen 
---
 t/t7610-mergetool.sh | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 7d5e1df88..efcf5c3f1 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -184,6 +184,7 @@ test_expect_success 'mergetool in subdir' '
 
 test_expect_success 'mergetool on file in parent dir' '
test_when_finished "git reset --hard" &&
+   git checkout -b test$test_count &&
git submodule update -N &&
(
cd subdir &&
@@ -217,6 +218,7 @@ test_expect_success 'mergetool skips autoresolved' '
 
 test_expect_success 'mergetool merges all from subdir' '
test_when_finished "git reset --hard" &&
+   git checkout -b test$test_count &&
test_config rerere.enabled false &&
(
cd subdir &&
@@ -288,7 +290,7 @@ test_expect_success 'mergetool takes partial path' '
 
 test_expect_success 'mergetool delete/delete conflict' '
test_when_finished "git reset --hard HEAD" &&
-   git checkout move-to-c &&
+   git checkout -b test$test_count move-to-c &&
test_must_fail git merge move-to-b &&
echo d | git mergetool a/a/file.txt &&
! test -f a/a/file.txt &&
@@ -304,6 +306,7 @@ test_expect_success 'mergetool delete/delete conflict' '
 
 test_expect_success 'mergetool produces no errors when keepBackup is used' '
test_when_finished "git reset --hard HEAD" &&
+   git checkout -b test$test_count &&
test_config mergetool.keepBackup true &&
test_must_fail git merge move-to-b &&
: >expect &&
@@ -314,6 +317,7 @@ test_expect_success 'mergetool produces no errors when 
keepBackup is used' '
 
 test_expect_success 'mergetool honors tempfile config for deleted files' '
test_when_finished "git reset --hard HEAD" &&
+   git checkout -b test$test_count &&
test_config mergetool.keepTemporaries false &&
test_must_fail git merge move-to-b &&
echo d | git mergetool a/a/file.txt &&
@@ -323,6 +327,7 @@ test_expect_success 'mergetool honors tempfile config for 
deleted files' '
 test_expect_success 'mergetool keeps tempfiles when aborting delete/delete' '
test_when_finished "git reset --hard HEAD" &&
test_when_finished "git clean -fdx" &&
+   git checkout -b test$test_count &&
test_config mergetool.keepTemporaries true &&
test_must_fail git merge move-to-b &&
! (echo a; echo n) | git mergetool a/a/file.txt &&
@@ -640,7 +645,7 @@ test_expect_success MKTEMP 'temporary filenames are used 
with mergetool.writeToT
 
 test_expect_success 'diff.orderFile configuration is honored' '
test_when_finished "git reset --hard >/dev/null" &&
-   git checkout order-file-side2 &&
+   git checkout -b test$test_count order-file-side2 &&
test_config diff.orderFile order-file &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
test_config mergetool.myecho.trustExitCode true &&
@@ -658,6 +663,7 @@ test_expect_success 'diff.orderFile configuration is 
honored' '
 '
 test_expect_success 'mergetool -Oorder-file is honored' '
test_when_finished "git reset --hard >/dev/null 2>&1" &&
+   git checkout -b test$test_count &&
test_config diff.orderFile order-file &&
test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
test_config mergetool.myecho.trustExitCode true &&
-- 
2.11.0.390.gc69c2f50cf-goog

Re: [PATCH 1/4] read-tree: use OPT_BOOL instead of OPT_SET_INT

[Junio C Hamano]

Stefan Beller  writes:

> All occurrences of OPT_SET_INT were setting the value to 1;
> internally OPT_BOOL is just that.
>
> Signed-off-by: Stefan Beller 
> ---
>  builtin/read-tree.c | 36 ++--
>  1 file changed, 18 insertions(+), 18 deletions(-)

The result is much easier to read, partly because the "1" (true) is
at the very end of the line when OPT_SET_INT() is used for the
reader to notice that this is merely a boolean.

More importantly, as OPT_SET_INT() can be used multiple times on the
same variable (or field) to set it to different values, it is easier
to read if OPT_BOOL() is used when OPT_SET_INT() to 1 is not used
for that purpose.

Thanks.

>> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> index fa6edb35b2..8ba64bc785 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -109,34 +109,34 @@ int cmd_read_tree(int argc, const char **argv, const 
> char *unused_prefix)
>   { OPTION_CALLBACK, 0, "index-output", NULL, N_("file"),
> N_("write resulting index to "),
> PARSE_OPT_NONEG, index_output_cb },
> - OPT_SET_INT(0, "empty", _empty,
> - N_("only empty the index"), 1),
> + OPT_BOOL(0, "empty", _empty,
> + N_("only empty the index")),
>   OPT__VERBOSE(_update, N_("be verbose")),
>   OPT_GROUP(N_("Merging")),
> - OPT_SET_INT('m', NULL, ,
> - N_("perform a merge in addition to a read"), 1),
> - OPT_SET_INT(0, "trivial", _merges_only,
> - N_("3-way merge if no file level merging 
> required"), 1),
> - OPT_SET_INT(0, "aggressive", ,
> - N_("3-way merge in presence of adds and removes"), 
> 1),
> - OPT_SET_INT(0, "reset", ,
> - N_("same as -m, but discard unmerged entries"), 1),
> + OPT_BOOL('m', NULL, ,
> +  N_("perform a merge in addition to a read")),
> + OPT_BOOL(0, "trivial", _merges_only,
> +  N_("3-way merge if no file level merging required")),
> + OPT_BOOL(0, "aggressive", ,
> +  N_("3-way merge in presence of adds and removes")),
> + OPT_BOOL(0, "reset", ,
> +  N_("same as -m, but discard unmerged entries")),
>   { OPTION_STRING, 0, "prefix", , 
> N_("/"),
> N_("read the tree into the index under /"),
> PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP },
> - OPT_SET_INT('u', NULL, ,
> - N_("update working tree with merge result"), 1),
> + OPT_BOOL('u', NULL, ,
> +  N_("update working tree with merge result")),
>   { OPTION_CALLBACK, 0, "exclude-per-directory", ,
> N_("gitignore"),
> N_("allow explicitly ignored files to be overwritten"),
> PARSE_OPT_NONEG, exclude_per_directory_cb },
> - OPT_SET_INT('i', NULL, _only,
> - N_("don't check the working tree after merging"), 
> 1),
> + OPT_BOOL('i', NULL, _only,
> +  N_("don't check the working tree after merging")),
>   OPT__DRY_RUN(_run, N_("don't update the index or the 
> work tree")),
> - OPT_SET_INT(0, "no-sparse-checkout", _sparse_checkout,
> - N_("skip applying sparse checkout filter"), 1),
> - OPT_SET_INT(0, "debug-unpack", _unpack,
> - N_("debug unpack-trees"), 1),
> + OPT_BOOL(0, "no-sparse-checkout", _sparse_checkout,
> +  N_("skip applying sparse checkout filter")),
> + OPT_BOOL(0, "debug-unpack", _unpack,
> +  N_("debug unpack-trees")),
>   OPT_END()
>   };

Re: [PATCH 2/4] t1000: modernize style

[Junio C Hamano]

Stefan Beller  writes:

> The preferred style in tests seems to be

s/seems to be/is/;

>
> test_expect_success 'short description, ended by 2 single quotes' '
>   here comes the test &&
>   and chains over many lines &&
>   ended by a quote
> '

Thanks.  This is way overdue.  During the time the script has been
dormant for more than two years, we should have done this.

Re: [PATCH v4 14/14] mergetool: fix running in subdir when rerere enabled

[Johannes Sixt]

Am 10.01.2017 um 20:25 schrieb Junio C Hamano:

Johannes Sixt  writes:

BTW, the --sq and eval business is not required here. At this point,
$IFS = $'\n', so

set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")

will do. (Except that it would not detect errors.)


I thought you are suggesting not to use --sq but it is still there.


A copy-paste-p. Of course, I want to suggest not to use --sq.


Unrelated, but I notice that in this:

eval "set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")"
shift

It is my fault but it is a roundabout way to say:

eval "set $(git rev-parse --sq --prefix "$prefix" -- "$@")"


Clever! But I fear that half a year down the road we will appreciate a 
comment like


# rev-parse provides the -- needed for `set`

and then we are back at two lines, so I dunno...

-- Hannes

Re: git-mergetool to be used for rebases as well?

[Junio C Hamano]

Stefan Beller  writes:

> $ git status
> rebase in progress; onto ...
> You are currently rebasing branch '...' on '...'.
>
> Changes to be committed:
> modified:   ...
>
> Unmerged paths:
> both modified:   ...
>
> $ git mergetool
> No files need merging
> $ git diff 
> diff --cc 
> index ...
> --- a/file
> +++ b/file
> @@@ ...
>   content
> ++<<< HEAD
>  +  content
> ++===
> +   content
> ++>>> other commit
> content
>
>
> The mergetool used to work apparently, but stopped for rebases.
> I noticed in neither t7610-mergetool.sh nor any rebase test the combination of
> rebase and mergetool is tested.

The above redacts too much to be useful for guessing, but are you by
any chance being hit by a recent regression, i.e. have rerere enabled
and running mergetool from a subdirectory?

See <20170109232941.43637-15-hans...@google.com> (and previous
rounds).

Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile

[Junio C Hamano]

Junio C Hamano  writes:

> Richard Hansen  writes:
>
>> On 2017-01-10 01:58, Jeff King wrote:
>> ...
>>> What happens in a bare repository?
>>>
>>> I'm guessing it's relative to the top-level of the repository,
>>
>> I just tried it out and it's relative to $PWD.
>
> That is understandable.  When the user says
>
> $ cmd -O $file
>
> with any option -O that takes a filename, it is most natural if we
> used $PWD/$file when $file is not absolute path.

Ahh, ignore me in the above.  The discussion is about the
configuration variable, and I agree that being relative to GIT_DIR
would have made more sense.  In fact taking it as relative to PWD
does not make any sense.

We should have been a lot more careful when we added 6d8940b562
("diff: add diff.orderfile configuration variable", 2013-12-18), but
it is too late to complain now.

A related tangent.  

I wonder if anything that uses git_config_pathname() should be
relative to GIT_DIR when it is not absolute.

Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile

[Junio C Hamano]

Richard Hansen  writes:

> On 2017-01-10 01:58, Jeff King wrote:
> ...
>> What happens in a bare repository?
>>
>> I'm guessing it's relative to the top-level of the repository,
>
> I just tried it out and it's relative to $PWD.

That is understandable.  When the user says

$ cmd -O $file

with any option -O that takes a filename, it is most natural if we
used $PWD/$file when $file is not absolute path.

Re: [PATCH 2/2] diff: document the pattern format for diff.orderFile

[Junio C Hamano]

Richard Hansen  writes:

> Document the format of the patterns used for the diff.orderFile
> setting and diff's '-O' option by referring the reader to the
> gitignore[5] page.
>
> Signed-off-by: Richard Hansen 
> ---
>  Documentation/diff-config.txt  | 3 ++-
>  Documentation/diff-options.txt | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
> index 875212045..a35ecdd6b 100644
> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -100,7 +100,8 @@ diff.noprefix::
>  
>  diff.orderFile::
>   File indicating how to order files within a diff, using
> - one shell glob pattern per line.
> + one glob pattern per line.
> + See linkgit:gitignore[5] for the pattern format.


I do not think it is wise to suggest referring to gitignore, as the
logic of matching is quite different, other than the fact that they
both use wildmatch() internally.  Also, unlike gitignore, orderfile
does not allow any negative matching i.e. "!".

>   If `diff.orderFile` is a relative pathname, it is treated as
>   relative to the top of the work tree.
>   Can be overridden by the '-O' option to linkgit:git-diff[1].
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index e6215c372..dc6b1af71 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -467,7 +467,8 @@ endif::git-format-patch[]
>  
>  -O::
>   Output the patch in the order specified in the
> - , which has one shell glob pattern per line.
> + , which has one glob pattern per line.
> + See linkgit:gitignore[5] for the pattern format.
>   This overrides the `diff.orderFile` configuration variable
>   (see linkgit:git-config[1]).  To cancel `diff.orderFile`,
>   use `-O/dev/null`.

[PATCH] index: improve constness for reading blob data

[Brandon Williams]

Improve constness of the index_state parameter to the
'read_blob_data_from_index' function.

Signed-off-by: Brandon Williams 
---
 cache.h  | 2 +-
 read-cache.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index a50a61a19..363953c1a 100644
--- a/cache.h
+++ b/cache.h
@@ -599,7 +599,7 @@ extern int chmod_index_entry(struct index_state *, struct 
cache_entry *ce, char
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry 
*b);
 extern void set_object_name_for_intent_to_add_entry(struct cache_entry *ce);
 extern int index_name_is_other(const struct index_state *, const char *, int);
-extern void *read_blob_data_from_index(struct index_state *, const char *, 
unsigned long *);
+extern void *read_blob_data_from_index(const struct index_state *, const char 
*, unsigned long *);
 
 /* do stat comparison even if CE_VALID is true */
 #define CE_MATCH_IGNORE_VALID  01
diff --git a/read-cache.c b/read-cache.c
index f92a912dc..6ee02 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2292,7 +2292,8 @@ int index_name_is_other(const struct index_state *istate, 
const char *name,
return 1;
 }
 
-void *read_blob_data_from_index(struct index_state *istate, const char *path, 
unsigned long *size)
+void *read_blob_data_from_index(const struct index_state *istate,
+   const char *path, unsigned long *size)
 {
int pos, len;
unsigned long sz;
-- 
2.11.0.390.gc69c2f50cf-goog

Re: [PATCHv2 4/5] unpack-trees: factor file removal out of check_updates

[Junio C Hamano]

Stefan Beller  writes:

> This makes check_updates shorter and easier to understand.
>
> Signed-off-by: Stefan Beller 
> ---

I agree that 3/5 made it easier to understand by ejecting a block
that is not essential to the functionality of the function out of
it, making the remainder of the fuction about "removing gone files
and then write out the modified files".  

The ejecting of the first half of these two operations, both are
what this function is about, done by this step feels backwards.  If
anything, the "only do the actual working tree manipulation when not
doing a dry-run and told to update" logic that must be in both are
spread in two helper functions after step 5/5, and with the added
boilerplate for these two helpers, the end result becomes _longer_
to understand what is really going on when check_updates() is
called.

Is the original after step 3/5 too long and hard to understand?

>  unpack-trees.c | 29 +
>  1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index b564024472..ac59510251 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -218,6 +218,26 @@ static void unlink_entry(const struct cache_entry *ce)
>   schedule_dir_for_removal(ce->name, ce_namelen(ce));
>  }
>  
> +static unsigned remove_workingtree_files(struct unpack_trees_options *o,
> +  struct progress *progress)
> +{
> + int i;
> + unsigned cnt = 0;
> + struct index_state *index = >result;
> +
> + for (i = 0; i < index->cache_nr; i++) {
> + const struct cache_entry *ce = index->cache[i];
> +
> + if (ce->ce_flags & CE_WT_REMOVE) {
> + display_progress(progress, ++cnt);
> + if (o->update && !o->dry_run)
> + unlink_entry(ce);
> + }
> + }
> +
> + return cnt;
> +}
> +
>  static struct progress *get_progress(struct unpack_trees_options *o)
>  {
>   unsigned cnt = 0, total = 0;
> @@ -254,15 +274,8 @@ static int check_updates(struct unpack_trees_options *o)
>  
>   if (o->update)
>   git_attr_set_direction(GIT_ATTR_CHECKOUT, index);
> - for (i = 0; i < index->cache_nr; i++) {
> - const struct cache_entry *ce = index->cache[i];
>  
> - if (ce->ce_flags & CE_WT_REMOVE) {
> - display_progress(progress, ++cnt);
> - if (o->update && !o->dry_run)
> - unlink_entry(ce);
> - }
> - }
> + cnt = remove_workingtree_files(o, progress);
>   remove_marked_cache_entries(index);
>   remove_scheduled_dirs();

Re: [PATCHv2 2/5] unpack-trees: remove unneeded continue

[Junio C Hamano]

Stefan Beller  writes:

> The continue is the last statement in the loop, so not needed.
> This situation arose in 700e66d66 (2010-07-30, unpack-trees: let
> read-tree -u remove index entries outside sparse area) when statements
> after the continue were removed.
>
> Signed-off-by: Stefan Beller 
> ---

Thanks for digging the history to find the culprit.  

This is an unrelated tangent, but I do not think of a way other than
"log -L" to find it; conceptually, "blame" ought to be usable for
it, but it is not useful for finding deleted lines.

>  unpack-trees.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 8e6768f283..d443decb23 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -253,7 +253,6 @@ static int check_updates(struct unpack_trees_options *o)
>   display_progress(progress, ++cnt);
>   if (o->update && !o->dry_run)
>   unlink_entry(ce);
> - continue;
>   }
>   }
>   remove_marked_cache_entries(index);

Re: [PATCH v4 14/14] mergetool: fix running in subdir when rerere enabled

[Junio C Hamano]

Johannes Sixt  writes:

>> +prefix=$(git rev-parse --show-prefix) || exit 1
>> +cd_to_toplevel
>> +
>> +if test -n "$orderfile"
>> +then
>> +orderfile=$(git rev-parse --prefix "$prefix" -- "$orderfile") 
>> || exit 1
>> +orderfile=$(printf %s\\n "$orderfile" | sed -e 1d)
>
> Is the purpose of this complication only to detect errors of the git
> invocation? IMHO, we could dispense with that, but others might
> disagree. I am arguing because this adds yet another process; but it
> is only paid when -O is used, so...

I do not terribly mind an added process, but this change makes it
harder to read and also forces the readers to wonder if the quoting
around printf is correct.

>> @@ -461,14 +470,17 @@ main () {
>>  then
>>  print_noop_and_exit
>>  fi
>> +elif test $# -ge 0
>> +then
>> +files_quoted=$(git rev-parse --sq --prefix "$prefix" -- "$@") 
>> || exit 1
>> +eval "set -- $files_quoted"
>
> BTW, the --sq and eval business is not required here. At this point,
> $IFS = $'\n', so
>
>   set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")
>
> will do. (Except that it would not detect errors.)

I thought you are suggesting not to use --sq but it is still there.
I think the original is written in such a way that any letter in
each of "$@" is preserved, even an LF in the filename.

Such a pathname probably won't correctly be given from the "rerere
remaining" side (i.e. when you are lazy and run mergetool without
pathname), so it may appear not to matter, but being able to give an
explicit pathname from the command line is a workaround for that, so
in that sense, it has value to prepare this side of the codepath to
be able to cope with such a pathname.

Unrelated, but I notice that in this:

eval "set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")"
shift

"set" will get one "--" from its direct command line argument,
followed by "--" that comes out of rev-parse, and then the first
pathname (i.e. "$prefix/$1", if "$1" is relative).  Then the first
"--" is discarded and the second "--" that came out of rev-parse
becomes "$1", and the first pathname becomes "$2", etc.  We use
"shift" to get rid of "--" and move everything down by one.

It is my fault but it is a roundabout way to say:

eval "set $(git rev-parse --sq --prefix "$prefix" -- "$@")"

I would think, and we should probably want to write this like so.
[PATCH v4 02/14] would probably want to be updated as well.

I can locally update them if everybody agrees it is a good idea.

git-mergetool to be used for rebases as well?

[Stefan Beller]

An internal user report running on origin/next:

$ git pull
From .
 * branch... -> FETCH_HEAD
First, rewinding head to replay your work on top of it...
Applying: ...
Applying: ...
Using index info to reconstruct a base tree...
CONFLICT (content): 
error: Failed to merge in the changes.
Patch failed at 0002...
The copy of the patch that failed is found in: .git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase 
--abort".
$ git status
rebase in progress; onto ...
You are currently rebasing branch '...' on '...'.

Changes to be committed:
modified:   ...

Unmerged paths:
both modified:   ...

$ git mergetool
No files need merging
$ git diff 
diff --cc 
index ...
--- a/file
+++ b/file
@@@ ...
  content
++<<< HEAD
 +  content
++===
+   content
++>>> other commit
content


The mergetool used to work apparently, but stopped for rebases.
I noticed in neither t7610-mergetool.sh nor any rebase test the combination of
rebase and mergetool is tested.

Stefan

Re: [RFC PATCH 0/5] Localise error headers

[Stefan Beller]

On Tue, Jan 10, 2017 at 1:04 AM, Jeff King  wrote:
> On Mon, Jan 09, 2017 at 01:43:15PM +0100, Michael J Gruber wrote:
>
>> > I can't say I'm excited about having matching "_" variants for each
>> > function. Are we sure that they are necessary? I.e., would it be
>> > acceptable to just translate them always?
>>
>> We would still need to mark the strings, e.g.
>>
>> die(N_("oopsie"));
>>
>> and would not be able to opt out of translating in the code (only in the
>> po file, by not providing a translation).
>
> I meant more along the lines of: would it be OK to just always translate
> the prefix, even if the message itself is not translated? I.e.,
>
> diff --git a/usage.c b/usage.c
> index 82ff13163..8e5400f57 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -32,7 +32,7 @@ static NORETURN void usage_builtin(const char *err, va_list 
> params)
>
>  static NORETURN void die_builtin(const char *err, va_list params)
>  {
> -   vreportf("fatal: ", err, params);
> +   vreportf(_("fatal: "), err, params);
> exit(128);
>  }
>
>> In any case, the question is whether we want to tell the user
>>
>> A: B
>>
>> where A is in English and B is localised, or rather localise both A and
>> B (for A in "error", "fatal", "warning"...).
>>
>> For localising A and B, we'd need this series or something similar. For
>> keeping the mix, we don't need to do anything ;)
>
> What I wrote above would keep the mix, but switch it in the other
> direction.
>
> And then presumably that mix would gradually move to 100% consistency as
> more messages are translated. But the implicit question is: are there
> die() messages that should never be translated? I'm not sure.

I would assume any plumbing command is not localizing?
Because in plumbing land, (easily scriptable) you may find
a grep on the output/stderr for a certain condition?

To find a good example, "git grep die" giving me some food of though:

die_errno(..) should always take a string marked up for translation,
because the errno string is translated?
(-> we'd have to fix up any occurrence of git grep "die_errno(\"")

apply.c:die(_("internal error"));

That is funny, too. I think we should substitute that with

die("BUG: untranslated, but what went wrong instead")


>
> -Peff

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

[Junio C Hamano]

Stefan Beller  writes:

>   This comes as a single patch again, replacing sb/pathspec-errors.
>   It goes directly on top of bw/pathspec-cleanup.
>
>   v8:
>   cleaned white spaces in commit message
>   removed TEST_CREATE_SUBMODULE=yes
>   : > instead of touch.
>
>   v7:
>   do not rely on "test_commit -C" being there, nor the infrastructure
>   to request a "good" submodule upstream. Just create a submodule outselves
>   to test in.
>
>   Thanks,
>   Stefan

Thanks, queued.

Re: [PATCH 1/2] diff: document behavior of relative diff.orderFile

[Richard Hansen]

On 2017-01-10 01:58, Jeff King wrote:

On Mon, Jan 09, 2017 at 07:40:30PM -0500, Richard Hansen wrote:


Document that a relative pathname for diff.orderFile is interpreted as
relative to the top-level work directory.

Signed-off-by: Richard Hansen 
---
 Documentation/diff-config.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 58f4bd6af..875212045 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -101,6 +101,8 @@ diff.noprefix::
 diff.orderFile::
File indicating how to order files within a diff, using
one shell glob pattern per line.
+   If `diff.orderFile` is a relative pathname, it is treated as
+   relative to the top of the work tree.
Can be overridden by the '-O' option to linkgit:git-diff[1].


What happens in a bare repository?


I didn't know which is why this patch is silent on that topic.  :)



I'm guessing it's relative to the top-level of the repository,


I just tried it out and it's relative to $PWD.


but we should probably spell it out.


Do we want it to be relative to $PWD?  I think relative to $GIT_DIR is 
more useful.  If we want it to be relative to $GIT_DIR, then I think we 
should stay silent regarding bare repositories so that the behavior 
remains unspecified until we update the logic.




The other case is --no-index when we are not in a repository at all, but
that should just be relative to the current directory,


It is.


which isn't really worth mentioning.


Agreed.

I'll post a re-roll if people prefer the current behavior over relative 
to $GIT_DIR.


Thanks for reviewing,
Richard



-Peff




smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v4 14/14] mergetool: fix running in subdir when rerere enabled

[Richard Hansen]

On 2017-01-10 01:17, Johannes Sixt wrote:

Am 10.01.2017 um 00:29 schrieb Richard Hansen:

The pathnames output by the 'git rerere remaining' command are
relative to the top-level directory but the 'git diff --name-only'
command expects its pathname arguments to be relative to the current
working directory.  Run cd_to_toplevel before running 'git diff
--name-only' and adjust any relative pathnames so that 'git mergetool'
does not fail when run from a subdirectory with rerere enabled.

This fixes a regression introduced in
57937f70a09c12ef484c290865dac4066d207c9c (v2.11.0).

Based-on-patch-by: Junio C Hamano 
Signed-off-by: Richard Hansen 
---
 git-mergetool.sh | 16 ++--
 t/t7610-mergetool.sh |  7 ++-
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index b506896dc..cba6bbd05 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -454,6 +454,15 @@ main () {
 merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
 merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo 
false)"

+prefix=$(git rev-parse --show-prefix) || exit 1
+cd_to_toplevel
+
+if test -n "$orderfile"
+then
+orderfile=$(git rev-parse --prefix "$prefix" -- "$orderfile") || exit 1
+orderfile=$(printf %s\\n "$orderfile" | sed -e 1d)


Is the purpose of this complication only to detect errors of the git
invocation?


Yes.  I've been burned so many times by the shell not stopping when 
there's an error that I always like to do things one step at a time with 
error checking, even if it is less efficient.



IMHO, we could dispense with that, but others might
disagree. I am arguing because this adds yet another process; but it is
only paid when -O is used, so...


+fi
+
 if test $# -eq 0 && test -e "$GIT_DIR/MERGE_RR"
 then
 set -- $(git rerere remaining)
@@ -461,14 +470,17 @@ main () {
 then
 print_noop_and_exit
 fi
+elif test $# -ge 0
+then
+files_quoted=$(git rev-parse --sq --prefix "$prefix" -- "$@") || exit 1
+eval "set -- $files_quoted"


BTW, the --sq and eval business is not required here. At this point,
$IFS = $'\n',


Whoa, really?  That's surprising and feels wrong.

(BTW, I just noticed that guess_merge_tool sets IFS to a space without 
resetting it afterward.  That function is always run in a subshell so 
there's no problem at the moment, but it's still a bit risky.)



so

set -- $(git rev-parse --sq --prefix "$prefix" -- "$@")

will do. (Except that it would not detect errors.)


I think I would prefer to leave it as-is in case IFS is ever changed 
back to the default.





+shift
 fi


As I don't see anything wrong with what you have written, these comments
alone do not warrant another re-roll.


I'll reroll if I hear other votes in favor of changing.  Thanks for 
reviewing!


-Richard



-- Hannes





smime.p7s
Description: S/MIME Cryptographic Signature

Re: [PATCH v5 0/4] Per-worktree config file support

[Stefan Beller]

On Tue, Jan 10, 2017 at 3:25 AM, Nguyễn Thái Ngọc Duy  wrote:
> Let's get this rolling again. To refresh your memory because it's half
> a year since v4 [1], this is about letting each worktree in multi
> worktree setup has their own config settings. The most prominent ones
> are core.worktree, used by submodules, and core.sparseCheckout.

Thanks for getting this rolling again.

>
> This time I'm not touching submodules at all. I'm leaving it in the
> good hands of "submodule people". All I'm providing is mechanism. How
> you use it is up to you. So the series benefits sparse checkout users
> only.

As one of the "submodule people", I have no complaints here.

>
> Not much has changed from v4, except that the migration to new config
> layout is done automatically _update_ a config variable with "git
> config --worktree".
>
> I think this one is more or less ready. I have an RFC follow-up patch
> about core.bare, but that could be handled separately.

I have read through the series and think the design is sound for worktrees
(though I have little knowledge about them).

---
Now even further:

So to build on top of this series, I'd like to make submodules usable
with worktrees (i.e. shared object store, only clone/fetch once and
all worktrees
benefit from it), the big question is how to get the underlying data
model right.

Would a submodule go into the superprojects

.git/worktrees//modules//

or rather

.git/modules/worktrees/

Or both (one of them being a gitlink file pointing at the other?)

I have not made up my mind, as I haven't laid out all cases that are
relevant here.

Thanks,
Stefan


>
> [1] http://public-inbox.org/git/20160720172419.25473-1-pclo...@gmail.com/
>
> Nguyễn Thái Ngọc Duy (4):
>   config: read per-worktree config files
>   config: --worktree for manipulating per-worktree config file
>   config: automatically migrate to new config layout when --worktree is used
>   t2029: add tests for per-worktree config
>
>  Documentation/config.txt   | 11 -
>  Documentation/git-config.txt   | 26 ---
>  Documentation/git-worktree.txt | 37 +++
>  Documentation/gitrepository-layout.txt |  8 
>  builtin/config.c   | 16 ++-
>  cache.h|  2 +
>  config.c   |  7 +++
>  environment.c  |  1 +
>  setup.c|  5 ++-
>  t/t2029-worktree-config.sh (new +x)| 82 
> ++
>  worktree.c | 40 +
>  worktree.h |  6 +++
>  12 files changed, 230 insertions(+), 11 deletions(-)
>  create mode 100755 t/t2029-worktree-config.sh
>
> --
> 2.8.2.524.g6ff3d78
>

Re: [PATCH v5 2/4] config: --worktree for manipulating per-worktree config file

[Stefan Beller]

> +   if (repository_format_worktree_config)
> +   given_config_source.file = 
> git_pathdup("config.worktree");
> +   else if (worktrees[0] && worktrees[1]) {
> +   die("BUG: migration is not supported yet");
> +   } else
> +   given_config_source.file = git_pathdup("config");

nit: inconsistent use uf braces for single statements here.
I mean the braces are needed in the BUG case, as otherwise we'd
have a dangling else, but then you could put the brace in the else case as well.
This nit doesn't warrant a reroll on its own.

> +   free_worktrees(worktrees);
> +   } else if (given_config_source.file) {
> if (!is_absolute_path(given_config_source.file) && prefix)
> given_config_source.file =
> xstrdup(prefix_filename(prefix,
> --
> 2.8.2.524.g6ff3d78
>

Re: Refreshing index timestamps without reading content

[Quentin Casasnovas]

On Mon, Jan 09, 2017 at 04:55:37PM +0100, Quentin Casasnovas wrote:
> On Mon, Jan 09, 2017 at 07:01:36AM -0800, Junio C Hamano wrote:
> > Duy Nguyen  writes:
> >
> > > On Thu, Jan 5, 2017 at 6:23 PM, Quentin Casasnovas
> > >  wrote:
> > >> Is there any way to tell git, after the git ls-tree command above, to
> > >> refresh its stat cache information and trust us that the file content has
> > >> not changed, as to avoid any useless file read (though it will obviously
> > >> will have to stat all of them, but that's not something we can really
> > >> avoid)
> > >
> > > I don't think there's any way to do that, unfortunately.
> >
> > Lose "unfortunately".
> >
> > >> If not, I am willing to implement a --assume-content-unchanged to the git
> > >> update-index if you guys don't see something fundamentally wrong with 
> > >> this
> > >> approach.
> > >
> > > If you do that, I think you should go with either of the following options
> > >
> > > - Extend git-update-index --index-info to take stat info as well (or
> > > maybe make a new option instead). Then you can feed stat info directly
> > > to git without a use-case-specific "assume-content-unchanged".
> > >
> > > - Add "git update-index --touch" that does what "touch" does. In this
> > > case, it blindly updates stat info to latest. But like touch, we can
> > > also specify  mtime from command line if we need to. It's a bit less
> > > generic than the above option, but easier to use.
> >
> > Even if we assume that it is a good idea to let people muck with the
> > index like this, either of the above would be a usable addition,
> > because the cached stat information does not consist solely of
> > mtime.
> >
> > "git update-index --index-info" was invented for the case where a
> > user or a script _knows_ the object ID of the blob that _would_
> > result if a contents of a file on the filesystem were run through
> > hash-object.  So from the interface's point of view, it may make
> > sense to teach it to take an extra/optional argument that is the
> > path to the file and take the stat info out of the named file when
> > the extra/optional argument was given.
> >
> > But that assumes that it is a good idea to do this in the first
> > place.  It was deliberate design decision that setting the cached
> > stat info for the entry was protected behind actual content
> > comparison, and removing that protection will open the index to
> > abuse.
> >
> 
> Hi Junio,
> 
> Thanks for your feedback, appreciated :)
> 
> I do understand how it would be possible for someone to shoot themselves in
> the feet with such option, but it solves real life use cases and improved
> build times very signficantly here.
> 
> Another use case we have is setting up very lightweight linux work trees,
> by reflinking from a base work-tree.  This allows for a completely
> different work-tree taking up almost no size at first, whereas using a
> shared clone or the recent worktree subcommand would "waste" ~500MB*:
> 
>  # linux-2.6 is a shared clone of a bare clone residing locally
>  ~ $ cp --reflink -a linux-2.6 linux-2.6-reflinked
> 
>  # At this point, the mtime inside linux-2.6-reflinked are matching the
>  # mtime of the source linux-2.6 (since we used the '-a' option of 'cp)
>  ~ $ diff -u <(stat linux-2.6/README) <(stat linux-2.6-reflinked/README)
>  --- /proc/self/fd/11  2017-01-09 16:34:04.523438942 +0100
>  +++ /proc/self/fd/12  2017-01-09 16:34:04.523438942 +0100
>  @@ -1,8 +1,8 @@
>  -  File: 'linux-2.6/README'
>  +  File: 'linux-2.6-reflinked/README'
> Size: 18372   Blocks: 40 IO Block: 4096   regular file
>  -Device: fd00h/64768dInode: 268467090   Links: 1
>  +Device: fd00h/64768dInode: 805970606   Links: 1
>   Access: (0644/-rw-r--r--)  Uid: ( 1000/ quentin)   Gid: ( 1000/ quentin)
>   Access: 2017-01-09 12:04:15.317758718 +0100
>   Modify: 2017-01-09 12:04:12.566758772 +0100
>  -Change: 2017-01-09 12:04:12.566758772 +0100
>  +Change: 2017-01-09 16:29:48.305444003 +0100
>Birth:
> 
>   # Now let's check how long it takes to refresh the index from the source
>   # and destination..
>   ~/linux-2.6 $ time git update-index --refresh
>   git update-index --refresh  0.04s user 0.08s system 204% cpu 0.058 total
>~~~
>   ~/linux-2.6-reflinked $ time git update-index --refresh
>   git update-index --refresh  2.40s user 1.43s system 38% cpu 10.003 total
>   
> 

After discussing this with my friend Vegard, he found the core.checkStat
config which, if set to 'minimal', ignores the inode number which is enough
for the above use case to work just fine - so please excuse my ignorance!

For the initial problem I had when changing the mtime of all the files in
the tree, I should be able to change the mtime of the object files instead,
hence I don't really 

Re: [musl] Re: Test failures when Git is built with libpcre and grep is built without it

[Szabolcs Nagy]

* A. Wilcox  [2017-01-10 04:36:50 -0600]:
> On 09/01/17 15:33, Jeff King wrote:
> > The problem is that we are expecting the regex "\x{2b}" to complain
> > in regcomp() (as an ERE), but it doesn't. And that probably _is_
> > related to musl, which is providing the libc regex (I know this
> > looks like a pcre test, but it's checking that "-P -E" overrides
> > the pcre option with "-E").
> > 
> > I'm not sure if musl is wrong for failing to complain about a
> > bogus regex. Generally making something that would break into
> > something that works is an OK way to extend the standard. So our
> > test is at fault for assuming that the regex will fail. I guess

\x is undefined in posix and musl is based on tre which
supports \x{hexdigits} in ere.

(i think some bsd platforms use tre as libc regex so
i'm surprised musl is the first to run into this.)

> > we'd need to find some more exotic syntax that pcre supports, but
> > that ERE doesn't. Maybe "(?:)" or something.

i think you would have to use something that's invalid
in posix ere, ? after empty expression is undefined,
not an error so "(?:)" is a valid ere extension.

since most syntax is either defined or undefined in ere
instead of being invalid, distinguishing pcre using
syntax is not easy.

there are semantic differences in subexpression matching:
leftmost match has higher priority in pcre, longest match
has higher priority in ere.

$ echo ab | grep -o -E '(a|ab)'
ab
$ echo ab | grep -o -P '(a|ab)'
a

unfortunately grep -o is not portable.

Re: [PATCH v5 0/4] Per-worktree config file support

[Duy Nguyen]

On Tue, Jan 10, 2017 at 6:25 PM, Nguyễn Thái Ngọc Duy  wrote:
> Not much has changed from v4, except that the migration to new config
> layout is done automatically _update_ a config variable with "git
> config --worktree".

I accidentally two words that may make it hard to understand this
sentence. It should be "... is done automatically when you update.."
-- 
Duy

[PATCH/RFC 5/4] Redefine core.bare in multiple working tree setting

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

When core.bare was added, time was simpler, we only had one worktree
associated to one repository. The situation gets a bit complicated when
multiple worktrees are added. If core.bare is set in the per-repo config
file, should all worktrees see this variable?

Since core.bare affects worktree-related commands (e.g. you are not
supposed to run "git status" when core.bare is true because no worktree
is supposed to link to the repository), when multi worktree is added,
core.bare is evaluated true by the main worktree only. Other worktrees
simply do not see core.bare even if it's there.

With per-worktree configuration in place, core.bare is moved to main
worktree's private config file. But it does not really make sense
because this is about _repository_. Instead we could leave core.bare in
the per-repo config and change/extend its definition from:

   If true this repository is assumed to be 'bare' and has no working
   directory associated with it.

to

   If true this repository is assumed to be 'bare' and has no _main_
   working directory associated with it.

In other words, linked worktrees are not covered by core.bare. This
definition is the same as before when it comes to single worktree setup.

A plus of this definition is, it allows a setup where we only have
linked worktrees (e.g. core.bare set to true, and the main repo is
tucked somewhere safe), which makes all worktrees equal again because
"the special one" is gone.

This patch is incomplete. I need to go through all is_bare_repository()
calls and adjust their behavior. But I wanted to run the idea through
the community first..

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/config.txt   | 2 +-
 Documentation/git-worktree.txt | 7 +++
 t/t2029-worktree-config.sh | 4 ++--
 worktree.c | 6 --
 4 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c508386..ff146be 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -484,7 +484,7 @@ core.preferSymlinkRefs::
expect HEAD to be a symbolic link.
 
 core.bare::
-   If true this repository is assumed to be 'bare' and has no
+   If true this repository is assumed to be 'bare' and has no main
working directory associated with it.  If this is the case a
number of commands that require a working directory will be
disabled, such as linkgit:git-add[1] or linkgit:git-merge[1].
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index f5aad0a..a331d0a 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -161,7 +161,7 @@ them to the `config.worktree` of the main working 
directory. You may
 also take this opportunity to move other configuration that you do not
 want to share to all working directories:
 
- - `core.worktree` and `core.bare` should never be shared
+ - `core.worktree` should never be shared
 
  - `core.sparseCheckout` is recommended per working directory, unless
you are sure you always use sparse checkout for all working
@@ -169,9 +169,8 @@ want to share to all working directories:
 
 When `git config --worktree` is used to set a configuration variable
 in multiple working directory setup, `extensions.worktreeConfig` will
-be automatically set. The two variables `core.worktree` and
-`core.bare` if present will be moved to `config.worktree` of the main
-working tree.
+be automatically set. The variable `core.worktree` if present will be
+moved to `config.worktree` of the main working tree.
 
 DETAILS
 ---
diff --git a/t/t2029-worktree-config.sh b/t/t2029-worktree-config.sh
index 4ebdf13..dc84c94 100755
--- a/t/t2029-worktree-config.sh
+++ b/t/t2029-worktree-config.sh
@@ -70,13 +70,13 @@ test_expect_success 'config.worktree no longer read without 
extension' '
cmp_config -C wt2 shared this.is
 '
 
-test_expect_success 'config --worktree migrate core.bare and core.worktree' '
+test_expect_success 'config --worktree migrate core.worktree' '
git config core.bare true &&
git config --worktree foo.bar true &&
cmp_config true extensions.worktreeConfig &&
cmp_config true foo.bar &&
cmp_config true core.bare &&
-   ! git -C wt1 config core.bare
+   cmp_config -C wt1 true core.bare
 '
 
 test_done
diff --git a/worktree.c b/worktree.c
index d8c9d85..c07cc50 100644
--- a/worktree.c
+++ b/worktree.c
@@ -395,12 +395,6 @@ void migrate_worktree_config(void)
read_repository_format(, main_path.buf);
assert(format.worktree_config == 0);
 
-   if (format.is_bare >= 0) {
-   git_config_set_in_file(worktree_path.buf,
-  "core.bare", "true");
-   git_config_set_in_file(main_path.buf,
-  "core.bare", NULL);
-   }
if (format.work_tree) {
git_config_set_in_file(worktree_path.buf,
  

[PATCH v5 4/4] t2029: add tests for per-worktree config

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 t/t2029-worktree-config.sh (new +x) | 82 +
 1 file changed, 82 insertions(+)
 create mode 100755 t/t2029-worktree-config.sh

diff --git a/t/t2029-worktree-config.sh b/t/t2029-worktree-config.sh
new file mode 100755
index 000..4ebdf13
--- /dev/null
+++ b/t/t2029-worktree-config.sh
@@ -0,0 +1,82 @@
+#!/bin/sh
+
+test_description="config file in multi worktree"
+
+. ./test-lib.sh
+
+cmp_config() {
+   if [ "$1" = "-C" ]; then
+   shift &&
+   GD="-C $1" &&
+   shift
+   else
+   GD=
+   fi &&
+   echo "$1" >expected &&
+   shift &&
+   git $GD config "$@" >actual &&
+   test_cmp expected actual
+}
+
+test_expect_success 'setup' '
+   test_commit start &&
+   git config --worktree per.worktree is-ok &&
+   git worktree add wt1 &&
+   git worktree add wt2 &&
+   git config --worktree per.worktree is-ok &&
+   cmp_config true extensions.worktreeConfig
+'
+
+test_expect_success 'config is shared as before' '
+   git config this.is shared &&
+   cmp_config shared this.is &&
+   cmp_config -C wt1 shared this.is &&
+   cmp_config -C wt2 shared this.is
+'
+
+test_expect_success 'config is shared (set from another worktree)' '
+   git -C wt1 config that.is also-shared &&
+   cmp_config also-shared that.is &&
+   cmp_config -C wt1 also-shared that.is &&
+   cmp_config -C wt2 also-shared that.is
+'
+
+test_expect_success 'config private to main worktree' '
+   git config --worktree this.is for-main &&
+   cmp_config for-main this.is &&
+   cmp_config -C wt1 shared this.is &&
+   cmp_config -C wt2 shared this.is
+'
+
+test_expect_success 'config private to linked worktree' '
+   git -C wt1 config --worktree this.is for-wt1 &&
+   cmp_config for-main this.is &&
+   cmp_config -C wt1 for-wt1 this.is &&
+   cmp_config -C wt2 shared this.is
+'
+
+test_expect_success 'core.bare no longer for main only' '
+   git config core.bare true &&
+   cmp_config true core.bare &&
+   cmp_config -C wt1 true core.bare &&
+   cmp_config -C wt2 true core.bare &&
+   git config --unset core.bare
+'
+
+test_expect_success 'config.worktree no longer read without extension' '
+   git config --unset extensions.worktreeConfig &&
+   cmp_config shared this.is &&
+   cmp_config -C wt1 shared this.is &&
+   cmp_config -C wt2 shared this.is
+'
+
+test_expect_success 'config --worktree migrate core.bare and core.worktree' '
+   git config core.bare true &&
+   git config --worktree foo.bar true &&
+   cmp_config true extensions.worktreeConfig &&
+   cmp_config true foo.bar &&
+   cmp_config true core.bare &&
+   ! git -C wt1 config core.bare
+'
+
+test_done
-- 
2.8.2.524.g6ff3d78

[PATCH v5 0/4] Per-worktree config file support

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

Let's get this rolling again. To refresh your memory because it's half
a year since v4 [1], this is about letting each worktree in multi
worktree setup has their own config settings. The most prominent ones
are core.worktree, used by submodules, and core.sparseCheckout.

This time I'm not touching submodules at all. I'm leaving it in the
good hands of "submodule people". All I'm providing is mechanism. How
you use it is up to you. So the series benefits sparse checkout users
only.

Not much has changed from v4, except that the migration to new config
layout is done automatically _update_ a config variable with "git
config --worktree".

I think this one is more or less ready. I have an RFC follow-up patch
about core.bare, but that could be handled separately.

[1] http://public-inbox.org/git/20160720172419.25473-1-pclo...@gmail.com/

Nguyễn Thái Ngọc Duy (4):
  config: read per-worktree config files
  config: --worktree for manipulating per-worktree config file
  config: automatically migrate to new config layout when --worktree is used
  t2029: add tests for per-worktree config

 Documentation/config.txt   | 11 -
 Documentation/git-config.txt   | 26 ---
 Documentation/git-worktree.txt | 37 +++
 Documentation/gitrepository-layout.txt |  8 
 builtin/config.c   | 16 ++-
 cache.h|  2 +
 config.c   |  7 +++
 environment.c  |  1 +
 setup.c|  5 ++-
 t/t2029-worktree-config.sh (new +x)| 82 ++
 worktree.c | 40 +
 worktree.h |  6 +++
 12 files changed, 230 insertions(+), 11 deletions(-)
 create mode 100755 t/t2029-worktree-config.sh

-- 
2.8.2.524.g6ff3d78

[PATCH v5 2/4] config: --worktree for manipulating per-worktree config file

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

As noted in the previous commit, "git config" without options will read
both per-worktree and per-repo by default. --worktree is needed to read
just per-worktree config. Writing goes to per-repo by default though,
unless --worktree is given.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-config.txt | 22 +++---
 builtin/config.c | 15 ++-
 2 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 806873c..ead33a8 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -47,13 +47,15 @@ checks or transformations are performed on the value.
 
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
-`--system`, `--global`, `--local` and `--file ` can be
-used to tell the command to read from only that location (see <>).
+`--system`, `--global`, `--local`, `--worktree` and
+`--file ` can be used to tell the command to read from only
+that location (see <>).
 
 When writing, the new value is written to the repository local
 configuration file by default, and options `--system`, `--global`,
-`--file ` can be used to tell the command to write to
-that location (you can say `--local` but that is the default).
+`--worktree`, `--file ` can be used to tell the command to
+write to that location (you can say `--local` but that is the
+default).
 
 This command will fail with non-zero status upon error.  Some exit
 codes are:
@@ -133,6 +135,11 @@ from all available files.
 +
 See also <>.
 
+--worktree::
+   Similar to `--local` except that `.git/config.worktree` is
+   read from or written to if `extensions.worktreeConfig` is
+   present. If not it's the same as `--local`.
+
 -f config-file::
 --file config-file::
Use the given config file instead of the one specified by GIT_CONFIG.
@@ -275,9 +282,10 @@ configuration file. Note that this also affects options 
like `--replace-all`
 and `--unset`. *'git config' will only ever change one file at a time*.
 
 You can override these rules either by command-line options or by environment
-variables. The `--global` and the `--system` options will limit the file used
-to the global or system-wide file respectively. The `GIT_CONFIG` environment
-variable has a similar effect, but you can specify any filename you want.
+variables. The `--global`, `--system` and `--worktree` options will limit
+the file used to the global, system-wide or per-worktree file respectively.
+The `GIT_CONFIG` environment variable has a similar effect, but you
+can specify any filename you want.
 
 
 ENVIRONMENT
diff --git a/builtin/config.c b/builtin/config.c
index 05843a0..7d390af 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -4,6 +4,7 @@
 #include "parse-options.h"
 #include "urlmatch.h"
 #include "quote.h"
+#include "worktree.h"
 
 static const char *const builtin_config_usage[] = {
N_("git config []"),
@@ -23,6 +24,7 @@ static char key_delim = ' ';
 static char term = '\n';
 
 static int use_global_config, use_system_config, use_local_config;
+static int use_worktree_config;
 static struct git_config_source given_config_source;
 static int actions, types;
 static int end_null;
@@ -56,6 +58,7 @@ static struct option builtin_config_options[] = {
OPT_BOOL(0, "global", _global_config, N_("use global config file")),
OPT_BOOL(0, "system", _system_config, N_("use system config file")),
OPT_BOOL(0, "local", _local_config, N_("use repository config 
file")),
+   OPT_BOOL(0, "worktree", _worktree_config, N_("use per-worktree 
config file")),
OPT_STRING('f', "file", _config_source.file, N_("file"), N_("use 
given config file")),
OPT_STRING(0, "blob", _config_source.blob, N_("blob-id"), 
N_("read config from given blob object")),
OPT_GROUP(N_("Action")),
@@ -490,6 +493,7 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
 PARSE_OPT_STOP_AT_NON_OPTION);
 
if (use_global_config + use_system_config + use_local_config +
+   use_worktree_config +
!!given_config_source.file + !!given_config_source.blob > 1) {
error("only one config file at a time.");
usage_with_options(builtin_config_usage, 
builtin_config_options);
@@ -524,7 +528,16 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
given_config_source.file = git_etc_gitconfig();
else if (use_local_config)
given_config_source.file = git_pathdup("config");
-   else if (given_config_source.file) {
+   else if (use_worktree_config) {
+   struct worktree **worktrees = get_worktrees(0);
+   if (repository_format_worktree_config)
+   given_config_source.file = 
git_pathdup("config.worktree");
+   else if (worktrees[0] && 

[PATCH v5 3/4] config: automatically migrate to new config layout when --worktree is used

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

It's not fun to ask the user to set extensions.worktreeConfig manually.
It's error-prone too. So we do it automatically whenever anybody sets a
per-worktree config with "git config" (support for builtin commands is
coming later).

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-worktree.txt |  6 ++
 builtin/config.c   |  3 ++-
 worktree.c | 40 
 worktree.h |  6 ++
 4 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 329a673..f5aad0a 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -167,6 +167,12 @@ want to share to all working directories:
you are sure you always use sparse checkout for all working
directories.
 
+When `git config --worktree` is used to set a configuration variable
+in multiple working directory setup, `extensions.worktreeConfig` will
+be automatically set. The two variables `core.worktree` and
+`core.bare` if present will be moved to `config.worktree` of the main
+working tree.
+
 DETAILS
 ---
 Each linked working tree has a private sub-directory in the repository's
diff --git a/builtin/config.c b/builtin/config.c
index 7d390af..9dafefd 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -533,7 +533,8 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
if (repository_format_worktree_config)
given_config_source.file = 
git_pathdup("config.worktree");
else if (worktrees[0] && worktrees[1]) {
-   die("BUG: migration is not supported yet");
+   migrate_worktree_config();
+   given_config_source.file = 
git_pathdup("config.worktree");
} else
given_config_source.file = git_pathdup("config");
free_worktrees(worktrees);
diff --git a/worktree.c b/worktree.c
index eb61212..d8c9d85 100644
--- a/worktree.c
+++ b/worktree.c
@@ -380,3 +380,43 @@ const struct worktree *find_shared_symref(const char 
*symref,
 
return existing;
 }
+
+void migrate_worktree_config(void)
+{
+   struct strbuf worktree_path = STRBUF_INIT;
+   struct strbuf main_path = STRBUF_INIT;
+   struct repository_format format;
+
+   assert(repository_format_worktree_config == 0);
+
+   strbuf_git_common_path(_path, "config.worktree");
+   strbuf_git_path(_path, "config");
+
+   read_repository_format(, main_path.buf);
+   assert(format.worktree_config == 0);
+
+   if (format.is_bare >= 0) {
+   git_config_set_in_file(worktree_path.buf,
+  "core.bare", "true");
+   git_config_set_in_file(main_path.buf,
+  "core.bare", NULL);
+   }
+   if (format.work_tree) {
+   git_config_set_in_file(worktree_path.buf,
+  "core.worktree",
+  format.work_tree);
+   git_config_set_in_file(main_path.buf,
+  "core.worktree", NULL);
+   }
+
+   git_config_set_in_file(main_path.buf,
+  "extensions.worktreeConfig", "true");
+   if (format.version == 0)
+   git_config_set_in_file(main_path.buf,
+  "core.repositoryFormatVersion", "1");
+
+   repository_format_worktree_config = 1;
+
+   strbuf_release(_path);
+   strbuf_release(_path);
+}
diff --git a/worktree.h b/worktree.h
index d59ce1f..cf82676 100644
--- a/worktree.h
+++ b/worktree.h
@@ -76,4 +76,10 @@ extern const char *worktree_git_path(const struct worktree 
*wt,
 const char *fmt, ...)
__attribute__((format (printf, 2, 3)));
 
+/*
+ * Called to add extensions.worktreeConfig to $GIT_DIR/config and move
+ * main worktree specific config variables to $GIT_DIR/config.worktree.
+ */
+extern void migrate_worktree_config(void);
+
 #endif
-- 
2.8.2.524.g6ff3d78

[PATCH v5 1/4] config: read per-worktree config files

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

A new repo extension is added, worktreeConfig. When it is present:

 - Repository config reading by default includes $GIT_DIR/config _and_
   $GIT_DIR/config.worktree. "config" file remains shared in multiple
   worktree setup.

 - The special treatment for core.bare and core.worktree, to stay
   effective only in main worktree, is gone. These config files are
   supposed to be in config.worktree.

This extension is most useful in multiple worktree setup because you
now have an option to store per-worktree config (which is either
.git/config.worktree for main worktree, or
.git/worktrees/xx/config.worktree for linked ones).

This extension can be used in single worktree mode, even though it's
pretty much useless (but this can happen after you remove all linked
worktrees and move back to single worktree).

"git config" reads from both "config" and "config.worktree" by default
(i.e. without either --user, --file...) when this extension is
present. Default writes (not implemented in this patch) still go to
"config", not "config.worktree". A new option --worktree is added for
that (*).

Since a new repo extension is introduced, existing git binaries should
refuse to access to the repo (both from main and linked worktrees). So
they will not misread the config file (i.e. skip the config.worktree
part). They may still accidentally write to the config file anyway if
they use with "git config --file ".

This design places a bet on the assumption that the majority of config
variables are shared so it is the default mode. A safer move would be
default writes go to per-worktree file, so that accidental changes are
isolated.

(*) "git config --worktree" points back to "config" file when this
extension is not present so that it works in any setup.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/config.txt   | 11 +--
 Documentation/git-config.txt   |  4 
 Documentation/git-worktree.txt | 31 +++
 Documentation/gitrepository-layout.txt |  8 
 cache.h|  2 ++
 config.c   |  7 +++
 environment.c  |  1 +
 setup.c|  5 -
 8 files changed, 66 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 30cb946..c508386 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2,8 +2,9 @@ CONFIGURATION FILE
 --
 
 The Git configuration file contains a number of variables that affect
-the Git commands' behavior. The `.git/config` file in each repository
-is used to store the configuration for that repository, and
+the Git commands' behavior. The files `.git/config` and optionally
+`config.worktree` (see `extensions.worktreeConfig` below) are each
+repository is used to store the configuration for that repository, and
 `$HOME/.gitconfig` is used to store a per-user configuration as
 fallback values for the `.git/config` file. The file `/etc/gitconfig`
 can be used to store a system-wide default configuration.
@@ -264,6 +265,12 @@ advice.*::
show directions on how to proceed from the current state.
 --
 
+extensions.worktreeConfig::
+   If set, by default "git config" reads from both "config" and
+   "config.worktree" file in that order. In multiple working
+   directory mode, "config" file is shared while
+   "config.worktree" is per-working directory.
+
 core.fileMode::
Tells Git if the executable bit of files in the working tree
is to be honored.
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 83f86b9..806873c 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -253,6 +253,10 @@ $XDG_CONFIG_HOME/git/config::
 $GIT_DIR/config::
Repository specific configuration file.
 
+$GIT_DIR/config.worktree::
+   This is optional and is only searched when
+   `extensions.worktreeConfig` is present in $GIT_DIR/config.
+
 If no further options are given, all reading options will read all of these
 files that are available. If the global or the system-wide configuration
 file are not available they will be ignored. If the repository configuration
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index e257c19..329a673 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -136,6 +136,37 @@ working trees, it can be used to identify worktrees. For 
example if
 you only have two working trees, at "/abc/def/ghi" and "/abc/def/ggg",
 then "ghi" or "def/ghi" is enough to point to the former working tree.
 
+CONFIGURATION FILE
+--
+By default, the repository "config" file is shared across all working
+directories. If the config variables `core.bare` or `core.worktree`
+are already present in the config file, they will be applied to the
+main working directory only.

Re: git branch -D doesn't work with deleted worktree

[Duy Nguyen]

On Tue, Jan 10, 2017 at 12:08 PM, Jacob Keller  wrote:
> On Mon, Jan 9, 2017 at 2:07 AM, Duy Nguyen  wrote:
>> On Mon, Jan 9, 2017 at 10:44 AM, Jacob Keller  wrote:
>>> Why not just update the documentation to be "when you are done with a
>>> work tree you can delete it and then run git worktree prune"?
>>
>> The document does say that (a bit verbose though):
>>
>> When you are done with a linked working tree you can simply delete it.
>> The working tree's administrative files in the repository (see
>> "DETAILS" below) will eventually be removed automatically (see
>> `gc.worktreePruneExpire` in linkgit:git-config[1]), or you can run
>> `git worktree prune` in the main or any linked working tree to
>> clean up any stale administrative files.
>> --
>> Duy
>
> Right, so maybe we can make it more clear since it's not quite as
> simple as "deleting the work tree" because of stuff like stale
> branches..

Patches are welcome. I wrote that paragraph and you could clearly see
its "quality" (from user perspective).

> or would it be worth re-scanning worktrees when we do
> branch deletion? (obviously ignoring the ones that are marked as on
> removable media)

Probably. We run "git worktree prune" as part of "git gc" and that
command is automatically called in a couple places. Adding another
"git gc" here probably does not hurt. However it could trigger a full
repack and make "git branch" hang for a few seconds...

And no I don't want to single out "git worktree prune" to be called in
git-branch. We should either go through git-gc or none. We could check
if a worktree is deleted and suggest the user to run "git gc" though,
but it's getting too complicated when "git worktree remove" will soon
be the recommended way of deleting a worktree. So.. I don't know..
-- 
Duy

Re: Test failures when Git is built with libpcre and grep is built without it

[A. Wilcox]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

(Hi, musl developers!  Read on for some information regarding what
began as a test failure during packaging of Git 2.7.3 that appears to
be related to musl's regexp library.)


On 09/01/17 05:27, Jeff King wrote:
> Are you trying with a version of git older than v2.7.x?


We are using 2.7.3.  I will admit the version is a bit old, but it is
working on all other arches (and it took an inordinate amount of time
to spin up PowerPC - hooray for toolchain issues).


On 09/01/17 15:33, Jeff King wrote:
> On Mon, Jan 09, 2017 at 02:05:44PM +0100, Andreas Schwab wrote:
>> You need to quote the regexp argument, see the line starting
>> with "test_must_fail" above.
> 
> Oh, duh. I checked that the line in the test was quoted


Guilty of this as well.  Sorry about that.  With the proper
invocation, I receive a success:


elaine trash directory.t7810-grep # git grep -G -F -P -E
"a\x{2b}b\x{2a}c" ab
error: cannot run less: No such file or directory
ab:a+b*c
elaine trash directory.t7810-grep # echo $?
0


Haven't managed to build less(1) for PowerPC yet, but it does seem to
work when quoted.  Yikes!


> The problem is that we are expecting the regex "\x{2b}" to complain
> in regcomp() (as an ERE), but it doesn't. And that probably _is_
> related to musl, which is providing the libc regex (I know this
> looks like a pcre test, but it's checking that "-P -E" overrides
> the pcre option with "-E").
> 
> I'm not sure if musl is wrong for failing to complain about a
> bogus regex. Generally making something that would break into
> something that works is an OK way to extend the standard. So our
> test is at fault for assuming that the regex will fail. I guess
> we'd need to find some more exotic syntax that pcre supports, but
> that ERE doesn't. Maybe "(?:)" or something.
> 
> -Peff
> 


I am cc:ing in the musl development list to see what their thoughts are.

Thanks for the help so far; I really appreciate it.  Hopefully we can
resolve this in a way that makes musl and Git's test suite both more
correct, if necessary.

Happy new year!

- --arw


- -- 
A. Wilcox (awilfox)
Project Lead, Adélie Linux
http://adelielinux.org
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQIcBAEBCAAGBQJYdLk+AAoJEMspy1GSK50Uz9kP/2JUSzSSXkKLy3788aAjgBOq
aTPBqKXLCvnTeBpsymXoGueaJzgDnYP6Xi9tUb/j4JAXqaJKGHXTgz8ixsFQJ4SJ
p7NN1JZXsIVGKWQHcxvEXEIRmBK7T9BQ2Hq6qUuk3n4PM6lbD1Ur3G1rUqIM/z76
5cSkvwgvGuOVYXLwSTTprb6EbbZc6WTgcDECFIl4z7eJ3GihChg+EgH4cTDt3xLD
9VHx++hWoEZXued0g0myENjBMeCiFOpYw1bzxn7d2s8masx/If2TkK0CLdexy2ti
hFV/F7g8etgZSdmrmiaJIudTsY6p46/sm/VmGm+8yPIeR8/8EOBlyhKhz4EidNtA
2dpUUJW/RD4uz9yVNjmWhIHaL10weuNLQLd/Tt5O/0dG422vhJxOwwJL9Vhxw70y
1PprD11+ZUJeWsz91C/Bl9CqHqljDP00QX8w/dbHCDsjeKgfUdksy2iKlHBG0sx3
CbL0EcHeE9BLmqmJi9ED7w78cTmNryemK29WFHUGSwzR99Rv9QlMn+4GCF1s0O3N
UKPCueLpPcNvRK+1WHkAwUC0iV97Un3WI6kyExs9khygIHRd1y/yRhbpxibbz337
FJfOTcT7e/YqML8Nb8EdwECRhdGX5u1tKqaOfBwLC62SKGTPi5qXpjaSPPJcJsrZ
phxFt6a45PPECV3ZsEO1
=PMkh
-END PGP SIGNATURE-

Microsoft Outlook säkerhetsgruppen

[Devaraj Veerasamy, Dr]

Din e-rutan konto behöver vara verifiera nu för oegentligheter finns i din 
e-box-konto eller kommer att blockera. KLICKA 
här för att verifiera din 
e-postbrevlåda och fylla i ditt fullständiga användarnamn och lösenord 
omedelbart

Microsoft Outlook säkerhetsgruppen
Tack.

Re: [ANNOUNCE] git-test: run automated tests against a range of Git commits

[Jeff King]

On Sun, Jan 08, 2017 at 10:52:25AM +0100, Michael Haggerty wrote:

> > I see the symmetry and simplicity in allowing the user to specify a full
> > range. But it also seems like it's easy to make a mistake that's going
> > to want to test a lot of commits. I wonder if it should complain when
> > there's no lower bound to the commit range. Or alternatively, if there's
> > a single positive reference, treat it as a lower bound, with HEAD as the
> > upper bound (which is vaguely rebase-like).
> 
> I see how this might be unexpected, and it's definitely inconvenient at
> some times (like when you want to test a single commit). I thought it
> would be nice to allow arbitrary `rev-list` expressions (albeit
> currently only a single word), but I think that you are right that other
> semantics would be more convenient.
> 
> I'm thinking of maybe
> 
> * If an argument matches `*..*`, pass it to `rev-list` (like now).
> 
> * Otherwise, treat each argument as a single commit/tree (i.e., pass it
> to `rev-parse`).
> 
> * If no argument is specified, test `@{u}..` (assuming that an
>   upstream is configured). Though actually, this won't be as
>   convenient as it sounds, because (a) `git test` is often run
>   in a separate worktree, and (2) on errors, it currently leaves the
>   repository with a detached `HEAD`.
> 
> * Support a `--stdin` option, to read a list of commits/trees to test
>   from standard input. By this mechanism, users could use arbitrary
>   `rev-list` commands to choose what to test.

That seems quite reasonable to me. The rebase semantics of "a single
argument is my upstream" is convenient, but I think it also confuses
people. Yours seem very simple to explain.

It doesn't allow complex stuff like "git test ^foo ^bar HEAD", but I
don't think "git rev-list ^foo ^bar HEAD | git test --stdin" is really
so bad for complicated cases like that (if anybody would even ever want
such a thing).

> Aside: It would be nice if `git notes` had a subcommand to initialize a
> note reference with an empty tree. (I know how to do it longhand, but
> it's awkward and it should be possible to do it via the `notes` interface.)
> 
> I think ideally `git notes add` would look for pre-existing notes, and:
> 
> * If none are found, create an empty notes reference.
> 
> * If pre-existing notes are found and there was no existing test with
>   that name, probably just leave the old notes in place.
> 
> * If pre-existing notes are found and there was already a test with
>   that name but a different command, perhaps insist that the user
>   decide explicitly whether to forget the old results or continue using
>   them. This might help users avoid the mistake of re-using old results
>   even if they change the manner of testing.

I'm not quite sure what you mean here. By "test" and "command", do you
mean the test name that is used in the notes ref, and the command that
it is defined as?

In the notes-cache.c subsystem, the commit message stores a validity
token which must match in order to use the cache. You could do something
similar here (store the executed command in the commit message, and
invalidate the cache the user has changed the command). The notes-cache
stuff isn't available outside of the C code, though. You could either
expose it, or just do something similar longhand.

Thinking about it, though, I did notice that the tree sha1 is not the
only input to the cache. Things like config.mak (not to mention the
system itself) contribute to the test results. So no system will ever be
perfect, and it seems like just making an easy way to force a retest (or
just invalidate the whole cache) would be sufficient.

But maybe I totally missed what you were trying to say here.

> > It would be even easier if I could just repeat my range and only re-test
> > the "bad" commits. It was then that I decided to actually read the rest
> > of "git test help range" and see that you already wrote such an option,
> > cleverly hidden under the name "--retest".
> 
> I think you were being ironic, but if not, would this have been easier
> to find under another name?

Sorry, the knob on my sarcasm module must have accidentally been knocked
down from "so thick it's obvious even by email".

Yes, I did just mean that I was being blind. You not only had added the
option already, but gave it the exact name I was about to propose it
under. :)

> Yeah, this is awkward, not only because many people don't know what to
> make of detached HEAD, but also because it makes it awkward in general
> to use `git test` in your main working directory. I didn't model this
> behavior on `git rebase --interactive`'s `edit` command, because I
> rarely use that. But I can see how they would fit together pretty well
> for people who like that workflow.

Yeah, after sleeping on it, I think it's best if "git test" remains
separate from that. It's primary function is to run the test, possibly
serving up a cached answer. So it would be perfectly reasonable to do:

  git 

Re: Preserve/Prune Old Pack Files

[Jeff King]

On Mon, Jan 09, 2017 at 09:17:56AM -0700, Martin Fick wrote:

> > I suspect the name-change will break a few tools that you
> > might want to use to look at a preserved pack (like
> > verify-pack).  I know that's not your primary use case,
> > but it seems plausible that somebody may one day want to
> > use a preserved pack to try to recover from corruption. I
> > think "git index-pack --stdin
> >  > be a last-resort for re-admitting the objects to the
> > repository.
> 
> or even a simple manual rename/move back to its orginal 
> place?

Yes, that would work. There's not a tool to do it, but it's a fairly
straightforward transformation.

> [loose objects]
> Where would you suggest we store those?  Maybe under 
> ".git/objects/preserved//"?  Do they need to be 
> renamed also somehow to avoid a find?

It would make sense to me to have a single "preserved" root, with
"/.old" and "packs/pack-.old-pack" together under it.

You could also move the objects out of objects/ entirely. Say, to
".git/preserved-objects" or something. Then you could probably do away
with the filename munging altogether, and "restoring" an object or pack
would be a simple "mv" or "cp" (or you could even add preserved-objects
to $GIT_ALTERNATE_OBJECT_DIRECTORIES if you wanted to do a single
operation looking at both sets).

That's all outside the scope of your original purpose (which I think was
just to keep the files _somewhere_ so that the open descriptor stays
valid on NFS). But maybe it would make other related things more
convenient. I dunno. I'm just speaking off the top of my head.

> > That's _way_ more complicated than your problem, and as I
> > said, I do not have a finished solution. But it seems
> > like they touch on a similar concept (a post-delete
> > holding area for objects). So I thought I'd mention it in
> > case if spurs any brilliance.
> 
> I agree, this is a problem I have wanted to solve also.  I 
> think having a "preserved" directory does open the door to 
> such "recovery" solutions, although I think you would 
> actually want to modify the many read code paths to fall 
> back to looking at the preserved area and performing 
> immediate "recovery" of the pack file if it ends up being 
> needed.

In my (admittedly not very concrete) plan, the read code paths
_wouldn't_ know to look in the preserved area. It would be up to the
repacking process to rollback in case of a race. That does open a period
(between the faux delete and the rollback) where readers may be broken.
But that's much better than the state today, which is that the readers
are broken, and that breakage persists forever.

But there may be other better ways of doing it.  What we're really
talking about is a transactional system where neither side locks (or at
least not for an appreciable amount of time), and one side is capable of
falling back and modifying its operation when there's a relevant race.
There's probably some research in this area and some standard solutions,
but it's not an area I'm overly familiar with (and building any solution
on top of POSIX filesystem semantics adds an extra challenge).

> That's a lot of work, but having the packs (and 
> eventually the loose objects) preserved into a location 
> where no new references will be built to depend on them is 
> likely the first step.  Does the name "preserved" do well for 
> that use case also, or would there be some better name, what 
> would a transactional system call them?

I wasn't going to bikeshed, but since you ask...:)

"preserved" to me sounds like something we'd be keeping forever. These
objects are more in a "pending delete" state, or a purgatory. Maybe
something along those lines would be more appropriate.

-Peff

Re: [RFC PATCH 0/5] Localise error headers

[Jeff King]

On Mon, Jan 09, 2017 at 01:43:15PM +0100, Michael J Gruber wrote:

> > I can't say I'm excited about having matching "_" variants for each
> > function. Are we sure that they are necessary? I.e., would it be
> > acceptable to just translate them always?
> 
> We would still need to mark the strings, e.g.
> 
> die(N_("oopsie"));
> 
> and would not be able to opt out of translating in the code (only in the
> po file, by not providing a translation).

I meant more along the lines of: would it be OK to just always translate
the prefix, even if the message itself is not translated? I.e.,

diff --git a/usage.c b/usage.c
index 82ff13163..8e5400f57 100644
--- a/usage.c
+++ b/usage.c
@@ -32,7 +32,7 @@ static NORETURN void usage_builtin(const char *err, va_list 
params)
 
 static NORETURN void die_builtin(const char *err, va_list params)
 {
-   vreportf("fatal: ", err, params);
+   vreportf(_("fatal: "), err, params);
exit(128);
 }

> In any case, the question is whether we want to tell the user
> 
> A: B
> 
> where A is in English and B is localised, or rather localise both A and
> B (for A in "error", "fatal", "warning"...).
> 
> For localising A and B, we'd need this series or something similar. For
> keeping the mix, we don't need to do anything ;)

What I wrote above would keep the mix, but switch it in the other
direction.

And then presumably that mix would gradually move to 100% consistency as
more messages are translated. But the implicit question is: are there
die() messages that should never be translated? I'm not sure.

-Peff

[PATCH v10 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 5de22cf64..b18eabd69 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 7bfd65633..9140539df 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 v10 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 b18eabd69..b0d94deea 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 73df72811..b4789cec4 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 9140539df..e0015c60f 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 

[PATCH v10 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 b0d94deea..04ffc3552 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 e0015c60f..76553ebc4 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 5eb013ca2..d3d1a97db 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.11.0

[PATCH v10 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 94c6b88fa..14240b407 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 998991873..6de0927d1 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 = 

[PATCH v10 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 | 247 ---
 t/t3203-branch-output.sh |   2 +-
 t/t6040-tracking-info.sh |   2 +-
 3 files changed, 87 insertions(+), 164 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 34cd61cd9..f293ee5b0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -37,11 +37,11 @@ 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 */
+   GIT_COLOR_NORMAL,   /* PLAIN */
+   GIT_COLOR_RED,  /* REMOTE */
+   GIT_COLOR_NORMAL,   /* LOCAL */
+   GIT_COLOR_GREEN,/* CURRENT */
+   GIT_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 (show_upstream_ref)
- 

[PATCH v10 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 abe13f3be..6b671ae92 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 only its result
+from the top-level 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) Authored by: 
%(authorname)%(end)"
+
+
 SEE ALSO
 
 linkgit:git-show-ref[1]
diff --git a/ref-filter.c b/ref-filter.c
index 1a978405e..0a578722d 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, );

[PATCH v10 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 6423ebce5..34cd61cd9 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 b4789cec4..8a1a476db 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.11.0

[PATCH v10 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 6b671ae92..d5be41973 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 f31c4b68b..e002629ff 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, 

[PATCH v10 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 | 38 +++
 ref-filter.c   | 41 --
 t/t6300-for-each-ref.sh| 19 ++
 3 files changed, 79 insertions(+), 19 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 331e1fef8..08be8462c 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -97,11 +97,13 @@ refname::
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`.
+   (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. (e.g., `%(refname:lstrip=-2)` turns
-   `refs/tags/foo` into `tags/foo`). When the ref does not have
-   enough components, the result becomes an empty string if
+   `refs/tags/foo` into `tags/foo` and `%(refname:rstrip=-1)`
+   turns `refs/tags/foo` into `refs`). 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.
 
@@ -120,22 +122,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.
 
@@ -177,8 +180,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 1cd92ea4e..dd7e751f2 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))
+  

[PATCH v10 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 14240b407..5de22cf64 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 c1ebc406c..7bfd65633 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 v10 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 5516a47b5..1fae4 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 f293ee5b0..cbaa6d03c 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 45213280a..5778c0afe 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 

[PATCH v10 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 6de0927d1..e98ef4bb6 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 af76dc530..7663a3661 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 v10 17/20] ref-filter: allow porcelain to translate messages in the output

[Karthik Nayak]

From: Karthik Nayak 

Introduce setup_ref_filter_porcelain_msg() so that the messages used in
the atom %(upstream:track) can be translated if needed. By default, keep
the messages untranslated, which is the right behavior for plumbing
commands. This is needed as we port branch.c to use ref-filter's
printing API's.

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

diff --git a/ref-filter.c b/ref-filter.c
index dd7e751f2..e478ec6c8 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -16,6 +16,27 @@
 #include "trailer.h"
 #include "wt-status.h"
 
+static struct ref_msg {
+   const char *gone;
+   const char *ahead;
+   const char *behind;
+   const char *ahead_behind;
+} msgs = {
+/* Untranslated plumbing messages: */
+   "gone",
+   "ahead %d",
+   "behind %d",
+   "ahead %d, behind %d"
+};
+
+void setup_ref_filter_porcelain_msg(void)
+{
+   msgs.gone = _("gone");
+   msgs.ahead = _("ahead %d");
+   msgs.behind = _("behind %d");
+   msgs.ahead_behind = _("ahead %d, behind %d");
+}
+
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 typedef enum { COMPARE_EQUAL, COMPARE_UNEQUAL, COMPARE_NONE } cmp_status;
 
@@ -1181,15 +1202,15 @@ static void fill_remote_ref_details(struct used_atom 
*atom, const char *refname,
else if (atom->u.remote_ref.option == RR_TRACK) {
if (stat_tracking_info(branch, _ours,
   _theirs, NULL)) {
-   *s = xstrdup("gone");
+   *s = xstrdup(msgs.gone);
} else if (!num_ours && !num_theirs)
*s = "";
else if (!num_ours)
-   *s = xstrfmt("behind %d", num_theirs);
+   *s = xstrfmt(msgs.behind, num_theirs);
else if (!num_theirs)
-   *s = xstrfmt("ahead %d", num_ours);
+   *s = xstrfmt(msgs.ahead, num_ours);
else
-   *s = xstrfmt("ahead %d, behind %d",
+   *s = xstrfmt(msgs.ahead_behind,
 num_ours, num_theirs);
if (!atom->u.remote_ref.nobracket && *s[0]) {
const char *to_free = *s;
diff --git a/ref-filter.h b/ref-filter.h
index 630e7c2b9..44b36eded 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -113,5 +113,7 @@ struct ref_sorting *ref_default_sorting(void);
 int parse_opt_merge_filter(const struct option *opt, const char *arg, int 
unset);
 /*  Get the current HEAD's description */
 char *get_head_description(void);
+/*  Set up translated strings in the output. */
+void setup_ref_filter_porcelain_msg(void);
 
 #endif /*  REF_FILTER_H  */
-- 
2.11.0

[PATCH v10 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 d7ab4c961..94c6b88fa 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 47b521cca..998991873 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 e67c694c3..a2e3f5525 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.11.0

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

[Karthik Nayak]

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
strip from the left as necessary and _leave_ behind only 'N'
slash-separated path components from the right-most end.

For e.g. %(refname:lstrip=-1) would make 'foo/goo/abc' into 'abc'.

Add documentation and tests for the same.

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

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 04ffc3552..331e1fef8 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -98,7 +98,12 @@ 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. (e.g., `%(refname:lstrip=-2)` turns
+   `refs/tags/foo` into `tags/foo`). 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.
 
 objecttype::
The type of the object (`blob`, `tree`, `commit`, `tag`).
diff --git a/ref-filter.c b/ref-filter.c
index 76553ebc4..1cd92ea4e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -34,7 +34,7 @@ struct if_then_else {
 
 struct refname_atom {
enum { R_NORMAL, R_SHORT, R_LSTRIP } option;
-   unsigned int lstrip;
+   int lstrip;
 };
 
 /*
@@ -93,8 +93,8 @@ static void refname_atom_parser_internal(struct refname_atom 
*atom,
atom->option = R_SHORT;
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);
+   if (strtol_i(arg, 10, >lstrip))
+   die(_("Integer value expected refname:lstrip=%s"), arg);
} else
die(_("unrecognized %%(%s) argument: %s"), name, arg);
 }
@@ -1091,12 +1091,28 @@ static inline char *copy_advance(char *dst, const char 
*src)
return dst;
 }
 
-static const char *lstrip_ref_components(const char *refname, unsigned int len)
+static const char *lstrip_ref_components(const char *refname, int len)
 {
long remaining = len;
const char *start = refname;
 
-   while (remaining) {
+   if (len < 0) {
+   int i;
+   const char *p = refname;
+
+   /* Find total no of '/' separated path-components */
+   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
+* because we count the number of '/', but the number
+* of components is one more than the no of '/').
+*/
+   remaining = i + len + 1;
+   }
+
+   while (remaining > 0) {
switch (*start++) {
case '\0':
return "";
@@ -1105,6 +1121,7 @@ static const char *lstrip_ref_components(const char 
*refname, unsigned int len)
break;
}
}
+
return start;
 }
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index d3d1a97db..203cfaa15 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -53,12 +53,16 @@ test_atom head refname refs/heads/master
 test_atom head refname:short master
 test_atom head refname:lstrip=1 heads/master
 test_atom head refname:lstrip=2 master
+test_atom head refname:lstrip=-1 master
+test_atom head refname:lstrip=-2 heads/master
 test_atom head upstream refs/remotes/origin/master
 test_atom head upstream:short origin/master
 test_atom head upstream:lstrip=2 origin/master
+test_atom head upstream:lstrip=-2 origin/master
 test_atom head push refs/remotes/myfork/master
 test_atom head push:short myfork/master
 test_atom head push:lstrip=1 remotes/myfork/master
+test_atom head push:lstrip=-1 master
 test_atom head objecttype commit
 test_atom head objectsize 171
 test_atom head objectname $(git rev-parse refs/heads/master)
@@ -141,12 +145,6 @@ test_expect_success 'Check invalid atoms names are errors' 
'
test_must_fail git for-each-ref --format="%(INVALID)" refs/heads
 '
 
-test_expect_success 'arguments to :lstrip must be positive integers' '
-   

[PATCH v10 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 lets 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 0a578722d..f31c4b68b 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.11.0

[PATCH v10 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 e98ef4bb6..c1ebc406c 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.11.0

[PATCH v10 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 9d30f55b0..6423ebce5 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 385fc04d0..5511a200c 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 v10 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 d5be41973..d7ab4c961 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 e002629ff..385fc04d0 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 aea1dfc71..e67c694c3 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 

[PATCH v10 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
v9 can be found here: http://marc.info/?l=git=148285579607683=2

Changes in this version:
1. Changes in commit messages. Made 01/20 a little more clearer.
2. Better documentation regarding usage of negative refs (15/20) and
small changes to the examples introduced to avoid confusion.
3. Revert the changes made to 'static char branch_colors[]'.
(http://marc.info/?t=148285594700011=1=2)

Thanks to Junio and Jacob for their suggestions wrt the previous version.
Interdiff at the bottom.

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 |  87 +--
 builtin/branch.c   | 288 +++---
 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, 781 insertions(+), 304 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 81db67d74..08be8462c 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -95,13 +95,17 @@ 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. 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.
+   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` and
+   `%(refname:rstrip=2)` turns `refs/tags/foo` into `refs`).
+   if `` is a negative number, then only `` path components
+   are left behind. (e.g., `%(refname:lstrip=-2)` turns
+   `refs/tags/foo` into `tags/foo` and `%(refname:rstrip=-1)`
+   turns `refs/tags/foo` into `refs`). 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.

 objecttype::
The type of the object (`blob`, `tree`, `commit`, `tag`).
@@ -222,8 +226,8 @@ 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.
+according to the 

[PATCH v10 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 5511a200c..47b521cca 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 f78323de0..630e7c2b9 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.11.0