Re: [RFH - Tcl/Tk] use of procedure before declaration?

[Konstantin Khomoutov]

On Tue, 17 Jan 2017 12:29:23 +0100 (CET)
Johannes Schindelin  wrote:

> > In
> > https://github.com/git/git/blob/master/git-gui/lib/choose_repository.tcl#L242
> > the procedure `_unset_recentrepo` is called, however the procedure
> > isn't declared until line 248. My reading of the various Tcl
> > tutorials suggest (but not explictly) that this isn't the right way.
> 
> Indeed, calling a procedure before it is declared sounds incorrect.
[...]
> And it is perfectly legitimate to use not-yet-declared procedures in
> other procedures, otherwise recursion would not work.
[...]

Sorry for chiming in too late, but I'd throw a bit of theory in.

Since Tcl is an interpreter (though it automatically compiles certain
stuff to bytecode as it goes through the script, and caches this
representation), everything is interpreted in the normal script order --
top to bottom as we usually see it in a text editor.

That is, there are simply no declaration vs definition: the main script
passed to tclsh / wish is read and interpreted from top to bottom;
as soon as it calls the [source] command, the specified script is read
and interpreted from top to bottom etc; after that the control is back
to the original script and its interpretation continues.

Hence when Tcl sees a command (everything it executes is a command; this
includes stuff like [proc], [foreach] and others, which are syntax in
other languages) it looks up this command in the current list of
commands it knows and this either succeeds or fails.  The built-in
command [proc] defines a new Tcl procedure with the given name, and
registers it in that list of known commands.

So the general rule for user-defined procedures is relatively
straightforward: to call a procedure, the interpreter should have read
and executed its definition before the attempted call.

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Johannes Sixt]

Am 17.01.2017 um 20:17 schrieb Junio C Hamano:

So... can we move this forward?


I have no objects anymore.

-- Hannes

difflame

[Edmundo Carmona Antoranz]

Hi!

For a very long time I had wanted to get the output of diff to include
blame information as well (to see when something was added/removed).

I just created a very small (and rough) tool for that purpose. It's
written in python and if it gets to something better than a small
tool, I think it could be worth to be added into git main (perhaps
into contrib?).

If you want to give ir a try:
https://github.com/eantoranz/difflame

Just provide the two treeishs you would like to diff (no more
parameters are accepted at the time) and you will get the diff along
with blame. Running it right now on the project itself:

✔ ~/difflame [master L|⚑ 1]
23:21 $ ./difflame.py HEAD~3 HEAD~
diff --git a/README.txt b/README.txt
new file mode 100644
index 000..a82aa27
--- /dev/null
+++ b/README.txt
@@ -0,0 +1,11 @@
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600  1) difflame
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600  2)
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600  3)
Copyright 2017 Edmundo Carmona Antoranz
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600  4)
Released under the terms of GPLv2
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600  5)
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600  6) Show
the output of diff with the additional information of blame.
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600  7)
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600  8)
Lines that remain the same or that were added will indicate when those
lines were 'added' to the file
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600  9)
Lines that were removed will display the last revision where those
lines were _present_ on the file (as provided by blame --re
verse)
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 10)
+3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 11) At
the moment, only two parameters need to be provided: two treeishs to
get the diff from
diff --git a/difflame.py b/difflame.py
index f6e879b..06bfc03 100755
--- a/difflame.py
+++ b/difflame.py
@@ -112,16 +112,20 @@ def process_file_from_diff_output(output_lines,
starting_line):
c661286f (Edmundo Carmona Antoranz 2017-01-17 20:10:07 -0600 112)
diff_line = output_lines[i].split()
c661286f (Edmundo Carmona Antoranz 2017-01-17 20:10:07 -0600 113)
if diff_line[0] != "diff":
c661286f (Edmundo Carmona Antoranz 2017-01-17 20:10:07 -0600 114)
   raise Exception("Doesn't seem to exist a 'diff' line at line " +
str(i + 1) + ": " + output_lines[i])
-3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 115)
original_name = diff_line[1]
-3d426842 (Edmundo Carmona Antoranz 2017-01-17 22:26:18 -0600 116)
final_name = diff_line[2]
+f135bf04 (Edmundo Carmona Antoranz 2017-01-17 22:50:50 -0600 115)
original_name = diff_line[2]
+f135bf04 (Edmundo Carmona Antoranz 2017-01-17 22:50:50 -0600 116)
final_name = diff_line[3]
c661286f (Edmundo Carmona Antoranz 2017-01-17 20:10:07 -0600 117)
print output_lines[i]; i+=1
.
.
.


Hope you find it useful

Best regards!

[PATCH] gitk: remove translated message from comments

[David Aguilar]

"make update-po" fails because a previously untranslated string
has now been translated:

Updating po/sv.po
po/sv.po:1388: duplicate message definition...
po/sv.po:380: ...this is the location of the first definition

Remove the duplicate message definition.

Reported-by: Junio C Hamano 
Signed-off-by: David Aguilar 
---
 po/sv.po | 15 ---
 1 file changed, 15 deletions(-)

diff --git a/po/sv.po b/po/sv.po
index 32fc752..2a06fe5 100644
--- a/po/sv.po
+++ b/po/sv.po
@@ -1385,21 +1385,6 @@ msgstr "Felaktiga argument till gitk:"
 #~ msgid "mc"
 #~ msgstr "mc"
 
-#~ msgid ""
-#~ "\n"
-#~ "Gitk - a commit viewer for git\n"
-#~ "\n"
-#~ "Copyright © 2005-2016 Paul Mackerras\n"
-#~ "\n"
-#~ "Use and redistribute under the terms of the GNU General Public License"
-#~ msgstr ""
-#~ "\n"
-#~ "Gitk - en incheckningsvisare för git\n"
-#~ "\n"
-#~ "Copyright © 2005-2016 Paul Mackerras\n"
-#~ "\n"
-#~ "Använd och vidareförmedla enligt villkoren i GNU General Public License"
-
 #~ msgid "next"
 #~ msgstr "nästa"
 
-- 
2.11.0.536.gaf746e49c2

"git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

[Matt McCutchen]

A bug report: I noticed that "git diff --ignore-space-change --stat"
lists files with only whitespace differences as having changed with 0
differing lines.  This is inconsistent with the behavior without --
stat, which doesn't list such files at all.  (Same behavior with all
the --ignore*space* flags.)  I can reproduce this with the current
"next", af746e4.  Quick test case:

echo ' ' >test1 && echo '  ' >test2 &&
git diff --stat --no-index --ignore-space-change test1 test2

This caused me some inconvenience in the following scenario: I was
reading a commit diff that had a bulk license change in all files
combined with code changes.  I attempted to revert the bulk license
change locally using "sed" to more easily read the code diff, but my
reversion left some whitespace diffs where the original files had
inconsistent whitespace.  So the diffstat after my reversion was
cluttered with these "0" entries.

Regards,
Matt

[PATCHv3 4/4] unpack-trees: support super-prefix option

[Stefan Beller]

In the future we want to support working tree operations within submodules,
e.g. "git checkout --recurse-submodules", which will update the submodule
to the commit as recorded in its superproject. In the submodule the
unpack-tree operation is carried out as usual, but the reporting to the
user needs to prefix any path with the superproject. The mechanism for
this is the super-prefix. (see 74866d757, git: make super-prefix option)

Add support for the super-prefix option for commands that unpack trees
by wrapping any path output in unpacking trees in the newly introduced
super_prefixed function. This new function prefixes any path with the
super-prefix if there is one.  Assuming the submodule case doesn't happen
in the majority of the cases, we'd want to have a fast behavior for no
super prefix, i.e. no reallocation/copying, but just returning path.

Another aspect of introducing the `super_prefixed` function is to consider
who owns the memory and if this is the right place where the path gets
modified. As the super prefix ought to change the output behavior only and
not the actual unpack tree part, it is fine to be that late in the line.
As we get passed in 'const char *path', we cannot change the path itself,
which means in case of a super prefix we have to copy over the path.
We need two static buffers in that function as the error messages
contain at most two paths.

For testing purposes enable it in read-tree, which has no output
of paths other than an unpack-trees.c. These are all converted in
this patch.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---

This replaces the last two commits (squash and unpack-trees:
support super-prefix option) of sb/unpack-trees-super-prefix.

> * sb/unpack-trees-super-prefix (2017-01-12) 5 commits
>  - SQUASH
>  - unpack-trees: support super-prefix option
>  - t1001: modernize style
>  - t1000: modernize style
>  - read-tree: use OPT_BOOL instead of OPT_SET_INT
>
>  "git read-tree" and its underlying unpack_trees() machinery learned
>  to report problematic paths prefixed with the --super-prefix option.
>
>  Expecting a reroll.
>  The first three are in good shape.  The last one needs a better
>  explanation and possibly an update to its test.
>  cf. 

[PATCH 4/4] documentation: retire unfinished documentation

[Stefan Beller]

When looking for documentation for a specific function, you may be tempted
to run

  git -C Documentation grep index_name_pos

only to find the file technical/api-in-core-index.txt, which doesn't
help for understanding the given function. It would be better to not find
these functions in the documentation, such that people directly dive into
the code instead.

In the previous patches we have documented
* index_name_pos()
* remove_index_entry_at()
* add_[file_]to_index()
in cache.h

We already have documentation for:
* add_index_entry()
* read_index()

Which leaves us with a TODO for:
* cache -> the_index macros
* refresh_index()
* discard_index()
* ie_match_stat() and ie_modified(); how they are different and when to
  use which.
* write_index() that was renamed to write_locked_index
* cache_tree_invalidate_path()
* cache_tree_update()

Signed-off-by: Stefan Beller 
---
 Documentation/technical/api-in-core-index.txt | 21 -
 1 file changed, 21 deletions(-)
 delete mode 100644 Documentation/technical/api-in-core-index.txt

diff --git a/Documentation/technical/api-in-core-index.txt 
b/Documentation/technical/api-in-core-index.txt
deleted file mode 100644
index adbdbf5d75..00
--- a/Documentation/technical/api-in-core-index.txt
+++ /dev/null
@@ -1,21 +0,0 @@
-in-core index API
-=
-
-Talk about  and , things like:
-
-* cache -> the_index macros
-* read_index()
-* write_index()
-* ie_match_stat() and ie_modified(); how they are different and when to
-  use which.
-* index_name_pos()
-* remove_index_entry_at()
-* remove_file_from_index()
-* add_file_to_index()
-* add_index_entry()
-* refresh_index()
-* discard_index()
-* cache_tree_invalidate_path()
-* cache_tree_update()
-
-(JC, Linus)
-- 
2.11.0.299.g762782ba8a

Re: [PATCH] transport submodules: correct error message

[Stefan Beller]

On Tue, Jan 17, 2017 at 4:15 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> On Tue, Jan 17, 2017 at 3:42 PM, Junio C Hamano  wrote:
>>> Stefan Beller  writes:
>>>
 Trying to push with --recurse-submodules=on-demand would run into
 the same problem. To fix this issue
 1) specifically mention that we looked for branches on the remote.
>>>
>>> That makes an incorrect statement ("not found on any remote"---we
>>> did not inspect all of the said remote, only heads and tags) into an
>>> irrelevant statement ("not found on any remote branch"---the end
>>> user would say "so what?  I know it exists there, it's just that not
>>> all remote refs have corresponding tracking ref locally on our side").
>>
>> eh. So to be correct we need to tell the user we did not find any match on
>> a "remote-tracking branch" as the gitglossary puts it.
>
> I think the updated text is already "correct".  I am pointing out
> that it may be correct but not very helpful to the users.
>
>>> where remote tracking information is
>>> incomplete if you only look at heads and refs, in the sense that we
>>> no longer suggest ineffective workaround.
>>
>> s/ineffective/an effective/ ?
>
> Even though I make many typoes, I meant ineffective in this case.
> "The old message suggested workaround that would not help.  You no
> longer give that workaround that does not work."

Oh, I misunderstood the original as I lumped on-demand and check
together mentally, because in the Gerrit world they behave similar.
(Both error out; in the on-demand case the server produces the failure
message, though)

>
>>> If that is the case, perhaps configuring push.recurseSubmodules to
>>> turn this off (especially because you plan to turn the defaul to
>>> "check") and not giving the command line option would give a more
>>> pleasant end-user experience, I suspect.
>>
>> I though about going another way and adding another new value
>> to the enum, such that
>>
>> git push --recurse-submodules=sameRefSpecButNoCheck \
>> origin HEAD:refs/for/master
>>
>> works for Gerrit users.
>
> It is unclear what that enum tells Git to do.  Care to explain?  How
> is it different from "no"?

In those submodules, that are checked positively, blindly run
git -C ${sub} push ${ref_spec} and do not double check again,
which we currently do in the on-demand case.

Re: [PATCH] transport submodules: correct error message

[Junio C Hamano]

Stefan Beller  writes:

> On Tue, Jan 17, 2017 at 3:42 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> Trying to push with --recurse-submodules=on-demand would run into
>>> the same problem. To fix this issue
>>> 1) specifically mention that we looked for branches on the remote.
>>
>> That makes an incorrect statement ("not found on any remote"---we
>> did not inspect all of the said remote, only heads and tags) into an
>> irrelevant statement ("not found on any remote branch"---the end
>> user would say "so what?  I know it exists there, it's just that not
>> all remote refs have corresponding tracking ref locally on our side").
>
> eh. So to be correct we need to tell the user we did not find any match on
> a "remote-tracking branch" as the gitglossary puts it.

I think the updated text is already "correct".  I am pointing out
that it may be correct but not very helpful to the users.

>> where remote tracking information is
>> incomplete if you only look at heads and refs, in the sense that we
>> no longer suggest ineffective workaround.
>
> s/ineffective/an effective/ ?

Even though I make many typoes, I meant ineffective in this case.
"The old message suggested workaround that would not help.  You no
longer give that workaround that does not work."

>> If that is the case, perhaps configuring push.recurseSubmodules to
>> turn this off (especially because you plan to turn the defaul to
>> "check") and not giving the command line option would give a more
>> pleasant end-user experience, I suspect.
>
> I though about going another way and adding another new value
> to the enum, such that
>
> git push --recurse-submodules=sameRefSpecButNoCheck \
> origin HEAD:refs/for/master
>
> works for Gerrit users.

It is unclear what that enum tells Git to do.  Care to explain?  How
is it different from "no"?

Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v

[Junio C Hamano]

santi...@nyu.edu writes:

> @@ -428,9 +443,12 @@ int cmd_tag(int argc, const char **argv, const char 
> *prefix)
>   if (filter.merge_commit)
>   die(_("--merged and --no-merged option are only allowed with 
> -l"));
>   if (cmdmode == 'd')
> - return for_each_tag_name(argv, delete_tag);
> - if (cmdmode == 'v')
> - return for_each_tag_name(argv, verify_tag);
> + return for_each_tag_name(argv, delete_tag, NULL);
> + if (cmdmode == 'v') {
> + if (format)
> + verify_ref_format(format);
> + return for_each_tag_name(argv, verify_tag, format);
> + }

This triggers:

builtin/tag.c: In function 'cmd_tag':
builtin/tag.c:451:3: error: passing argument 3 of
'for_each_tag_name' discards 'const' qualifier from pointer target type 
[-Werror]
   return for_each_tag_name(argv, verify_tag, format);

Either for-each-tag-name's new parameter needs to be typed
correctly, or the type of the "format" variable needs to be updated.

[PATCH v2 0/5] extend git-describe pattern matching

[Jacob Keller]

From: Jacob Keller 

Teach git describe and git name-rev the ability to match multiple
patterns inclusively. Additionally, teach these commands to also accept
negative patterns to exclude any refs which match.

The pattern lists for positive and negative patterns are inclusive. This
means that for the positive patterns, a reference will be considered as
long as it matches at least one of the match patterns. It need not match
all given patterns. Additionally for negative patterns, we will not
consider any ref which matches any negative pattern, even if it matches
one of the positive patterns.

Together this allows the ability to express far more sets of tags than a
single match pattern alone. It does not provide quite the same depth as
would teaching full regexp but it is simpler and easy enough to
understand.

- v2
* use --exclude instead of --discard
* use modern style in tests

I chose *not* to implement the suggestion of ordered values for exclude
and match, since this is not how the current implementation of
git-describe worked, and it didn't really make sense to me what was
being requested. I looked at the interface for git-log, and it appears
that the command accepts multiple invocations of --branches, --remotes,
and similar. I do not believe these need to be identical interfaces. I
welcome feedback on this decision, but I am not convinced yet that the
ordered arguments are worth the trouble.

diff --git c/Documentation/git-describe.txt w/Documentation/git-describe.txt
index a89bbde207b2..21a43b78924a 100644
--- c/Documentation/git-describe.txt
+++ w/Documentation/git-describe.txt
@@ -88,12 +88,12 @@ OPTIONS
patterns will be considered. Use `--no-match` to clear and reset the
list of patterns.
 
---discard ::
+--exclude ::
Do not consider tags matching the given `glob(7)` pattern, excluding
the "refs/tags/" prefix. This can be used to narrow the tag space and
find only tags matching some meaningful criteria. If given multiple
times, a list of patterns will be accumulated and tags matching any
-   of the patterns will be discarded. Use `--no-discard` to clear and
+   of the patterns will be excluded. Use `--no-exclude` to clear and
reset the list of patterns.
 
 --always::
diff --git c/Documentation/git-name-rev.txt w/Documentation/git-name-rev.txt
index 9b46e5ea9aae..301b4a8d55e6 100644
--- c/Documentation/git-name-rev.txt
+++ w/Documentation/git-name-rev.txt
@@ -30,11 +30,11 @@ OPTIONS
given multiple times, use refs whose names match any of the given shell
patterns. Use `--no-refs` to clear any previous ref patterns given.
 
---discard=::
+--exclude=::
Do not use any ref whose name matches a given shell pattern. The
pattern can be one of branch name, tag name or fully qualified ref
-   name. If given multiple times, discard refs that match any of the given
-   shell patterns. Use `--no-discards` to clear the list of discard
+   name. If given multiple times, exclude refs that match any of the given
+   shell patterns. Use `--no-exclude` to clear the list of exclude
patterns.
 
 --all::
diff --git c/Documentation/technical/api-parse-options.txt 
w/Documentation/technical/api-parse-options.txt
index 15e876e4c804..6914f54f5f44 100644
--- c/Documentation/technical/api-parse-options.txt
+++ w/Documentation/technical/api-parse-options.txt
@@ -169,9 +169,9 @@ There are some macros to easily define options:
The string argument is put into `str_var`.
 
 `OPT_STRING_LIST(short, long, , arg_str, description)`::
-   Introduce an option with a string argument. Repeated invocations
-   accumulate into a list of strings. Reset and clear the list with
-   `--no-option`.
+   Introduce an option with string argument.
+   The string argument is stored as an element in `` which must be a
+   struct string_list. Reset the list using `--no-option`.
 
 `OPT_INTEGER(short, long, _var, description)`::
Introduce an option with integer argument.
diff --git c/builtin/describe.c w/builtin/describe.c
index c09288ee6321..6769446e1f57 100644
--- c/builtin/describe.c
+++ w/builtin/describe.c
@@ -29,7 +29,7 @@ static int max_candidates = 10;
 static struct hashmap names;
 static int have_util;
 static struct string_list patterns = STRING_LIST_INIT_NODUP;
-static struct string_list discard_patterns = STRING_LIST_INIT_NODUP;
+static struct string_list exclude_patterns = STRING_LIST_INIT_NODUP;
 static int always;
 static const char *dirty;
 
@@ -131,16 +131,16 @@ static int get_name(const char *path, const struct 
object_id *oid, int flag, voi
return 0;
 
/*
-* If we're given discard patterns, first discard any tag which match
-* any of the discard pattern.
+* If we're given exclude patterns, first exclude any tag which match
+* any of the exclude pattern.
 */
-   if 

[PATCH v3 3/5] name-rev: add support to exclude refs by pattern match

[Jacob Keller]

From: Jacob Keller 

Extend name-rev further to support matching refs by adding `--exclude`
patterns. These patterns will limit the scope of refs by excluding any
ref that matches at least one exclude pattern. Checking the exclude refs
shall happen first, before checking the include --refs patterns. This
will allow more flexibility to matching certain kinds of references.

Add tests and update Documentation for this change.

Signed-off-by: Jacob Keller 
---
 Documentation/git-name-rev.txt   |  7 +++
 builtin/name-rev.c   | 14 +-
 t/t6007-rev-list-cherry-pick-file.sh | 12 
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index 7433627db12d..301b4a8d55e6 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -30,6 +30,13 @@ OPTIONS
given multiple times, use refs whose names match any of the given shell
patterns. Use `--no-refs` to clear any previous ref patterns given.
 
+--exclude=::
+   Do not use any ref whose name matches a given shell pattern. The
+   pattern can be one of branch name, tag name or fully qualified ref
+   name. If given multiple times, exclude refs that match any of the given
+   shell patterns. Use `--no-exclude` to clear the list of exclude
+   patterns.
+
 --all::
List all commits reachable from all refs
 
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 000a2a700ed3..da4a0d7c0fdf 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -109,6 +109,7 @@ struct name_ref_data {
int tags_only;
int name_only;
struct string_list ref_filters;
+   struct string_list exclude_filters;
 };
 
 static struct tip_table {
@@ -150,6 +151,15 @@ static int name_ref(const char *path, const struct 
object_id *oid, int flags, vo
if (data->tags_only && !starts_with(path, "refs/tags/"))
return 0;
 
+   if (data->exclude_filters.nr) {
+   struct string_list_item *item;
+
+   for_each_string_list_item(item, >exclude_filters) {
+   if (subpath_matches(path, item->string) >= 0)
+   return 0;
+   }
+   }
+
if (data->ref_filters.nr) {
struct string_list_item *item;
int matched = 0;
@@ -323,12 +333,14 @@ int cmd_name_rev(int argc, const char **argv, const char 
*prefix)
 {
struct object_array revs = OBJECT_ARRAY_INIT;
int all = 0, transform_stdin = 0, allow_undefined = 1, always = 0, 
peel_tag = 0;
-   struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP };
+   struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP, 
STRING_LIST_INIT_NODUP };
struct option opts[] = {
OPT_BOOL(0, "name-only", _only, N_("print only names 
(no SHA-1)")),
OPT_BOOL(0, "tags", _only, N_("only use tags to name 
the commits")),
OPT_STRING_LIST(0, "refs", _filters, N_("pattern"),
   N_("only use refs matching ")),
+   OPT_STRING_LIST(0, "exclude", _filters, 
N_("pattern"),
+  N_("ignore refs matching ")),
OPT_GROUP(""),
OPT_BOOL(0, "all", , N_("list all commits reachable from 
all refs")),
OPT_BOOL(0, "stdin", _stdin, N_("read from stdin")),
diff --git a/t/t6007-rev-list-cherry-pick-file.sh 
b/t/t6007-rev-list-cherry-pick-file.sh
index d9827a6389a3..29597451967d 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -118,6 +118,18 @@ test_expect_success 'name-rev --refs excludes non-matched 
patterns' '
test_cmp actual.named expect
 '
 
+cat >expect <>expect &&
+   git rev-list --left-right --cherry-pick F...E -- bar >actual &&
+   git name-rev --stdin --name-only --refs="*tags/*" --exclude="*E" \
+   actual.named &&
+   test_cmp actual.named expect
+'
+
 test_expect_success 'name-rev --no-refs clears the refs list' '
git rev-list --left-right --cherry-pick F...E -- bar >expect &&
git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" 
--no-refs --refs="*tags/G" \
-- 
2.11.0.403.g196674b8396b

[PATCH v3 4/5] describe: teach --match to accept multiple patterns

[Jacob Keller]

From: Jacob Keller 

Teach `--match` to be accepted multiple times, accumulating a list of
patterns to match into a string list. Each pattern is inclusive, such
that a tag need only match one of the provided patterns to be
considered for matching.

This extension is useful as it enables more flexibility in what tags
match, and may avoid the need to run the describe command multiple
times to get the same result.

Add tests and update the documentation for this change.

Signed-off-by: Jacob Keller 
---
 Documentation/git-describe.txt |  5 -
 builtin/describe.c | 30 +++---
 t/t6120-describe.sh| 19 +++
 3 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index e4ac448ff565..7ad41e2f6ade 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -83,7 +83,10 @@ OPTIONS
 --match ::
Only consider tags matching the given `glob(7)` pattern,
excluding the "refs/tags/" prefix.  This can be used to avoid
-   leaking private tags from the repository.
+   leaking private tags from the repository. If given multiple times, a
+   list of patterns will be accumulated, and tags matching any of the
+   patterns will be considered. Use `--no-match` to clear and reset the
+   list of patterns.
 
 --always::
Show uniquely abbreviated commit object as fallback.
diff --git a/builtin/describe.c b/builtin/describe.c
index 01490a157efc..5cc9e9abe798 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -28,7 +28,7 @@ static int abbrev = -1; /* unspecified */
 static int max_candidates = 10;
 static struct hashmap names;
 static int have_util;
-static const char *pattern;
+static struct string_list patterns = STRING_LIST_INIT_NODUP;
 static int always;
 static const char *dirty;
 
@@ -129,9 +129,24 @@ static int get_name(const char *path, const struct 
object_id *oid, int flag, voi
if (!all && !is_tag)
return 0;
 
-   /* Accept only tags that match the pattern, if given */
-   if (pattern && (!is_tag || wildmatch(pattern, path + 10, 0, NULL)))
-   return 0;
+   /*
+* If we're given patterns, accept only tags which match at least one
+* pattern.
+*/
+   if (patterns.nr) {
+   struct string_list_item *item;
+
+   if (!is_tag)
+   return 0;
+
+   for_each_string_list_item(item, ) {
+   if (!wildmatch(item->string, path + 10, 0, NULL))
+   break;
+
+   /* If we get here, no pattern matched. */
+   return 0;
+   }
+   }
 
/* Is it annotated? */
if (!peel_ref(path, peeled.hash)) {
@@ -404,7 +419,7 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
N_("only output exact matches"), 0),
OPT_INTEGER(0, "candidates", _candidates,
N_("consider  most recent tags (default: 10)")),
-   OPT_STRING(0, "match",   , N_("pattern"),
+   OPT_STRING_LIST(0, "match", , N_("pattern"),
   N_("only consider tags matching ")),
OPT_BOOL(0, "always",,
N_("show abbreviated commit object as fallback")),
@@ -430,6 +445,7 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
die(_("--long is incompatible with --abbrev=0"));
 
if (contains) {
+   struct string_list_item *item;
struct argv_array args;
 
argv_array_init();
@@ -440,8 +456,8 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
argv_array_push(, "--always");
if (!all) {
argv_array_push(, "--tags");
-   if (pattern)
-   argv_array_pushf(, "--refs=refs/tags/%s", 
pattern);
+   for_each_string_list_item(item, )
+   argv_array_pushf(, "--refs=refs/tags/%s", 
item->string);
}
if (argc)
argv_array_pushv(, argv);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 85f269411cb3..9e5db9b87a1f 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -182,6 +182,10 @@ check_describe "test2-lightweight-*" --tags 
--match="test2-*"
 
 check_describe "test2-lightweight-*" --long --tags --match="test2-*" HEAD^
 
+check_describe "test1-lightweight-*" --long --tags --match="test1-*" 
--match="test2-*" HEAD^
+
+check_describe "test2-lightweight-*" --long --tags --match="test1-*" 
--no-match --match="test2-*" HEAD^
+
 test_expect_success 'name-rev with exact tags' '
echo A >expect &&

[PATCH v3 2/5] name-rev: extend --refs to accept multiple patterns

[Jacob Keller]

From: Jacob Keller 

Teach git name-rev to take a string list of patterns from --refs instead
of only a single pattern. The list of patterns will be matched
inclusively, such that a ref only needs to match one pattern to be
included. If a ref will only be excluded if it does not match any of the
patterns.

Add tests and documentation for this change. The tests do use
dynamically generated output as part of the expected output because the
expected output is the raw commit-id and it seemed like a bad idea to
hardcode that into a tests expected output.

Signed-off-by: Jacob Keller 
---
 Documentation/git-name-rev.txt   |  4 +++-
 builtin/name-rev.c   | 41 +---
 t/t6007-rev-list-cherry-pick-file.sh | 26 +++
 3 files changed, 58 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index ca28fb8e2a07..7433627db12d 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -26,7 +26,9 @@ OPTIONS
 
 --refs=::
Only use refs whose names match a given shell pattern.  The pattern
-   can be one of branch name, tag name or fully qualified ref name.
+   can be one of branch name, tag name or fully qualified ref name. If
+   given multiple times, use refs whose names match any of the given shell
+   patterns. Use `--no-refs` to clear any previous ref patterns given.
 
 --all::
List all commits reachable from all refs
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index cd89d48b65e8..000a2a700ed3 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -108,7 +108,7 @@ static const char *name_ref_abbrev(const char *refname, int 
shorten_unambiguous)
 struct name_ref_data {
int tags_only;
int name_only;
-   const char *ref_filter;
+   struct string_list ref_filters;
 };
 
 static struct tip_table {
@@ -150,16 +150,33 @@ static int name_ref(const char *path, const struct 
object_id *oid, int flags, vo
if (data->tags_only && !starts_with(path, "refs/tags/"))
return 0;
 
-   if (data->ref_filter) {
-   switch (subpath_matches(path, data->ref_filter)) {
-   case -1: /* did not match */
-   return 0;
-   case 0:  /* matched fully */
-   break;
-   default: /* matched subpath */
-   can_abbreviate_output = 1;
-   break;
+   if (data->ref_filters.nr) {
+   struct string_list_item *item;
+   int matched = 0;
+
+   /* See if any of the patterns match. */
+   for_each_string_list_item(item, >ref_filters) {
+   /*
+* We want to check every pattern even if we already
+* found a match, just in case one of the later
+* patterns could abbreviate the output.
+*/
+   switch (subpath_matches(path, item->string)) {
+   case -1: /* did not match */
+   break;
+   case 0: /* matched fully */
+   matched = 1;
+   break;
+   default: /* matched subpath */
+   matched = 1;
+   can_abbreviate_output = 1;
+   break;
+   }
}
+
+   /* If none of the patterns matched, stop now */
+   if (!matched)
+   return 0;
}
 
add_to_tip_table(oid->hash, path, can_abbreviate_output);
@@ -306,11 +323,11 @@ int cmd_name_rev(int argc, const char **argv, const char 
*prefix)
 {
struct object_array revs = OBJECT_ARRAY_INIT;
int all = 0, transform_stdin = 0, allow_undefined = 1, always = 0, 
peel_tag = 0;
-   struct name_ref_data data = { 0, 0, NULL };
+   struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP };
struct option opts[] = {
OPT_BOOL(0, "name-only", _only, N_("print only names 
(no SHA-1)")),
OPT_BOOL(0, "tags", _only, N_("only use tags to name 
the commits")),
-   OPT_STRING(0, "refs", _filter, N_("pattern"),
+   OPT_STRING_LIST(0, "refs", _filters, N_("pattern"),
   N_("only use refs matching ")),
OPT_GROUP(""),
OPT_BOOL(0, "all", , N_("list all commits reachable from 
all refs")),
diff --git a/t/t6007-rev-list-cherry-pick-file.sh 
b/t/t6007-rev-list-cherry-pick-file.sh
index 1408b608eb03..d9827a6389a3 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -99,6 +99,32 @@ test_expect_success '--cherry-pick bar does not come up 
empty (II)' '
   

[PATCH v3 5/5] describe: teach describe negative pattern matches

[Jacob Keller]

From: Jacob Keller 

Teach git-describe the `--exclude` option which will allow specifying
a glob pattern of tags to ignore. This can be combined with the
`--match` patterns to enable more flexibility in determining which tags
to consider.

For example, suppose you wish to find the first official release tag
that contains a certain commit. If we assume that official release tags
are of the form "v*" and pre-release candidates include "*rc*" in their
name, we can now find the first tag that introduces commit abcdef via:

  git describe --contains --match="v*" --exclude="*rc*"

Add documentation and tests for this change.

Signed-off-by: Jacob Keller 
---
 Documentation/git-describe.txt |  8 
 builtin/describe.c | 21 +
 t/t6120-describe.sh|  8 
 3 files changed, 37 insertions(+)

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index 7ad41e2f6ade..21a43b78924a 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -88,6 +88,14 @@ OPTIONS
patterns will be considered. Use `--no-match` to clear and reset the
list of patterns.
 
+--exclude ::
+   Do not consider tags matching the given `glob(7)` pattern, excluding
+   the "refs/tags/" prefix. This can be used to narrow the tag space and
+   find only tags matching some meaningful criteria. If given multiple
+   times, a list of patterns will be accumulated and tags matching any
+   of the patterns will be excluded. Use `--no-exclude` to clear and
+   reset the list of patterns.
+
 --always::
Show uniquely abbreviated commit object as fallback.
 
diff --git a/builtin/describe.c b/builtin/describe.c
index 5cc9e9abe798..6769446e1f57 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -29,6 +29,7 @@ static int max_candidates = 10;
 static struct hashmap names;
 static int have_util;
 static struct string_list patterns = STRING_LIST_INIT_NODUP;
+static struct string_list exclude_patterns = STRING_LIST_INIT_NODUP;
 static int always;
 static const char *dirty;
 
@@ -130,6 +131,22 @@ static int get_name(const char *path, const struct 
object_id *oid, int flag, voi
return 0;
 
/*
+* If we're given exclude patterns, first exclude any tag which match
+* any of the exclude pattern.
+*/
+   if (exclude_patterns.nr) {
+   struct string_list_item *item;
+
+   if (!is_tag)
+   return 0;
+
+   for_each_string_list_item(item, _patterns) {
+   if (!wildmatch(item->string, path + 10, 0, NULL))
+   return 0;
+   }
+   }
+
+   /*
 * If we're given patterns, accept only tags which match at least one
 * pattern.
 */
@@ -421,6 +438,8 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
N_("consider  most recent tags (default: 10)")),
OPT_STRING_LIST(0, "match", , N_("pattern"),
   N_("only consider tags matching ")),
+   OPT_STRING_LIST(0, "exclude", _patterns, N_("pattern"),
+  N_("do not consider tags matching ")),
OPT_BOOL(0, "always",,
N_("show abbreviated commit object as fallback")),
{OPTION_STRING, 0, "dirty",  , N_("mark"),
@@ -458,6 +477,8 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
argv_array_push(, "--tags");
for_each_string_list_item(item, )
argv_array_pushf(, "--refs=refs/tags/%s", 
item->string);
+   for_each_string_list_item(item, _patterns)
+   argv_array_pushf(, 
"--exclude=refs/tags/%s", item->string);
}
if (argc)
argv_array_pushv(, argv);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 9e5db9b87a1f..167491fd5b0d 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -218,6 +218,14 @@ test_expect_success 'describe --contains and --match' '
test_cmp expect actual
 '
 
+test_expect_success 'describe --exclude' '
+   echo "c~1" >expect &&
+   tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+   test_must_fail git describe --contains --match="B" $tagged_commit &&
+   git describe --contains --match="?" --exclude="A" $tagged_commit 
>actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'describe --contains and --no-match' '
echo "A^0" >expect &&
tagged_commit=$(git rev-parse "refs/tags/A^0") &&
-- 
2.11.0.403.g196674b8396b

[PATCH v3 0/5] extend git-describe pattern matching

[Jacob Keller]

From: Jacob Keller 

** v3 fixes a minor typo in one of the test cases, so please ignore v2
   I left the interdiff as between v1 and v3 instead of v2 **

Teach git describe and git name-rev the ability to match multiple
patterns inclusively. Additionally, teach these commands to also accept
negative patterns to exclude any refs which match.

The pattern lists for positive and negative patterns are inclusive. This
means that for the positive patterns, a reference will be considered as
long as it matches at least one of the match patterns. It need not match
all given patterns. Additionally for negative patterns, we will not
consider any ref which matches any negative pattern, even if it matches
one of the positive patterns.

Together this allows the ability to express far more sets of tags than a
single match pattern alone. It does not provide quite the same depth as
would teaching full regexp but it is simpler and easy enough to
understand.

- v2
* use --exclude instead of --discard
* use modern style in tests

- v3
* fix broken test (sorry for the thrash!)

I chose *not* to implement the suggestion of ordered values for exclude
and match, since this is not how the current implementation of
git-describe worked, and it didn't really make sense to me what was
being requested. I looked at the interface for git-log, and it appears
that the command accepts multiple invocations of --branches, --remotes,
and similar. I do not believe these need to be identical interfaces. I
welcome feedback on this decision, but I am not convinced yet that the
ordered arguments are worth the trouble.

diff --git c/Documentation/git-describe.txt w/Documentation/git-describe.txt
index a89bbde207b2..21a43b78924a 100644
--- c/Documentation/git-describe.txt
+++ w/Documentation/git-describe.txt
@@ -88,12 +88,12 @@ OPTIONS
patterns will be considered. Use `--no-match` to clear and reset the
list of patterns.
 
---discard ::
+--exclude ::
Do not consider tags matching the given `glob(7)` pattern, excluding
the "refs/tags/" prefix. This can be used to narrow the tag space and
find only tags matching some meaningful criteria. If given multiple
times, a list of patterns will be accumulated and tags matching any
-   of the patterns will be discarded. Use `--no-discard` to clear and
+   of the patterns will be excluded. Use `--no-exclude` to clear and
reset the list of patterns.
 
 --always::
diff --git c/Documentation/git-name-rev.txt w/Documentation/git-name-rev.txt
index 9b46e5ea9aae..301b4a8d55e6 100644
--- c/Documentation/git-name-rev.txt
+++ w/Documentation/git-name-rev.txt
@@ -30,11 +30,11 @@ OPTIONS
given multiple times, use refs whose names match any of the given shell
patterns. Use `--no-refs` to clear any previous ref patterns given.
 
---discard=::
+--exclude=::
Do not use any ref whose name matches a given shell pattern. The
pattern can be one of branch name, tag name or fully qualified ref
-   name. If given multiple times, discard refs that match any of the given
-   shell patterns. Use `--no-discards` to clear the list of discard
+   name. If given multiple times, exclude refs that match any of the given
+   shell patterns. Use `--no-exclude` to clear the list of exclude
patterns.
 
 --all::
diff --git c/Documentation/technical/api-parse-options.txt 
w/Documentation/technical/api-parse-options.txt
index 15e876e4c804..6914f54f5f44 100644
--- c/Documentation/technical/api-parse-options.txt
+++ w/Documentation/technical/api-parse-options.txt
@@ -169,9 +169,9 @@ There are some macros to easily define options:
The string argument is put into `str_var`.
 
 `OPT_STRING_LIST(short, long, , arg_str, description)`::
-   Introduce an option with a string argument. Repeated invocations
-   accumulate into a list of strings. Reset and clear the list with
-   `--no-option`.
+   Introduce an option with string argument.
+   The string argument is stored as an element in `` which must be a
+   struct string_list. Reset the list using `--no-option`.
 
 `OPT_INTEGER(short, long, _var, description)`::
Introduce an option with integer argument.
diff --git c/builtin/describe.c w/builtin/describe.c
index c09288ee6321..6769446e1f57 100644
--- c/builtin/describe.c
+++ w/builtin/describe.c
@@ -29,7 +29,7 @@ static int max_candidates = 10;
 static struct hashmap names;
 static int have_util;
 static struct string_list patterns = STRING_LIST_INIT_NODUP;
-static struct string_list discard_patterns = STRING_LIST_INIT_NODUP;
+static struct string_list exclude_patterns = STRING_LIST_INIT_NODUP;
 static int always;
 static const char *dirty;
 
@@ -131,16 +131,16 @@ static int get_name(const char *path, const struct 
object_id *oid, int flag, voi
return 0;
 
/*
-* If we're given discard patterns, first discard any tag which match
-* any 

[PATCH v3 1/5] doc: add documentation for OPT_STRING_LIST

[Jacob Keller]

From: Jacob Keller 

Commit c8ba16391655 ("parse-options: add OPT_STRING_LIST helper",
2011-06-09) added the OPT_STRING_LIST as a way to accumulate a repeated
list of strings. However, this was not documented in the
api-parse-options documentation. Add documentation now so that future
developers may learn of its existence.

Signed-off-by: Jacob Keller 
---
 Documentation/technical/api-parse-options.txt | 5 +
 1 file changed, 5 insertions(+)

diff --git a/Documentation/technical/api-parse-options.txt 
b/Documentation/technical/api-parse-options.txt
index 27bd701c0d68..6914f54f5f44 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -168,6 +168,11 @@ There are some macros to easily define options:
Introduce an option with string argument.
The string argument is put into `str_var`.
 
+`OPT_STRING_LIST(short, long, , arg_str, description)`::
+   Introduce an option with string argument.
+   The string argument is stored as an element in `` which must be a
+   struct string_list. Reset the list using `--no-option`.
+
 `OPT_INTEGER(short, long, _var, description)`::
Introduce an option with integer argument.
The integer is put into `int_var`.
-- 
2.11.0.403.g196674b8396b

Re: [PATCH] transport submodules: correct error message

[Stefan Beller]

On Tue, Jan 17, 2017 at 3:42 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Trying to push with --recurse-submodules=on-demand would run into
>> the same problem. To fix this issue
>> 1) specifically mention that we looked for branches on the remote.
>
> That makes an incorrect statement ("not found on any remote"---we
> did not inspect all of the said remote, only heads and tags) into an
> irrelevant statement ("not found on any remote branch"---the end
> user would say "so what?  I know it exists there, it's just that not
> all remote refs have corresponding tracking ref locally on our side").

eh. So to be correct we need to tell the user we did not find any match on
a "remote-tracking branch" as the gitglossary puts it.

>
> Perhaps it may be an improvement.
>
>> 2) advertise pushing without recursing into submodules. ("Use this
>>command to make the error message go away")
>
> Not mentioning "on-demand" may be an improvement for those who do
> use set-up like Dave has,

Daves setup is not a special setup; it is just git@next (later than
1863e05af5) in a repo that happens to have submodules; using
Gerrit as the code review system.

> where remote tracking information is
> incomplete if you only look at heads and refs, in the sense that we
> no longer suggest ineffective workaround.

s/ineffective/an effective/ ?

Yes. I think this is a problem in general with Git and Gerrit, as they have
different assumptions of the refs/ namespace.
(You can obtain all Gerrit changes in flight via fetching the refs/changes/*
similar to you pushing all git.git working branches not to all repositories,
but only https://github.com/gitster/git/)

> But would it be an improvement to suggest --no-recurse-submodules?

Well it is certainly better than the current situation, the user DID
use --recurse-submodules but still git refuses and advises to use
--recurse-submodules, which is understandable with background
knowledge, but very confusing at least.

The hint for --no-recurse-submodules can be understood as:
  "I told you there is something fishy with submodules, now you
  can just ignore this warning and murk up your history as you like."
which let's me question the correctness of assumptions in
1863e05af5^2.

> This issue seems like a property of the particular set-up, as
> opposed to being a one-off issue.  The next, subsequent

until Dave got the changes in the submodules reviewed and they
appear on a target branch, then fetching them, then the warning will
be gone.

> and probably
> all future pushes from that repository will have the same issue
> because the root cause is not due to the relative position of
> commits we have locally vs the tips of remote, but due to the way
> remote tracking is set-up, no?

Yes; ideally we'd have a remote-tracking ref for each change
pushed to Gerrit.

>
> If that is the case, perhaps configuring push.recurseSubmodules to
> turn this off (especially because you plan to turn the defaul to
> "check") and not giving the command line option would give a more
> pleasant end-user experience, I suspect.

I though about going another way and adding another new value
to the enum, such that

git push --recurse-submodules=sameRefSpecButNoCheck \
origin HEAD:refs/for/master

works for Gerrit users.

If such a thing exists, then the check is the safest-but-inconvenient
option (in all setups I'd think), and users can switch to either
sameRefSpecButNoCheck (Gerrit) or on-demand (github et al)

[PATCH v2 3/5] name-rev: add support to exclude refs by pattern match

[Jacob Keller]

From: Jacob Keller 

Extend name-rev further to support matching refs by adding `--exclude`
patterns. These patterns will limit the scope of refs by excluding any
ref that matches at least one exclude pattern. Checking the exclude refs
shall happen first, before checking the include --refs patterns. This
will allow more flexibility to matching certain kinds of references.

Add tests and update Documentation for this change.

Signed-off-by: Jacob Keller 
---
 Documentation/git-name-rev.txt   |  7 +++
 builtin/name-rev.c   | 14 +-
 t/t6007-rev-list-cherry-pick-file.sh | 12 
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index 7433627db12d..301b4a8d55e6 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -30,6 +30,13 @@ OPTIONS
given multiple times, use refs whose names match any of the given shell
patterns. Use `--no-refs` to clear any previous ref patterns given.
 
+--exclude=::
+   Do not use any ref whose name matches a given shell pattern. The
+   pattern can be one of branch name, tag name or fully qualified ref
+   name. If given multiple times, exclude refs that match any of the given
+   shell patterns. Use `--no-exclude` to clear the list of exclude
+   patterns.
+
 --all::
List all commits reachable from all refs
 
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 000a2a700ed3..da4a0d7c0fdf 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -109,6 +109,7 @@ struct name_ref_data {
int tags_only;
int name_only;
struct string_list ref_filters;
+   struct string_list exclude_filters;
 };
 
 static struct tip_table {
@@ -150,6 +151,15 @@ static int name_ref(const char *path, const struct 
object_id *oid, int flags, vo
if (data->tags_only && !starts_with(path, "refs/tags/"))
return 0;
 
+   if (data->exclude_filters.nr) {
+   struct string_list_item *item;
+
+   for_each_string_list_item(item, >exclude_filters) {
+   if (subpath_matches(path, item->string) >= 0)
+   return 0;
+   }
+   }
+
if (data->ref_filters.nr) {
struct string_list_item *item;
int matched = 0;
@@ -323,12 +333,14 @@ int cmd_name_rev(int argc, const char **argv, const char 
*prefix)
 {
struct object_array revs = OBJECT_ARRAY_INIT;
int all = 0, transform_stdin = 0, allow_undefined = 1, always = 0, 
peel_tag = 0;
-   struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP };
+   struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP, 
STRING_LIST_INIT_NODUP };
struct option opts[] = {
OPT_BOOL(0, "name-only", _only, N_("print only names 
(no SHA-1)")),
OPT_BOOL(0, "tags", _only, N_("only use tags to name 
the commits")),
OPT_STRING_LIST(0, "refs", _filters, N_("pattern"),
   N_("only use refs matching ")),
+   OPT_STRING_LIST(0, "exclude", _filters, 
N_("pattern"),
+  N_("ignore refs matching ")),
OPT_GROUP(""),
OPT_BOOL(0, "all", , N_("list all commits reachable from 
all refs")),
OPT_BOOL(0, "stdin", _stdin, N_("read from stdin")),
diff --git a/t/t6007-rev-list-cherry-pick-file.sh 
b/t/t6007-rev-list-cherry-pick-file.sh
index f724ff24044b..83838d0da208 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -118,6 +118,18 @@ test_expect_success 'name-rev --refs excludes non-matched 
patterns' '
test_cmp actual.named expect
 '
 
+cat >expect <>expect &&
+   git rev-list --left-right --cherry-pick F...E -- bar >actual &&
+   git name-rev --stdin --name-only --refs="*tags/*" --exclude="*E" \
+   actual.named &&
+   test_cmp actual.named expect
+'
+
 test_expect_success 'name-rev --no-refs clears the refs list' '
git rev-list --left-right --cherry-pick F...E -- bar >expect &&
git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" 
--no-refs --refs="*tags/G" \
-- 
2.11.0.403.g196674b8396b

Re: [PATCH v6 0/6] Add --format to tag verification

[Junio C Hamano]

santi...@nyu.edu writes:

> From: Santiago Torres 
>
> This is the sixth iteration of [1][2][3][4][5], and as a result of the
> discussion in [5]. The main goal of this patch series is to bring
> --format to git tag verification so that upper-layer tools can inspect
> the content of a tag and make decisions based on it.
>
> In this re-woll we:
>
> * Changed the call interface so printing is done outside of verification. 
>
> * Fixed a couple of whitespace issues and whatnot. 

With the small code structure change Peff suggested the result looks
much easier to read.  I didn't spot any more problems.

Will replace what has been sitting in my tree.  Thanks.

[PATCH v2 2/5] name-rev: extend --refs to accept multiple patterns

[Jacob Keller]

From: Jacob Keller 

Teach git name-rev to take a string list of patterns from --refs instead
of only a single pattern. The list of patterns will be matched
inclusively, such that a ref only needs to match one pattern to be
included. If a ref will only be excluded if it does not match any of the
patterns.

Add tests and documentation for this change. The tests do use
dynamically generated output as part of the expected output because the
expected output is the raw commit-id and it seemed like a bad idea to
hardcode that into a tests expected output.

Signed-off-by: Jacob Keller 
---
 Documentation/git-name-rev.txt   |  4 +++-
 builtin/name-rev.c   | 41 +---
 t/t6007-rev-list-cherry-pick-file.sh | 26 +++
 3 files changed, 58 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index ca28fb8e2a07..7433627db12d 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -26,7 +26,9 @@ OPTIONS
 
 --refs=::
Only use refs whose names match a given shell pattern.  The pattern
-   can be one of branch name, tag name or fully qualified ref name.
+   can be one of branch name, tag name or fully qualified ref name. If
+   given multiple times, use refs whose names match any of the given shell
+   patterns. Use `--no-refs` to clear any previous ref patterns given.
 
 --all::
List all commits reachable from all refs
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index cd89d48b65e8..000a2a700ed3 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -108,7 +108,7 @@ static const char *name_ref_abbrev(const char *refname, int 
shorten_unambiguous)
 struct name_ref_data {
int tags_only;
int name_only;
-   const char *ref_filter;
+   struct string_list ref_filters;
 };
 
 static struct tip_table {
@@ -150,16 +150,33 @@ static int name_ref(const char *path, const struct 
object_id *oid, int flags, vo
if (data->tags_only && !starts_with(path, "refs/tags/"))
return 0;
 
-   if (data->ref_filter) {
-   switch (subpath_matches(path, data->ref_filter)) {
-   case -1: /* did not match */
-   return 0;
-   case 0:  /* matched fully */
-   break;
-   default: /* matched subpath */
-   can_abbreviate_output = 1;
-   break;
+   if (data->ref_filters.nr) {
+   struct string_list_item *item;
+   int matched = 0;
+
+   /* See if any of the patterns match. */
+   for_each_string_list_item(item, >ref_filters) {
+   /*
+* We want to check every pattern even if we already
+* found a match, just in case one of the later
+* patterns could abbreviate the output.
+*/
+   switch (subpath_matches(path, item->string)) {
+   case -1: /* did not match */
+   break;
+   case 0: /* matched fully */
+   matched = 1;
+   break;
+   default: /* matched subpath */
+   matched = 1;
+   can_abbreviate_output = 1;
+   break;
+   }
}
+
+   /* If none of the patterns matched, stop now */
+   if (!matched)
+   return 0;
}
 
add_to_tip_table(oid->hash, path, can_abbreviate_output);
@@ -306,11 +323,11 @@ int cmd_name_rev(int argc, const char **argv, const char 
*prefix)
 {
struct object_array revs = OBJECT_ARRAY_INIT;
int all = 0, transform_stdin = 0, allow_undefined = 1, always = 0, 
peel_tag = 0;
-   struct name_ref_data data = { 0, 0, NULL };
+   struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP };
struct option opts[] = {
OPT_BOOL(0, "name-only", _only, N_("print only names 
(no SHA-1)")),
OPT_BOOL(0, "tags", _only, N_("only use tags to name 
the commits")),
-   OPT_STRING(0, "refs", _filter, N_("pattern"),
+   OPT_STRING_LIST(0, "refs", _filters, N_("pattern"),
   N_("only use refs matching ")),
OPT_GROUP(""),
OPT_BOOL(0, "all", , N_("list all commits reachable from 
all refs")),
diff --git a/t/t6007-rev-list-cherry-pick-file.sh 
b/t/t6007-rev-list-cherry-pick-file.sh
index 1408b608eb03..f724ff24044b 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -99,6 +99,32 @@ test_expect_success '--cherry-pick bar does not come up 
empty (II)' '
   

[PATCH v2 4/5] describe: teach --match to accept multiple patterns

[Jacob Keller]

From: Jacob Keller 

Teach `--match` to be accepted multiple times, accumulating a list of
patterns to match into a string list. Each pattern is inclusive, such
that a tag need only match one of the provided patterns to be
considered for matching.

This extension is useful as it enables more flexibility in what tags
match, and may avoid the need to run the describe command multiple
times to get the same result.

Add tests and update the documentation for this change.

Signed-off-by: Jacob Keller 
---
 Documentation/git-describe.txt |  5 -
 builtin/describe.c | 30 +++---
 t/t6120-describe.sh| 19 +++
 3 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index e4ac448ff565..7ad41e2f6ade 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -83,7 +83,10 @@ OPTIONS
 --match ::
Only consider tags matching the given `glob(7)` pattern,
excluding the "refs/tags/" prefix.  This can be used to avoid
-   leaking private tags from the repository.
+   leaking private tags from the repository. If given multiple times, a
+   list of patterns will be accumulated, and tags matching any of the
+   patterns will be considered. Use `--no-match` to clear and reset the
+   list of patterns.
 
 --always::
Show uniquely abbreviated commit object as fallback.
diff --git a/builtin/describe.c b/builtin/describe.c
index 01490a157efc..5cc9e9abe798 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -28,7 +28,7 @@ static int abbrev = -1; /* unspecified */
 static int max_candidates = 10;
 static struct hashmap names;
 static int have_util;
-static const char *pattern;
+static struct string_list patterns = STRING_LIST_INIT_NODUP;
 static int always;
 static const char *dirty;
 
@@ -129,9 +129,24 @@ static int get_name(const char *path, const struct 
object_id *oid, int flag, voi
if (!all && !is_tag)
return 0;
 
-   /* Accept only tags that match the pattern, if given */
-   if (pattern && (!is_tag || wildmatch(pattern, path + 10, 0, NULL)))
-   return 0;
+   /*
+* If we're given patterns, accept only tags which match at least one
+* pattern.
+*/
+   if (patterns.nr) {
+   struct string_list_item *item;
+
+   if (!is_tag)
+   return 0;
+
+   for_each_string_list_item(item, ) {
+   if (!wildmatch(item->string, path + 10, 0, NULL))
+   break;
+
+   /* If we get here, no pattern matched. */
+   return 0;
+   }
+   }
 
/* Is it annotated? */
if (!peel_ref(path, peeled.hash)) {
@@ -404,7 +419,7 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
N_("only output exact matches"), 0),
OPT_INTEGER(0, "candidates", _candidates,
N_("consider  most recent tags (default: 10)")),
-   OPT_STRING(0, "match",   , N_("pattern"),
+   OPT_STRING_LIST(0, "match", , N_("pattern"),
   N_("only consider tags matching ")),
OPT_BOOL(0, "always",,
N_("show abbreviated commit object as fallback")),
@@ -430,6 +445,7 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
die(_("--long is incompatible with --abbrev=0"));
 
if (contains) {
+   struct string_list_item *item;
struct argv_array args;
 
argv_array_init();
@@ -440,8 +456,8 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
argv_array_push(, "--always");
if (!all) {
argv_array_push(, "--tags");
-   if (pattern)
-   argv_array_pushf(, "--refs=refs/tags/%s", 
pattern);
+   for_each_string_list_item(item, )
+   argv_array_pushf(, "--refs=refs/tags/%s", 
item->string);
}
if (argc)
argv_array_pushv(, argv);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 85f269411cb3..9e5db9b87a1f 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -182,6 +182,10 @@ check_describe "test2-lightweight-*" --tags 
--match="test2-*"
 
 check_describe "test2-lightweight-*" --long --tags --match="test2-*" HEAD^
 
+check_describe "test1-lightweight-*" --long --tags --match="test1-*" 
--match="test2-*" HEAD^
+
+check_describe "test2-lightweight-*" --long --tags --match="test1-*" 
--no-match --match="test2-*" HEAD^
+
 test_expect_success 'name-rev with exact tags' '
echo A >expect &&

Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v

[Junio C Hamano]

santi...@nyu.edu writes:

> -static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
> +static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
> + void *cb_data)
>  {
>   const char **p;
>   char ref[PATH_MAX];
>   int had_error = 0;
>   unsigned char sha1[20];
>  
> +

Why?  I'll remove this while queuing.

>   for (p = argv; *p; p++) {
>   if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p)
>   >= sizeof(ref)) {

[PATCH v2 1/5] doc: add documentation for OPT_STRING_LIST

[Jacob Keller]

From: Jacob Keller 

Commit c8ba16391655 ("parse-options: add OPT_STRING_LIST helper",
2011-06-09) added the OPT_STRING_LIST as a way to accumulate a repeated
list of strings. However, this was not documented in the
api-parse-options documentation. Add documentation now so that future
developers may learn of its existence.

Signed-off-by: Jacob Keller 
---
 Documentation/technical/api-parse-options.txt | 5 +
 1 file changed, 5 insertions(+)

diff --git a/Documentation/technical/api-parse-options.txt 
b/Documentation/technical/api-parse-options.txt
index 27bd701c0d68..6914f54f5f44 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -168,6 +168,11 @@ There are some macros to easily define options:
Introduce an option with string argument.
The string argument is put into `str_var`.
 
+`OPT_STRING_LIST(short, long, , arg_str, description)`::
+   Introduce an option with string argument.
+   The string argument is stored as an element in `` which must be a
+   struct string_list. Reset the list using `--no-option`.
+
 `OPT_INTEGER(short, long, _var, description)`::
Introduce an option with integer argument.
The integer is put into `int_var`.
-- 
2.11.0.403.g196674b8396b

[PATCH v2 5/5] describe: teach describe negative pattern matches

[Jacob Keller]

From: Jacob Keller 

Teach git-describe the `--exclude` option which will allow specifying
a glob pattern of tags to ignore. This can be combined with the
`--match` patterns to enable more flexibility in determining which tags
to consider.

For example, suppose you wish to find the first official release tag
that contains a certain commit. If we assume that official release tags
are of the form "v*" and pre-release candidates include "*rc*" in their
name, we can now find the first tag that introduces commit abcdef via:

  git describe --contains --match="v*" --exclude="*rc*"

Add documentation and tests for this change.

Signed-off-by: Jacob Keller 
---
 Documentation/git-describe.txt |  8 
 builtin/describe.c | 21 +
 t/t6120-describe.sh|  8 
 3 files changed, 37 insertions(+)

diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index 7ad41e2f6ade..21a43b78924a 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -88,6 +88,14 @@ OPTIONS
patterns will be considered. Use `--no-match` to clear and reset the
list of patterns.
 
+--exclude ::
+   Do not consider tags matching the given `glob(7)` pattern, excluding
+   the "refs/tags/" prefix. This can be used to narrow the tag space and
+   find only tags matching some meaningful criteria. If given multiple
+   times, a list of patterns will be accumulated and tags matching any
+   of the patterns will be excluded. Use `--no-exclude` to clear and
+   reset the list of patterns.
+
 --always::
Show uniquely abbreviated commit object as fallback.
 
diff --git a/builtin/describe.c b/builtin/describe.c
index 5cc9e9abe798..6769446e1f57 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -29,6 +29,7 @@ static int max_candidates = 10;
 static struct hashmap names;
 static int have_util;
 static struct string_list patterns = STRING_LIST_INIT_NODUP;
+static struct string_list exclude_patterns = STRING_LIST_INIT_NODUP;
 static int always;
 static const char *dirty;
 
@@ -130,6 +131,22 @@ static int get_name(const char *path, const struct 
object_id *oid, int flag, voi
return 0;
 
/*
+* If we're given exclude patterns, first exclude any tag which match
+* any of the exclude pattern.
+*/
+   if (exclude_patterns.nr) {
+   struct string_list_item *item;
+
+   if (!is_tag)
+   return 0;
+
+   for_each_string_list_item(item, _patterns) {
+   if (!wildmatch(item->string, path + 10, 0, NULL))
+   return 0;
+   }
+   }
+
+   /*
 * If we're given patterns, accept only tags which match at least one
 * pattern.
 */
@@ -421,6 +438,8 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
N_("consider  most recent tags (default: 10)")),
OPT_STRING_LIST(0, "match", , N_("pattern"),
   N_("only consider tags matching ")),
+   OPT_STRING_LIST(0, "exclude", _patterns, N_("pattern"),
+  N_("do not consider tags matching ")),
OPT_BOOL(0, "always",,
N_("show abbreviated commit object as fallback")),
{OPTION_STRING, 0, "dirty",  , N_("mark"),
@@ -458,6 +477,8 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
argv_array_push(, "--tags");
for_each_string_list_item(item, )
argv_array_pushf(, "--refs=refs/tags/%s", 
item->string);
+   for_each_string_list_item(item, _patterns)
+   argv_array_pushf(, 
"--exclude=refs/tags/%s", item->string);
}
if (argc)
argv_array_pushv(, argv);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 9e5db9b87a1f..167491fd5b0d 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -218,6 +218,14 @@ test_expect_success 'describe --contains and --match' '
test_cmp expect actual
 '
 
+test_expect_success 'describe --exclude' '
+   echo "c~1" >expect &&
+   tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+   test_must_fail git describe --contains --match="B" $tagged_commit &&
+   git describe --contains --match="?" --exclude="A" $tagged_commit 
>actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'describe --contains and --no-match' '
echo "A^0" >expect &&
tagged_commit=$(git rev-parse "refs/tags/A^0") &&
-- 
2.11.0.403.g196674b8396b

Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v

[Junio C Hamano]

santi...@nyu.edu writes:

> + if (gpg_verify_tag(sha1, name, flags))
> + return -1;
> +
> +if (fmt_pretty)
> + pretty_print_ref(name, sha1, fmt_pretty);

That's a funny indentation.  I'll fix it up locally while queuing.

> +
> + return 0;
>  }

Re: [PATCH] transport submodules: correct error message

[Junio C Hamano]

Stefan Beller  writes:

> Trying to push with --recurse-submodules=on-demand would run into
> the same problem. To fix this issue
> 1) specifically mention that we looked for branches on the remote.

That makes an incorrect statement ("not found on any remote"---we
did not inspect all of the said remote, only heads and tags) into an
irrelevant statement ("not found on any remote branch"---the end
user would say "so what?  I know it exists there, it's just that not
all remote refs have corresponding tracking ref locally on our side").

Perhaps it may be an improvement.

> 2) advertise pushing without recursing into submodules. ("Use this
>command to make the error message go away")

Not mentioning "on-demand" may be an improvement for those who do
use set-up like Dave has, where remote tracking information is
incomplete if you only look at heads and refs, in the sense that we
no longer suggest ineffective workaround.  

But would it be an improvement to suggest --no-recurse-submodules?

This issue seems like a property of the particular set-up, as
opposed to being a one-off issue.  The next, subsequent and probably
all future pushes from that repository will have the same issue
because the root cause is not due to the relative position of
commits we have locally vs the tips of remote, but due to the way
remote tracking is set-up, no?

If that is the case, perhaps configuring push.recurseSubmodules to
turn this off (especially because you plan to turn the defaul to
"check") and not giving the command line option would give a more
pleasant end-user experience, I suspect.


>
> While at it, remove some empty lines, as they blow up the error message.
>
> Reported-by: Dave Borowitz 
> Signed-off-by: Stefan Beller 
> ---
>  transport.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/transport.c b/transport.c
> index 3e8799a611..2445bf0dca 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -883,14 +883,14 @@ static void die_with_unpushed_submodules(struct 
> string_list *needs_pushing)
>   int i;
>  
>   fprintf(stderr, _("The following submodule paths contain changes that 
> can\n"
> - "not be found on any remote:\n"));
> + "not be found on any remote branch:\n"));
>   for (i = 0; i < needs_pushing->nr; i++)
>   fprintf(stderr, "  %s\n", needs_pushing->items[i].string);
> - fprintf(stderr, _("\nPlease try\n\n"
> -   " git push --recurse-submodules=on-demand\n\n"
> -   "or cd to the path and use\n\n"
> -   " git push\n\n"
> -   "to push them to a remote.\n\n"));
> + fprintf(stderr, _("\nSuppress submodule checks via\n"
> +   " git push --no-recurse-submodules\n"
> +   "or cd to the path and use\n"
> +   " git push\n"
> +   "to push them to a remote.\n"));
>  
>   string_list_clear(needs_pushing, 0);

[PATCH v6 2/6] ref-filter: add function to print single ref_array_item

[santiago]

From: Lukas Puehringer 

ref-filter functions are useful for printing git object information
using a format specifier. However, some other modules may not want to use
this functionality on a ref-array but only print a single item.

Expose a pretty_print_ref function to create, pretty print and free
individual ref-items.

Signed-off-by: Lukas Puehringer 
---
 ref-filter.c | 27 +--
 ref-filter.h |  7 +++
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 1a978405e..5f4b08792 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1361,7 +1361,7 @@ static struct ref_array_item *new_ref_array_item(const 
char *refname,
return ref;
 }
 
-static int filter_ref_kind(struct ref_filter *filter, const char *refname)
+static int ref_kind_from_refname(const char *refname)
 {
unsigned int i;
 
@@ -1374,11 +1374,7 @@ static int filter_ref_kind(struct ref_filter *filter, 
const char *refname)
{ "refs/tags/", FILTER_REFS_TAGS}
};
 
-   if (filter->kind == FILTER_REFS_BRANCHES ||
-   filter->kind == FILTER_REFS_REMOTES ||
-   filter->kind == FILTER_REFS_TAGS)
-   return filter->kind;
-   else if (!strcmp(refname, "HEAD"))
+   if (!strcmp(refname, "HEAD"))
return FILTER_REFS_DETACHED_HEAD;
 
for (i = 0; i < ARRAY_SIZE(ref_kind); i++) {
@@ -1389,6 +1385,15 @@ static int filter_ref_kind(struct ref_filter *filter, 
const char *refname)
return FILTER_REFS_OTHERS;
 }
 
+static int filter_ref_kind(struct ref_filter *filter, const char *refname)
+{
+   if (filter->kind == FILTER_REFS_BRANCHES ||
+   filter->kind == FILTER_REFS_REMOTES ||
+   filter->kind == FILTER_REFS_TAGS)
+   return filter->kind;
+   return ref_kind_from_refname(refname);
+}
+
 /*
  * A call-back given to for_each_ref().  Filter refs and keep them for
  * later object processing.
@@ -1671,6 +1676,16 @@ void show_ref_array_item(struct ref_array_item *info, 
const char *format, int qu
putchar('\n');
 }
 
+void pretty_print_ref(const char *name, const unsigned char *sha1,
+   const char *format)
+{
+   struct ref_array_item *ref_item;
+   ref_item = new_ref_array_item(name, sha1, 0);
+   ref_item->kind = ref_kind_from_refname(name);
+   show_ref_array_item(ref_item, format, 0);
+   free_array_item(ref_item);
+}
+
 /*  If no sorting option is given, use refname to sort as default */
 struct ref_sorting *ref_default_sorting(void)
 {
diff --git a/ref-filter.h b/ref-filter.h
index fc55fa357..7b05592ba 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -109,4 +109,11 @@ struct ref_sorting *ref_default_sorting(void);
 /*  Function to parse --merged and --no-merged options */
 int parse_opt_merge_filter(const struct option *opt, const char *arg, int 
unset);
 
+/*
+ * Print a single ref, outside of any ref-filter. Note that the
+ * name must be a fully qualified refname.
+ */
+void pretty_print_ref(const char *name, const unsigned char *sha1,
+   const char *format);
+
 #endif /*  REF_FILTER_H  */
-- 
2.11.0

[PATCH v6 6/6] t/t7004-tag: Add --format specifier tests

[santiago]

From: Santiago Torres 

tag -v now supports --format specifiers to inspect the contents of a tag
upon verification. Add two tests to ensure this behavior is respected in
future changes.

Signed-off-by: Santiago Torres 
---
 t/t7004-tag.sh | 16 
 1 file changed, 16 insertions(+)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 07869b0c0..ba88b556b 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -874,6 +874,22 @@ test_expect_success GPG 'verifying a forged tag should 
fail' '
test_must_fail git tag -v forged-tag
 '
 
+test_expect_success 'verifying a proper tag with --format pass and format 
accordingly' '
+   cat >expect <<-\EOF
+   tagname : signed-tag
+   EOF &&
+   git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'verifying a forged tag with --format fail and format 
accordingly' '
+   cat >expect <<-\EOF
+   tagname : forged-tag
+   EOF &&
+   test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" 
>actual &&
+   test_cmp expect actual
+'
+
 # blank and empty messages for signed tags:
 
 get_tag_header empty-signed-tag $commit commit $time >expect
-- 
2.11.0

[PATCH v6 4/6] builtin/tag: add --format argument for tag -v

[santiago]

From: Lukas Puehringer 

Adding --format to git tag -v mutes the default output of the GPG
verification and instead prints the formatted tag object.
This allows callers to cross-check the tagname from refs/tags with
the tagname from the tag object header upon GPG verification.

The callback function for for_each_tag_name() didn't allow callers to
pass custom data to their callback functions. Add a new opaque pointer
to each_tag_name_fn's parameter to allow this.

Signed-off-by: Lukas Puehringer 
---
 Documentation/git-tag.txt |  2 +-
 builtin/tag.c | 38 --
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 76cfe40d9..586aaa315 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -15,7 +15,7 @@ SYNOPSIS
 'git tag' [-n[]] -l [--contains ] [--points-at ]
[--column[=] | --no-column] [--create-reflog] [--sort=]
[--format=] [--[no-]merged []] [...]
-'git tag' -v ...
+'git tag' -v [--format=] ...
 
 DESCRIPTION
 ---
diff --git a/builtin/tag.c b/builtin/tag.c
index 73df72811..b9da72761 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -24,7 +24,7 @@ static const char * const git_tag_usage[] = {
N_("git tag -d ..."),
N_("git tag -l [-n[]] [--contains ] [--points-at ]"
"\n\t\t[--format=] [--[no-]merged []] 
[...]"),
-   N_("git tag -v ..."),
+   N_("git tag -v [--format=] ..."),
NULL
 };
 
@@ -66,15 +66,17 @@ static int list_tags(struct ref_filter *filter, struct 
ref_sorting *sorting, con
 }
 
 typedef int (*each_tag_name_fn)(const char *name, const char *ref,
-   const unsigned char *sha1);
+   const unsigned char *sha1, void *cb_data);
 
-static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
+static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
+   void *cb_data)
 {
const char **p;
char ref[PATH_MAX];
int had_error = 0;
unsigned char sha1[20];
 
+
for (p = argv; *p; p++) {
if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p)
>= sizeof(ref)) {
@@ -87,14 +89,14 @@ static int for_each_tag_name(const char **argv, 
each_tag_name_fn fn)
had_error = 1;
continue;
}
-   if (fn(*p, ref, sha1))
+   if (fn(*p, ref, sha1, cb_data))
had_error = 1;
}
return had_error;
 }
 
 static int delete_tag(const char *name, const char *ref,
-   const unsigned char *sha1)
+   const unsigned char *sha1, void *cb_data)
 {
if (delete_ref(ref, sha1, 0))
return 1;
@@ -103,9 +105,22 @@ static int delete_tag(const char *name, const char *ref,
 }
 
 static int verify_tag(const char *name, const char *ref,
-   const unsigned char *sha1)
+   const unsigned char *sha1, void *cb_data)
 {
-   return gpg_verify_tag(sha1, name, GPG_VERIFY_VERBOSE);
+   int flags;
+   char *fmt_pretty = cb_data;
+   flags = GPG_VERIFY_VERBOSE;
+
+   if (fmt_pretty)
+   flags = GPG_VERIFY_OMIT_STATUS;
+
+   if (gpg_verify_tag(sha1, name, flags))
+   return -1;
+
+if (fmt_pretty)
+   pretty_print_ref(name, sha1, fmt_pretty);
+
+   return 0;
 }
 
 static int do_sign(struct strbuf *buffer)
@@ -428,9 +443,12 @@ int cmd_tag(int argc, const char **argv, const char 
*prefix)
if (filter.merge_commit)
die(_("--merged and --no-merged option are only allowed with 
-l"));
if (cmdmode == 'd')
-   return for_each_tag_name(argv, delete_tag);
-   if (cmdmode == 'v')
-   return for_each_tag_name(argv, verify_tag);
+   return for_each_tag_name(argv, delete_tag, NULL);
+   if (cmdmode == 'v') {
+   if (format)
+   verify_ref_format(format);
+   return for_each_tag_name(argv, verify_tag, format);
+   }
 
if (msg.given || msgfile) {
if (msg.given && msgfile)
-- 
2.11.0

[PATCH v6 3/6] builtin/verify-tag: add --format to verify-tag

[santiago]

From: Santiago Torres 

Callers of verify-tag may want to cross-check the tagname from refs/tags
with the tagname from the tag object header upon GPG verification. This
is to avoid tag refs that point to an incorrect object.

Add a --format parameter to git verify-tag to print the formatted tag
object header in addition to or instead of the --verbose or --raw GPG
verification output.

Signed-off-by: Santiago Torres 
---
 Documentation/git-verify-tag.txt |  2 +-
 builtin/verify-tag.c | 23 ---
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-verify-tag.txt b/Documentation/git-verify-tag.txt
index d590edceb..0b8075dad 100644
--- a/Documentation/git-verify-tag.txt
+++ b/Documentation/git-verify-tag.txt
@@ -8,7 +8,7 @@ git-verify-tag - Check the GPG signature of tags
 SYNOPSIS
 
 [verse]
-'git verify-tag' ...
+'git verify-tag' [--format=] ...
 
 DESCRIPTION
 ---
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 99f8148cf..18443bf9f 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -12,12 +12,14 @@
 #include 
 #include "parse-options.h"
 #include "gpg-interface.h"
+#include "ref-filter.h"
 
 static const char * const verify_tag_usage[] = {
-   N_("git verify-tag [-v | --verbose] ..."),
+   N_("git verify-tag [-v | --verbose] [--format=] 
..."),
NULL
 };
 
+
 static int git_verify_tag_config(const char *var, const char *value, void *cb)
 {
int status = git_gpg_config(var, value, cb);
@@ -30,9 +32,11 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
 {
int i = 1, verbose = 0, had_error = 0;
unsigned flags = 0;
+   char *fmt_pretty = NULL;
const struct option verify_tag_options[] = {
OPT__VERBOSE(, N_("print tag contents")),
OPT_BIT(0, "raw", , N_("print raw gpg status output"), 
GPG_VERIFY_RAW),
+   OPT_STRING(  0 , "format", _pretty, N_("format"), 
N_("format to use for the output")),
OPT_END()
};
 
@@ -46,13 +50,26 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
if (verbose)
flags |= GPG_VERIFY_VERBOSE;
 
+   if (fmt_pretty) {
+   verify_ref_format(fmt_pretty);
+   flags |= GPG_VERIFY_OMIT_STATUS;
+   }
+
while (i < argc) {
unsigned char sha1[20];
const char *name = argv[i++];
-   if (get_sha1(name, sha1))
+   if (get_sha1(name, sha1)) {
had_error = !!error("tag '%s' not found.", name);
-   else if (gpg_verify_tag(sha1, name, flags))
+   continue;
+   }
+
+   if (gpg_verify_tag(sha1, name, flags)) {
had_error = 1;
+   continue;
+   }
+
+   if (fmt_pretty)
+   pretty_print_ref(name, sha1, fmt_pretty);
}
return had_error;
 }
-- 
2.11.0

[PATCH v6 1/6] gpg-interface,tag: add GPG_VERIFY_OMIT_STATUS flag

[santiago]

From: Lukas Puehringer 

Functions that print git object information may require that the
gpg-interface functions be silent. Add GPG_VERIFY_OMIT_STATUS flag and
prevent print_signature_buffer from being called if flag is set.

Signed-off-by: Lukas Puehringer 
---
 gpg-interface.h | 5 +++--
 tag.c   | 5 -
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/gpg-interface.h b/gpg-interface.h
index ea68885ad..d2d4fd3a6 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -1,8 +1,9 @@
 #ifndef GPG_INTERFACE_H
 #define GPG_INTERFACE_H
 
-#define GPG_VERIFY_VERBOSE 1
-#define GPG_VERIFY_RAW 2
+#define GPG_VERIFY_VERBOSE 1
+#define GPG_VERIFY_RAW 2
+#define GPG_VERIFY_OMIT_STATUS 4
 
 struct signature_check {
char *payload;
diff --git a/tag.c b/tag.c
index d1dcd18cd..243d1fdbb 100644
--- a/tag.c
+++ b/tag.c
@@ -3,6 +3,7 @@
 #include "commit.h"
 #include "tree.h"
 #include "blob.h"
+#include "gpg-interface.h"
 
 const char *tag_type = "tag";
 
@@ -24,7 +25,9 @@ static int run_gpg_verify(const char *buf, unsigned long 
size, unsigned flags)
 
ret = check_signature(buf, payload_size, buf + payload_size,
size - payload_size, );
-   print_signature_buffer(, flags);
+
+   if (!(flags & GPG_VERIFY_OMIT_STATUS))
+   print_signature_buffer(, flags);
 
signature_check_clear();
return ret;
-- 
2.11.0

[PATCH v6 5/6] t/t7030-verify-tag: Add --format specifier tests

[santiago]

From: Santiago Torres 

Verify-tag now provides --format specifiers to inspect and ensure the
contents of the tag are proper. We add two tests to ensure this
functionality works as expected: the return value should indicate if
verification passed, and the format specifiers must be respected.

Signed-off-by: Santiago Torres 
---
 t/t7030-verify-tag.sh | 16 
 1 file changed, 16 insertions(+)

diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index 07079a41c..d62ccbb98 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -125,4 +125,20 @@ test_expect_success GPG 'verify multiple tags' '
test_cmp expect.stderr actual.stderr
 '
 
+test_expect_success 'verifying tag with --format' '
+   cat >expect <<-\EOF
+   tagname : fourth-signed
+   EOF &&
+   git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'verifying a forged tag with --format fail and format 
accordingly' '
+   cat >expect <<-\EOF
+   tagname : 7th forged-signed
+   EOF &&
+   test_must_fail git verify-tag --format="tagname : %(tag)" $(cat 
forged1.tag) >actual-forged &&
+   test_cmp expect actual-forged
+'
+
 test_done
-- 
2.11.0

[PATCH v6 0/6] Add --format to tag verification

[santiago]

From: Santiago Torres 

This is the sixth iteration of [1][2][3][4][5], and as a result of the
discussion in [5]. The main goal of this patch series is to bring
--format to git tag verification so that upper-layer tools can inspect
the content of a tag and make decisions based on it.

In this re-woll we:

* Changed the call interface so printing is done outside of verification. 

* Fixed a couple of whitespace issues and whatnot. 

Thanks,
-Santiago

[1] http://public-inbox.org/git/20170115184705.10376-1-santi...@nyu.edu/
[2] http://public-inbox.org/git/20161007210721.20437-1-santi...@nyu.edu/
[3] http://public-inbox.org/git/20160930221806.3398-1-santi...@nyu.edu/
[4] http://public-inbox.org/git/20160922185317.349-1-santi...@nyu.edu/
[5] http://public-inbox.org/git/20160926224233.32702-1-santi...@nyu.edu/
[6] http://public-inbox.org/git/20160607195608.16643-1-santi...@nyu.edu/
[7] 
http://public-inbox.org/git/20161019203546.dfqmi2czcxopg...@sigill.intra.peff.net/
[8] 
http://public-inbox.org/git/20161019203943.epjxnfci7vcqg...@sigill.intra.peff.net/

Lukas Puehringer (3):
  gpg-interface,tag: add GPG_VERIFY_OMIT_STATUS flag
  ref-filter: add function to print single ref_array_item
  builtin/tag: add --format argument for tag -v

Santiago Torres (3):
  builtin/verify-tag: add --format to verify-tag
  t/t7030-verify-tag: Add --format specifier tests
  t/t7004-tag: Add --format specifier tests

 Documentation/git-tag.txt|  2 +-
 Documentation/git-verify-tag.txt |  2 +-
 builtin/tag.c| 38 --
 builtin/verify-tag.c | 23 ---
 gpg-interface.h  |  5 +++--
 ref-filter.c | 27 +--
 ref-filter.h |  7 +++
 t/t7004-tag.sh   | 16 
 t/t7030-verify-tag.sh| 16 
 tag.c|  5 -
 10 files changed, 117 insertions(+), 24 deletions(-)

-- 
2.11.0

[PATCH 0/4] start documenting core functions

[Stefan Beller]

The two single patches[1] are turned into a series here.

[1] https://public-inbox.org/git/20170117200147.25425-1-sbel...@google.com/

Thanks,
Stefan

Stefan Beller (4):
  document index_name_pos
  remove_index_entry_at: move documentation to cache.h
  document add_[file_]to_index
  documentation: retire unfinished documentation

 Documentation/technical/api-in-core-index.txt | 21 
 cache.h   | 35 +++
 read-cache.c  |  1 -
 3 files changed, 30 insertions(+), 27 deletions(-)
 delete mode 100644 Documentation/technical/api-in-core-index.txt

-- 
2.11.0.299.g762782ba8a

[PATCH 1/4] document index_name_pos

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 cache.h | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/cache.h b/cache.h
index 1b67f078dd..270a0d0ea7 100644
--- a/cache.h
+++ b/cache.h
@@ -575,7 +575,22 @@ extern int verify_path(const char *path);
 extern int index_dir_exists(struct index_state *istate, const char *name, int 
namelen);
 extern void adjust_dirname_case(struct index_state *istate, char *name);
 extern struct cache_entry *index_file_exists(struct index_state *istate, const 
char *name, int namelen, int igncase);
+
+/*
+ * Searches for an entry defined by name and namelen in the given index.
+ * If the return value is positive (including 0) it is the position of an
+ * exact match. If the return value is negative, the negated value minus 1 is 
the
+ * position where the entry would be inserted.
+ * Example: In the current index we have the files b,d,e:
+ * index_name_pos(, "a", 1) -> -1
+ * index_name_pos(, "b", 1) ->  0
+ * index_name_pos(, "c", 1) -> -2
+ * index_name_pos(, "d", 1) ->  1
+ * index_name_pos(, "e", 1) ->  2
+ * index_name_pos(, "f", 1) -> -3
+ */
 extern int index_name_pos(const struct index_state *, const char *name, int 
namelen);
+
 #define ADD_CACHE_OK_TO_ADD 1  /* Ok to add */
 #define ADD_CACHE_OK_TO_REPLACE 2  /* Ok to replace file/directory */
 #define ADD_CACHE_SKIP_DFCHECK 4   /* Ok to skip DF conflict checks */
-- 
2.11.0.299.g762782ba8a

[PATCH 3/4] document add_[file_]to_index

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 cache.h | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index 26632065a5..acc639d6e0 100644
--- a/cache.h
+++ b/cache.h
@@ -605,13 +605,20 @@ extern int remove_index_entry_at(struct index_state *, 
int pos);
 
 extern void remove_marked_cache_entries(struct index_state *istate);
 extern int remove_file_from_index(struct index_state *, const char *path);
-#define ADD_CACHE_VERBOSE 1
-#define ADD_CACHE_PRETEND 2
-#define ADD_CACHE_IGNORE_ERRORS4
-#define ADD_CACHE_IGNORE_REMOVAL 8
-#define ADD_CACHE_INTENT 16
+
+#define ADD_CACHE_VERBOSE 1/* verbose */
+#define ADD_CACHE_PRETEND 2/* dry run */
+#define ADD_CACHE_IGNORE_ERRORS 4  /* ignore errors */
+#define ADD_CACHE_IGNORE_REMOVAL 8 /* do not remove files from index */
+#define ADD_CACHE_INTENT 16/* intend to add later; stage empty 
file */
+/*
+ * Adds the given path the index, respecting the repsitory configuration, e.g.
+ * in case insensitive file systems, the path is normalized.
+ */
 extern int add_to_index(struct index_state *, const char *path, struct stat *, 
int flags);
+/* stat the file then call add_to_index */
 extern int add_file_to_index(struct index_state *, const char *path, int 
flags);
+
 extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned 
char *sha1, const char *path, int stage, unsigned int refresh_options);
 extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, 
char flip);
 extern int ce_same_name(const struct cache_entry *a, const struct cache_entry 
*b);
-- 
2.11.0.299.g762782ba8a

[PATCH 2/4] remove_index_entry_at: move documentation to cache.h

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 cache.h  | 3 +++
 read-cache.c | 1 -
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index 270a0d0ea7..26632065a5 100644
--- a/cache.h
+++ b/cache.h
@@ -599,7 +599,10 @@ extern int index_name_pos(const struct index_state *, 
const char *name, int name
 #define ADD_CACHE_KEEP_CACHE_TREE 32   /* Do not invalidate cache-tree */
 extern int add_index_entry(struct index_state *, struct cache_entry *ce, int 
option);
 extern void rename_index_entry_at(struct index_state *, int pos, const char 
*new_name);
+
+/* Remove entry, return 1 if there are more entries after pos. */
 extern int remove_index_entry_at(struct index_state *, int pos);
+
 extern void remove_marked_cache_entries(struct index_state *istate);
 extern int remove_file_from_index(struct index_state *, const char *path);
 #define ADD_CACHE_VERBOSE 1
diff --git a/read-cache.c b/read-cache.c
index 2eca639cce..63a414cdb5 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -503,7 +503,6 @@ int index_name_pos(const struct index_state *istate, const 
char *name, int namel
return index_name_stage_pos(istate, name, namelen, 0);
 }
 
-/* Remove entry, return true if there are more entries to go.. */
 int remove_index_entry_at(struct index_state *istate, int pos)
 {
struct cache_entry *ce = istate->cache[pos];
-- 
2.11.0.299.g762782ba8a

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Jacob Keller]

On Fri, Jan 13, 2017 at 1:31 PM, Johannes Sixt  wrote:
> Am 13.01.2017 um 07:57 schrieb Jacob Keller:
>>
>> On Thu, Jan 12, 2017 at 10:43 PM, Johannes Sixt  wrote:
>>>
>>>  When you write
>>>
>>>   git log --branches --exclude=origin/* --remotes
>>>
>>> --exclude=origin/* applies only to --remotes, but not to --branches.
>>
>>
>> Well for describe I don't think the order matters.
>
>
> That is certainly true today. But I would value consistency more. We would
> lose it if some time in the future 'describe' accepts --branches and
> --remotes in addition to --tags and --all.
>
> -- Hannes
>

I am not sure that the interface for git-log and git-describe are
similar enough to make this distinction work. --match already seems to
imply that it only works on refs in refs/tags, as it says it considers
globs matching excluding the "refs/tags" prefix.

In git-describe, we already have "--tags" and "--all" but they are
mutually exclusive. We don't support using more than one at once, and
I'm not really convinced that describe will ever support more than one
at a time. Additionally, match already doesn't respect order.

Thanks,
Jake

Re: [RFH - Tcl/Tk] use of procedure before declaration?

[Philip Oakley]

From: "Johannes Schindelin" 

Hi Philip,

On Mon, 16 Jan 2017, Philip Oakley wrote:


In
https://github.com/git/git/blob/master/git-gui/lib/choose_repository.tcl#L242
the procedure `_unset_recentrepo` is called, however the procedure isn't
declared until line 248. My reading of the various Tcl tutorials suggest
(but not explictly) that this isn't the right way.


Indeed, calling a procedure before it is declared sounds incorrect.

Since documentation can be treacherous, let's just test it. With a `tclsh`
whose `$tcl_version` variable claims that this is version 8.6, this
script:

```tcl
hello Philip

proc hello {arg} {
   puts "Hi, $arg"
}
```

... yields the error message:

invalid command name "hello"
while executing
"hello Philip"

... while this script:

```tcl
proc hello {arg} {
   puts "Hi, $arg"
}

hello Philip
```

... prints the expected "Hi, Philip".

Having said that, in the code to which you linked, the procedure is not
actually called before it is declared, as the call is inside another
procedure.

Indeed, the entire file declares one object-oriented class, so no code
gets executed in that file:

https://github.com/git/git/blob/d7dffce1c/git-gui/lib/choose_repository.tcl#L4

(I guess proper indentation would make it easier to understand that this
file is defining a class, not executing anything yet).

And it is perfectly legitimate to use not-yet-declared procedures in other
procedures, otherwise recursion would not work.

Should 3c6a287 ("git-gui: Keep repo_config(gui.recentrepos) and 
.gitconfig

in sync", 2010-01-23) have declared `proc _unset_recentrepo {p}` before
`proc _get_recentrepos {}` ?


Given the findings above, I believe that the patch is actually correct.

Ciao,
Dscho

Thanks for the clarification. I'll update the old patch series and see if we 
can get this fixed.


Philip 

Re: [RFC] Add support for downloading blobs on demand

[Stefan Beller]

On Tue, Jan 17, 2017 at 2:05 PM, Martin Fick  wrote:
> On Tuesday, January 17, 2017 04:50:13 PM Ben Peart wrote:
>> While large files can be a real problem, our biggest issue
>> today is having a lot (millions!) of source files when
>> any individual developer only needs a small percentage of
>> them.  Git with 3+ million local files just doesn't
>> perform well.
>
> Honestly, this sounds like a problem better dealt with by
> using git subtree or git submodules, have you considered
> that?
>
> -Martin
>

I cannot speak for subtrees as I have very little knowledge on them.
But there you also have the problem that *someone* has to have a
whole tree? (e.g. the build bot)

submodules however comes with a couple of things attached, both
positive as well as negative points:

* it offers ACLs along the way. ($user may not be allowed to
  clone all submodules, but only those related to the work)
* The conceptual understanding of git just got a lot harder.
  ("Yo dawg, I heard you like git, so I put git repos inside
  other git repos"), it is not easy to come up with reasonable
  defaults for all usecases, so the everyday user still has to
  have some understanding of submodules.
* typical cheap in-tree operations may become very expensive:
  -> moving a file from one location to another (in another
 submodule) adds overhead, no rename detection.
* We are actively working on submodules, so there is
  some momentum going already.
* our experiments with Android show that e.g. fetching (even
  if you have all of Android) becomes a lot faster for everyday
  usage as only a few repositories change each day). This
  comparision was against the repo tool, that we currently
  use for Android. I do not know how it would compare against
  single repo Git, as having such a large repository seemed
  complicated.
* the support for submodules in Git is already there, though
  not polished. The positive side is to have already a good base,
  the negative side is to have support current use cases.

Stefan

Re: [PATCH 2/2] Be more careful when determining whether a remote was configured

[Junio C Hamano]

Jeff King  writes:

>> Let's fix this by telling Git that a remote is not configured unless any
>> fetch/push URL or refspec is configured explicitly.
>
> I notice here that setting a refspec _does_ define a remote. Is there a
> reason you drew the line there, and not at, say, whether it has a URL?

"Not configured unless any URL or refspec is configured" means that
if URL is there, even if there is no refspec, then it is a remote
definition, right?  

But I think "what does it mean to define a remote" is a question
that you are asking, and that is not necessary to answer within the
scope of this topic.

I do agree that honoring .prune is nonsense unless refspecs are
defined, but the question "does it make sense to allow prune?"  is
different from "is it configured?".  Your "you can set .proxy and
other useful things, it is just .prune does not make sense" is a
quite appropriate statement to illustrate the difference.

Perhaps instead of adding "is it configured?" flag that is too
broadly named and has too narrow meaning, would it make more sense
to introduce "int can_prune(struct remote *remote)" that looks at
the remote refspecs?

Re: [RFC] Add support for downloading blobs on demand

[Martin Fick]

On Tuesday, January 17, 2017 04:50:13 PM Ben Peart wrote:
> While large files can be a real problem, our biggest issue
> today is having a lot (millions!) of source files when
> any individual developer only needs a small percentage of
> them.  Git with 3+ million local files just doesn't
> perform well.

Honestly, this sounds like a problem better dealt with by 
using git subtree or git submodules, have you considered 
that?

-Martin

-- 
The Qualcomm Innovation Center, Inc. is a member of Code 
Aurora Forum, hosted by The Linux Foundation

RE: [RFC] Add support for downloading blobs on demand

[Ben Peart]

Thanks for the encouragement, support, and good ideas to look into.

Ben

> -Original Message-
> From: Shawn Pearce [mailto:spea...@spearce.org]
> Sent: Friday, January 13, 2017 4:07 PM
> To: Ben Peart 
> Cc: git ; benpe...@microsoft.com
> Subject: Re: [RFC] Add support for downloading blobs on demand
> 
> On Fri, Jan 13, 2017 at 7:52 AM, Ben Peart  wrote:
> >
> > Goal
> > 
> >
> > To be able to better handle repos with many files that any individual
> > developer doesn’t need it would be nice if clone/fetch only brought
> > down those files that were actually needed.
> >
> > To enable that, we are proposing adding a flag to clone/fetch that
> > will instruct the server to limit the objects it sends to commits and
> > trees and to not send any blobs.
> >
> > When git performs an operation that requires a blob that isn’t
> > currently available locally, it will download the missing blob and add
> > it to the local object store.
> 
> Interesting. This is also an area I want to work on with my team at $DAY_JOB.
> Repositories are growing along multiple dimensions, and developers or
> editors don't always need all blobs for all time available locally to 
> successfully
> perform their work.
> 
> > Design
> > ~~
> >
> > Clone and fetch will pass a “--lazy-clone” flag (open to a better name
> > here) similar to “--depth” that instructs the server to only return
> > commits and trees and to ignore blobs.
> 
> My group at $DAY_JOB hasn't talked about it yet, but I want to add a
> protocol capability that lets clone/fetch ask only for blobs smaller than a
> specified byte count. This could be set to a reasonable text file size (e.g. 
> <= 5
> MiB) to predominately download only source files and text documentation,
> omitting larger binaries.
> 
> If the limit was set to 0, its the same as your idea to ignore all blobs.
> 

This is an interesting idea that may be an easier way to help mitigate 
the cost of very large files.  While our primary issue today is the 
sheer number of files, I'm sure at some point we'll run into issues with 
file size as well.  

> > Later during git operations like checkout, when a blob cannot be found
> > after checking all the regular places (loose, pack, alternates, etc),
> > git will download the missing object and place it into the local
> > object store (currently as a loose object) then resume the operation.
> 
> Right. I'd like to have this object retrieval be inside the native Git wire
> protocol, reusing the remote configuration and authentication setup. That
> requires expanding the server side of the protocol implementation slightly
> allowing any reachable object to be retrieved by SHA-1 alone. Bitmap indexes
> can significantly reduce the computational complexity for the server.
> 

Agree.  

> > To prevent git from accidentally downloading all missing blobs, some
> > git operations are updated to be aware of the potential for missing blobs.
> > The most obvious being check_connected which will return success as if
> > everything in the requested commits is available locally.
> 
> This ... sounds risky for the developer, as the repository may be corrupt due
> to a missing object, and the user cannot determine it.
> 
> Would it be reasonable for the server to return a list of SHA-1s it knows
> should exist, but has omitted due to the blob threshold (above), and the
> local repository store this in a binary searchable file?
> During connectivity checking its assumed OK if an object is not present in the
> object store, but is listed in this omitted objects file.
> 

Corrupt repos due to missing blobs must be pretty rare as I've never 
seen anyone report that error but for this and other reasons (see Peff's 
suggestion on how to minimize downloading unnecessary blobs) having this 
data could be valuable.  I'll add it to the list of things to look into.

> > To minimize the impact on the server, the existing dumb HTTP protocol
> > endpoint “objects/” can be used to retrieve the individual
> > missing blobs when needed.
> 
> I'd prefer this to be in the native wire protocol, where the objects are in 
> pack
> format (which unfortunately differs from loose format). I assume servers
> would combine many objects into pack files, potentially isolating large
> uncompressable binaries into their own packs, stored separately from
> commits/trees/small-text-blobs.
> 
> I get the value of this being in HTTP, where HTTP caching inside proxies can
> be leveraged to reduce master server load. I wonder if the native wire
> protocol could be taught to use a variation of an HTTP GET that includes the
> object SHA-1 in the URL line, to retrieve a one-object pack file.
> 

You make a good point. I don't think the benefit of hitting this 
"existing" end point outweighs the many drawbacks.  Adding the ability 
to retrieve an individual blob via the native wire protocol seems a 
better plan.

> > Performance considerations

RE: [RFC] Add support for downloading blobs on demand

[Ben Peart]

Thanks for the thoughtful response.  No need to appologize for the 
length, it's a tough problem to solve so I don't expect it to be handled 
with a single, short email. :)

> -Original Message-
> From: Jeff King [mailto:p...@peff.net]
> Sent: Tuesday, January 17, 2017 1:43 PM
> To: Ben Peart 
> Cc: git@vger.kernel.org; Ben Peart 
> Subject: Re: [RFC] Add support for downloading blobs on demand
> 
> This is an issue I've thought a lot about. So apologies in advance that this
> response turned out a bit long. :)
> 
> On Fri, Jan 13, 2017 at 10:52:53AM -0500, Ben Peart wrote:
> 
> > Design
> > ~~
> >
> > Clone and fetch will pass a  --lazy-clone  flag (open to a better name
> > here) similar to  --depth  that instructs the server to only return
> > commits and trees and to ignore blobs.
> >
> > Later during git operations like checkout, when a blob cannot be found
> > after checking all the regular places (loose, pack, alternates, etc),
> > git will download the missing object and place it into the local
> > object store (currently as a loose object) then resume the operation.
> 
> Have you looked at the "external odb" patches I wrote a while ago, and
> which Christian has been trying to resurrect?
> 
>   https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpublic-
> inbox.org%2Fgit%2F20161130210420.15982-1-
> chriscool%40tuxfamily.org%2F=02%7C01%7CBen.Peart%40microsoft.c
> om%7C9596d3bf32564f123e0c08d43f08a9e1%7C72f988bf86f141af91ab2d7c
> d011db47%7C1%7C0%7C636202753822020527=a6%2BGOAQoRhjFoxS
> vftY8JZAVUssmrXuDZ9OBy3xqNZk%3D=0
> 
> This is a similar approach, though I pushed the policy for "how do you get the
> objects" out into an external script. One advantage there is that large 
> objects
> could easily be fetched from another source entirely (e.g., S3 or equivalent)
> rather than the repo itself.
> 
> The downside is that it makes things more complicated, because a push or a
> fetch now involves three parties (server, client, and the alternate object
> store). So questions like "do I have all the objects I need" are hard to 
> reason
> about.
> 
> If you assume that there's going to be _some_ central Git repo which has all
> of the objects, you might as well fetch from there (and do it over normal git
> protocols). And that simplifies things a bit, at the cost of being less 
> flexible.
> 

We looked quite a bit at the external odb patches, as well as lfs and 
even using alternates.  They all share a common downside that you must 
maintain a separate service that contains _some_ of the files.  These 
files must also be versioned, replicated, backed up and the service 
itself scaled out to handle the load.  As you mentioned, having multiple 
services involved increases flexability but it also increases the 
complexity and decreases the reliability of the overall version control 
service.  

For operational simplicity, we opted to go with a design that uses a 
single, central git repo which has _all_ the objects and to focus on 
enhancing it to handle large numbers of files efficiently.  This allows 
us to focus our efforts on a great git service and to avoid having to 
build out these other services.

> > To prevent git from accidentally downloading all missing blobs, some
> > git operations are updated to be aware of the potential for missing blobs.
> > The most obvious being check_connected which will return success as if
> > everything in the requested commits is available locally.
> 
> Actually, Git is pretty good about trying not to access blobs when it doesn't
> need to. The important thing is that you know enough about the blobs to
> fulfill has_sha1_file() and sha1_object_info() requests without actually
> fetching the data.
> 
> So the client definitely needs to have some list of which objects exist, and
> which it _could_ get if it needed to.
> 
> The one place you'd probably want to tweak things is in the diff code, as a
> single "git log -Sfoo" would fault in all of the blobs.
> 

It is an interesting idea to explore how we could be smarter about 
preventing blobs from faulting in if we had enough info to fulfill 
has_sha1_file() and sha1_object_info().  Given we also heavily prune the 
working directory using sparse-checkout, this hasn't been our top focus 
but it is certainly something worth looking into.

> > To minimize the impact on the server, the existing dumb HTTP protocol
> > endpoint  objects/  can be used to retrieve the individual
> > missing blobs when needed.
> 
> This is going to behave badly on well-packed repositories, because there isn't
> a good way to fetch a single object. The best case (which is not implemented
> at all in Git) is that you grab the pack .idx, then grab "slices" of the pack
> corresponding to specific objects, including hunting down delta bases.
> 
> But then next time the server repacks, you have to throw away your .idx file.
> And those can be big. The .idx for linux.git is 135MB. 

Re: [PATCH] document index_name_pos

[Stefan Beller]

On Tue, Jan 17, 2017 at 1:43 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Signed-off-by: Stefan Beller 
>> ---
>>
>>> These placeholders are meant to encourage those people who dove into
>>> the code to update it, so from that point of view, I think removing
>>> it is backwards.
>>
>> Yes, I am currently understanding and writing up documentation for
>> index_name_pos. If I recall the latest discussion where we want to have
>> documentation, I think a quorum favored documentation in the header itself,
>> c.f. strbuf.h, string-list.h for the most desired state. (Although we do have
>> Documentation/technical/api-string-list.txt as well ...)
>>
>> So maybe starting like this?
>
> That is very good.  Let's drop that file from Documentation/technical
> and do it like this (meaning, take both patches from you).
>
> Thanks.

This patch is incorrect, as we need to do s/b -> -1/b -> -2/.
(Wrong documentation is the worst)

Also we'd probably want to see more of the functions documented.
I'll see if I can extend this into a series documenting more functions.

Thanks,
Stefan

Re: [PATCH v5 3/3] Retire the scripted difftool

[Junio C Hamano]

Johannes Schindelin  writes:

> It served its purpose, but now we have a builtin difftool. Time for the
> Perl script to enjoy Florida.
>
> Signed-off-by: Johannes Schindelin 
> ---

The endgame makes a lot of sense.  Both in the cover letter and in
the previous patch you talk about having both in the released
version, so do you want this step to proceed slower than the other
two?  I.e. merge all three to 'next' but graduate only the first two
to 'master' and after a while make this last step graduate?

Re: [PATCH 2/2] Be more careful when determining whether a remote was configured

[Jeff King]

On Tue, Jan 17, 2017 at 10:19:24PM +0100, Johannes Schindelin wrote:

> One of the really nice features of the ~/.gitconfig file is that users
> can override defaults by their own preferred settings for all of their
> repositories.
> 
> One such default that some users like to override is whether the
> "origin" remote gets auto-pruned or not. The user would simply call
> 
>   git config --global remote.origin.prune true
> 
> and from now on all "origin" remotes would be pruned automatically when
> fetching into the local repository.
> 
> There is just one catch: now Git thinks that the "origin" remote is
> configured, as it does not discern between having a remote whose
> fetch (and/or push) URL and refspec is set, and merely having
> preemptively-configured, global flags for specific remotes.
> 
> Let's fix this by telling Git that a remote is not configured unless any
> fetch/push URL or refspect is configured explicitly.

Hmm. Old versions of GitHub for Windows used to set fetch refspecs in
the system gitconfig, for a similar purpose to what you want to do with
remote.origin.prune.

I notice here that setting a refspec _does_ define a remote. Is there a
reason you drew the line there, and not at, say, whether it has a URL?

-Peff

Re: [PATCH 0/2] Fix remote_is_configured()

[Jeff King]

On Tue, Jan 17, 2017 at 10:18:58PM +0100, Johannes Schindelin wrote:

> A surprising behavior triggered the bug report in
> https://github.com/git-for-windows/git/issues/888: the mere existence of
> the config setting "remote.origin.prune" (in this instance, configured
> via ~/.gitconfig so that it applies to all repositories) fooled `git
> remote rename  ` into believing that the  remote
> is already there.
> 
> This patch pair demonstrates the problem, and then fixes it (along with
> potential similar problems, such as setting an HTTP proxy for remotes of
> a given name via ~/.gitconfig).

I thought it was intentional that any config "created" the remote, even
without a url field. E.g., you can set:

  [remote "https://example.com/foo/git;]
  proxy = localhost:1234

and then a bare "git fetch https://example.com/foo/git; will respect it.

I admit that "prune" is probably useless without a fetch refspec,
though.

Note that I don't disagree that the rule "it's not a remote unless it
has a url" would be a lot saner. But I have a feeling you may be
breaking some existing setups.

I grepped around in the list but I couldn't find any relevant
discussion. So maybe I just dreamed it.

-Peff

Re: [PATCH] document index_name_pos

[Junio C Hamano]

Stefan Beller  writes:

> Signed-off-by: Stefan Beller 
> ---
>
>> These placeholders are meant to encourage those people who dove into
>> the code to update it, so from that point of view, I think removing
>> it is backwards.
>
> Yes, I am currently understanding and writing up documentation for
> index_name_pos. If I recall the latest discussion where we want to have
> documentation, I think a quorum favored documentation in the header itself,
> c.f. strbuf.h, string-list.h for the most desired state. (Although we do have
> Documentation/technical/api-string-list.txt as well ...)
>
> So maybe starting like this?

That is very good.  Let's drop that file from Documentation/technical
and do it like this (meaning, take both patches from you).

Thanks.


>
> Thanks,
> Stefan
>
>  cache.h | 13 +
>  1 file changed, 13 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index 1b67f078dd..e168e4e073 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -575,7 +575,20 @@ extern int verify_path(const char *path);
>  extern int index_dir_exists(struct index_state *istate, const char *name, 
> int namelen);
>  extern void adjust_dirname_case(struct index_state *istate, char *name);
>  extern struct cache_entry *index_file_exists(struct index_state *istate, 
> const char *name, int namelen, int igncase);
> +
> +/*
> + * Searches for an entry defined by name and namelen in the given index.
> + * If the return value is positive (including 0) it is the position of an
> + * exact match. If the return value is negative, the negated value minus 1 
> is the
> + * position where the entry would be inserted.
> + * Example: In the current index we have the files a,c,d:
> + * index_name_pos(, "a", 1) ->  0
> + * index_name_pos(, "b", 1) -> -1
> + * index_name_pos(, "c", 1) ->  1
> + * index_name_pos(, "d", 1) ->  2
> + */
>  extern int index_name_pos(const struct index_state *, const char *name, int 
> namelen);
> +
>  #define ADD_CACHE_OK_TO_ADD 1/* Ok to add */
>  #define ADD_CACHE_OK_TO_REPLACE 2/* Ok to replace file/directory */
>  #define ADD_CACHE_SKIP_DFCHECK 4 /* Ok to skip DF conflict checks */

Re: [PATCH] CodingGuidelines: clarify multi-line brace style

[Junio C Hamano]

Jeff King  writes:

> Yeah. I obviously was adapting the original text, and I think I left too
> much of the wishy-washy-ness in. As the point of the patch is to avoid
> that, let's take your suggestion. A re-rolled patch is below.
>
> Now the patch is at least self-consistent. The bigger question remains
> of: do we want to dictate these rules, or did we prefer the vague
> version?

I would say no to make all of them "rules to be dictated".  It is OK
to leave some things "gray".  

But obviously others can vote differently ;-)

> I _thought_ the vague rules were doing fine, but this whole discussion
> has made me think otherwise. And I'd just as soon not ever have to
> repeat it. ;-/
>
> OTOH, I really do not want to review a bunch of patches that do nothing
> but change brace style (and I am sure there is a mix of styles already
> in the code base).

Yes, exactly, that is why the last sentence is in there below.

>>  When the project says it does not have strong preference
>>  among multiple choices, you are welcome to write your new
>>  code using any of them, as long as you are consistent with
>>  surrounding code.  Do not change style of existing code only
>>  to flip among these styles, though.

> The existing document says:
>
> Make your code readable and sensible, and don't try to be clever.
> 
> As for more concrete guidelines, just imitate the existing code
> (this is a good guideline, no matter which project you are
> contributing to). It is always preferable to match the _local_
> convention. New code added to Git suite is expected to match
> the overall style of existing code. Modifications to existing
> code is expected to match the style the surrounding code already
> uses (even if it doesn't match the overall style of existing code).
> 
> But if you must have a list of rules, here they are.
>
> I guess that is the place to expound on how to interpret the rules. I
> dunno. Some of the individual rules that go into "gray areas" already
> spell out the "surrounding code" rule.


We are not saying one important thing and that was what invited
useless arguing this time: For "gray" things, the choice is yours in
your new code, but do not churn existing code for "gray" guidelines.

If we made _that_ clear as a rule we dictate, hopefully we'd have
less chance to see "a bunch of patches that do nothing but change
brace style", I would think.

> -- >8 --
> Subject: [PATCH] CodingGuidelines: clarify multi-line brace style
>
> There are some "gray areas" around when to omit braces from
> a conditional or loop body. Since that seems to have
> resulted in some arguments, let's be a little more clear
> about our preferred style.
>
> Signed-off-by: Jeff King 
> ---
>  Documentation/CodingGuidelines | 37 -
>  1 file changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 4cd95da6b..a4191aa38 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -206,11 +206,38 @@ For C programs:
>   x = 1;
>   }
>  
> -   is frowned upon.  A gray area is when the statement extends
> -   over a few lines, and/or you have a lengthy comment atop of
> -   it.  Also, like in the Linux kernel, if there is a long list
> -   of "else if" statements, it can make sense to add braces to
> -   single line blocks.
> +   is frowned upon. But there are a few exceptions:
> +
> + - When the statement extends over a few lines (e.g., a while loop
> +   with an embedded conditional, or a comment). E.g.:
> +
> + while (foo) {
> + if (x)
> + one();
> + else
> + two();
> + }
> +
> + if (foo) {
> + /*
> +  * This one requires some explanation,
> +  * so we're better off with braces to make
> +  * it obvious that the indentation is correct.
> +  */
> + doit();
> + }
> +
> + - When there are multiple arms to a conditional and some of them
> +   require braces, enclose even a single line block in braces for
> +   consistency. E.g.:
> +
> + if (foo) {
> + doit();
> + } else {
> + one();
> + two();
> + three();
> + }
>  
>   - We try to avoid assignments in the condition of an "if" statement.

Re: [PATCH v5 0/3] Turn the difftool into a builtin

[Junio C Hamano]

Johannes Schindelin  writes:

> - replaced the cross-validation with the Perl script by a patch that
>   retires the Perl script instead.

Yup.  That makes things a lot simpler.  While we try to be careful
during major rewrite, we usually do not go extra careful to leave
two competing implementations in-tree.

Will replace js/difftool-builtin topic.  Thanks.

Re: [PATCH 3/6] fsck: prepare dummy objects for --connectivity-check

[Jeff King]

On Tue, Jan 17, 2017 at 01:15:57PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > +static void mark_object_for_connectivity(const unsigned char *sha1)
> > +{
> > +   struct object *obj = lookup_object(sha1);
> > +
> > +   /*
> > +* Setting the object type here isn't strictly necessary here for a
> > +* connectivity check.
> 
> Drop one of these "here"s?

Yeah.

> > The cmd_fsck() part of the diff is pretty nasty without
> > "-b".
> 
> True.  I also wonder if swapping the if/else arms make the end
> result and the patch easier to read. i.e.
> 
> + if (connectivity_only) {
> + mark loose for connectivity;
> + mark packed for connectivity;
> + } else {
>   ... existing code comes here reindented ...
>   }
> 
> But the patch makes sense.

Yeah. It doesn't help the diff, but the end result is more readable. And
the conditional lacks a negation, which is nice. Will change.

> > +test_expect_success 'fsck --connectivity-only with explicit head' '
> > +   (
> > +   cd connectivity-only &&
> > +   test_must_fail git fsck --connectivity-only $tree
> > +   )
> > +'
> 
> Most of the earlier "tree=..." assignments are done in subshells,
> and it is not clear which tree this refers to.  Is this the one that
> was written in 'rev-list --verify-objects with bad sha1' that has
> been removed in its when-finished handler?

Doh. It was meant to be the one from the earlier --connectivity-only
check. But that is doubly silly, even, because the tree variable in that
case is holding the filename, not the sha1. The right thing is to call
rev-parse again, but of course that doesn't work because the repo has
been corrupted. :-/

Here's a re-roll with the two changes above (and the notice thing you
mentioned elsewhere), plus a test that actually checks the right thing
(fails before this commit and passes after).

If everything else looks good, you can queue it along with the rest.
Otherwise if I need a re-roll, it will be in the next version.

-- >8 --
Subject: fsck: prepare dummy objects for --connectivity-check

Normally fsck makes a pass over all objects to check their
integrity, and then follows up with a reachability check to
make sure we have all of the referenced objects (and to know
which ones are dangling). The latter checks for the HAS_OBJ
flag in obj->flags to see if we found the object in the
first pass.

Commit 02976bf85 (fsck: introduce `git fsck --connectivity-only`,
2015-06-22) taught fsck to skip the initial pass, and to
fallback to has_sha1_file() instead of the HAS_OBJ check.

However, it converted only one HAS_OBJ check to use
has_sha1_file(). But there are many other places in
builtin/fsck.c that assume that the flag is set (or that
lookup_object() will return an object at all). This leads to
several bugs with --connectivity-only:

  1. mark_object() will not queue objects for examination,
 so recursively following links from commits to trees,
 etc, did nothing. I.e., we were checking the
 reachability of hardly anything at all.

  2. When a set of heads is given on the command-line, we
 use lookup_object() to see if they exist. But without
 the initial pass, we assume nothing exists.

  3. When loading reflog entries, we do a similar
 lookup_object() check, and complain that the reflog is
 broken if the object doesn't exist in our hash.

So in short, --connectivity-only is broken pretty badly, and
will claim that your repository is fine when it's not.
Presumably nobody noticed for a few reasons.

One is that the embedded test does not actually test the
recursive nature of the reachability check. All of the
missing objects are still in the index, and we directly
check items from the index. This patch modifies the test to
delete the index, which shows off breakage (1).

Another is that --connectivity-only just skips the initial
pass for loose objects. So on a real repository, the packed
objects were still checked correctly. But on the flipside,
it means that "git fsck --connectivity-only" still checks
the sha1 of all of the packed objects, nullifying its
original purpose of being a faster git-fsck.

And of course the final problem is that the bug only shows
up when there _is_ corruption, which is rare. So anybody
running "git fsck --connectivity-only" proactively would
assume it was being thorough, when it was not.

One possibility for fixing this is to find all of the spots
that rely on HAS_OBJ and tweak them for the connectivity-only
case. But besides the risk that we might miss a spot (and I
found three already, corresponding to the three bugs above),
there are other parts of fsck that _can't_ work without a
full list of objects. E.g., the list of dangling objects.

Instead, let's make the connectivity-only case look more
like the normal case. Rather than skip the initial pass
completely, we'll do an abbreviated one that sets up the
HAS_OBJ flag for each object, without 

Re: [PATCH 3/6] fsck: prepare dummy objects for --connectivity-check

[Junio C Hamano]

Jeff King  writes:

> +static void mark_object_for_connectivity(const unsigned char *sha1)
> +{
> + struct object *obj = lookup_object(sha1);
> +
> + /*
> +  * Setting the object type here isn't strictly necessary here for a
> +  * connectivity check.

Drop one of these "here"s?


> The cmd_fsck() part of the diff is pretty nasty without
> "-b".

True.  I also wonder if swapping the if/else arms make the end
result and the patch easier to read. i.e.

+   if (connectivity_only) {
+   mark loose for connectivity;
+   mark packed for connectivity;
+   } else {
... existing code comes here reindented ...
}

But the patch makes sense.

> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> index e88ec7747..c1b2dda33 100755
> --- a/t/t1450-fsck.sh
> +++ b/t/t1450-fsck.sh
> @@ -523,9 +523,21 @@ test_expect_success 'fsck --connectivity-only' '
>   touch empty &&
>   git add empty &&
>   test_commit empty &&
> +
> + # Drop the index now; we want to be sure that we
> + # recursively notice that we notice the broken objects
> + # because they are reachable from refs, not because
> + # they are in the index.
> + rm -f .git/index &&
> +
> + # corrupt the blob, but in a way that we can still identify
> + # its type. That lets us see that --connectivity-only is
> + # not actually looking at the contents, but leaves it
> + # free to examine the type if it chooses.
>   empty=.git/objects/e6/9de29bb2d1d6434b8b29ae775ad8c2e48c5391 &&
> - rm -f $empty &&
> - echo invalid >$empty &&
> + blob=$(echo unrelated | git hash-object -w --stdin) &&
> + mv $(sha1_file $blob) $empty &&
> +
>   test_must_fail git fsck --strict &&
>   git fsck --strict --connectivity-only &&
>   tree=$(git rev-parse HEAD:) &&
> @@ -537,6 +549,13 @@ test_expect_success 'fsck --connectivity-only' '
>   )
>  '
>  
> +test_expect_success 'fsck --connectivity-only with explicit head' '
> + (
> + cd connectivity-only &&
> + test_must_fail git fsck --connectivity-only $tree
> + )
> +'

Most of the earlier "tree=..." assignments are done in subshells,
and it is not clear which tree this refers to.  Is this the one that
was written in 'rev-list --verify-objects with bad sha1' that has
been removed in its when-finished handler?

>  remove_loose_object () {
>   sha1="$(git rev-parse "$1")" &&
>   remainder=${sha1#??} &&

[PATCH 0/2] Fix remote_is_configured()

[Johannes Schindelin]

A surprising behavior triggered the bug report in
https://github.com/git-for-windows/git/issues/888: the mere existence of
the config setting "remote.origin.prune" (in this instance, configured
via ~/.gitconfig so that it applies to all repositories) fooled `git
remote rename  ` into believing that the  remote
is already there.

This patch pair demonstrates the problem, and then fixes it (along with
potential similar problems, such as setting an HTTP proxy for remotes of
a given name via ~/.gitconfig).


Johannes Schindelin (2):
  remote rename: demonstrate a bogus "remote exists" bug
  Be more careful when determining whether a remote was configured

 remote.c  | 9 -
 remote.h  | 2 +-
 t/t5505-remote.sh | 9 +
 3 files changed, 18 insertions(+), 2 deletions(-)


base-commit: d7dffce1cebde29a0c4b309a79e4345450bf352a
Published-As: https://github.com/dscho/git/releases/tag/rename-remote-v1
Fetch-It-Via: git fetch https://github.com/dscho/git rename-remote-v1

-- 
2.11.0.windows.3

[PATCH 2/2] Be more careful when determining whether a remote was configured

[Johannes Schindelin]

One of the really nice features of the ~/.gitconfig file is that users
can override defaults by their own preferred settings for all of their
repositories.

One such default that some users like to override is whether the
"origin" remote gets auto-pruned or not. The user would simply call

git config --global remote.origin.prune true

and from now on all "origin" remotes would be pruned automatically when
fetching into the local repository.

There is just one catch: now Git thinks that the "origin" remote is
configured, as it does not discern between having a remote whose
fetch (and/or push) URL and refspec is set, and merely having
preemptively-configured, global flags for specific remotes.

Let's fix this by telling Git that a remote is not configured unless any
fetch/push URL or refspect is configured explicitly.

As a special exception, we deem a remote configured also when *only* the
"vcs" setting is configured. The commit a31eeae27f (remote: use
remote_is_configured() for add and rename, 2016-02-16) specifically
extended our test suite to verify this, so it is safe to assume that there
has been at least one user with a legitimate use case for this.

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

Signed-off-by: Johannes Schindelin 
---
 remote.c  | 9 -
 remote.h  | 2 +-
 t/t5505-remote.sh | 2 +-
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/remote.c b/remote.c
index ad6c5424ed..298f2f93fa 100644
--- a/remote.c
+++ b/remote.c
@@ -255,6 +255,7 @@ static void read_remotes_file(struct remote *remote)
 
if (!f)
return;
+   remote->configured = 1;
remote->origin = REMOTE_REMOTES;
while (strbuf_getline(, f) != EOF) {
const char *v;
@@ -289,6 +290,7 @@ static void read_branches_file(struct remote *remote)
return;
}
 
+   remote->configured = 1;
remote->origin = REMOTE_BRANCHES;
 
/*
@@ -384,21 +386,25 @@ static int handle_config(const char *key, const char 
*value, void *cb)
if (git_config_string(, key, value))
return -1;
add_url(remote, v);
+   remote->configured = 1;
} else if (!strcmp(subkey, "pushurl")) {
const char *v;
if (git_config_string(, key, value))
return -1;
add_pushurl(remote, v);
+   remote->configured = 1;
} else if (!strcmp(subkey, "push")) {
const char *v;
if (git_config_string(, key, value))
return -1;
add_push_refspec(remote, v);
+   remote->configured = 1;
} else if (!strcmp(subkey, "fetch")) {
const char *v;
if (git_config_string(, key, value))
return -1;
add_fetch_refspec(remote, v);
+   remote->configured = 1;
} else if (!strcmp(subkey, "receivepack")) {
const char *v;
if (git_config_string(, key, value))
@@ -427,6 +433,7 @@ static int handle_config(const char *key, const char 
*value, void *cb)
return git_config_string((const char 
**)>http_proxy_authmethod,
 key, value);
} else if (!strcmp(subkey, "vcs")) {
+   remote->configured = 1;
return git_config_string(>foreign_vcs, key, value);
}
return 0;
@@ -716,7 +723,7 @@ struct remote *pushremote_get(const char *name)
 
 int remote_is_configured(struct remote *remote)
 {
-   return remote && remote->origin;
+   return remote && remote->configured;
 }
 
 int for_each_remote(each_remote_fn fn, void *priv)
diff --git a/remote.h b/remote.h
index 924881169d..7e6c8067bb 100644
--- a/remote.h
+++ b/remote.h
@@ -15,7 +15,7 @@ struct remote {
struct hashmap_entry ent;  /* must be first */
 
const char *name;
-   int origin;
+   int origin, configured;
 
const char *foreign_vcs;
 
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index d7e41e9230..09c9823002 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -764,7 +764,7 @@ test_expect_success 'rename a remote with name prefix of 
other remote' '
)
 '
 
-test_expect_failure 'rename succeeds with existing remote..prune' '
+test_expect_success 'rename succeeds with existing remote..prune' '
git clone one four.four &&
(
cd four.four &&
-- 
2.11.0.windows.3

[PATCH 1/2] remote rename: demonstrate a bogus "remote exists" bug

[Johannes Schindelin]

Some users like to set `remote.origin.prune = true` in their ~/.gitconfig
so that all of their repositories use that default.

However, our code is ill-prepared for this, mistaking that single entry to
mean that there is already a remote of the name "origin", even if there is
not.

This patch adds a test case demonstrating this issue.

Reported by Andrew Arnott.

Signed-off-by: Johannes Schindelin 
---
 t/t5505-remote.sh | 9 +
 1 file changed, 9 insertions(+)

diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 8198d8eb05..d7e41e9230 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -764,6 +764,15 @@ test_expect_success 'rename a remote with name prefix of 
other remote' '
)
 '
 
+test_expect_failure 'rename succeeds with existing remote..prune' '
+   git clone one four.four &&
+   (
+   cd four.four &&
+   git config remote.upstream.prune true &&
+   git remote rename origin upstream
+   )
+'
+
 cat >remotes_origin <

Re: [PATCH 4/6] fsck: tighten error-checks of "git fsck "

[Junio C Hamano]

Jeff King  writes:

> Instead of checking reachability from the refs, you can ask
> fsck to check from a particular set of heads. However, the
> error checking here is quite lax. In particular:
>
>   1. It claims lookup_object() will report an error, which
>  is not true. It only does a hash lookup, and the user
>  has no clue that their argument was skipped.
>
>   2. When either the name or sha1 cannot be resolved, we
>  continue to exit with a successful error code, even
>  though we didn't check what the user asked us to.
>
> This patch fixes both of these cases.
>
> Signed-off-by: Jeff King 
> ---

Makes sense, too.  Thanks.

Re: [PATCH 3/6] fsck: prepare dummy objects for --connectivity-check

[Junio C Hamano]

Jeff King  writes:

> + # Drop the index now; we want to be sure that we
> + # recursively notice that we notice the broken objects
> + # because they are reachable from refs, not because
> + # they are in the index.

Rephrase to reduce redundunt "notice"s?

Re: [PATCH 1/6] t1450: clean up sub-objects in duplicate-entry test

[Jeff King]

On Tue, Jan 17, 2017 at 12:52:43PM -0800, Junio C Hamano wrote:

> > Since the setup code happens inside a subshell, we can't
> > just set a variable for each object. However, we can stuff
> > all of the sha1s into the $T output variable, which is not
> > used for anything except cleanup.
> >
> > Signed-off-by: Jeff King 
> > ---
> >  t/t1450-fsck.sh | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Thanks.  
> 
> It is tempting to move this loop to remove_object, but that is not
> necessary while the user is only this one.

I agree it would be less gross. I avoided it because I knew that I
hacked up remove_object() in the other topic.

-Peff

Re: [PATCH 2/6] fsck: report trees as dangling

[Junio C Hamano]

Jeff King  writes:

> After checking connectivity, fsck looks through the list of
> any objects we've seen mentioned, and reports unreachable
> and un-"used" ones as dangling. However, it skips any object
> which is not marked as "parsed", as that is an object that
> we _don't_ have (but that somebody mentioned).
>
> Since 6e454b9a3 (clear parsed flag when we free tree
> buffers, 2013-06-05), that flag can't be relied on, and the
> correct method is to check the HAS_OBJ flag. The cleanup in
> that commit missed this callsite, though. As a result, we
> would generally fail to report dangling trees.
>
> We never noticed because there were no tests in this area
> (for trees or otherwise). Let's add some.
>
> Signed-off-by: Jeff King 
> ---

Makes sense, and the new test is very easy to read, too.

Queued; thanks.

Re: [PATCH 1/6] t1450: clean up sub-objects in duplicate-entry test

[Junio C Hamano]

Jeff King  writes:

> This test creates a multi-level set of trees, but its
> cleanup routine only removes the top-level tree. After the
> test finishes, the inner tree and the blob it points to
> remain, making the inner tree dangling.
>
> A later test ("cleaned up") verifies that we've removed any
> cruft and "git fsck" output is clean. This passes only
> because of a bug in git-fsck which fails to notice dangling
> trees.
>
> In preparation for fixing the bug, let's teach this earlier
> test to clean up after itself correctly. We have to remove
> the inner tree (and therefore the blob, too, which becomes
> dangling after removing that tree).
>
> Since the setup code happens inside a subshell, we can't
> just set a variable for each object. However, we can stuff
> all of the sha1s into the $T output variable, which is not
> used for anything except cleanup.
>
> Signed-off-by: Jeff King 
> ---
>  t/t1450-fsck.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Thanks.  

It is tempting to move this loop to remove_object, but that is not
necessary while the user is only this one.

>
> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> index ee7d4736d..6eef8b28e 100755
> --- a/t/t1450-fsck.sh
> +++ b/t/t1450-fsck.sh
> @@ -189,14 +189,16 @@ test_expect_success 'commit with NUL in header' '
>  '
>  
>  test_expect_success 'tree object with duplicate entries' '
> - test_when_finished "remove_object \$T" &&
> + test_when_finished "for i in \$T; do remove_object \$i; done" &&
>   T=$(
>   GIT_INDEX_FILE=test-index &&
>   export GIT_INDEX_FILE &&
>   rm -f test-index &&
>   >x &&
>   git add x &&
> + git rev-parse :x &&
>   T=$(git write-tree) &&
> + echo $T &&
>   (
>   git cat-file tree $T &&
>   git cat-file tree $T

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Jeff King]

On Tue, Jan 17, 2017 at 12:32:26PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > That was my general impression, too. But I seem to recall it was you in
> > a nearby thread saying that:
> >
> >   if (foo)
> > bar();
> >   else {
> > one();
> > two();
> >   }
> >
> > was wrong. Maybe I misunderstood.
> 
> If it were a new code written like the above, that would have been
> fine.  If a new code written with both sides inside {}, that would
> have been fine, too.
> 
> IIRC, it was that the original had {} on both, and a patch tried to
> turn that into the above, triggering "why are we churning between
> two acceptable forms?"

Ah, OK. I didn't follow that discussion closely enough to realize that.

-Peff

[PATCH] document index_name_pos

[Stefan Beller]

Signed-off-by: Stefan Beller 
---

> These placeholders are meant to encourage those people who dove into
> the code to update it, so from that point of view, I think removing
> it is backwards.

Yes, I am currently understanding and writing up documentation for
index_name_pos. If I recall the latest discussion where we want to have
documentation, I think a quorum favored documentation in the header itself,
c.f. strbuf.h, string-list.h for the most desired state. (Although we do have
Documentation/technical/api-string-list.txt as well ...)

So maybe starting like this?

Thanks,
Stefan

 cache.h | 13 +
 1 file changed, 13 insertions(+)

diff --git a/cache.h b/cache.h
index 1b67f078dd..e168e4e073 100644
--- a/cache.h
+++ b/cache.h
@@ -575,7 +575,20 @@ extern int verify_path(const char *path);
 extern int index_dir_exists(struct index_state *istate, const char *name, int 
namelen);
 extern void adjust_dirname_case(struct index_state *istate, char *name);
 extern struct cache_entry *index_file_exists(struct index_state *istate, const 
char *name, int namelen, int igncase);
+
+/*
+ * Searches for an entry defined by name and namelen in the given index.
+ * If the return value is positive (including 0) it is the position of an
+ * exact match. If the return value is negative, the negated value minus 1 is 
the
+ * position where the entry would be inserted.
+ * Example: In the current index we have the files a,c,d:
+ * index_name_pos(, "a", 1) ->  0
+ * index_name_pos(, "b", 1) -> -1
+ * index_name_pos(, "c", 1) ->  1
+ * index_name_pos(, "d", 1) ->  2
+ */
 extern int index_name_pos(const struct index_state *, const char *name, int 
namelen);
+
 #define ADD_CACHE_OK_TO_ADD 1  /* Ok to add */
 #define ADD_CACHE_OK_TO_REPLACE 2  /* Ok to replace file/directory */
 #define ADD_CACHE_SKIP_DFCHECK 4   /* Ok to skip DF conflict checks */
-- 
2.11.0.297.g298debce27

Re: [PATCH] documentation: remove unfinished documentation

[Junio C Hamano]

Stefan Beller  writes:

> When looking for documentation for a specific function, you may be tempted
> to run
>
>   git -C Documentation grep index_name_pos
>
> only to find the file technical/api-in-core-index.txt, which doesn't
> help for understanding the given function. It would be better to not find
> these functions in the documentation, such that people directly dive into
> the code instead.
>
> Signed-off-by: Stefan Beller 
> ---
>
> I run into this a couple of times now, so I put this out tentatively.

These placeholders are meant to encourage those people who dove into
the code to update it, so from that point of view, I think removing
it is backwards.

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Junio C Hamano]

Jeff King  writes:

> That was my general impression, too. But I seem to recall it was you in
> a nearby thread saying that:
>
>   if (foo)
>   bar();
>   else {
> one();
>   two();
>   }
>
> was wrong. Maybe I misunderstood.

If it were a new code written like the above, that would have been
fine.  If a new code written with both sides inside {}, that would
have been fine, too.

IIRC, it was that the original had {} on both, and a patch tried to
turn that into the above, triggering "why are we churning between
two acceptable forms?"

Re: [RFC] stash --continue

[Junio C Hamano]

Stephan Beyer  writes:

> This led to the idea to have something like "git stash --continue"[1]
> that would expect the user to "git add" the resolved files (as "git
> status" suggests) but then leads to the expected result, i.e. the index
> being the same as before the conflict, the stash being dropped (if "pop"
> was used instead of "apply"), etc.
>
> Likewise, some "git stash --abort"[2] might be useful in case you did
> "git stash pop" with the wrong stash in mind.
>
> What do you think about that?

"git stash pop --continue" (and "git stash apply --continue") would
make quite a lot of sense.  I like it very much primarily because it
will give us an opportunity to correct a major UI glitches around
applying stashed changes to the working tree.

Don't people find it strange that "stash pop" that applies cleanly
would not touch the index, leaving (an equivalent of) the changes
stashed earlier floating in the working tree, but "stash pop" that
conflicts and needs a three-way merge touches the index and the
usual way of concluding the manual conflict resolution is "git add"
the paths, meaning that the changes that were not ready hence
floating in the working tree back when the stash was made goes into
the index when the user concludes "stash pop"?

With an explicit "--continue", we can fix that so that we reset the
index to the HEAD.  That way, whether the changes have conflict with
the HEAD's tree or not, the end user after "stash pop" will see the
changes in the working tree and "git diff" (no HEAD argument or
"--cached" option) will consistently show what came from the stash.

Re: [PATCH] CodingGuidelines: clarify multi-line brace style

[Jeff King]

On Tue, Jan 17, 2017 at 11:39:31AM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> >> I think this is pretty clearly the "gray area" mentioned there. Which
> >> yes, does not say "definitely do it this way", but I hope makes it clear
> >> that you're supposed to use judgement about readability.
> >
> > So here's a patch.
> >
> > I know we've usually tried to keep this file to guidelines and not
> > rules, but clearly it has not been clear-cut enough in this instance.
> 
> I have two "Huh?" with this patch.
> 
>  1. Two exceptions are not treated the same way.  The first one is
> "... extends over a few lines, it is excempt from the rule,
> period".  The second one, is ambivalent by saying "it can make
> sense", implying that "it may not make sense", so I am not sure
> if this is clarifying that much.
> 
> If we want to clarify, perhaps drop "it can make sense to" and
> say
> 
>   When there are multiple arms to a conditional, and some of
>   them require braces, enclose even a single line block in
>   braces for consistency.
> 
> perhaps?

Yeah. I obviously was adapting the original text, and I think I left too
much of the wishy-washy-ness in. As the point of the patch is to avoid
that, let's take your suggestion. A re-rolled patch is below.

Now the patch is at least self-consistent. The bigger question remains
of: do we want to dictate these rules, or did we prefer the vague
version?

I _thought_ the vague rules were doing fine, but this whole discussion
has made me think otherwise. And I'd just as soon not ever have to
repeat it. ;-/

OTOH, I really do not want to review a bunch of patches that do nothing
but change brace style (and I am sure there is a mix of styles already
in the code base).

>  2. I actually think it is OK to leave some things "gray", but the
> confusion comes when people do not know what to do to things
> that are "gray", and they need a rule for that to be spelled
> out.
> 
>   When the project says it does not have strong preference
>   among multiple choices, you are welcome to write your new
>   code using any of them, as long as you are consistent with
>   surrounding code.  Do not change style of existing code only
>   to flip among these styles, though.
> 
> That obviously is not limited to the rule/guideline for braces.

The existing document says:

Make your code readable and sensible, and don't try to be clever.

As for more concrete guidelines, just imitate the existing code
(this is a good guideline, no matter which project you are
contributing to). It is always preferable to match the _local_
convention. New code added to Git suite is expected to match
the overall style of existing code. Modifications to existing
code is expected to match the style the surrounding code already
uses (even if it doesn't match the overall style of existing code).

But if you must have a list of rules, here they are.

I guess that is the place to expound on how to interpret the rules. I
dunno. Some of the individual rules that go into "gray areas" already
spell out the "surrounding code" rule.

-Peff

-- >8 --
Subject: [PATCH] CodingGuidelines: clarify multi-line brace style

There are some "gray areas" around when to omit braces from
a conditional or loop body. Since that seems to have
resulted in some arguments, let's be a little more clear
about our preferred style.

Signed-off-by: Jeff King 
---
 Documentation/CodingGuidelines | 37 -
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 4cd95da6b..a4191aa38 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -206,11 +206,38 @@ For C programs:
x = 1;
}
 
-   is frowned upon.  A gray area is when the statement extends
-   over a few lines, and/or you have a lengthy comment atop of
-   it.  Also, like in the Linux kernel, if there is a long list
-   of "else if" statements, it can make sense to add braces to
-   single line blocks.
+   is frowned upon. But there are a few exceptions:
+
+   - When the statement extends over a few lines (e.g., a while loop
+ with an embedded conditional, or a comment). E.g.:
+
+   while (foo) {
+   if (x)
+   one();
+   else
+   two();
+   }
+
+   if (foo) {
+   /*
+* This one requires some explanation,
+* so we're better off with braces to make
+* it obvious that the indentation is correct.
+*/
+   doit();
+   }
+
+   - When there are multiple arms to a conditional and some of them
+ 

[PATCH] documentation: remove unfinished documentation

[Stefan Beller]

When looking for documentation for a specific function, you may be tempted
to run

  git -C Documentation grep index_name_pos

only to find the file technical/api-in-core-index.txt, which doesn't
help for understanding the given function. It would be better to not find
these functions in the documentation, such that people directly dive into
the code instead.

Signed-off-by: Stefan Beller 
---

I run into this a couple of times now, so I put this out tentatively.

Thanks,
Stefan

 Documentation/technical/api-in-core-index.txt | 21 -
 1 file changed, 21 deletions(-)
 delete mode 100644 Documentation/technical/api-in-core-index.txt

diff --git a/Documentation/technical/api-in-core-index.txt 
b/Documentation/technical/api-in-core-index.txt
deleted file mode 100644
index adbdbf5d75..00
--- a/Documentation/technical/api-in-core-index.txt
+++ /dev/null
@@ -1,21 +0,0 @@
-in-core index API
-=
-
-Talk about  and , things like:
-
-* cache -> the_index macros
-* read_index()
-* write_index()
-* ie_match_stat() and ie_modified(); how they are different and when to
-  use which.
-* index_name_pos()
-* remove_index_entry_at()
-* remove_file_from_index()
-* add_file_to_index()
-* add_index_entry()
-* refresh_index()
-* discard_index()
-* cache_tree_invalidate_path()
-* cache_tree_update()
-
-(JC, Linus)
-- 
2.11.0.297.g298debce27

Re: [PATCH] Documentation/bisect: improve on (bad|new) and (good|bad)

[Junio C Hamano]

Matthieu Moy  writes:

>> But what if bad-A and bad-B have more than one merge bases?  We
>> won't know which side the badness came from.
>>
>>   o---o---o---bad-A
>>  / \ / 
>> -Good---o---o---o   / 
>>  \ / \
>>   o---o---o---bad-B
>>
>> Being able to bisect the region of DAG bound by "^Good bad-A bad-B"
>> may have value in such a case.  I dunno.
>
> I could help finding several guilty commits, but anyway you can't
> guarantee you'll find them all as soon as you use a binary search: if
> the history looks like
>
> --- Good --- Bad --- Good --- Good --- Bad --- Good --- Bad
>
> then without examining all commits, you can't tell how many good->bad
> switches occured.
>
> But keeping several bad commits wouldn't help keeping the set of
> potentially guilty commits small: bad commits appear on the positive
> side in "^Good bad-A bad-B", so having more bad commits mean having a
> larger DAG to explore (which is a bit counter-intuitive: without
> thinking about it I'd have said "more info => less commits to explore").
>
> So, if finding all guilty commits is not possible, I'm not sure how
> valuable it is to try to find several of them.

The criss-cross merge example, is not trying to find multiple
sources of badness.  It still assumes [*1*] that there is only one
event that introduced the badness observed at bad-A and bad-B, both
of which inherited the badness from the same such event.  Unlike a
case with a single/unique merge-base, we cannot say "we can start
from the merge-base, as their common badness must be coming from the
same place".  The badness may exist in the first 'o' on the same
line as bad-A in the above picture, which is an ancestor of one
merge-base on that line and does not break the other merge base on
the same line as bad-B, for example.

> OTOH, keeping several good commits is needed to find a commit for which
> all parents are good and the commit is bad.

Yes, that is correct.


[Footnote]

*1* The assumption is what makes "bisect" workable.  If the
assumption does not hold, then "bisect" would not give a useful
answer "where did I screw up?".  It gives a fairly useless "I
found one bad commit whose parent is good---there is no
guarantee if that has anything to do with the badness you are
seeing at the tip".

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Junio,
>
> On Sun, 15 Jan 2017, Junio C Hamano wrote:
>
>> * js/prepare-sequencer-more (2017-01-09) 38 commits
>
> I think that it adds confusion rather than value to specifically use a
> different branch name than I indicated in my submission, unless there is a
> really good reason to do so (which I fail to see here).

I've been meaning to rename it to match yours, for the exact reason.
The only reason was I needed my time spent to deal with other
topics, and reusing the same topic name as I used very first time
was less work.  I'll find time to update it (if you are curious, it
is not just "git branch -m").

Thanks.

> The only outstanding review comments I know about are your objection to
> the name of the read_env_script() function (which I shot down as bogus),

Not the "name", but the implementation not sharing the same code
with "am" codepath even though they originate from the same shell
function and serve the same purpose.

> and the rather real bug fix I sent out as a fixup! which you may want to
> squash in (in the alternative, I can mailbomb v4 of the entire sequencer-i
> patch series, that is your choice).

I'd rather see the "make elengant" thing redone in a more sensible
way, but I feel that it is waste of my time to help you see the
distinction between maintainable code and code that happens to work
with today's requirement, so I give up, hoping that other people
will later refactor the code that should be shared after the series
lands.  I'll squash the fixup! thing in, and as I already said a few
days ago, I do not think we'd want v4 (rather we'd want to go
incremental).

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

[Junio C Hamano]

Johannes Schindelin  writes:

>> > The original code is:
>> >
>> >. "$author_script"
>> 
>> [...]
>>
>> If the code in the sequencer.c reads things other than the three
>> variables we ourselves set, and make them into environment variables
>> and propagate to subprocesses (hooks and editors), it would be a
>> bug.  The original did not intend to do that (the dot-sourcing is
>> overly loose than reading three known variables and nothing else,
>> but is OK because we do not support the case where end users muck
>> with the file).  Also, writing FOO=BAR alone (not "export FOO=BAR"
>> or "FOO=BAR; export FOO") to the file wouldn't have exported FOO to
>> subprocesses anyway.
>
> That analysis cannot be completely correct, as the GIT_AUTHOR_* variables
> *are* used by the `git commit` subprocess.

In the scripted version, do_with_author shell function explicitly
exports GIT_AUTHOR_* variables because they are the only ones we
care about that are read from existing commit and carried forward
via the author-script mechanism.  We do not care about, and in fact
we do not intend to support, any other variables or shell commands
that appear in the author-script.

Re: [PATCH] CodingGuidelines: clarify multi-line brace style

[Junio C Hamano]

Jeff King  writes:

>> I think this is pretty clearly the "gray area" mentioned there. Which
>> yes, does not say "definitely do it this way", but I hope makes it clear
>> that you're supposed to use judgement about readability.
>
> So here's a patch.
>
> I know we've usually tried to keep this file to guidelines and not
> rules, but clearly it has not been clear-cut enough in this instance.

I have two "Huh?" with this patch.

 1. Two exceptions are not treated the same way.  The first one is
"... extends over a few lines, it is excempt from the rule,
period".  The second one, is ambivalent by saying "it can make
sense", implying that "it may not make sense", so I am not sure
if this is clarifying that much.

If we want to clarify, perhaps drop "it can make sense to" and
say

When there are multiple arms to a conditional, and some of
them require braces, enclose even a single line block in
braces for consistency.

perhaps?

 2. I actually think it is OK to leave some things "gray", but the
confusion comes when people do not know what to do to things
that are "gray", and they need a rule for that to be spelled
out.

When the project says it does not have strong preference
among multiple choices, you are welcome to write your new
code using any of them, as long as you are consistent with
surrounding code.  Do not change style of existing code only
to flip among these styles, though.

That obviously is not limited to the rule/guideline for braces.

> -- >8 --
> Subject: [PATCH] CodingGuidelines: clarify multi-line brace style
>
> There are some "gray areas" around when to omit braces from
> a conditional or loop body. Since that seems to have
> resulted in some arguments, let's be a little more clear
> about our preferred style.
>
> Signed-off-by: Jeff King 
> ---
>  Documentation/CodingGuidelines | 37 -
>  1 file changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 4cd95da6b..0e336e99d 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -206,11 +206,38 @@ For C programs:
>   x = 1;
>   }
>  
> -   is frowned upon.  A gray area is when the statement extends
> -   over a few lines, and/or you have a lengthy comment atop of
> -   it.  Also, like in the Linux kernel, if there is a long list
> -   of "else if" statements, it can make sense to add braces to
> -   single line blocks.
> +   is frowned upon. But there are a few exceptions:
> +
> + - When the statement extends over a few lines (e.g., a while loop
> +   with an embedded conditional, or a comment). E.g.:
> +
> + while (foo) {
> + if (x)
> + one();
> + else
> + two();
> + }
> +
> + if (foo) {
> + /*
> +  * This one requires some explanation,
> +  * so we're better off with braces to make
> +  * it obvious that the indentation is correct.
> +  */
> + doit();
> + }
> +
> + - When there are multiple arms to a conditional, it can make
> +   sense to add braces to single line blocks for consistency.
> +   E.g.:
> +
> + if (foo) {
> + doit();
> + } else {
> + one();
> + two();
> + three();
> + }
>  
>   - We try to avoid assignments in the condition of an "if" statement.

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Jeff King]

On Tue, Jan 17, 2017 at 11:20:58AM -0800, Junio C Hamano wrote:

> > Documentation/CodingGuidelines says:
> >
> >  - We avoid using braces unnecessarily.  I.e.
> >
> > if (bla) {
> > x = 1;
> > }
> >
> >is frowned upon.  A gray area is when the statement extends
> >over a few lines, and/or you have a lengthy comment atop of
> >it.  Also, like in the Linux kernel, if there is a long list
> >of "else if" statements, it can make sense to add braces to
> >single line blocks.
> >
> > I think this is pretty clearly the "gray area" mentioned there. Which
> > yes, does not say "definitely do it this way", but I hope makes it clear
> > that you're supposed to use judgement about readability.
> 
> I always took "gray area" to mean "we do not have strong preference
> either way, i.e.
> 
>  * It is OK for you to write your new code in either style (the
>usual "match existing style in surrounding code" applies,
>obviously);
> 
>  * It is not OK for you to churn the codebase with a patch that only
>changes existing code to flip between the two styles.

That was my general impression, too. But I seem to recall it was you in
a nearby thread saying that:

  if (foo)
bar();
  else {
one();
two();
  }

was wrong. Maybe I misunderstood.

-Peff

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Junio C Hamano]

Jeff King  writes:

> Documentation/CodingGuidelines says:
>
>  - We avoid using braces unnecessarily.  I.e.
>
> if (bla) {
> x = 1;
> }
>
>is frowned upon.  A gray area is when the statement extends
>over a few lines, and/or you have a lengthy comment atop of
>it.  Also, like in the Linux kernel, if there is a long list
>of "else if" statements, it can make sense to add braces to
>single line blocks.
>
> I think this is pretty clearly the "gray area" mentioned there. Which
> yes, does not say "definitely do it this way", but I hope makes it clear
> that you're supposed to use judgement about readability.

I always took "gray area" to mean "we do not have strong preference
either way, i.e.

 * It is OK for you to write your new code in either style (the
   usual "match existing style in surrounding code" applies,
   obviously);

 * It is not OK for you to churn the codebase with a patch that only
   changes existing code to flip between the two styles.

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Junio C Hamano]

Jeff King  writes:

> On Mon, Jan 16, 2017 at 09:33:07PM +0100, Johannes Sixt wrote:
>
>> However, Jeff's patch is intended to catch exactly these cases (not for the
>> cases where this happens accidentally, but when they happen with malicious
>> intent).
>> 
>> We are talking about user-provided data that is reproduced by die() or
>> error(). I daresay that we do not have a single case where it is intended
>> that this data is intentionally multi-lined, like a commit message. It can
>> only be an accident or malicious when it spans across lines.
>> 
>> I know we allow CR and LF in file names, but in all cases where such a name
>> appears in an error message, it is *not important* that the data is
>> reproduced exactly. On the contrary, it is usually more helpful to know that
>> something strange is going on. The question marks are a strong indication to
>> the user for this.
>
> Yes, exactly. Thanks for explaining this better than I obviously was
> doing. :)

Yup.  In the "funny filename" case, the updated code indeed is doing
the right thing.  So one remaining possible issue is if we ourselves
deliberately feed a raw CR (or any non-filtered control code) to
error() and friends because their native effect (like returning the
carriage to overwrite what we have already said with the remainder
of the message by using CR) to functions that go through vreportf()
interface.  I personally do not think there is any---the progress
meter code does use CR for that but they don't do vreportf().

I do not think we write "message\r\n" even for Windows; we rely on
vfprintf() and other stdio layer to turn "message\n" into
"message\r\n" before it hits write(2) or its equivalent?  So I
understand that this should affect LF and CRLF systems the same way
(meaning, no negative effect).

So... can we move this forward?

Re: [BUG/RFC] Git Gui: GIT_DIR leaks to spawned Git Bash

[Max Kirillov]

On Tue, Jan 17, 2017 at 11:52:29AM +0100, Johannes Schindelin wrote:
> Hi Max,
> 
> On Tue, 17 Jan 2017, Max Kirillov wrote:
> 
>> Apparently various GIT_* environment variables (most
>> interesting is GIT_DIR but AFAIR there were more) leak to
>> shell session launched from Git Gui's "Git Bash" menu item.

...

> After all, you won't even notice unless you use the very
> same Git Bash to navigate to a *different* worktree.

That's right, I haven't been noticing it myself for quite a
time. But once I decided to tweak my habits to having only
one terminal window opened, and immediately noticed.

> And having said *that*, I have to admit that I was unable to reproduce

I have found exact way to reproduce it and actually the
reason. Turns out to be obvious mistake in code. PR is in
github: https://github.com/git-for-windows/git/pull/1032

-- 
Max

Re: submodule network operations [WAS: Re: [RFC/PATCH 0/4] working tree operations: support superprefix]

[Stefan Beller]

On Sun, Jan 15, 2017 at 1:02 PM, Brian J. Davis  wrote:

>>
>> Technically it is submodule..url instead of
>> submodule..url. The name is usually the path initially, and once
>> you move the submodule, only the path changes, the name is supposed to
>> be constant and stay the same.
>
> I am not certain what is meant by this.  All I know is I can use my
> "directory_to_checkout" above to place in tree relative from root the
> project any where in the tree not already tracked by git.  You state name
> instead of path, but it allows path correct? Either that or I have gone off
> reservation with my use of git for years now. Maybe this is a deviation from
> how it is documented/should work and how it actually works?  It works great
> how I use it.

Yes name can equal the path (and usually does). This is a minor detail
that is only relevant for renaming submodules, so ... maybe let's not
focus on it too much. :)

>>>
>>>
>>> but if say I want to pull from some server 2 and do a
>>>
>>> git submodule update --init --recursive
>>
>> That is why the "git submodule init" exists at all.
>>
>>  git submodule init
>>  $EDIT .git/config # change submodule..url to server2
>>  git submodule update # that uses the adapted url and
>>  # creates the submodule repository.
>>
>>  # From now on the submodule is on its own.
>>  cd  && git config --list
>>  # prints an "origin" remote and a lot more
>>
>> For a better explanation, I started a documentation series, see
>>
>> https://github.com/gitster/git/commit/e2b51b9df618ceeff7c4ec044e20f5ce9a87241e
>>
>> (It is not finished, but that is supposed to explain this exact pain
>> point in the
>> STATES section, feel free to point out missing things or what is hard
>> to understand)
>
> I am not sure I got much out of the STATES section regarding my problem.

Your original problem as far as I understand is this:

  You have a project with submodules.
  The submodules are described in the .gitmodules file.
  But the URL is pointing to an undesired location.
  You want to rewrite the URLs before actually cloning the submodules.

And to solve this problem we need to perform multiple steps:

  # --no is the default, just for clarity here:
  git clone  --no-recurse-submodules
  # The submodules are now in the *uninitialized* state

  git submodule init
  # the submodules are in the initialized state

  git submodule update
  # submodules are populated, i.e. cloned from
  # the configured URLs and put into the working tree at
  # the appropriate path.

Between the init and the update step you can modify the URLs.
These commands are just a repetition from the first email, but the
git commands can be viewed as moving from one state to another
for submodules; submodules itself can be seen as a state machine
according to that proposed documentation. Maybe such a state machine
makes it easier to understand for some people.

>>> what I would get is from someserver1 not someserver2 as there is no
>>> "origin"
>>> support for submodules.  Is this correct?  If so can origin support be
>>> added
>>> to submodules?
>>
>> Can you explain more in detail what you mean by origin support?
>
> Yes so when we do a:
>
> git push origin master
>
> origin is of course the Remote (Remotes
> https://git-scm.com/book/en/v2/Git-Basics-Working-with-Remotes)
>
> So I best use terminology "Remotes" support.  Git push supports remotes, but
> git submodules does not.  The basic idea is this:
>
> If Git allowed multiple submodule
> (https://git-scm.com/book/en/v2/Git-Tools-Submodules) Remotes to be
> specified say as an example:
>
> git submodule add [remote] [url]
>
> git submodule add origin https://github.com/chaconinc/DbConnector
> git submodule add indhouse https://indhouse .corp/chaconinc/DbConnector
>
> Where:
>
> [submodule "DbConnector"]
> path = DbConnector
> url = https://github.com/chaconinc/DbConnector
>
> Could then change to:
>
> [submodule "DbConnector"]
> path = DbConnector
> remote.origin.url = https://github.com/chaconinc/DbConnector
> remote.origin.url = https://indhouse .corp/chaconinc/DbConnector

here I assume there is a typo and the second remote.origin.url should be
remote.inhouse.url ?

>
>
> Then it should be possible to get:
>
> git submodules update --init --recursive

which would setup the submodule with both

[remote "origin"]
  url = https://github.com/..
[remote "inhouse"]
  url = https://inhouse.corp/..

But where do we clone it from?
(Or do we just do a "git init" on that submodule and fetch
from both remotes? in which order?)

>
> To support
>
> git submodules update [remote] --init --recursive

This would just clone/fetch from the specified remote?
If implementing this, we may run into a collision with the
specified submodules, what if a submodule is at
path "origin" ?

Does "git submodule update origin --init --recursive"
then mean to update the single "origin" submodule or
all submodules from their origin 

Re: [RFC] Add support for downloading blobs on demand

[Jeff King]

This is an issue I've thought a lot about. So apologies in advance that
this response turned out a bit long. :)

On Fri, Jan 13, 2017 at 10:52:53AM -0500, Ben Peart wrote:

> Design
> ~~
> 
> Clone and fetch will pass a �--lazy-clone� flag (open to a better name 
> here) similar to �--depth� that instructs the server to only return 
> commits and trees and to ignore blobs.
> 
> Later during git operations like checkout, when a blob cannot be found
> after checking all the regular places (loose, pack, alternates, etc), 
> git will download the missing object and place it into the local object 
> store (currently as a loose object) then resume the operation.

Have you looked at the "external odb" patches I wrote a while ago, and
which Christian has been trying to resurrect?

  http://public-inbox.org/git/20161130210420.15982-1-chrisc...@tuxfamily.org/

This is a similar approach, though I pushed the policy for "how do you
get the objects" out into an external script. One advantage there is
that large objects could easily be fetched from another source entirely
(e.g., S3 or equivalent) rather than the repo itself.

The downside is that it makes things more complicated, because a push or
a fetch now involves three parties (server, client, and the alternate
object store). So questions like "do I have all the objects I need" are
hard to reason about.

If you assume that there's going to be _some_ central Git repo which has
all of the objects, you might as well fetch from there (and do it over
normal git protocols). And that simplifies things a bit, at the cost of
being less flexible.

> To prevent git from accidentally downloading all missing blobs, some git
> operations are updated to be aware of the potential for missing blobs.  
> The most obvious being check_connected which will return success as if 
> everything in the requested commits is available locally.

Actually, Git is pretty good about trying not to access blobs when it
doesn't need to. The important thing is that you know enough about the
blobs to fulfill has_sha1_file() and sha1_object_info() requests without
actually fetching the data.

So the client definitely needs to have some list of which objects exist,
and which it _could_ get if it needed to.

The one place you'd probably want to tweak things is in the diff code,
as a single "git log -Sfoo" would fault in all of the blobs.

> To minimize the impact on the server, the existing dumb HTTP protocol 
> endpoint �objects/� can be used to retrieve the individual missing
> blobs when needed.

This is going to behave badly on well-packed repositories, because there
isn't a good way to fetch a single object. The best case (which is not
implemented at all in Git) is that you grab the pack .idx, then grab
"slices" of the pack corresponding to specific objects, including
hunting down delta bases.

But then next time the server repacks, you have to throw away your .idx
file. And those can be big. The .idx for linux.git is 135MB. You really
wouldn't want to do an incremental fetch of 1MB worth of objects and
have to grab the whole .idx just to figure out which bytes you needed.

You can solve this by replacing the dumb-http server with a smart one
that actually serves up the individual objects as if they were truly
sitting on the filesystem. But then you haven't really minimized impact
on the server, and you might as well teach the smart protocols to do
blob fetches.


One big hurdle to this approach, no matter the protocol, is how you are
going to handle deltas. Right now, a git client tells the server "I have
this commit, but I want this other one". And the server knows which
objects the client has from the first, and which it needs from the
second. Moreover, it knows that it can send objects in delta form
directly from disk if the other side has the delta base.

So what happens in this system? We know we don't need to send any blobs
in a regular fetch, because the whole idea is that we only send blobs on
demand. So we wait for the client to ask us for blob A. But then what do
we send? If we send the whole blob without deltas, we're going to waste
a lot of bandwidth.

The on-disk size of all of the blobs in linux.git is ~500MB. The actual
data size is ~48GB. Some of that is from zlib, which you get even for
non-deltas. But the rest of it is from the delta compression. I don't
think it's feasible to give that up, at least not for "normal" source
repos like linux.git (more on that in a minute).

So ideally you do want to send deltas. But how do you know which objects
the other side already has, which you can use as a delta base? Sending
the list of "here are the blobs I have" doesn't scale. Just the sha1s
start to add up, especially when you are doing incremental fetches.

I think this sort of things performs a lot better when you just focus on
large objects. Because they don't tend to delta well anyway, and the
savings are much bigger by avoiding ones you don't want. So a directive
like "don't 

Re: gitk pull request // was: Re: gitk: "lime" color incompatible with older Tk versions

[Junio C Hamano]

Junio C Hamano  writes:

> Paul Mackerras  writes:
>
>>> Paul, is it a good time to pull, or do you still have something not
>>> published yet that should go together with what you have already
>>> queued?
>>
>> I recently pushed out one more commit to update the Russian
>> translation from Dimitriy Ryazantcev.  The head is now 8fef3f36b779.
>> I have a couple more series that I am currently reviewing, but nothing
>> immediately ready to publish.  It would be a good time for you to do a
>> pull, since the "lime" color fix and the memory consumption fixes
>> should be helpful for a lot of people.
>
> Thanks.  I did want to get the memory consumption fix sooner rather
> than later, and this is very much appreciated.
>
> Pulled.

Hmph.  I am getting these:

SUBDIR gitk-git
Generating catalog po/sv.msg
msgfmt --statistics --tcl po/sv.po -l sv -d po/
po/sv.po:1388: duplicate message definition...
po/sv.po:380: ...this is the location of the first definition
msgfmt: found 1 fatal error
make[1]: *** [po/sv.msg] Error 1
make: *** [all] Error 2

Anybody else see this?

Re: [PATCH v5 5/7] builtin/tag: add --format argument for tag -v

[Santiago Torres]

> VERBOSE|QUIET _does_ have a meaning, which is "show the payload, but do
> not print the signature buffer". Perhaps just renaming QUIET to
> OMIT_STATUS or something would make it more clear.
> 
Let me give this a go too. OMIT_STATUS does sound less confusing.

Thanks,
-Santiago.

> -Peff


signature.asc
Description: PGP signature

Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag

[Santiago Torres]

Yeah, this actually looks more cleaner.

Let me give it a go.

Thanks!
-Santiago.

On Tue, Jan 17, 2017 at 12:30:04PM -0500, Jeff King wrote:
> On Tue, Jan 17, 2017 at 12:25:31PM -0500, Jeff King wrote:
> 
> > Actually, looking at the callsites, I think they are fine to just call
> > pretty_print_ref() themselves, and I don't think it actually matters if
> > it happens before or after the verification.
> 
> Oh, sorry, I misread it. We do indeed early-return from the verification
> and skip the printing in that case. So it'd be more like:
> 
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 9da11e0c2..068f392b6 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -114,7 +114,11 @@ static int verify_tag(const char *name, const char *ref,
>   if (fmt_pretty)
>   flags = GPG_VERIFY_QUIET;
>  
> - return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
> + if (gpg_verify_tag(sha1, ref, flags))
> + return -1;
> +
> + pretty_print_ref(name, sha1, fmt_pretty);
> + return 0;
>  }
>  
>  static int do_sign(struct strbuf *buffer)
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index 212449f47..b3f08f705 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -58,10 +58,19 @@ int cmd_verify_tag(int argc, const char **argv, const 
> char *prefix)
>   while (i < argc) {
>   unsigned char sha1[20];
>   const char *name = argv[i++];
> - if (get_sha1(name, sha1))
> +
> + if (get_sha1(name, sha1)) {
>   had_error = !!error("tag '%s' not found.", name);
> - else if (verify_and_format_tag(sha1, name, fmt_pretty, flags))
> + continue;
> + }
> +
> + if (gpg_verify_tag(sha1, name, flags)) {
>   had_error = 1;
> + continue;
> + }
> +
> + if (fmt_pretty)
> + pretty_print_ref(name, sha1, fmt_pretty);
>   }
>   return had_error;
>  }
> 
> which I think is still an improvement (the printing, rather than being
> an obscure parameter to the overloaded verify_and_format_tag()
> function, is clearly a first-class operation).
> 
> -Peff


signature.asc
Description: PGP signature

Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag

[Jeff King]

On Tue, Jan 17, 2017 at 12:33:46PM -0500, Santiago Torres wrote:

> Yeah, this actually looks more cleaner.
> 
> Let me give it a go.

Neat. Note there's a bug:

> > diff --git a/builtin/tag.c b/builtin/tag.c
> > index 9da11e0c2..068f392b6 100644
> > --- a/builtin/tag.c
> > +++ b/builtin/tag.c
> > @@ -114,7 +114,11 @@ static int verify_tag(const char *name, const char 
> > *ref,
> > if (fmt_pretty)
> > flags = GPG_VERIFY_QUIET;
> >  
> > -   return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
> > +   if (gpg_verify_tag(sha1, ref, flags))
> > +   return -1;
> > +
> > +   pretty_print_ref(name, sha1, fmt_pretty);
> > +   return 0;

The final print should be guarded by "if (fmt_pretty)".

-Peff

Re: [PATCH v5 5/7] builtin/tag: add --format argument for tag -v

[Jeff King]

On Tue, Jan 17, 2017 at 12:00:19PM -0500, Santiago Torres wrote:

> > >  {
> > > - return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
> > > + int flags;
> > > + char *fmt_pretty = cb_data;
> > > + flags = GPG_VERIFY_VERBOSE;
> > > +
> > > + if (fmt_pretty)
> > > + flags = GPG_VERIFY_QUIET;
> > > +
> > > + return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
> > 
> > It seems funny that VERBOSE and QUIET are bit-flags. What happens when
> > you ask for GPG_VERIFY_VERBOSE|GPG_VERIFY_QUIET?
> 
> If I'm not mistaken, the way the code works right now this is not
> possible (GPG_VERIFY_VERBOSE will be unset when GPG_VERIFY_QUIET). I
> would have to re-read the patch to make sure this is the case then.

No, you're right that the two flags are mutually exclusive in the
current callers. So I don't think there's a bug here. It's just that the
interface is potentially awkward/confusing, which may be a problem down
the line.

VERBOSE|QUIET _does_ have a meaning, which is "show the payload, but do
not print the signature buffer". Perhaps just renaming QUIET to
OMIT_STATUS or something would make it more clear.

-Peff

Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag

[Jeff King]

On Tue, Jan 17, 2017 at 12:25:31PM -0500, Jeff King wrote:

> Actually, looking at the callsites, I think they are fine to just call
> pretty_print_ref() themselves, and I don't think it actually matters if
> it happens before or after the verification.

Oh, sorry, I misread it. We do indeed early-return from the verification
and skip the printing in that case. So it'd be more like:

diff --git a/builtin/tag.c b/builtin/tag.c
index 9da11e0c2..068f392b6 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -114,7 +114,11 @@ static int verify_tag(const char *name, const char *ref,
if (fmt_pretty)
flags = GPG_VERIFY_QUIET;
 
-   return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
+   if (gpg_verify_tag(sha1, ref, flags))
+   return -1;
+
+   pretty_print_ref(name, sha1, fmt_pretty);
+   return 0;
 }
 
 static int do_sign(struct strbuf *buffer)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 212449f47..b3f08f705 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -58,10 +58,19 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
while (i < argc) {
unsigned char sha1[20];
const char *name = argv[i++];
-   if (get_sha1(name, sha1))
+
+   if (get_sha1(name, sha1)) {
had_error = !!error("tag '%s' not found.", name);
-   else if (verify_and_format_tag(sha1, name, fmt_pretty, flags))
+   continue;
+   }
+
+   if (gpg_verify_tag(sha1, name, flags)) {
had_error = 1;
+   continue;
+   }
+
+   if (fmt_pretty)
+   pretty_print_ref(name, sha1, fmt_pretty);
}
return had_error;
 }

which I think is still an improvement (the printing, rather than being
an obscure parameter to the overloaded verify_and_format_tag()
function, is clearly a first-class operation).

-Peff

Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag

[Jeff King]

On Tue, Jan 17, 2017 at 11:57:25AM -0500, Santiago Torres wrote:

> > Having read through the rest of the series, it looks like you'd
> > sometimes have to do:
> 
> IIRC, we did this to make the diff "simpler". It also feeds odd that the
> call chain is the following:
> 
> verify_and_format_tag()
> gpg_verify_tag()
> run_gpg_verification()
> 
> I'm afraid that adding yet another wrapper would further convolute the
> call chain. If you think this is not an issue, I could easily do it. Do
> you have any suggested name for the wrapper?

Actually, looking at the callsites, I think they are fine to just call
pretty_print_ref() themselves, and I don't think it actually matters if
it happens before or after the verification.

So I think you could just throw out patch 3 entirely and squash these
hunks into patches 4 and 5:

diff --git a/builtin/tag.c b/builtin/tag.c
index 9da11e0c2..fab9fa8f9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -111,10 +111,12 @@ static int verify_tag(const char *name, const char *ref,
char *fmt_pretty = cb_data;
flags = GPG_VERIFY_VERBOSE;
 
-   if (fmt_pretty)
+   if (fmt_pretty) {
flags = GPG_VERIFY_QUIET;
+   pretty_print_ref(name, sha1, fmt_pretty);
+   }
 
-   return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
+   return gpg_verify_tag(sha1, ref, flags);
 }
 
 static int do_sign(struct strbuf *buffer)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 212449f47..114df1c52 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -58,9 +58,15 @@ int cmd_verify_tag(int argc, const char **argv, const char 
*prefix)
while (i < argc) {
unsigned char sha1[20];
const char *name = argv[i++];
-   if (get_sha1(name, sha1))
+
+   if (get_sha1(name, sha1)) {
had_error = !!error("tag '%s' not found.", name);
-   else if (verify_and_format_tag(sha1, name, fmt_pretty, flags))
+   continue;
+   }
+
+   if (fmt_pretty)
+   pretty_print_ref(name, sha1, fmt_pretty);
+   if (gpg_verify_tag(sha1, name, flags))
had_error = 1;
}
return had_error;

You could make the diff in the second one simpler by skipping the
"continue" and just doing the whole thing in an "else" block. But IMHO
the continue-on-error makes the logic more clear.

-Peff

Re: [PATCH v5 5/7] builtin/tag: add --format argument for tag -v

[Santiago Torres]

> >  {
> > -   return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
> > +   int flags;
> > +   char *fmt_pretty = cb_data;
> > +   flags = GPG_VERIFY_VERBOSE;
> > +
> > +   if (fmt_pretty)
> > +   flags = GPG_VERIFY_QUIET;
> > +
> > +   return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
> 
> It seems funny that VERBOSE and QUIET are bit-flags. What happens when
> you ask for GPG_VERIFY_VERBOSE|GPG_VERIFY_QUIET?

If I'm not mistaken, the way the code works right now this is not
possible (GPG_VERIFY_VERBOSE will be unset when GPG_VERIFY_QUIET). I
would have to re-read the patch to make sure this is the case then.

GPG_VERIFY_QUIET was added to suppress any VERBOSE|RAW flags, we could
defeault to QUIET if flags are not set. What do you think?

Thanks!
-Santiago


signature.asc
Description: PGP signature

Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag

[Santiago Torres]

> > Hrm. Maybe I am missing something, but what does:
> > 
> >   verify_and_format_tag(sha1, name, fmt, flags);
> > 
> > get you over:
> > 
> >   gpg_verify_tag(sha1, name, flags);
> >   pretty_print_ref(name, sha1, fmt);
> > 
> > ? The latter seems much more flexible, and I do not see how the
> > verification step impacts the printing at all (or vice versa).
> > 
> > I could understand it more if there were patches later in the series
> > that somehow used the format and verification results together. But I
> > didn't see that.
> 
> Having read through the rest of the series, it looks like you'd
> sometimes have to do:

IIRC, we did this to make the diff "simpler". It also feeds odd that the
call chain is the following:

verify_and_format_tag()
gpg_verify_tag()
run_gpg_verification()

I'm afraid that adding yet another wrapper would further convolute the
call chain. If you think this is not an issue, I could easily do it. Do
you have any suggested name for the wrapper?

Thanks!
-Santiago


signature.asc
Description: PGP signature

[PATCH v5 1/3] difftool: add a skeleton for the upcoming builtin

[Johannes Schindelin]

This adds a builtin difftool that still falls back to the legacy Perl
version, which has been renamed to `legacy-difftool`.

The idea is that the new, experimental, builtin difftool immediately hands
off to the legacy difftool for now, unless the config variable
difftool.useBuiltin is set to true.

This feature flag will be used in the upcoming Git for Windows v2.11.0
release, to allow early testers to opt-in to use the builtin difftool and
flesh out any bugs.

Signed-off-by: Johannes Schindelin 
---
 .gitignore|  1 +
 Makefile  |  3 +-
 builtin.h |  1 +
 builtin/difftool.c| 63 +++
 git-difftool.perl => git-legacy-difftool.perl |  0
 git.c |  6 +++
 t/t7800-difftool.sh   |  2 +
 7 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 builtin/difftool.c
 rename git-difftool.perl => git-legacy-difftool.perl (100%)

diff --git a/.gitignore b/.gitignore
index 6722f78f9a..ae025b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -76,6 +76,7 @@
 /git-init-db
 /git-interpret-trailers
 /git-instaweb
+/git-legacy-difftool
 /git-log
 /git-ls-files
 /git-ls-remote
diff --git a/Makefile b/Makefile
index d861bd9985..8cf5bef034 100644
--- a/Makefile
+++ b/Makefile
@@ -522,7 +522,7 @@ SCRIPT_LIB += git-sh-setup
 SCRIPT_LIB += git-sh-i18n
 
 SCRIPT_PERL += git-add--interactive.perl
-SCRIPT_PERL += git-difftool.perl
+SCRIPT_PERL += git-legacy-difftool.perl
 SCRIPT_PERL += git-archimport.perl
 SCRIPT_PERL += git-cvsexportcommit.perl
 SCRIPT_PERL += git-cvsimport.perl
@@ -883,6 +883,7 @@ BUILTIN_OBJS += builtin/diff-files.o
 BUILTIN_OBJS += builtin/diff-index.o
 BUILTIN_OBJS += builtin/diff-tree.o
 BUILTIN_OBJS += builtin/diff.o
+BUILTIN_OBJS += builtin/difftool.o
 BUILTIN_OBJS += builtin/fast-export.o
 BUILTIN_OBJS += builtin/fetch-pack.o
 BUILTIN_OBJS += builtin/fetch.o
diff --git a/builtin.h b/builtin.h
index b9122bc5f4..67f80519da 100644
--- a/builtin.h
+++ b/builtin.h
@@ -60,6 +60,7 @@ extern int cmd_diff_files(int argc, const char **argv, const 
char *prefix);
 extern int cmd_diff_index(int argc, const char **argv, const char *prefix);
 extern int cmd_diff(int argc, const char **argv, const char *prefix);
 extern int cmd_diff_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_difftool(int argc, const char **argv, const char *prefix);
 extern int cmd_fast_export(int argc, const char **argv, const char *prefix);
 extern int cmd_fetch(int argc, const char **argv, const char *prefix);
 extern int cmd_fetch_pack(int argc, const char **argv, const char *prefix);
diff --git a/builtin/difftool.c b/builtin/difftool.c
new file mode 100644
index 00..53870bbaf7
--- /dev/null
+++ b/builtin/difftool.c
@@ -0,0 +1,63 @@
+/*
+ * "git difftool" builtin command
+ *
+ * This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
+ * git-difftool--helper script.
+ *
+ * This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.
+ * The GIT_DIFF* variables are exported for use by git-difftool--helper.
+ *
+ * Any arguments that are unknown to this script are forwarded to 'git diff'.
+ *
+ * Copyright (C) 2016 Johannes Schindelin
+ */
+#include "builtin.h"
+#include "run-command.h"
+#include "exec_cmd.h"
+
+/*
+ * NEEDSWORK: this function can go once the legacy-difftool Perl script is
+ * retired.
+ *
+ * We intentionally avoid reading the config directly here, to avoid messing up
+ * the GIT_* environment variables when we need to fall back to exec()ing the
+ * Perl script.
+ */
+static int use_builtin_difftool(void) {
+   struct child_process cp = CHILD_PROCESS_INIT;
+   struct strbuf out = STRBUF_INIT;
+   int ret;
+
+   argv_array_pushl(,
+"config", "--bool", "difftool.usebuiltin", NULL);
+   cp.git_cmd = 1;
+   if (capture_command(, , 6))
+   return 0;
+   strbuf_trim();
+   ret = !strcmp("true", out.buf);
+   strbuf_release();
+   return ret;
+}
+
+int cmd_difftool(int argc, const char **argv, const char *prefix)
+{
+   /*
+* NEEDSWORK: Once the builtin difftool has been tested enough
+* and git-legacy-difftool.perl is retired to contrib/, this preamble
+* can be removed.
+*/
+   if (!use_builtin_difftool()) {
+   const char *path = mkpath("%s/git-legacy-difftool",
+ git_exec_path());
+
+   if (sane_execvp(path, (char **)argv) < 0)
+   die_errno("could not exec %s", path);
+
+   return 0;
+   }
+   prefix = setup_git_directory();
+   trace_repo_setup(prefix);
+   setup_work_tree();
+
+   die("TODO");
+}
diff --git a/git-difftool.perl b/git-legacy-difftool.perl
similarity index 100%
rename from git-difftool.perl

[PATCH v5 0/3] Turn the difftool into a builtin

[Johannes Schindelin]

This patch series converts the difftool from a Perl script into a
builtin, for three reasons:

1. Perl is really not native on Windows. Not only is there a performance
   penalty to be paid just for running Perl scripts, we also have to deal
   with the fact that users may have different Perl installations, with
   different options, and some other Perl installation may decide to set
   PERL5LIB globally, wreaking havoc with Git for Windows' Perl (which we
   have to use because almost all other Perl distributions lack the
   Subversion bindings we need for `git svn`).

2. As the Perl script uses Unix-y paths that are not native to Windows,
   the Perl interpreter has to go through a POSIX emulation layer (the
   MSYS2 runtime). This means that paths have to be converted from
   Unix-y paths to Windows-y paths (and vice versa) whenever crossing
   the POSIX emulation barrier, leading to quite possibly surprising path
   translation errors.

3. Perl makes for a rather large reason that Git for Windows' installer
   weighs in with >30MB. While one Perl script less does not relieve us
   of that burden, it is one step in the right direction.

Changes since v4:

- skipped the unrelated Coverity-appeasing patch.

- replaced the cross-validation with the Perl script by a patch that
  retires the Perl script instead.


Johannes Schindelin (3):
  difftool: add a skeleton for the upcoming builtin
  difftool: implement the functionality in the builtin
  Retire the scripted difftool

 Makefile   |   2 +-
 builtin.h  |   1 +
 builtin/difftool.c | 692 +
 .../examples/git-difftool.perl |   0
 git.c  |   1 +
 t/t7800-difftool.sh|  92 +--
 6 files changed, 741 insertions(+), 47 deletions(-)
 create mode 100644 builtin/difftool.c
 rename git-difftool.perl => contrib/examples/git-difftool.perl (100%)


base-commit: d7dffce1cebde29a0c4b309a79e4345450bf352a
Published-As: https://github.com/dscho/git/releases/tag/builtin-difftool-v5
Fetch-It-Via: git fetch https://github.com/dscho/git builtin-difftool-v5

Interdiff vs v4:

 diff --git a/.gitignore b/.gitignore
 index ae025b..6722f78f9a 100644
 --- a/.gitignore
 +++ b/.gitignore
 @@ -76,7 +76,6 @@
  /git-init-db
  /git-interpret-trailers
  /git-instaweb
 -/git-legacy-difftool
  /git-log
  /git-ls-files
  /git-ls-remote
 diff --git a/Makefile b/Makefile
 index 8cf5bef034..e9aa6ae57c 100644
 --- a/Makefile
 +++ b/Makefile
 @@ -522,7 +522,6 @@ SCRIPT_LIB += git-sh-setup
  SCRIPT_LIB += git-sh-i18n
  
  SCRIPT_PERL += git-add--interactive.perl
 -SCRIPT_PERL += git-legacy-difftool.perl
  SCRIPT_PERL += git-archimport.perl
  SCRIPT_PERL += git-cvsexportcommit.perl
  SCRIPT_PERL += git-cvsimport.perl
 diff --git a/builtin/difftool.c b/builtin/difftool.c
 index 2115e548a5..42ad9e804a 100644
 --- a/builtin/difftool.c
 +++ b/builtin/difftool.c
 @@ -616,30 +616,6 @@ static int run_file_diff(int prompt, const char *prefix,
exit(ret);
  }
  
 -/*
 - * NEEDSWORK: this function can go once the legacy-difftool Perl script is
 - * retired.
 - *
 - * We intentionally avoid reading the config directly here, to avoid messing 
up
 - * the GIT_* environment variables when we need to fall back to exec()ing the
 - * Perl script.
 - */
 -static int use_builtin_difftool(void) {
 -  struct child_process cp = CHILD_PROCESS_INIT;
 -  struct strbuf out = STRBUF_INIT;
 -  int ret;
 -
 -  argv_array_pushl(,
 -   "config", "--bool", "difftool.usebuiltin", NULL);
 -  cp.git_cmd = 1;
 -  if (capture_command(, , 6))
 -  return 0;
 -  strbuf_trim();
 -  ret = !strcmp("true", out.buf);
 -  strbuf_release();
 -  return ret;
 -}
 -
  int cmd_difftool(int argc, const char **argv, const char *prefix)
  {
int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
 @@ -671,23 +647,6 @@ int cmd_difftool(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
  
 -  /*
 -   * NEEDSWORK: Once the builtin difftool has been tested enough
 -   * and git-legacy-difftool.perl is retired to contrib/, this preamble
 -   * can be removed.
 -   */
 -  if (!use_builtin_difftool()) {
 -  const char *path = mkpath("%s/git-legacy-difftool",
 -git_exec_path());
 -
 -  if (sane_execvp(path, (char **)argv) < 0)
 -  die_errno("could not exec %s", path);
 -
 -  return 0;
 -  }
 -  prefix = setup_git_directory();
 -  trace_repo_setup(prefix);
 -  setup_work_tree();
/* NEEDSWORK: once we no longer spawn anything, remove this */
setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
setenv(GIT_WORK_TREE_ENVIRONMENT, 

[PATCH v5 2/3] difftool: implement the functionality in the builtin

[Johannes Schindelin]

This patch gives life to the skeleton added in the previous patch.

The motivation for converting the difftool is that Perl scripts are not at
all native on Windows, and that `git difftool` therefore is pretty slow on
that platform, when there is no good reason for it to be slow.

In addition, Perl does not really have access to Git's internals. That
means that any script will always have to jump through unnecessary
hoops.

The current version of the builtin difftool does not, however, make full
use of the internals but instead chooses to spawn a couple of Git
processes, still, to make for an easier conversion. There remains a lot
of room for improvement, left for a later date.

Note: to play it safe, the original difftool is still called unless the
config setting difftool.useBuiltin is set to true.

The reason: this new, experimental, builtin difftool will be shipped as
part of Git for Windows v2.11.0, to allow for easier large-scale
testing, but of course as an opt-in feature.

Sadly, the speedup is more noticable on Linux than on Windows: a quick
test shows that t7800-difftool.sh runs in (2.183s/0.052s/0.108s)
(real/user/sys) in a Linux VM, down from  (6.529s/3.112s/0.644s), while
on Windows, it is (36.064s/2.730s/7.194s), down from
(47.637s/2.407s/6.863s). The culprit is most likely the overhead
incurred from *still* having to shell out to mergetool-lib.sh and
difftool--helper.sh.

Signed-off-by: Johannes Schindelin 
---
 builtin/difftool.c | 672 -
 1 file changed, 671 insertions(+), 1 deletion(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 53870bbaf7..2115e548a5 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -11,9 +11,610 @@
  *
  * Copyright (C) 2016 Johannes Schindelin
  */
+#include "cache.h"
 #include "builtin.h"
 #include "run-command.h"
 #include "exec_cmd.h"
+#include "parse-options.h"
+#include "argv-array.h"
+#include "strbuf.h"
+#include "lockfile.h"
+#include "dir.h"
+
+static char *diff_gui_tool;
+static int trust_exit_code;
+
+static const char *const builtin_difftool_usage[] = {
+   N_("git difftool [] [ []] [--] [...]"),
+   NULL
+};
+
+static int difftool_config(const char *var, const char *value, void *cb)
+{
+   if (!strcmp(var, "diff.guitool")) {
+   diff_gui_tool = xstrdup(value);
+   return 0;
+   }
+
+   if (!strcmp(var, "difftool.trustexitcode")) {
+   trust_exit_code = git_config_bool(var, value);
+   return 0;
+   }
+
+   return git_default_config(var, value, cb);
+}
+
+static int print_tool_help(void)
+{
+   const char *argv[] = { "mergetool", "--tool-help=diff", NULL };
+   return run_command_v_opt(argv, RUN_GIT_CMD);
+}
+
+static int parse_index_info(char *p, int *mode1, int *mode2,
+   struct object_id *oid1, struct object_id *oid2,
+   char *status)
+{
+   if (*p != ':')
+   return error("expected ':', got '%c'", *p);
+   *mode1 = (int)strtol(p + 1, , 8);
+   if (*p != ' ')
+   return error("expected ' ', got '%c'", *p);
+   *mode2 = (int)strtol(p + 1, , 8);
+   if (*p != ' ')
+   return error("expected ' ', got '%c'", *p);
+   if (get_oid_hex(++p, oid1))
+   return error("expected object ID, got '%s'", p + 1);
+   p += GIT_SHA1_HEXSZ;
+   if (*p != ' ')
+   return error("expected ' ', got '%c'", *p);
+   if (get_oid_hex(++p, oid2))
+   return error("expected object ID, got '%s'", p + 1);
+   p += GIT_SHA1_HEXSZ;
+   if (*p != ' ')
+   return error("expected ' ', got '%c'", *p);
+   *status = *++p;
+   if (!*status)
+   return error("missing status");
+   if (p[1] && !isdigit(p[1]))
+   return error("unexpected trailer: '%s'", p + 1);
+   return 0;
+}
+
+/*
+ * Remove any trailing slash from $workdir
+ * before starting to avoid double slashes in symlink targets.
+ */
+static void add_path(struct strbuf *buf, size_t base_len, const char *path)
+{
+   strbuf_setlen(buf, base_len);
+   if (buf->len && buf->buf[buf->len - 1] != '/')
+   strbuf_addch(buf, '/');
+   strbuf_addstr(buf, path);
+}
+
+/*
+ * Determine whether we can simply reuse the file in the worktree.
+ */
+static int use_wt_file(const char *workdir, const char *name,
+  struct object_id *oid)
+{
+   struct strbuf buf = STRBUF_INIT;
+   struct stat st;
+   int use = 0;
+
+   strbuf_addstr(, workdir);
+   add_path(, buf.len, name);
+
+   if (!lstat(buf.buf, ) && !S_ISLNK(st.st_mode)) {
+   struct object_id wt_oid;
+   int fd = open(buf.buf, O_RDONLY);
+
+   if (fd >= 0 &&
+   !index_fd(wt_oid.hash, fd, , OBJ_BLOB, name, 0)) {
+   if (is_null_oid(oid)) {
+   

[PATCH v5 3/3] Retire the scripted difftool

[Johannes Schindelin]

It served its purpose, but now we have a builtin difftool. Time for the
Perl script to enjoy Florida.

Signed-off-by: Johannes Schindelin 
---
 .gitignore |  1 -
 Makefile   |  1 -
 builtin/difftool.c | 41 --
 .../examples/git-difftool.perl |  0
 git.c  |  7 +-
 t/t7800-difftool.sh| 94 +++---
 6 files changed, 47 insertions(+), 97 deletions(-)
 rename git-legacy-difftool.perl => contrib/examples/git-difftool.perl (100%)

diff --git a/.gitignore b/.gitignore
index ae025b..6722f78f9a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -76,7 +76,6 @@
 /git-init-db
 /git-interpret-trailers
 /git-instaweb
-/git-legacy-difftool
 /git-log
 /git-ls-files
 /git-ls-remote
diff --git a/Makefile b/Makefile
index 8cf5bef034..e9aa6ae57c 100644
--- a/Makefile
+++ b/Makefile
@@ -522,7 +522,6 @@ SCRIPT_LIB += git-sh-setup
 SCRIPT_LIB += git-sh-i18n
 
 SCRIPT_PERL += git-add--interactive.perl
-SCRIPT_PERL += git-legacy-difftool.perl
 SCRIPT_PERL += git-archimport.perl
 SCRIPT_PERL += git-cvsexportcommit.perl
 SCRIPT_PERL += git-cvsimport.perl
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 2115e548a5..42ad9e804a 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -616,30 +616,6 @@ static int run_file_diff(int prompt, const char *prefix,
exit(ret);
 }
 
-/*
- * NEEDSWORK: this function can go once the legacy-difftool Perl script is
- * retired.
- *
- * We intentionally avoid reading the config directly here, to avoid messing up
- * the GIT_* environment variables when we need to fall back to exec()ing the
- * Perl script.
- */
-static int use_builtin_difftool(void) {
-   struct child_process cp = CHILD_PROCESS_INIT;
-   struct strbuf out = STRBUF_INIT;
-   int ret;
-
-   argv_array_pushl(,
-"config", "--bool", "difftool.usebuiltin", NULL);
-   cp.git_cmd = 1;
-   if (capture_command(, , 6))
-   return 0;
-   strbuf_trim();
-   ret = !strcmp("true", out.buf);
-   strbuf_release();
-   return ret;
-}
-
 int cmd_difftool(int argc, const char **argv, const char *prefix)
 {
int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
@@ -671,23 +647,6 @@ int cmd_difftool(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
-   /*
-* NEEDSWORK: Once the builtin difftool has been tested enough
-* and git-legacy-difftool.perl is retired to contrib/, this preamble
-* can be removed.
-*/
-   if (!use_builtin_difftool()) {
-   const char *path = mkpath("%s/git-legacy-difftool",
- git_exec_path());
-
-   if (sane_execvp(path, (char **)argv) < 0)
-   die_errno("could not exec %s", path);
-
-   return 0;
-   }
-   prefix = setup_git_directory();
-   trace_repo_setup(prefix);
-   setup_work_tree();
/* NEEDSWORK: once we no longer spawn anything, remove this */
setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 
1);
diff --git a/git-legacy-difftool.perl b/contrib/examples/git-difftool.perl
similarity index 100%
rename from git-legacy-difftool.perl
rename to contrib/examples/git-difftool.perl
diff --git a/git.c b/git.c
index c58181e5ef..bd4d668a21 100644
--- a/git.c
+++ b/git.c
@@ -424,12 +424,7 @@ static struct cmd_struct commands[] = {
{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
{ "diff-index", cmd_diff_index, RUN_SETUP },
{ "diff-tree", cmd_diff_tree, RUN_SETUP },
-   /*
-* NEEDSWORK: Once the redirection to git-legacy-difftool.perl in
-* builtin/difftool.c has been removed, this entry should be changed to
-* RUN_SETUP | NEED_WORK_TREE
-*/
-   { "difftool", cmd_difftool },
+   { "difftool", cmd_difftool, RUN_SETUP | NEED_WORK_TREE },
{ "fast-export", cmd_fast_export, RUN_SETUP },
{ "fetch", cmd_fetch, RUN_SETUP },
{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index e94910c563..aa0ef02597 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -23,10 +23,8 @@ prompt_given ()
test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
 }
 
-# NEEDSWORK: lose all the PERL prereqs once legacy-difftool is retired.
-
 # Create a file on master and change it on branch
-test_expect_success PERL 'setup' '
+test_expect_success 'setup' '
echo master >file &&
git add file &&
git commit -m "added file" &&
@@ -38,7 +36,7 @@ test_expect_success PERL 'setup' '
 '
 
 # Configure a custom difftool..cmd and use it

Re: fatal: bad revision 'git rm -r --ignore-unmatch -- folder'

[Jeff King]

On Tue, Jan 17, 2017 at 04:30:48PM +0100, jean-christophe manciot wrote:

> I'm trying to purge a complete folder and its files from the
> repository history with:
> 
> git-Games# git filter-branch 'git rm -r --ignore-unmatch --
> Ubuntu/16.04/' --tag-name-filter cat -- --all HEAD
> fatal: bad revision 'git rm -r --ignore-unmatch -- Ubuntu/16.04/'

Did you forget "--tree-filter" or "--index-filter" before the "git rm"
parameter? Without an option it will be interpreted as a refname, which
it obviously isn't.

-Peff

Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag

[Jeff King]

On Tue, Jan 17, 2017 at 10:24:55AM -0500, Jeff King wrote:

> On Sun, Jan 15, 2017 at 01:47:01PM -0500, santi...@nyu.edu wrote:
> 
> > From: Lukas Puehringer 
> > 
> > Calling functions for gpg_verify_tag() may desire to print relevant
> > information about the header for further verification. Add an optional
> > format argument to print any desired information after GPG verification.
> 
> Hrm. Maybe I am missing something, but what does:
> 
>   verify_and_format_tag(sha1, name, fmt, flags);
> 
> get you over:
> 
>   gpg_verify_tag(sha1, name, flags);
>   pretty_print_ref(name, sha1, fmt);
> 
> ? The latter seems much more flexible, and I do not see how the
> verification step impacts the printing at all (or vice versa).
> 
> I could understand it more if there were patches later in the series
> that somehow used the format and verification results together. But I
> didn't see that.

Having read through the rest of the series, it looks like you'd
sometimes have to do:

  int ret;

  ret = gpg_verify_tag(sha1, name, flags);
  pretty_print_ref(name, sha1, fmt);
  if (ret)
  ... do something ...

and this function lets you do it in a single line.

Still, I think I'd rather see it done as a wrapper than modifying
gpg_verify_tag().

-Peff

Re: [PATCH v5 0/7] Add --format to tag verification

[Jeff King]

On Sun, Jan 15, 2017 at 01:46:58PM -0500, santi...@nyu.edu wrote:

> From: Santiago Torres 
> 
> This is the fifth iteration of [1][2][3][4], and as a result of the
> discussion in [5]. The main goal of this patch series is to bring
> --format to git tag verification so that upper-layer tools can inspect
> the content of a tag and make decisions based on it.

Thanks for picking this back up.  I didn't see any bugs, but I had a few
interface nits, which I sent inline.

-Peff

Re: [PATCH v5 7/7] t/t7004-tag: Add --format specifier tests

[Jeff King]

On Sun, Jan 15, 2017 at 01:47:05PM -0500, santi...@nyu.edu wrote:

> From: Santiago Torres 
> 
> tag -v now supports --format specifiers to inspect the contents of a tag
> upon verification. Add two tests to ensure this behavior is respected in
> future changes.
> 
> Signed-off-by: Santiago Torres 
> ---
>  t/t7004-tag.sh | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 07869b0c0..b2b81f203 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -874,6 +874,22 @@ test_expect_success GPG 'verifying a forged tag should 
> fail' '
>   test_must_fail git tag -v forged-tag
>  '
>  
> +test_expect_success 'verifying a proper tag with --format pass and format 
> accordingly' '
> + cat >expect <<-\EOF 

Trailing whitespace after "EOF" here (and below). Otherwise the tests
look reasonable to me.

-Peff

Re: [PATCH v5 5/7] builtin/tag: add --format argument for tag -v

[Jeff King]

On Sun, Jan 15, 2017 at 01:47:03PM -0500, santi...@nyu.edu wrote:

> -static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
> +static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
> + void *cb_data)
>  {
>   const char **p;
>   char ref[PATH_MAX];
>   int had_error = 0;
>   unsigned char sha1[20];
>  
> +
>   for (p = argv; *p; p++) {
>   if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p)
>   >= sizeof(ref)) {

Funny extra line?

>  {
> - return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
> + int flags;
> + char *fmt_pretty = cb_data;
> + flags = GPG_VERIFY_VERBOSE;
> +
> + if (fmt_pretty)
> + flags = GPG_VERIFY_QUIET;
> +
> + return verify_and_format_tag(sha1, ref, fmt_pretty, flags);

It seems funny that VERBOSE and QUIET are bit-flags. What happens when
you ask for GPG_VERIFY_VERBOSE|GPG_VERIFY_QUIET?

I suppose this is actually not a problem in _this_ patch, but in the
very first one that adds the QUIET flag. I'm not sure if the problem is
that the options should be more orthogonal, or that they are just badly
named to appear as opposites when they aren't.

-Peff

fatal: bad revision 'git rm -r --ignore-unmatch -- folder'

[jean-christophe manciot]

Hi there,

I'm trying to purge a complete folder and its files from the
repository history with:

git-Games# git filter-branch 'git rm -r --ignore-unmatch --
Ubuntu/16.04/' --tag-name-filter cat -- --all HEAD
fatal: bad revision 'git rm -r --ignore-unmatch -- Ubuntu/16.04/'

git does not find the folder although it's there:

git-Games# ll Ubuntu/16.04/
total 150316
drwxr-x--- 2 actionmystique actionmystique 4096 Nov 19 13:40 ./
drwxr-x--- 4 actionmystique actionmystique 4096 Oct 30 14:02 ../
-rwxr-x--- 1 actionmystique actionmystique  2190394 May  9  2016
residualvm_0.3.0~git-1_amd64.deb*
...
-rw-r--r-- 1 actionmystique actionmystique 67382492 Oct 13 09:15
scummvm-dbgsym_1.9.0_amd64.deb

What is going on?

-- 
Jean-Christophe

Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag

[Jeff King]

On Sun, Jan 15, 2017 at 01:47:01PM -0500, santi...@nyu.edu wrote:

> From: Lukas Puehringer 
> 
> Calling functions for gpg_verify_tag() may desire to print relevant
> information about the header for further verification. Add an optional
> format argument to print any desired information after GPG verification.

Hrm. Maybe I am missing something, but what does:

  verify_and_format_tag(sha1, name, fmt, flags);

get you over:

  gpg_verify_tag(sha1, name, flags);
  pretty_print_ref(name, sha1, fmt);

? The latter seems much more flexible, and I do not see how the
verification step impacts the printing at all (or vice versa).

I could understand it more if there were patches later in the series
that somehow used the format and verification results together. But I
didn't see that.

-Peff

Re: [RFH - Tcl/Tk] use of procedure before declaration?

[Johannes Schindelin]

Hi Philip,

On Mon, 16 Jan 2017, Philip Oakley wrote:

> In
> https://github.com/git/git/blob/master/git-gui/lib/choose_repository.tcl#L242
> the procedure `_unset_recentrepo` is called, however the procedure isn't
> declared until line 248. My reading of the various Tcl tutorials suggest
> (but not explictly) that this isn't the right way.

Indeed, calling a procedure before it is declared sounds incorrect.

Since documentation can be treacherous, let's just test it. With a `tclsh`
whose `$tcl_version` variable claims that this is version 8.6, this
script:

```tcl
hello Philip

proc hello {arg} {
puts "Hi, $arg"
}
```

... yields the error message:

invalid command name "hello"
while executing
"hello Philip"

... while this script:

```tcl
proc hello {arg} {
puts "Hi, $arg"
}

hello Philip
```

... prints the expected "Hi, Philip".

Having said that, in the code to which you linked, the procedure is not
actually called before it is declared, as the call is inside another
procedure.

Indeed, the entire file declares one object-oriented class, so no code
gets executed in that file:

https://github.com/git/git/blob/d7dffce1c/git-gui/lib/choose_repository.tcl#L4

(I guess proper indentation would make it easier to understand that this
file is defining a class, not executing anything yet).

And it is perfectly legitimate to use not-yet-declared procedures in other
procedures, otherwise recursion would not work.

> Should 3c6a287 ("git-gui: Keep repo_config(gui.recentrepos) and .gitconfig
> in sync", 2010-01-23) have declared `proc _unset_recentrepo {p}` before
> `proc _get_recentrepos {}` ?

Given the findings above, I believe that the patch is actually correct.

Ciao,
Dscho